Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Agave v2 RPC: replace getRecentBlockhash with getLatestBlockhash #3419

Open
wants to merge 2 commits into
base: 10-23-feat_agave_v2_rpc_replace_getconfirmedtransaction_with_gettransaction_
Choose a base branch
from

Conversation

buffalojoec
Copy link
Collaborator

@buffalojoec buffalojoec commented Oct 23, 2024

This is a bad idea...

Copy link

changeset-bot bot commented Oct 23, 2024

⚠️ No Changeset found

Latest commit: fff4e5a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Collaborator Author

buffalojoec commented Oct 23, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@buffalojoec buffalojoec force-pushed the 10-23-feat_agave_v2_rpc_replace_getconfirmedtransaction_with_gettransaction_ branch from 950dba5 to ee67085 Compare October 29, 2024 07:05
@buffalojoec buffalojoec force-pushed the 10-23-feat_agave_v2_rpc_replace_getrecentblockhash_with_getlatestblockhash_ branch from b1112a4 to 4e4ba0c Compare October 29, 2024 07:05
@buffalojoec buffalojoec marked this pull request as ready for review October 29, 2024 07:05
@buffalojoec buffalojoec marked this pull request as draft October 29, 2024 07:18
@buffalojoec buffalojoec force-pushed the 10-23-feat_agave_v2_rpc_replace_getconfirmedtransaction_with_gettransaction_ branch from ee67085 to f96e4d2 Compare October 29, 2024 09:54
@buffalojoec buffalojoec force-pushed the 10-23-feat_agave_v2_rpc_replace_getrecentblockhash_with_getlatestblockhash_ branch 3 times, most recently from ea25c54 to 02c228c Compare November 1, 2024 13:09
@mcintyre94
Copy link
Collaborator

What's our semver deal here, are we committed to backward compatibility across 1.x? I'd like to just remove the deprecated method TBH, but maybe we can't do that in a 1.x release. Failing that I'd prefer to just hardcode 5000, but not too worried either way.

@buffalojoec
Copy link
Collaborator Author

What's our semver deal here, are we committed to backward compatibility across 1.x? I'd like to just remove the deprecated method TBH, but maybe we can't do that in a 1.x release. Failing that I'd prefer to just hardcode 5000, but not too worried either way.

We can't just remove them, even if they are deprecated. I can change this to be hard-coded, but we'd lose out on the fact that fees are supposed to change under varying network load.
https://github.com/anza-xyz/agave/blob/96249691b4b7c873220b27376f271ead38392541/sdk/fee-calculator/src/lib.rs#L109

This might not be the prettiest solution, but it does capture fee rate changes with accuracy.

It's worth noting that the probability of someone using this field - on a long-deprecated method no less - is quite low.

@steveluscher what do you think here?

@steveluscher
Copy link
Collaborator

OK, hear me out. How absolutely nuts would it be to:

  1. Deploy a program to mainnet/testnet/devnet that gave you the fee-per-signature as returnData
  2. Create a message that calls that program, having no blockhash
  3. Use simulateTransaction with that message, configured with replaceRecentBlockhash set to true
  4. Pull out the replacementBlockhash and returnData from that simulation

That way you could get this all done in a single call to the server.

@buffalojoec
Copy link
Collaborator Author

  1. Deploy a program to mainnet/testnet/devnet that gave you the fee-per-signature as returnData

How does a program get this info?

@steveluscher
Copy link
Collaborator

How does a program get this info?

I was hoping you'd know better than me. This?

@buffalojoec
Copy link
Collaborator Author

buffalojoec commented Nov 13, 2024

How does a program get this info?

I was hoping you'd know better than me. This?

Actually I was tempted to use this and just load it from account data, but it's deprecated. So we can officially kick the can down the road and just load from a deprecated sysvar for now, and probably avoid the requirement for an on-chain program altogether.

https://github.com/anza-xyz/agave/blob/32b677bf4f2b4a718f62683e1daec87a3a83b83d/sdk/program/src/sysvar/fees.rs#L42
https://github.com/anza-xyz/agave/blob/32b677bf4f2b4a718f62683e1daec87a3a83b83d/sdk/fee-calculator/src/lib.rs#L23

This sysvar is still updated per-slot by Bank, so it would totally work (for now).

It would still be two calls, one to getLatestBlockhash and one to getAccountInfo to load the Fees sysvar and decode the lamports per signature. At least we wouldn't have to craft and serialize a message, though.

@steveluscher
Copy link
Collaborator

It would still be two calls…

Well, the difference with that approach is that you could parallelize the calls, so that's much, much better than the implementation with serial calls. Go for it.

@buffalojoec buffalojoec force-pushed the 10-23-feat_agave_v2_rpc_replace_getrecentblockhash_with_getlatestblockhash_ branch from 02c228c to c250727 Compare November 14, 2024 14:19
@buffalojoec buffalojoec marked this pull request as ready for review November 14, 2024 14:19
@buffalojoec
Copy link
Collaborator Author

@steveluscher the feature to disable the fees sysvar (JAN1trEUEtZjgXYzNBYHU9DYd7GnThhXfFP7SzPXkPsG) has been activated on all clusters.

The only way to get it properly loaded into the test validator is the hackery that is my second commit. The test validator's genesis config overwrites the account fixture if the feature is enabled.

If we still want to go this route, we just have to be aware that the sysvar's account itself hasn't been cleaned up on any higher clusters, but it's not being updated anymore.

@steveluscher
Copy link
Collaborator

Oof. I'm not sure who we're building this for anymore.

  • The person who can't just switch to getFeeForMessage()
  • …but who is willing to upgrade all the way to the latest 1.x version

Surely that's nobody, right?

At this point I'm tempted to just:

return {
  context,
  value: {
    blockhash,
    feeCalculator: {
      get lamportsPerSignature() {
        throw new Error(
          'The capability to fetch `lamportsPerSignature` using the `getRecentBlockhash` API is ' +
          'no longer offered by the network. Use the `getFeeForMessage` API to obtain the fee ' +
          'for a given message.',
        );
      }
    },
  },
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants