refactor: mining interface 30955 followups

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) {

TheCharlatan 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

TheCharlatan approved

TheCharlatan 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.

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

 0diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
 1index 4d5faea23e..473fcaf1b9 100644
 2--- a/src/bitcoin-mine.cpp
 3+++ b/src/bitcoin-mine.cpp
 4@@ -120,5 +120,14 @@ MAIN_FUNCTION
 5         tfm::format(std::cout, "Tip hash is null.\n");
 6     }
 7 
 8+    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};
 9+    CScript coinbase_script = CScript() << std::vector<unsigned char>(v.begin(), v.end());
10+    std::unique_ptr<interfaces::BlockTemplate> new_block{mining->createNewBlock(coinbase_script)};
11+    auto merkle_path{new_block->getCoinbaseMerklePath()};
12+    tfm::format(std::cout, "Printing merkle path:\n");
13+    for (auto const& b : merkle_path) {
14+        tfm::format(std::cout, "%s\n", b.GetHex());
15+    }
16+
17     return EXIT_SUCCESS;
18 }

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

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

 0$ build/src/bitcoin-mine -regtest
 1Connected to bitcoin-node
 2Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
 3Printing merkle path:
 4
 5$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
 6354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
 7
 8$ build/src/bitcoin-mine -regtest
 9Connected to bitcoin-node
10Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
11Printing merkle path:
12354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
13
14$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
1562b8b4e735316df78125d4119c0ab867c71fe588668bdc7323c0b7707e3235dc
16
17$ build/src/bitcoin-mine -regtest
18Connected to bitcoin-node
19Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
20Printing merkle path:
21354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
2284cabb7420050b1c3b18a162dccf7526b055b195ac873a0baea3c0c52a2c7e56
23
24$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
25425686bfda45ef369c7acb2636c5c234084c8361f54ffd39dda3c1a330bd3a22
26
27$ build/src/bitcoin-mine -regtest
28Connected to bitcoin-node
29Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
30Printing merkle path:
31354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
328f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
33
34$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
35e152a9ac29651ce96e5ee59b648c6e14ff850df1799dfd5453f96e681f3d0267
36
37$ build/src/bitcoin-mine -regtest
38Connected to bitcoin-node
39Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
40Printing merkle path:
41354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
428f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
4373a8292c417ccdd779e725e54c71968a7feacaa00f083fc8b08865f0de8caf5a
44
45$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
46251ff17e509ca64eb4a026b571943de86202420d15ceccfc273bb9806accc6a1
47
48$ build/src/bitcoin-mine -regtest
49Connected to bitcoin-node
50Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
51Printing merkle path:
52354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
538f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
546db4d6f4ed256dff78d636ea2950e801ebc0b8aa9c781374dbf545cc38f5f705
55
56$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
5765fc3f53395ff343f684c666027ca565a41dda1e8aa69ff06d38baf56e160e00
58
59$ build/src/bitcoin-mine -regtest
60Connected to bitcoin-node
61Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
62Printing merkle path:
63354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
648f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
657d315aaad342ffb8b0c9431fc7601dfd79fa72ea64492f1b27288e83e43ff22e
66
67$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
6851c21fab60d667d346a2ea81c813efbad181daffe2381c1ab40b3e8541678a0f
69
70$ build/src/bitcoin-mine -regtest
71Connected to bitcoin-node
72Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
73Printing merkle path:
74354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
758f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
76e4e50b35186ef0bec23c7df9473856a673dc2cf401c9b6552e540618fecc6954
77
78$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
79aad26e5184382623d900c06de7ef5cf4eb861b4a40d418a7f80e0f007fa0cb9a
80
81$ build/src/bitcoin-mine -regtest
82Connected to bitcoin-node
83Tip hash is 12dda23b94be0cffe84c094fa30c25e08ead0b3ca2e8b8b738a87d4decc96d04.
84Printing merkle path:
85354f5f8a7b741395656417958d3339b8affb74f2bc7c93efc8c2710207c74e79
868f0b568257b75176c56703ae57036cba1bf2cc23a5909f60d6bc9544473a553f
87e4e50b35186ef0bec23c7df9473856a673dc2cf401c9b6552e540618fecc6954
88a56bf13e0afae34cb58cf8065a08b52d714659cbe3f45e217024495c27146186

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.

0$ 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 TheCharlatan 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

refactor: mining interface 30955 followups #31197

Code Coverage & Benchmarks

Reviews

Conflicts