Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tree-wide: De-globalize ChainstateManager #20158

Closed
wants to merge 70 commits into from
+668 −660
Closed

tree-wide: De-globalize ChainstateManager #20158

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
333d823
validation: Pass in chainstate to ::LimitMempoolSize
dongcarl Sep 16, 2020
7f51417
validation: Pass in chainstate to IsCurrentForFeeEstimation
dongcarl Sep 16, 2020
e83c853
validation: Pass in chainstate to CheckInputsFromMempoolAndCache
dongcarl Sep 16, 2020
e5198ef
validation: Pass in chain tip to ::CheckFinalTx
dongcarl Sep 16, 2020
6e0f4ae
scripted-diff: Invoke ::CheckFinalTx with chain tip
dongcarl Sep 16, 2020
3404ac0
validation: Remove old CheckFinalTx w/o chain tip param
dongcarl Dec 18, 2020
76e0811
validation: Pass in chainstate to ::CheckSequenceLocks
dongcarl Sep 16, 2020
dd7e989
validation: Add chainstate member to MemPoolAccept
dongcarl Sep 16, 2020
7d90cf9
validation: Pass in chainstate to AcceptToMemoryPoolWithTime
dongcarl Sep 16, 2020
cffd76a
validation: Pass in chainstate to ::LoadMempool
dongcarl Sep 16, 2020
314c92b
validation: Pass in chainstate to ::AcceptToMemoryPool
dongcarl Sep 16, 2020
8b65ddd
scripted-diff: Invoke ::AcceptToMemoryPool with chainstate
dongcarl Sep 16, 2020
85f9844
validation: Remove old AcceptToMemoryPool w/o chainstate param
dongcarl Dec 18, 2020
d722f7d
tree-wide: Fix erroneous AcceptToMemoryPool replacements
dongcarl Oct 15, 2020
b9185dc
validation: Pass in chain to ::TestLockPointValidity
dongcarl Sep 16, 2020
28dd955
validation: Pass in chainstate to CTxMemPool::removeForReorg
dongcarl Sep 16, 2020
5203b85
validation: Pass in chainstate to UpdateMempoolForReorg
dongcarl Sep 16, 2020
64db9d6
validation: Use *this in CChainState::LoadMempool
dongcarl Sep 17, 2020
107a028
COMMITS AFTER THIS ARE NON-BASE
dongcarl Dec 22, 2020
eb8fff3
validation: Remove global ::LoadGenesisBlock
dongcarl Aug 26, 2020
7b76fdb
validation: Remove global ::{{Precious,Invalidate}Block,ResetBlockFai…
dongcarl Sep 15, 2020
403c5cb
validation: Pass in chainstate to UpdateTip
dongcarl Sep 15, 2020
081631e
validation: Pass in chainstate to ::PruneBlockFilesManual
dongcarl Sep 15, 2020
a756ecb
validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}
dongcarl Sep 15, 2020
bcf23b5
validation: Pass in chainstate to CVerifyDB::VerifyDB
dongcarl Sep 15, 2020
61a45ad
validation: Move invalid block handling to CChainState
dongcarl Oct 5, 2020
fa730f7
validation: Move LoadBlockIndexDB to CChainState
dongcarl Aug 26, 2020
6b7cf5d
validation: Use *this in CChainState::InvalidateBlock
dongcarl Sep 17, 2020
a765747
validation: Pass in spendheight to CTxMemPool::check
dongcarl Sep 9, 2020
4582551
validation: Use *this in CChainState::ActivateBestChainStep
dongcarl Sep 17, 2020
55a2f2c
validation: Pass in chain to FindBlockPos+SaveBlockToDisk
dongcarl Oct 6, 2020
1886642
validation: Use existing chain member in CChainState::AcceptBlock
dongcarl Oct 6, 2020
f5ee2f3
validation: Use existing chain member in CChainState::LoadGenesisBlock
dongcarl Oct 6, 2020
15b6e6a
COMMITS AFTER THIS ARE NON-BASE
dongcarl Dec 22, 2020
f80378c
miner: Pass in chainstate to BlockAssembler::CreateNewBlock
dongcarl Nov 5, 2020
363d6d5
scripted-diff: Invoke CreateNewBlock with chainstate
dongcarl Nov 5, 2020
ff53263
miner: Remove old CreateNewBlock w/o chainstate param
dongcarl Dec 18, 2020
4620ba1
miner: Pass in blockman to ::RegenerateCommitments
dongcarl Oct 6, 2020
05083dc
node/coinstats: Pass in BlockManager to GetUTXOStats
dongcarl Sep 1, 2020
973fbe8
node: Use existing NodeContext
dongcarl Oct 14, 2020
b652f40
node/ifaces: NodeImpl: Use existing NodeContext member
dongcarl Dec 18, 2020
1bd97da
node/ifaces: ChainImpl: Use existing NodeContext member
dongcarl Dec 18, 2020
2ad7239
net_processing: Move some static functions to PeerManager
dongcarl Sep 22, 2020
4328d09
scripted-diff: net_processing: Use existing chainman
dongcarl Oct 14, 2020
9890464
net_processing: Add review-only assertion to PeerManager
dongcarl Oct 15, 2020
fa970fb
COMMITS AFTER THIS ARE NON-BASE
dongcarl Dec 22, 2020
8919757
rpc/*,rest: Add review-only assertion to EnsureChainman
dongcarl Oct 15, 2020
1ac12e9
rpc/blockchain: Use existing NodeContext
dongcarl Oct 14, 2020
2cb0446
rpc/mining: Use existing NodeContext
dongcarl Oct 14, 2020
a910e41
rpc/rawtx: Use existing NodeContext
dongcarl Oct 14, 2020
0be47f8
rest: Pass in NodeContext to rest_block
dongcarl Sep 2, 2020
3a2a8cf
rest: Use existing NodeContext
dongcarl Oct 14, 2020
2acbe46
fixup! rpc/mining: Use existing NodeContext
dongcarl Jan 26, 2021
711f37a
COMMITS AFTER THIS ARE NON-BASE
dongcarl Dec 22, 2020
409ebc3
bench: Use existing NodeContext in DuplicateInputs
dongcarl Sep 9, 2020
053893a
bench: Use existing chainman in AssembleBlock
dongcarl Sep 18, 2020
b476400
index: Add chainstate member to BaseIndex
dongcarl Oct 14, 2020
9cac0b0
COMMITS AFTER THIS ARE NON-BASE
dongcarl Dec 22, 2020
e391187
init: Use existing chainman
dongcarl Aug 26, 2020
a7461a5
test/util: Use existing chainman in ::PrepareBlock
dongcarl Oct 14, 2020
755ceea
test/miner_tests: Pass in chain tip to CreateBlockIndex
dongcarl Oct 14, 2020
b38c3c9
test: Pass in CoinsTip to ValidateCheckInputsForAllFlags
dongcarl Sep 9, 2020
3d54028
scripted-diff: test: Use existing chainman in unit tests
dongcarl Sep 9, 2020
c3efbcb
fuzz: Initialize a TestingSetup for test_one_input
dongcarl Oct 27, 2020
b24576e
scripted-diff: wallet/test: Use existing chainman
dongcarl Sep 18, 2020
947b3c5
qt/test: Use existing chainman in ::TestGUI (can be scripted-diff)
dongcarl Sep 18, 2020
a3d401c
miner: Remove stray review-only assertion
dongcarl Nov 4, 2020
9b3925b
scripted-diff: tree-wide: Remove all review-only assertions
dongcarl Oct 6, 2020
62893af
qt/test: Reset chainman in ~ChainstateManager instead
dongcarl Oct 6, 2020
6bda9eb
validation: Farewell, global Chainstate!
dongcarl Oct 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
miner: Pass in blockman to ::RegenerateCommitments 
REQUIRES ATTENTION
  • Loading branch information
@dongcarl
dongcarl committed Feb 2, 2021
commit 4620ba194acc269681168497144a9952d8e0fe47
4 changes: 2 additions & 2 deletions src/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
return nNewTime - nOldTime;
}

void RegenerateCommitments(CBlock& block)
void RegenerateCommitments(CBlock& block, BlockManager& blockman)
{
CMutableTransaction tx{*block.vtx.at(0)};
tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
block.vtx.at(0) = MakeTransactionRef(tx);

GenerateCoinbaseCommitment(block, WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());
GenerateCoinbaseCommitment(block, WITH_LOCK(::cs_main, assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman)); return blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());

block.hashMerkleRoot = BlockMerkleRoot(block);
}
Expand Down
2 changes: 1 addition & 1 deletion src/miner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,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);
void RegenerateCommitments(CBlock& block, BlockManager& blockman);

#endif // BITCOIN_MINER_H
2 changes: 1 addition & 1 deletion src/rpc/mining.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ static RPCHelpMan generateblock()

// Add transactions
block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
RegenerateCommitments(block);
RegenerateCommitments(block, WITH_LOCK(::cs_main, return std::ref(g_chainman.m_blockman)));

{
LOCK(cs_main);
Expand Down
2 changes: 1 addition & 1 deletion src/test/util/setup_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
for (const CMutableTransaction& tx : txns) {
block.vtx.push_back(MakeTransactionRef(tx));
}
RegenerateCommitments(block);
RegenerateCommitments(block, WITH_LOCK(::cs_main, return std::ref(g_chainman.m_blockman)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pass directly the result of LookupBlockIndex(block.hashPrevBlock) ? Block is already known here. Because AFAIU, the lock isn't held once RegenerateCommitment argument are loaded on the stack thus do you have guarantee you're calling the new blockman.LookupBlockIndex() with a lock ?

I think so otherwise LookupBlockIndex's lock annotation would yelled but I can't convince myself of it...

Copy link
Contributor Author

@dongcarl dongcarl Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is a shitty situation where the lock in this diff is only needed because:

bitcoin/src/validation.h

Line 944 in 831675c

extern ChainstateManager g_chainman GUARDED_BY(::cs_main);

and

bitcoin/src/validation.h

Line 840 in 831675c

BlockManager m_blockman GUARDED_BY(::cs_main);

Then the lock is released as RegenerateCommitments's assembly function prologue is run, and the same lock is taken again when LookupBlockIndex is called inside RegenerateCommitments.

The original reason why I wanted to just pass in the blockman is because I didn't want to impose the constraint of "arg 2 must be the block index of arg 1" on RegenerateCommitments's arguments, which seems like something that might be easy to cause a bug if someone were to work with multiple blocks or multiple block indices. Perhaps adequate documentation can prevent this?


while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;

Expand Down