refactor(consensus): separate block and mempool transaction verification#10843
Open
syszery wants to merge 4 commits into
Open
refactor(consensus): separate block and mempool transaction verification#10843syszery wants to merge 4 commits into
syszery wants to merge 4 commits into
Conversation
…fy_mempool helpers
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is an internal refactor of the Zcash transaction verifier in zebra-consensus. It splits the single Service::call implementation of Verifier<ZS, Mempool> — which previously interleaved block and mempool logic and branched on req.is_mempool() throughout one async block — into two dedicated private methods, verify_block and verify_mempool. It makes progress toward #10131 (eventually splitting the verifier into two service implementations) without changing the public Verifier type, request/response API, or verification behavior.
Changes:
call()now dispatches by request variant toverify_block/verify_mempool, each containing only its path's logic (block uses block time +AwaitUtxo; mempool uses median-time-past,UnspentBestChainUtxo+AwaitOutput, maturity check, and the nullifier/anchor check).spent_utxosis split intoblock_spent_utxos(known_utxos +AwaitUtxo) andmempool_spent_utxos(best-chain + mempool fallback).check_maturity_heightnow takes explicittx/heightarguments instead of&Request, hardcoding an emptyknown_utxosmap (equivalent for the mempool-only call site).
Review notes (per Zebra guidelines)
- NITPICK: A relocated TODO in
verify_mempoolstill references the oldspent_utxos()function name (nowmempool_spent_utxos()). - Process: This touches consensus-critical verification. Behavior appears faithfully preserved (check ordering, coinbase handling, block vs. mempool lock-time, omission of maturity/anchor checks in the block path, mempool UTXO fallback, and
spent_mempool_outpointspropagation all match the prior logic). The PR description's "discussed in an issue/with the team" and "changelog/docs up to date" checkboxes are unchecked; for a behavior-preserving internal refactor a changelog entry is likely unnecessary, but the consensus surface warrants careful human verification.
|
|
||
| // TODO: `spent_outputs` may not align with `tx.inputs()` when a transaction | ||
| // spends both chain and mempool UTXOs (mempool outputs are appended last by | ||
| // `spent_utxos()`), causing policy checks to pair the wrong input with |
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
This PR makes progress toward #10131 by separating the block and mempool transaction verification paths inside
Verifier<ZS, Mempool>.The existing
Service::call()implementation interleaved block and mempool verification logic throughout a single async block, branching onreq.is_mempool()at multiple points. Separating the two paths makes each one easier to follow independently and should make any future architectural split simpler.Solution
This PR consists of three related refactors:
Split
spent_utxosintoblock_spent_utxosandmempool_spent_utxosSplit
Service::callintoverify_blockandverify_mempoolcall()now dispatches immediately to one of two private methods. Each method contains only the logic for its respective verification path, eliminating the internalis_mempool()branching. Shared validation steps remain the same, while block- and mempool-specific behavior is handled directly within each method.Replace
&Requestincheck_maturity_heightwith explicit argumentscheck_maturity_heightpreviously accepted&Requestonly to extract the transaction, height, andknown_utxos. Since the function is only used during mempool verification, whereRequest::known_utxos()always returns an empty map, it now accepts only the values it actually needs (tx,height, andspent_utxos).These refactors are intended to preserve the existing verification behavior while making the two verification paths easier to understand and modify independently.
Tests
Follow-up Work
This PR intentionally keeps the existing
Verifier<ZS, Mempool>type and public API unchanged. With the verification logic now isolated intoverify_blockandverify_mempool, a future refactor could split these into separate verifier types if that is still the desired direction for #10131.Question for reviewers: Is the intended resolution of #10131 to introduce separate verifier types, or is the current separation of the internal verification paths sufficient? I'm happy to continue with a follow-up refactor—or extend this PR—depending on the preferred direction.
AI Disclosure
PR Checklist
type(scope): description