Remove buggy and confusing IncrementExtraNonce #24732

1. 
   MarcoFalke commented at 8:27 am on April 1, 2022: member

   IncrementExtraNonce has many issues:

   • It is test-only code, but part of bitcoind
   • It is using the block height of the tip, as opposed to the block’s previous block as reference for the new height. See #24730 (comment)
   • It has no use case in regtest testing. With a low difficulty the extra nonce won’t be incremented. With a high difficulty the test-only functions are clumsy to handle anyway. For example, the generate* RPCs will return an empty array once they reached maxtries, as opposed to an error. Also the calls can’t be aborted early unless the node shuts down completely. So I think it is fine to just remove the extra nonce functionality and leave it to the outside to implement, if needed. For example, a wrapper script can call the generate* RPCs once every second, to use the timestamp as extra nonce.
2. MarcoFalke added the label Refactoring on Apr 1, 2022
3. MarcoFalke added the label Mining on Apr 1, 2022
4. MarcoFalke force-pushed on Apr 1, 2022
5. Remove buggy and confusing IncrementExtraNonce fa38b1c8bd
6. MarcoFalke force-pushed on Apr 1, 2022
7. 
   laanwj commented at 9:56 am on April 1, 2022: member

   Concept ACK, nice cleanup.
8. 
   ajtowns commented at 11:06 am on April 1, 2022: contributor

   Concept ACK
9. 
   theStack commented at 12:15 pm on April 1, 2022: contributor

   Concept ACK
10. 
    DrahtBot commented at 1:32 pm on April 1, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24629 (Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block by luke-jr)
    • #24202 (rpc: allow dumptxoutset to dump human-readable data by w0xlt)
    • #21726 (Improve Indices on pruned nodes via prune blockers by fjahr)
    • #18933 (rpc: Add submit option to generateblock by MarcoFalke)

    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.

11. 
    laanwj commented at 10:50 am on April 4, 2022: member

    Code review ACK fa38b1c8bd29e2c792737f6481ab928e46396b7e
12. in src/rpc/mining.cpp:157 in fa38b1c8bd outdated

    152@@ -157,7 +153,6 @@ static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& me
    153         nHeight = chainman.ActiveChain().Height();
    154         nHeightEnd = nHeight+nGenerate;
    155     }
    156-    unsigned int nExtraNonce = 0;
    157     UniValue blockHashes(UniValue::VARR);
    158     while (nHeight < nHeightEnd && !ShutdownRequested())
    

    ---

    ---

    

    ajtowns commented at 2:21 am on April 5, 2022:

    Is there any reason to keep nHeight and nHeightEnd here?

     0@@ -145,16 +145,8 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
     1 
     2 static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& mempool, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
     3 {
     4-    int nHeightEnd = 0;
     5-    int nHeight = 0;
     6-
     7-    {   // Don't keep cs_main locked
     8-        LOCK(cs_main);
     9-        nHeight = chainman.ActiveChain().Height();
    10-        nHeightEnd = nHeight+nGenerate;
    11-    }
    12     UniValue blockHashes(UniValue::VARR);
    13-    while (nHeight < nHeightEnd && !ShutdownRequested())
    14+    while (nGenerate > 0 && !ShutdownRequested())
    15     {
    16         std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(chainman.ActiveChainstate(), mempool, Params()).CreateNewBlock(coinbase_script));
    17         if (!pblocktemplate.get())
    18@@ -167,7 +159,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& me
    19         }
    20 
    21         if (!block_hash.IsNull()) {
    22-            ++nHeight;
    23+            --nGenerate;
    24             blockHashes.push_back(block_hash.GetHex());
    25         }
    26     }
    

    ---

    

    MarcoFalke commented at 11:05 am on April 5, 2022:

    thanks, removed
13. 
    ajtowns commented at 4:05 am on April 5, 2022: contributor

    ACK fa38b1c8bd29e2c792737f6481ab928e46396b7e
14. Remove nHeightEnd and nHeight in generateBlocks helper cccc4e879a
15. 
    MarcoFalke commented at 11:05 am on April 5, 2022: member

    Removed more code
16. 
    ajtowns commented at 9:08 am on April 6, 2022: contributor

    ACK cccc4e879a8cb9d858a88ea46b28ea27ab79ca55
17. fanquake requested review from laanwj on Apr 6, 2022
18. MarcoFalke merged this on Apr 6, 2022
19. MarcoFalke closed this on Apr 6, 2022

    ---

20. MarcoFalke deleted the branch on Apr 6, 2022
21. 
    Sjors commented at 7:21 pm on August 6, 2022: member

    The RPC should probably throw an error if maxtries >= 2^32.
22. 
    MarcoFalke commented at 7:33 pm on August 6, 2022: member

    Yeah, and also when max_tries==0, as explained in the pull request description
23. bitcoinhodler referenced this in commit f8d7b772c3 on Oct 27, 2022
24. bitcoinhodler referenced this in commit cc3dfa67e6 on Oct 27, 2022
25. bitcoinhodler referenced this in commit 87d02900d3 on Oct 28, 2022
26. Fabcien referenced this in commit 00a6ea72f7 on Jul 27, 2023
27. bitcoin locked this on Aug 6, 2023

Contributors
MarcoFalke laanwj ajtowns theStack DrahtBot Sjors

Review Requested
laanwj

Labels
Refactoring Mining