From 0253438503c156ecfa8f4074a67c8c0c2ade6a62 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 19 Sep 2024 00:55:14 +0700 Subject: [PATCH 01/69] feat: new fork WITHDRAWALS introduced --- src/chainparams.cpp | 36 +++++++++++++++++++++++++++++++ src/consensus/params.h | 1 + src/deploymentinfo.cpp | 4 ++++ src/rpc/blockchain.cpp | 1 + test/functional/rpc_blockchain.py | 11 ++++++++++ 5 files changed, 53 insertions(+) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index e041149cd1..d551dfeea0 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -209,6 +209,15 @@ class CMainParams : public CChainParams { consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1728864000; // October 14, 2024 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = 1760400000; // October 14, 2025 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 4032; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 3226; // 80% of 4032 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 2420; // 60% of 4032 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5; // this corresponds to 10 periods + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true; + // The best chain should have at least this much work. consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000988117deadb0db9cd5b8"); // 2109672 @@ -395,6 +404,15 @@ class CTestNetParams : public CChainParams { consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1728864000; // October 14, 2024 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 100; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 80; // 80% of 100 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 60; // 60% of 100 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5; // this corresponds to 10 periods + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true; + // The best chain should have at least this much work. consensus.nMinimumChainWork = uint256S("0x000000000000000000000000000000000000000000000000031779704a0f54b4"); // 1069875 @@ -556,6 +574,15 @@ class CDevNetParams : public CChainParams { consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1704067200; // January 1, 2024 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 120; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 80; // 80% of 100 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 60; // 60% of 100 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5; // this corresponds to 10 periods + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true; + // The best chain should have at least this much work. consensus.nMinimumChainWork = uint256S("0x000000000000000000000000000000000000000000000000000000000000000"); @@ -782,6 +809,15 @@ class CRegTestParams : public CChainParams { consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 0; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 300; + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 300 / 5 * 4; // 80% of 12 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 300 / 5 * 3; // 60% of 7 + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5; // this corresponds to 10 periods + consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true; + // The best chain should have at least this much work. consensus.nMinimumChainWork = uint256S("0x00"); diff --git a/src/consensus/params.h b/src/consensus/params.h index 95918aa6db..508d477c54 100644 --- a/src/consensus/params.h +++ b/src/consensus/params.h @@ -39,6 +39,7 @@ constexpr bool ValidDeployment(BuriedDeployment dep) { return dep <= DEPLOYMENT_ enum DeploymentPos : uint16_t { DEPLOYMENT_TESTDUMMY, + DEPLOYMENT_WITHDRAWALS, // Deployment of Fix for quorum selection for withdrawals // NOTE: Also add new deployments to VersionBitsDeploymentInfo in deploymentinfo.cpp MAX_VERSION_BITS_DEPLOYMENTS }; diff --git a/src/deploymentinfo.cpp b/src/deploymentinfo.cpp index 98ba9b3652..6ea50a3d47 100644 --- a/src/deploymentinfo.cpp +++ b/src/deploymentinfo.cpp @@ -11,6 +11,10 @@ const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_B /*.name =*/ "testdummy", /*.gbt_force =*/ true, }, + { + /*.name =*/"withdrawals", + /*.gbt_force =*/true, + }, }; std::string DeploymentName(Consensus::BuriedDeployment dep) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 6bf41d7ba8..defe3fb34e 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1841,6 +1841,7 @@ RPCHelpMan getblockchaininfo() SoftForkDescPushBack(tip, softforks, consensusParams, Consensus::DEPLOYMENT_V19); SoftForkDescPushBack(tip, softforks, consensusParams, Consensus::DEPLOYMENT_V20); SoftForkDescPushBack(tip, softforks, consensusParams, Consensus::DEPLOYMENT_MN_RR); + SoftForkDescPushBack(tip, ehfSignals, softforks, consensusParams, Consensus::DEPLOYMENT_WITHDRAWALS); SoftForkDescPushBack(tip, ehfSignals, softforks, consensusParams, Consensus::DEPLOYMENT_TESTDUMMY); obj.pushKV("softforks", softforks); diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index a36e881c68..a327a630aa 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -157,6 +157,17 @@ def _test_getblockchaininfo(self): 'v19': { 'type': 'buried', 'active': False, 'height': 900}, 'v20': { 'type': 'buried', 'active': False, 'height': 900}, 'mn_rr': { 'type': 'buried', 'active': False, 'height': 900}, + 'withdrawals': { + 'type': 'bip9', + 'bip9': { + 'status': 'defined', + 'start_time': 0, + 'timeout': 9223372036854775807, # testdummy does not have a timeout so is set to the max int64 value + 'since': 0, + 'min_activation_height': 0, + 'ehf': True + }, + 'active': False}, 'testdummy': { 'type': 'bip9', 'bip9': { From 27040030e9b7c78ca2197bc8b653d483d7589ced Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 18 Sep 2024 19:46:45 +0700 Subject: [PATCH 02/69] fix: using proper quorums for asset unlock validation --- src/evo/assetlocktx.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/evo/assetlocktx.cpp b/src/evo/assetlocktx.cpp index ccbb0042e0..64ee2b32c4 100644 --- a/src/evo/assetlocktx.cpp +++ b/src/evo/assetlocktx.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -113,13 +114,18 @@ bool CAssetUnlockPayload::VerifySig(const llmq::CQuorumManager& qman, const uint // and at the quorumHash must be active in either the current or previous quorum cycle // and the sig must validate against that specific quorumHash. + Consensus::LLMQType llmqType = Params().GetConsensus().llmqTypePlatform; - // We check at most 2 quorums - const auto quorums = qman.ScanQuorums(llmqType, pindexTip, 2); + const auto& llmq_params_opt = Params().GetLLMQ(llmqType); + assert(llmq_params_opt.has_value()); + + // after deployment WITHDRAWALS activated we check not to quorum, but all active quorums + 1 the latest inactive + const int quorums_to_scan = DeploymentActiveAt(*pindexTip, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS) ? (llmq_params_opt->signingActiveQuorumCount + 1) : 2; + const auto quorums = qman.ScanQuorums(llmqType, pindexTip, quorums_to_scan); if (bool isActive = std::any_of(quorums.begin(), quorums.end(), [&](const auto &q) { return q->qc->quorumHash == quorumHash; }); !isActive) { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-not-active-quorum"); + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-too-old-quorum"); } if (static_cast(pindexTip->nHeight) < requestedHeight || pindexTip->nHeight >= getHeightToExpiry()) { From 5fd7b07ddc24fe5fbb2bebbf8e38d6b92f1ddff8 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 19 Sep 2024 03:50:44 +0700 Subject: [PATCH 03/69] chore: test new validation of asset unlocks tests after fork 'withdrawals' --- test/functional/feature_asset_locks.py | 35 ++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index 91480c0007..c49903f996 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -38,6 +38,7 @@ assert_equal, assert_greater_than, assert_greater_than_or_equal, + softfork_active, ) from test_framework.wallet_util import bytes_to_wif @@ -271,6 +272,7 @@ def run_test(self): self.test_asset_unlocks(node_wallet, node, pubkey) self.test_withdrawal_limits(node_wallet, node, pubkey) self.test_mn_rr(node_wallet, node, pubkey) + self.test_withdrawal_fork(node_wallet, node, pubkey) def test_asset_locks(self, node_wallet, node, pubkey): @@ -430,8 +432,9 @@ def test_asset_unlocks(self, node_wallet, node, pubkey): self.log.info("Checking that two quorums later it is too late because quorum is not active...") self.mine_quorum_2_nodes(llmq_type_name='llmq_test_platform', llmq_type=106) self.log.info("Expecting new reject-reason...") + assert not softfork_active(self.nodes[0], 'withdrawals') self.check_mempool_result(tx=asset_unlock_tx_too_late, - result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-not-active-quorum'}) + result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-too-old-quorum'}) block_to_reconsider = node.getbestblockhash() self.log.info("Test block invalidation with asset unlock tx...") @@ -445,7 +448,8 @@ def test_asset_unlocks(self, node_wallet, node, pubkey): self.validate_credit_pool_balance(locked - 2 * COIN) self.log.info("Forcibly mining asset_unlock_tx_too_late and ensure block is invalid") - self.create_and_check_block([asset_unlock_tx_too_late], expected_error = "bad-assetunlock-not-active-quorum") + assert not softfork_active(self.nodes[0], 'withdrawals') + self.create_and_check_block([asset_unlock_tx_too_late], expected_error = "bad-assetunlock-too-old-quorum") self.generate(node, 1) @@ -636,6 +640,33 @@ def test_mn_rr(self, node_wallet, node, pubkey): self.generate(node, 1) assert_equal(locked, self.get_credit_pool_balance()) + def test_withdrawal_fork(self, node_wallet, node, pubkey): + self.log.info("Testing asset unlock after 'withdrawal' activation...") + + assert softfork_active(self.nodes[0], 'withdrawals') + self.log.info("Generating several txes by same quorum....") + + asset_unlock_tx = self.create_assetunlock(501, COIN, pubkey) + asset_unlock_tx_payload = CAssetUnlockTx() + asset_unlock_tx_payload.deserialize(BytesIO(asset_unlock_tx.vExtraPayload)) + + self.log.info("Check that new Asset Unlock is valid for current quorum") + self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) + + while asset_unlock_tx_payload.quorumHash in node.quorum('list')['llmq_test_platform']: + self.log.info(f"Generate one more quorum until signing quorum becomes not available") + self.mine_quorum_2_nodes(llmq_type_name="llmq_test_platform", llmq_type=106) + + self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) + + self.log.info(f"Generate one more quorum after which asset unlock meant to be still valid") + self.mine_quorum_2_nodes(llmq_type_name="llmq_test_platform", llmq_type=106) + self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) + self.log.info(f"Generate one more quorum after which asset unlock meant to be expired") + self.mine_quorum_2_nodes(llmq_type_name="llmq_test_platform", llmq_type=106) + self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': False, 'reject-reason': 'bad-assetunlock-too-old-quorum'}) + + if __name__ == '__main__': AssetLocksTest().main() From e4774b9dad2fa7cbc59cdd57a83eed81e84d75f1 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 26 Aug 2021 22:49:27 +0300 Subject: [PATCH 04/69] Merge bitcoin-core/gui#384: Add copy IP/Netmask action for banned peer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ab1461d5d36b70fd4982679ac6143c25e7617dbf qt: Add copy IP/Netmask action for banned peer (Shashwat) Pull request description: This PR adds a Copy IP/Netmask context menu action to the Banned Peers Table. This feature is helpful if a node using GUI might want to alert its peer about a particular malicious user. So it can copy that user’s IP/Netmask and broadcast it to its peers so they can ban it instantly using the setban command in the console. | Master | PR | | ----------- | ----------- | | ![Screenshot_from_2021-07-21_00-01-331](https://user-images.githubusercontent.com/23396902/126377808-bd23bb19-3f47-4f1b-8371-39baa9747bbe.png) | ![Screenshot from 2021-08-20 20-13-28(1)(1)](https://user-images.githubusercontent.com/85434418/130251441-a8d0f816-a2e9-4e63-a22d-94885c5cec98.png) | ACKs for top commit: jarolrod: re-ACK ab1461d hebasto: re-ACK ab1461d5d36b70fd4982679ac6143c25e7617dbf, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: a528f089bd4cb5b51fec987550d21c2587459ad80f854b55850bc62c776c21f3fa31052a17e2b0e9e9d0b3468799c8070ed306543730fb7b324f283847151e17 --- src/qt/rpcconsole.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index b76b05bced..ab15df60c9 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -744,6 +744,13 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_ // create ban table context menu banTableContextMenu = new QMenu(this); + /*: Context menu action to copy the IP/Netmask of a banned peer. + IP/Netmask is the combination of a peer's IP address and its Netmask. + For IP address see: https://en.wikipedia.org/wiki/IP_address */ + banTableContextMenu->addAction(tr("&Copy IP/Netmask"), [this] { + GUIUtil::copyEntryData(ui->banlistWidget, BanTableModel::Address, Qt::DisplayRole); + }); + banTableContextMenu->addSeparator(); banTableContextMenu->addAction(tr("&Unban"), this, &RPCConsole::unbanSelectedNode); connect(ui->banlistWidget, &QTableView::customContextMenuRequested, this, &RPCConsole::showBanTableContextMenu); From 02b5fce94266f1bd3caf337b43e749a32dab9517 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 12 Sep 2021 18:58:21 +0300 Subject: [PATCH 05/69] Merge bitcoin-core/gui#318: Add `Copy address` Peers Tab Context Menu Action 3ec061d9da0c8742bd9dec94ffeb82a11d000aba qt: Add "Copy address" item to the context menu in the Peers table (Hennadii Stepanov) Pull request description: Picking up #264 This adds a `Copy Address` context menu action to the `Peers Tab`. Based on the first commit of PR #317 so that we can use `Qt::DisplayRole` in the `copyEntryData` function. | Master | PR | | ----------- | ----------- | | ![Screen Shot 2021-05-05 at 4 51 11 AM](https://user-images.githubusercontent.com/23396902/117117822-fb067400-ad5d-11eb-9466-228456108e52.png) | ![Screen Shot 2021-05-05 at 4 49 15 AM](https://user-images.githubusercontent.com/23396902/117117835-fe99fb00-ad5d-11eb-8de0-f6a9acdbf40e.png) | ACKs for top commit: shaavan: tACK 3ec061d9da0c8742bd9dec94ffeb82a11d000aba luke-jr: utACK 3ec061d9da0c8742bd9dec94ffeb82a11d000aba hebasto: ACK 3ec061d9da0c8742bd9dec94ffeb82a11d000aba, tested on Linux Mint 20.2 (Qt 5.12.8): Tree-SHA512: be0d7324592aae3928fa3cc522294f17226419fe8cbe3587df12a36bd4fa9c81bead377b13051e950b9a3fcd290b273861e70d6c76b75cdf76eaf58224b834cd --- src/qt/rpcconsole.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index ab15df60c9..084a65ff68 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -718,6 +718,11 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_ // create peer table context menu peersTableContextMenu = new QMenu(this); + //: Context menu action to copy the address of a peer + peersTableContextMenu->addAction(tr("&Copy address"), [this] { + GUIUtil::copyEntryData(ui->peerWidget, PeerTableModel::Address, Qt::DisplayRole); + }); + peersTableContextMenu->addSeparator(); peersTableContextMenu->addAction(tr("&Disconnect"), this, &RPCConsole::disconnectSelectedNode); peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 &hour"), [this] { banSelectedNode(60 * 60); }); peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 d&ay"), [this] { banSelectedNode(60 * 60 * 24); }); From 95aeb6a08d20c20bae46a2279df27a6bc9d9dc2b Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 29 Sep 2021 17:18:33 +0300 Subject: [PATCH 06/69] Merge bitcoin-core/gui#436: Include vout when copying transaction ID from coin selection 10c6929d55ba9bc203bbadfb834537445dbd67ce Include vout when copying transaction ID from coin selection (Samuel Dobson) Pull request description: Fixes #432 I think it makes sense to just add the vout to the existing function because I can't imagine a situation where a user in the coin selection dialog would want just the transaction ID rather than the specific outpoint, and they can just delete it from the end anyway. ACKs for top commit: kristapsk: ACK 10c6929d55ba9bc203bbadfb834537445dbd67ce hebasto: ACK 10c6929d55ba9bc203bbadfb834537445dbd67ce, tested on Linux Mint 20.2 (Qt 5.12.8). shaavan: ACK 10c6929 Tree-SHA512: df4d132b6c2fd0b590594e91cf54f82c6c0f77ee9ca06296fb726bc3c52b9ae459ca3b50c48b2bf303ccafe832b6b4dba692a812f439991ca6d807ea0d8df934 --- src/qt/coincontroldialog.cpp | 16 ++++++++++------ src/qt/coincontroldialog.h | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 8a88c6161b..4ada490a7a 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -67,7 +67,7 @@ CoinControlDialog::CoinControlDialog(CCoinControl& coin_control, WalletModel* _m contextMenu->addAction(tr("&Copy address"), this, &CoinControlDialog::copyAddress); contextMenu->addAction(tr("Copy &label"), this, &CoinControlDialog::copyLabel); contextMenu->addAction(tr("Copy &amount"), this, &CoinControlDialog::copyAmount); - copyTransactionHashAction = contextMenu->addAction(tr("Copy transaction &ID"), this, &CoinControlDialog::copyTransactionHash); + m_copy_transaction_outpoint_action = contextMenu->addAction(tr("Copy transaction &ID and output index"), this, &CoinControlDialog::copyTransactionOutpoint); contextMenu->addSeparator(); lockAction = contextMenu->addAction(tr("L&ock unspent"), this, &CoinControlDialog::lockCoin); unlockAction = contextMenu->addAction(tr("&Unlock unspent"), this, &CoinControlDialog::unlockCoin); @@ -235,7 +235,7 @@ void CoinControlDialog::showMenu(const QPoint &point) // disable some items (like Copy Transaction ID, lock, unlock) for tree roots in context menu if (item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode) { - copyTransactionHashAction->setEnabled(true); + m_copy_transaction_outpoint_action->setEnabled(true); if (model->wallet().isLockedCoin(COutPoint(uint256S(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()))) { lockAction->setEnabled(false); @@ -249,7 +249,7 @@ void CoinControlDialog::showMenu(const QPoint &point) } else // this means click on parent node in tree mode -> disable all { - copyTransactionHashAction->setEnabled(false); + m_copy_transaction_outpoint_action->setEnabled(false); lockAction->setEnabled(false); unlockAction->setEnabled(false); } @@ -283,10 +283,14 @@ void CoinControlDialog::copyAddress() GUIUtil::setClipboard(contextMenuItem->text(COLUMN_ADDRESS)); } -// context menu action: copy transaction id -void CoinControlDialog::copyTransactionHash() +// context menu action: copy transaction id and vout index +void CoinControlDialog::copyTransactionOutpoint() { - GUIUtil::setClipboard(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString()); + const QString address = contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString(); + const QString vout = contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toString(); + const QString outpoint = QString("%1:%2").arg(address).arg(vout); + + GUIUtil::setClipboard(outpoint); } // context menu action: lock coin diff --git a/src/qt/coincontroldialog.h b/src/qt/coincontroldialog.h index 3336f2c396..601b4bcb5b 100644 --- a/src/qt/coincontroldialog.h +++ b/src/qt/coincontroldialog.h @@ -59,7 +59,7 @@ class CoinControlDialog : public QDialog QMenu *contextMenu; QTreeWidgetItem *contextMenuItem; - QAction *copyTransactionHashAction; + QAction* m_copy_transaction_outpoint_action; QAction *lockAction; QAction *unlockAction; @@ -92,7 +92,7 @@ private Q_SLOTS: void copyAmount(); void copyLabel(); void copyAddress(); - void copyTransactionHash(); + void copyTransactionOutpoint(); void lockCoin(); void unlockCoin(); void clipboardQuantity(); From 042e8a4101d8776b5eaf75f559ac705404245205 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 7 Oct 2024 00:21:21 +0700 Subject: [PATCH 07/69] fix: drop unneded lock assert in net.cpp for m_nodes_mutex --- src/net.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 474fbec3e7..49e9ca2c82 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -643,8 +643,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo void CNode::CloseSocketDisconnect(CConnman* connman) { - AssertLockHeld(connman->m_nodes_mutex); - fDisconnect = true; LOCK2(connman->cs_mapSocketToNode, m_sock_mutex); if (!m_sock) { From a219a8be154be6d31873de2985dcf4509e19b62b Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 26 Sep 2024 00:47:28 +0700 Subject: [PATCH 08/69] partial Merge #29040: refactor: Remove pre-C++20 code, fs::path cleanup It fixes return reference to const variable under mutex which is not a good idea --- src/util/system.cpp | 4 ++-- src/util/system.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index b0af7eb30b..9ecadeabce 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -420,7 +420,7 @@ std::optional ArgsManager::GetArgFlags(const std::string& name) co return std::nullopt; } -const fs::path& ArgsManager::GetBlocksDirPath() const +const fs::path ArgsManager::GetBlocksDirPath() const { LOCK(cs_args); fs::path& path = m_cached_blocks_path; @@ -445,7 +445,7 @@ const fs::path& ArgsManager::GetBlocksDirPath() const return path; } -const fs::path& ArgsManager::GetDataDir(bool net_specific) const +const fs::path ArgsManager::GetDataDir(bool net_specific) const { LOCK(cs_args); fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path; diff --git a/src/util/system.h b/src/util/system.h index 1ae230d4f8..4ab1905459 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -288,7 +288,7 @@ class ArgsManager * * @return Blocks path which is network specific */ - const fs::path& GetBlocksDirPath() const; + const fs::path GetBlocksDirPath() const; /** * Get data directory path @@ -296,7 +296,7 @@ class ArgsManager * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned * @post Returned directory path is created unless it is empty */ - const fs::path& GetDataDirBase() const { return GetDataDir(false); } + const fs::path GetDataDirBase() const { return GetDataDir(false); } /** * Get data directory path with appended network identifier @@ -304,7 +304,7 @@ class ArgsManager * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned * @post Returned directory path is created unless it is empty */ - const fs::path& GetDataDirNet() const { return GetDataDir(true); } + const fs::path GetDataDirNet() const { return GetDataDir(true); } fs::path GetBackupsDirPath(); @@ -483,7 +483,7 @@ class ArgsManager * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned * @post Returned directory path is created unless it is empty */ - const fs::path& GetDataDir(bool net_specific) const; + const fs::path GetDataDir(bool net_specific) const; // Helper function for LogArgs(). void logArgsPrefix( From 502d6ae8efd024827afbb93db5bb10f50f485505 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 26 Sep 2024 00:20:01 +0700 Subject: [PATCH 09/69] fix: add multiple missing annotation about locks for dash specific code --- src/coinjoin/client.cpp | 9 +++++++-- src/coinjoin/client.h | 13 +++++++++---- src/coinjoin/coinjoin.h | 4 ++-- src/evo/deterministicmns.cpp | 12 +++++++----- src/evo/simplifiedmns.h | 7 ++++--- src/evo/specialtxman.cpp | 7 ++++--- src/governance/governance.h | 9 +++++---- src/governance/object.h | 3 ++- src/llmq/snapshot.h | 2 +- src/masternode/meta.h | 6 +++++- src/net_processing.cpp | 2 +- src/rpc/evo.cpp | 5 +++-- src/txmempool.cpp | 4 ++-- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 18 +++++++++++------- 15 files changed, 64 insertions(+), 39 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 9739727243..4c15f98f4a 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -1448,11 +1448,16 @@ bool CCoinJoinClientSession::MakeCollateralAmounts() }); // First try to use only non-denominated funds - if (std::any_of(vecTally.begin(), vecTally.end(), [&](const auto& item) { return MakeCollateralAmounts(item, false); })) { + if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet) { + return MakeCollateralAmounts(item, false); + })) { return true; } + // There should be at least some denominated funds we should be able to break in pieces to continue mixing - if (std::any_of(vecTally.begin(), vecTally.end(), [&](const auto& item) { return MakeCollateralAmounts(item, true); })) { + if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet) { + return MakeCollateralAmounts(item, true); + })) { return true; } diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index a0deebdbd2..c4e22b7123 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -161,13 +161,16 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession /// Create denominations bool CreateDenominated(CAmount nBalanceToDenominate); - bool CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals); + bool CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); /// Split up large inputs or make fee sized inputs bool MakeCollateralAmounts(); - bool MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated); + bool MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); - bool CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason); + bool CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); bool JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman); bool StartNewQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman); @@ -175,7 +178,9 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession /// step 0: select denominated inputs and txouts bool SelectDenominate(std::string& strErrorRet, std::vector& vecTxDSInRet); /// step 1: prepare denominated inputs and outputs - bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, std::vector >& vecPSInOutPairsRet, bool fDryRun = false); + bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, + std::vector>& vecPSInOutPairsRet, bool fDryRun = false) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); /// step 2: send denominated inputs and outputs prepared in step 1 bool SendDenominate(const std::vector >& vecPSInOutPairsIn, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); diff --git a/src/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index 6bf8e93e11..5680b8abc2 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -398,8 +398,8 @@ class CDSTXManager private: void CheckDSTXes(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx); - void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, std::optional nHeight); - + void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, std::optional nHeight) + EXCLUSIVE_LOCKS_REQUIRED(cs_mapdstx); }; bool ATMPIfSaneFee(ChainstateManager& chainman, const CTransactionRef& tx, bool test_accept = false) diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index ac167a298b..49912b8880 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -1067,11 +1067,13 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n mnListsCache.emplace(snapshot.GetBlockHash(), snapshot); } else { // keep snapshots for yet alive quorums - if (ranges::any_of(Params().GetConsensus().llmqs, [&snapshot, this](const auto& params){ - AssertLockHeld(cs); - return (snapshot.GetHeight() % params.dkgInterval == 0) && - (snapshot.GetHeight() + params.dkgInterval * (params.keepOldConnections + 1) >= tipIndex->nHeight); - })) { + if (ranges::any_of(Params().GetConsensus().llmqs, + [&snapshot, this](const auto& params) EXCLUSIVE_LOCKS_REQUIRED(cs) { + AssertLockHeld(cs); + return (snapshot.GetHeight() % params.dkgInterval == 0) && + (snapshot.GetHeight() + params.dkgInterval * (params.keepOldConnections + 1) >= + tipIndex->nHeight); + })) { mnListsCache.emplace(snapshot.GetBlockHash(), snapshot); } } diff --git a/src/evo/simplifiedmns.h b/src/evo/simplifiedmns.h index 9999a5f352..0cfd5603d9 100644 --- a/src/evo/simplifiedmns.h +++ b/src/evo/simplifiedmns.h @@ -171,8 +171,9 @@ class CSimplifiedMNListDiff [[nodiscard]] UniValue ToJson(bool extended = false) const; }; -bool BuildSimplifiedMNListDiff(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, const llmq::CQuorumBlockProcessor& qblockman, - const llmq::CQuorumManager& qman, const uint256& baseBlockHash, const uint256& blockHash, - CSimplifiedMNListDiff& mnListDiffRet, std::string& errorRet, bool extended = false); +bool BuildSimplifiedMNListDiff(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, + const llmq::CQuorumBlockProcessor& qblockman, const llmq::CQuorumManager& qman, + const uint256& baseBlockHash, const uint256& blockHash, CSimplifiedMNListDiff& mnListDiffRet, + std::string& errorRet, bool extended = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); #endif // BITCOIN_EVO_SIMPLIFIEDMNS_H diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index e20a9e0d4f..c8c84e84be 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -19,9 +19,10 @@ #include #include -static bool CheckSpecialTxInner(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, const llmq::CQuorumManager& qman, const CTransaction& tx, - const CBlockIndex* pindexPrev, const CCoinsViewCache& view, const std::optional& indexes, bool check_sigs, - TxValidationState& state) +static bool CheckSpecialTxInner(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, + const llmq::CQuorumManager& qman, const CTransaction& tx, const CBlockIndex* pindexPrev, + const CCoinsViewCache& view, const std::optional& indexes, bool check_sigs, + TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(cs_main); diff --git a/src/governance/governance.h b/src/governance/governance.h index e5b8e077a8..adc25bb543 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -347,9 +347,9 @@ class CGovernanceManager : public GovernanceStore * - Track governance objects which are triggers * - After triggers are activated and executed, they can be removed */ - std::vector> GetActiveTriggers() const; - bool AddNewTrigger(uint256 nHash); - void CleanAndRemoveTriggers(); + std::vector> GetActiveTriggers() const EXCLUSIVE_LOCKS_REQUIRED(cs); + bool AddNewTrigger(uint256 nHash) EXCLUSIVE_LOCKS_REQUIRED(cs); + void CleanAndRemoveTriggers() EXCLUSIVE_LOCKS_REQUIRED(cs); // Superblocks related: @@ -373,7 +373,8 @@ class CGovernanceManager : public GovernanceStore private: void ExecuteBestSuperblock(const CDeterministicMNList& tip_mn_list, int nBlockHeight); - bool GetBestSuperblock(const CDeterministicMNList& tip_mn_list, CSuperblock_sptr& pSuperblockRet, int nBlockHeight); + bool GetBestSuperblock(const CDeterministicMNList& tip_mn_list, CSuperblock_sptr& pSuperblockRet, int nBlockHeight) + EXCLUSIVE_LOCKS_REQUIRED(cs); std::optional CreateSuperblockCandidate(int nHeight) const; std::optional CreateGovernanceTrigger(const std::optional& sb_opt, PeerManager& peerman, diff --git a/src/governance/object.h b/src/governance/object.h index e58da6a8a1..8ced4950f9 100644 --- a/src/governance/object.h +++ b/src/governance/object.h @@ -235,7 +235,8 @@ class CGovernanceObject /// Check the collateral transaction for the budget proposal/finalized budget bool IsCollateralValid(const ChainstateManager& chainman, std::string& strError, bool& fMissingConfirmations) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); - void UpdateLocalValidity(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman); + void UpdateLocalValidity(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); void UpdateSentinelVariables(const CDeterministicMNList& tip_mn_list); diff --git a/src/llmq/snapshot.h b/src/llmq/snapshot.h index b9ca9ac985..1551789f37 100644 --- a/src/llmq/snapshot.h +++ b/src/llmq/snapshot.h @@ -209,7 +209,7 @@ class CQuorumRotationInfo bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, const CQuorumManager& qman, const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request, - CQuorumRotationInfo& response, std::string& errorRet); + CQuorumRotationInfo& response, std::string& errorRet) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); uint256 GetLastBaseBlockHash(Span baseBlockIndexes, const CBlockIndex* blockIndex); class CQuorumSnapshotManager diff --git a/src/masternode/meta.h b/src/masternode/meta.h index 0388905c44..14870afa63 100644 --- a/src/masternode/meta.h +++ b/src/masternode/meta.h @@ -73,7 +73,11 @@ class CMasternodeMetaInfo UniValue ToJson() const; public: - const uint256& GetProTxHash() const { LOCK(cs); return proTxHash; } + const uint256 GetProTxHash() const + { + LOCK(cs); + return proTxHash; + } int64_t GetLastDsq() const { return nLastDsq; } int GetMixingTxCount() const { return nMixingTxCount; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9536b8fd07..4537754231 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5771,7 +5771,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) peer->m_blocks_for_inv_relay.clear(); } - auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) { + auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) EXCLUSIVE_LOCKS_REQUIRED(peer->m_tx_relay->m_tx_inventory_mutex) { AssertLockHeld(peer->m_tx_relay->m_tx_inventory_mutex); peer->m_tx_relay->m_tx_inventory_known_filter.insert(invIn.hash); LogPrint(BCLog::NET, "SendMessages -- queued inv: %s index=%d peer=%d\n", invIn.ToString(), vInv.size(), pto->GetId()); diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index 30fdab04b7..5fa7506cda 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -213,8 +213,9 @@ static bool ValidatePlatformPort(const int32_t port) #ifdef ENABLE_WALLET -template -static void FundSpecialTx(CWallet& wallet, CMutableTransaction& tx, const SpecialTxPayload& payload, const CTxDestination& fundDest) +template +static void FundSpecialTx(CWallet& wallet, CMutableTransaction& tx, const SpecialTxPayload& payload, + const CTxDestination& fundDest) EXCLUSIVE_LOCKS_REQUIRED(!wallet.cs_wallet) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now diff --git a/src/txmempool.cpp b/src/txmempool.cpp index cb16e2ddfc..291599aea2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -955,7 +955,7 @@ void CTxMemPool::removeProTxSpentCollateralConflicts(const CTransaction &tx) assert(m_dmnman); // Remove TXs that refer to a MN for which the collateral was spent - auto removeSpentCollateralConflict = [&](const uint256& proTxHash) { + auto removeSpentCollateralConflict = [&](const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs) { // Can't use equal_range here as every call to removeRecursive might invalidate iterators AssertLockHeld(cs); while (true) { @@ -1353,7 +1353,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { LOCK(cs); - auto hasKeyChangeInMempool = [&](const uint256& proTxHash) { + auto hasKeyChangeInMempool = [&](const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs) { AssertLockHeld(cs); for (auto its = mapProTxRefs.equal_range(proTxHash); its.first != its.second; ++its.first) { auto txit = mapTx.find(its.first->second); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index edc7b41c9e..b6ac4595a8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3681,7 +3681,7 @@ bool CWallet::CreateTransactionInternal( txNew.vin.emplace_back(coin.outpoint, CScript(), CTxIn::SEQUENCE_FINAL - 1); } - auto calculateFee = [&](CAmount& nFee) -> bool { + auto calculateFee = [&](CAmount& nFee) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) -> bool { AssertLockHeld(cs_wallet); nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); if (nBytes < 0) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 11824d79ee..411367ac2b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -516,8 +516,12 @@ class CWalletTx CAmount GetImmatureWatchOnlyCredit(const bool fUseCache = true) const; CAmount GetChange() const; - CAmount GetAnonymizedCredit(const CCoinControl* coinControl = nullptr) const; - CAmount GetDenominatedCredit(bool unconfirmed, bool fUseCache=true) const; + // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct + // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The + // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid + // having to resolve the issue of member access into incomplete type CWallet. + CAmount GetAnonymizedCredit(const CCoinControl* coinControl = nullptr) const NO_THREAD_SAFETY_ANALYSIS; + CAmount GetDenominatedCredit(bool unconfirmed, bool fUseCache=true) const NO_THREAD_SAFETY_ANALYSIS; /** Get the marginal bytes if spending the specified output from this transaction */ int GetSpendSize(unsigned int out, bool use_max_sig = false) const @@ -569,7 +573,7 @@ class CWalletTx int GetDepthInMainChain() const NO_THREAD_SAFETY_ANALYSIS; bool IsInMainChain() const { return GetDepthInMainChain() > 0; } bool IsLockedByInstantSend() const; - bool IsChainLocked() const; + bool IsChainLocked() const NO_THREAD_SAFETY_ANALYSIS; /** * @return number of blocks to maturity for this transaction: @@ -1289,7 +1293,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::set GetConflicts(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Check if a given transaction has any of its outputs spent by another transaction in the wallet - bool HasWalletSpend(const uint256& txid) const; + bool HasWalletSpend(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Flush wallet (bitdb flush) void Flush(); @@ -1386,11 +1390,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void notifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr& clsig) override; /** Load a CGovernanceObject into m_gobjects. */ - bool LoadGovernanceObject(const Governance::Object& obj); + bool LoadGovernanceObject(const Governance::Object& obj) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Store a CGovernanceObject in the wallet database. This should only be used by governance objects that are created by this wallet via `gobject prepare`. */ - bool WriteGovernanceObject(const Governance::Object& obj); + bool WriteGovernanceObject(const Governance::Object& obj) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Returns a vector containing pointers to the governance objects in m_gobjects */ - std::vector GetGovernanceObjects(); + std::vector GetGovernanceObjects() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Blocks until the wallet state is up-to-date to /at least/ the current From 002580c42d8fc2d7f91ec0ed27876464db0771d2 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sun, 6 Oct 2024 23:54:32 +0700 Subject: [PATCH 10/69] fix: add ignoring safety-annotation for llmq/signing_shares due to template lambdas --- src/llmq/signing_shares.cpp | 56 +++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 0b157f218a..47a70430fa 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -565,24 +565,29 @@ void CSigSharesManager::CollectPendingSigSharesToVerify( // other nodes would be able to poison us with a large batch with N-1 valid shares and the last one being // invalid, making batch verification fail and revert to per-share verification, which in turn would slow down // the whole verification process - std::unordered_set, StaticSaltedHasher> uniqueSignHashes; - IterateNodesRandom(nodeStates, [&]() { - return uniqueSignHashes.size() < maxUniqueSessions; - }, [&](NodeId nodeId, CSigSharesNodeState& ns) { - if (ns.pendingIncomingSigShares.Empty()) { - return false; - } - const auto& sigShare = *ns.pendingIncomingSigShares.GetFirst(); + IterateNodesRandom( + nodeStates, + [&]() { + return uniqueSignHashes.size() < maxUniqueSessions; + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template IterateNodesRandom makes impossible to use lock annotation + }, + [&](NodeId nodeId, CSigSharesNodeState& ns) NO_THREAD_SAFETY_ANALYSIS { + if (ns.pendingIncomingSigShares.Empty()) { + return false; + } + const auto& sigShare = *ns.pendingIncomingSigShares.GetFirst(); - AssertLockHeld(cs); - if (const bool alreadyHave = this->sigShares.Has(sigShare.GetKey()); !alreadyHave) { - uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash()); - retSigShares[nodeId].emplace_back(sigShare); - } - ns.pendingIncomingSigShares.Erase(sigShare.GetKey()); - return !ns.pendingIncomingSigShares.Empty(); - }, rnd); + AssertLockHeld(cs); + if (const bool alreadyHave = this->sigShares.Has(sigShare.GetKey()); !alreadyHave) { + uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash()); + retSigShares[nodeId].emplace_back(sigShare); + } + ns.pendingIncomingSigShares.Erase(sigShare.GetKey()); + return !ns.pendingIncomingSigShares.Empty(); + }, + rnd); if (retSigShares.empty()) { return; @@ -1020,7 +1025,10 @@ void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map, std::unordered_set, StaticSaltedHasher> quorumNodesMap; - sigSharesQueuedToAnnounce.ForEach([this, &quorumNodesMap, &sigSharesToAnnounce](const SigShareKey& sigShareKey, bool) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + sigSharesQueuedToAnnounce.ForEach([this, &quorumNodesMap, &sigSharesToAnnounce](const SigShareKey& sigShareKey, + bool) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); const auto& signHash = sigShareKey.first; auto quorumMember = sigShareKey.second; @@ -1076,7 +1084,7 @@ bool CSigSharesManager::SendMessages() std::unordered_map> sigSharesToAnnounce; std::unordered_map> sigSessionAnnouncements; - auto addSigSesAnnIfNeeded = [&](NodeId nodeId, const uint256& signHash) { + auto addSigSesAnnIfNeeded = [&](NodeId nodeId, const uint256& signHash) EXCLUSIVE_LOCKS_REQUIRED(cs) { AssertLockHeld(cs); auto& nodeState = nodeStates[nodeId]; auto* session = nodeState.GetSessionBySignHash(signHash); @@ -1357,7 +1365,9 @@ void CSigSharesManager::Cleanup() continue; } // remove global requested state to force a re-request from another node - it->second.requestedSigShares.ForEach([this](const SigShareKey& k, bool) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + it->second.requestedSigShares.ForEach([this](const SigShareKey& k, bool) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); sigSharesRequested.Erase(k); }); @@ -1390,7 +1400,9 @@ void CSigSharesManager::RemoveBannedNodeStates() for (auto it = nodeStates.begin(); it != nodeStates.end();) { if (Assert(m_peerman)->IsBanned(it->first)) { // re-request sigshares from other nodes - it->second.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + it->second.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); sigSharesRequested.Erase(k); }); @@ -1419,7 +1431,9 @@ void CSigSharesManager::BanNode(NodeId nodeId) auto& nodeState = it->second; // Whatever we requested from him, let's request it from someone else now - nodeState.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + nodeState.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); sigSharesRequested.Erase(k); }); From 846ebab6e06601105a3eb22eb6b3c6ff68426adb Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 7 Oct 2024 00:35:30 +0700 Subject: [PATCH 11/69] fix: fixes for orphange's locks and cs_main --- src/net_processing.cpp | 12 +++++++----- src/txorphanage.cpp | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4537754231..803f3d51aa 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1930,12 +1930,14 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) */ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { - LOCK2(::cs_main, g_cs_orphans); + { + LOCK2(::cs_main, g_cs_orphans); - auto orphanWorkSet = m_orphanage.GetCandidatesForBlock(*pblock); - while (!orphanWorkSet.empty()) { - LogPrint(BCLog::MEMPOOL, "Trying to process %d orphans\n", orphanWorkSet.size()); - ProcessOrphanTx(orphanWorkSet); + auto orphanWorkSet = m_orphanage.GetCandidatesForBlock(*pblock); + while (!orphanWorkSet.empty()) { + LogPrint(BCLog::MEMPOOL, "Trying to process %d orphans\n", orphanWorkSet.size()); + ProcessOrphanTx(orphanWorkSet); + } } m_orphanage.EraseForBlock(*pblock); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 84835e0aad..ce9d6435ce 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -197,7 +197,7 @@ std::set TxOrphanage::GetCandidatesForBlock(const CBlock& block) void TxOrphanage::EraseForBlock(const CBlock& block) { - AssertLockHeld(g_cs_orphans); + LOCK(g_cs_orphans); std::vector vOrphanErase; From 6d452845dc8caf2c77231218a508676e35180bdf Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 7 Oct 2024 01:09:14 +0700 Subject: [PATCH 12/69] fix: add missing lock annotation for ContextualCheckBlock and related TODO --- src/validation.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 38a030ace1..afd909c993 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3644,9 +3644,10 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio * in ConnectBlock(). * Note that -reindex-chainstate skips the validation that happens here! */ -static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) +static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - AssertLockHeld(cs_main); + // TODO: validate - why do we need this cs_main ? + AssertLockHeld(::cs_main); const int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1; // Enforce BIP113 (Median Time Past). From 898282d6206381532ae78ade0b47f614afbe7c60 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 23 Jan 2024 14:50:58 -0500 Subject: [PATCH 13/69] Merge bitcoin/bitcoin#28774: wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov) Pull request description: `CWallet::GetEncryptionKey()` would return a reference to the internal `CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe. Returning a copy would be a shorter solution, but could have security implications of the master key remaining somewhere in the memory even after `CWallet::Lock()` (the current calls to `CWallet::GetEncryptionKey()` are safe, but that is not future proof). So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)` change the `GetEncryptionKey()` method to provide the encryption key to a given callback: `m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })` This silences the following (clang 18): ``` wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return] 3520 | return vMasterKey; | ^ ``` --- _Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit https://github.com/bitcoin/bitcoin/commit/856c88776f8486446602476a1c9e133ac0cff510 was merged in https://github.com/bitcoin/bitcoin/pull/29040 so now this only affects wallet code. The previous PR description was:_ Avoid this unsafe pattern from `ArgsManager` and `CWallet`: ```cpp class A { Mutex mutex; Foo member GUARDED_BY(mutex); const Foo& Get() { LOCK(mutex); return member; } // callers of `Get()` will have access to `member` without owning the mutex. ``` ACKs for top commit: achow101: ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b ryanofsky: Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple. furszy: ACK 32a9f13c Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1 --- src/wallet/scriptpubkeyman.cpp | 36 ++++++++++++++++++++++++++-------- src/wallet/scriptpubkeyman.h | 4 +++- src/wallet/wallet.cpp | 5 +++-- src/wallet/wallet.h | 3 ++- 4 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 9d84a04b6a..52d07ff7fa 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -342,8 +342,11 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata() CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainCurrent)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainCurrent); + })) { throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } CExtKey masterKey; SecureVector vchSeed = hdChainCurrent.GetSeed(); @@ -474,8 +477,11 @@ bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) return false; } - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainTmp)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainTmp); + })) { return false; + } // make sure seed matches this chain if (hdChainTmp.GetID() != hdChainTmp.GetSeedHash()) @@ -911,7 +917,9 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu std::vector vchCryptedSecret; CKeyingMaterial vchSecret(key.begin(), key.end()); - if (!EncryptSecret(m_storage.GetEncryptionKey(), vchSecret, pubkey.GetHash(), vchCryptedSecret)) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, vchSecret, pubkey.GetHash(), vchCryptedSecret); + })) { return false; } @@ -933,7 +941,9 @@ bool LegacyScriptPubKeyMan::GetKeyInner(const CKeyID &address, CKey& keyOut) con { const CPubKey &vchPubKey = (*mi).second.first; const std::vector &vchCryptedSecret = (*mi).second.second; - return DecryptKey(m_storage.GetEncryptionKey(), vchCryptedSecret, vchPubKey, keyOut); + return m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, vchCryptedSecret, vchPubKey, keyOut); + }); } return false; } @@ -1164,8 +1174,11 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainCurrent)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainCurrent); + })) { throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } // make sure seed matches this chain if (hdChainCurrent.GetID() != hdChainCurrent.GetSeedHash()) throw std::runtime_error(std::string(__func__) + ": Wrong HD chain!"); @@ -1276,8 +1289,11 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); } - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainTmp)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainTmp); + })) { throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } // make sure seed matches this chain if (hdChainTmp.GetID() != hdChainTmp.GetSeedHash()) throw std::runtime_error(std::string(__func__) + ": Wrong HD chain!"); @@ -1897,7 +1913,9 @@ std::map DescriptorScriptPubKeyMan::GetKeys() const const CPubKey& pubkey = key_pair.second.first; const std::vector& crypted_secret = key_pair.second.second; CKey key; - DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, pubkey, key); + m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, crypted_secret, pubkey, key); + }); keys[pubkey.GetID()] = key; } return keys; @@ -2010,7 +2028,9 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const std::vector crypted_secret; CKeyingMaterial secret(key.begin(), key.end()); - if (!EncryptSecret(m_storage.GetEncryptionKey(), secret, pubkey.GetHash(), crypted_secret)) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, secret, pubkey.GetHash(), crypted_secret); + })) { return false; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 0d6450d671..05f2452d2f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -21,6 +21,7 @@ #include +#include #include // Wallet storage things that ScriptPubKeyMans need in order to be able to store things to the wallet database. @@ -38,7 +39,8 @@ class WalletStorage virtual void UnsetBlankWalletFlag(WalletBatch&) = 0; virtual bool CanSupportFeature(enum WalletFeature) const = 0; virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0; - virtual const CKeyingMaterial& GetEncryptionKey() const = 0; + //! Pass the encryption key to cb(). + virtual bool WithEncryptionKey(std::function cb) const = 0; virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked(bool fForMixing) const = 0; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b6ac4595a8..00d872582a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5743,9 +5743,10 @@ void CWallet::SetupLegacyScriptPubKeyMan() m_spk_managers[spk_manager->GetID()] = std::move(spk_manager); } -const CKeyingMaterial& CWallet::GetEncryptionKey() const +bool CWallet::WithEncryptionKey(std::function cb) const { - return vMasterKey; + LOCK(cs_wallet); + return cb(vMasterKey); } bool CWallet::HasEncryptionKeys() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 411367ac2b..72c4c747ba 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1468,7 +1468,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external. void SetupLegacyScriptPubKeyMan(); - const CKeyingMaterial& GetEncryptionKey() const override; + bool WithEncryptionKey(std::function cb) const override; + bool HasEncryptionKeys() const override; /** Get last block processed height */ From 55114a682e52ecba0ee27c0cb6f739d09b3f62df Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 1 Sep 2020 08:18:20 +0200 Subject: [PATCH 14/69] Merge #19668: Do not hide compile-time thread safety warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ea74e10acf17903e44c85e3678853414653dd4e1 doc: Add best practice for annotating/asserting locks (Hennadii Stepanov) 2ee7743fe723227f2ea1b031eddb14fc6863f4c8 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns) 23d71d171e6e22ba5e4a909d597a54595b2a2c1f Do not hide compile-time thread safety warnings (Hennadii Stepanov) 3ddc150857178bfb1c854c05bf9b526777876f56 Add missed thread safety annotations (Hennadii Stepanov) af9ea55a72c94678b343f5dd98dc78f3a3ac58cb Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov) Pull request description: On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings. On master (65e4ecabd5b4252154640c7bac38c92a3f3a7018) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied: ```diff --- a/src/txmempool.h +++ b/src/txmempool.h @@ -607,7 +607,7 @@ public: void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); ``` Clang compiles the code without any thread safety warnings. See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR. ACKs for top commit: MarcoFalke: ACK ea74e10acf 🎙 jnewbery: ACK ea74e10acf17903e44c85e3678853414653dd4e1 ajtowns: ACK ea74e10acf17903e44c85e3678853414653dd4e1 Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447 --- doc/developer-notes.md | 66 ++++++++++++++++++++++++++++++++++++ src/sync.cpp | 6 +++- src/sync.h | 10 +++--- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.h | 4 +-- 5 files changed, 80 insertions(+), 8 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 248557919a..22785e7349 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -899,6 +899,72 @@ the upper cycle, etc. Threads and synchronization ---------------------------- +- Prefer `Mutex` type to `RecursiveMutex` one + +- Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to + get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with + run-time asserts in function definitions: + +```C++ +// txmempool.h +class CTxMemPool +{ +public: + ... + mutable RecursiveMutex cs; + ... + void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs); + ... +} + +// txmempool.cpp +void CTxMemPool::UpdateTransactionsFromBlock(...) +{ + AssertLockHeld(::cs_main); + AssertLockHeld(cs); + ... +} +``` + +```C++ +// validation.h +class ChainstateManager +{ +public: + ... + bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main); + ... +} + +// validation.cpp +bool ChainstateManager::ProcessNewBlock(...) +{ + AssertLockNotHeld(::cs_main); + ... + LOCK(::cs_main); + ... +} +``` + +- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances: + +```C++ +// net_processing.h +void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + +// net_processing.cpp +void RelayTransaction(...) +{ + AssertLockHeld(::cs_main); + + connman.ForEachNode([&txid, &wtxid](CNode* pnode) { + LockAssertion lock(::cs_main); + ... + }); +} + +``` + - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. diff --git a/src/sync.cpp b/src/sync.cpp index 35d3ffaf12..7540819262 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -285,12 +285,16 @@ template void AssertLockHeldInternal(const char*, const char*, int, Mutex*); template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*); template void AssertLockHeldInternal(const char*, const char*, int, SharedMutex*); -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) { if (!LockHeld(cs)) return; tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); abort(); } +template void AssertLockNotHeldInternal(const char*, const char*, int, Mutex*); +template void AssertLockNotHeldInternal(const char*, const char*, int, RecursiveMutex*); +template void AssertLockNotHeldInternal(const char*, const char*, int, SharedMutex*); void DeleteLock(void* cs) { diff --git a/src/sync.h b/src/sync.h index 3712bd9824..8d02830726 100644 --- a/src/sync.h +++ b/src/sync.h @@ -57,8 +57,9 @@ void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); std::string LocksHeld(); template -void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs); -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); +void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs); void DeleteLock(void* cs); bool LockStackEmpty(); @@ -74,8 +75,9 @@ inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, M inline void LeaveCritical() {} inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} template -inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) {} -inline void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} +inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {} +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) {} inline void DeleteLock(void* cs) {} inline bool LockStackEmpty() { return true; } #endif diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 05f2452d2f..e3f1916272 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -528,7 +528,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan //! keeps track of whether Unlock has run a thorough check before bool m_decryption_thoroughly_checked = false; - bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey); + bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 72c4c747ba..88ec3db2f1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1402,7 +1402,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * Obviously holding cs_main/cs_wallet when going into this call may cause * deadlock */ - void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(cs_main, cs_wallet); + void BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !cs_wallet); /** set a single wallet flag */ void SetWalletFlag(uint64_t flags); @@ -1515,7 +1515,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void DeactivateScriptPubKeyMan(uint256 id, bool internal); //! Create new DescriptorScriptPubKeyMans and add them to the wallet - void SetupDescriptorScriptPubKeyMans(); + void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; From d0a4198166d0670ce814213d77c39b77377cd373 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 6 Apr 2021 07:54:07 +0200 Subject: [PATCH 15/69] Merge #21598: refactor: Remove negative lock annotations from globals fa5eabe72117f6e3704858e8d5b2c57a120258ed refactor: Remove negative lock annotations from globals (MarcoFalke) Pull request description: They only make sense for mutexes that are private members. Until cs_main is a private member the negative annotations should be replaced by excluded annotations, which are optional. ACKs for top commit: sipa: utACK fa5eabe72117f6e3704858e8d5b2c57a120258ed ajtowns: ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed hebasto: ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed vasild: ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed Tree-SHA512: 06f8a200304f81533010efcc42d9f59b8c4d0ae355920c0a28efb6fa161a3e3e68f2dfffb0c009afd9c2501e6a293c6e5a419a64d718f1f4e79668ab2ab1fcdc --- doc/developer-notes.md | 2 +- src/index/base.h | 2 +- src/net_processing.cpp | 2 +- src/sync.h | 4 ++-- src/txorphanage.h | 2 +- src/wallet/wallet.h | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 22785e7349..9e9fb162e9 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -932,7 +932,7 @@ class ChainstateManager { public: ... - bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main); + bool ProcessNewBlock(...) LOCKS_EXCLUDED(::cs_main); ... } diff --git a/src/index/base.h b/src/index/base.h index 403c8c87b1..e1f2a9b82d 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -118,7 +118,7 @@ class BaseIndex : public CValidationInterface /// sync once and only needs to process blocks in the ValidationInterface /// queue. If the index is catching up from far behind, this method does /// not block and immediately returns false. - bool BlockUntilSyncedToCurrentChain() const; + bool BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main); void Interrupt(); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 803f3d51aa..688a01d19d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -944,7 +944,7 @@ class PeerManagerImpl final : public PeerManager /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */ CTransactionRef FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main); - void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) LOCKS_EXCLUDED(cs_main) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex); + void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main); /** Process a new block. Perform any post-processing housekeeping */ void ProcessBlock(CNode& from, const std::shared_ptr& pblock, bool force_processing); diff --git a/src/sync.h b/src/sync.h index 8d02830726..cab9d559d5 100644 --- a/src/sync.h +++ b/src/sync.h @@ -59,7 +59,7 @@ std::string LocksHeld(); template void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); template -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs); +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) LOCKS_EXCLUDED(cs); void DeleteLock(void* cs); bool LockStackEmpty(); @@ -77,7 +77,7 @@ inline void CheckLastCritical(void* cs, std::string& lockname, const char* guard template inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {} template -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) {} +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) LOCKS_EXCLUDED(cs) {} inline void DeleteLock(void* cs) {} inline bool LockStackEmpty() { return true; } #endif diff --git a/src/txorphanage.h b/src/txorphanage.h index 6fc043eda6..d8ef7a5c44 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -41,7 +41,7 @@ class TxOrphanage { void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); /** Erase all orphans included in or invalidated by a new block */ - void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + void EraseForBlock(const CBlock& block) LOCKS_EXCLUDED(::g_cs_orphans); /** Limit the orphanage to the given maximum */ unsigned int LimitOrphans(unsigned int max_orphans_size) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 88ec3db2f1..af80efb535 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1402,7 +1402,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * Obviously holding cs_main/cs_wallet when going into this call may cause * deadlock */ - void BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !cs_wallet); + void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet); /** set a single wallet flag */ void SetWalletFlag(uint64_t flags); From 1db9e6ac76d895d5e80aba7bf0f0a42d13a904e6 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 23 Sep 2020 16:36:52 +0200 Subject: [PATCH 16/69] Merge #19979: Replace LockAssertion with AssertLockHeld, remove LockAssertion 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 Remove unused LockAssertion struct (Hennadii Stepanov) ab2a44297fd0796bf5797ae2a477e8e56d9c3c12 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov) 73f71e19965e07534eb47701f2b23c9ed59ef475 refactor: Use explicit function type instead of template (Hennadii Stepanov) Pull request description: This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`. This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs ACKs for top commit: MarcoFalke: ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 ajtowns: ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 vasild: ACK 0bd1184ad Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41 --- doc/developer-notes.md | 19 ------------------- src/net.cpp | 8 ++++---- src/net.h | 12 ++++++------ src/net_processing.cpp | 12 ++++++------ src/sync.h | 14 -------------- 5 files changed, 16 insertions(+), 49 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 9e9fb162e9..162547f1dd 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -946,25 +946,6 @@ bool ChainstateManager::ProcessNewBlock(...) } ``` -- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances: - -```C++ -// net_processing.h -void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - -// net_processing.cpp -void RelayTransaction(...) -{ - AssertLockHeld(::cs_main); - - connman.ForEachNode([&txid, &wtxid](CNode* pnode) { - LockAssertion lock(::cs_main); - ... - }); -} - -``` - - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. diff --git a/src/net.cpp b/src/net.cpp index 49e9ca2c82..2b6fd4608b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3510,8 +3510,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, MasternodeProbeConn isProbe = MasternodeProbeConn::IsNotConnection; - const auto getPendingQuorumNodes = [&]() { - LockAssertion lock(cs_vPendingMasternodes); + const auto getPendingQuorumNodes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (const auto& group : masternodeQuorumNodes) { for (const auto& proRegTxHash : group.second) { @@ -3549,8 +3549,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, return ret; }; - const auto getPendingProbes = [&]() { - LockAssertion lock(cs_vPendingMasternodes); + const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) { auto dmn = mnList.GetMN(*it); diff --git a/src/net.h b/src/net.h index 248a8a6b04..c34c87104a 100644 --- a/src/net.h +++ b/src/net.h @@ -1275,6 +1275,8 @@ friend class CNode; return ForNode(id, FullyConnectedOnly, func); } + using NodeFn = std::function; + bool IsConnected(const CService& addr, std::function cond) { return ForNode(addr, cond, [](CNode* pnode){ @@ -1331,10 +1333,9 @@ friend class CNode; } }; - template - void ForEachNode(Callable&& func) + void ForEachNode(const NodeFn& fn) { - ForEachNode(FullyConnectedOnly, func); + ForEachNode(FullyConnectedOnly, fn); } template @@ -1347,10 +1348,9 @@ friend class CNode; } }; - template - void ForEachNode(Callable&& func) const + void ForEachNode(const NodeFn& fn) const { - ForEachNode(FullyConnectedOnly, func); + ForEachNode(FullyConnectedOnly, fn); } template diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 688a01d19d..d05eb8917e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2000,8 +2000,8 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha m_most_recent_compact_block = pcmpctblock; } - m_connman.ForEachNode([this, pindex, &lazy_ser, &hashBlock](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([this, pindex, &lazy_ser, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // TODO: Avoid the repeated-serialization here if (pnode->fDisconnect) return; @@ -5275,8 +5275,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) // We want to prevent recently connected to Onion peers from being disconnected here, protect them as long as // there are more non_onion nodes than onion nodes so far size_t onion_count = 0; - m_connman.ForEachNode([&](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); if (pnode->addr.IsTor() && ++onion_count <= m_connman.GetMaxOutboundOnionNodeCount()) return; // Don't disconnect masternodes just because they were slow in block announcement if (pnode->m_masternode_connection) return; @@ -5293,8 +5293,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) } }); if (worst_peer != -1) { - bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) { - LockAssertion lock(::cs_main); + bool disconnected = m_connman.ForNode(worst_peer, [&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // Only disconnect a peer that has been connected to us for // some reasonable fraction of our check-frequency, to give diff --git a/src/sync.h b/src/sync.h index cab9d559d5..078ee4f6e8 100644 --- a/src/sync.h +++ b/src/sync.h @@ -452,18 +452,4 @@ class CSemaphoreGrant } }; -// Utility class for indicating to compiler thread analysis that a mutex is -// locked (when it couldn't be determined otherwise). -struct SCOPED_LOCKABLE LockAssertion -{ - template - explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) - { -#ifdef DEBUG_LOCKORDER - AssertLockHeld(mutex); -#endif - } - ~LockAssertion() UNLOCK_FUNCTION() {} -}; - #endif // BITCOIN_SYNC_H From ebae59eedfd9abcfbbb3cdb7be97eb730d62e0da Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 13 Oct 2024 15:43:07 +0000 Subject: [PATCH 17/69] fix: make sure we flush our committed best block in no-upgrade cases This was spotted when working on `feature_txindex_compatibility.py`, attempting to load old databases would fail because `MigrateDBIfNeeded()` would delete `DB_OLD_BEST_BLOCK`, write `EVODB_BEST_BLOCK`, commit it but never flush it. So when `MigrateDBIfNeeded2()` would read it again, it would note the lack of `DB_OLD_BEST_BLOCK` and conclude it was a failed run and exit. This is solved by actually flushing our new best block, which would prevent `MigrateDBIfNeeded2` from raising an objection. --- src/evo/deterministicmns.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index ac167a298b..495ca8a03b 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -1238,6 +1238,10 @@ bool CDeterministicMNManager::MigrateDBIfNeeded() auto dbTx = m_evoDb.BeginTransaction(); m_evoDb.WriteBestBlock(m_chainstate.m_chain.Tip()->GetBlockHash()); dbTx->Commit(); + if (!m_evoDb.CommitRootTransaction()) { + LogPrintf("CDeterministicMNManager::%s -- failed to commit to evoDB\n", __func__); + return false; + } return true; } @@ -1349,6 +1353,10 @@ bool CDeterministicMNManager::MigrateDBIfNeeded2() auto dbTx = m_evoDb.BeginTransaction(); m_evoDb.WriteBestBlock(m_chainstate.m_chain.Tip()->GetBlockHash()); dbTx->Commit(); + if (!m_evoDb.CommitRootTransaction()) { + LogPrintf("CDeterministicMNManager::%s -- failed to commit to evoDB\n", __func__); + return false; + } return true; } From b218f123b7c86da0c483d6e54c25468968505c52 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 15 Oct 2024 06:58:49 +0000 Subject: [PATCH 18/69] merge bitcoin#23046: Add txindex migration test --- .../feature_txindex_compatibility.py | 97 +++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 98 insertions(+) create mode 100755 test/functional/feature_txindex_compatibility.py diff --git a/test/functional/feature_txindex_compatibility.py b/test/functional/feature_txindex_compatibility.py new file mode 100755 index 0000000000..f4a8e7cf55 --- /dev/null +++ b/test/functional/feature_txindex_compatibility.py @@ -0,0 +1,97 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test that legacy txindex will be disabled on upgrade. + +Previous releases are required by this test, see test/README.md. +""" + +import os +import shutil + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.wallet import MiniWallet + + +class MempoolCompatibilityTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 3 + self.extra_args = [ + ["-reindex", "-txindex"], + ["-txindex=0"], + ["-txindex=0"], + ] + + def skip_test_if_missing_module(self): + self.skip_if_no_previous_releases() + + def setup_network(self): + self.add_nodes( + self.num_nodes, + self.extra_args, + versions=[ + 170003, # Last release with legacy txindex + None, # For MiniWallet, without migration code + 18020200, # Any release with migration code (0.18.x - 21.x) + # We are using v18.2.2 to avoid MigrateDBIfNeeded(2) routines as + # they don't handle a no-upgrade case correctly + ], + ) + # Delete v18.2.2 cached datadir to avoid making a legacy version try to + # make sense of our current database formats + shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain)) + self.start_nodes() + self.connect_nodes(0, 1) + self.connect_nodes(1, 2) + + def run_test(self): + mini_wallet = MiniWallet(self.nodes[1]) + mini_wallet.rescan_utxos() + spend_utxo = mini_wallet.get_utxo() + mini_wallet.send_self_transfer(from_node=self.nodes[1], utxo_to_spend=spend_utxo) + self.generate(self.nodes[1], 1) + + self.log.info("Check legacy txindex") + self.nodes[0].getrawtransaction(txid=spend_utxo["txid"]) # Requires -txindex + + self.stop_nodes() + legacy_chain_dir = os.path.join(self.nodes[0].datadir, self.chain) + + self.log.info("Migrate legacy txindex") + migrate_chain_dir = os.path.join(self.nodes[2].datadir, self.chain) + shutil.rmtree(migrate_chain_dir) + shutil.copytree(legacy_chain_dir, migrate_chain_dir) + with self.nodes[2].assert_debug_log([ + "Upgrading txindex database...", + "txindex is enabled at height 200", + ]): + self.start_node(2, extra_args=["-txindex"]) + self.nodes[2].getrawtransaction(txid=spend_utxo["txid"]) # Requires -txindex + + self.log.info("Drop legacy txindex") + drop_index_chain_dir = os.path.join(self.nodes[1].datadir, self.chain) + shutil.rmtree(drop_index_chain_dir) + shutil.copytree(legacy_chain_dir, drop_index_chain_dir) + self.nodes[1].assert_start_raises_init_error( + extra_args=["-txindex"], + expected_msg="Error: The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.", + ) + # Build txindex from scratch and check there is no error this time + self.start_node(1, extra_args=["-txindex"]) + self.nodes[2].getrawtransaction(txid=spend_utxo["txid"]) # Requires -txindex + + self.stop_nodes() + + self.log.info("Check migrated txindex can not be read by legacy node") + err_msg = f": You need to rebuild the database using -reindex to change -txindex.{os.linesep}Please restart with -reindex or -reindex-chainstate to recover." + shutil.rmtree(legacy_chain_dir) + shutil.copytree(migrate_chain_dir, legacy_chain_dir) + self.nodes[0].assert_start_raises_init_error(extra_args=["-txindex"], expected_msg=err_msg) + shutil.rmtree(legacy_chain_dir) + shutil.copytree(drop_index_chain_dir, legacy_chain_dir) + self.nodes[0].assert_start_raises_init_error(extra_args=["-txindex"], expected_msg=err_msg) + + +if __name__ == "__main__": + MempoolCompatibilityTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 5e0cffdf2d..9045045236 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -346,6 +346,7 @@ 'rpc_deriveaddresses.py --usecli', 'p2p_ping.py', 'rpc_scantxoutset.py', + 'feature_txindex_compatibility.py', 'feature_logging.py', 'feature_anchors.py', 'feature_coinstatsindex.py', From 1caaa85716f526c285c0c1ced91225de46448b6b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 12 Oct 2024 05:57:17 +0000 Subject: [PATCH 19/69] merge bitcoin#24138: Commit MuHash and best block together for coinstatsindex Continuation of c21f23db from dash#5501 --- src/test/coinstatsindex_tests.cpp | 42 +++++++++++++++++++++++++++++++ src/test/util/validation.cpp | 6 +++++ src/test/util/validation.h | 8 ++++++ src/validationinterface.h | 1 + 4 files changed, 57 insertions(+) diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index 6ba80397a3..653790f7d9 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -2,9 +2,11 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include +#include #include #include @@ -74,4 +76,44 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) coin_stats_index.Stop(); } +// Test shutdown between BlockConnected and ChainStateFlushed notifications, +// make sure index is not corrupted and is able to reload. +BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) +{ + CChainState& chainstate = Assert(m_node.chainman)->ActiveChainstate(); + const CChainParams& params = Params(); + { + CoinStatsIndex index{1 << 20}; + BOOST_REQUIRE(index.Start(chainstate)); + IndexWaitSynced(index); + std::shared_ptr new_block; + CBlockIndex* new_block_index = nullptr; + { + const CScript script_pub_key{CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG}; + const CBlock block = this->CreateBlock({}, script_pub_key, chainstate); + + new_block = std::make_shared(block); + + LOCK(cs_main); + BlockValidationState state; + BOOST_CHECK(CheckBlock(block, state, params.GetConsensus())); + BOOST_CHECK(chainstate.AcceptBlock(new_block, state, &new_block_index, true, nullptr, nullptr)); + CCoinsViewCache view(&chainstate.CoinsTip()); + BOOST_CHECK(chainstate.ConnectBlock(block, state, new_block_index, view)); + } + // Send block connected notification, then stop the index without + // sending a chainstate flushed notification. Prior to #24138, this + // would cause the index to be corrupted and fail to reload. + ValidationInterfaceTest::BlockConnected(index, new_block, new_block_index); + index.Stop(); + } + + { + CoinStatsIndex index{1 << 20}; + // Make sure the index can be loaded. + BOOST_REQUIRE(index.Start(chainstate)); + index.Stop(); + } +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/validation.cpp b/src/test/util/validation.cpp index 1aed492c3c..49535855f9 100644 --- a/src/test/util/validation.cpp +++ b/src/test/util/validation.cpp @@ -7,6 +7,7 @@ #include #include #include +#include void TestChainState::ResetIbd() { @@ -20,3 +21,8 @@ void TestChainState::JumpOutOfIbd() m_cached_finished_ibd = true; Assert(!IsInitialBlockDownload()); } + +void ValidationInterfaceTest::BlockConnected(CValidationInterface& obj, const std::shared_ptr& block, const CBlockIndex* pindex) +{ + obj.BlockConnected(block, pindex); +} diff --git a/src/test/util/validation.h b/src/test/util/validation.h index b13aa0be60..b0bc717b6c 100644 --- a/src/test/util/validation.h +++ b/src/test/util/validation.h @@ -7,6 +7,8 @@ #include +class CValidationInterface; + struct TestChainState : public CChainState { /** Reset the ibd cache to its initial state */ void ResetIbd(); @@ -14,4 +16,10 @@ struct TestChainState : public CChainState { void JumpOutOfIbd(); }; +class ValidationInterfaceTest +{ +public: + static void BlockConnected(CValidationInterface& obj, const std::shared_ptr& block, const CBlockIndex* pindex); +}; + #endif // BITCOIN_TEST_UTIL_VALIDATION_H diff --git a/src/validationinterface.h b/src/validationinterface.h index c860f7fb86..a3b45dcf9d 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -200,6 +200,7 @@ class CValidationInterface { * has been received and connected to the headers tree, though not validated yet */ virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; friend class CMainSignals; + friend class ValidationInterfaceTest; }; struct MainSignalsInstance; From 10203560f5367acd9cd96b4857b8b4a223c73550 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 9 Apr 2022 02:14:02 +0200 Subject: [PATCH 20/69] merge bitcoin#24812: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro --- src/rpc/blockchain.cpp | 25 +++++++---------- src/rpc/coinjoin.cpp | 16 ++++------- src/rpc/evo.cpp | 53 +++++++++++-------------------------- src/rpc/governance.cpp | 42 +++++++++++------------------ src/rpc/masternode.cpp | 25 +++++++---------- src/rpc/mining.cpp | 10 +++---- src/rpc/misc.cpp | 3 +-- src/rpc/quorums.cpp | 15 ++++++----- src/rpc/rawtransaction.cpp | 5 ++-- src/rpc/util.cpp | 15 ++++++----- src/util/check.h | 41 +++++++++++++++++++--------- src/wallet/rpcdump.cpp | 2 +- test/functional/rpc_misc.py | 2 +- 13 files changed, 110 insertions(+), 144 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 6bf41d7ba8..94426f7da4 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -1359,8 +1360,7 @@ static RPCHelpMan pruneblockchain() } PruneBlockFilesManual(active_chainstate, height); - const CBlockIndex* block = active_chain.Tip(); - CHECK_NONFATAL(block); + const CBlockIndex* block = CHECK_NONFATAL(active_chain.Tip()); while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) { block = block->pprev; } @@ -1635,14 +1635,13 @@ static RPCHelpMan verifychain() const int check_depth{request.params[1].isNull() ? DEFAULT_CHECKBLOCKS : request.params[1].get_int()}; const NodeContext& node = EnsureAnyNodeContext(request.context); - CHECK_NONFATAL(node.evodb); ChainstateManager& chainman = EnsureChainman(node); LOCK(cs_main); CChainState& active_chainstate = chainman.ActiveChainstate(); return CVerifyDB().VerifyDB( - active_chainstate, Params(), active_chainstate.CoinsTip(), *node.evodb, check_level, check_depth); + active_chainstate, Params(), active_chainstate.CoinsTip(), *CHECK_NONFATAL(node.evodb), check_level, check_depth); }, }; } @@ -1782,13 +1781,11 @@ RPCHelpMan getblockchaininfo() LOCK(cs_main); CChainState& active_chainstate = chainman.ActiveChainstate(); - const CBlockIndex* tip = active_chainstate.m_chain.Tip(); - CHECK_NONFATAL(tip); + const CBlockIndex* tip = CHECK_NONFATAL(active_chainstate.m_chain.Tip()); const int height = tip->nHeight; - CHECK_NONFATAL(node.mnhf_manager); - const auto ehfSignals = node.mnhf_manager->GetSignalsStage(tip); + const auto ehfSignals = CHECK_NONFATAL(node.mnhf_manager)->GetSignalsStage(tip); UniValue obj(UniValue::VOBJ); if (args.IsArgSet("-devnet")) { @@ -1808,8 +1805,7 @@ RPCHelpMan getblockchaininfo() obj.pushKV("size_on_disk", chainman.m_blockman.CalculateCurrentUsage()); obj.pushKV("pruned", fPruneMode); if (fPruneMode) { - const CBlockIndex* block = tip; - CHECK_NONFATAL(block); + const CBlockIndex* block = CHECK_NONFATAL(tip); while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) { block = block->pprev; } @@ -2863,10 +2859,8 @@ static RPCHelpMan scantxoutset() LOCK(cs_main); CChainState& active_chainstate = chainman.ActiveChainstate(); active_chainstate.ForceFlushStateToDisk(); - pcursor = active_chainstate.CoinsDB().Cursor(); - CHECK_NONFATAL(pcursor); - tip = active_chainstate.m_chain.Tip(); - CHECK_NONFATAL(tip); + pcursor = CHECK_NONFATAL(active_chainstate.CoinsDB().Cursor()); + tip = CHECK_NONFATAL(active_chainstate.m_chain.Tip()); } bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins, node.rpc_interruption_point); result.pushKV("success", res); @@ -3061,8 +3055,7 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil } pcursor = chainstate.CoinsDB().Cursor(); - tip = chainstate.m_blockman.LookupBlockIndex(stats.hashBlock); - CHECK_NONFATAL(tip); + tip = CHECK_NONFATAL(chainstate.m_blockman.LookupBlockIndex(stats.hashBlock)); } SnapshotMetadata metadata{tip->GetBlockHash(), stats.coins_count, tip->nChainTx}; diff --git a/src/rpc/coinjoin.cpp b/src/rpc/coinjoin.cpp index 0ce1bffff9..8e477ce8ce 100644 --- a/src/rpc/coinjoin.cpp +++ b/src/rpc/coinjoin.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -83,11 +84,8 @@ static RPCHelpMan coinjoin_reset() ValidateCoinJoinArguments(); - CHECK_NONFATAL(node.coinjoin_loader); - auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName()); - - CHECK_NONFATAL(cj_clientman); - cj_clientman->ResetPool(); + auto cj_clientman = CHECK_NONFATAL(node.coinjoin_loader)->walletman().Get(wallet->GetName()); + CHECK_NONFATAL(cj_clientman)->ResetPool(); return "Mixing was reset"; }, @@ -126,10 +124,7 @@ static RPCHelpMan coinjoin_start() throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Please unlock wallet for mixing with walletpassphrase first."); } - CHECK_NONFATAL(node.coinjoin_loader); - auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName()); - - CHECK_NONFATAL(cj_clientman); + auto cj_clientman = CHECK_NONFATAL(CHECK_NONFATAL(node.coinjoin_loader)->walletman().Get(wallet->GetName())); if (!cj_clientman->StartMixing()) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Mixing has been started already."); } @@ -450,8 +445,7 @@ static RPCHelpMan getcoinjoininfo() return obj; } - auto manager = node.coinjoin_loader->walletman().Get(wallet->GetName()); - CHECK_NONFATAL(manager != nullptr); + auto* manager = CHECK_NONFATAL(node.coinjoin_loader->walletman().Get(wallet->GetName())); manager->GetJsonInfo(obj); std::string warning_msg{""}; diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index 30fdab04b7..b584f888ab 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -628,8 +629,7 @@ static UniValue protx_register_common_wrapper(const JSONRPCRequest& request, const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.chain_helper); - CChainstateHelper& chain_helper = *node.chain_helper; + CChainstateHelper& chain_helper = *CHECK_NONFATAL(node.chain_helper); const bool isEvoRequested = mnType == MnType::Evo; @@ -855,8 +855,7 @@ static RPCHelpMan protx_register_submit() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.chain_helper); - CChainstateHelper& chain_helper = *node.chain_helper; + CChainstateHelper& chain_helper = *CHECK_NONFATAL(node.chain_helper); std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); if (!wallet) return NullUniValue; @@ -954,11 +953,8 @@ static UniValue protx_update_service_common_wrapper(const JSONRPCRequest& reques const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CDeterministicMNManager& dmnman = *node.dmnman; - - CHECK_NONFATAL(node.chain_helper); - CChainstateHelper& chain_helper = *node.chain_helper; + CDeterministicMNManager& dmnman = *CHECK_NONFATAL(node.dmnman); + CChainstateHelper& chain_helper = *CHECK_NONFATAL(node.chain_helper); const bool isEvoRequested = mnType == MnType::Evo; std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); @@ -1092,11 +1088,8 @@ static RPCHelpMan protx_update_registrar_wrapper(bool specific_legacy_bls_scheme const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CDeterministicMNManager& dmnman = *node.dmnman; - - CHECK_NONFATAL(node.chain_helper); - CChainstateHelper& chain_helper = *node.chain_helper; + CDeterministicMNManager& dmnman = *CHECK_NONFATAL(node.dmnman); + CChainstateHelper& chain_helper = *CHECK_NONFATAL(node.chain_helper); std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); if (!wallet) return NullUniValue; @@ -1207,12 +1200,8 @@ static RPCHelpMan protx_revoke() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CDeterministicMNManager& dmnman = *node.dmnman; - - CHECK_NONFATAL(node.chain_helper); - CChainstateHelper& chain_helper = *node.chain_helper; - + CDeterministicMNManager& dmnman = *CHECK_NONFATAL(node.dmnman); + CChainstateHelper& chain_helper = *CHECK_NONFATAL(node.chain_helper); std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; @@ -1370,11 +1359,8 @@ static RPCHelpMan protx_list() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CDeterministicMNManager& dmnman = *node.dmnman; - - CHECK_NONFATAL(node.mn_metaman); - CMasternodeMetaMan& mn_metaman = *node.mn_metaman; + CDeterministicMNManager& dmnman = *CHECK_NONFATAL(node.dmnman); + CMasternodeMetaMan& mn_metaman = *CHECK_NONFATAL(node.mn_metaman); std::shared_ptr wallet{nullptr}; #ifdef ENABLE_WALLET @@ -1486,11 +1472,8 @@ static RPCHelpMan protx_info() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CDeterministicMNManager& dmnman = *node.dmnman; - - CHECK_NONFATAL(node.mn_metaman); - CMasternodeMetaMan& mn_metaman = *node.mn_metaman; + CDeterministicMNManager& dmnman = *CHECK_NONFATAL(node.dmnman); + CMasternodeMetaMan& mn_metaman = *CHECK_NONFATAL(node.mn_metaman); std::shared_ptr wallet{nullptr}; #ifdef ENABLE_WALLET @@ -1560,11 +1543,8 @@ static RPCHelpMan protx_diff() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CDeterministicMNManager& dmnman = *node.dmnman; - - CHECK_NONFATAL(node.llmq_ctx); - const LLMQContext& llmq_ctx = *node.llmq_ctx; + CDeterministicMNManager& dmnman = *CHECK_NONFATAL(node.dmnman); + const LLMQContext& llmq_ctx = *CHECK_NONFATAL(node.llmq_ctx); LOCK(cs_main); uint256 baseBlockHash = ParseBlock(request.params[0], chainman, "baseBlock"); @@ -1621,8 +1601,7 @@ static RPCHelpMan protx_listdiff() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CDeterministicMNManager& dmnman = *node.dmnman; + CDeterministicMNManager& dmnman = *CHECK_NONFATAL(node.dmnman); LOCK(cs_main); UniValue ret(UniValue::VOBJ); diff --git a/src/rpc/governance.cpp b/src/rpc/governance.cpp index 2137e91a7b..5123945c3f 100644 --- a/src/rpc/governance.cpp +++ b/src/rpc/governance.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -194,12 +195,11 @@ static RPCHelpMan gobject_prepare() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); { LOCK(cs_main); std::string strError = ""; - if (!govobj.IsValidLocally(node.dmnman->GetListAtChainTip(), chainman, strError, false)) + if (!govobj.IsValidLocally(CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(), chainman, strError, false)) throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + govobj.GetHash().ToString() + " - " + strError); } @@ -303,14 +303,12 @@ static RPCHelpMan gobject_submit() { const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CHECK_NONFATAL(node.govman); if(!node.mn_sync->IsBlockchainSynced()) { throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Must wait for client to sync with masternode network. Try again in a minute or so."); } - auto mnList = node.dmnman->GetListAtChainTip(); + auto mnList = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); if (node.mn_activeman) { const bool fMnFound = mnList.HasValidMNByCollateral(node.mn_activeman->GetOutPoint()); @@ -373,7 +371,6 @@ static RPCHelpMan gobject_submit() } LOCK2(cs_main, mempool.cs); - CHECK_NONFATAL(node.dmnman); std::string strError; if (!govobj.IsValidLocally(node.dmnman->GetListAtChainTip(), chainman, strError, fMissingConfirmations, true) && !fMissingConfirmations) { @@ -384,7 +381,7 @@ static RPCHelpMan gobject_submit() // RELAY THIS OBJECT // Reject if rate check fails but don't update buffer - if (!node.govman->MasternodeRateCheck(govobj)) { + if (!CHECK_NONFATAL(node.govman)->MasternodeRateCheck(govobj)) { LogPrintf("gobject(submit) -- Object submission rejected because of rate check failure - hash = %s\n", strHash); throw JSONRPCError(RPC_INVALID_PARAMETER, "Object creation rate limit exceeded"); } @@ -393,9 +390,8 @@ static RPCHelpMan gobject_submit() PeerManager& peerman = EnsurePeerman(node); if (fMissingConfirmations) { - CHECK_NONFATAL(node.mn_sync); node.govman->AddPostponedObject(govobj); - govobj.Relay(peerman, *node.mn_sync); + govobj.Relay(peerman, *CHECK_NONFATAL(node.mn_sync)); } else { node.govman->AddGovernanceObject(govobj, peerman); } @@ -452,7 +448,7 @@ static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const CWallet int nSuccessful = 0; int nFailed = 0; - auto mnList = node.dmnman->GetListAtChainTip(); + auto mnList = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); UniValue resultsObj(UniValue::VOBJ); @@ -529,7 +525,6 @@ static RPCHelpMan gobject_vote_many() if (!wallet) return NullUniValue; const NodeContext& node = EnsureAnyNodeContext(request.context); - CHECK_NONFATAL(node.dmnman); uint256 hash(ParseHashV(request.params[0], "Object hash")); std::string strVoteSignal = request.params[1].get_str(); @@ -551,7 +546,7 @@ static RPCHelpMan gobject_vote_many() std::map votingKeys; - auto mnList = node.dmnman->GetListAtChainTip(); + auto mnList = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); mnList.ForEachMN(true, [&](auto& dmn) { const bool is_mine = CheckWalletOwnsKey(*wallet, dmn.pdmnState->keyIDVoting); if (is_mine) { @@ -583,7 +578,6 @@ static RPCHelpMan gobject_vote_alias() if (!wallet) return NullUniValue; const NodeContext& node = EnsureAnyNodeContext(request.context); - CHECK_NONFATAL(node.dmnman); uint256 hash(ParseHashV(request.params[0], "Object hash")); std::string strVoteSignal = request.params[1].get_str(); @@ -604,7 +598,7 @@ static RPCHelpMan gobject_vote_alias() EnsureWalletIsUnlocked(*wallet); uint256 proTxHash(ParseHashV(request.params[3], "protx-hash")); - auto dmn = node.dmnman->GetListAtChainTip().GetValidMN(proTxHash); + auto dmn = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetValidMN(proTxHash); if (!dmn) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid or unknown proTxHash"); } @@ -718,11 +712,10 @@ static RPCHelpMan gobject_list_helper(const bool make_a_diff) const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - CHECK_NONFATAL(node.govman); const int64_t last_time = make_a_diff ? node.govman->GetLastDiffTime() : 0; - return ListObjects(*node.govman, node.dmnman->GetListAtChainTip(), chainman, strCachedSignal, strType, last_time); + return ListObjects(*CHECK_NONFATAL(node.govman), CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(), chainman, + strCachedSignal, strType, last_time); }, }; } @@ -759,8 +752,8 @@ static RPCHelpMan gobject_get() // FIND THE GOVERNANCE OBJECT THE USER IS LOOKING FOR const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman && node.govman); + CHECK_NONFATAL(node.govman); LOCK2(cs_main, node.govman->cs); const CGovernanceObject* pGovObj = node.govman->FindConstGovernanceObject(hash); @@ -785,7 +778,7 @@ static RPCHelpMan gobject_get() // SHOW (MUCH MORE) INFORMATION ABOUT VOTES FOR GOVERNANCE OBJECT (THAN LIST/DIFF ABOVE) // -- FUNDING VOTING RESULTS - auto tip_mn_list = node.dmnman->GetListAtChainTip(); + auto tip_mn_list = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); UniValue objFundingResult(UniValue::VOBJ); objFundingResult.pushKV("AbsoluteYesCount", pGovObj->GetAbsoluteYesCount(tip_mn_list, VOTE_SIGNAL_FUNDING)); @@ -858,8 +851,8 @@ static RPCHelpMan gobject_getcurrentvotes() // FIND OBJECT USER IS LOOKING FOR const NodeContext& node = EnsureAnyNodeContext(request.context); - CHECK_NONFATAL(node.govman); + CHECK_NONFATAL(node.govman); LOCK(node.govman->cs); const CGovernanceObject* pGovObj = node.govman->FindConstGovernanceObject(hash); @@ -872,10 +865,9 @@ static RPCHelpMan gobject_getcurrentvotes() UniValue bResult(UniValue::VOBJ); // GET MATCHING VOTES BY HASH, THEN SHOW USERS VOTE INFORMATION - CHECK_NONFATAL(node.dmnman); std::vector vecVotes = node.govman->GetCurrentVotes(hash, mnCollateralOutpoint); for (const auto& vote : vecVotes) { - bResult.pushKV(vote.GetHash().ToString(), vote.ToString(node.dmnman->GetListAtChainTip())); + bResult.pushKV(vote.GetHash().ToString(), vote.ToString(CHECK_NONFATAL(node.dmnman)->GetListAtChainTip())); } return bResult; @@ -975,8 +967,7 @@ static RPCHelpMan voteraw() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Malformed base64 encoding"); } - CHECK_NONFATAL(node.dmnman); - const auto tip_mn_list = node.dmnman->GetListAtChainTip(); + const auto tip_mn_list = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); auto dmn = tip_mn_list.GetValidMNByCollateral(outpoint); if (!dmn) { @@ -1034,7 +1025,6 @@ static RPCHelpMan getgovernanceinfo() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureAnyChainman(request.context); - CHECK_NONFATAL(node.dmnman); const auto* pindex = WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()); int nBlockHeight = pindex->nHeight; @@ -1048,7 +1038,7 @@ static RPCHelpMan getgovernanceinfo() obj.pushKV("superblockmaturitywindow", Params().GetConsensus().nSuperblockMaturityWindow); obj.pushKV("lastsuperblock", nLastSuperblock); obj.pushKV("nextsuperblock", nNextSuperblock); - obj.pushKV("fundingthreshold", int(node.dmnman->GetListAtChainTip().GetValidWeightedMNsCount() / 10)); + obj.pushKV("fundingthreshold", int(CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetValidWeightedMNsCount() / 10)); obj.pushKV("governancebudget", ValueFromAmount(CSuperblock::GetPaymentsLimit(chainman.ActiveChain(), nNextSuperblock))); return obj; diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index 5b30f0944b..ee5fcbdea0 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -138,8 +139,7 @@ static RPCHelpMan masternode_winner() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - return GetNextMasternodeForPayment(chainman.ActiveChain(), *node.dmnman, 10); + return GetNextMasternodeForPayment(chainman.ActiveChain(), *CHECK_NONFATAL(node.dmnman), 10); }, }; } @@ -159,8 +159,7 @@ static RPCHelpMan masternode_current() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); - return GetNextMasternodeForPayment(chainman.ActiveChain(), *node.dmnman, 1); + return GetNextMasternodeForPayment(chainman.ActiveChain(), *CHECK_NONFATAL(node.dmnman), 1); }, }; } @@ -212,7 +211,7 @@ static RPCHelpMan masternode_status() [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { const NodeContext& node = EnsureAnyNodeContext(request.context); - CHECK_NONFATAL(node.dmnman); + if (!node.mn_activeman) { throw JSONRPCError(RPC_INTERNAL_ERROR, "This node does not run an active masternode."); } @@ -221,7 +220,7 @@ static RPCHelpMan masternode_status() // keep compatibility with legacy status for now (might get deprecated/removed later) mnObj.pushKV("outpoint", node.mn_activeman->GetOutPoint().ToStringShort()); mnObj.pushKV("service", node.mn_activeman->GetService().ToStringAddrPort()); - CDeterministicMNCPtr dmn = node.dmnman->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash()); + CDeterministicMNCPtr dmn = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash()); if (dmn) { mnObj.pushKV("proTxHash", dmn->proTxHash.ToString()); mnObj.pushKV("type", std::string(GetMnType(dmn->nType).description)); @@ -243,12 +242,12 @@ static std::string GetRequiredPaymentsString(CGovernanceManager& govman, const C if (payee) { CTxDestination dest; if (!ExtractDestination(payee->pdmnState->scriptPayout, dest)) { - CHECK_NONFATAL(false); + NONFATAL_UNREACHABLE(); } strPayments = EncodeDestination(dest); if (payee->nOperatorReward != 0 && payee->pdmnState->scriptOperatorPayout != CScript()) { if (!ExtractDestination(payee->pdmnState->scriptOperatorPayout, dest)) { - CHECK_NONFATAL(false); + NONFATAL_UNREACHABLE(); } strPayments += ", " + EncodeDestination(dest); } @@ -310,14 +309,11 @@ static RPCHelpMan masternode_winners() int nChainTipHeight = pindexTip->nHeight; int nStartHeight = std::max(nChainTipHeight - nCount, 1); - CHECK_NONFATAL(node.dmnman); - CHECK_NONFATAL(node.govman); - - const auto tip_mn_list = node.dmnman->GetListAtChainTip(); + const auto tip_mn_list = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); for (int h = nStartHeight; h <= nChainTipHeight; h++) { const CBlockIndex* pIndex = pindexTip->GetAncestor(h - 1); auto payee = node.dmnman->GetListForBlock(pIndex).GetMNPayee(pIndex); - std::string strPayments = GetRequiredPaymentsString(*node.govman, tip_mn_list, h, payee); + std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.govman), tip_mn_list, h, payee); if (strFilter != "" && strPayments.find(strFilter) == std::string::npos) continue; obj.pushKV(strprintf("%d", h), strPayments); } @@ -558,11 +554,10 @@ static RPCHelpMan masternodelist_helper(bool is_composite) const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CHECK_NONFATAL(node.dmnman); UniValue obj(UniValue::VOBJ); - const auto mnList = node.dmnman->GetListAtChainTip(); + const auto mnList = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); const auto dmnToStatus = [&](const auto& dmn) { if (mnList.IsMNValid(dmn)) { return "ENABLED"; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index af51ecc9f8..0444fc5a91 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -391,12 +392,11 @@ static RPCHelpMan generateblock() block.vtx.insert(block.vtx.end(), txs.begin(), txs.end()); { - CHECK_NONFATAL(node.evodb); - LOCK(cs_main); BlockValidationState state; - if (!TestBlockValidity(state, *llmq_ctx.clhandler, *node.evodb, chainparams, active_chainstate, block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), false, false)) { + if (!TestBlockValidity(state, *llmq_ctx.clhandler, *CHECK_NONFATAL(node.evodb), chainparams, active_chainstate, + block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), false, false)) { throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.GetRejectReason())); } } @@ -710,14 +710,14 @@ static RPCHelpMan getblocktemplate() } LLMQContext& llmq_ctx = EnsureLLMQContext(node); - CHECK_NONFATAL(node.evodb); CBlockIndex* const pindexPrev = active_chain.Tip(); // TestBlockValidity only supports blocks built on the current Tip if (block.hashPrevBlock != pindexPrev->GetBlockHash()) return "inconclusive-not-best-prevblk"; BlockValidationState state; - TestBlockValidity(state, *llmq_ctx.clhandler, *node.evodb, Params(), active_chainstate, block, pindexPrev, false, true); + TestBlockValidity(state, *llmq_ctx.clhandler, *CHECK_NONFATAL(node.evodb), Params(), active_chainstate, + block, pindexPrev, false, true); return BIP22ValidationResult(state); } diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index 0250036d04..40ed64f821 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -1194,9 +1194,8 @@ static RPCHelpMan mockscheduler() throw std::runtime_error("delta_time must be between 1 and 3600 seconds (1 hr)"); } - auto* node_context = GetContext(request.context); + auto* node_context = CHECK_NONFATAL(GetContext(request.context)); // protect against null pointer dereference - CHECK_NONFATAL(node_context); CHECK_NONFATAL(node_context->scheduler); node_context->scheduler->MockForward(std::chrono::seconds(delta_seconds)); diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index f6c27f4c73..3aa5de1a5a 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -282,7 +283,6 @@ static RPCHelpMan quorum_dkgstatus() const ChainstateManager& chainman = EnsureChainman(node); const LLMQContext& llmq_ctx = EnsureLLMQContext(node); const CConnman& connman = EnsureConnman(node); - CHECK_NONFATAL(node.dmnman); CHECK_NONFATAL(node.sporkman); int detailLevel = 0; @@ -296,7 +296,7 @@ static RPCHelpMan quorum_dkgstatus() llmq::CDKGDebugStatus status; llmq_ctx.dkg_debugman->GetLocalDebugStatus(status); - auto ret = status.ToJson(*node.dmnman, chainman, detailLevel); + auto ret = status.ToJson(*CHECK_NONFATAL(node.dmnman), chainman, detailLevel); CBlockIndex* pindexTip = WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()); int tipHeight = pindexTip->nHeight; @@ -385,7 +385,6 @@ static RPCHelpMan quorum_memberof() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); const LLMQContext& llmq_ctx = EnsureLLMQContext(node); - CHECK_NONFATAL(node.dmnman); uint256 protxHash(ParseHashV(request.params[0], "proTxHash")); int scanQuorumsCount = -1; @@ -397,7 +396,7 @@ static RPCHelpMan quorum_memberof() } const CBlockIndex* pindexTip = WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()); - auto mnList = node.dmnman->GetListForBlock(pindexTip); + auto mnList = CHECK_NONFATAL(node.dmnman)->GetListForBlock(pindexTip); auto dmn = mnList.GetMN(protxHash); if (!dmn) { throw JSONRPCError(RPC_INVALID_PARAMETER, "masternode not found"); @@ -842,7 +841,7 @@ static RPCHelpMan quorum_rotationinfo() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); const LLMQContext& llmq_ctx = EnsureLLMQContext(node); - CHECK_NONFATAL(node.dmnman); + ; llmq::CGetQuorumRotationInfo cmd; llmq::CQuorumRotationInfo quorumRotationInfoRet; @@ -859,7 +858,8 @@ static RPCHelpMan quorum_rotationinfo() LOCK(cs_main); - if (!BuildQuorumRotationInfo(*node.dmnman, chainman, *llmq_ctx.qman, *llmq_ctx.quorum_block_processor, cmd, quorumRotationInfoRet, strError)) { + if (!BuildQuorumRotationInfo(*CHECK_NONFATAL(node.dmnman), chainman, *llmq_ctx.qman, *llmq_ctx.quorum_block_processor, + cmd, quorumRotationInfoRet, strError)) { throw JSONRPCError(RPC_INVALID_REQUEST, strError); } @@ -1073,7 +1073,8 @@ static RPCHelpMan verifyislock() auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend; const auto llmq_params_opt = Params().GetLLMQ(llmqType); CHECK_NONFATAL(llmq_params_opt.has_value()); - return VerifyRecoveredSigLatestQuorums(*llmq_params_opt, chainman.ActiveChain(), *llmq_ctx.qman, signHeight, id, txid, sig); + return VerifyRecoveredSigLatestQuorums(*llmq_params_opt, chainman.ActiveChain(), *CHECK_NONFATAL(llmq_ctx.qman), + signHeight, id, txid, sig); }, }; } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 1b01eb3195..e06382711e 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -495,8 +496,6 @@ static RPCHelpMan getassetunlockstatuses() throw JSONRPCError(RPC_INTERNAL_ERROR, "No blocks in chain"); } - CHECK_NONFATAL(node.cpoolman); - std::optional poolCL{std::nullopt}; std::optional poolOnTip{std::nullopt}; std::optional nSpecificCoreHeight{std::nullopt}; @@ -506,7 +505,7 @@ static RPCHelpMan getassetunlockstatuses() if (nSpecificCoreHeight.value() < 0 || nSpecificCoreHeight.value() > chainman.ActiveChain().Height()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range"); } - poolCL = std::make_optional(node.cpoolman->GetCreditPool(chainman.ActiveChain()[nSpecificCoreHeight.value()], Params().GetConsensus())); + poolCL = std::make_optional(CHECK_NONFATAL(node.cpoolman)->GetCreditPool(chainman.ActiveChain()[nSpecificCoreHeight.value()], Params().GetConsensus())); } else { const auto pBlockIndexBestCL = [&]() -> const CBlockIndex* { diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 727fa5e694..4a63378b84 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -10,6 +10,7 @@ #include