refactor(consensus)!: split transaction verifier into BlockVerifier and MempoolVerifier - #11095
Conversation
25b624e to
7ccd86a
Compare
…rom zebra-consensus
|
Thanks syszery, we'll get this reviewed! |
conradoplg
left a comment
conradoplg
left a comment
There was a problem hiding this comment.
Thank you. This looks good. Claude found some stuff to improve which I agree with (including item 5; the suggested rename looks better), would you mind changing those?
Findings
- BlockRequest.known_outpoint_hashes is a dead public field —
transaction.rs:177
Nothing in BlockVerifier::call reads it, and block.rs:294-295 builds a HashSet
over every created outpoint per block solely to populate it. It's residue of
the mempool-cache fast path (find_verified_unmined_tx) deleted when
CVE-2026-34377 was fixed. Since this PR is defining a brand-new public type,
this is the moment to drop the field and the per-block set construction —
leaving it invites someone to re-wire a cache bypass into the block path.
- BlockResponse.sigops doc contradicts the value it holds —
transaction.rs:224-226
Doc says "The number of legacy signature operations." The constructed value is
sigops.saturating_add(cached_ffi_transaction.p2sh_sigops()) (:369), i.e.
legacy + P2SH — which is the point of GHSA-jv4h-j224-23cc, and zebra-chain's
own p2sh_sigop_count doc says it "must be added to legacy_sigop_count for the
block-level MAX_BLOCK_SIGOPS check." The wrong text was carried over verbatim
from the old enum variant (renamed from legacy_sigop_count to sigops in a
prior PR without updating the doc). This value feeds MAX_BLOCK_SIGOPS, so a
misleading doc here is worth fixing in a branch that already has a docs
commit.
- Stale test docs describe machinery that no longer exists — tests.rs:4250,
4320, 4330, 4386
block_with_garbage_orchard_proofs_is_rejected (the CVE-2026-34377 regression
test) lost its mempool mock, but its doc still says "even if the mempool has a
valid version of the same transaction" — a scenario the test no longer sets
up. And mempool_cached_result_bypasses_expiry_check_for_block_at_next_height
still carries ~20 lines of doc plus an inline comment about
find_verified_unmined_tx and "the cache hit fires immediately" — that function
exists nowhere in the tree.
To be fair to the change: this is not a coverage regression. The mempool arm
was already vacuous on main — the mock was driven from an unjoined
tokio::spawn, so its expect_request(...).unwrap() panic could never fail the
test. And the refactor replaces the guarantee with a stronger structural one.
But the docs now overstate what's tested, in a consensus-critical crate, and
this PR touches exactly these tests.
- Changelog entry is under-specified for a library-consumer breaking change —
zebra-consensus/CHANGELOG.md:12-15
Per book/src/dev/changelog-guidelines.md ("Name specific types, traits,
functions affected", "Focus on what code changes are needed"), the entry
should name the removed items (transaction::Verifier, transaction::Request,
transaction::Response and their accessors) and — most importantly for
consumers like Zaino/Zallet — state that the second return value of
router::init/init_test is now a mempool-only verifier that can no longer serve
block requests. The guidelines' both-breaking-and-additive rule also calls
for an ### Added entry for the new types. (Line 16 is a whitespace-only line;
it passes MD009's br_spaces: 2 allowance, so it's cosmetic only, not a CI
failure.)
- transaction::BlockVerifier collides with an existing name family — worth a
maintainer decision
zebra-consensus already has BlockVerifierRouter, SemanticBlockVerifier, and
router::service_trait::BlockVerifierService, all of which verify whole blocks.
transaction::BlockVerifier verifies a transaction in block context. Fully
qualified it reads fine, but a bare use
zebra_consensus::transaction::BlockVerifier in a file that also touches block
verification will be actively confusing. BlockTxVerifier/MempoolTxVerifier
would disambiguate.
- Minor
- BlockRequest.transaction_hash is also unread — call recomputes
tx.unmined_id(). The old Request::tx_id() carried a // TODO: get the
precalculated ID from the block verifier that was deleted in the refactor
without the field becoming used. block.rs already computes all tx hashes for
the merkle root, so wiring it is a cheap win (note unmined_id() != hash() for
v5, so it can only supply the mined_id half). - zebra-chain/src/transaction/tests/vectors.rs:265,289 still reference
zebra_consensus::transaction::Verifier. Plain prose, not an intra-doc link, so
no CI failure. - mempool::Request::TransactionWithDepsByMinedId /
Response::TransactionWithDeps (zebra-node-services/src/mempool.rs:67,151) now
have a handler at zebrad/src/components/mempool.rs:891 but no sender anywhere.
Dead since the CVE fix; this PR removes the last test that even pretended to
exercise it. Out of scope here, but worth a follow-up issue so a cache-shaped
hole doesn't linger in a public API.
…eld, fix sigops docs
|
|
Thanks @conradoplg for cleaning up the test docs and for implementing the I've updated the root On the new |
…refactor/split-transaction-verifier-types
Merge Queue Status
This pull request spent 30 minutes 40 seconds in the queue, including 29 minutes 44 seconds running CI. Required conditions to merge
|
Link the split transaction verifier API to PR #11095 so consumers can find its design and implementation context.
Link the removed unified verifier API to PR #11095 so consumers can find its migration and review context.
Link the `BlockRequest::transaction_hash` contract to PR #11095 so consumers can find its implementation and rationale.
Link the new transaction verifier types to PR #11095 so consumers can find their design and implementation context.
Link the dedicated verifier request and response types to PR #11095 so consumers can find their implementation context.
Link the split transaction verifier API to PR #11095 so consumers can find its design and implementation context.
Link the removed unified verifier API to PR #11095 so consumers can find its migration and review context.
Link the `BlockRequest::transaction_hash` contract to PR #11095 so consumers can find its implementation and rationale.
Link the new transaction verifier types to PR #11095 so consumers can find their design and implementation context.
Link the dedicated verifier request and response types to PR #11095 so consumers can find their implementation context.
Link the split transaction verifier API to PR #11095 so consumers can find its design and implementation context.
Link the removed unified verifier API to PR #11095 so consumers can find its migration and review context.
Link the `BlockRequest::transaction_hash` contract to PR #11095 so consumers can find its implementation and rationale.
Link the new transaction verifier types to PR #11095 so consumers can find their design and implementation context.
Link the dedicated verifier request and response types to PR #11095 so consumers can find their implementation context.
5 participants
Motivation
Follow-up to #10843, closing #10950.
#10131 asked for the transaction verifier's
tower::Serviceimplementation to be split by request variant. After #10843, the verifier still exposes a singleVerifier<ZS, Mempool>with oneRequest/Responseenum pair and a singleServiceimpl that dispatches on the request variant at runtime. This PR introduces two separate types:BlockVerifier<ZS>andMempoolVerifier<ZS, Mempool>.Solution
ZS/Mempoolinto free functions. No behavior change intended; these helpers were already logically decoupled fromRequestandself.BlockRequest/BlockResponseMempoolRequest/MempoolResponseBlockVerifier<ZS>MempoolVerifier<ZS, Mempool>zebra-consensusto use the split verifier types end-to-end:zebra-consensus/src/router.rszebra-consensus/src/block.rszebradcallers to use the split verifier types end-to-end:zebrad/src/components/mempool/downloads.rszebrad/src/components/mempool.rszebra-consensusandzebradto useBlockVerifier/MempoolVerifierdirectly.transaction::Verifiertransaction::Requesttransaction::Responsezebra-consensus/CHANGELOG.mdentry for the breaking API change.Reviewer question
Two former legacy tests in
zebra-consensus/src/transaction/tests.rsoriginally covered block-vs-mempool cross-context regressions (expiry-cache bypass and garbage Orchard proof handling):mempool_cached_result_bypasses_expiry_check_for_block_at_next_heightblock_with_garbage_orchard_proofs_is_rejectedAfter the verifier split,
BlockVerifierno longer depends on mempool state directly, so that setup no longer exists in the same form. I kept direct block-verification coverage where the invariant still exists, and removed legacy scaffolding that no longer participates in the code path.Tests
cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo nextest runall pass locally for the current commit. No behavior change intended.AI Disclosure
PR Checklist
type(scope): description