Merge bitcoin/bitcoin#31197: refactor: mining interface 30955 followups

f866781 Check leaves size maximum in MerkleComputation (Sjors Provoost)
4d57288 refactor: use CTransactionRef in submitSolution (Sjors Provoost)
2e81791 Drop TransactionMerklePath default position arg (Sjors Provoost)
39d3b53 Rename merkle branch to path (Sjors Provoost)

Pull request description:

  This PR implements the refactors suggested in bitcoin/bitcoin#30955 (review).

ACKs for top commit:
  tdb3:
    code review re-ACK f866781
  itornaza:
    re ACK f866781
  ryanofsky:
    Code review ACK f866781 only changes since last review are a whitespace change and adding an Assume statement to check for size_t -> uint32_t overflows

Tree-SHA512: 661b5d5d0e24b2269bf33ab1484e37c36e67b32a7796d77ca3b1856d3043378b081ad43c32a8638b46fa8c0de51c823fd9747dd9fc81f958f20d327bf330a47c

Author: ryanofsky

src/consensus/merkle.cpp

@@ -4,6 +4,7 @@
 
 #include <consensus/merkle.h>
 #include <hash.h>
+#include <util/check.h>
 
 /*     WARNING! If you're reading this because you're learning about crypto
        and/or designing a new system that will use merkle trees, keep in mind
@@ -84,8 +85,10 @@ uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated)
 }
 
 /* This implements a constant-space merkle root/path calculator, limited to 2^32 leaves. */
-static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot, bool* pmutated, uint32_t branchpos, std::vector<uint256>* pbranch) {
-    if (pbranch) pbranch->clear();
+static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot, bool* pmutated, uint32_t leaf_pos, std::vector<uint256>* path)
+{
+    if (path) path->clear();
+    Assume(leaves.size() <= UINT32_MAX);
     if (leaves.size() == 0) {
         if (pmutated) *pmutated = false;
         if (proot) *proot = uint256();
@@ -105,18 +108,18 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot
     // First process all leaves into 'inner' values.
     while (count < leaves.size()) {
         uint256 h = leaves[count];
-        bool matchh = count == branchpos;
+        bool matchh = count == leaf_pos;
         count++;
         int level;
         // For each of the lower bits in count that are 0, do 1 step. Each
         // 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 (pbranch) {
+            if (path) {
                 if (matchh) {
-                    pbranch->push_back(inner[level]);
+                    path->push_back(inner[level]);
                 } else if (matchlevel == level) {
-                    pbranch->push_back(h);
+                    path->push_back(h);
                     matchh = true;
                 }
             }
@@ -144,8 +147,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 (pbranch && matchh) {
-            pbranch->push_back(h);
+        if (path && matchh) {
+            path->push_back(h);
         }
         h = Hash(h, h);
         // Increment count to the value it would have if two entries at this
@@ -154,11 +157,11 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot
         level++;
         // And propagate the result upwards accordingly.
         while (!(count & ((uint32_t{1}) << level))) {
-            if (pbranch) {
+            if (path) {
                 if (matchh) {
-                    pbranch->push_back(inner[level]);
+                    path->push_back(inner[level]);
                 } else if (matchlevel == level) {
-                    pbranch->push_back(h);
+                    path->push_back(h);
                     matchh = true;
                 }
             }
@@ -171,18 +174,18 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot
     if (proot) *proot = h;
 }
 
-static std::vector<uint256> ComputeMerkleBranch(const std::vector<uint256>& leaves, uint32_t position) {
+static std::vector<uint256> ComputeMerklePath(const std::vector<uint256>& leaves, uint32_t position) {
     std::vector<uint256> ret;
     MerkleComputation(leaves, nullptr, nullptr, position, &ret);
     return ret;
 }
 
-std::vector<uint256> BlockMerkleBranch(const CBlock& block, uint32_t position)
+std::vector<uint256> TransactionMerklePath(const CBlock& block, uint32_t position)
 {
     std::vector<uint256> leaves;
     leaves.resize(block.vtx.size());
     for (size_t s = 0; s < block.vtx.size(); s++) {
         leaves[s] = block.vtx[s]->GetHash();
     }
-    return ComputeMerkleBranch(leaves, position);
+    return ComputeMerklePath(leaves, position);
 }

src/consensus/merkle.h

@@ -28,10 +28,10 @@ uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated = nullptr);
  * Compute merkle path to the specified transaction
  *
  * @param[in] block the block
- * @param[in] position transaction for which to calculate the merkle path, defaults to coinbase
+ * @param[in] position transaction for which to calculate the merkle path (0 is the coinbase)
  *
  * @return merkle path ordered from the deepest
  */
-std::vector<uint256> BlockMerkleBranch(const CBlock& block, uint32_t position = 0);
+std::vector<uint256> TransactionMerklePath(const CBlock& block, uint32_t position);
 
 #endif // BITCOIN_CONSENSUS_MERKLE_H

src/interfaces/mining.h

@@ -55,7 +55,7 @@ class BlockTemplate
      *
      * @returns if the block was processed, independent of block validity
      */
-    virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CMutableTransaction coinbase) = 0;
+    virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) = 0;
 };
 
 //! Interface giving clients (RPC, Stratum v2 Template Provider in the future)

src/node/interfaces.cpp

@@ -913,19 +913,17 @@ class BlockTemplateImpl : public BlockTemplate
 
     std::vector<uint256> getCoinbaseMerklePath() override
     {
-        return BlockMerkleBranch(m_block_template->block);
+        return TransactionMerklePath(m_block_template->block, 0);
     }
 
-    bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CMutableTransaction coinbase) override
+    bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
     {
         CBlock block{m_block_template->block};
 
-        auto cb = MakeTransactionRef(std::move(coinbase));
-
         if (block.vtx.size() == 0) {
-            block.vtx.push_back(cb);
+            block.vtx.push_back(coinbase);
         } else {
-            block.vtx[0] = cb;
+            block.vtx[0] = coinbase;
         }
 
         block.nVersion = version;

src/test/merkle_tests.cpp

@@ -135,7 +135,7 @@ BOOST_AUTO_TEST_CASE(merkle_test)
                     if (ntx > 16) {
                         mtx = m_rng.randrange(ntx);
                     }
-                    std::vector<uint256> newBranch = BlockMerkleBranch(block, mtx);
+                    std::vector<uint256> newBranch = TransactionMerklePath(block, mtx);
                     std::vector<uint256> oldBranch = BlockGetMerkleBranch(block, merkleTree, mtx);
                     BOOST_CHECK(oldBranch == newBranch);
                     BOOST_CHECK(ComputeMerkleRootFromBranch(block.vtx[mtx]->GetHash(), newBranch, mtx) == oldRoot);