Mining interface: getCoinbaseMerklePath() and submitSolution() #30955

pull Sjors wants to merge 3 commits into bitcoin:master from Sjors:2024/07/merkle_path changing 6 files +162 −106
  1. Sjors commented at 7:55 am on September 24, 2024: member

    The new BlockTemplate interface introduced in #30440 allows for a more efficient way for a miner to submit the block solution. Instead of having the send the full block, it only needs to provide the nonce, timestamp, version fields and coinbase transaction.

    This PR introduces submitSolution() for that. It’s currently unused.

    #29432 and https://github.com/Sjors/bitcoin/pull/48 use it to process the Stratum v2 message SubmitSolution. The method should be sufficiently generic to work with alternative mining protocols (none exist that I’m aware off).

    This PR also introduces getCoinbaseMerklePath(), which is needed in Stratum v2 to construct the merkle_path field of the NewTemplate message (see spec). The coinbase merkle path is also used in Stratum “v1”, see e.g. https://bitcoin.stackexchange.com/questions/109820/questions-on-merkle-root-hashing-for-stratum-pools

    This last function uses BlockMerkleBranch which was moved to the test code in #13191. The reason back then for moving it was that it was no longer used. This PR moves it back.

    This PR does not change behaviour since both methods are unused.

  2. DrahtBot commented at 7:55 am on September 24, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK itornaza, tdb3, ryanofsky, achow101
    Concept ACK TheCharlatan

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

  3. TheCharlatan commented at 8:19 am on September 24, 2024: contributor
    Concept ACK
  4. tdb3 commented at 10:27 am on September 24, 2024: contributor
    Concept ACK
  5. Panga9309 approved
  6. Move BlockMerkleBranch back to merkle.{h,cpp} 
    The Mining interface uses this function in the next commit
to calculate the coinbase merkle path. Stratum v2 uses
this to send a compact work template.

This partially undoes the change in 4defdfab94504018f822dc34a313ad26cedc8255,
but is not a revert, because the implementation changed in the meantime.

This commit also documents the function.
    63d6ad7c89
  7. Add getCoinbaseMerklePath() to Mining interface 47b4875ef0
  8. Add submitSolution to BlockTemplate interface 525e9dcba0
  9. Sjors force-pushed on Sep 26, 2024
  10. Sjors commented at 8:05 am on September 26, 2024: member
    Rebased after #30510.
  11. itornaza approved
  12. itornaza commented at 8:22 am on September 26, 2024: contributor

    Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5

    In order to verify that MerkleComputation(), ComputeMerkleBranch(), BlockMerkleBranch() were copied verbatim from their resting place at src/test/merkle_tests.cpp to their new home src/consensus/merkle.cpp, I copied the two versions of the functions in separate files and compared them with diff. The only difference I noticed is that now the function BlockMerkleBranch() is not declared as static which is correct, because we want to be using it from outside it’s compilation unit, like for example from src/node/interfaces.cpp. Built the pr locally and run all unit functional and extended tests and all pass.

  13. DrahtBot requested review from TheCharlatan on Sep 26, 2024
  14. DrahtBot requested review from tdb3 on Sep 26, 2024
  15. Sjors commented at 8:24 am on September 26, 2024: member
    @itornaza you may also find git diff --color-moved=dimmed-zebra useful.
  16. tdb3 approved
  17. tdb3 commented at 1:56 am on October 1, 2024: contributor

    Code review and light test ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5

    Cherry-picked bitcoin-mine from #30437 to exercise some of the interface.

    0git cherry-pick aec7e7d897741fe25fbf54995e1825772e0872d7
1git cherry-pick 2227afac0372358287fb879f3b0bd07fd653f3f8
2
3make -C depends NO_QT=1 MULTIPROCESS=1
4HOST_PLATFORM="x86_64-pc-linux-gnu"
5cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake
6cmake --build build -j18
7
8build/src/bitcoin-node -regtest -printtoconsole -debug=ipc -ipcbind=unix

    Made the following update:

     0diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
 1index 47e8e4cf52..77bc1e759c 100644
 2--- a/src/bitcoin-mine.cpp
 3+++ b/src/bitcoin-mine.cpp
 4@@ -112,5 +112,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 }

    and exercised getCoinbaseMerklePath() by viewing the updated merkle path with transactions being added to the mempool.

     0$ build/src/bitcoin-mine -regtest -debug
 1...
 2Printing merkle path:
 3$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
 44dbc1286bcda6a3c72e3d3f0b4cbf32da6cb69c5013520ecd4203de5cb0c8678
 5$ build/src/bitcoin-mine -regtest -debug
 6...
 7Printing merkle path:
 84dbc1286bcda6a3c72e3d3f0b4cbf32da6cb69c5013520ecd4203de5cb0c8678
 9$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
10dcd234ee9ce7c8023bfe39109ae1582018ec18842baf6d853147c55248a09e71
11$ build/src/bitcoin-mine -regtest -debug
12...
13Printing merkle path:
14dcd234ee9ce7c8023bfe39109ae1582018ec18842baf6d853147c55248a09e71
1514cd53f2bc327792c8b8bffd0d19a7b93ed173de904f010b0261d14c243f1225
16$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
172ee486b5e338d078e46500fc304ce922253422c3301679914ffce78e210a5541
18$ build/src/bitcoin-mine -regtest -debug
19...
20Printing merkle path:
212ee486b5e338d078e46500fc304ce922253422c3301679914ffce78e210a5541
22a09d497bde7bf0561fd2c94758640b2fad831d11976d4f732c377753ee2eeca0
23$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
24f670812c2bf2b625891120f46888761c216850aaefd9ca70c234c4709d5f5d9c
25$ build/src/bitcoin-mine -regtest -debug
26...
27Printing merkle path:
282ee486b5e338d078e46500fc304ce922253422c3301679914ffce78e210a5541
29a09d497bde7bf0561fd2c94758640b2fad831d11976d4f732c377753ee2eeca0
309f41cd8210ee51050fb766931f59299f12d3adb9979b9d9d6066312f71969e84
31$ build/src/bitcoin-cli -named sendtoaddress address=bcrt1qctncrkr9ucfvhxkkvf70zegrkqm0k8jwxg7m9u amount=1.2 fee_rate=2
32d2742062a2641b43ca532fc2b292a36b67456dd6f32877389468937331267117
33$ build/src/bitcoin-mine -regtest -debug
34...
35Printing merkle path:
364dbc1286bcda6a3c72e3d3f0b4cbf32da6cb69c5013520ecd4203de5cb0c8678
37afaf34ad1e406fdf5e22cab465098b1ebc9005686e8163940ca0015bc0e1b64e
38f0453cea298e82272e4a6afdac5f88b8d8804f59d08ef2c5d2344a96c3d4db07

    Then added lines to exercise submitSolution() with some hard-coded values:

     0+    // submit a dummy solution
 1+
 2+    // Create coinbase transaction.
 3+    CMutableTransaction coinbaseTx;
 4+    coinbaseTx.vin.resize(1);
 5+    coinbaseTx.vin[0].prevout.SetNull();
 6+    coinbaseTx.vout.resize(1);
 7+    coinbaseTx.vout[0].scriptPubKey = coinbase_script;
 8+    coinbaseTx.vout[0].nValue = 50 * COIN;
 9+    coinbaseTx.vin[0].scriptSig = CScript() << tip->height + 1 << OP_0;
10+
11+    bool processed{new_block->submitSolution(4, 1727746274, 0, coinbaseTx)};
12+    if (processed) {
13+        tfm::format(std::cout, "Successfully processed block\n");
14+    } else {
15+        tfm::format(std::cout, "Unable to process block\n");
16+    }

    submitSolution() was returning false when, for example, proof-of-work was insufficient (2024-10-01T01:38:57Z [error] ProcessNewBlock: AcceptBlock FAILED (high-hash, proof of work failed)). I was initially confused by this because the function documentation states: @returns if the block was processed, independent of block validity but then realized this is essentially echoing ProcessNewBlock()’s documentation:

    https://github.com/bitcoin/bitcoin/blob/fc642c33ef28829eda0119a0fe39fd9bc4b84051/src/validation.h#L1204-L1206

  18. in src/consensus/merkle.cpp:87 in 63d6ad7c89 outdated 
    82@@ -83,3 +83,106 @@ uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated)
83     return ComputeMerkleRoot(std::move(leaves), 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) {
    ryanofsky commented at 4:00 pm on October 8, 2024:

    In commit “Move BlockMerkleBranch back to merkle.{h,cpp}” (63d6ad7c89cd682f6e779968c4861ea085058356)

    Not important, and this is not really an uninformed opinion, but to me the phrase “merkle path” is more descriptive than “merkle branch” and I think it might be worth adding a followup commit to replace branch with path everywhere in this file so it is using consistent terminology.

    Specifically might suggest: ComputeMerkleBranch -> ComputeMerklePath, BlockMerkleBranch -> TransactionMerklePath, pbranch -> path, branchpos -> path_pos.

    Sjors commented at 6:07 pm on October 31, 2024:
    Done in #31197, though I used leaf_pos.
  19. in src/consensus/merkle.h:35 in 63d6ad7c89 outdated 
    30+ * @param[in] block the block
31+ * @param[in] position transaction for which to calculate the merkle path, defaults to coinbase
32+ *
33+ * @return merkle path ordered from the deepest
34+ */
35+std::vector<uint256> BlockMerkleBranch(const CBlock& block, uint32_t position = 0);
    ryanofsky commented at 4:04 pm on October 8, 2024:

    In commit “Move BlockMerkleBranch back to merkle.{h,cpp}” (63d6ad7c89cd682f6e779968c4861ea085058356)

    Not important, but IMO it would be clearer to drop position = 0 default argument so transaction has to be specified explicitly, and rename s/Block/Transaction/ to be clear this is returning merkle path to a specific transaction.

    Sjors commented at 6:07 pm on October 31, 2024:
    Done in #31197
  20. in src/interfaces/mining.h:58 in 525e9dcba0 
    53+    /**
54+     * Construct and broadcast the block.
55+     *
56+     * @returns if the block was processed, independent of block validity
57+     */
58+    virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CMutableTransaction coinbase) = 0;
    ryanofsky commented at 4:13 pm on October 8, 2024:

    In commit “Add submitSolution to BlockTemplate interface” (525e9dcba0b8c6744bcd3725864f39786afc8ed5)

    It might make more sense for this function to take a CTransactionRef parameter instead of a CMutableTransaction parameter, since it is just going to turn the CMutableTransaction into a CTransactionRef anyway.

    Sjors commented at 6:07 pm on October 31, 2024:
    Done in #31197
  21. ryanofsky approved
  22. ryanofsky commented at 4:15 pm on October 8, 2024: contributor
    Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5. Left minor suggestions but none are important, and looks like this could be merged as-is
  23. achow101 commented at 0:11 am on October 10, 2024: member
    ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
  24. achow101 merged this on Oct 10, 2024
  25. achow101 closed this on Oct 10, 2024
  26. Sjors deleted the branch on Oct 31, 2024


Sjors DrahtBot TheCharlatan tdb3 Panga9309 itornaza ryanofsky achow101


TheCharlatan

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me