fix(merkle): descend using build_tree's midpoint in collect_proof#6
Merged
Merged
Conversation
build_tree splits leaves at ceil(n/2), but collect_proof descended using floor((start + end)/2). The two disagree whenever a subtree holds an odd number of leaves, so proof collection walked into the wrong child and emitted a path authenticating a neighbouring leaf rather than the target. The result: MerkleTree::generate_proof only returned verifiable proofs when the leaf count was an exact power of two. For any other count most leaves produced proofs that fail MerkleProof::verify -- with 3 leaves, indices 0 and 1 both fail; with 9 leaves, 8 of 9 fail. The existing test_odd_number_of_leaves built a 3-leaf tree but asserted only on index 2, the single index that happens to survive the mismatch, so the bug went unnoticed. Fix collect_proof to mirror build_tree's ceil split. Merkle roots are byte-identical before and after (verified for n = 1..=9), so block validation and consensus are unaffected -- only proof generation changes. Also drop the unreachable "duplicate last if odd" branch in build_tree: mid < hashes.len() always holds for len >= 2, so no leaf was ever duplicated. Document the ceil split and the coupling between the two functions. Tests: cover every leaf count 1..=33 for every leaf index, and assert a proof cannot be replayed against a different leaf.
Contributor
|
Thank you for your contribution! |
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.
Description
MerkleTree::generate_proofonly returns verifiable proofs when the number of leaves is an exact power of two. For every other leaf count, most leaves produce a proof that failsMerkleProof::verify.build_treesplits leaves atceil(n / 2):but
collect_proofdescends usingfloor((start + end) / 2):The two disagree whenever a subtree holds an odd number of leaves. Proof collection then walks into the wrong child and emits a path that authenticates a neighbouring leaf rather than the requested one.
Failing leaf indices with the current code:
1, 2, 4 and 8 leaves pass only because
ceil == floorfor powers of two.The existing
test_odd_number_of_leavesbuilds a 3-leaf tree but asserts only on index2— the one index that happens to survive the mismatch — which is why this went unnoticed.Type of Change
Motivation and Context
generate_proof/MerkleProofare the SPV surface of thequantalibrary. Any light client built on them would reject valid inclusion proofs for essentially every real block, since blocks rarely contain exactly 2^k transactions.This is not a consensus issue. Merkle roots are computed by
build_tree, which is internally self-consistent and used identically byBlock::new,Block::validateandBlockchain. Only proof generation is wrong. I verified roots are byte-identical before and after this change for n = 1..=9, so block validation and chain state are untouched.How Has This Been Tested?
mainand pass with the fix.cargo test --lib→ 94 passed, 0 failed (existing block/genesis/consensus tests unchanged).root_hash()for n = 1..=9 with and without the patch, diffed, identical.cargo clippy --libandcargo fmt --checkare clean forsrc/core/merkle.rs.New coverage:
test_proofs_verify_for_every_leaf_count— every leaf index for every leaf count1..=33.test_proof_does_not_verify_for_wrong_leaf— a proof cannot be replayed against a different leaf. (This one is a direct regression guard: onmainthe proof generated for leaf 0 of a 3-leaf tree actually verifies leaf 1.)test_odd_number_of_leaves— now asserts on all three leaves, not just the last.Secondary cleanup
build_tree'selsebranch was unreachable dead code:mid = ceil(len/2) < lenholds for everylen >= 2, andlen == 1returns early, so the duplicate-last path never executed and no leaf was ever duplicated. I removed it and documented the actual invariant. Worth noting this is a good property: because leaves are never duplicated, the tree is not susceptible to the CVE-2012-2459 style root collision. The old comment implied the opposite scheme.Note for maintainers (not addressed here, happy to file separately)
hash_to_byteswill panic on malformed input —hex::decode(...).expect("Invalid hex hash")andcopy_from_slice(&bytes[..32])on a short slice. It is reachable from the publicgenerate_proof_hexandverify_hex. Internal callers always pass well-formed 64-char hashes so nothing panics today, but returningOption<Hash>would harden the public API. I kept this PR focused rather than folding that in.Checklist
cargo test)build_tree/collect_proof)