From 57af825f115e272b62b8164cbcd514250d10b0a1 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 11 Jan 2025 18:10:14 +0000 Subject: [PATCH 01/14] merge bitcoin#23065: Allow UTXO locks to be written to wallet DB --- doc/release-notes-6530.md | 15 +++++++++ src/interfaces/wallet.h | 4 +-- src/qt/coincontroldialog.cpp | 4 +-- src/rpc/client.cpp | 1 + src/wallet/interfaces.cpp | 10 +++--- src/wallet/rpcwallet.cpp | 29 +++++++++++++---- src/wallet/wallet.cpp | 58 +++++++++++++++++++++++---------- src/wallet/wallet.h | 12 ++++--- src/wallet/walletdb.cpp | 17 ++++++++++ src/wallet/walletdb.h | 4 +++ test/functional/wallet_basic.py | 36 ++++++++++++++++++++ 11 files changed, 152 insertions(+), 38 deletions(-) create mode 100644 doc/release-notes-6530.md diff --git a/doc/release-notes-6530.md b/doc/release-notes-6530.md new file mode 100644 index 0000000000000..4988cbfb55a02 --- /dev/null +++ b/doc/release-notes-6530.md @@ -0,0 +1,15 @@ +Notable changes +=============== + +Updated RPCs +------------ + +- `lockunspent` now optionally takes a third parameter, `persistent`, which +causes the lock to be written persistently to the wallet database. This +allows UTXOs to remain locked even after node restarts or crashes. + +GUI changes +----------- + +- UTXOs which are locked via the GUI are now stored persistently in the +wallet database, so are not lost on node shutdown or crash. diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 9e9ba4ec27366..6de6776ef5d26 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -136,10 +136,10 @@ class Wallet virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; //! Lock coin. - virtual void lockCoin(const COutPoint& output) = 0; + virtual bool lockCoin(const COutPoint& output, const bool write_to_db) = 0; //! Unlock coin. - virtual void unlockCoin(const COutPoint& output) = 0; + virtual bool unlockCoin(const COutPoint& output) = 0; //! Return whether coin is locked. virtual bool isLockedCoin(const COutPoint& output) = 0; diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 89206909d3dad..541e8b2980365 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -207,7 +207,7 @@ void CoinControlDialog::buttonToggleLockClicked() item->setIcon(COLUMN_CHECKBOX, QIcon()); } else{ - model->wallet().lockCoin(outpt); + model->wallet().lockCoin(outpt, /*write_to_db=*/true); item->setDisabled(true); item->setIcon(COLUMN_CHECKBOX, GUIUtil::getIcon("lock_closed", GUIUtil::ThemedColor::RED)); } @@ -300,7 +300,7 @@ void CoinControlDialog::lockCoin() contextMenuItem->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked); COutPoint outpt(uint256S(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt()); - model->wallet().lockCoin(outpt); + model->wallet().lockCoin(outpt, /*write_to_db=*/true); contextMenuItem->setDisabled(true); contextMenuItem->setIcon(COLUMN_CHECKBOX, GUIUtil::getIcon("lock_closed", GUIUtil::ThemedColor::RED)); updateLabelLocked(); diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 90c8590214677..f28247a165399 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -154,6 +154,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "gettxoutsetinfo", 2, "use_index"}, { "lockunspent", 0, "unlock" }, { "lockunspent", 1, "transactions" }, + { "lockunspent", 2, "persistent" }, { "send", 0, "outputs" }, { "send", 1, "conf_target" }, { "send", 3, "fee_rate"}, diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d1996ecc1932a..c00924572abd7 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -238,15 +238,17 @@ class WalletImpl : public Wallet WalletBatch batch{m_wallet->GetDatabase()}; return m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } - void lockCoin(const COutPoint& output) override + bool lockCoin(const COutPoint& output, const bool write_to_db) override { LOCK(m_wallet->cs_wallet); - return m_wallet->LockCoin(output); + std::unique_ptr batch = write_to_db ? std::make_unique(m_wallet->GetDatabase()) : nullptr; + return m_wallet->LockCoin(output, batch.get()); } - void unlockCoin(const COutPoint& output) override + bool unlockCoin(const COutPoint& output) override { LOCK(m_wallet->cs_wallet); - return m_wallet->UnlockCoin(output); + std::unique_ptr batch = std::make_unique(m_wallet->GetDatabase()); + return m_wallet->UnlockCoin(output, batch.get()); } bool isLockedCoin(const COutPoint& output) override { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5c67b88bbd84d..6d4d76b5833f5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2204,8 +2204,9 @@ static RPCHelpMan lockunspent() "If no transaction outputs are specified when unlocking then all current locked transaction outputs are unlocked.\n" "A locked transaction output will not be chosen by automatic coin selection, when spending Dash.\n" "Manually selected coins are automatically unlocked.\n" - "Locks are stored in memory only. Nodes start with zero locked outputs, and the locked output list\n" - "is always cleared (by virtue of process exit) when a node stops or fails.\n" + "Locks are stored in memory only, unless persistent=true, in which case they will be written to the\n" + "wallet database and loaded on node start. Unwritten (persistent=false) locks are always cleared\n" + "(by virtue of process exit) when a node stops or fails. Unlocking will clear both persistent and not.\n" "Also see the listunspent call\n", { {"unlock", RPCArg::Type::BOOL, RPCArg::Optional::NO, "Whether to unlock (true) or lock (false) the specified transactions"}, @@ -2219,6 +2220,7 @@ static RPCHelpMan lockunspent() }, }, }, + {"persistent", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to write/erase this lock in the wallet database, or keep the change in memory only. Ignored for unlocking."}, }, RPCResult{ RPCResult::Type::BOOL, "", "Whether the command was successful or not" @@ -2232,6 +2234,8 @@ static RPCHelpMan lockunspent() + HelpExampleCli("listlockunspent", "") + "\nUnlock the transaction again\n" + HelpExampleCli("lockunspent", "true \"[{\\\"txid\\\":\\\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\\\",\\\"vout\\\":1}]\"") + + "\nLock the transaction persistently in the wallet database\n" + + HelpExampleCli("lockunspent", "false \"[{\\\"txid\\\":\\\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\\\",\\\"vout\\\":1}]\" true") + "\nAs a JSON-RPC call\n" + HelpExampleRpc("lockunspent", "false, \"[{\\\"txid\\\":\\\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\\\",\\\"vout\\\":1}]\"") }, @@ -2250,9 +2254,13 @@ static RPCHelpMan lockunspent() bool fUnlock = request.params[0].get_bool(); + const bool persistent{request.params[2].isNull() ? false : request.params[2].get_bool()}; + if (request.params[1].isNull()) { - if (fUnlock) - pwallet->UnlockAllCoins(); + if (fUnlock) { + if (!pwallet->UnlockAllCoins()) + throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coins failed"); + } return true; } @@ -2303,17 +2311,24 @@ static RPCHelpMan lockunspent() throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected locked output"); } - if (!fUnlock && is_locked) { + if (!fUnlock && is_locked && !persistent) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output already locked"); } outputs.push_back(outpt); } + std::unique_ptr batch = nullptr; + // Unlock is always persistent + if (fUnlock || persistent) batch = std::make_unique(pwallet->GetDatabase()); + // Atomically set (un)locked status for the outputs. for (const COutPoint& outpt : outputs) { - if (fUnlock) pwallet->UnlockCoin(outpt); - else pwallet->LockCoin(outpt); + if (fUnlock) { + if (!pwallet->UnlockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed"); + } else { + if (!pwallet->LockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed"); + } } return true; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 05b31cd5439e5..d2c06fadabb6c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -625,12 +625,17 @@ bool CWallet::IsSpent(const uint256& hash, unsigned int n) const return false; } -void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid) +void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch) { mapTxSpends.insert(std::make_pair(outpoint, wtxid)); setWalletUTXO.erase(outpoint); - setLockedCoins.erase(outpoint); + if (batch) { + UnlockCoin(outpoint, batch); + } else { + WalletBatch temp_batch(GetDatabase()); + UnlockCoin(outpoint, &temp_batch); + } std::pair range; range = mapTxSpends.equal_range(outpoint); @@ -638,7 +643,7 @@ void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid) } -void CWallet::AddToSpends(const uint256& wtxid) +void CWallet::AddToSpends(const uint256& wtxid, WalletBatch* batch) { auto it = mapWallet.find(wtxid); assert(it != mapWallet.end()); @@ -647,7 +652,7 @@ void CWallet::AddToSpends(const uint256& wtxid) return; for (const CTxIn& txin : thisTx.tx->vin) - AddToSpends(txin.prevout, wtxid); + AddToSpends(txin.prevout, wtxid, batch); } bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) @@ -915,7 +920,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio wtx.nOrderPos = IncOrderPosNext(&batch); wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx)); wtx.nTimeSmart = ComputeTimeSmart(wtx); - AddToSpends(hash); + AddToSpends(hash, &batch); std::vector> outputs; for(unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { @@ -927,7 +932,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio // TODO: refactor duplicated code between CWallet::AddToWallet and CWallet::AutoLockMasternodeCollaterals if (m_chain) { for (const auto& outPoint : m_chain->listMNCollaterials(outputs)) { - LockCoin(outPoint); + LockCoin(outPoint, &batch); } } } @@ -4444,32 +4449,49 @@ void ReserveDestination::ReturnDestination() address = CNoDestination(); } -void CWallet::LockCoin(const COutPoint& output) +void CWallet::RecalculateMixedCredit(const uint256 hash) { AssertLockHeld(cs_wallet); - setLockedCoins.insert(output); - std::map::iterator it = mapWallet.find(output.hash); - if (it != mapWallet.end()) it->second.MarkDirty(); // recalculate all credits for this tx - + if (auto it = mapWallet.find(hash); it != mapWallet.end()) { + // Recalculate all credits for this tx + it->second.MarkDirty(); + } fAnonymizableTallyCached = false; fAnonymizableTallyCachedNonDenom = false; } -void CWallet::UnlockCoin(const COutPoint& output) +bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch) { AssertLockHeld(cs_wallet); - setLockedCoins.erase(output); - std::map::iterator it = mapWallet.find(output.hash); - if (it != mapWallet.end()) it->second.MarkDirty(); // recalculate all credits for this tx + setLockedCoins.insert(output); + RecalculateMixedCredit(output.hash); + if (batch) { + return batch->WriteLockedUTXO(output); + } + return true; +} - fAnonymizableTallyCached = false; - fAnonymizableTallyCachedNonDenom = false; +bool CWallet::UnlockCoin(const COutPoint& output, WalletBatch* batch) +{ + AssertLockHeld(cs_wallet); + bool was_locked = setLockedCoins.erase(output); + RecalculateMixedCredit(output.hash); + if (batch && was_locked) { + return batch->EraseLockedUTXO(output); + } + return true; } -void CWallet::UnlockAllCoins() +bool CWallet::UnlockAllCoins() { AssertLockHeld(cs_wallet); + bool success = true; + WalletBatch batch(GetDatabase()); + for (auto it = setLockedCoins.begin(); it != setLockedCoins.end(); ++it) { + success &= batch.EraseLockedUTXO(*it); + } setLockedCoins.clear(); + return success; } bool CWallet::IsLockedCoin(uint256 hash, unsigned int n) const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 99af94418e70b..f2b8713d77d74 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -759,8 +759,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati */ typedef std::multimap TxSpends; TxSpends mapTxSpends GUARDED_BY(cs_wallet); - void AddToSpends(const COutPoint& outpoint, const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void AddToSpends(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void AddToSpends(const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::set setWalletUTXO; mutable std::map mapOutpointRoundsCache GUARDED_BY(cs_wallet); @@ -1032,10 +1032,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::vector GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; + void RecalculateMixedCredit(const uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool UnlockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void ListLockedCoins(std::vector& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void ListProTxCoins(std::vector& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 5992579d857f0..028c41c1e73de 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -47,6 +47,7 @@ const std::string HDCHAIN{"hdchain"}; const std::string HDPUBKEY{"hdpubkey"}; const std::string KEYMETA{"keymeta"}; const std::string KEY{"key"}; +const std::string LOCKED_UTXO{"lockedutxo"}; const std::string MASTER_KEY{"mkey"}; const std::string MINVERSION{"minversion"}; const std::string NAME{"name"}; @@ -308,6 +309,16 @@ bool WalletBatch::WriteDescriptorCacheItems(const uint256& desc_id, const Descri return true; } +bool WalletBatch::WriteLockedUTXO(const COutPoint& output) +{ + return WriteIC(std::make_pair(DBKeys::LOCKED_UTXO, std::make_pair(output.hash, output.n)), uint8_t{'1'}); +} + +bool WalletBatch::EraseLockedUTXO(const COutPoint& output) +{ + return EraseIC(std::make_pair(DBKeys::LOCKED_UTXO, std::make_pair(output.hash, output.n))); +} + class CWalletScanState { public: unsigned int nKeys{0}; @@ -709,6 +720,12 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, wss.m_descriptor_crypt_keys.insert(std::make_pair(std::make_pair(desc_id, pubkey.GetID()), std::make_pair(pubkey, privkey))); wss.fIsEncrypted = true; + } else if (strType == DBKeys::LOCKED_UTXO) { + uint256 hash; + uint32_t n; + ssKey >> hash; + ssKey >> n; + pwallet->LockCoin(COutPoint(hash, n)); } else if (strType != DBKeys::BESTBLOCK && strType != DBKeys::BESTBLOCK_NOMERKLE && strType != DBKeys::MINVERSION && strType != DBKeys::ACENTRY && strType != DBKeys::VERSION && strType != DBKeys::SETTINGS && diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index c2d9dfd256c98..8a980405ef139 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -74,6 +74,7 @@ extern const std::string HDCHAIN; extern const std::string HDPUBKEY; extern const std::string KEY; extern const std::string KEYMETA; +extern const std::string LOCKED_UTXO; extern const std::string MASTER_KEY; extern const std::string MINVERSION; extern const std::string NAME; @@ -219,6 +220,9 @@ class WalletBatch bool WriteDescriptorLastHardenedCache(const CExtPubKey& xpub, const uint256& desc_id, uint32_t key_exp_index); bool WriteDescriptorCacheItems(const uint256& desc_id, const DescriptorCache& cache); + bool WriteLockedUTXO(const COutPoint& output); + bool EraseLockedUTXO(const COutPoint& output); + /// Write destination data key,value tuple to database bool WriteDestData(const std::string &address, const std::string &key, const std::string &value); /// Erase destination data tuple from wallet database diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 603e0d50060eb..79eb44fa25134 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -129,13 +129,49 @@ def run_test(self): # Exercise locking of unspent outputs unspent_0 = self.nodes[2].listunspent()[0] unspent_0 = {"txid": unspent_0["txid"], "vout": unspent_0["vout"]} + # Trying to unlock an output which isn't locked should error assert_raises_rpc_error(-8, "Invalid parameter, expected locked output", self.nodes[2].lockunspent, True, [unspent_0]) + + # Locking an already-locked output should error self.nodes[2].lockunspent(False, [unspent_0]) assert_raises_rpc_error(-8, "Invalid parameter, output already locked", self.nodes[2].lockunspent, False, [unspent_0]) + + # Restarting the node should clear the lock + self.restart_node(2) + self.nodes[2].lockunspent(False, [unspent_0]) + + # Unloading and reloating the wallet should clear the lock + assert_equal(self.nodes[0].listwallets(), [self.default_wallet_name]) + self.nodes[2].unloadwallet(self.default_wallet_name) + self.nodes[2].loadwallet(self.default_wallet_name) + assert_equal(len(self.nodes[2].listlockunspent()), 0) + + # Locking non-persistently, then re-locking persistently, is allowed + self.nodes[2].lockunspent(False, [unspent_0]) + self.nodes[2].lockunspent(False, [unspent_0], True) + + # Restarting the node with the lock written to the wallet should keep the lock + self.restart_node(2) + assert_raises_rpc_error(-8, "Invalid parameter, output already locked", self.nodes[2].lockunspent, False, [unspent_0]) + + # Unloading and reloading the wallet with a persistent lock should keep the lock + self.nodes[2].unloadwallet(self.default_wallet_name) + self.nodes[2].loadwallet(self.default_wallet_name) + assert_raises_rpc_error(-8, "Invalid parameter, output already locked", self.nodes[2].lockunspent, False, [unspent_0]) + + # Locked outputs should not be used, even if they are the only available funds assert_raises_rpc_error(-6, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 200) assert_equal([unspent_0], self.nodes[2].listlockunspent()) + + # Unlocking should remove the persistent lock self.nodes[2].lockunspent(True, [unspent_0]) + self.restart_node(2) assert_equal(len(self.nodes[2].listlockunspent()), 0) + + # Reconnect node 2 after restarts + self.connect_nodes(1, 2) + self.connect_nodes(0, 2) + assert_raises_rpc_error(-8, "txid must be of length 64 (not 34, for '0000000000000000000000000000000000')", self.nodes[2].lockunspent, False, [{"txid": "0000000000000000000000000000000000", "vout": 0}]) From e2a83f9e3933dad2e5115e356f3ea26a3ca405fc Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 11 Jan 2025 16:29:22 +0000 Subject: [PATCH 02/14] merge bitcoin#23498: remove unnecessary block rehashing prior to solving --- test/functional/feature_assumevalid.py | 1 - test/functional/feature_bip68_sequence.py | 2 -- test/functional/feature_block.py | 1 - test/functional/feature_csv_activation.py | 1 - test/functional/feature_dersig.py | 4 ---- test/functional/p2p_invalid_block.py | 4 ---- test/functional/wallet_resendwallettransactions.py | 1 - 7 files changed, 14 deletions(-) diff --git a/test/functional/feature_assumevalid.py b/test/functional/feature_assumevalid.py index 644b33a03af68..2b96eac69853c 100755 --- a/test/functional/feature_assumevalid.py +++ b/test/functional/feature_assumevalid.py @@ -127,7 +127,6 @@ def run_test(self): self.block_time += 1 block102.vtx.extend([tx]) block102.hashMerkleRoot = block102.calc_merkle_root() - block102.rehash() block102.solve() self.blocks.append(block102) self.tip = block102.sha256 diff --git a/test/functional/feature_bip68_sequence.py b/test/functional/feature_bip68_sequence.py index 66587e7e14a72..9ad7e65c73493 100755 --- a/test/functional/feature_bip68_sequence.py +++ b/test/functional/feature_bip68_sequence.py @@ -332,7 +332,6 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock): # tx3 to be removed. for i in range(2): block = create_block(tmpl=tmpl, ntime=cur_time) - block.rehash() block.solve() tip = block.sha256 assert_equal(None if i == 1 else 'inconclusive', self.nodes[0].submitblock(block.serialize().hex())) @@ -389,7 +388,6 @@ def test_bip68_not_consensus(self): block = create_block(tmpl=self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)) block.vtx.extend([tx1, tx2, tx3]) block.hashMerkleRoot = block.calc_merkle_root() - block.rehash() block.solve() assert_equal(None, self.nodes[0].submitblock(block.serialize().hex())) diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index fd0cca4e56e7e..6eca88d3813cf 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -626,7 +626,6 @@ def run_test(self): b45.nBits = 0x207fffff b45.vtx.append(non_coinbase) b45.hashMerkleRoot = b45.calc_merkle_root() - b45.calc_sha256() b45.solve() self.block_heights[b45.sha256] = self.block_heights[self.tip.sha256] + 1 self.tip = b45 diff --git a/test/functional/feature_csv_activation.py b/test/functional/feature_csv_activation.py index 11ae97c9e848a..3b91b95bd5730 100755 --- a/test/functional/feature_csv_activation.py +++ b/test/functional/feature_csv_activation.py @@ -183,7 +183,6 @@ def create_test_block(self, txs): block.nVersion = 4 block.vtx.extend(txs) block.hashMerkleRoot = block.calc_merkle_root() - block.rehash() block.solve() return block diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index 4c4d959e6903b..e9e5cbce27f30 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -91,7 +91,6 @@ def run_test(self): block = create_block(int(tip, 16), create_coinbase(DERSIG_HEIGHT - 1), block_time) block.vtx.append(spendtx) block.hashMerkleRoot = block.calc_merkle_root() - block.rehash() block.solve() assert_equal(self.nodes[0].getblockcount(), DERSIG_HEIGHT - 2) @@ -106,7 +105,6 @@ def run_test(self): block_time += 1 block = create_block(tip, create_coinbase(DERSIG_HEIGHT), block_time) block.nVersion = 2 - block.rehash() block.solve() with self.nodes[0].assert_debug_log(expected_msgs=[f'{block.hash}, bad-version(0x00000002)']): @@ -128,7 +126,6 @@ def run_test(self): # Now we verify that a block with this transaction is also invalid. block.vtx.append(spendtx) block.hashMerkleRoot = block.calc_merkle_root() - block.rehash() block.solve() with self.nodes[0].assert_debug_log(expected_msgs=[f'CheckInputScripts on {block.vtx[-1].hash} failed with non-mandatory-script-verify-flag (Non-canonical DER signature)']): @@ -139,7 +136,6 @@ def run_test(self): self.log.info("Test that a block with a DERSIG-compliant transaction is accepted") block.vtx[1] = self.create_tx(self.coinbase_txids[1]) block.hashMerkleRoot = block.calc_merkle_root() - block.rehash() block.solve() self.test_dersig_info(is_active=True) # Not active as of current tip, but next block must obey rules diff --git a/test/functional/p2p_invalid_block.py b/test/functional/p2p_invalid_block.py index b99234bb39227..39b962734a7f1 100755 --- a/test/functional/p2p_invalid_block.py +++ b/test/functional/p2p_invalid_block.py @@ -71,7 +71,6 @@ def run_test(self): block2.vtx.extend([tx1, tx2]) block2.hashMerkleRoot = block2.calc_merkle_root() - block2.rehash() block2.solve() orig_hash = block2.sha256 block2_orig = copy.deepcopy(block2) @@ -91,7 +90,6 @@ def run_test(self): block2_dup.vtx[2].vin.append(block2_dup.vtx[2].vin[0]) block2_dup.vtx[2].rehash() block2_dup.hashMerkleRoot = block2_dup.calc_merkle_root() - block2_dup.rehash() block2_dup.solve() peer.send_blocks_and_test([block2_dup], node, success=False, reject_reason='bad-txns-inputs-duplicate') @@ -103,7 +101,6 @@ def run_test(self): block3.vtx[0].sha256 = None block3.vtx[0].calc_sha256() block3.hashMerkleRoot = block3.calc_merkle_root() - block3.rehash() block3.solve() peer.send_blocks_and_test([block3], node, success=False, reject_reason='bad-cb-amount') @@ -130,7 +127,6 @@ def run_test(self): tx3.rehash() block4.vtx.append(tx3) block4.hashMerkleRoot = block4.calc_merkle_root() - block4.rehash() block4.solve() self.log.info("Test inflation by duplicating input") peer.send_blocks_and_test([block4], node, success=False, reject_reason='bad-txns-inputs-duplicate') diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index df7c1ecf6ffc4..f5eae33288c04 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -50,7 +50,6 @@ def wait_p2p(): block_time = self.mocktime + 6 * 60 node.setmocktime(block_time) block = create_block(int(node.getbestblockhash(), 16), create_coinbase(node.getblockcount() + 1), block_time) - block.rehash() block.solve() node.submitblock(block.serialize().hex()) From ed65719db5eabb819329f8bb716a107ab5f6999c Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 19 Nov 2021 14:26:58 -0500 Subject: [PATCH 03/14] merge bitcoin#23557: remove Bashism --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 915dc8d49d720..94963c20cf816 100644 --- a/configure.ac +++ b/configure.ac @@ -1983,7 +1983,7 @@ if test x$bitcoin_enable_qt != xno; then echo " with qr = $use_qr" fi echo " with zmq = $use_zmq" -if test x$enable_fuzz == xno; then +if test x$enable_fuzz = xno; then echo " with test = $use_tests" else echo " with test = not building test_dash because fuzzing is enabled" From 1f5a7c1706cf576ad6824271dec13d54ffbedd22 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 14 Jan 2025 17:02:36 +0300 Subject: [PATCH 04/14] trivial: fix padding in configure print --- configure.ac | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 94963c20cf816..fb5297522b638 100644 --- a/configure.ac +++ b/configure.ac @@ -1974,11 +1974,11 @@ echo " boost process = $with_boost_process" echo " multiprocess = $build_multiprocess" echo " with libs = $build_bitcoin_libs" echo " with wallet = $enable_wallet" -echo " with gui / qt = $bitcoin_enable_qt" if test "x$enable_wallet" != "xno"; then - echo " with sqlite = $use_sqlite" - echo " with bdb = $use_bdb" + echo " with sqlite = $use_sqlite" + echo " with bdb = $use_bdb" fi +echo " with gui / qt = $bitcoin_enable_qt" if test x$bitcoin_enable_qt != xno; then echo " with qr = $use_qr" fi From 5b2852f42f4570924365fe4666848befa94ff9f7 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 3 Dec 2021 11:57:56 -0500 Subject: [PATCH 05/14] merge bitcoin#23736: call VerifyLoadedChainstate during ChainTestingSetup --- src/test/util/setup_common.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index b7606f163f3c6..531007e4360d3 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -309,6 +309,20 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(GetTime), + [](bool bls_state) { + LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls_state); + }); + assert(!maybe_verify_failure.has_value()); + m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, m_node.banman.get(), *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync, From c908a227af9754c31ee57aba2d7dfb5b0adbe9db Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 23 Dec 2021 16:26:15 -0500 Subject: [PATCH 06/14] merge bitcoin#23855: Post-"Chainstate loading sequence coalescence" fixups --- src/init.cpp | 106 ++++++++++++++++----------------- src/node/chainstate.cpp | 2 +- src/node/chainstate.h | 2 +- src/test/util/setup_common.cpp | 66 ++++++++++---------- 4 files changed, 88 insertions(+), 88 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 258be50fee660..43225e17cdebf 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1833,48 +1833,48 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) uiInterface.InitMessage(_("Loading block index…").translated); const auto load_block_index_start_time{SteadyClock::now()}; - std::optional rv; + std::optional maybe_load_error; try { - rv = LoadChainstate(fReset, - chainman, - *node.govman, - *node.mn_metaman, - *node.mn_sync, - *node.sporkman, - node.mn_activeman, - node.chain_helper, - node.cpoolman, - node.dmnman, - node.evodb, - node.mnhf_manager, - node.llmq_ctx, - Assert(node.mempool.get()), - fPruneMode, - args.GetBoolArg("-addressindex", DEFAULT_ADDRESSINDEX), - is_governance_enabled, - args.GetBoolArg("-spentindex", DEFAULT_SPENTINDEX), - args.GetBoolArg("-timestampindex", DEFAULT_TIMESTAMPINDEX), - args.GetBoolArg("-txindex", DEFAULT_TXINDEX), - chainparams.GetConsensus(), - chainparams.NetworkIDString(), - fReindexChainState, - cache_sizes.block_tree_db, - cache_sizes.coins_db, - cache_sizes.coins, - /*block_tree_db_in_memory=*/false, - /*coins_db_in_memory=*/false, - ShutdownRequested, - []() { - uiInterface.ThreadSafeMessageBox( - _("Error reading from database, shutting down."), - "", CClientUIInterface::MSG_ERROR); - }); + maybe_load_error = LoadChainstate(fReset, + chainman, + *node.govman, + *node.mn_metaman, + *node.mn_sync, + *node.sporkman, + node.mn_activeman, + node.chain_helper, + node.cpoolman, + node.dmnman, + node.evodb, + node.mnhf_manager, + node.llmq_ctx, + Assert(node.mempool.get()), + fPruneMode, + args.GetBoolArg("-addressindex", DEFAULT_ADDRESSINDEX), + is_governance_enabled, + args.GetBoolArg("-spentindex", DEFAULT_SPENTINDEX), + args.GetBoolArg("-timestampindex", DEFAULT_TIMESTAMPINDEX), + args.GetBoolArg("-txindex", DEFAULT_TXINDEX), + chainparams.GetConsensus(), + chainparams.NetworkIDString(), + fReindexChainState, + cache_sizes.block_tree_db, + cache_sizes.coins_db, + cache_sizes.coins, + /*block_tree_db_in_memory=*/false, + /*coins_db_in_memory=*/false, + /*shutdown_requested=*/ShutdownRequested, + /*coins_error_cb=*/[]() { + uiInterface.ThreadSafeMessageBox( + _("Error reading from database, shutting down."), + "", CClientUIInterface::MSG_ERROR); + }); } catch (const std::exception& e) { LogPrintf("%s\n", e.what()); - rv = ChainstateLoadingError::ERROR_GENERIC_BLOCKDB_OPEN_FAILED; + maybe_load_error = ChainstateLoadingError::ERROR_GENERIC_BLOCKDB_OPEN_FAILED; } - if (rv.has_value()) { - switch (rv.value()) { + if (maybe_load_error.has_value()) { + switch (maybe_load_error.value()) { case ChainstateLoadingError::ERROR_LOADING_BLOCK_DB: strLoadError = _("Error loading block database"); break; @@ -1930,7 +1930,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) LogPrintf("%s: timestamp index %s\n", __func__, fTimestampIndex ? "enabled" : "disabled"); LogPrintf("%s: spent index %s\n", __func__, fSpentIndex ? "enabled" : "disabled"); - std::optional rv2; + std::optional maybe_verify_error; try { uiInterface.InitMessage(_("Verifying blocks…").translated); auto check_blocks = args.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS); @@ -1938,23 +1938,23 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) LogPrintf("Prune: pruned datadir may not have more than %d blocks; only checking available blocks\n", MIN_BLOCKS_TO_KEEP); } - rv2 = VerifyLoadedChainstate(chainman, - *Assert(node.evodb.get()), - fReset, - fReindexChainState, - chainparams.GetConsensus(), - check_blocks, - args.GetArg("-checklevel", DEFAULT_CHECKLEVEL), - static_cast(GetTime), - [](bool bls_state) { - LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls_state); - }); + maybe_verify_error = VerifyLoadedChainstate(chainman, + *Assert(node.evodb.get()), + fReset, + fReindexChainState, + chainparams.GetConsensus(), + check_blocks, + args.GetArg("-checklevel", DEFAULT_CHECKLEVEL), + /*get_unix_time_seconds=*/static_cast(GetTime), + [](bool bls_state) { + LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls_state); + }); } catch (const std::exception& e) { LogPrintf("%s\n", e.what()); - rv2 = ChainstateLoadVerifyError::ERROR_GENERIC_FAILURE; + maybe_verify_error = ChainstateLoadVerifyError::ERROR_GENERIC_FAILURE; } - if (rv2.has_value()) { - switch (rv2.value()) { + if (maybe_verify_error.has_value()) { + switch (maybe_verify_error.value()) { case ChainstateLoadVerifyError::ERROR_BLOCK_FROM_FUTURE: strLoadError = _("The block database contains a block which appears to be from the future. " "This may be due to your computer's date and time being set incorrectly. " diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index d379a0ee6531d..f781d3d1ef305 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -275,7 +275,7 @@ std::optional VerifyLoadedChainstate(ChainstateManage for (CChainState* chainstate : chainman.GetAll()) { if (!is_coinsview_empty(chainstate)) { const CBlockIndex* tip = chainstate->m_chain.Tip(); - if (tip && tip->nTime > get_unix_time_seconds() + 2 * 60 * 60) { + if (tip && tip->nTime > get_unix_time_seconds() + MAX_FUTURE_BLOCK_TIME) { return ChainstateLoadVerifyError::ERROR_BLOCK_FROM_FUTURE; } const bool v19active{DeploymentActiveAfter(tip, consensus_params, Consensus::DEPLOYMENT_V19)}; diff --git a/src/node/chainstate.h b/src/node/chainstate.h index c7b399177e412..b228d0089f153 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -71,7 +71,7 @@ enum class ChainstateLoadingError { * this sequence, when we explicitly checked shutdown_requested() at * arbitrary points, one of those calls returned true". Therefore, a * return value other than SHUTDOWN_PROBED does not guarantee that - * shutdown_requested() hasn't been called indirectly. + * shutdown hasn't been called indirectly. * - else * - Success! */ diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 531007e4360d3..7587672624567 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -279,37 +279,37 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(GetTime), + /*get_unix_time_seconds=*/static_cast(GetTime), [](bool bls_state) { LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls_state); }); - assert(!maybe_verify_failure.has_value()); + assert(!maybe_verify_error.has_value()); m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, m_node.banman.get(), From 5069a0b2396c67f69f5af06f3880a4e52dfc39bf Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 11 Jan 2025 19:48:11 +0000 Subject: [PATCH 07/14] merge bitcoin#21464: Mempool Update Cut-Through Optimization --- src/txmempool.cpp | 32 ++++++---- src/txmempool.h | 68 +++++++++++++++------- src/validation.cpp | 4 +- test/functional/mempool_updatefromblock.py | 2 +- 4 files changed, 73 insertions(+), 33 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index d4adf717ce344..9eb047d868b0b 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -124,10 +124,9 @@ size_t CTxMemPoolEntry::GetTxSize() const return GetVirtualTransactionSize(nTxSize, sigOpCount); } -// Update the given tx for any in-mempool descendants. -// Assumes that CTxMemPool::m_children is correct for the given tx and all -// descendants. -void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendants, const std::set &setExclude) +void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants, + const std::set& setExclude, std::set& descendants_to_remove, + uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) { CTxMemPoolEntry::Children stageEntries, descendants; stageEntries = updateIt->GetMemPoolChildrenConst(); @@ -164,17 +163,18 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan cachedDescendants[updateIt].insert(mapTx.iterator_to(descendant)); // Update ancestor state for each descendant mapTx.modify(mapTx.iterator_to(descendant), update_ancestor_state(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCount())); + // Don't directly remove the transaction here -- doing so would + // invalidate iterators in cachedDescendants. Mark it for removal + // by inserting into descendants_to_remove. + if (descendant.GetCountWithAncestors() > ancestor_count_limit || descendant.GetSizeWithAncestors() > ancestor_size_limit) { + descendants_to_remove.insert(descendant.GetTx().GetHash()); + } } } mapTx.modify(updateIt, update_descendant_state(modifySize, modifyFee, modifyCount)); } -// vHashesToUpdate is the set of transaction hashes from a disconnected block -// which has been re-added to the mempool. -// for each entry, look for descendants that are outside vHashesToUpdate, and -// add fee/size information for such descendants to the parent. -// for each such descendant, also update the ancestor state to include the parent. -void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashesToUpdate) +void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashesToUpdate, uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) { AssertLockHeld(cs); // For each entry in vHashesToUpdate, store the set of in-mempool, but not @@ -186,6 +186,8 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes // accounted for in the state of their ancestors) std::set setAlreadyIncluded(vHashesToUpdate.begin(), vHashesToUpdate.end()); + std::set descendants_to_remove; + // Iterate in reverse, so that whenever we are looking at a transaction // we are sure that all in-mempool descendants have already been processed. // This maximizes the benefit of the descendant cache and guarantees that @@ -215,7 +217,15 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes } } } // release epoch guard for UpdateForDescendants - UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded); + UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded, descendants_to_remove, ancestor_size_limit, ancestor_count_limit); + } + + for (const auto& txid : descendants_to_remove) { + // This txid may have been removed already in a prior call to removeRecursive. + // Therefore we ensure it is not yet removed already. + if (const std::optional txiter = GetIter(txid)) { + removeRecursive((*txiter)->GetTx(), MemPoolRemovalReason::SIZELIMIT); + } } } diff --git a/src/txmempool.h b/src/txmempool.h index 80e12f4e7df58..f1f93c8a7d5a8 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -680,16 +680,25 @@ class CTxMemPool */ void RemoveStaged(setEntries& stage, bool updateDescendants, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); - /** When adding transactions from a disconnected block back to the mempool, - * new mempool entries may have children in the mempool (which is generally - * not the case when otherwise adding transactions). - * UpdateTransactionsFromBlock() will find child transactions and update the - * descendant state for each transaction in vHashesToUpdate (excluding any - * child transactions present in vHashesToUpdate, which are already accounted - * for). Note: vHashesToUpdate should be the set of transactions from the - * disconnected block that have been accepted back into the mempool. + /** UpdateTransactionsFromBlock is called when adding transactions from a + * disconnected block back to the mempool, new mempool entries may have + * children in the mempool (which is generally not the case when otherwise + * adding transactions). + * @post updated descendant state for descendants of each transaction in + * vHashesToUpdate (excluding any child transactions present in + * vHashesToUpdate, which are already accounted for). Updated state + * includes add fee/size information for such descendants to the + * parent and updated ancestor state to include the parent. + * + * @param[in] vHashesToUpdate The set of txids from the + * disconnected block that have been accepted back into the mempool. + * @param[in] ancestor_size_limit The maximum allowed size in virtual + * bytes of an entry and its ancestors + * @param[in] ancestor_count_limit The maximum allowed number of + * transactions including the entry and its ancestors. */ - void UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); + void UpdateTransactionsFromBlock(const std::vector& vHashesToUpdate, + uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); /** Try to calculate all in-mempool ancestors of entry. * (these are all calculated including the tx itself) @@ -836,19 +845,38 @@ class CTxMemPool /** UpdateForDescendants is used by UpdateTransactionsFromBlock to update * the descendants for a single transaction that has been added to the * mempool but may have child transactions in the mempool, eg during a - * chain reorg. setExclude is the set of descendant transactions in the - * mempool that must not be accounted for (because any descendants in - * setExclude were added to the mempool after the transaction being - * updated and hence their state is already reflected in the parent - * state). + * chain reorg. + * + * @pre CTxMemPool::m_children is correct for the given tx and all + * descendants. + * @pre cachedDescendants is an accurate cache where each entry has all + * descendants of the corresponding key, including those that should + * be removed for violation of ancestor limits. + * @post if updateIt has any non-excluded descendants, cachedDescendants has + * a new cache line for updateIt. + * @post descendants_to_remove has a new entry for any descendant which exceeded + * ancestor limits relative to updateIt. * - * cachedDescendants will be updated with the descendants of the transaction - * being updated, so that future invocations don't need to walk the - * same transaction again, if encountered in another transaction chain. + * @param[in] updateIt the entry to update for its descendants + * @param[in,out] cachedDescendants a cache where each line corresponds to all + * descendants. It will be updated with the descendants of the transaction + * being updated, so that future invocations don't need to walk the same + * transaction again, if encountered in another transaction chain. + * @param[in] setExclude the set of descendant transactions in the mempool + * that must not be accounted for (because any descendants in setExclude + * were added to the mempool after the transaction being updated and hence + * their state is already reflected in the parent state). + * @param[out] descendants_to_remove Populated with the txids of entries that + * exceed ancestor limits. It's the responsibility of the caller to + * removeRecursive them. + * @param[in] ancestor_size_limit the max number of ancestral bytes allowed + * for any descendant + * @param[in] ancestor_count_limit the max number of ancestor transactions + * allowed for any descendant */ - void UpdateForDescendants(txiter updateIt, - cacheMap &cachedDescendants, - const std::set &setExclude) EXCLUSIVE_LOCKS_REQUIRED(cs); + void UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants, + const std::set& setExclude, std::set& descendants_to_remove, + uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) EXCLUSIVE_LOCKS_REQUIRED(cs); /** Update ancestors of hash to add/remove it as a descendant transaction. */ void UpdateAncestorsOf(bool add, txiter hash, setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs); /** Set ancestor state for an entry */ diff --git a/src/validation.cpp b/src/validation.cpp index c2f9f95447521..447a158bc2a29 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -357,7 +357,9 @@ void CChainState::MaybeUpdateMempoolForReorg( // previously-confirmed transactions back to the mempool. // UpdateTransactionsFromBlock finds descendants of any transactions in // the disconnectpool that were added back and cleans up the mempool state. - m_mempool->UpdateTransactionsFromBlock(vHashUpdate); + const uint64_t ancestor_count_limit = gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); + const uint64_t ancestor_size_limit = gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000; + m_mempool->UpdateTransactionsFromBlock(vHashUpdate, ancestor_size_limit, ancestor_count_limit); // Predicate to use for filtering transactions in removeForReorg. // Checks whether the transaction is still final and, if it spends a coinbase output, mature. diff --git a/test/functional/mempool_updatefromblock.py b/test/functional/mempool_updatefromblock.py index 22f136d1a5c33..f2e5aa1a8985a 100755 --- a/test/functional/mempool_updatefromblock.py +++ b/test/functional/mempool_updatefromblock.py @@ -17,7 +17,7 @@ class MempoolUpdateFromBlockTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 - self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000']] + self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000', '-limitancestorcount=100']] def skip_test_if_missing_module(self): self.skip_if_no_wallet() From 5da523c030ebe9ccbf5d731a449d2b8c302f36ed Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 21 Feb 2022 10:41:08 +0100 Subject: [PATCH 08/14] merge bitcoin#24403: Avoid implicit-integer-sign-change in VerifyLoadedChainstate --- src/node/chainstate.cpp | 4 ++-- src/node/chainstate.h | 4 ++-- src/validation.cpp | 26 ++++++++++++++++---------- test/functional/rpc_blockchain.py | 10 +++++++++- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index f781d3d1ef305..dde3c8b021882 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -261,8 +261,8 @@ std::optional VerifyLoadedChainstate(ChainstateManage bool fReset, bool fReindexChainState, const Consensus::Params& consensus_params, - unsigned int check_blocks, - unsigned int check_level, + int check_blocks, + int check_level, std::function get_unix_time_seconds, std::function notify_bls_state) { diff --git a/src/node/chainstate.h b/src/node/chainstate.h index b228d0089f153..8b0c738a85baa 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -143,8 +143,8 @@ std::optional VerifyLoadedChainstate(ChainstateManage bool fReset, bool fReindexChainState, const Consensus::Params& consensus_params, - unsigned int check_blocks, - unsigned int check_level, + int check_blocks, + int check_level, std::function get_unix_time_seconds, std::function notify_bls_state = nullptr); diff --git a/src/validation.cpp b/src/validation.cpp index 447a158bc2a29..8f5bff7708b9e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4434,15 +4434,17 @@ bool CVerifyDB::VerifyDB( { AssertLockHeld(cs_main); - if (chainstate.m_chain.Tip() == nullptr || chainstate.m_chain.Tip()->pprev == nullptr) + if (chainstate.m_chain.Tip() == nullptr || chainstate.m_chain.Tip()->pprev == nullptr) { return true; + } // begin tx and let it rollback auto dbTx = evoDb.BeginTransaction(); // Verify blocks in the best chain - if (nCheckDepth <= 0 || nCheckDepth > chainstate.m_chain.Height()) + if (nCheckDepth <= 0 || nCheckDepth > chainstate.m_chain.Height()) { nCheckDepth = chainstate.m_chain.Height(); + } nCheckLevel = std::max(0, std::min(4, nCheckLevel)); LogPrintf("Verifying last %i blocks at level %i\n", nCheckDepth, nCheckLevel); CCoinsViewCache coins(&coinsview); @@ -4457,14 +4459,15 @@ bool CVerifyDB::VerifyDB( for (pindex = chainstate.m_chain.Tip(); pindex && pindex->pprev; pindex = pindex->pprev) { const int percentageDone = std::max(1, std::min(99, (int)(((double)(chainstate.m_chain.Height() - pindex->nHeight)) / (double)nCheckDepth * (nCheckLevel >= 4 ? 50 : 100)))); - if (reportDone < percentageDone/10) { + if (reportDone < percentageDone / 10) { // report every 10% step LogPrintf("[%d%%]...", percentageDone); /* Continued */ - reportDone = percentageDone/10; + reportDone = percentageDone / 10; } uiInterface.ShowProgress(_("Verifying blocks…").translated, percentageDone, false); - if (pindex->nHeight <= chainstate.m_chain.Height()-nCheckDepth) + if (pindex->nHeight <= chainstate.m_chain.Height() - nCheckDepth) { break; + } if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) { // If pruning or running under an assumeutxo snapshot, only go // back as far as we have data. @@ -4473,12 +4476,14 @@ bool CVerifyDB::VerifyDB( } CBlock block; // check level 0: read from disk - if (!ReadBlockFromDisk(block, pindex, consensus_params)) + if (!ReadBlockFromDisk(block, pindex, consensus_params)) { return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); + } // check level 1: verify block validity - if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) + if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) { return error("%s: *** found bad block at %d, hash=%s (%s)\n", __func__, pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + } // check level 2: verify undo validity if (nCheckLevel >= 2 && pindex) { CBlockUndo undo; @@ -4506,8 +4511,9 @@ bool CVerifyDB::VerifyDB( } if (ShutdownRequested()) return true; } - if (pindexFailure) + if (pindexFailure) { return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions); + } // store block count as we move pindex at check level >= 4 int block_count = chainstate.m_chain.Height() - pindex->nHeight; @@ -4516,10 +4522,10 @@ bool CVerifyDB::VerifyDB( if (nCheckLevel >= 4) { while (pindex != chainstate.m_chain.Tip()) { const int percentageDone = std::max(1, std::min(99, 100 - (int)(((double)(chainstate.m_chain.Height() - pindex->nHeight)) / (double)nCheckDepth * 50))); - if (reportDone < percentageDone/10) { + if (reportDone < percentageDone / 10) { // report every 10% step LogPrintf("[%d%%]...", percentageDone); /* Continued */ - reportDone = percentageDone/10; + reportDone = percentageDone / 10; } uiInterface.ShowProgress(_("Verifying blocks…").translated, percentageDone, false); pindex = chainstate.m_chain.Next(pindex); diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 013a16cf385f7..3ef3017cf2eae 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -71,7 +71,15 @@ def set_test_params(self): def run_test(self): self.mine_chain() self._test_max_future_block_time() - self.restart_node(0, extra_args=['-stopatheight=207', '-prune=1', '-txindex=0']) # Set extra args with pruning after rescan is complete + self.restart_node( + 0, + extra_args=[ + "-stopatheight=207", + "-checkblocks=-1", # Check all blocks + "-prune=1", # Set pruning after rescan is complete + '-txindex=0', + ], + ) # Actual tests self._test_getblockchaininfo() From f53466d0eaecfc52cc80e2032cde051d493c3b71 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 7 Feb 2022 10:18:00 +0000 Subject: [PATCH 09/14] merge bitcoin#24968: Move TxOrphange tests to orphange_tests.cpp --- src/Makefile.test.include | 1 + src/test/denialofservice_tests.cpp | 118 ------------------------- src/test/orphanage_tests.cpp | 137 +++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 118 deletions(-) create mode 100644 src/test/orphanage_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index c859010acf23f..00e0429688fd7 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -139,6 +139,7 @@ BITCOIN_TESTS =\ test/net_peer_eviction_tests.cpp \ test/net_tests.cpp \ test/netbase_tests.cpp \ + test/orphanage_tests.cpp \ test/pmt_tests.cpp \ test/policyestimator_tests.cpp \ test/pool_tests.cpp \ diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 3d3495328091f..2ecdb8d12b6b9 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -15,7 +15,6 @@ #include #include #include -#include #include #include #include @@ -456,121 +455,4 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) peerLogic->FinalizeNode(dummyNode); } -class TxOrphanageTest : public TxOrphanage -{ -public: - inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) - { - return m_orphans.size(); - } - - CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) - { - std::map::iterator it; - it = m_orphans.lower_bound(InsecureRand256()); - if (it == m_orphans.end()) - it = m_orphans.begin(); - return it->second.tx; - } -}; - -static void MakeNewKeyWithFastRandomContext(CKey& key) -{ - std::vector keydata; - keydata = g_insecure_rand_ctx.randbytes(32); - key.Set(keydata.data(), keydata.data() + keydata.size(), /*fCompressedIn*/ true); - assert(key.IsValid()); -} - -BOOST_AUTO_TEST_CASE(DoS_mapOrphans) -{ - // This test had non-deterministic coverage due to - // randomly selected seeds. - // This seed is chosen so that all branches of the function - // ecdsa_signature_parse_der_lax are executed during this test. - // Specifically branches that run only when an ECDSA - // signature's R and S values have leading zeros. - g_insecure_rand_ctx = FastRandomContext{uint256{33}}; - - TxOrphanageTest orphanage; - CKey key; - MakeNewKeyWithFastRandomContext(key); - FillableSigningProvider keystore; - BOOST_CHECK(keystore.AddKey(key)); - - LOCK(g_cs_orphans); - - // 50 orphan transactions: - for (int i = 0; i < 50; i++) - { - CMutableTransaction tx; - tx.vin.resize(1); - tx.vin[0].prevout.n = 0; - tx.vin[0].prevout.hash = InsecureRand256(); - tx.vin[0].scriptSig << OP_1; - tx.vout.resize(1); - tx.vout[0].nValue = 1*CENT; - tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); - - orphanage.AddTx(MakeTransactionRef(tx), i); - } - - // ... and 50 that depend on other orphans: - for (int i = 0; i < 50; i++) - { - CTransactionRef txPrev = orphanage.RandomOrphan(); - - CMutableTransaction tx; - tx.vin.resize(1); - tx.vin[0].prevout.n = 0; - tx.vin[0].prevout.hash = txPrev->GetHash(); - tx.vout.resize(1); - tx.vout[0].nValue = 1*CENT; - tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); - BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL)); - - orphanage.AddTx(MakeTransactionRef(tx), i); - } - - // This really-big orphan should be ignored: - for (int i = 0; i < 10; i++) - { - CTransactionRef txPrev = orphanage.RandomOrphan(); - - CMutableTransaction tx; - tx.vout.resize(1); - tx.vout[0].nValue = 1*CENT; - tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); - tx.vin.resize(2777); - for (unsigned int j = 0; j < tx.vin.size(); j++) - { - tx.vin[j].prevout.n = j; - tx.vin[j].prevout.hash = txPrev->GetHash(); - } - BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL)); - // Re-use same signature for other inputs - // (they don't have to be valid for this test) - for (unsigned int j = 1; j < tx.vin.size(); j++) - tx.vin[j].scriptSig = tx.vin[0].scriptSig; - - BOOST_CHECK(!orphanage.AddTx(MakeTransactionRef(tx), i)); - } - - // Test EraseOrphansFor: - for (NodeId i = 0; i < 3; i++) - { - size_t sizeBefore = orphanage.CountOrphans(); - orphanage.EraseForPeer(i); - BOOST_CHECK(orphanage.CountOrphans() < sizeBefore); - } - - // Test LimitOrphanTxSize() function: - orphanage.LimitOrphans(40); - BOOST_CHECK(orphanage.CountOrphans() <= 40); - orphanage.LimitOrphans(10); - BOOST_CHECK(orphanage.CountOrphans() <= 10); - orphanage.LimitOrphans(0); - BOOST_CHECK(orphanage.CountOrphans() == 0); -} - BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp new file mode 100644 index 0000000000000..842daa8bd4d4b --- /dev/null +++ b/src/test/orphanage_tests.cpp @@ -0,0 +1,137 @@ +// Copyright (c) 2011-2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include