refactor: mining interface 30955 followups #31197

Sjors commented at 6:07 PM on October 31, 2024: member

This PR implements the refactors suggested in #30955#pullrequestreview-2354931253.

DrahtBot commented at 6:07 PM on October 31, 2024: 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/31197.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, itornaza, ryanofsky
Stale ACK	TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31283 (Add waitNext() to BlockTemplate interface by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Oct 31, 2024

Skate-Cloud approved

AlexWater123456789 approved

ryanofsky approved

ryanofsky commented at 1:49 PM on November 5, 2024: contributor

Code review ACK 058862581085316927287817b2af01e8f4765a1d. Looks good, thanks for the cleanups!

in src/consensus/merkle.cpp:87 in 0588625810 outdated

  83 | @@ -84,8 +84,8 @@ uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated)
  84 |  }
  85 |  
  86 |  /* This implements a constant-space merkle root/path calculator, limited to 2^32 leaves. */
  87 | -static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot, bool* pmutated, uint32_t branchpos, std::vector<uint256>* pbranch) {
  88 | -    if (pbranch) pbranch->clear();
  89 | +static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot, bool* pmutated, uint32_t leaf_pos, std::vector<uint256>* path) {

sedited commented at 8:15 AM on November 8, 2024:

Nit: Since this is a style fix in the first place already, could also put the bracket on a new line to make clang-format happy.

Sjors commented at 3:30 PM on November 8, 2024:

Will do if I need to retouch.

Sjors commented at 3:10 AM on December 17, 2024:

Done

sedited approved

sedited commented at 8:17 AM on November 8, 2024: contributor

ACK 058862581085316927287817b2af01e8f4765a1d

Sjors commented at 7:14 AM on December 16, 2024: member

cc @itornaza, @tdb3 maybe you can take a look?

itornaza approved

itornaza commented at 10:35 AM on December 16, 2024: contributor

ACK 058862581085316927287817b2af01e8f4765a1d

Clean refactor in Merkle context variable descriptions from using branch to path. I also find not using default parameters more beneficial as in the current case of the TransactionMerklePath's, position argument.

Also, did a clean built locally and run all unit, functional and extended tests which all pass.

in src/consensus/merkle.cpp:112 in 0588625810 outdated

 104 | @@ -105,18 +105,18 @@ static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot
 105 |      // First process all leaves into 'inner' values.
 106 |      while (count < leaves.size()) {
 107 |          uint256 h = leaves[count];
 108 | -        bool matchh = count == branchpos;
 109 | +        bool matchh = count == leaf_pos;
 110 |          count++;

tdb3 commented at 5:08 PM on December 16, 2024:

The function is limited to 2^32 leaves (uint32_t count). I'm wondering if it's worth placing a check (e.g. leaves.size() >= (2^32)) and returning an empty path and default hash for proot for this condition, or an Assert/Assume. Seems pretty unlikely someone might want to use the function with that many leaves, but the condition would be covered (vs overflowing count).

Or alternatively, maybe count's type could be changed (e.g. std::vector<uint256>::size_type so the limitation is handled by the platform)?

Sjors commented at 3:09 AM on December 17, 2024:

Currently ComputeMerklePath is only called from TransactionMerklePath which sets leaves.resize(block.vtx.size()), so there's no way the array can be bigger.

Added a commit to check it anyway.

tdb3 approved

tdb3 commented at 5:09 PM on December 16, 2024: contributor

code review ACK 058862581085316927287817b2af01e8f4765a1d

The updates make the interface clearer. Left one non-blocking comment.

Lightly tested by cherry-picking bitcoin-mine from #30437 to exercise some of the interface (exercising TransactionMerklePath(), ComputeMerklePath(), and MerkleComputation()) . Saw the merkle path being built up as transactions were added.

<summary>Output from bitcoin-mine</summary>

git cherry-pick 1a4ca89fad87464023430ebe02685fa23ed2bb1d
git cherry-pick 8fab41611a473635ca5fc4804d765cc4de25857c
git cherry-pick 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b

diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
index 4d5faea23e..473fcaf1b9 100644
--- a/src/bitcoin-mine.cpp
+++ b/src/bitcoin-mine.cpp
@@ -120,5 +120,14 @@ MAIN_FUNCTION
         tfm::format(std::cout, "Tip hash is null.\n");
     }
 
+    std::vector<unsigned char> v{0x00, 0x14, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0x00};
+    CScript coinbase_script = CScript() << std::vector<unsigned char>(v.begin(), v.end());
+    std::unique_ptr<interfaces::BlockTemplate> new_block{mining->createNewBlock(coinbase_script)};
+    auto merkle_path{new_block->getCoinbaseMerklePath()};
+    tfm::format(std::cout, "Printing merkle path:\n");
+    for (auto const& b : merkle_path) {
+        tfm::format(std::cout, "%s\n", b.GetHex());
+    }
+
     return EXIT_SUCCESS;
 }

make -C depends NO_QT=1 MULTIPROCESS=1
HOST_PLATFORM="x86_64-pc-linux-gnu"
cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake
cmake --build build -j12
build/src/bitcoin-node -regtest -printtoconsole -debug=ipc -ipcbind=unix

$ build/src/bitcoin-cli -regtest createwallet test
$ build/src/bitcoin-cli -regtest -generate 101

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
62b8b4e735316df78125d4119c0ab867c71fe588668bdc7323c0b7707e3235dc

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
84cabb7420050b1c3b18a162dccf7526b055b195ac873a0baea3c0c52a2c7e56

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
425686bfda45ef369c7acb2636c5c234084c8361f54ffd39dda3c1a330bd3a22

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
8f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
e152a9ac29651ce96e5ee59b648c6e14ff850df1799dfd5453f96e681f3d0267

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
8f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
73a8292c417ccdd779e725e54c71968a7feacaa00f083fc8b08865f0de8caf5a

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
251ff17e509ca64eb4a026b571943de86202420d15ceccfc273bb9806accc6a1

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
8f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
6db4d6f4ed256dff78d636ea2950e801ebc0b8aa9c781374dbf545cc38f5f705

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
65fc3f53395ff343f684c666027ca565a41dda1e8aa69ff06d38baf56e160e00

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
8f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
7d315aaad342ffb8b0c9431fc7601dfd79fa72ea64492f1b27288e83e43ff22e

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
51c21fab60d667d346a2ea81c813efbad181daffe2381c1ab40b3e8541678a0f

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
8f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
e4e50b35186ef0bec23c7df9473856a673dc2cf401c9b6552e540618fecc6954

$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
aad26e5184382623d900c06de7ef5cf4eb861b4a40d418a7f80e0f007fa0cb9a

$ build/src/bitcoin-mine -regtest
Connected to bitcoin-node
Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
Printing merkle path:
354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
8f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
e4e50b35186ef0bec23c7df9473856a673dc2cf401c9b6552e540618fecc6954
a56bf13e0afae34cb58cf8065a08b52d714659cbe3f45e217024495c27146186

</details>

Rename merkle branch to path 39d3b538e6

Drop TransactionMerklePath default position arg 2e81791d90

refactor: use CTransactionRef in submitSolution 4d57288246

Check leaves size maximum in MerkleComputation

Belt and suspenders for future code changes.

Currently this function is only called from TransactionMerklePath() which sets leaves to the block transactions, so the Assume always holds.

f86678156a

Sjors force-pushed on Dec 17, 2024

Sjors commented at 3:13 AM on December 17, 2024: member

Addressed the two comments and rebased.

tdb3 approved

tdb3 commented at 3:34 AM on December 17, 2024: contributor

code review re-ACK f86678156a3d3ce6488b383a2d6d91d28fcd2b73

Other than the rebase, the only differences observed were the { movement and the addition of the Assume(leaves.size() <= UINT32_MAX); in the last commit.

$ git range-diff f07a533dfcb172321972e5afb3b38a4bd24edb87..058862581085316927287817b2af01e8f4765a1d 39d3b538e6a2af8db85077e958970cdcd0ee7f7d~..f86678156a3d3ce6488b383a2d6d91d28fcd2b73

DrahtBot requested review from itornaza on Dec 17, 2024

DrahtBot requested review from ryanofsky on Dec 17, 2024

DrahtBot requested review from sedited on Dec 17, 2024

itornaza approved

itornaza commented at 11:44 AM on December 17, 2024: contributor

re ACK f86678156a3d3ce6488b383a2d6d91d28fcd2b73

ryanofsky approved

ryanofsky commented at 4:20 PM on December 17, 2024: contributor

Code review ACK f86678156a3d3ce6488b383a2d6d91d28fcd2b73 only changes since last review are a whitespace change and adding an Assume statement to check for size_t -> uint32_t overflows

ryanofsky merged this on Dec 17, 2024

ryanofsky closed this on Dec 17, 2024

sedited referenced this in commit 230a439a4a on Jan 17, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

bug-castercv502 referenced this in commit 44b075fe71 on Sep 28, 2025

bitcoin locked this on Dec 17, 2025