fix: BUMP txid leaf misplaced in Path[0] and nil deref in CalculateRootGivenTxid#89
Open
tngdomingo wants to merge 2 commits into
Open
fix: BUMP txid leaf misplaced in Path[0] and nil deref in CalculateRootGivenTxid#89tngdomingo wants to merge 2 commits into
tngdomingo wants to merge 2 commits into
Conversation
…otGivenTxid NewBUMPFromMerkleTreeAndIndex decided the txid leaf's position in Path[0] from an odd-offset flag accumulated across all tree levels instead of the leaf level's parity. For a transaction at the last (even) index of a block with an odd transaction count, Path[0] came out as [duplicate, txid]. CalculateRootGivenTxid then dereferenced the duplicate leaf's nil Hash while searching for the txid and panicked. The nil check also protects against previously produced malformed BUMPs. Adds an exhaustive regression test covering every block size up to 33 transactions and every transaction position. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
CalculateRootGivenTxid resolves leaves by offset, so the exhaustive test alone cannot detect a misordered Path[0]. Assert ascending offset order at every level, plus a focused test pinning the exact bug shape: 11-tx block, tx at index 10 must produce Path[0] = [txid, duplicate]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Bugs
1.
NewBUMPFromMerkleTreeAndIndexmisplaces the txid leaf inPath[0].The position of the txid leaf (before or after its sibling) is decided by an
oddTxIndexflag that is set whenever the offset at any tree level is odd, instead of only the leaf level's parity. For a transaction at the last (even) index of a block with an odd transaction count,Path[0]comes out as[duplicate, txid]— the padding leaf first.2.
CalculateRootGivenTxidpanics on duplicate leaves.While searching
Path[0]for the txid it dereferences each leaf'sHashwithout a nil check. A duplicate leaf has a nilHash, so any BUMP shaped as above (or any malformed BUMP with a duplicate leaf first) causes a nil-pointer panic.Reproduction
Any block with an odd number of transactions, requesting the BUMP for the transaction at the last (even) index — e.g. an 11-tx block, tx index 10. Construction misorders the path; verification then segfaults. Found in production: ARC (Teranode) block processing crash-looped on such blocks, and the malformed BUMPs it had already stored crashed downstream consumers on parse-and-verify.
Fixes
oddTxIndexfromtxIndex&1before the level loop (leaf-level parity only).l.Hashin the txid search loop — this also protects consumers verifying previously-produced malformed BUMPs.Test
TestNewBUMPFromMerkleTreeEverySizeAndIndex: every block size up to 33 transactions × every transaction index, each BUMP verified viaCalculateRootGivenTxidagainst the root fromBuildMerkleTreeStoreChainHash. Panics with the nil deref on current master; passes with the fixes. Full existing suite passes unchanged.🤖 Generated with Claude Code