RPC sendTransaction: be more liberal in determining and setting last_valid_block_height for skip_preflight case #483
Conversation
… for last_valid_block_height and sanitization
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #483 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 842 842
Lines 228436 228439 +3
=========================================
- Hits 187063 187053 -10
- Misses 41373 41386 +13 |
|
I put up a PR for the JavaScript clients that does what this PR is doing, but from the client side. Until this PR is deployed widely, the client-side patch should provide some relief in the meantime. solana-labs/solana-web3.js#2415 |
| let preflight_commitment = | ||
| preflight_commitment.map(|commitment| CommitmentConfig { commitment }); | ||
| let preflight_commitment = if skip_preflight { | ||
| Some(CommitmentConfig::processed()) |
There was a problem hiding this comment.
on second thought, would confirmed be more reliable here? more likely to pick up ephemeral forks with processed
There was a problem hiding this comment.
I can't see a drawback to picking up an ephemeral fork here. This is only being used to look for the recent_blockhash to determine last_valid_block_height. If the blockhash is from a confirmed or finalized block, it should appear in the ancestor tree of the fork.
There was a problem hiding this comment.
But I will hold off merging until you reply, in case you've thought of something I missed. @t-nelson
There was a problem hiding this comment.
was thinking something like...
C - 1' - 2' - 3'(STS)
\ 1 - 2(TX) - 3
There was a problem hiding this comment.
I mean, the bigger problem for the tx using a processed blockhash in that example is really that the transaction won't ever land if the leaders are on the same fork-prime as STS.
However, STS will still retry the transaction the fixed number of blocks (300) as per the logic at L3668 (edit) with this patch
There was a problem hiding this comment.
...I'm going to merge this as-is, but there's no big risk in tweaking this later.
There was a problem hiding this comment.
I'll similarly land solana-labs/solana-web3.js#2415. If you change your mind on the commitment, let's make the change in the client too!
…eflight checks (#2415) # Summary There's a bug in the `sendTransaction` RPC method where, when bypassing preflight, we nevertheless materially use the value of `preflightCommitment` to determine how to behave when sending the transaction. If you supply _nothing_ – as you might think appropriate when _skipping_ preflight – then the default of `finalized` will be used. Far from irrelevant, such a value can actually affect the retry behaviour of the send-transaction-service (STS). Read anza-xyz/agave#483 for more detail. In this PR, we try to get ahead of anza-xyz/agave#483 by setting this value to `processed` in the client. Until the server PR is deployed widely, this should cover those who choose to upgrade Addresses anza-xyz/agave#479 # Test plan ``` pnpm turbo test:unit:node test:unit:browser ```
|
Has there been a discussion about backporting this to 1.17 and 1.18? It seems like it's a small fix that would be really useful to RPCs |
Not much discussion, no. In my opinion, this isn't so much a bug fix, per se, as it is extending how and when SendTransactionService retries transactions. |
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
…g last_valid_block_height for skip_preflight case (backport of #483) (#873) RPC sendTransaction: be more liberal in determining and setting last_valid_block_height for skip_preflight case (#483) * RPC sendTransaction: if skip_preflight, use processed-commitment Bank for last_valid_block_height and sanitization * Use nonce retry logic for skip_preflight transactions if blockhash was not found (cherry picked from commit 65a24d5) Co-authored-by: Tyera <[email protected]>
…g last_valid_block_height for skip_preflight case (backport of anza-xyz#483) (anza-xyz#873) RPC sendTransaction: be more liberal in determining and setting last_valid_block_height for skip_preflight case (anza-xyz#483) * RPC sendTransaction: if skip_preflight, use processed-commitment Bank for last_valid_block_height and sanitization * Use nonce retry logic for skip_preflight transactions if blockhash was not found (cherry picked from commit 65a24d5) Co-authored-by: Tyera <[email protected]>
This PR removes the temporary request transformer that was waiting for [this PR](anza-xyz/agave#483) to be merged and available on mainnet-beta.
Problem
Transactions submitted to RPC
sendTransactionwithskipPreflight: truefrequently aren't retried by the SendTransactionService (STS). This happens when the transaction's recent blockhash is newer than the RPC node's finalized bank, because unlesspreflightCommitmentis speficied, the RPC handler uses the default finalized bank to find alast_valid_block_heightto provide to the STS. If the blockhash cannot be found (as when too new),last_valid_block_heightis set to0, and the STS only broadcasts the transaction once.Summary of Changes
skip_preflightis set, use theprocessedcommitment Bank to search for the recent blockhash.last_valid_block_heightstill cannot be determined, use the durable-nonce logic to retry the transaction for an arbitrary, fixed number of blocks (skip_preflightbeing an indication that a client wants their transactions to be sprayed around regardless of likelihood of success).Fixes #479