[Mining] IncrementExtraNonce without cs_main

Changes the signature of IncrementExtraNonce:
Of the values used by IncrementExtraNonce, cs_main only protects the
chain tip; the rest are entirely local to the thread.
The chain tip, in turn, is only used for getting the Height of the tip.
Thus, we can hold cs_main only to get the Height, then perform the
work without holding the lock.
(Noting that not all accesses to `chainActive.Height()` are locked,
so if it's safe, we could remove this usage of cs_main entirely.)

Removes one extraneous copy of the nonce into a thread local variable
(there were previously two, and I suspect the extra was already being
optimized away).

Author: Zannick

src/miner.cpp

@@ -814,7 +814,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
     }
 }
 
-void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce)
+void IncrementExtraNonce(CBlock* pblock, unsigned int nHeight, unsigned int& nExtraNonce)
 {
     // Update nExtraNonce
     static uint256 hashPrevBlock;
@@ -824,9 +824,9 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned
         hashPrevBlock = pblock->hashPrevBlock;
     }
     ++nExtraNonce;
-    unsigned int nHeight = pindexPrev->nHeight+1; // Height first in coinbase required for block.version=2
     CMutableTransaction txCoinbase(*pblock->vtx[0]);
-    txCoinbase.vin[0].scriptSig = (CScript() << nHeight << CScriptNum(nExtraNonce)) + COINBASE_FLAGS;
+    // Height first in coinbase required for block.version=2
+    txCoinbase.vin[0].scriptSig = (CScript() << (nHeight+1) << CScriptNum(nExtraNonce)) + COINBASE_FLAGS;
     assert(txCoinbase.vin[0].scriptSig.size() <= 100);
 
     pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase));
@@ -964,24 +964,25 @@ void BitcoinMiner(std::shared_ptr<CReserveScript> coinbaseScript, bool fProofOfS
         }
 
         CBlock *pblock = &pblocktemplate->block;
-        int32_t nNonceLocal;
 
         if (!fProofOfStake)
         {
             {
                 LOCK(cs_nonce);
-                nNonceLocal = nNonce_base++;
+                nExtraNonce = nNonce_base++;
                 if (!nTimeStart)
                     nTimeStart = GetTime();
             }
 
             pblock->nNonce = 0;
             pblock->nNonce64 = rand();
+            // Avoid holding cs_main during IncrementExtraNonce which doesn't require it.
+            unsigned int nHeight;
             {
                 LOCK(cs_main);
-                nExtraNonce = nNonceLocal;
-                IncrementExtraNonce(pblock, chainActive.Tip(), nExtraNonce);
+                nHeight = chainActive.Height();
             }
+            IncrementExtraNonce(pblock, nHeight, nExtraNonce);
 
             int nTries = 0;
             if (pblock->IsProgPow() && pblock->nTime >= Params().PowUpdateTimestamp()) {
@@ -1073,21 +1074,22 @@ void BitcoinRandomXMiner(std::shared_ptr<CReserveScript> coinbaseScript, int vm_
             continue;
 
         CBlock *pblock = &pblocktemplate->block;
-        int32_t nNonceLocal;
 
         {
             LOCK(cs_nonce);
-            nNonceLocal = nNonce_base++;
+            nExtraNonce = nNonce_base++;
             if (!nTimeStart)
                 nTimeStart = GetTime();
         }
 
         pblock->nNonce = startNonce;
+        // Avoid holding cs_main during IncrementExtraNonce which doesn't require it.
+        unsigned int nHeight;
         {
             LOCK(cs_main);
-            nExtraNonce = nNonceLocal;
-            IncrementExtraNonce(pblock, chainActive.Tip(), nExtraNonce);
+            nHeight = chainActive.Height();
         }
+        IncrementExtraNonce(pblock, nHeight, nExtraNonce);
 
         int nTries = 0;
         if (pblock->IsRandomX() && pblock->nTime >= Params().PowUpdateTimestamp()) {

src/miner.h

@@ -247,7 +247,7 @@ bool GenerateActive();
 void setGenerate(bool fGenerate);
 
 /** Modify the extranonce in a block */
-void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
+void IncrementExtraNonce(CBlock* pblock, unsigned int nHeight, unsigned int& nExtraNonce);
 int64_t UpdateTime(CBlock* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
 void GenerateBitcoins(bool fGenerate, int nThreads, std::shared_ptr<CReserveScript> coinbaseScript);
 void ThreadStakeMiner();

src/rpc/mining.cpp

@@ -220,10 +220,13 @@ UniValue generateBlocks(std::shared_ptr<CReserveScript> coinbaseScript, int nGen
         if (!pblocktemplate.get())
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
         CBlock *pblock = &pblocktemplate->block;
+        // Avoid holding cs_main during IncrementExtraNonce
+        unsigned int nChainHeight;
         {
             LOCK(cs_main);
-            IncrementExtraNonce(pblock, chainActive.Tip(), nExtraNonce);
+            nChainHeight = chainActive.Height();
         }
+        IncrementExtraNonce(pblock, nChainHeight, nExtraNonce);
 
         // This will check if the key block needs to change and will take down the cache and vm, and spin up the new ones
         CheckIfValidationKeyShouldChangeAndUpdate(GetKeyBlock(pblock->nHeight));

src/test/test_veil.cpp

@@ -155,11 +155,14 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransaction>&
     for (const CMutableTransaction& tx : txns)
         block.vtx.push_back(MakeTransactionRef(tx));
     // IncrementExtraNonce creates a valid coinbase and merkleRoot
+    // Avoid holding cs_main during IncrementExtraNonce
+    unsigned int nHeight;
     {
         LOCK(cs_main);
-        unsigned int extraNonce = 0;
-        IncrementExtraNonce(&block, chainActive.Tip(), extraNonce);
+        nHeight = chainActive.Height();
     }
+    unsigned int extraNonce = 0;
+    IncrementExtraNonce(&block, nHeight, extraNonce);
 
     while (!CheckProofOfWork(block.GetX16RTPoWHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;