Skip to content

fix(consensus): score misbehavior for blocks with duplicate transactions - #11157

Open
jiehuo100net wants to merge 4 commits into
ZcashFoundation:mainfrom
jiehuo100net:fix/10688-duplicate-transaction-misbehavior-score
Open

fix(consensus): score misbehavior for blocks with duplicate transactions#11157
jiehuo100net wants to merge 4 commits into
ZcashFoundation:mainfrom
jiehuo100net:fix/10688-duplicate-transaction-misbehavior-score

Conversation

@jiehuo100net

Copy link
Copy Markdown
Contributor

Motivation

Closes #10688.

BlockError::misbehavior_score() returns 0 for DuplicateTransaction: the variant is not in the
explicit scoring arms and falls through to the _other => 0 catch-all. The syncer only forwards a
score to the misbehaviour channel when it is non-zero, so a peer that advertises a block containing
duplicate transactions is not scored at all, while every other definitive block-validity violation
scores 100.

A block with duplicate transaction hashes is the Merkle malleability case from CVE-2012-2459:
duplicating the trailing transaction of an odd-length list can reproduce the honest block's Merkle
root, which is why merkle_root_validity() checks for duplicates right after the BadMerkleRoot
check. An honest node never produces such a block, so it is exactly the kind of unambiguous signal
the 100-point arms are for.

Both verifiers reach that check — the semantic verifier in block.rs, and the checkpoint verifier
before it queues a block — so the gap applies to the whole sync range, not only to full verification
near the tip.

A scoring arm consistent with that policy already exists, but it is unreachable on the current error
path:
VerifyCheckpointError::DuplicateTransaction scores 100, yet that variant is never constructed
anywhere in the repository. The error from merkle_root_validity() reaches VerifyCheckpointError
through From<BlockError>, which wraps it as VerifyBlock(..) and forwards the score straight back
to BlockError::misbehavior_score() — so the 100 in that arm is dead today.

Solution

Add DuplicateTransaction to the 100-point arm, as the issue's "Suggested fix" section proposes.

The issue also asks for the rest of the match to be audited rather than fixing this one arm. All 14
BlockError variants, on dcf12fe84:

  • 9 already score 100: MissingHeight, MaxHeight, InvalidDifficulty, TargetDifficultyLimit,
    DifficultyFilter, NoTransactions, BadMerkleRoot, WrongTransactionConsensusBranchId,
    TooManyTransparentSignatureOperations
  • Transaction(err) forwards to TransactionError::mempool_misbehavior_score()
  • 4 fall through to _other => 0:
    • DuplicateTransaction — scored here
    • AlreadyInChain — correctly 0: it is a duplicate request, and is_duplicate_request() relies on
      that classification
    • Other(String) — correctly 0 as an unclassified catch-all
    • SummingMinerFees — left unchanged, but it looks inconsistent, see below

SummingMinerFees wraps the amount::Error from accumulating the block's miner fees, and scores 0.
The summed total is then passed to check::miner_fees_are_valid(), and related overflows in that
accounting path are converted to SubsidyError::Overflow, which scores 100 via
Transaction(Subsidy(_)); impl From<amount::Error> for BlockError routes an amount::Error the
same way. So amount-arithmetic failures on a peer-provided block score 0 or 100 depending on which
call site produced them. That is a behaviour decision rather than a completeness bug, so this PR does
not change it — happy to follow up if you want it scored.

Two things deliberately not touched:

  • The unreachable VerifyCheckpointError::DuplicateTransaction and
    VerifyCheckpointError::BadMerkleRoot variants. Removing a variant from a public enum is a
    breaking change, and it is unrelated to the scoring gap.
  • #10616. Until the inbound gossip path downcasts to the error type it actually receives, it
    forwards no score at all, so this fix takes effect on the syncer path only.

Tests

zebra-consensus/src/block/tests.rs:

  • block_error_misbehavior_scores — one assertion added for the bare BlockError value.
  • duplicate_transaction_scores_misbehavior_through_both_verifier_paths — builds a block with
    duplicate transaction hashes the same way the existing
    merkle_root_validity_rejects_duplicate_transaction_hash test does (duplicate coinbase
    transaction, Merkle root recomputed from the duplicated list so the block passes the
    BadMerkleRoot check and reaches the duplicate check), takes the real error out of
    merkle_root_validity(), wraps that BlockError exactly as the semantic and checkpoint paths do,
    and asserts the score is 100 after each wrapper: VerifyBlockError then RouterError for the
    semantic path, and VerifyCheckpointError then RouterError for the checkpoint path. It does not
    start either verifier service; it exercises the wrapper chain the syncer reads the score from,
    because asserting only on the bare BlockError would not show that the score survives to the
    channel.

Gates run locally on this branch, in debug:

cargo fmt --all -- --check                              clean
cargo clippy --workspace --all-targets -- -D warnings   0 warnings
cargo test -p zebra-consensus --lib                     133 passed; 0 failed

Control: with the one-line change reverted and the tests kept, both
block_error_misbehavior_scores and
duplicate_transaction_scores_misbehavior_through_both_verifier_paths fail (0 where 100 is
expected). So the new assertions are bound to this change rather than passing for an unrelated
reason.

Specifications & References

Follow-up Work

  • SummingMinerFees (above): score it, or document why the two call sites differ.
  • VerifyCheckpointError::DuplicateTransaction (declared, scored 100, never constructed) and
    VerifyCheckpointError::BadMerkleRoot (declared, never constructed, and not in the scoring arms
    at all).

AI Disclosure

  • No AI tools were used in this PR
  • AI tools were used: Claude Code, for the test code and for drafting this description. The
    change, the audit, the assertions and the local test evidence were reviewed and verified by me.

PR Checklist

  • The PR title follows conventional commits format: type(scope): description
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

`BlockError::misbehavior_score()` returned 0 for `DuplicateTransaction`,
because that variant was not in the explicit scoring arms and fell through
to the `_other => 0` catch-all.

A block with duplicate transaction hashes is the CVE-2012-2459
Merkle-malleability case: it is unambiguously invalid, and honest nodes
never produce one. Both verifiers run `merkle_root_validity()`, so a peer
could advertise such a block on either path without being scored.

Score it as definitive misbehavior, alongside the other unambiguous
block-validity violations.
…-transaction-misbehavior-score

# Conflicts:
#	zebra-consensus/CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BlockError::DuplicateTransaction scores zero misbehavior

1 participant