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

Add mechanism to get blockhash's last valid slot #10239

Conversation

CriesofCarrots
Copy link
Contributor

Problem

It can be crucial for clients to know if a particular blockhash has expired. getFeeCalculatorForBlockhash offers one way to test this, but it would be a lot easier to know the expiration slot concretely.

re: #10054 (comment)

Summary of Changes

  • Add new getRecentBlockhashLastValidSlot rpc endpoint that returns a blockhash with its last valid slot, and expose last-valid-slot in RpcClient::get_recent_blockhash_with_commitment

I've left application of last_valid_slot in various solana client crates, like solana-tokens, solana-bench-tps, or solana-cli ping, for separate prs. In particular, solana-bench-tps would require exposing last_valid_slot in ThinClient (perhaps via the Client trait), and so deserves additional consideration.

Replaces #10162 and #10237

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #10239 into master will decrease coverage by 0.1%.
The diff coverage is 86.3%.

@@            Coverage Diff            @@
##           master   #10239     +/-   ##
=========================================
- Coverage    81.5%    81.3%   -0.2%     
=========================================
  Files         288      288             
  Lines       66608    66665     +57     
=========================================
- Hits        54303    54263     -40     
- Misses      12305    12402     +97     

{"jsonrpc":"2.0","result":{"context":{"slot":1},"value":{"blockhash":"CSymwgTNX1j3E4qhKfJAUE41nBWEwXufoYryPbkde5RR","feeCalculator":{"lamportsPerSignature":5000}}},"id":1}
```

### getRecentBlockhashLastValidSlot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of a name like getFees? No change in functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm definitely up for a shorter name, although I'm not sure getFees really describes what you're getting. I'll sleep on it...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought there is that a blockhash and its expiration slot are both necessary means of qualifying the fee parameters. That is, if you send a signed transaction including the given blockhash by the given slot, the transaction fee is guaranteed to be determined by the given fee parameters. Deterministic blockhash expiration turns out to be a bonus feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes some sense, and I didn't think of anything better overnight. (I was toying with getRecentFees for a bit, but I think I mostly liked it because it would land next to getRecentBlockhash in the docs.) I'll push getFees and see how it looks

garious
garious previously approved these changes May 26, 2020
@mergify mergify bot dismissed garious’s stale review May 26, 2020 17:35

Pull request has been modified.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label May 26, 2020
@solana-grimes solana-grimes merged commit 4e431bc into solana-labs:master May 26, 2020
mergify bot pushed a commit that referenced this pull request May 26, 2020
@CriesofCarrots CriesofCarrots deleted the rpc-get-blockhash-last-valid-slot-2 branch May 26, 2020 23:29
CriesofCarrots added a commit to CriesofCarrots/solana that referenced this pull request May 29, 2020
danpaul000 pushed a commit to danpaul000/solana that referenced this pull request Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants