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

24ed820d4f merkle: remove unused `mutated` arg from `BlockWitnessMerkleRoot` (Lőrinc)
63d640fa6a merkle: remove unused `proot` and `pmutated` args from `MerkleComputation` (Lőrinc)
be270551df merkle: migrate `path` arg of `MerkleComputation` to a reference (Lőrinc)

Pull request description:

  ### 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

ACKs for top commit:
  optout21:
    utACK 24ed820d4f
  Sjors:
    utACK 24ed820d4f
  achow101:
    ACK 24ed820d4f
  sedited:
    ACK 24ed820d4f
  hodlinator:
    ACK 24ed820d4f

Tree-SHA512: 6960411304631bc381a3db7a682f6b6ba51bd58936ca85aa237c69a9109265b736b22ec4d891875bddfcbe8517bd3f014c44a4b387942eee4b01029c91ec93e1

src/consensus/merkle.cpp

@@ -73,7 +73,7 @@ uint256 BlockMerkleRoot(const CBlock& block, bool* mutated)
     return ComputeMerkleRoot(std::move(leaves), mutated);
 }
 
-uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated)
+uint256 BlockWitnessMerkleRoot(const CBlock& block)
 {
     std::vector<uint256> leaves;
     leaves.resize(block.vtx.size());
@@ -81,20 +81,17 @@ uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated)
     for (size_t s = 1; s < block.vtx.size(); s++) {
         leaves[s] = block.vtx[s]->GetWitnessHash().ToUint256();
     }
-    return ComputeMerkleRoot(std::move(leaves), mutated);
+    return ComputeMerkleRoot(std::move(leaves));
 }
 
-/* This implements a constant-space merkle root/path calculator, limited to 2^32 leaves. */
+/* This implements a constant-space merkle path calculator, limited to 2^32 leaves. */
-static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot, bool* pmutated, uint32_t leaf_pos, std::vector<uint256>* path)
+static void MerkleComputation(const std::vector<uint256>& leaves, uint32_t leaf_pos, std::vector<uint256>& path)
 {
-    if (path) path->clear();
+    path.clear();
     Assume(leaves.size() <= UINT32_MAX);
     if (leaves.size() == 0) {
-        if (pmutated) *pmutated = false;
-        if (proot) *proot = uint256();
         return;
     }
-    bool mutated = false;
     // count is the number of leaves processed so far.
     uint32_t count = 0;
     // inner is an array of eagerly computed subtree hashes, indexed by tree
@@ -115,15 +112,12 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot
         // corresponds to an inner value that existed before processing the
         // current leaf, and each needs a hash to combine it.
         for (level = 0; !(count & ((uint32_t{1}) << level)); level++) {
-            if (path) {
+            if (matchh) {
-                if (matchh) {
+                path.push_back(inner[level]);
-                    path->push_back(inner[level]);
+            } else if (matchlevel == level) {
-                } else if (matchlevel == level) {
+                path.push_back(h);
-                    path->push_back(h);
+                matchh = true;
-                    matchh = true;
-                }
             }
-            mutated |= (inner[level] == h);
             h = Hash(inner[level], h);
         }
         // Store the resulting hash at inner position level.
@@ -147,8 +141,8 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot
         // If we reach this point, h is an inner value that is not the top.
         // We combine it with itself (Bitcoin's special rule for odd levels in
         // the tree) to produce a higher level one.
-        if (path && matchh) {
+        if (matchh) {
-            path->push_back(h);
+            path.push_back(h);
         }
         h = Hash(h, h);
         // Increment count to the value it would have if two entries at this
@@ -157,26 +151,21 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot
         level++;
         // And propagate the result upwards accordingly.
         while (!(count & ((uint32_t{1}) << level))) {
-            if (path) {
+            if (matchh) {
-                if (matchh) {
+                path.push_back(inner[level]);
-                    path->push_back(inner[level]);
+            } else if (matchlevel == level) {
-                } else if (matchlevel == level) {
+                path.push_back(h);
-                    path->push_back(h);
+                matchh = true;
-                    matchh = true;
-                }
             }
             h = Hash(inner[level], h);
             level++;
         }
     }
-    // Return result.
-    if (pmutated) *pmutated = mutated;
-    if (proot) *proot = h;
 }
 
 static std::vector<uint256> ComputeMerklePath(const std::vector<uint256>& leaves, uint32_t position) {
     std::vector<uint256> ret;
-    MerkleComputation(leaves, nullptr, nullptr, position, &ret);
+    MerkleComputation(leaves, position, ret);
     return ret;
 }
 

src/consensus/merkle.h

@@ -20,9 +20,8 @@ uint256 BlockMerkleRoot(const CBlock& block, bool* mutated = nullptr);
 
 /*
  * Compute the Merkle root of the witness transactions in a block.
- * *mutated is set to true if a duplicated subtree was found.
  */
-uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated = nullptr);
+uint256 BlockWitnessMerkleRoot(const CBlock& block);
 
 /**
  * Compute merkle path to the specified transaction

src/test/fuzz/merkle.cpp

@@ -51,7 +51,7 @@ FUZZ_TARGET(merkle)
     }
 
     // Test ComputeMerkleRoot
-    bool mutated = fuzzed_data_provider.ConsumeBool();
+    bool mutated = fuzzed_data_provider.ConsumeBool(); // output param, initial value shouldn't matter
     const uint256 merkle_root = ComputeMerkleRoot(tx_hashes, &mutated);
 
     // Basic sanity checks for ComputeMerkleRoot
@@ -66,7 +66,7 @@ FUZZ_TARGET(merkle)
     }
 
     if (!block->vtx.empty()){
-        const uint256 block_witness_merkle_root = BlockWitnessMerkleRoot(*block, &mutated);
+        const uint256 block_witness_merkle_root = BlockWitnessMerkleRoot(*block);
         if (tx_hashes.size() == 1) {
             assert(block_witness_merkle_root == uint256());
         }

src/validation.cpp

@@ -3926,7 +3926,7 @@ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_comm
             // The malleation check is ignored; as the transaction tree itself
             // already does not permit it, it is impossible to trigger in the
             // witness tree.
-            uint256 hash_witness = BlockWitnessMerkleRoot(block, /*mutated=*/nullptr);
+            uint256 hash_witness = BlockWitnessMerkleRoot(block);
 
             CHash256().Write(hash_witness).Write(witness_stack[0]).Finalize(hash_witness);
             if (memcmp(hash_witness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) {
@@ -4039,7 +4039,7 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&
     int commitpos = GetWitnessCommitmentIndex(block);
     std::vector<unsigned char> ret(32, 0x00);
     if (commitpos == NO_WITNESS_COMMITMENT) {
-        uint256 witnessroot = BlockWitnessMerkleRoot(block, nullptr);
+        uint256 witnessroot = BlockWitnessMerkleRoot(block);
         CHash256().Write(witnessroot).Write(ret).Finalize(witnessroot);
         CTxOut out;
         out.nValue = 0;