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	ryanofsky, 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:

   • #31318 (Drop script_pub_key arg from createNewBlock 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.

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:389 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:1000 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. 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.

    bd6ee7f393
16. Remove getTransactionsUpdated() from mining interface

    #31003 proposes a waitFeesChanged() method which uses this method in its implementation.
    
    It's unnecessary to expose it via this interface.

    1114e14c28
17. 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).

    a67ba64f8f
18. Sjors force-pushed on Nov 10, 2024
19. DrahtBot removed the label CI failed on Nov 10, 2024
20. ryanofsky approved
21. 
    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.
22. TheCharlatan approved
23. 
    TheCharlatan commented at 1:40 pm on November 19, 2024: contributor

    ACK a67ba64f8f63a0f9f2c86d72cf7240bee7c5388c
24. 
    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.

Contributors
Sjors DrahtBot ryanofsky dergoegge TheCharlatan