miner: Pass in chainman to RegenerateCommitments

Pass in chainman instead of prev_block so that we can enforce the
block.hashPrevBlock refers to prev_block invariant in the function
itself.

We should probably rethink BlockAssembler's API and somehow include
commitment regeneration functionality in there. Something like a variant
of CreateNewBlock that takes in a std::vector<TxRef> and return a CBlock
instead of CBlockTemplate. That could avoid reaching for
LookupBlockIndex at all.

Author: dongcarl

src/miner.cpp

@@ -39,13 +39,21 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
     return nNewTime - nOldTime;
 }
 
-void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block)
+void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 {
     CMutableTransaction tx{*block.vtx.at(0)};
     tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
     block.vtx.at(0) = MakeTransactionRef(tx);
 
-    WITH_LOCK(::cs_main, assert(g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock) == prev_block));
+    CBlockIndex* prev_block;
+    {
+        // TODO: Temporary scope to check correctness of refactored code.
+        // Should be removed manually after merge of
+        // https://github.com/bitcoin/bitcoin/pull/20158
+        LOCK(::cs_main);
+        assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman));
+        prev_block = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock);
+    }
     GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus());
 
     block.hashMerkleRoot = BlockMerkleRoot(block);

src/miner.h

@@ -203,6 +203,6 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned
 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
 
 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
-void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block);
+void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
 
 #endif // BITCOIN_MINER_H

src/rpc/mining.cpp

@@ -378,8 +378,7 @@ static RPCHelpMan generateblock()
 
     // Add transactions
     block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
-    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
-    RegenerateCommitments(block, prev_block);
+    RegenerateCommitments(block, chainman);
 
     {
         LOCK(cs_main);

src/test/util/setup_common.cpp

@@ -246,8 +246,7 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
     for (const CMutableTransaction& tx : txns) {
         block.vtx.push_back(MakeTransactionRef(tx));
     }
-    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
-    RegenerateCommitments(block, prev_block);
+    RegenerateCommitments(block, *Assert(m_node.chainman));
 
     while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;