fix(rpc): use consistent chain-state snapshots in getblock, getblockheader, gettxout#10606
Open
zmanian wants to merge 3 commits into
Open
fix(rpc): use consistent chain-state snapshots in getblock, getblockheader, gettxout#10606zmanian wants to merge 3 commits into
zmanian wants to merge 3 commits into
Conversation
…eader, gettxout Several RPC methods issue multiple sequential ReadStateService queries and combine the results into a single response. Each oneshot call independently samples the latest best chain, so a reorg or tip advance between queries can produce internally inconsistent responses. PR ZcashFoundation#10523 fixed this for getrawtransaction; this commit applies the same pattern to the three remaining sibling sites. - getblock (verbosity 1 and 2): build the transaction-fetch ReadRequest after shadowing hash_or_height with the resolved hash, so the transactions are read from the same chain view as the header. - getblockheader (verbose): pass the resolved hash to the SaplingTree query so it is taken from the same chain view as the BlockHeader/Depth reads. - gettxout: drop the separate Tip query and reuse the best-chain tip hash captured in the Transaction read's snapshot (new MinedTx.best_chain_tip_hash field), so the response's bestblock anchor matches the chain view used to compute confirmations. Also hardens the getblock verbosity-2 path so the confirmations = -1 case (block no longer on the best chain) returns a normal RPC response labeled as not in the active chain, instead of panicking on a u32 try_into during transaction serialization. Closes ZcashFoundation#10550 AI disclosure: drafted with Claude Code; author reviewed and is responsible for the change.
…elog Address Codex review recommendation on PR ZcashFoundation#10606.
Contributor
Author
|
Codex code review verdict: APPROVE. One recommendation surfaced — added a |
…apshots # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Closes #10550.
Several RPC methods issue multiple sequential
ReadStateServicequeries and combine the results into a single response. Eachoneshotcall independently samples the latest best chain, so a reorg or tip advance between queries can produce internally inconsistent responses. PR #10523 fixed this forgetrawtransaction; this PR addresses the three remaining sibling sites called out in #10550 (getblock,getblockheader,gettxout), and also folds in the verbosity-2 panic reported in the issue by @sangsoo-osec (via @mpguerra).Solution
getblock(verbosity 1 and 2): construct the transaction-fetchReadRequestafter shadowinghash_or_heightwith the resolved hash, so theTransactionIdsForBlock/BlockAndSizelookup is taken from the same chain view as the header. Header/orchard/block-info already used the resolved hash; only the transaction read was racy.getblockheader(verbose): pass the resolvedhashto theSaplingTreelookup. TheDepthread was already pinned tohash;SaplingTreewas still using the caller-suppliedhash_or_height.gettxout: extendMinedTxwith abest_chain_tip_hashfield captured in the sametip(chain, db)call used to computeconfirmations.get_tx_outnow drops the separateTipquery entirely and uses that hash as the response'sbestblock.getblockverbosity-2 transaction-serialization loop, replace theconfirmations.try_into().expect(...)(which panicked whenconfirmations = -1) withu32::try_from(...).ok()and passSome(confirmations >= 0)forin_active_chain. The atomic-read fix prevents this state from being produced in normal flow, but a graceful RPC error is the right floor.Tests
rpc_getblock_verbosity_2_side_chain_no_panicdrives the verbosity-2 path withDepth = None(i.e.confirmations = -1) and asserts that:get_blockreturns a normal response instead of panicking,height = -1andconfirmations = 0(the existing zcashd-compatible "side chain" shape).rpc_getblock,rpc_getblockheader,rpc_gettxout, andzebra-statepopulated-state tests still pass (MinedTx's new field is set from the same chain-snapshottip(...)lookup that already computedconfirmations).All clean.
Linked issue
confirmations = -1panic reported by @sangsoo-osec via @mpguerra's comment on that issue.AI disclosure
Drafted with Claude Code (RPC method edits,
MinedTxfield addition, regression test scaffolding). The author reviewed each change and is the responsible author.