Skip to content

consensus: document merkle mutation root invariant#35161

Open
l0rinc wants to merge 1 commit intobitcoin:masterfrom
l0rinc:l0rinc/doc-merkle-root-mutated
Open

consensus: document merkle mutation root invariant#35161
l0rinc wants to merge 1 commit intobitcoin:masterfrom
l0rinc:l0rinc/doc-merkle-root-mutated

Conversation

@l0rinc
Copy link
Copy Markdown
Contributor

@l0rinc l0rinc commented Apr 26, 2026

Problem

ComputeMerkleRoot has a subtle two-output contract: an optional mutation flag and a return value that callers expect to always equal the Merkle root of the full input (regardless of mutations).
That contract is currently undocumented and only exercised indirectly by merkle_test (random duplications, old-vs-new comparison), so a refactor could silently break it - as the discussion under #22046/#28430 illustrates.

Fix

Document the contract on the function declaration and inside the inner loop, and add a direct regression test for the duplicate-subtree construction described in the code comments for CVE-2012-2459:

A A
/ \ / \
B C B C
/ \ | / \ / \
D E F D E F F
/ \ / \ / \ / \ / \ / \ / \
1 2 3 4 5 6 1 2 3 4 5 6 5 6
for transaction lists [1,2,3,4,5,6] and [1,2,3,4,5,6,5,6] (where 5 and
6 are repeated) result in the same root hash A (because the hash of both

Coverage check

Both merkle_test and the new merkle_test_mutated_return_value would fail under a refactor that stops the outer reduction once mutation is detected, e.g.:

Hypothetical regression
diff --git a/src/consensus/merkle.cpp b/src/consensus/merkle.cpp
--- a/src/consensus/merkle.cpp
+++ b/src/consensus/merkle.cpp
@@ -53,6 +53,7 @@
                 if (hashes[pos] == hashes[pos + 1]) mutation = true;
             }
         }
+        if (mutation) break;
         if (hashes.size() & 1) {
             hashes.push_back(hashes.back());
         }

Fixes #28457

@DrahtBot
Copy link
Copy Markdown
Contributor

DrahtBot commented Apr 26, 2026

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

`ComputeMerkleRoot` reports duplicate-subtree mutations through its optional output flag, but callers still rely on the return value being the Merkle root for the whole input.

Document that contract on the function declaration and inside the inner loop, and add a direct regression test for the duplicate-subtree construction described in the code comments for CVE-2012-2459: `[1,2,3,4,5,6]` and `[1,2,3,4,5,6,5,6]` produce the same root.

The existing `merkle_test` already exercises this invariant indirectly through random duplications and old-vs-new comparisons; the new test pins it down explicitly at the `ComputeMerkleRoot` API boundary. Both would fail under a refactor that stops the outer reduction once mutation is detected, e.g.:

```diff
diff --git a/src/consensus/merkle.cpp b/src/consensus/merkle.cpp
--- a/src/consensus/merkle.cpp
+++ b/src/consensus/merkle.cpp
@@ -53,6 +53,7 @@
                 if (hashes[pos] == hashes[pos + 1]) mutation = true;
             }
         }
+        if (mutation) break;
         if (hashes.size() & 1) {
             hashes.push_back(hashes.back());
         }
```
@l0rinc l0rinc force-pushed the l0rinc/doc-merkle-root-mutated branch from 8faf7b6 to f2dbc6a Compare April 26, 2026 19:14
@DrahtBot
Copy link
Copy Markdown
Contributor

🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/24963858314/job/73095124395
LLM reason (✨ experimental): CI failed because clang-tidy reported a bugprone-use-after-move error (moved mutated_leaves then used) in test/merkle_tests.cpp, treated as warnings-as-errors.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

consensus: Better document ComputeMerkleRoot & add test for return value

2 participants