merkle: migrate `path` arg to reference and drop unused args

l0rinc commented at 9:15 am on November 6, 2025: contributor

Summary

Simplifies merkle tree computation by removing dead code found through coverage analysis (following up on #33768 and #33786).

History

BlockWitnessMerkleRoot

Original MerkleComputation was added in https://github.com/bitcoin/bitcoin/commit/ee60e5625bf8a11c8e5509b9cea8b6465056c448#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3R131 where it was called for either &hash, mutated or position, &ret args. In https://github.com/bitcoin/bitcoin/commit/1f0e7ca09c9d7c5787c218156fa5096a1bdf2ea8#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165 the first usage was inlined in ComputeMerkleRoot, leaving the proot and , pmutated values unused in MerkleComputation. Later in https://github.com/bitcoin/bitcoin/commit/4defdfab94504018f822dc34a313ad26cedc8255 the method was moved to test and in https://github.com/bitcoin/bitcoin/commit/63d6ad7c89cd682f6e779968c4861ea085058356#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3R87-R95 was restored to the code, though with unused parameters again.

BlockWitnessMerkleRoot

BlockWitnessMerkleRoot was introduced in https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93 where it was already called with NULL https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3509 or an unused dummy https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3598-R3599 for the mutated parameter.

Fixes

BlockWitnessMerkleRoot

Converts path parameter from pointer to reference (always non-null at call site)
Removes proot and pmutated parameters (always nullptr at call site)

BlockWitnessMerkleRoot

Removes unused mutated output parameter (always passed as nullptr)

The change is a refactor that shouldn’t introduce any behavioral change, only remove dead code, leftovers from previous refactors.

Coverage proof

https://maflcko.github.io/b-c-cov/total.coverage/src/consensus/merkle.cpp.gcov.html

DrahtBot commented at 9:15 am on November 6, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33805.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, sedited, hodlinator
Concept ACK	Sjors

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

merkle: migrate `path` arg of `MerkleComputation` to a reference

There's a single call to the methods from `ComputeMerklePath` where the last parameter is always provided.
This simplifies the implementation by not having to check for missing parameter.

be270551df

merkle: remove unused `proot` and `pmutated` args from `MerkleComputation`

There's a single call to the methods from `ComputeMerklePath` where these were always set to `nullptr`.

63d640fa6a

merkle: remove unused `mutated` arg from `BlockWitnessMerkleRoot`

The `mutated` parameter is never used at any call site - all callers pass `nullptr`.
The explicit comment in `validation.cpp` explains the reason:
// The malleation check is ignored; as the transaction tree itself
// already does not permit it, it is impossible to trigger in the
// witness tree.

24ed820d4f

l0rinc force-pushed on Nov 6, 2025

l0rinc marked this as ready for review on Nov 6, 2025

maflcko commented at 4:06 pm on November 6, 2025: member

Since when is this “unused”?

l0rinc commented at 4:10 pm on November 6, 2025: contributor

Since when is this “unused”?

Can you be more specific please? These methods were always called with the same values (nullptr), always disabling that functionality.

optout21 commented at 2:44 pm on November 10, 2025: none

Concept ACK

I can’t judge the probability of mutability check here needed again in the future, but in that case, it can be added.

optout21 commented at 5:49 pm on November 10, 2025: none

utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

Removes some unused code lines and method parameters, fairly local, no change in functionality.

l0rinc commented at 6:34 pm on November 10, 2025: contributor

~~@maflcko, what happened to our friend, @DrahtBot? Was it banned from GitHub?~~ Edit: @DrahtBot is back \:D/

l0rinc commented at 11:21 am on November 12, 2025: contributor

Since when is this “unused”?

Did some digging, https://github.com/bitcoin/bitcoin/commit/1f0e7ca09c9d7c5787c218156fa5096a1bdf2ea8#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165 inlined ComputeMerkleRoot in one case, leaving the proot and pmutated values unused in MerkleComputation (cc: @sipa, @Sjors). And BlockWitnessMerkleRoot was introduced in https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93 where it was already called with NULL https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3509 or an unused dummy https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3598-R3599 for the mutated parameter (cc: @sipa).

Added the relevant historical commits to the PR’s description.

Sjors commented at 1:02 pm on November 12, 2025: member

Concept ACK

utACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

This was merely test code up until recently when I needed it for the mining interface. That happened in #30955 by moving BlockMerkleBranch, MerkleComputation and ComputeMerkleBranch from test code (back) to merkle.{h,cpp}.

At the time I didn’t check for unused code paths. The mining interface doesn’t need mutated or proot.

The exception is BlockWitnessMerkleRoot which wasn’t moved in that PR, but as you point it was called with nullptr. That has been the case since #29412 (cc @dergoegge). Even before that the result was (documented to be) ignored since the introduction of SegWit in #8149

in src/test/fuzz/merkle.cpp:54 in 24ed820d4f

50@@ -51,7 +51,7 @@ FUZZ_TARGET(merkle)
51     }
52 
53     // Test ComputeMerkleRoot
54-    bool mutated = fuzzed_data_provider.ConsumeBool();
55+    bool mutated = fuzzed_data_provider.ConsumeBool(); // output param, initial value shouldn't matter

frankomosh commented at 1:51 pm on November 21, 2025:

0    bool mutated = fuzzed_data_provider.ConsumeBool(); // output param, initial value ignored

l0rinc commented at 1:55 pm on November 21, 2025:

Why is that better?

frankomosh commented at 1:59 pm on November 21, 2025:

nit

l0rinc commented at 2:09 pm on November 21, 2025:

Why is that better?

frankomosh commented at 2:12 pm on November 21, 2025:

Why is that better?

“ignored” clarifies the value is not read at all, while “shouldn’t matter” could mean read-but-no-effect. Very minor preference though, I think either can works just fine

l0rinc commented at 2:18 pm on November 21, 2025:

Thanks, I prefer the previous versions since it’s higher level, it’s expressing the intent, not the implementation detail

sedited commented at 1:24 pm on December 4, 2025:

Do you think there would be value in also passing in nullptr:

0diff --git a/src/test/fuzz/merkle.cpp b/src/test/fuzz/merkle.cpp
1index 4bb91faf0b..5b91a97b75 100644
2--- a/src/test/fuzz/merkle.cpp
3+++ b/src/test/fuzz/merkle.cpp
4@@ -55 +55,2 @@ FUZZ_TARGET(merkle)
5-    const uint256 merkle_root = ComputeMerkleRoot(tx_hashes, &mutated);
6+    bool* pmutated = fuzzed_data_provider.PickValueInArray({&mutated, static_cast<bool*>(nullptr)});
7+    const uint256 merkle_root = ComputeMerkleRoot(tx_hashes, pmutated);

l0rinc commented at 12:28 pm on December 5, 2025:

Yes, we should do that in a follow-up, thanks

sedited approved

sedited commented at 1:29 pm on December 4, 2025: contributor

ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

DrahtBot requested review from Sjors on Dec 4, 2025

in src/consensus/merkle.cpp:88 in 24ed820d4f

87 }
88 
89-/* This implements a constant-space merkle root/path calculator, limited to 2^32 leaves. */
90-static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot, bool* pmutated, uint32_t leaf_pos, std::vector<uint256>* path)
91+/* This implements a constant-space merkle path calculator, limited to 2^32 leaves. */
92+static void MerkleComputation(const std::vector<uint256>& leaves, uint32_t leaf_pos, std::vector<uint256>& path)

hodlinator commented at 2:18 pm on December 4, 2025:

meganit: Could declare parameter const to decrease cognitive complexity

0static void MerkleComputation(const std::vector<uint256>& leaves, const uint32_t leaf_pos, std::vector<uint256>& path)

l0rinc commented at 12:28 pm on December 5, 2025:

We don’t usually do that with primitives, but I agree that sometimes it helps documenting the behavior better

in src/consensus/merkle.cpp:126 in 24ed820d4f

123+                path.push_back(inner[level]);
124+            } else if (matchlevel == level) {
125+                path.push_back(h);
126+                matchh = true;
127             }
128-            mutated |= (inner[level] == h);

hodlinator commented at 10:53 am on December 5, 2025:

remark: The expression on the right side is side-effect free, so removing it is safe. (inner is a raw C++ array - no overloaded operator[], comparing uint256 values doesn’t mutate them).

hodlinator approved

hodlinator commented at 10:57 am on December 5, 2025: contributor

ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94

Effectively removes dead code.

l0rinc commented at 12:28 pm on December 5, 2025: contributor

rfm?

merkle: migrate path arg to reference and drop unused args #33805

Summary

History

BlockWitnessMerkleRoot

BlockWitnessMerkleRoot

Fixes

BlockWitnessMerkleRoot

BlockWitnessMerkleRoot

Coverage proof

Code Coverage & Benchmarks

Reviews

Conflicts

merkle: migrate `path` arg to reference and drop unused args #33805