Skip to content

Commit 0dd7b23

Browse files
author
MarcoFalke
committed
Merge #21391: [Bundle 5/n] Prune g_chainman usage in RPC modules
586190f rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong) bc3bd36 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong) f999139 rpc: Remove unnecessary casting of block height (Carl Dong) 6a3d192 rpc: Tidy up local references (see commit message) (Carl Dong) 038854f rest/rpc: Remove now-unused old Ensure functions (Carl Dong) 6fb65b4 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong) 1570c7e rpc: Add renamed EnsureAny*() functions (Carl Dong) 306b1cd rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong) d7824ac rest: Use existing NodeContext (Carl Dong) 3f08934 rest: Pass in NodeContext to rest_block (Carl Dong) 7be0671 rpc/rawtx: Use existing NodeContext (Carl Dong) 60dc05a rpc/mining: Use existing NodeContext (Carl Dong) d485e81 rpc/blockchain: Use existing NodeContext (Carl Dong) d0abf0b rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong) cced0f4 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong) Pull request description: Overall PR: #20158 (tree-wide: De-globalize ChainstateManager) Based on: - [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules - [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4 Note to reviewers: 1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits) 2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits) 3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so: 1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only** 2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase** 3. Remove `old_function` ACKs for top commit: ryanofsky: Code review ACK 586190f. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated `ActiveChain()` call made in a loop. Thanks for the updates! jnewbery: utACK 586190f MarcoFalke: review ACK 586190f 🍯 Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c
2 parents 4a1751a + 586190f commit 0dd7b23

File tree

9 files changed

+271
-183
lines changed

9 files changed

+271
-183
lines changed

src/miner.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,14 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
3939
return nNewTime - nOldTime;
4040
}
4141

42-
void RegenerateCommitments(CBlock& block, BlockManager& blockman)
42+
void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block)
4343
{
4444
CMutableTransaction tx{*block.vtx.at(0)};
4545
tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
4646
block.vtx.at(0) = MakeTransactionRef(tx);
4747

48-
GenerateCoinbaseCommitment(block, WITH_LOCK(::cs_main, assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman)); return blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());
48+
WITH_LOCK(::cs_main, assert(g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock) == prev_block));
49+
GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus());
4950

5051
block.hashMerkleRoot = BlockMerkleRoot(block);
5152
}

src/miner.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,7 @@ class BlockAssembler
202202
void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
203203
int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
204204

205-
// TODO just accept a CBlockIndex*
206205
/** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
207-
void RegenerateCommitments(CBlock& block, BlockManager& blockman);
206+
void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block);
208207

209208
#endif // BITCOIN_MINER_H

src/rest.cpp

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -181,14 +181,16 @@ static bool rest_headers(const std::any& context,
181181
std::vector<const CBlockIndex *> headers;
182182
headers.reserve(count);
183183
{
184+
ChainstateManager& chainman = EnsureAnyChainman(context);
184185
LOCK(cs_main);
185-
tip = ::ChainActive().Tip();
186-
const CBlockIndex* pindex = g_chainman.m_blockman.LookupBlockIndex(hash);
187-
while (pindex != nullptr && ::ChainActive().Contains(pindex)) {
186+
CChain& active_chain = chainman.ActiveChain();
187+
tip = active_chain.Tip();
188+
const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
189+
while (pindex != nullptr && active_chain.Contains(pindex)) {
188190
headers.push_back(pindex);
189191
if (headers.size() == (unsigned long)count)
190192
break;
191-
pindex = ::ChainActive().Next(pindex);
193+
pindex = active_chain.Next(pindex);
192194
}
193195
}
194196

@@ -232,7 +234,8 @@ static bool rest_headers(const std::any& context,
232234
}
233235
}
234236

235-
static bool rest_block(HTTPRequest* req,
237+
static bool rest_block(const std::any& context,
238+
HTTPRequest* req,
236239
const std::string& strURIPart,
237240
bool showTxDetails)
238241
{
@@ -249,9 +252,10 @@ static bool rest_block(HTTPRequest* req,
249252
CBlockIndex* pblockindex = nullptr;
250253
CBlockIndex* tip = nullptr;
251254
{
255+
ChainstateManager& chainman = EnsureAnyChainman(context);
252256
LOCK(cs_main);
253-
tip = ::ChainActive().Tip();
254-
pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
257+
tip = chainman.ActiveChain().Tip();
258+
pblockindex = chainman.m_blockman.LookupBlockIndex(hash);
255259
if (!pblockindex) {
256260
return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
257261
}
@@ -298,12 +302,12 @@ static bool rest_block(HTTPRequest* req,
298302

299303
static bool rest_block_extended(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
300304
{
301-
return rest_block(req, strURIPart, true);
305+
return rest_block(context, req, strURIPart, true);
302306
}
303307

304308
static bool rest_block_notxdetails(const std::any& context, HTTPRequest* req, const std::string& strURIPart)
305309
{
306-
return rest_block(req, strURIPart, false);
310+
return rest_block(context, req, strURIPart, false);
307311
}
308312

309313
// A bit of a hack - dependency on a function defined in rpc/blockchain.cpp
@@ -537,6 +541,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
537541
std::string bitmapStringRepresentation;
538542
std::vector<bool> hits;
539543
bitmap.resize((vOutPoints.size() + 7) / 8);
544+
ChainstateManager& chainman = EnsureAnyChainman(context);
540545
{
541546
auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) {
542547
for (const COutPoint& vOutPoint : vOutPoints) {
@@ -552,12 +557,12 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
552557
if (!mempool) return false;
553558
// use db+mempool as cache backend in case user likes to query mempool
554559
LOCK2(cs_main, mempool->cs);
555-
CCoinsViewCache& viewChain = ::ChainstateActive().CoinsTip();
560+
CCoinsViewCache& viewChain = chainman.ActiveChainstate().CoinsTip();
556561
CCoinsViewMemPool viewMempool(&viewChain, *mempool);
557562
process_utxos(viewMempool, *mempool);
558563
} else {
559564
LOCK(cs_main); // no need to lock mempool!
560-
process_utxos(::ChainstateActive().CoinsTip(), CTxMemPool());
565+
process_utxos(chainman.ActiveChainstate().CoinsTip(), CTxMemPool());
561566
}
562567

563568
for (size_t i = 0; i < hits.size(); ++i) {
@@ -572,7 +577,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
572577
// serialize data
573578
// use exact same output as mentioned in Bip64
574579
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
575-
ssGetUTXOResponse << ::ChainActive().Height() << ::ChainActive().Tip()->GetBlockHash() << bitmap << outs;
580+
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
576581
std::string ssGetUTXOResponseString = ssGetUTXOResponse.str();
577582

578583
req->WriteHeader("Content-Type", "application/octet-stream");
@@ -582,7 +587,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
582587

583588
case RetFormat::HEX: {
584589
CDataStream ssGetUTXOResponse(SER_NETWORK, PROTOCOL_VERSION);
585-
ssGetUTXOResponse << ::ChainActive().Height() << ::ChainActive().Tip()->GetBlockHash() << bitmap << outs;
590+
ssGetUTXOResponse << chainman.ActiveChain().Height() << chainman.ActiveChain().Tip()->GetBlockHash() << bitmap << outs;
586591
std::string strHex = HexStr(ssGetUTXOResponse) + "\n";
587592

588593
req->WriteHeader("Content-Type", "text/plain");
@@ -595,8 +600,8 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
595600

596601
// pack in some essentials
597602
// use more or less the same output as mentioned in Bip64
598-
objGetUTXOResponse.pushKV("chainHeight", ::ChainActive().Height());
599-
objGetUTXOResponse.pushKV("chaintipHash", ::ChainActive().Tip()->GetBlockHash().GetHex());
603+
objGetUTXOResponse.pushKV("chainHeight", chainman.ActiveChain().Height());
604+
objGetUTXOResponse.pushKV("chaintipHash", chainman.ActiveChain().Tip()->GetBlockHash().GetHex());
600605
objGetUTXOResponse.pushKV("bitmap", bitmapStringRepresentation);
601606

602607
UniValue utxos(UniValue::VARR);
@@ -639,11 +644,13 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
639644

640645
CBlockIndex* pblockindex = nullptr;
641646
{
647+
ChainstateManager& chainman = EnsureAnyChainman(context);
642648
LOCK(cs_main);
643-
if (blockheight > ::ChainActive().Height()) {
649+
const CChain& active_chain = chainman.ActiveChain();
650+
if (blockheight > active_chain.Height()) {
644651
return RESTERR(req, HTTP_NOT_FOUND, "Block height out of range");
645652
}
646-
pblockindex = ::ChainActive()[blockheight];
653+
pblockindex = active_chain[blockheight];
647654
}
648655
switch (rf) {
649656
case RetFormat::BINARY: {

0 commit comments

Comments
 (0)