block: compute merkle roots, with mutation detection (stacked on #395) - #399
Open
xanimo wants to merge 5 commits into
Open
block: compute merkle roots, with mutation detection (stacked on #395)#399xanimo wants to merge 5 commits into
xanimo wants to merge 5 commits into
Conversation
dogecoin_block_header carried an auxpow member holding only the validation hook -- check, ctx and is -- never the proof itself. The proof was built into a local dogecoin_auxpow_block inside dogecoin_block_header_deserialize and freed at cleanup, so by the time the caller had its header the parent coinbase, merkle branches and parent header were gone. dogecoin_block_header_copy made that worse by looking complete: it copied the three hook fields, so a copied merge-mined header was silently missing its proof with nothing to indicate it. Anything needing the proof after the parse therefore had to re-parse the wire bytes, or retain them separately. That is why BIP152 keeps a header_raw span on dogecoin_compact_block: not because a compact block wants raw bytes, but because the parsed header could not answer for itself. Add dogecoin_auxpow_payload, owned by the header. The deserializer moves the parsed fields onto it and nulls them on the scratch block, so ownership transfers rather than duplicating; _free releases it; _copy deep-copies it. The payload deliberately has no back-pointer to its header, unlike dogecoin_auxpow_block, which owns both its header and its parent_header and frees them. A header holding one of those would own the thing that owns it. The payload owns only parent_header, a plain 80-byte header whose own payload is NULL, so ownership terminates. auxpow.check / auxpow.ctx are untouched. That is a validation hook whose context the caller supplies at call time -- validation.c passes the block directly -- not a reference to this data. dogecoin_block_header_copy assigns the copied payload rather than freeing what dest held. Every other field there is a plain overwrite: the function treats dest as raw memory, and callers pass uninitialised stack headers to it -- net_tests.c does, through dogecoin_block_header_deserialize. Freeing dest->auxpow_payload dereferenced whatever the stack contained, which is a SEGV on eight platforms and the reason ASAN caught this and a local build did not: every local caller happened to use dogecoin_block_header_new, where the pointer is NULL. A dest that already owns a payload is the caller's to release, as with every other member. The test extends the height-371338 vector: the proof survives the parse, a copy owns an independent one, and freeing the source leaves the copy intact, which it can only do if nothing is shared. Disabling just the deep copy fails it at line 351. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 78/78. WITH_NET=OFF: 72/72. ASAN+UBSAN: 78/78, no leaks.
check_auxpow ran inside dogecoin_block_header_deserialize, so every caller that wanted a header's fields paid for scrypt work over the parent chain during parsing, before any peer-level gating could decide whether the message was worth the effort. Core defers this to CheckBlock. Split into dogecoin_block_header_parse, which reads the base fields and, when version bit 0x100 is set, the AuxPoW proof, and dogecoin_block_header_validate, which runs check_auxpow and fills chainwork. The split is deliberately this way round. Making dogecoin_block_header_ deserialize the pure parse and adding a _checked variant would silently stop verifying proof of work for every existing caller of the name, with no compile error anywhere to catch it. Wrong direction for a symbol whose job is validation. Instead the existing name keeps its signature and its behaviour -- it is now parse followed by validate -- and the opt-out is explicit at the call site. deserialize_dogecoin_auxpow_block is split the same way and keeps its signature: parse_dogecoin_auxpow_fields does the reading, the public function adds the check. Validation needs the proof to still exist after parsing, which is what the preceding commit made possible. check_auxpow takes a dogecoin_auxpow_block, so validate builds one that borrows from the header and its payload. It is never freed: dogecoin_auxpow_block_free would take the header and parent_header with it, which is the ownership tangle the payload type exists to avoid. A header with no AuxPoW validates trivially. Its proof of work is over the 80 base bytes and belongs to the caller -- headersdb_file.c already runs check_pow itself for that case and fills chainwork. That asymmetry is unchanged. Verified by stubbing check_auxpow to always fail: dogecoin_block_header_ parse still succeeds, dogecoin_block_header_validate does not. A refactor that merely moved the call would fail both. The test also asserts the deferred chainwork equals what the parse-and-validate path computes, so the split does not change the answer. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 78/78. WITH_NET=OFF: 72/72. ASAN+UBSAN with leak detection: 78/78, clean.
There was no way to write an AuxPoW proof back out. The tree could parse one and, since the preceding commits, retain it, but nothing could emit it, so a header that arrived over the wire could not be reproduced. Add dogecoin_auxpow_payload_serialize, in the wire order parse_dogecoin_auxpow_fields reads, and dogecoin_block_header_serialize_full, which writes the 80 base bytes and then the proof when the header carries one. dogecoin_block_header_serialize is untouched and still emits exactly 80 bytes. That is deliberate: its output is what the block hash, the scrypt proof of work, check_auxpow's own hashing and the fixed-width headers.db record are computed over. Making it AuxPoW-aware would change all four. This is the same split Core draws between CPureBlockHeader and CBlockHeader, and the same reason. The full form keys off whether the proof is present rather than off version bit 0x100 alone, so a header carrying the bit without a proof serializes as the 80 bytes it actually has instead of emitting a truncated blob. Tested by round-tripping the height-371338 mainnet vector: parse it, write it back, and require the result to be byte-identical to the bytes it came from. Emitting something merely well-formed is not enough -- the point of this is that short IDs and hashes computed over the output match Core's, which only holds if the bytes match exactly. The pure form is asserted to stay at 80 bytes and to match the first 80 of the input. The first attempt at checking that test had no bite: it swapped parent_merkle_index with aux_merkle_index, which are both zero in this block, so the output was identical and the test passed either way. Zeroing parent_hash instead fails it at the memcmp, which is what a real serialization bug would do. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 78/78. WITH_NET=OFF: 72/72. ASAN+UBSAN with leak detection: 78/78, clean.
The tree could serialize a header and it could serialize a transaction, but nothing could serialize a block. Anything holding a header and a set of transactions -- a block assembled locally, or one reconstructed from a compact block -- had no way to produce the bytes the rest of the client parses, because every path that consumes a block takes wire bytes and deserializes them. dogecoin_block_serialize writes the header in wire form, so AuxPoW and all, then the transaction vector. It uses the full header serializer rather than the pure one for that reason: a block carries the header a peer sent, not the 80 bytes the block hash is computed over. It stops rather than emitting a short block if the transaction array contains a NULL. A vector with a hole in it is a reconstruction that did not finish, and a block that is well-formed but missing transactions is worse than no output at all. Tested by round-tripping the whole height-371338 mainnet block: parse the header span, parse the transaction vector, assert the vector accounts for every remaining byte, serialize it all back, and require byte-identity with the input. Omitting the transaction count alone fails it at the memcmp. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 78/78. WITH_NET=OFF: 72/72. ASAN+UBSAN with leak detection: 78/78, clean.
libdogecoin could verify a merkle branch (check_merkle_branch) but could not compute a root. Anything assembling a block -- a pool building an AuxPoW candidate, a test harness, anything checking a header against its own transactions -- had no way to produce the value the header commits to. dogecoin_compute_merkle_root reduces pre-hashed leaves; dogecoin_block_merkle_root hashes a transaction vector and reduces it. This is a port of Core's MerkleComputation (consensus/merkle.cpp), kept in its eager inner[] form rather than rewritten as the textbook loop, so the two can be compared line by line. The textbook version -- duplicate the last hash on an odd level, hash pairwise, repeat -- computes the same roots for well-formed input and diverges on exactly the case that matters. That case is CVE-2012-2459. An odd leaf count leaves the last leaf unpaired and the tree self-pairs it as hash(L,L). An attacker appends a copy of that leaf, making the count even, so the pair (L,L) now forms explicitly and produces the *same root* from a different transaction list. The root cannot distinguish them. Core detects it by noticing a node combined with a value equal to itself and reporting `mutated`; a caller that ignores that flag accepts the forged block. Verified against the height-371338 mainnet vector: the root computed from its six transactions equals the one in its own header, and reports no mutation. Ground truth from the chain rather than a fixture of our own. The mutation test builds the attack rather than asserting a flag: take five of the six transactions for an odd count, append a copy of the fifth, and assert both that the forged root *equals* the honest one and that only the flag separates them. Disabling the check alone fails it. An earlier version of that test duplicated into a count of seven and saw no mutation. That was correct behaviour -- with seven leaves the copy sits unpaired and is never combined with its twin -- but the test had been written expecting detection, so it reported a failure that was really its own. Worth recording, because a mutation test that never forms the duplicated pair looks like coverage and is not. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 78/78. WITH_NET=OFF: 72/72. ASAN+UBSAN with leak detection: 78/78, clean.
xanimo
force-pushed
the
0.1.5-dev-merkle-root
branch
from
August 5, 2026 21:03
6773c5c to
a766d99
Compare
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.
Stacked on #395 (→ #394 → #393 → #392). Review only the top commit.
libdogecoin could verify a merkle branch (
check_merkle_branch) but could notcompute a root. Anything assembling a block — a pool building an AuxPoW
candidate, a test harness, anything checking a header against its own
transactions — had no way to produce the value the header commits to.
dogecoin_compute_merkle_root— reduces pre-hashed leavesdogecoin_block_merkle_root— hashes a transaction vector and reducesA port, deliberately
This keeps Core's
MerkleComputation(consensus/merkle.cpp) in its eagerinner[]form rather than rewriting it as the textbook loop, so the two can becompared line by line.
The textbook version — duplicate the last hash on an odd level, hash pairwise,
repeat — computes the same roots for well-formed input and diverges on exactly
the case that matters.
That case is CVE-2012-2459
An odd leaf count leaves the last leaf unpaired, and the tree self-pairs it as
hash(L,L). An attacker appends a copy of that leaf, making the count even, sothe pair
(L,L)now forms explicitly — producing the same root from adifferent transaction list. The root cannot distinguish them.
Core detects it by noticing a node combined with a value equal to itself and
reporting
mutated. A caller that ignores that flag accepts the forged block,which is why it is documented on the API rather than left as an out-param
detail.
Tests
Ground truth from the chain. The root computed from block 371338's six
transactions equals the one in its own header, and reports no mutation. Not a
fixture of our own choosing.
The mutation test builds the attack rather than asserting a flag: take five
of the six transactions for an odd count, append a copy of the fifth, then
assert both that the forged root equals the honest one and that only the
flag separates them. Disabling the mutation check alone fails it.
Worth recording: an earlier version duplicated into a count of seven and saw
no mutation. That was correct behaviour — with seven leaves the copy sits
unpaired and is never combined with its twin — but the test expected detection,
so it reported a failure that was really its own. A mutation test that never
forms the duplicated pair looks like coverage and isn't.
WITH_NET=ONwith-DBUILD_SHARED_LIBS=1: 78/78.WITH_NET=OFF: 72/72.ASAN+UBSAN with leak detection: 78/78, clean.
Scope
This is a primitive, not a step toward block template assembly.
CreateNewBlockneeds a mempool, UTXO set and fee estimation — that is a node, and out of scope
for this library. Merkle root computation is useful to anyone assembling or
checking a block regardless.