Prune mining interface #31196

1. 
   Sjors commented at 5:17 pm on October 31, 2024: member

   There are three methods in the mining interface that can be dropped. The Template Provider doesn’t need them and other application should probably not use them either.

   1. processNewBlock() was added in 7b4d3249ced93ec5986500e43b324005ed89502f, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ced93ec5986500e43b324005ed89502f.

   Dropping it was suggested in #30200 (comment)

   2. getTransactionsUpdated(): this is used in the implementation of #31003 waitFeesChanged. It’s not very useful generically because the mempool updates very frequently.

   3. testBlockValidity(): it might be useful for mining application to have a way to check the validity of a block template they modified, but the Stratum v2 Template Provider doesn’t do that, and this method is a bit brittle (e.g. the block needs to build on the tip).

2. 
   DrahtBot commented at 5:17 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/31196.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, ryanofsky, tdb3

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

   Conflicts

   No conflicts as of last run.

3. Sjors force-pushed on Oct 31, 2024
4. DrahtBot added the label CI failed on Oct 31, 2024
5. 
   DrahtBot commented at 5:45 pm on October 31, 2024: contributor

   🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32350220781

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

6. Sjors force-pushed on Oct 31, 2024
7. Sjors renamed this:
   Remove processNewBlock from mining interface
   Prune mining interface
   on Oct 31, 2024
8. 
   Sjors commented at 7:09 pm on October 31, 2024: member

   I added commits to drop getTransactionsUpdated() and testBlockValidity() as well.
9. in src/rpc/mining.cpp:390 in 4d4a1822ba outdated

   382@@ -383,9 +383,10 @@ static RPCHelpMan generateblock()
   383     RegenerateCommitments(block, chainman);
   384 
   385     {
   386+        LOCK(::cs_main);
   387         BlockValidationState state;
   388-        if (!miner.testBlockValidity(block, /*check_merkle_root=*/false, state)) {
   389-            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("testBlockValidity failed: %s", state.ToString()));
   390+        if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock),  /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
   391+            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
   

   ---

   ---

   

   ryanofsky commented at 1:33 pm on November 5, 2024:

   In commit “Remove testBlockValidity() from mining interface” (4d4a1822ba8eef2ff857885c5253156dacff8d3c)

   This change seem to be causing the rpc_generate.py test to fail. Can be fixed with:

    0--- a/test/functional/rpc_generate.py
    1+++ b/test/functional/rpc_generate.py
    2@@ -87,7 +87,7 @@ class RPCGenerateTest(BitcoinTestFramework):
    3         txid1 = miniwallet.send_self_transfer(from_node=node)['txid']
    4         utxo1 = miniwallet.get_utxo(txid=txid1)
    5         rawtx2 = miniwallet.create_self_transfer(utxo_to_spend=utxo1)['hex']
    6-        assert_raises_rpc_error(-25, 'testBlockValidity failed: bad-txns-inputs-missingorspent', self.generateblock, node, address, [rawtx2, txid1])
    7+        assert_raises_rpc_error(-25, 'TestBlockValidity failed: bad-txns-inputs-missingorspent', self.generateblock, node, address, [rawtx2, txid1])
    8 
    9         self.log.info('Fail to generate block with txid not in mempool')
   10         missing_txid = '0000000000000000000000000000000000000000000000000000000000000000'
   

   ---

   

   Sjors commented at 3:23 pm on November 10, 2024:

   Fixed
10. in src/node/interfaces.cpp:998 in 4d4a1822ba outdated

     995-    {
     996-        LOCK(cs_main);
     997-        CBlockIndex* tip{chainman().ActiveChain().Tip()};
     998-        // Fail if the tip updated before the lock was taken
     999-        if (block.hashPrevBlock != tip->GetBlockHash()) {
    1000-            state.Error("Block does not connect to current chain tip.");
    

    ---

    ---

    

    ryanofsky commented at 1:37 pm on November 5, 2024:

    In commit “Remove testBlockValidity() from mining interface” (4d4a1822ba8eef2ff857885c5253156dacff8d3c)

    Review note: it seemed like a potential problem that this check is being dropped, but looking more closely it should be ok. In the getblocktemplate “proposal” call this condition is already checked in, and in the generateblock call the check wouldn’t reliable because cs_main isn’t hold, and more reliable checking happens later when the block is connected.

    ---

    

    Sjors commented at 3:24 pm on November 10, 2024:

    I updated the commit message to include a reference to a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed where this check was added.
11. ryanofsky approved
12. 
    ryanofsky commented at 1:43 pm on November 5, 2024: contributor

    Code review 6d46e067b71901308433afd7c70d4ea64c2a1d1b, but the rpc_generate.py test is currently broken (https://cirrus-ci.com/task/4929149096165376?logs=ci#L4665). Suggested a fix below
13. 
    dergoegge commented at 10:39 am on November 6, 2024: member

    Perhaps relevant for the discussion around removing testBlockValidity: #31136 (comment)
14. 
    Sjors commented at 3:27 pm on November 8, 2024: member

    @dergoegge I made a note to re-introduce a verification method for blocks. IIUC the use case would be to verify externally generated blocks, similar to how proposal works over RPC - but faster because it avoids RPC overhead.

    I’d rather design that method from scratch (dropping the current implementation). E.g. it should provide useful feedback if our tip doesn’t match, maybe even have an option to reorg in that case, etc. This is not needed for Stratum v2, so it’s lower on my list of priorities.

15. Sjors force-pushed on Nov 10, 2024
16. DrahtBot removed the label CI failed on Nov 10, 2024
17. ryanofsky approved
18. 
    ryanofsky commented at 11:39 am on November 12, 2024: contributor

    Code review ACK a67ba64f8f63a0f9f2c86d72cf7240bee7c5388c. Only changes since last review were a tweak to fix a failing test, and a commit message update.
19. TheCharlatan approved
20. 
    TheCharlatan commented at 1:40 pm on November 19, 2024: contributor

    ACK a67ba64f8f63a0f9f2c86d72cf7240bee7c5388c
21. 
    Sjors commented at 3:37 pm on November 21, 2024: member

    I tested cherry-picking this PR on top of #31175 and the tests still pass.
22. Sjors force-pushed on Dec 16, 2024
23. 
    Sjors commented at 6:58 am on December 16, 2024: member

    Rebased just in case after #31175 and #31175 landed.
24. ryanofsky approved
25. 
    ryanofsky commented at 4:17 pm on December 17, 2024: contributor

    Code review ACK bf0966b7788af735fb06e743e70d0f59ceac20d5. No changes since last review other than rebase. These are nice changes that simplify code and make should make the mining interface easier to understand.
26. DrahtBot requested review from TheCharlatan on Dec 17, 2024
27. DrahtBot added the label Needs rebase on Dec 17, 2024
28. Remove testBlockValidity() from mining interface

    It's very low level and not used by the proposed Template Provider.
    
    This method was introduced in d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3
    and a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed.

    bfc4e029d4
29. Remove getTransactionsUpdated() from mining interface

    It's unnecessary to expose it via this interface.

    9a47852d88
30. Remove processNewBlock() from mining interface

    processNewBlock was added in 7b4d3249ced93ec5986500e43b324005ed89502f, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ced93ec5986500e43b324005ed89502f.
    
    getTransactionsUpdated() is only needed by the implementation of waitFeesChanged() (not yet part of the interface).

    c991cea1a0
31. Sjors force-pushed on Dec 18, 2024
32. 
    Sjors commented at 2:22 am on December 18, 2024: member

    Thanks for the review. Another rebase after #31318.
33. DrahtBot removed the label Needs rebase on Dec 18, 2024
34. TheCharlatan approved
35. 
    TheCharlatan commented at 9:30 am on December 18, 2024: contributor

    Re-ACK c991cea1a0c3ea99dc3e3919789ddf32a338a59d
36. DrahtBot requested review from ryanofsky on Dec 18, 2024
37. 
    Sjors commented at 9:52 am on December 18, 2024: member

    cc @tdb3 @itornaza @ismaelsadeeq you may find this interesting
38. ryanofsky approved
39. 
    ryanofsky commented at 4:26 pm on December 18, 2024: contributor

    Code review ACK c991cea1a0c3ea99dc3e3919789ddf32a338a59d. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message
40. tdb3 approved
41. 
    tdb3 commented at 5:09 pm on December 18, 2024: contributor

    code review ACK c991cea1a0c3ea99dc3e3919789ddf32a338a59d Nice cleanup. Simpler and more concise promotes usage.

    Briefly checked that getblocktemplate and generateblock’s usage of TestBlockValidity() were reverted to what they were (logically) prior to d8a3496b5ad (i.e. https://github.com/bitcoin/bitcoin/commit/ff9039f6ea876bab2c40a06a93e0dd087f445fa2).

    0if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), false, false)) {
    

    0TestBlockValidity(state, chainman.GetParams(), active_chainstate, block, pindexPrev, false, true);
    

    non-blocking nit: if updating again, could update the ProcessNewBlock() comment to increase clarity (but don’t feel strongly about it). Makes it explicit to the user that creating a bool is unneeded and they don’t need to dig into ProcessNewBlock() (and nested) code to see that null is checked.

     0diff --git a/src/validation.h b/src/validation.h
     1index aea7a4621b..a9289deb74 100644
     2--- a/src/validation.h
     3+++ b/src/validation.h
     4@@ -1203,7 +1203,7 @@ public:
     5      *                               (note: only affects headers acceptance; if
     6      *                               block header is already present in block
     7      *                               index then this parameter has no effect)
     8-     * [@param](/bitcoin-bitcoin/contributor/param/)[out]  new_block A boolean which is set to indicate if the block was first received via this call
     9+     * [@param](/bitcoin-bitcoin/contributor/param/)[out]  new_block A boolean which is set to indicate if the block was first received via this call. nullptr can be provided by the caller if not needed.
    10      * [@returns](/bitcoin-bitcoin/contributor/returns/)     If the block was processed, independently of block validity
    11      */
    12     bool ProcessNewBlock(const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked, bool* new_block) LOCKS_EXCLUDED(cs_main);
    

42. ryanofsky merged this on Dec 18, 2024
43. ryanofsky closed this on Dec 18, 2024

    ---

44. 
    ryanofsky commented at 7:47 pm on December 18, 2024: contributor

    Merged this even though tdb3’s suggestion #31196#pullrequestreview-2512419354 could be followed up on. (IMO the suggestion is good but not too important because it’s normal in c++ to allow output pointer parameters to be null. An output parameter that couldn’t be null would usually be a reference.)
45. Sjors deleted the branch on Dec 19, 2024

Contributors
Sjors DrahtBot ryanofsky dergoegge TheCharlatan tdb3