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
validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics} 
Tip: versionbitscache is currently a global so we didn't need to pass it
     in to any of ::VersionBitsTip*'s callers
  • Loading branch information
@dongcarl
dongcarl committed Feb 2, 2021
commit a756ecb7bcf3ddc2ff86d2005b6556668f47f988
6 changes: 3 additions & 3 deletions src/rpc/blockchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,7 +1236,7 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
if (consensusParams.vDeployments[id].nTimeout <= 1230768000) return;

UniValue bip9(UniValue::VOBJ);
const ThresholdState thresholdState = VersionBitsTipState(consensusParams, id);
const ThresholdState thresholdState = VersionBitsState(::ChainActive().Tip(), consensusParams, id, versionbitscache);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a friendly comment in commit about the fact that versionbitscache is currently a global and thus the stays the same. Though we can't assert it ?

switch (thresholdState) {
case ThresholdState::DEFINED: bip9.pushKV("status", "defined"); break;
case ThresholdState::STARTED: bip9.pushKV("status", "started"); break;
Expand All @@ -1250,12 +1250,12 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
}
bip9.pushKV("start_time", consensusParams.vDeployments[id].nStartTime);
bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
int64_t since_height = VersionBitsTipStateSinceHeight(consensusParams, id);
int64_t since_height = VersionBitsStateSinceHeight(::ChainActive().Tip(), consensusParams, id, versionbitscache);
bip9.pushKV("since", since_height);
if (ThresholdState::STARTED == thresholdState)
{
UniValue statsUV(UniValue::VOBJ);
BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
BIP9Stats statsStruct = VersionBitsStatistics(::ChainActive().Tip(), consensusParams, id);
statsUV.pushKV("period", statsStruct.period);
statsUV.pushKV("threshold", statsStruct.threshold);
statsUV.pushKV("elapsed", statsStruct.elapsed);
Expand Down
18 changes: 0 additions & 18 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4973,24 +4973,6 @@ CBlockFileInfo* GetBlockFileInfo(size_t n)
return &vinfoBlockFile.at(n);
}

ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::DeploymentPos pos)
{
LOCK(cs_main);
Copy link
Contributor

Choose a reason for hiding this comment

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

In commit "validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}" (b60c7e5)

I'm a little unclear why it's safe to drop these locks, or why they were here to begin with, or if VersionBits or ChainActive or Tip functions should have been annotated to require cs_main. It seems like something should have be annotated if cs_main was required here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Digging in the git history a bit, it seems that the earliest of this trio -- namely VersionBitsTipState -- was introduced in d23f6c6. My hypothesis is that the LOCK(cs_main) was added in order to stop the chain from advancing and make sure that the caller got info on the actual tip. However, it seems like a AssertLockHeld(cs_main); annotation would have been much more suitable in that case (and existed in the codebase at d23f6c6).

I'm not entirely sure about the original intent here, so perhaps @sipa can enlighten me and make sure I'm not missing something!

Copy link
Contributor

Choose a reason for hiding this comment

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

At least add a comment about the fact the cs_main lock is already taken in this code path L1317 (src/rpc/blockchain.cpp) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

re: #20158 (comment)

In commit "validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}" (ff894f9)

Digging in the git history a bit, it seems that the earliest of this trio -- namely VersionBitsTipState -- was introduced in d23f6c6.

Thanks for checking! I see EXCLUSIVE_LOCKS_REQUIRED(cs_main) is used where the new code is called so there should not actually be a concern here.

At least add a comment about the fact the cs_main lock is already taken in this code path L1317 (src/rpc/blockchain.cpp) ?

Line 1317 seems to be referring to LOCK(cs_main) getblockchaininfo. Writing a comment referencing a specific lock in a specific calling function seems a little fragile, and hopefully unnecessary with EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ariard @ryanofsky would it be preferable to add a EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation to VersionBitsTip{State,SinceHeight,Statistics} here just in case they get new callers in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

re: #20158 (comment)

In commit "validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}" (6ed743d)

@ariard @ryanofsky would it be preferable to add a EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation to VersionBitsTip{State,SinceHeight,Statistics} here just in case they get new callers in the future?

I'd keep what you have now unless there's a reason to think conclusion from digging into history is incorrect.

It would be confusing to add a lock required annotation if we don't think a lock is required. In theory you could write EXCLUSIVE_LOCKS_REQUIRED(cs_main) with a comment "There is no known reason to think a lock is required to call this function, but the original code had an explicit recursive lock, so locking is stilll required to be conservative in case there is an unknown reason locking is needed." This would probably be overkill though.

return VersionBitsState(::ChainActive().Tip(), params, pos, versionbitscache);
}

BIP9Stats VersionBitsTipStatistics(const Consensus::Params& params, Consensus::DeploymentPos pos)
{
LOCK(cs_main);
return VersionBitsStatistics(::ChainActive().Tip(), params, pos);
}

int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::DeploymentPos pos)
{
LOCK(cs_main);
return VersionBitsStateSinceHeight(::ChainActive().Tip(), params, pos, versionbitscache);
}

static const uint64_t MEMPOOL_DUMP_VERSION = 1;

bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate)
Expand Down
9 changes: 0 additions & 9 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,15 +188,6 @@ bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxVali
std::list<CTransactionRef>* plTxnReplaced,
bool bypass_limits, bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

/** Get the BIP9 state for a given deployment at the current tip. */
ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::DeploymentPos pos);

/** Get the numerical statistics for the BIP9 state for a given deployment at the current tip. */
BIP9Stats VersionBitsTipStatistics(const Consensus::Params& params, Consensus::DeploymentPos pos);

/** Get the block height at which the BIP9 deployment switched into the state for the block building on the current tip. */
int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::DeploymentPos pos);


/** Apply the effects of this transaction on the UTXO set represented by view */
void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, int nHeight);
Expand Down
3 changes: 3 additions & 0 deletions src/versionbits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,11 @@ struct VersionBitsCache
void Clear();
};

/** Get the BIP9 state for a given deployment at the current tip. */
ThresholdState VersionBitsState(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos, VersionBitsCache& cache);
/** Get the numerical statistics for the BIP9 state for a given deployment at the current tip. */
BIP9Stats VersionBitsStatistics(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos);
/** Get the block height at which the BIP9 deployment switched into the state for the block building on the current tip. */
int VersionBitsStateSinceHeight(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos, VersionBitsCache& cache);
uint32_t VersionBitsMask(const Consensus::Params& params, Consensus::DeploymentPos pos);

Expand Down