From e6b4aa6eb53dc555ecab2922af35e7a2572faf4f Mon Sep 17 00:00:00 2001
From: Carl Dong <contact@carldong.me>
Date: Fri, 23 Apr 2021 15:24:47 -0400
Subject: 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.
---
 src/miner.cpp                  | 12 ++++++++++--
 src/miner.h                    |  2 +-
 src/rpc/mining.cpp             |  3 +--
 src/test/util/setup_common.cpp |  3 +--
 4 files changed, 13 insertions(+), 7 deletions(-)

(limited to 'src')

diff --git a/src/miner.cpp b/src/miner.cpp
index 3bc7fdd458..eccddbb04f 100644
--- a/src/miner.cpp
+++ b/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);
diff --git a/src/miner.h b/src/miner.h
index becf362b79..10a80f4392 100644
--- a/src/miner.h
+++ b/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
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 8190a2f006..6826e6fd07 100644
--- a/src/rpc/mining.cpp
+++ b/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);
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index f92e4c4b99..f53c505e7c 100644
--- a/src/test/util/setup_common.cpp
+++ b/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;
 
-- 
cgit v1.2.3