From b81f37031c8f2ccad9346f1b65ee0f8083c44796 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 5 Oct 2023 11:20:04 +1000 Subject: [PATCH 0001/2363] p2p: Increase tx relay rate In the presence of smaller transactions on the network, blocks can sustain a higher relay rate than 7tx/second. In this event, the per-peer inventory queues can grow too large. This commit bumps the rate up to 14 tx/s (for inbound peers), increasing the safety margin by a factor of 2. Outbound peers continue to receive relayed transactions at 2.5x the rate of inbound peers, for a rate of 35tx/second. Co-Authored-By: Suhas Daftuar --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e503a6838276..f71378295b35 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -136,7 +136,7 @@ static constexpr auto INBOUND_INVENTORY_BROADCAST_INTERVAL{5s}; static constexpr auto OUTBOUND_INVENTORY_BROADCAST_INTERVAL{2s}; /** Maximum rate of inventory items to send per second. * Limits the impact of low-fee transaction floods. */ -static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND = 7; +static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND{14}; /** Target number of tx inventory items to send per transmission. */ static constexpr unsigned int INVENTORY_BROADCAST_TARGET = INVENTORY_BROADCAST_PER_SECOND * count_seconds(INBOUND_INVENTORY_BROADCAST_INTERVAL); /** Maximum number of inventory items to send per transmission. */ From 0dc337f73d013e342b880746292f1c3247b287cf Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Tue, 9 Apr 2024 15:53:38 -0300 Subject: [PATCH 0002/2363] gui: Fix TransactionsView on setCurrentWallet Making sure that if the privacy mode is activaded during the wallet selection, the transaction view is not shown. --- src/qt/bitcoingui.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 6d66c7473bd0..630450011ab8 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -756,9 +756,7 @@ void BitcoinGUI::addWallet(WalletModel* walletModel) connect(wallet_view, &WalletView::encryptionStatusChanged, this, &BitcoinGUI::updateWalletStatus); connect(wallet_view, &WalletView::incomingTransaction, this, &BitcoinGUI::incomingTransaction); connect(this, &BitcoinGUI::setPrivacy, wallet_view, &WalletView::setPrivacy); - const bool privacy = isPrivacyModeActivated(); - wallet_view->setPrivacy(privacy); - enableHistoryAction(privacy); + wallet_view->setPrivacy(isPrivacyModeActivated()); const QString display_name = walletModel->getDisplayName(); m_wallet_selector->addItem(display_name, QVariant::fromValue(walletModel)); } @@ -817,7 +815,7 @@ void BitcoinGUI::setWalletActionsEnabled(bool enabled) overviewAction->setEnabled(enabled); sendCoinsAction->setEnabled(enabled); receiveCoinsAction->setEnabled(enabled); - historyAction->setEnabled(enabled); + historyAction->setEnabled(enabled && !isPrivacyModeActivated()); encryptWalletAction->setEnabled(enabled); backupWalletAction->setEnabled(enabled); changePassphraseAction->setEnabled(enabled); From 975783cb79e929260873c1055d4b415cd33bb6b9 Mon Sep 17 00:00:00 2001 From: pythcoiner Date: Fri, 24 Jan 2025 07:31:09 +0100 Subject: [PATCH 0003/2363] descriptor: account for all StringType in MiniscriptDescriptor::ToStringHelper() --- src/script/descriptor.cpp | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 2e1a30744ec1..99aba6c46418 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -1286,20 +1286,33 @@ class StringMaker { const SigningProvider* m_arg; //! Keys contained in the Miniscript (a reference to DescriptorImpl::m_pubkey_args). const std::vector>& m_pubkeys; - //! Whether to serialize keys as private or public. - bool m_private; + //! StringType to serialize keys + const DescriptorImpl::StringType m_type; + const DescriptorCache* m_cache; public: - StringMaker(const SigningProvider* arg LIFETIMEBOUND, const std::vector>& pubkeys LIFETIMEBOUND, bool priv) - : m_arg(arg), m_pubkeys(pubkeys), m_private(priv) {} + StringMaker(const SigningProvider* arg LIFETIMEBOUND, + const std::vector>& pubkeys LIFETIMEBOUND, + DescriptorImpl::StringType type, + const DescriptorCache* cache LIFETIMEBOUND) + : m_arg(arg), m_pubkeys(pubkeys), m_type(type), m_cache(cache) {} std::optional ToString(uint32_t key) const { std::string ret; - if (m_private) { - if (!m_pubkeys[key]->ToPrivateString(*m_arg, ret)) return {}; - } else { + switch (m_type) { + case DescriptorImpl::StringType::PUBLIC: ret = m_pubkeys[key]->ToString(); + break; + case DescriptorImpl::StringType::PRIVATE: + if (!m_pubkeys[key]->ToPrivateString(*m_arg, ret)) return {}; + break; + case DescriptorImpl::StringType::NORMALIZED: + if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {}; + break; + case DescriptorImpl::StringType::COMPAT: + ret = m_pubkeys[key]->ToString(PubkeyProvider::StringType::COMPAT); + break; } return ret; } @@ -1332,7 +1345,7 @@ class MiniscriptDescriptor final : public DescriptorImpl bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type, const DescriptorCache* cache = nullptr) const override { - if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type == StringType::PRIVATE))) { + if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache))) { out = *res; return true; } From 28a4fcb03c0fb1cd5112eca1eb36dcb13e0b4ff2 Mon Sep 17 00:00:00 2001 From: pythcoiner Date: Fri, 24 Jan 2025 17:27:06 +0100 Subject: [PATCH 0004/2363] test: check listdescriptors do not return a mix of hardened derivation marker --- test/functional/wallet_listdescriptors.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/functional/wallet_listdescriptors.py b/test/functional/wallet_listdescriptors.py index c9d6c1f190df..1b9d59a959cf 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -134,6 +134,26 @@ def run_test(self): } assert_equal(expected, wallet.listdescriptors()) + self.log.info('Test taproot descriptor do not have mixed hardened derivation marker') + node.createwallet(wallet_name='w5', descriptors=True, disable_private_keys=True) + wallet = node.get_wallet_rpc('w5') + wallet.importdescriptors([{ + 'desc': "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([c658b283/48'/1'/0'/2']tpubDFL5wzgPBYK5pZ2Kh1T8qrxnp43kjE5CXfguZHHBrZSWpkfASy5rVfj7prh11XdqkC1P3kRwUPBeX7AHN8XBNx8UwiprnFnEm5jyswiRD4p/0/*),older(65535)))#xl20m6md", + 'timestamp': TIME_GENESIS_BLOCK, + }]) + expected = { + 'wallet_name': 'w5', + 'descriptors': [ + {'active': False, + 'desc': 'tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([c658b283/48h/1h/0h/2h]tpubDFL5wzgPBYK5pZ2Kh1T8qrxnp43kjE5CXfguZHHBrZSWpkfASy5rVfj7prh11XdqkC1P3kRwUPBeX7AHN8XBNx8UwiprnFnEm5jyswiRD4p/0/*),older(65535)))#m4uznndk', + 'timestamp': TIME_GENESIS_BLOCK, + 'range': [0, 0], + 'next': 0, + 'next_index': 0}, + ] + } + assert_equal(expected, wallet.listdescriptors()) + if __name__ == '__main__': ListDescriptorsTest(__file__).main() From a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43 Mon Sep 17 00:00:00 2001 From: Calin Culianu Date: Wed, 26 Mar 2025 08:12:39 -0500 Subject: [PATCH 0005/2363] Fix 11-year-old mis-categorized error code in OP_IF evaluation This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708. It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here. This commit fixes the situation. Also in this commit: - Fix script_tests to adjust to the corrected error message - Fix p2p_invalid_tx functional test to produce the desired error message --- src/script/interpreter.cpp | 2 +- src/test/data/script_tests.json | 24 ++++++++++++------------ test/functional/data/invalid_txs.py | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 61ea7f4503c2..ec34de5e7602 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -607,7 +607,7 @@ bool EvalScript(std::vector >& stack, const CScript& if (fExec) { if (stack.size() < 1) - return set_error(serror, SCRIPT_ERR_UNBALANCED_CONDITIONAL); + return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION); valtype& vch = stacktop(-1); // Tapscript requires minimal IF/NOTIF inputs as a consensus rule. if (sigversion == SigVersion::TAPSCRIPT) { diff --git a/src/test/data/script_tests.json b/src/test/data/script_tests.json index ad05240369b3..83f20ff44ee0 100644 --- a/src/test/data/script_tests.json +++ b/src/test/data/script_tests.json @@ -1027,8 +1027,8 @@ ["1", "IF 1", "P2SH,STRICTENC", "UNBALANCED_CONDITIONAL", "IF without ENDIF"], ["1 IF 1", "ENDIF", "P2SH,STRICTENC", "UNBALANCED_CONDITIONAL", "IFs don't carry over"], -["NOP", "IF 1 ENDIF", "P2SH,STRICTENC", "UNBALANCED_CONDITIONAL", "The following tests check the if(stack.size() < N) tests in each opcode"], -["NOP", "NOTIF 1 ENDIF", "P2SH,STRICTENC", "UNBALANCED_CONDITIONAL", "They are here to catch copy-and-paste errors"], +["NOP", "IF 1 ENDIF", "P2SH,STRICTENC", "INVALID_STACK_OPERATION", "The following tests check the if(stack.size() < N) tests in each opcode"], +["NOP", "NOTIF 1 ENDIF", "P2SH,STRICTENC", "INVALID_STACK_OPERATION", "They are here to catch copy-and-paste errors"], ["NOP", "VERIFY 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION", "Most of them are duplicated elsewhere,"], ["NOP", "TOALTSTACK 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION", "but, hey, more is always better, right?"], @@ -2546,14 +2546,14 @@ ["0x02 0x0100 0x03 0x635168", "HASH160 0x14 0xe7309652a8e3f600f06f5d8d52d6df03d2176cc3 EQUAL", "P2SH,WITNESS,MINIMALIF", "OK"], ["0 0x03 0x635168", "HASH160 0x14 0xe7309652a8e3f600f06f5d8d52d6df03d2176cc3 EQUAL", "P2SH,WITNESS,MINIMALIF", "EVAL_FALSE"], ["0x01 0x00 0x03 0x635168", "HASH160 0x14 0xe7309652a8e3f600f06f5d8d52d6df03d2176cc3 EQUAL", "P2SH,WITNESS,MINIMALIF", "EVAL_FALSE"], -["0x03 0x635168", "HASH160 0x14 0xe7309652a8e3f600f06f5d8d52d6df03d2176cc3 EQUAL", "P2SH,WITNESS,MINIMALIF", "UNBALANCED_CONDITIONAL"], +["0x03 0x635168", "HASH160 0x14 0xe7309652a8e3f600f06f5d8d52d6df03d2176cc3 EQUAL", "P2SH,WITNESS,MINIMALIF", "INVALID_STACK_OPERATION"], ["Normal P2SH NOTIF 1 ENDIF"], ["1 0x03 0x645168", "HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL", "P2SH,WITNESS,MINIMALIF", "EVAL_FALSE"], ["2 0x03 0x645168", "HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL", "P2SH,WITNESS,MINIMALIF", "EVAL_FALSE"], ["0x02 0x0100 0x03 0x645168", "HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL", "P2SH,WITNESS,MINIMALIF", "EVAL_FALSE"], ["0 0x03 0x645168", "HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL", "P2SH,WITNESS,MINIMALIF", "OK"], ["0x01 0x00 0x03 0x645168", "HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL", "P2SH,WITNESS,MINIMALIF", "OK"], -["0x03 0x645168", "HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL", "P2SH,WITNESS,MINIMALIF", "UNBALANCED_CONDITIONAL"], +["0x03 0x645168", "HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL", "P2SH,WITNESS,MINIMALIF", "INVALID_STACK_OPERATION"], ["P2WSH IF 1 ENDIF"], [["01", "635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS", "OK"], [["02", "635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS", "OK"], @@ -2565,8 +2565,8 @@ [["0100", "635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], [["", "635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS,MINIMALIF", "CLEANSTACK"], [["00", "635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], -[["635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS", "UNBALANCED_CONDITIONAL"], -[["635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS,MINIMALIF", "UNBALANCED_CONDITIONAL"], +[["635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS", "INVALID_STACK_OPERATION"], +[["635168", 0.00000001], "", "0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "P2SH,WITNESS,MINIMALIF", "INVALID_STACK_OPERATION"], ["P2WSH NOTIF 1 ENDIF"], [["01", "645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS", "CLEANSTACK"], [["02", "645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS", "CLEANSTACK"], @@ -2578,8 +2578,8 @@ [["0100", "645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], [["", "645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS,MINIMALIF", "OK"], [["00", "645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], -[["645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS", "UNBALANCED_CONDITIONAL"], -[["645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS,MINIMALIF", "UNBALANCED_CONDITIONAL"], +[["645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS", "INVALID_STACK_OPERATION"], +[["645168", 0.00000001], "", "0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "P2SH,WITNESS,MINIMALIF", "INVALID_STACK_OPERATION"], @@ -2594,8 +2594,8 @@ [["0100", "635168", 0.00000001], "0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], [["", "635168", 0.00000001], "0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL", "P2SH,WITNESS,MINIMALIF", "CLEANSTACK"], [["00", "635168", 0.00000001], "0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], -[["635168", 0.00000001], "0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL", "P2SH,WITNESS", "UNBALANCED_CONDITIONAL"], -[["635168", 0.00000001], "0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL", "P2SH,WITNESS,MINIMALIF", "UNBALANCED_CONDITIONAL"], +[["635168", 0.00000001], "0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL", "P2SH,WITNESS", "INVALID_STACK_OPERATION"], +[["635168", 0.00000001], "0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d", "HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL", "P2SH,WITNESS,MINIMALIF", "INVALID_STACK_OPERATION"], ["P2SH-P2WSH NOTIF 1 ENDIF"], [["01", "645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS", "CLEANSTACK"], [["02", "645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS", "CLEANSTACK"], @@ -2607,8 +2607,8 @@ [["0100", "645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], [["", "645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS,MINIMALIF", "OK"], [["00", "645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS,MINIMALIF", "MINIMALIF"], -[["645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS", "UNBALANCED_CONDITIONAL"], -[["645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS,MINIMALIF", "UNBALANCED_CONDITIONAL"], +[["645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS", "INVALID_STACK_OPERATION"], +[["645168", 0.00000001], "0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8", "HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL", "P2SH,WITNESS,MINIMALIF", "INVALID_STACK_OPERATION"], ["NULLFAIL should cover all signatures and signatures only"], ["0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG", "OK", "BIP66 and NULLFAIL-compliant"], diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index d2d7202d8601..f0f2e61a592e 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -229,7 +229,7 @@ class InvalidOPIFConstruction(BadTxTemplate): def get_tx(self): return create_tx_with_script( - self.spend_tx, 0, script_sig=b'\x64' * 35, + self.spend_tx, 0, script_sig=b'\x68' * 35, amount=(self.spend_avail // 2)) From fe71a4b139f3a142468c2e931775813bc8f9d2ad Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 14 Apr 2025 16:21:40 +0100 Subject: [PATCH 0006/2363] depends: Avoid `warning: "_FORTIFY_SOURCE" redefined` for `libevent` On Alpine Linux 3.12.3, compiling the `libevent` package produces multiple warnings: ``` $ gmake -C depends -j $(nproc) libevent : warning: "_FORTIFY_SOURCE" redefined : note: this is the location of the previous definition ``` --- depends/packages/libevent.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depends/packages/libevent.mk b/depends/packages/libevent.mk index dadd3b1b75da..1575c6768f8f 100644 --- a/depends/packages/libevent.mk +++ b/depends/packages/libevent.mk @@ -15,7 +15,7 @@ define $(package)_set_vars $(package)_config_opts+=-DEVENT__DISABLE_SAMPLES=ON -DEVENT__DISABLE_REGRESS=ON $(package)_config_opts+=-DEVENT__DISABLE_TESTS=ON -DEVENT__LIBRARY_TYPE=STATIC $(package)_cflags += -fdebug-prefix-map=$($(package)_extract_dir)=/usr -fmacro-prefix-map=$($(package)_extract_dir)=/usr - $(package)_cppflags += -D_GNU_SOURCE -D_FORTIFY_SOURCE=3 + $(package)_cppflags += -D_GNU_SOURCE -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 $(package)_cppflags_mingw32=-D_WIN32_WINNT=0x0A00 endef From 664657ed134365588914c2cf6a3975ce368a4f49 Mon Sep 17 00:00:00 2001 From: scgbckbone Date: Tue, 17 Dec 2024 11:21:56 +0100 Subject: [PATCH 0007/2363] bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label * do not only check user provided range data to decide whether descriptor is ranged * properly handle std::optional when checking if descriptor is internal --- src/wallet/rpc/backup.cpp | 8 +++++--- test/functional/wallet_importdescriptors.py | 13 +++++++++++++ test/functional/wallet_rescan_unconfirmed.py | 3 ++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index e12453458c46..f4b6e192de86 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1487,6 +1487,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c } // Range check + std::optional is_ranged; int64_t range_start = 0, range_end = 1, next_index = 0; if (!parsed_descs.at(0)->IsRange() && data.exists("range")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor"); @@ -1501,6 +1502,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c range_end = wallet.m_keypool_size; } next_index = range_start; + is_ranged = true; if (data.exists("next_index")) { next_index = data["next_index"].getInt(); @@ -1522,12 +1524,13 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c } // Ranged descriptors should not have a label - if (data.exists("range") && data.exists("label")) { + if (is_ranged.has_value() && is_ranged.value() && data.exists("label")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Ranged descriptors should not have a label"); } + bool desc_internal = internal.has_value() && internal.value(); // Internal addresses should not have a label either - if (internal && data.exists("label")) { + if (desc_internal && data.exists("label")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label"); } @@ -1543,7 +1546,6 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c for (size_t j = 0; j < parsed_descs.size(); ++j) { auto parsed_desc = std::move(parsed_descs[j]); - bool desc_internal = internal.has_value() && internal.value(); if (parsed_descs.size() == 2) { desc_internal = j == 1; } else if (parsed_descs.size() > 2) { diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index bd50b8cdc028..151ab05de478 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -118,6 +118,9 @@ def run_test(self): error_code=-8, error_message="Internal addresses should not have a label") + self.log.info("External non-ranged addresses can have labels") + self.test_importdesc({**import_request, "internal": False}, success=True) + self.log.info("Internal addresses should be detected as such") key = get_generate_key() self.test_importdesc({"desc": descsum_create("pkh(" + key.pubkey + ")"), @@ -219,6 +222,16 @@ def run_test(self): error_code=-8, error_message='Ranged descriptors should not have a label') + self.log.info("Ranged descriptors cannot have labels - even if range not provided by user and only implied by asterisk (*)") + self.test_importdesc({"desc":descsum_create("wpkh(" + xpub + "/100/0/*)"), + "timestamp": "now", + "label": "test", + "active": True}, + success=False, + warnings=['Range not given, using default keypool range'], + error_code=-8, + error_message='Ranged descriptors should not have a label') + self.log.info("Private keys required for private keys enabled wallet") self.test_importdesc({"desc":descsum_create(desc), "timestamp": "now", diff --git a/test/functional/wallet_rescan_unconfirmed.py b/test/functional/wallet_rescan_unconfirmed.py index 23c58b92f429..4a7e44c0ddcb 100755 --- a/test/functional/wallet_rescan_unconfirmed.py +++ b/test/functional/wallet_rescan_unconfirmed.py @@ -65,7 +65,8 @@ def run_test(self): assert tx_parent_to_reorg["txid"] in node.getrawmempool() self.log.info("Import descriptor wallet on another node") - descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0, "label": "w0 import"}] + # descriptor is ranged - label not allowed + descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0}] node.createwallet(wallet_name="w1", disable_private_keys=True) w1 = node.get_wallet_rpc("w1") From 84820561dcb2d156d1a1151a480fc1be6649cae4 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 3 May 2025 08:41:02 -0400 Subject: [PATCH 0008/2363] validation: periodically flush dbcache during reindex-chainstate Move the periodic flush inside the outer loop of ActivateBestChain. For very long activations, such as with reindex-chainstate, this calls periodic flushes so progress can be saved to disk. Co-Authored-By: l0rinc --- src/validation.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 5ad2ebdcd7eb..94b9de3a49e7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3567,6 +3567,11 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< m_chainman.MaybeRebalanceCaches(); } + // Write changes periodically to disk, after relay. + if (!FlushStateToDisk(state, FlushStateMode::PERIODIC)) { + return false; + } + if (WITH_LOCK(::cs_main, return m_disabled)) { // Background chainstate has reached the snapshot base block, so exit. @@ -3591,11 +3596,6 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< m_chainman.CheckBlockIndex(); - // Write changes periodically to disk, after relay. - if (!FlushStateToDisk(state, FlushStateMode::PERIODIC)) { - return false; - } - return true; } From 41479ed1d23ea752d0ce14c2cf5627f43bceb722 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 4 May 2025 13:34:29 -0400 Subject: [PATCH 0009/2363] test: add test for periodic flush inside ActivateBestChain --- src/test/chainstate_write_tests.cpp | 61 ++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/src/test/chainstate_write_tests.cpp b/src/test/chainstate_write_tests.cpp index ccca2f9be101..e1b82ebc1210 100644 --- a/src/test/chainstate_write_tests.cpp +++ b/src/test/chainstate_write_tests.cpp @@ -8,6 +8,10 @@ #include +// Taken from validation.cpp +static constexpr auto DATABASE_WRITE_INTERVAL_MIN{50min}; +static constexpr auto DATABASE_WRITE_INTERVAL_MAX{70min}; + BOOST_AUTO_TEST_SUITE(chainstate_write_tests) BOOST_FIXTURE_TEST_CASE(chainstate_write_interval, TestingSetup) @@ -31,15 +35,68 @@ BOOST_FIXTURE_TEST_CASE(chainstate_write_interval, TestingSetup) BOOST_CHECK(!sub->m_did_flush); // The periodic flush interval is between 50 and 70 minutes (inclusive) - SetMockTime(GetTime() + 49min); + SetMockTime(GetTime() + DATABASE_WRITE_INTERVAL_MIN - 1min); chainstate.FlushStateToDisk(state_dummy, FlushStateMode::PERIODIC); m_node.validation_signals->SyncWithValidationInterfaceQueue(); BOOST_CHECK(!sub->m_did_flush); - SetMockTime(GetTime() + 70min); + SetMockTime(GetTime() + DATABASE_WRITE_INTERVAL_MAX); chainstate.FlushStateToDisk(state_dummy, FlushStateMode::PERIODIC); m_node.validation_signals->SyncWithValidationInterfaceQueue(); BOOST_CHECK(sub->m_did_flush); } +// Test that we do PERIODIC flushes inside ActivateBestChain. +// This is necessary for reindex-chainstate to be able to periodically flush +// before reaching chain tip. +BOOST_FIXTURE_TEST_CASE(write_during_multiblock_activation, TestChain100Setup) +{ + struct TestSubscriber final : CValidationInterface + { + const CBlockIndex* m_tip{nullptr}; + const CBlockIndex* m_flushed_at_block{nullptr}; + void ChainStateFlushed(ChainstateRole, const CBlockLocator&) override + { + m_flushed_at_block = m_tip; + } + void UpdatedBlockTip(const CBlockIndex* block_index, const CBlockIndex*, bool) override { + m_tip = block_index; + } + }; + + auto& chainstate{Assert(m_node.chainman)->ActiveChainstate()}; + BlockValidationState state_dummy{}; + + // Pop two blocks from the tip + const CBlockIndex* tip{chainstate.m_chain.Tip()}; + CBlockIndex* second_from_tip{tip->pprev}; + + { + LOCK2(m_node.chainman->GetMutex(), chainstate.MempoolMutex()); + chainstate.DisconnectTip(state_dummy, nullptr); + chainstate.DisconnectTip(state_dummy, nullptr); + } + + BOOST_CHECK_EQUAL(second_from_tip->pprev, chainstate.m_chain.Tip()); + + // Set m_next_write to current time + chainstate.FlushStateToDisk(state_dummy, FlushStateMode::ALWAYS); + m_node.validation_signals->SyncWithValidationInterfaceQueue(); + // The periodic flush interval is between 50 and 70 minutes (inclusive) + // The next call to a PERIODIC write will flush + SetMockTime(GetMockTime() + DATABASE_WRITE_INTERVAL_MAX); + + const auto sub{std::make_shared()}; + m_node.validation_signals->RegisterSharedValidationInterface(sub); + + // ActivateBestChain back to tip + chainstate.ActivateBestChain(state_dummy, nullptr); + BOOST_CHECK_EQUAL(tip, chainstate.m_chain.Tip()); + // Check that we flushed inside ActivateBestChain while we were at the + // second block from tip, since FlushStateToDisk is called with PERIODIC + // inside the outer loop. + m_node.validation_signals->SyncWithValidationInterfaceQueue(); + BOOST_CHECK_EQUAL(sub->m_flushed_at_block, second_from_tip); +} + BOOST_AUTO_TEST_SUITE_END() From c1e554d3e5834a140f2a53854018499a3bfe6822 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sun, 4 May 2025 13:35:29 -0400 Subject: [PATCH 0010/2363] refactor: consolidate 3 separate locks into one block The main lock needs to be taken 3 separate times. It simplifies readability to take it once, and is more efficient. --- src/validation.cpp | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 94b9de3a49e7..b3f6d637a988 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3560,19 +3560,24 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< } // release cs_main // When we reach this point, we switched to a new tip (stored in pindexNewTip). - if (exited_ibd) { - // If a background chainstate is in use, we may need to rebalance our - // allocation of caches once a chainstate exits initial block download. - LOCK(::cs_main); - m_chainman.MaybeRebalanceCaches(); - } + bool disabled{false}; + { + LOCK(m_chainman.GetMutex()); + if (exited_ibd) { + // If a background chainstate is in use, we may need to rebalance our + // allocation of caches once a chainstate exits initial block download. + m_chainman.MaybeRebalanceCaches(); + } - // Write changes periodically to disk, after relay. - if (!FlushStateToDisk(state, FlushStateMode::PERIODIC)) { - return false; + // Write changes periodically to disk, after relay. + if (!FlushStateToDisk(state, FlushStateMode::PERIODIC)) { + return false; + } + + disabled = m_disabled; } - if (WITH_LOCK(::cs_main, return m_disabled)) { + if (disabled) { // Background chainstate has reached the snapshot base block, so exit. // Restart indexes to resume indexing for all blocks unique to the snapshot From d31158d3646f3c7e4832b9ca50f6ffe02800ff4c Mon Sep 17 00:00:00 2001 From: rkrux Date: Mon, 5 May 2025 17:04:27 +0530 Subject: [PATCH 0011/2363] psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows The unserialization flows of the PSBT types work based on few underlying assumptions of functions from `serialize.h` & `stream.h` that takes some to understand when read the first time. Add few comments that highlight these assumptions hopefully making it easier to grasp. Also, mention key/value format types as per BIP 174. --- src/psbt.h | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/psbt.h b/src/psbt.h index dd7b02dc6071..03b1462e6064 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -465,7 +465,8 @@ struct PSBTInput // Read loop bool found_sep = false; while(!s.empty()) { - // Read + // Read the key of format "" after which + // "key" will contain "" std::vector key; s >> key; @@ -476,11 +477,13 @@ struct PSBTInput break; } - // Type is compact size uint at beginning of key + // "skey" is used so that "key" is unchanged after reading keytype below SpanReader skey{key}; + // keytype is of the format compact size uint at the beginning of "key" uint64_t type = ReadCompactSize(skey); - // Do stuff based on type + // Do stuff based on keytype "type", i.e., key checks, reading values of the + // format "" from the stream "s", and value checks switch(type) { case PSBT_IN_NON_WITNESS_UTXO: { @@ -949,7 +952,8 @@ struct PSBTOutput // Read loop bool found_sep = false; while(!s.empty()) { - // Read + // Read the key of format "" after which + // "key" will contain "" std::vector key; s >> key; @@ -960,11 +964,13 @@ struct PSBTOutput break; } - // Type is compact size uint at beginning of key + // "skey" is used so that "key" is unchanged after reading keytype below SpanReader skey{key}; + // keytype is of the format compact size uint at the beginning of "key" uint64_t type = ReadCompactSize(skey); - // Do stuff based on type + // Do stuff based on keytype "type", i.e., key checks, reading values of the + // format "" from the stream "s", and value checks switch(type) { case PSBT_OUT_REDEEMSCRIPT: { @@ -1212,7 +1218,8 @@ struct PartiallySignedTransaction // Read global data bool found_sep = false; while(!s.empty()) { - // Read + // Read the key of format "" after which + // "key" will contain "" std::vector key; s >> key; @@ -1223,11 +1230,13 @@ struct PartiallySignedTransaction break; } - // Type is compact size uint at beginning of key + // "skey" is used so that "key" is unchanged after reading keytype below SpanReader skey{key}; + // keytype is of the format compact size uint at the beginning of "key" uint64_t type = ReadCompactSize(skey); - // Do stuff based on type + // Do stuff based on keytype "type", i.e., key checks, reading values of the + // format "" from the stream "s", and value checks switch(type) { case PSBT_GLOBAL_UNSIGNED_TX: { From 58e55b17e632dbd4425dd64825b087f242ac4b7b Mon Sep 17 00:00:00 2001 From: brunoerg Date: Wed, 14 May 2025 10:42:28 -0300 Subject: [PATCH 0012/2363] test: descriptor: cover invalid multi/multi_a cases --- src/test/descriptor_tests.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index c063dd90af34..74c74c5819b7 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -927,6 +927,9 @@ BOOST_AUTO_TEST_CASE(descriptor_test) CheckUnparsable("multi(+1,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(+1,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "Multi threshold '+1' is not valid"); // Invalid threshold CheckUnparsable("multi(0,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(0,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "Multisig threshold cannot be 0, must be at least 1"); // Threshold of 0 CheckUnparsable("multi(3,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "multi(3,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,04a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd5b8dec5235a0fa8722476c7709c02559e3aa73aa03918ba2d492eea75abea235)", "Multisig threshold cannot be larger than the number of keys; threshold is 3 but only 2 keys specified"); // Threshold larger than number of keys + CheckUnparsable("", "multi(0)()", "Multi: expected ',', got ')'"); + CheckUnparsable("", "multi(123)", "Cannot have 0 keys in multisig; must have between 1 and 20 keys, inclusive"); + CheckUnparsable("", "tr(a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd,multi_a(1))", "Cannot have 0 keys in multi_a; must have between 1 and 999 keys, inclusive"); CheckUnparsable("multi(3,KzoAz5CanayRKex3fSLQ2BwJpN7U52gZvxMyk78nDMHuqrUxuSJy,KwGNz6YCCQtYvFzMtrC6D3tKTKdBBboMrLTsjr2NYVBwapCkn7Mr,KxogYhiNfwxuswvXV66eFyKcCpm7dZ7TqHVqujHAVUjJxyivxQ9X,L2BUNduTSyZwZjwNHynQTF14mv2uz2NRq5n5sYWTb4FkkmqgEE9f)", "multi(3,03669b8afcec803a0d323e9a17f3ea8e68e8abe5a278020a929adbec52421adbd0,0260b2003c386519fc9eadf2b5cf124dd8eea4c4e68d5e154050a9346ea98ce600,0362a74e399c39ed5593852a30147f2959b56bb827dfa3e60e464b02ccf87dc5e8,0261345b53de74a4d721ef877c255429961b7e43714171ac06168d7e08c542a8b8)", "Cannot have 4 pubkeys in bare multisig; only at most 3 pubkeys"); // Threshold larger than number of keys CheckUnparsable("sh(multi(16,KzoAz5CanayRKex3fSLQ2BwJpN7U52gZvxMyk78nDMHuqrUxuSJy,KwGNz6YCCQtYvFzMtrC6D3tKTKdBBboMrLTsjr2NYVBwapCkn7Mr,KxogYhiNfwxuswvXV66eFyKcCpm7dZ7TqHVqujHAVUjJxyivxQ9X,L2BUNduTSyZwZjwNHynQTF14mv2uz2NRq5n5sYWTb4FkkmqgEE9f,L1okJGHGn1kFjdXHKxXjwVVtmCMR2JA5QsbKCSpSb7ReQjezKeoD,KxDCNSST75HFPaW5QKpzHtAyaCQC7p9Vo3FYfi2u4dXD1vgMiboK,L5edQjFtnkcf5UWURn6UuuoFrabgDQUHdheKCziwN42aLwS3KizU,KzF8UWFcEC7BYTq8Go1xVimMkDmyNYVmXV5PV7RuDicvAocoPB8i,L3nHUboKG2w4VSJ5jYZ5CBM97oeK6YuKvfZxrefdShECcjEYKMWZ,KyjHo36dWkYhimKmVVmQTq3gERv3pnqA4xFCpvUgbGDJad7eS8WE,KwsfyHKRUTZPQtysN7M3tZ4GXTnuov5XRgjdF2XCG8faAPmFruRF,KzCUbGhN9LJhdeFfL9zQgTJMjqxdBKEekRGZX24hXdgCNCijkkap,KzgpMBwwsDLwkaC5UrmBgCYaBD2WgZ7PBoGYXR8KT7gCA9UTN5a3,KyBXTPy4T7YG4q9tcAM3LkvfRpD1ybHMvcJ2ehaWXaSqeGUxEdkP,KzJDe9iwJRPtKP2F2AoN6zBgzS7uiuAwhWCfGdNeYJ3PC1HNJ8M8,L1xbHrxynrqLKkoYc4qtoQPx6uy5qYXR5ZDYVYBSRmCV5piU3JG9,L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1))","sh(multi(16,03669b8afcec803a0d323e9a17f3ea8e68e8abe5a278020a929adbec52421adbd0,0260b2003c386519fc9eadf2b5cf124dd8eea4c4e68d5e154050a9346ea98ce600,0362a74e399c39ed5593852a30147f2959b56bb827dfa3e60e464b02ccf87dc5e8,0261345b53de74a4d721ef877c255429961b7e43714171ac06168d7e08c542a8b8,02da72e8b46901a65d4374fe6315538d8f368557dda3a1dcf9ea903f3afe7314c8,0318c82dd0b53fd3a932d16e0ba9e278fcc937c582d5781be626ff16e201f72286,0297ccef1ef99f9d73dec9ad37476ddb232f1238aff877af19e72ba04493361009,02e502cfd5c3f972fe9a3e2a18827820638f96b6f347e54d63deb839011fd5765d,03e687710f0e3ebe81c1037074da939d409c0025f17eb86adb9427d28f0f7ae0e9,02c04d3a5274952acdbc76987f3184b346a483d43be40874624b29e3692c1df5af,02ed06e0f418b5b43a7ec01d1d7d27290fa15f75771cb69b642a51471c29c84acd,036d46073cbb9ffee90473f3da429abc8de7f8751199da44485682a989a4bebb24,02f5d1ff7c9029a80a4e36b9a5497027ef7f3e73384a4a94fbfe7c4e9164eec8bc,02e41deffd1b7cce11cde209a781adcffdabd1b91c0ba0375857a2bfd9302419f3,02d76625f7956a7fc505ab02556c23ee72d832f1bac391bcd2d3abce5710a13d06,0399eb0a5487515802dc14544cf10b3666623762fbed2ec38a3975716e2c29c232,03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd))", "P2SH script is too large, 581 bytes is larger than 520 bytes"); // Cannot have more than 15 keys in a P2SH multisig, or we exceed maximum push size Check("wsh(multi(20,KzoAz5CanayRKex3fSLQ2BwJpN7U52gZvxMyk78nDMHuqrUxuSJy,KwGNz6YCCQtYvFzMtrC6D3tKTKdBBboMrLTsjr2NYVBwapCkn7Mr,KxogYhiNfwxuswvXV66eFyKcCpm7dZ7TqHVqujHAVUjJxyivxQ9X,L2BUNduTSyZwZjwNHynQTF14mv2uz2NRq5n5sYWTb4FkkmqgEE9f,L1okJGHGn1kFjdXHKxXjwVVtmCMR2JA5QsbKCSpSb7ReQjezKeoD,KxDCNSST75HFPaW5QKpzHtAyaCQC7p9Vo3FYfi2u4dXD1vgMiboK,L5edQjFtnkcf5UWURn6UuuoFrabgDQUHdheKCziwN42aLwS3KizU,KzF8UWFcEC7BYTq8Go1xVimMkDmyNYVmXV5PV7RuDicvAocoPB8i,L3nHUboKG2w4VSJ5jYZ5CBM97oeK6YuKvfZxrefdShECcjEYKMWZ,KyjHo36dWkYhimKmVVmQTq3gERv3pnqA4xFCpvUgbGDJad7eS8WE,KwsfyHKRUTZPQtysN7M3tZ4GXTnuov5XRgjdF2XCG8faAPmFruRF,KzCUbGhN9LJhdeFfL9zQgTJMjqxdBKEekRGZX24hXdgCNCijkkap,KzgpMBwwsDLwkaC5UrmBgCYaBD2WgZ7PBoGYXR8KT7gCA9UTN5a3,KyBXTPy4T7YG4q9tcAM3LkvfRpD1ybHMvcJ2ehaWXaSqeGUxEdkP,KzJDe9iwJRPtKP2F2AoN6zBgzS7uiuAwhWCfGdNeYJ3PC1HNJ8M8,L1xbHrxynrqLKkoYc4qtoQPx6uy5qYXR5ZDYVYBSRmCV5piU3JG9,KzRedjSwMggebB3VufhbzpYJnvHfHe9kPJSjCU5QpJdAW3NSZxYS,Kyjtp5858xL7JfeV4PNRCKy2t6XvgqNNepArGY9F9F1SSPqNEMs3,L2D4RLHPiHBidkHS8ftx11jJk1hGFELvxh8LoxNQheaGT58dKenW,KyLPZdwY4td98bKkXqEXTEBX3vwEYTQo1yyLjX2jKXA63GBpmSjv))","wsh(multi(20,03669b8afcec803a0d323e9a17f3ea8e68e8abe5a278020a929adbec52421adbd0,0260b2003c386519fc9eadf2b5cf124dd8eea4c4e68d5e154050a9346ea98ce600,0362a74e399c39ed5593852a30147f2959b56bb827dfa3e60e464b02ccf87dc5e8,0261345b53de74a4d721ef877c255429961b7e43714171ac06168d7e08c542a8b8,02da72e8b46901a65d4374fe6315538d8f368557dda3a1dcf9ea903f3afe7314c8,0318c82dd0b53fd3a932d16e0ba9e278fcc937c582d5781be626ff16e201f72286,0297ccef1ef99f9d73dec9ad37476ddb232f1238aff877af19e72ba04493361009,02e502cfd5c3f972fe9a3e2a18827820638f96b6f347e54d63deb839011fd5765d,03e687710f0e3ebe81c1037074da939d409c0025f17eb86adb9427d28f0f7ae0e9,02c04d3a5274952acdbc76987f3184b346a483d43be40874624b29e3692c1df5af,02ed06e0f418b5b43a7ec01d1d7d27290fa15f75771cb69b642a51471c29c84acd,036d46073cbb9ffee90473f3da429abc8de7f8751199da44485682a989a4bebb24,02f5d1ff7c9029a80a4e36b9a5497027ef7f3e73384a4a94fbfe7c4e9164eec8bc,02e41deffd1b7cce11cde209a781adcffdabd1b91c0ba0375857a2bfd9302419f3,02d76625f7956a7fc505ab02556c23ee72d832f1bac391bcd2d3abce5710a13d06,0399eb0a5487515802dc14544cf10b3666623762fbed2ec38a3975716e2c29c232,02bc2feaa536991d269aae46abb8f3772a5b3ad592314945e51543e7da84c4af6e,0318bf32e5217c1eb771a6d5ce1cd39395dff7ff665704f175c9a5451d95a2f2ca,02c681a6243f16208c2004bb81f5a8a67edfdd3e3711534eadeec3dcf0b010c759,0249fdd6b69768b8d84b4893f8ff84b36835c50183de20fcae8f366a45290d01fd))", "wsh(multi(20,03669b8afcec803a0d323e9a17f3ea8e68e8abe5a278020a929adbec52421adbd0,0260b2003c386519fc9eadf2b5cf124dd8eea4c4e68d5e154050a9346ea98ce600,0362a74e399c39ed5593852a30147f2959b56bb827dfa3e60e464b02ccf87dc5e8,0261345b53de74a4d721ef877c255429961b7e43714171ac06168d7e08c542a8b8,02da72e8b46901a65d4374fe6315538d8f368557dda3a1dcf9ea903f3afe7314c8,0318c82dd0b53fd3a932d16e0ba9e278fcc937c582d5781be626ff16e201f72286,0297ccef1ef99f9d73dec9ad37476ddb232f1238aff877af19e72ba04493361009,02e502cfd5c3f972fe9a3e2a18827820638f96b6f347e54d63deb839011fd5765d,03e687710f0e3ebe81c1037074da939d409c0025f17eb86adb9427d28f0f7ae0e9,02c04d3a5274952acdbc76987f3184b346a483d43be40874624b29e3692c1df5af,02ed06e0f418b5b43a7ec01d1d7d27290fa15f75771cb69b642a51471c29c84acd,036d46073cbb9ffee90473f3da429abc8de7f8751199da44485682a989a4bebb24,02f5d1ff7c9029a80a4e36b9a5497027ef7f3e73384a4a94fbfe7c4e9164eec8bc,02e41deffd1b7cce11cde209a781adcffdabd1b91c0ba0375857a2bfd9302419f3,02d76625f7956a7fc505ab02556c23ee72d832f1bac391bcd2d3abce5710a13d06,0399eb0a5487515802dc14544cf10b3666623762fbed2ec38a3975716e2c29c232,02bc2feaa536991d269aae46abb8f3772a5b3ad592314945e51543e7da84c4af6e,0318bf32e5217c1eb771a6d5ce1cd39395dff7ff665704f175c9a5451d95a2f2ca,02c681a6243f16208c2004bb81f5a8a67edfdd3e3711534eadeec3dcf0b010c759,0249fdd6b69768b8d84b4893f8ff84b36835c50183de20fcae8f366a45290d01fd))", SIGNABLE, {{"0020376bd8344b8b6ebe504ff85ef743eaa1aa9272178223bcb6887e9378efb341ac"}}, OutputType::BECH32, /*op_desc_id=*/uint256{"2bb9d418ebdc3a75c465383985881527f3e5d6e520fb3efb152d4191b80e8412"}); // In P2WSH we can have up to 20 keys From b30fca7498c93356fbdb8c2ce881aa8e548bae17 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 27 May 2025 03:40:34 +0200 Subject: [PATCH 0013/2363] contrib: utxo_to_sqlite.py: add options to store txid/spk as BLOBs Co-authored-by: Anthony Towns --- contrib/utxo-tools/utxo_to_sqlite.py | 29 +++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/contrib/utxo-tools/utxo_to_sqlite.py b/contrib/utxo-tools/utxo_to_sqlite.py index 4758fe39aaa5..56de73698c20 100755 --- a/contrib/utxo-tools/utxo_to_sqlite.py +++ b/contrib/utxo-tools/utxo_to_sqlite.py @@ -9,6 +9,9 @@ The created database contains a table `utxos` with the following schema: (txid TEXT, vout INT, value INT, coinbase INT, height INT, scriptpubkey TEXT) + +If --txid=raw or --txid=rawle is specified, txid will be BLOB instead; +if --spk=raw, then scriptpubkey will be BLOB instead. """ import argparse import os @@ -111,7 +114,9 @@ def main(): parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) parser.add_argument('infile', help='filename of compact-serialized UTXO set (input)') parser.add_argument('outfile', help='filename of created SQLite3 database (output)') - parser.add_argument('-v', '--verbose', action='store_true', help='show details about each UTXO') + parser.add_argument('--verbose', action='store_true', help='show details about each UTXO') + parser.add_argument('--spk', choices=['hex', 'raw'], default='hex', help='encode scriptPubKey as hex or raw bytes') + parser.add_argument('--txid', choices=['hex', 'raw', 'rawle'], default='hex', help='encode txid as hex, raw bytes (sha256 byteorder), or reversed raw bytes (little endian)') args = parser.parse_args() if not os.path.exists(args.infile): @@ -122,9 +127,15 @@ def main(): print(f"Error: provided output file '{args.outfile}' already exists.") sys.exit(1) + spk_hex = (args.spk == 'hex') + txid_hex = (args.txid == 'hex') + txid_reverse = (args.txid != 'raw') + # create database table + txid_fmt = "TEXT" if txid_hex else "BLOB" + spk_fmt = "TEXT" if spk_hex else "BLOB" con = sqlite3.connect(args.outfile) - con.execute("CREATE TABLE utxos(txid TEXT, vout INT, value INT, coinbase INT, height INT, scriptpubkey TEXT)") + con.execute(f"CREATE TABLE utxos(txid {txid_fmt}, vout INT, value INT, coinbase INT, height INT, scriptpubkey {spk_fmt})") # read metadata (magic bytes, version, network magic, block hash, UTXO count) f = open(args.infile, 'rb') @@ -153,7 +164,7 @@ def main(): for coin_idx in range(1, num_utxos+1): # read key (COutPoint) if coins_per_hash_left == 0: # read next prevout hash - prevout_hash = f.read(32)[::-1].hex() + prevout_hash = f.read(32) coins_per_hash_left = read_compactsize(f) prevout_index = read_compactsize(f) # read value (Coin) @@ -161,17 +172,21 @@ def main(): height = code >> 1 is_coinbase = code & 1 amount = decompress_amount(read_varint(f)) - scriptpubkey = decompress_script(f).hex() - write_batch.append((prevout_hash, prevout_index, amount, is_coinbase, height, scriptpubkey)) + scriptpubkey = decompress_script(f) + + scriptpubkey_write = scriptpubkey.hex() if spk_hex else scriptpubkey + txid_write = prevout_hash[::-1] if txid_reverse else prevout_hash + txid_write = txid_write.hex() if txid_hex else txid_write + write_batch.append((txid_write, prevout_index, amount, is_coinbase, height, scriptpubkey_write)) if height > max_height: max_height = height coins_per_hash_left -= 1 if args.verbose: print(f"Coin {coin_idx}/{num_utxos}:") - print(f" prevout = {prevout_hash}:{prevout_index}") + print(f" prevout = {prevout_hash[::-1].hex()}:{prevout_index}") print(f" amount = {amount}, height = {height}, coinbase = {is_coinbase}") - print(f" scriptPubKey = {scriptpubkey}\n") + print(f" scriptPubKey = {scriptpubkey.hex()}\n") if coin_idx % (16*1024) == 0 or coin_idx == num_utxos: # write utxo batch to database From 7378f27b4fb512567b6152f986f67d9263d08d7a Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 27 May 2025 03:41:53 +0200 Subject: [PATCH 0014/2363] test: run utxo-to-sqlite script test with spk/txid format option combinations --- test/functional/tool_utxo_to_sqlite.py | 53 ++++++++++++++++++-------- 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/test/functional/tool_utxo_to_sqlite.py b/test/functional/tool_utxo_to_sqlite.py index 2da7c42a86bc..d1f3e7e19348 100755 --- a/test/functional/tool_utxo_to_sqlite.py +++ b/test/functional/tool_utxo_to_sqlite.py @@ -3,6 +3,7 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test utxo-to-sqlite conversion tool""" +from itertools import product import os.path try: import sqlite3 @@ -15,6 +16,7 @@ from test_framework.messages import ( COutPoint, CTxOut, + uint256_from_str, ) from test_framework.crypto.muhash import MuHash3072 from test_framework.script import ( @@ -38,15 +40,33 @@ from test_framework.wallet import MiniWallet -def calculate_muhash_from_sqlite_utxos(filename): +def calculate_muhash_from_sqlite_utxos(filename, txid_format, spk_format): muhash = MuHash3072() con = sqlite3.connect(filename) cur = con.cursor() - for (txid_hex, vout, value, coinbase, height, spk_hex) in cur.execute("SELECT * FROM utxos"): + for (txid, vout, value, coinbase, height, spk) in cur.execute("SELECT * FROM utxos"): + match txid_format: + case "hex": + assert type(txid) is str + txid_bytes = bytes.fromhex(txid)[::-1] + case "raw": + assert type(txid) is bytes + txid_bytes = txid + case "rawle": + assert type(txid) is bytes + txid_bytes = txid[::-1] + match spk_format: + case "hex": + assert type(spk) is str + spk_bytes = bytes.fromhex(spk) + case "raw": + assert type(spk) is bytes + spk_bytes = spk + # serialize UTXO for MuHash (see function `TxOutSer` in the coinstats module) - utxo_ser = COutPoint(int(txid_hex, 16), vout).serialize() + utxo_ser = COutPoint(uint256_from_str(txid_bytes), vout).serialize() utxo_ser += (height * 2 + coinbase).to_bytes(4, 'little') - utxo_ser += CTxOut(value, bytes.fromhex(spk_hex)).serialize() + utxo_ser += CTxOut(value, spk_bytes).serialize() muhash.insert(utxo_ser) con.close() return muhash.digest()[::-1].hex() @@ -100,17 +120,20 @@ def run_test(self): input_filename = os.path.join(self.options.tmpdir, "utxos.dat") node.dumptxoutset(input_filename, "latest") - self.log.info('Convert UTXO set from compact-serialized format to sqlite format') - output_filename = os.path.join(self.options.tmpdir, "utxos.sqlite") - base_dir = self.config["environment"]["SRCDIR"] - utxo_to_sqlite_path = os.path.join(base_dir, "contrib", "utxo-tools", "utxo_to_sqlite.py") - subprocess.run([sys.executable, utxo_to_sqlite_path, input_filename, output_filename], - check=True, stderr=subprocess.STDOUT) - - self.log.info('Verify that both UTXO sets match by comparing their MuHash') - muhash_sqlite = calculate_muhash_from_sqlite_utxos(output_filename) - muhash_compact_serialized = node.gettxoutsetinfo('muhash')['muhash'] - assert_equal(muhash_sqlite, muhash_compact_serialized) + for i, (txid_format, spk_format) in enumerate(product(["hex", "raw", "rawle"], ["hex", "raw"])): + self.log.info(f'Test utxo-to-sqlite script using txid format "{txid_format}" and spk format "{spk_format}" ({i+1})') + self.log.info('-> Convert UTXO set from compact-serialized format to sqlite format') + output_filename = os.path.join(self.options.tmpdir, f"utxos_{i+1}.sqlite") + base_dir = self.config["environment"]["SRCDIR"] + utxo_to_sqlite_path = os.path.join(base_dir, "contrib", "utxo-tools", "utxo_to_sqlite.py") + arguments = [input_filename, output_filename, f'--txid={txid_format}', f'--spk={spk_format}'] + subprocess.run([sys.executable, utxo_to_sqlite_path] + arguments, check=True, stderr=subprocess.STDOUT) + + self.log.info('-> Verify that both UTXO sets match by comparing their MuHash') + muhash_sqlite = calculate_muhash_from_sqlite_utxos(output_filename, txid_format, spk_format) + muhash_compact_serialized = node.gettxoutsetinfo('muhash')['muhash'] + assert_equal(muhash_sqlite, muhash_compact_serialized) + self.log.info('') if __name__ == "__main__": From e883b37768812d96feec207a37202c7d1b603c1f Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 15 Nov 2024 15:06:39 +0100 Subject: [PATCH 0015/2363] fuzz: set the output argument of FuzzedSock::Accept() `FuzzedSock::Accept()` properly returns a new socket, but it forgot to set the output argument `addr`, like `accept(2)` is expected to. This could lead to reading uninitialized data during testing when we read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family` member. Set `addr` to a fuzzed IPv4 or IPv6 address. --- src/test/fuzz/util/net.cpp | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/test/fuzz/util/net.cpp b/src/test/fuzz/util/net.cpp index 8cbf6bdffec1..e49c043364e0 100644 --- a/src/test/fuzz/util/net.cpp +++ b/src/test/fuzz/util/net.cpp @@ -312,6 +312,33 @@ std::unique_ptr FuzzedSock::Accept(sockaddr* addr, socklen_t* addr_len) co SetFuzzedErrNo(m_fuzzed_data_provider, accept_errnos); return std::unique_ptr(); } + if (addr != nullptr) { + // Set a fuzzed address in the output argument addr. + memset(addr, 0x00, *addr_len); + if (m_fuzzed_data_provider.ConsumeBool()) { + // IPv4 + const socklen_t write_len = static_cast(sizeof(sockaddr_in)); + if (*addr_len >= write_len) { + *addr_len = write_len; + auto addr4 = reinterpret_cast(addr); + addr4->sin_family = AF_INET; + const auto sin_addr_bytes = m_fuzzed_data_provider.ConsumeBytes(sizeof(addr4->sin_addr)); + memcpy(&addr4->sin_addr, sin_addr_bytes.data(), sin_addr_bytes.size()); + addr4->sin_port = m_fuzzed_data_provider.ConsumeIntegralInRange(1, 65535); + } + } else { + // IPv6 + const socklen_t write_len = static_cast(sizeof(sockaddr_in6)); + if (*addr_len >= write_len) { + *addr_len = write_len; + auto addr6 = reinterpret_cast(addr); + addr6->sin6_family = AF_INET6; + const auto sin_addr_bytes = m_fuzzed_data_provider.ConsumeBytes(sizeof(addr6->sin6_addr)); + memcpy(&addr6->sin6_addr, sin_addr_bytes.data(), sin_addr_bytes.size()); + addr6->sin6_port = m_fuzzed_data_provider.ConsumeIntegralInRange(1, 65535); + } + } + } return std::make_unique(m_fuzzed_data_provider); } From e6a917c8f8e0f1a0fa71dc9bbb6e1074f81edea3 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 9 Jun 2025 13:29:19 +0200 Subject: [PATCH 0016/2363] fuzz: add Fuzzed NetEventsInterface and use it in connman tests --- src/test/fuzz/connman.cpp | 2 ++ src/test/fuzz/util/net.h | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index a62d227da8ef..8513f07feadb 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -51,6 +51,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) } } AddrManDeterministic& addr_man{*addr_man_ptr}; + auto net_events{ConsumeNetEvents(fuzzed_data_provider)}; ConnmanTestMsg connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addr_man, @@ -60,6 +61,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) const uint64_t max_outbound_limit{fuzzed_data_provider.ConsumeIntegral()}; CConnman::Options options; + options.m_msgproc = &net_events; options.nMaxOutboundLimit = max_outbound_limit; connman.Init(options); diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h index 698001a7f15e..200638441eec 100644 --- a/src/test/fuzz/util/net.h +++ b/src/test/fuzz/util/net.h @@ -139,6 +139,25 @@ class AddrManDeterministic : public AddrMan } }; +class FuzzedNetEvents : public NetEventsInterface +{ +public: + FuzzedNetEvents(FuzzedDataProvider& fdp) : m_fdp(fdp) {} + + virtual void InitializeNode(const CNode&, ServiceFlags) override {} + + virtual void FinalizeNode(const CNode&) override {} + + virtual bool HasAllDesirableServiceFlags(ServiceFlags) const override { return m_fdp.ConsumeBool(); } + + virtual bool ProcessMessages(CNode*, std::atomic&) override { return m_fdp.ConsumeBool(); } + + virtual bool SendMessages(CNode*) override { return m_fdp.ConsumeBool(); } + +private: + FuzzedDataProvider& m_fdp; +}; + class FuzzedSock : public Sock { FuzzedDataProvider& m_fuzzed_data_provider; @@ -203,6 +222,11 @@ class FuzzedSock : public Sock bool IsConnected(std::string& errmsg) const override; }; +[[nodiscard]] inline FuzzedNetEvents ConsumeNetEvents(FuzzedDataProvider& fdp) noexcept +{ + return FuzzedNetEvents{fdp}; +} + [[nodiscard]] inline FuzzedSock ConsumeSock(FuzzedDataProvider& fuzzed_data_provider) { return FuzzedSock{fuzzed_data_provider}; From 50da7432ec1e5431b243aa30f8a9339f8e8ed97d Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 15 Apr 2021 10:40:45 +0200 Subject: [PATCH 0017/2363] fuzz: add CConnman::OpenNetworkConnection() to the tests Now that all network calls done by `CConnman::OpenNetworkConnection()` are done via `Sock` they can be redirected (mocked) to `FuzzedSocket` for testing. --- src/test/fuzz/connman.cpp | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 8513f07feadb..e65cf5ce58b9 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -52,6 +53,19 @@ FUZZ_TARGET(connman, .init = initialize_connman) } AddrManDeterministic& addr_man{*addr_man_ptr}; auto net_events{ConsumeNetEvents(fuzzed_data_provider)}; + + // Mock CreateSock() to create FuzzedSock. + auto CreateSockOrig = CreateSock; + CreateSock = [&fuzzed_data_provider](int, int, int) { + return std::make_unique(fuzzed_data_provider); + }; + + // Mock g_dns_lookup() to return a fuzzed address. + auto g_dns_lookup_orig = g_dns_lookup; + g_dns_lookup = [&fuzzed_data_provider](const std::string&, bool) { + return std::vector{ConsumeNetAddr(fuzzed_data_provider)}; + }; + ConnmanTestMsg connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addr_man, @@ -66,6 +80,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) connman.Init(options); CNetAddr random_netaddr; + CAddress random_address; CNode random_node = ConsumeNode(fuzzed_data_provider); CSubNet random_subnet; std::string random_string; @@ -81,6 +96,9 @@ FUZZ_TARGET(connman, .init = initialize_connman) [&] { random_netaddr = ConsumeNetAddr(fuzzed_data_provider); }, + [&] { + random_address = ConsumeAddress(fuzzed_data_provider); + }, [&] { random_subnet = ConsumeSubNet(fuzzed_data_provider); }, @@ -145,6 +163,21 @@ FUZZ_TARGET(connman, .init = initialize_connman) }, [&] { connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool()); + }, + [&] { + ConnectionType conn_type{ + fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES)}; + if (conn_type == ConnectionType::INBOUND) { // INBOUND is not allowed + conn_type = ConnectionType::OUTBOUND_FULL_RELAY; + } + + connman.OpenNetworkConnection( + /*addrConnect=*/random_address, + /*fCountFailure=*/fuzzed_data_provider.ConsumeBool(), + /*grant_outbound=*/{}, + /*strDest=*/fuzzed_data_provider.ConsumeBool() ? nullptr : random_string.c_str(), + /*conn_type=*/conn_type, + /*use_v2transport=*/fuzzed_data_provider.ConsumeBool()); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); @@ -164,4 +197,6 @@ FUZZ_TARGET(connman, .init = initialize_connman) (void)connman.ASMapHealthCheck(); connman.ClearTestNodes(); + g_dns_lookup = g_dns_lookup_orig; + CreateSock = CreateSockOrig; } From 91cbf4dbd864b65ba6b107957f087d1d305914b2 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 15 Apr 2021 14:01:52 +0200 Subject: [PATCH 0018/2363] fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests --- src/test/fuzz/connman.cpp | 9 +++++++++ src/test/util/net.h | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index e65cf5ce58b9..ea7ad3c45742 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -178,6 +178,15 @@ FUZZ_TARGET(connman, .init = initialize_connman) /*strDest=*/fuzzed_data_provider.ConsumeBool() ? nullptr : random_string.c_str(), /*conn_type=*/conn_type, /*use_v2transport=*/fuzzed_data_provider.ConsumeBool()); + }, + [&] { + connman.SetNetworkActive(fuzzed_data_provider.ConsumeBool()); + const auto peer = ConsumeAddress(fuzzed_data_provider); + connman.CreateNodeFromAcceptedSocketPublic( + /*sock=*/CreateSock(AF_INET, SOCK_STREAM, IPPROTO_TCP), + /*permissions=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), + /*addr_bind=*/ConsumeAddress(fuzzed_data_provider), + /*addr_peer=*/peer); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); diff --git a/src/test/util/net.h b/src/test/util/net.h index 6bf2bf730075..19264ca5570b 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -68,6 +68,14 @@ struct ConnmanTestMsg : public CConnman { m_nodes.clear(); } + void CreateNodeFromAcceptedSocketPublic(std::unique_ptr sock, + NetPermissionFlags permissions, + const CAddress& addr_bind, + const CAddress& addr_peer) + { + CreateNodeFromAcceptedSocket(std::move(sock), permissions, addr_bind, addr_peer); + } + void Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, From 3265df63a48db187e0d240ce801ee573787fed80 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 15 Apr 2021 16:51:00 +0200 Subject: [PATCH 0019/2363] fuzz: add CConnman::InitBinds() to the tests --- src/test/fuzz/connman.cpp | 19 +++++++++++++++++++ src/test/fuzz/util/net.h | 12 ++++++++++++ src/test/util/net.h | 5 +++++ 3 files changed, 36 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index ea7ad3c45742..461362afe379 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -187,6 +187,25 @@ FUZZ_TARGET(connman, .init = initialize_connman) /*permissions=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), /*addr_bind=*/ConsumeAddress(fuzzed_data_provider), /*addr_peer=*/peer); + }, + [&] { + CConnman::Options options; + + options.vBinds = ConsumeServiceVector(fuzzed_data_provider); + + options.vWhiteBinds = std::vector{ + fuzzed_data_provider.ConsumeIntegralInRange(0, 5)}; + for (auto& wb : options.vWhiteBinds) { + wb.m_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS); + wb.m_service = ConsumeService(fuzzed_data_provider); + } + + options.onion_binds = ConsumeServiceVector(fuzzed_data_provider); + + options.bind_on_any = options.vBinds.empty() && options.vWhiteBinds.empty() && + options.onion_binds.empty(); + + connman.InitBindsPublic(options); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h index 200638441eec..2756e49cba68 100644 --- a/src/test/fuzz/util/net.h +++ b/src/test/fuzz/util/net.h @@ -249,6 +249,18 @@ inline CService ConsumeService(FuzzedDataProvider& fuzzed_data_provider) noexcep return {ConsumeNetAddr(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral()}; } +inline std::vector ConsumeServiceVector(FuzzedDataProvider& fuzzed_data_provider, + size_t max_vector_size = 5) noexcept +{ + std::vector ret; + const size_t size = fuzzed_data_provider.ConsumeIntegralInRange(0, max_vector_size); + ret.reserve(size); + for (size_t i = 0; i < size; ++i) { + ret.emplace_back(ConsumeService(fuzzed_data_provider)); + } + return ret; +} + CAddress ConsumeAddress(FuzzedDataProvider& fuzzed_data_provider) noexcept; template diff --git a/src/test/util/net.h b/src/test/util/net.h index 19264ca5570b..26e5c028d891 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -76,6 +76,11 @@ struct ConnmanTestMsg : public CConnman { CreateNodeFromAcceptedSocket(std::move(sock), permissions, addr_bind, addr_peer); } + bool InitBindsPublic(const CConnman::Options& options) + { + return InitBinds(options); + } + void Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, From 6d9e5d130d2e1d052044e9a72d44cfffb5d3c771 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 30 Apr 2021 17:07:35 +0200 Subject: [PATCH 0020/2363] fuzz: add CConnman::SocketHandler() to the tests --- src/test/fuzz/connman.cpp | 3 +++ src/test/util/net.h | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 461362afe379..ddb56fd804cc 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -206,6 +206,9 @@ FUZZ_TARGET(connman, .init = initialize_connman) options.onion_binds.empty(); connman.InitBindsPublic(options); + }, + [&] { + connman.SocketHandlerPublic(); }); } (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); diff --git a/src/test/util/net.h b/src/test/util/net.h index 26e5c028d891..66d209aed651 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -81,6 +81,11 @@ struct ConnmanTestMsg : public CConnman { return InitBinds(options); } + void SocketHandlerPublic() + { + SocketHandler(); + } + void Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, From 0802398e749c5e16fa7085cd87c91a31bbe043bd Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Wed, 6 Nov 2024 11:12:35 +0100 Subject: [PATCH 0021/2363] fuzz: make it possible to mock (fuzz) CThreadInterrupt * Make the methods of `CThreadInterrupt` virtual and store a pointer to it in `CConnman`, thus making it possible to override with a mocked instance. * Initialize `CConnman::m_interrupt_net` from the constructor, making it possible for callers to supply mocked version. * Introduce `FuzzedThreadInterrupt` and `ConsumeThreadInterrupt()` and use them in `src/test/fuzz/connman.cpp` and `src/test/fuzz/i2p.cpp`. This improves the CPU utilization of the `connman` fuzz test. As a nice side effect, the `std::shared_ptr` used for `CConnman::m_interrupt_net` resolves the possible lifetime issues with it (see the removed comment for that variable). --- src/i2p.cpp | 8 +-- src/i2p.h | 14 ++---- src/net.cpp | 70 +++++++++++++++----------- src/net.h | 15 +++--- src/test/fuzz/connman.cpp | 4 +- src/test/fuzz/i2p.cpp | 9 ++-- src/test/fuzz/util/CMakeLists.txt | 1 + src/test/fuzz/util/threadinterrupt.cpp | 22 ++++++++ src/test/fuzz/util/threadinterrupt.h | 33 ++++++++++++ src/test/i2p_tests.cpp | 12 ++--- src/util/threadinterrupt.cpp | 7 ++- src/util/threadinterrupt.h | 24 +++++++-- 12 files changed, 154 insertions(+), 65 deletions(-) create mode 100644 src/test/fuzz/util/threadinterrupt.cpp create mode 100644 src/test/fuzz/util/threadinterrupt.h diff --git a/src/i2p.cpp b/src/i2p.cpp index 8b48615afebc..80f3bde4cf2e 100644 --- a/src/i2p.cpp +++ b/src/i2p.cpp @@ -119,7 +119,7 @@ namespace sam { Session::Session(const fs::path& private_key_file, const Proxy& control_host, - CThreadInterrupt* interrupt) + std::shared_ptr interrupt) : m_private_key_file{private_key_file}, m_control_host{control_host}, m_interrupt{interrupt}, @@ -127,7 +127,7 @@ Session::Session(const fs::path& private_key_file, { } -Session::Session(const Proxy& control_host, CThreadInterrupt* interrupt) +Session::Session(const Proxy& control_host, std::shared_ptr interrupt) : m_control_host{control_host}, m_interrupt{interrupt}, m_transient{true} @@ -162,7 +162,7 @@ bool Session::Accept(Connection& conn) std::string errmsg; bool disconnect{false}; - while (!*m_interrupt) { + while (!m_interrupt->interrupted()) { Sock::Event occurred; if (!conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred)) { errmsg = "wait on socket failed"; @@ -205,7 +205,7 @@ bool Session::Accept(Connection& conn) return true; } - if (*m_interrupt) { + if (m_interrupt->interrupted()) { LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Accept was interrupted\n"); } else { LogPrintLevel(BCLog::I2P, BCLog::Level::Debug, "Error accepting%s: %s\n", disconnect ? " (will close the session)" : "", errmsg); diff --git a/src/i2p.h b/src/i2p.h index 153263399df0..a41406f380fb 100644 --- a/src/i2p.h +++ b/src/i2p.h @@ -63,13 +63,11 @@ class Session * private key will be generated and saved into the file. * @param[in] control_host Location of the SAM proxy. * @param[in,out] interrupt If this is signaled then all operations are canceled as soon as - * possible and executing methods throw an exception. Notice: only a pointer to the - * `CThreadInterrupt` object is saved, so it must not be destroyed earlier than this - * `Session` object. + * possible and executing methods throw an exception. */ Session(const fs::path& private_key_file, const Proxy& control_host, - CThreadInterrupt* interrupt); + std::shared_ptr interrupt); /** * Construct a transient session which will generate its own I2P private key @@ -78,11 +76,9 @@ class Session * the session will be lazily created later when first used. * @param[in] control_host Location of the SAM proxy. * @param[in,out] interrupt If this is signaled then all operations are canceled as soon as - * possible and executing methods throw an exception. Notice: only a pointer to the - * `CThreadInterrupt` object is saved, so it must not be destroyed earlier than this - * `Session` object. + * possible and executing methods throw an exception. */ - Session(const Proxy& control_host, CThreadInterrupt* interrupt); + Session(const Proxy& control_host, std::shared_ptr interrupt); /** * Destroy the session, closing the internally used sockets. The sockets that have been @@ -235,7 +231,7 @@ class Session /** * Cease network activity when this is signaled. */ - CThreadInterrupt* const m_interrupt; + const std::shared_ptr m_interrupt; /** * Mutex protecting the members that can be concurrently accessed. diff --git a/src/net.cpp b/src/net.cpp index 217d9a890373..85c25543d38d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -471,7 +471,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo LOCK(m_unused_i2p_sessions_mutex); if (m_unused_i2p_sessions.empty()) { i2p_transient_session = - std::make_unique(proxy, &interruptNet); + std::make_unique(proxy, m_interrupt_net); } else { i2p_transient_session.swap(m_unused_i2p_sessions.front()); m_unused_i2p_sessions.pop(); @@ -2094,7 +2094,7 @@ void CConnman::SocketHandler() // empty sets. events_per_sock = GenerateWaitSockets(snap.Nodes()); if (events_per_sock.empty() || !events_per_sock.begin()->first->WaitMany(timeout, events_per_sock)) { - interruptNet.sleep_for(timeout); + m_interrupt_net->sleep_for(timeout); } // Service (send/receive) each of the already connected nodes. @@ -2111,8 +2111,9 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, AssertLockNotHeld(m_total_bytes_sent_mutex); for (CNode* pnode : nodes) { - if (interruptNet) + if (m_interrupt_net->interrupted()) { return; + } // // Receive @@ -2207,7 +2208,7 @@ void CConnman::SocketHandlerConnected(const std::vector& nodes, void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) { for (const ListenSocket& listen_socket : vhListenSocket) { - if (interruptNet) { + if (m_interrupt_net->interrupted()) { return; } const auto it = events_per_sock.find(listen_socket.sock); @@ -2221,8 +2222,7 @@ void CConnman::ThreadSocketHandler() { AssertLockNotHeld(m_total_bytes_sent_mutex); - while (!interruptNet) - { + while (!m_interrupt_net->interrupted()) { DisconnectNodes(); NotifyNumConnectionsChanged(); SocketHandler(); @@ -2246,9 +2246,10 @@ void CConnman::ThreadDNSAddressSeed() auto start = NodeClock::now(); constexpr std::chrono::seconds SEEDNODE_TIMEOUT = 30s; LogPrintf("-seednode enabled. Trying the provided seeds for %d seconds before defaulting to the dnsseeds.\n", SEEDNODE_TIMEOUT.count()); - while (!interruptNet) { - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + while (!m_interrupt_net->interrupted()) { + if (!m_interrupt_net->sleep_for(500ms)) { return; + } // Abort if we have spent enough time without reaching our target. // Giving seed nodes 30 seconds so this does not become a race against fixedseeds (which triggers after 1 min) @@ -2309,7 +2310,7 @@ void CConnman::ThreadDNSAddressSeed() // early to see if we have enough peers and can stop // this thread entirely freeing up its resources std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait); - if (!interruptNet.sleep_for(w)) return; + if (!m_interrupt_net->sleep_for(w)) return; to_wait -= w; if (GetFullOutboundConnCount() >= SEED_OUTBOUND_CONNECTION_THRESHOLD) { @@ -2325,13 +2326,13 @@ void CConnman::ThreadDNSAddressSeed() } } - if (interruptNet) return; + if (m_interrupt_net->interrupted()) return; // hold off on querying seeds if P2P network deactivated if (!fNetworkActive) { LogPrintf("Waiting for network to be reactivated before querying DNS seeds.\n"); do { - if (!interruptNet.sleep_for(std::chrono::seconds{1})) return; + if (!m_interrupt_net->sleep_for(1s)) return; } while (!fNetworkActive); } @@ -2526,12 +2527,14 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std OpenNetworkConnection(addr, false, {}, strAddr.c_str(), ConnectionType::MANUAL, /*use_v2transport=*/use_v2transport); for (int i = 0; i < 10 && i < nLoop; i++) { - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + if (!m_interrupt_net->sleep_for(500ms)) { return; + } } } - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + if (!m_interrupt_net->sleep_for(500ms)) { return; + } PerformReconnections(); } } @@ -2555,8 +2558,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std LogPrintf("Fixed seeds are disabled\n"); } - while (!interruptNet) - { + while (!m_interrupt_net->interrupted()) { if (add_addr_fetch) { add_addr_fetch = false; const auto& seed{SpanPopBack(seed_nodes)}; @@ -2571,14 +2573,16 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std ProcessAddrFetch(); - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) + if (!m_interrupt_net->sleep_for(500ms)) { return; + } PerformReconnections(); CountingSemaphoreGrant<> grant(*semOutbound); - if (interruptNet) + if (m_interrupt_net->interrupted()) { return; + } const std::unordered_set fixed_seed_networks{GetReachableEmptyNetworks()}; if (add_fixed_seeds && !fixed_seed_networks.empty()) { @@ -2752,8 +2756,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std int nTries = 0; const auto reachable_nets{g_reachable_nets.All()}; - while (!interruptNet) - { + while (!m_interrupt_net->interrupted()) { if (anchor && !m_anchors.empty()) { const CAddress addr = m_anchors.back(); m_anchors.pop_back(); @@ -2855,7 +2858,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, std if (addrConnect.IsValid()) { if (fFeeler) { // Add small amount of random noise before connection to avoid synchronization. - if (!interruptNet.sleep_for(rng.rand_uniform_duration(FEELER_SLEEP_WINDOW))) { + if (!m_interrupt_net->sleep_for(rng.rand_uniform_duration(FEELER_SLEEP_WINDOW))) { return; } LogDebug(BCLog::NET, "Making feeler connection to %s\n", addrConnect.ToStringAddrPort()); @@ -2966,14 +2969,15 @@ void CConnman::ThreadOpenAddedConnections() tried = true; CAddress addr(CService(), NODE_NONE); OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport); - if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; + if (!m_interrupt_net->sleep_for(500ms)) return; grant = CountingSemaphoreGrant<>(*semAddnode, /*fTry=*/true); } // See if any reconnections are desired. PerformReconnections(); // Retry every 60 seconds if a connection was attempted, otherwise two seconds - if (!interruptNet.sleep_for(std::chrono::seconds(tried ? 60 : 2))) + if (!m_interrupt_net->sleep_for(tried ? 60s : 2s)) { return; + } } } @@ -2986,7 +2990,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai // // Initiate outbound network connection // - if (interruptNet) { + if (m_interrupt_net->interrupted()) { return; } if (!fNetworkActive) { @@ -3074,13 +3078,13 @@ void CConnman::ThreadI2PAcceptIncoming() i2p::Connection conn; auto SleepOnFailure = [&]() { - interruptNet.sleep_for(err_wait); + m_interrupt_net->sleep_for(err_wait); if (err_wait < err_wait_cap) { err_wait += 1s; } }; - while (!interruptNet) { + while (!m_interrupt_net->interrupted()) { if (!m_i2p_sam_session->Listen(conn)) { if (advertising_listen_addr && conn.me.IsValid()) { @@ -3202,12 +3206,18 @@ void CConnman::SetNetworkActive(bool active) } } -CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in, - const NetGroupManager& netgroupman, const CChainParams& params, bool network_active) +CConnman::CConnman(uint64_t nSeed0In, + uint64_t nSeed1In, + AddrMan& addrman_in, + const NetGroupManager& netgroupman, + const CChainParams& params, + bool network_active, + std::shared_ptr interrupt_net) : addrman(addrman_in) , m_netgroupman{netgroupman} , nSeed0(nSeed0In) , nSeed1(nSeed1In) + , m_interrupt_net{interrupt_net} , m_params(params) { SetTryNewOutboundPeer(false); @@ -3303,7 +3313,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) Proxy i2p_sam; if (GetProxy(NET_I2P, i2p_sam) && connOptions.m_i2p_accept_incoming) { m_i2p_sam_session = std::make_unique(gArgs.GetDataDirNet() / "i2p_private_key", - i2p_sam, &interruptNet); + i2p_sam, m_interrupt_net); } // Randomize the order in which we may query seednode to potentially prevent connecting to the same one every restart (and signal that we have restarted) @@ -3340,7 +3350,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) // Start threads // assert(m_msgproc); - interruptNet.reset(); + m_interrupt_net->reset(); flagInterruptMsgProc = false; { @@ -3416,7 +3426,7 @@ void CConnman::Interrupt() } condMsgProc.notify_all(); - interruptNet(); + (*m_interrupt_net)(); g_socks5_interrupt(); if (semOutbound) { diff --git a/src/net.h b/src/net.h index 4cb4cb906e2a..66a830da905e 100644 --- a/src/net.h +++ b/src/net.h @@ -1118,8 +1118,13 @@ class CConnman whitelist_relay = connOptions.whitelist_relay; } - CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, const NetGroupManager& netgroupman, - const CChainParams& params, bool network_active = true); + CConnman(uint64_t seed0, + uint64_t seed1, + AddrMan& addrman, + const NetGroupManager& netgroupman, + const CChainParams& params, + bool network_active = true, + std::shared_ptr interrupt_net = std::make_shared()); ~CConnman(); @@ -1543,11 +1548,9 @@ class CConnman /** * This is signaled when network activity should cease. - * A pointer to it is saved in `m_i2p_sam_session`, so make sure that - * the lifetime of `interruptNet` is not shorter than - * the lifetime of `m_i2p_sam_session`. + * A copy of this is saved in `m_i2p_sam_session`. */ - CThreadInterrupt interruptNet; + const std::shared_ptr m_interrupt_net; /** * I2P SAM session. diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index ddb56fd804cc..56034185b364 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -71,7 +72,8 @@ FUZZ_TARGET(connman, .init = initialize_connman) addr_man, netgroupman, Params(), - fuzzed_data_provider.ConsumeBool()}; + fuzzed_data_provider.ConsumeBool(), + ConsumeThreadInterrupt(fuzzed_data_provider)}; const uint64_t max_outbound_limit{fuzzed_data_provider.ConsumeIntegral()}; CConnman::Options options; diff --git a/src/test/fuzz/i2p.cpp b/src/test/fuzz/i2p.cpp index 29a11123d9a2..64f30719c582 100644 --- a/src/test/fuzz/i2p.cpp +++ b/src/test/fuzz/i2p.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -35,15 +36,15 @@ FUZZ_TARGET(i2p, .init = initialize_i2p) const fs::path private_key_path = gArgs.GetDataDirNet() / "fuzzed_i2p_private_key"; const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), 7656}; const Proxy sam_proxy{addr, /*tor_stream_isolation=*/false}; - CThreadInterrupt interrupt; + auto interrupt{ConsumeThreadInterrupt(fuzzed_data_provider)}; - i2p::sam::Session session{private_key_path, sam_proxy, &interrupt}; + i2p::sam::Session session{private_key_path, sam_proxy, interrupt}; i2p::Connection conn; if (session.Listen(conn)) { if (session.Accept(conn)) { try { - (void)conn.sock->RecvUntilTerminator('\n', 10ms, interrupt, i2p::sam::MAX_MSG_SIZE); + (void)conn.sock->RecvUntilTerminator('\n', 10ms, *interrupt, i2p::sam::MAX_MSG_SIZE); } catch (const std::runtime_error&) { } } @@ -53,7 +54,7 @@ FUZZ_TARGET(i2p, .init = initialize_i2p) if (session.Connect(CService{}, conn, proxy_error)) { try { - conn.sock->SendComplete("verack\n", 10ms, interrupt); + conn.sock->SendComplete("verack\n", 10ms, *interrupt); } catch (const std::runtime_error&) { } } diff --git a/src/test/fuzz/util/CMakeLists.txt b/src/test/fuzz/util/CMakeLists.txt index 878286b0f44f..282de3d708f0 100644 --- a/src/test/fuzz/util/CMakeLists.txt +++ b/src/test/fuzz/util/CMakeLists.txt @@ -7,6 +7,7 @@ add_library(test_fuzz STATIC EXCLUDE_FROM_ALL descriptor.cpp mempool.cpp net.cpp + threadinterrupt.cpp ../fuzz.cpp ../util.cpp ) diff --git a/src/test/fuzz/util/threadinterrupt.cpp b/src/test/fuzz/util/threadinterrupt.cpp new file mode 100644 index 000000000000..5dd87e05888e --- /dev/null +++ b/src/test/fuzz/util/threadinterrupt.cpp @@ -0,0 +1,22 @@ +// Copyright (c) 2024-present 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 + +FuzzedThreadInterrupt::FuzzedThreadInterrupt(FuzzedDataProvider& fuzzed_data_provider) + : m_fuzzed_data_provider{fuzzed_data_provider} +{ +} + +bool FuzzedThreadInterrupt::interrupted() const +{ + return m_fuzzed_data_provider.ConsumeBool(); +} + +bool FuzzedThreadInterrupt::sleep_for(Clock::duration) +{ + SetMockTime(ConsumeTime(m_fuzzed_data_provider)); // Time could go backwards. + return m_fuzzed_data_provider.ConsumeBool(); +} diff --git a/src/test/fuzz/util/threadinterrupt.h b/src/test/fuzz/util/threadinterrupt.h new file mode 100644 index 000000000000..d56aefd919fc --- /dev/null +++ b/src/test/fuzz/util/threadinterrupt.h @@ -0,0 +1,33 @@ +// Copyright (c) 2024-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_TEST_FUZZ_UTIL_THREADINTERRUPT_H +#define BITCOIN_TEST_FUZZ_UTIL_THREADINTERRUPT_H + +#include +#include + +#include + +/** + * Mocked CThreadInterrupt that returns "randomly" whether it is interrupted and never sleeps. + */ +class FuzzedThreadInterrupt : public CThreadInterrupt +{ +public: + explicit FuzzedThreadInterrupt(FuzzedDataProvider& fuzzed_data_provider); + + virtual bool interrupted() const override; + virtual bool sleep_for(Clock::duration) override; + +private: + FuzzedDataProvider& m_fuzzed_data_provider; +}; + +[[nodiscard]] inline std::shared_ptr ConsumeThreadInterrupt(FuzzedDataProvider& fuzzed_data_provider) +{ + return std::make_shared(fuzzed_data_provider); +} + +#endif // BITCOIN_TEST_FUZZ_UTIL_THREADINTERRUPT_H diff --git a/src/test/i2p_tests.cpp b/src/test/i2p_tests.cpp index bdb3408b66cf..da9cbb5d228b 100644 --- a/src/test/i2p_tests.cpp +++ b/src/test/i2p_tests.cpp @@ -50,10 +50,10 @@ BOOST_AUTO_TEST_CASE(unlimited_recv) return std::make_unique(std::string(i2p::sam::MAX_MSG_SIZE + 1, 'a')); }; - CThreadInterrupt interrupt; + auto interrupt{std::make_shared()}; const std::optional addr{Lookup("127.0.0.1", 9000, false)}; const Proxy sam_proxy(addr.value(), /*tor_stream_isolation=*/false); - i2p::sam::Session session(gArgs.GetDataDirNet() / "test_i2p_private_key", sam_proxy, &interrupt); + i2p::sam::Session session(gArgs.GetDataDirNet() / "test_i2p_private_key", sam_proxy, interrupt); { ASSERT_DEBUG_LOG("Creating persistent SAM session"); @@ -112,12 +112,12 @@ BOOST_AUTO_TEST_CASE(listen_ok_accept_fail) // clang-format on }; - CThreadInterrupt interrupt; + auto interrupt{std::make_shared()}; const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), /*port=*/7656}; const Proxy sam_proxy(addr, /*tor_stream_isolation=*/false); i2p::sam::Session session(gArgs.GetDataDirNet() / "test_i2p_private_key", sam_proxy, - &interrupt); + interrupt); i2p::Connection conn; for (size_t i = 0; i < 5; ++i) { @@ -155,10 +155,10 @@ BOOST_AUTO_TEST_CASE(damaged_private_key) "391 bytes"}}) { BOOST_REQUIRE(WriteBinaryFile(i2p_private_key_file, file_contents)); - CThreadInterrupt interrupt; + auto interrupt{std::make_shared()}; const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), /*port=*/7656}; const Proxy sam_proxy{addr, /*tor_stream_isolation=*/false}; - i2p::sam::Session session(i2p_private_key_file, sam_proxy, &interrupt); + i2p::sam::Session session(i2p_private_key_file, sam_proxy, interrupt); { ASSERT_DEBUG_LOG("Creating persistent SAM session"); diff --git a/src/util/threadinterrupt.cpp b/src/util/threadinterrupt.cpp index 3ea406d4a8ef..aaa9e831a912 100644 --- a/src/util/threadinterrupt.cpp +++ b/src/util/threadinterrupt.cpp @@ -9,11 +9,16 @@ CThreadInterrupt::CThreadInterrupt() : flag(false) {} -CThreadInterrupt::operator bool() const +bool CThreadInterrupt::interrupted() const { return flag.load(std::memory_order_acquire); } +CThreadInterrupt::operator bool() const +{ + return interrupted(); +} + void CThreadInterrupt::reset() { flag.store(false, std::memory_order_release); diff --git a/src/util/threadinterrupt.h b/src/util/threadinterrupt.h index 0b79b3827632..8b393c26dfce 100644 --- a/src/util/threadinterrupt.h +++ b/src/util/threadinterrupt.h @@ -27,11 +27,27 @@ class CThreadInterrupt { public: using Clock = std::chrono::steady_clock; + CThreadInterrupt(); - explicit operator bool() const; - void operator()() EXCLUSIVE_LOCKS_REQUIRED(!mut); - void reset(); - bool sleep_for(Clock::duration rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut); + + virtual ~CThreadInterrupt() = default; + + /// Return true if `operator()()` has been called. + virtual bool interrupted() const; + + /// An alias for `interrupted()`. + virtual explicit operator bool() const; + + /// Interrupt any sleeps. After this `interrupted()` will return `true`. + virtual void operator()() EXCLUSIVE_LOCKS_REQUIRED(!mut); + + /// Reset to an non-interrupted state. + virtual void reset(); + + /// Sleep for the given duration. + /// @retval true The time passed. + /// @retval false The sleep was interrupted. + virtual bool sleep_for(Clock::duration rel_time) EXCLUSIVE_LOCKS_REQUIRED(!mut); private: std::condition_variable cond; From 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 8 Jul 2025 19:22:27 +0200 Subject: [PATCH 0022/2363] test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded) see https://mempool.space/testnet/tx/c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433 and https://bitcointalk.org/index.php?topic=1729534.msg17309060#msg17309060 --- src/test/data/tx_valid.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/data/tx_valid.json b/src/test/data/tx_valid.json index d7c367b835cb..ac25f8149b4b 100644 --- a/src/test/data/tx_valid.json +++ b/src/test/data/tx_valid.json @@ -31,6 +31,10 @@ [[["60a20bd93aa49ab4b28d514ec10b06e1829ce6818ec06cd3aabd013ebcdc4bb1", 0, "1 0x41 0x04cc71eb30d653c0c3163990c47b976f3fb3f37cccdcbedb169a1dfef58bbfbfaff7d8a473e7e2e6d317b87bafe8bde97e3cf8f065dec022b51d11fcdd0d348ac4 0x41 0x0461cbdcc5409fb4b4d42b51d33381354d80e550078cb532a34bfa2fcfdeb7d76519aecc62770f5b0e4ef8551946d8a540911abe3e7854a26f39f58b25c15342af 2 OP_CHECKMULTISIG"]], "0100000001b14bdcbc3e01bdaad36cc08e81e69c82e1060bc14e518db2b49aa43ad90ba26000000000494f47304402203f16c6f40162ab686621ef3000b04e75418a0c0cb2d8aebeac894ae360ac1e780220ddc15ecdfc3507ac48e1681a33eb60996631bf6bf5bc0a0682c4db743ce7ca2b01ffffffff0140420f00000000001976a914660d4ef3a743e3e696ad990364e555c271ad504b88ac00000000", "DERSIG,LOW_S,STRICTENC,NULLDUMMY"], +["The following is c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433 on testnet3"], +["It contains an OP_CHECKSIG with the shortest valid DER encoded signature (8 bytes w/o sighash flag), i.e. 3006020101020101 (r=1, s=1)"], [[["cf016927962ec028964c186043d48e465b3d4672f758953b00d3c4682f71cad6", 0, "HASH160 0x14 0x58a994e9d5ed9baa03ecfd1137592a90ad3cdfc5 EQUAL"]], +"0100000001d6ca712f68c4d3003b9558f772463d5b468ed44360184c9628c02e96276901cf000000002f21026d2204a9535443657a88a0724fbd49a0e78d305f50a82f2cc9dd9bea10a6c5cd0c093006020101020101017cacffffffff010000000000000000016a00000000", "CONST_SCRIPTCODE"], + ["The following is c99c49da4c38af669dea436d3e73780dfdb6c1ecf9958baa52960e8baee30e73"], ["It is of interest because it contains a 0-sequence as well as a signature of SIGHASH type 0 (which is not a real type)"], [[["406b2b06bcd34d3c8733e6b79f7a394c8a431fbf4ff5ac705c93f4076bb77602", 0, "DUP HASH160 0x14 0xdc44b1164188067c3a32d4780f5996fa14a4f2d9 EQUALVERIFY CHECKSIG"]], From 88d909257104124bee3fc810f7fa32e5802b92fe Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 18 Jun 2025 20:40:52 +0100 Subject: [PATCH 0023/2363] cmake: Create subdirectories in build tree in advance This change ensures that subsequent symlink creation does not fail due to a missing path. --- test/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0b6520a08109..515c24f90942 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -33,6 +33,10 @@ endfunction() create_test_config() file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/functional) +file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/functional/data) +file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/functional/mocks) +file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/functional/test_framework) +file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/functional/test_framework/crypto) file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/fuzz) file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/util) From 8810642b571e1d8482375e962a1129b691d5d226 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Wed, 16 Jul 2025 12:53:29 -0300 Subject: [PATCH 0024/2363] test: add option to skip large re-org test in feature_block --- test/functional/feature_block.py | 93 +++++++++++++++++--------------- 1 file changed, 49 insertions(+), 44 deletions(-) diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 0e2e685247fe..4f36a673a329 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -93,6 +93,9 @@ def set_test_params(self): '-testactivationheight=bip34@2', ]] + def add_options(self, parser): + parser.add_argument("--skipreorg", action='store_true', dest="skip_reorg", help="Skip the large re-org test", default=False) + def run_test(self): node = self.nodes[0] # convenience reference to the node @@ -1278,61 +1281,63 @@ def run_test(self): b89a = self.update_block("89a", [tx]) self.send_blocks([b89a], success=False, reject_reason='bad-txns-inputs-missingorspent', reconnect=True) - # Don't use v2transport for the large reorg, which is too slow with the unoptimized python ChaCha20 implementation - if self.options.v2transport: - self.nodes[0].disconnect_p2ps() - self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore(), supports_v2_p2p=False) - self.log.info("Test a re-org of one week's worth of blocks (1088 blocks)") - - self.move_tip(88) - LARGE_REORG_SIZE = 1088 - blocks = [] - spend = out[32] - for i in range(89, LARGE_REORG_SIZE + 89): - b = self.next_block(i, spend) - tx = CTransaction() - script_length = (MAX_BLOCK_WEIGHT - b.get_weight() - 276) // 4 - script_output = CScript([b'\x00' * script_length]) - tx.vout.append(CTxOut(0, script_output)) - tx.vin.append(CTxIn(COutPoint(b.vtx[1].txid_int, 0))) - b = self.update_block(i, [tx]) - assert_equal(b.get_weight(), MAX_BLOCK_WEIGHT) - blocks.append(b) - self.save_spendable_output() - spend = self.get_spendable_output() - - self.send_blocks(blocks, True, timeout=2440) - chain1_tip = i - - # now create alt chain of same length self.move_tip(88) - blocks2 = [] - for i in range(89, LARGE_REORG_SIZE + 89): - blocks2.append(self.next_block("alt" + str(i))) - self.send_blocks(blocks2, False, force_send=False) - - # extend alt chain to trigger re-org - block = self.next_block("alt" + str(chain1_tip + 1)) - self.send_blocks([block], True, timeout=2440) - - # ... and re-org back to the first chain - self.move_tip(chain1_tip) - block = self.next_block(chain1_tip + 1) - self.send_blocks([block], False, force_send=True) - block = self.next_block(chain1_tip + 2) - self.send_blocks([block], True, timeout=2440) - self.log.info("Reject a block with an invalid block header version") b_v1 = self.next_block('b_v1', version=1) self.send_blocks([b_v1], success=False, force_send=True, reject_reason='bad-version(0x00000001)', reconnect=True) - self.move_tip(chain1_tip + 2) + self.move_tip(87) b_cb34 = self.next_block('b_cb34') b_cb34.vtx[0].vin[0].scriptSig = b_cb34.vtx[0].vin[0].scriptSig[:-1] b_cb34.hashMerkleRoot = b_cb34.calc_merkle_root() b_cb34.solve() self.send_blocks([b_cb34], success=False, reject_reason='bad-cb-height', reconnect=True) + # Don't use v2transport for the large reorg, which is too slow with the unoptimized python ChaCha20 implementation + if self.options.v2transport: + self.nodes[0].disconnect_p2ps() + self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore(), supports_v2_p2p=False) + + self.move_tip(88) + if not self.options.skip_reorg: + self.log.info("Test a re-org of one week's worth of blocks (1088 blocks)") + LARGE_REORG_SIZE = 1088 + blocks = [] + spend = out[32] + for i in range(89, LARGE_REORG_SIZE + 89): + b = self.next_block(i, spend) + tx = CTransaction() + script_length = (MAX_BLOCK_WEIGHT - b.get_weight() - 276) // 4 + script_output = CScript([b'\x00' * script_length]) + tx.vout.append(CTxOut(0, script_output)) + tx.vin.append(CTxIn(COutPoint(b.vtx[1].txid_int, 0))) + b = self.update_block(i, [tx]) + assert_equal(b.get_weight(), MAX_BLOCK_WEIGHT) + blocks.append(b) + self.save_spendable_output() + spend = self.get_spendable_output() + + self.send_blocks(blocks, True, timeout=2440) + chain1_tip = i + + # now create alt chain of same length + self.move_tip(88) + blocks2 = [] + for i in range(89, LARGE_REORG_SIZE + 89): + blocks2.append(self.next_block("alt" + str(i))) + self.send_blocks(blocks2, False, force_send=False) + + # extend alt chain to trigger re-org + block = self.next_block("alt" + str(chain1_tip + 1)) + self.send_blocks([block], True, timeout=2440) + + # ... and re-org back to the first chain + self.move_tip(chain1_tip) + block = self.next_block(chain1_tip + 1) + self.send_blocks([block], False, force_send=True) + block = self.next_block(chain1_tip + 2) + self.send_blocks([block], True, timeout=2440) + # Helper methods ################ From af041c405756d3b8bb04cb2ebd8c32cf237ac2a9 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 15 Jul 2025 16:55:46 -0700 Subject: [PATCH 0025/2363] wallet: Always rewrite tx records during migration Since loading a wallet may change some parts of tx records (e.g. adding nOrderPos), we should rewrite the records instead of copying them so that the automatic upgrade does not need to be performed again when the wallet is loaded. --- src/wallet/wallet.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3d83f356f641..de7e7e167785 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3947,14 +3947,16 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, // Mark as to remove from the migrated wallet only if it does not also belong to it if (!is_mine) { txids_to_delete.push_back(hash); + continue; } - continue; } } if (!is_mine) { // Both not ours and not in the watchonly wallet return util::Error{strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex())}; } + // Rewrite the transaction so that anything that may have changed about it in memory also persists to disk + local_wallet_batch.WriteTx(*wtx); } // Do the removes From faeb58fe668662d8262c4cc7c54ad2af756dbe3b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 22 May 2025 15:09:53 +0200 Subject: [PATCH 0026/2363] refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD This does not change behavior, but documents that G_ABORT_ON_FAILED_ASSUME is set when G_FUZZING_BUILD. --- src/util/check.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/check.h b/src/util/check.h index f7dea5a3c2e7..198e9e078b2e 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -21,7 +21,7 @@ constexpr bool G_FUZZING_BUILD{ false #endif }; -constexpr bool G_ABORT_ON_FAILED_ASSUME{ +constexpr bool G_ABORT_ON_FAILED_ASSUME{G_FUZZING_BUILD || #ifdef ABORT_ON_FAILED_ASSUME true #else @@ -75,7 +75,7 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std: template constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion) { - if (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING_BUILD || G_ABORT_ON_FAILED_ASSUME) { + if (IS_ASSERT || std::is_constant_evaluated() || G_ABORT_ON_FAILED_ASSUME) { if (!val) { assertion_fail(file, line, func, assertion); } From fa0dc4bdffb06b6f0c192fe1aa02b4dfdcdc6e15 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 23 May 2025 08:47:35 +0200 Subject: [PATCH 0027/2363] test: Allow testing of check failures This allows specific tests to mock the check behavior to consistently use exceptions instead of aborts for intentionally failing checks in all build configurations. --- src/test/CMakeLists.txt | 1 + src/test/util_check_tests.cpp | 33 +++++++++++++++++++++++++++++++++ src/util/check.cpp | 5 +++++ src/util/check.h | 6 ++++++ 4 files changed, 45 insertions(+) create mode 100644 src/test/util_check_tests.cpp diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 93db34d5a215..fa0e87911805 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -114,6 +114,7 @@ add_executable(test_bitcoin txvalidation_tests.cpp txvalidationcache_tests.cpp uint256_tests.cpp + util_check_tests.cpp util_string_tests.cpp util_tests.cpp util_threadnames_tests.cpp diff --git a/src/test/util_check_tests.cpp b/src/test/util_check_tests.cpp new file mode 100644 index 000000000000..93ac91946046 --- /dev/null +++ b/src/test/util_check_tests.cpp @@ -0,0 +1,33 @@ +// Copyright (c) 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 + +BOOST_AUTO_TEST_SUITE(util_check_tests) + +BOOST_AUTO_TEST_CASE(check_pass) +{ + Assume(true); + Assert(true); + CHECK_NONFATAL(true); +} + +BOOST_AUTO_TEST_CASE(check_fail) +{ + // Disable aborts for easier testing here + test_only_CheckFailuresAreExceptionsNotAborts mock_checks{}; + + if constexpr (G_ABORT_ON_FAILED_ASSUME) { + BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: false"}); + } else { + BOOST_CHECK_NO_THROW(Assume(false)); + } + BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: false"}); + BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: false"}); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/check.cpp b/src/util/check.cpp index 9f07fbe4780f..77783c843d78 100644 --- a/src/util/check.cpp +++ b/src/util/check.cpp @@ -27,8 +27,13 @@ NonFatalCheckError::NonFatalCheckError(std::string_view msg, std::string_view fi { } +bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts{false}; + void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion) { + if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) { + throw NonFatalCheckError{assertion, file, line, func}; + } auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion); fwrite(str.data(), 1, str.size(), stderr); std::abort(); diff --git a/src/util/check.h b/src/util/check.h index 198e9e078b2e..6b4ec11f1c87 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -46,6 +46,12 @@ inline bool EnableFuzzDeterminism() } } +extern bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts; +struct test_only_CheckFailuresAreExceptionsNotAborts { + test_only_CheckFailuresAreExceptionsNotAborts() { g_detail_test_only_CheckFailuresAreExceptionsNotAborts = true; }; + ~test_only_CheckFailuresAreExceptionsNotAborts() { g_detail_test_only_CheckFailuresAreExceptionsNotAborts = false; }; +}; + std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func); class NonFatalCheckError : public std::runtime_error From fa37153288ca420420636046ef6b8c4ba7e5a478 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 22 May 2025 16:24:45 +0200 Subject: [PATCH 0028/2363] util: Abort on failing CHECK_NONFATAL in debug builds This requires adjusting some tests to force exceptions over aborts, or accept either exceptions or aborts. Also, remove a fuzz test in integer.cpp that is mostly redundant with the unit test added in the prior commit. --- src/test/fuzz/integer.cpp | 5 ----- src/test/fuzz/rpc.cpp | 6 ++++++ src/test/rpc_tests.cpp | 31 +++++++++++++++++-------------- src/util/check.h | 9 ++++++--- test/functional/rpc_misc.py | 24 +++++++++++++++++++----- 5 files changed, 48 insertions(+), 27 deletions(-) diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp index a6729155d1f5..f40a2be1e852 100644 --- a/src/test/fuzz/integer.cpp +++ b/src/test/fuzz/integer.cpp @@ -255,9 +255,4 @@ FUZZ_TARGET(integer, .init = initialize_integer) } catch (const std::ios_base::failure&) { } } - - try { - CHECK_NONFATAL(b); - } catch (const NonFatalCheckError&) { - } } diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 580a6338a849..c8f5e87adfa1 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -381,6 +381,12 @@ FUZZ_TARGET(rpc, .init = initialize_rpc) arguments.push_back(ConsumeRPCArgument(fuzzed_data_provider, good_data)); } try { + std::optional maybe_mock{}; + if (rpc_command == "echo") { + // Avoid aborting fuzzing for this specific test-only RPC with an + // intentional trigger_internal_bug + maybe_mock.emplace(); + } rpc_testing_setup->CallRPC(rpc_command, arguments); } catch (const UniValue& json_rpc_error) { const std::string error_msg{json_rpc_error.find_value("message").get_str()}; diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 87303d741749..5d81b4287dd8 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -534,20 +534,23 @@ BOOST_AUTO_TEST_CASE(check_dup_param_names) make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED}}); make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED_ONLY}}); - // Error if parameters names are duplicates, unless one parameter is - // positional and the other is named and .also_positional is true. - BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError); - make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}}); - BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError); - make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}}); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError); - - // Make sure duplicate aliases are detected too. - BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError); + { + test_only_CheckFailuresAreExceptionsNotAborts mock_checks{}; + // Error if parameter names are duplicates, unless one parameter is + // positional and the other is named and .also_positional is true. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError); + make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}}); + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}}); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + + // Make sure duplicate aliases are detected too. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError); + } } BOOST_AUTO_TEST_CASE(help_example) diff --git a/src/util/check.h b/src/util/check.h index 6b4ec11f1c87..17318e6525a0 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -60,11 +60,17 @@ class NonFatalCheckError : public std::runtime_error NonFatalCheckError(std::string_view msg, std::string_view file, int line, std::string_view func); }; +/** Internal helper */ +void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion); + /** Helper for CHECK_NONFATAL() */ template T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, const char* func, const char* assertion) { if (!val) { + if constexpr (G_ABORT_ON_FAILED_ASSUME) { + assertion_fail(file, line, func, assertion); + } throw NonFatalCheckError{assertion, file, line, func}; } return std::forward(val); @@ -74,9 +80,6 @@ T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, co #error "Cannot compile without assertions!" #endif -/** Helper for Assert() */ -void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion); - /** Helper for Assert()/Assume() */ template constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion) diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py index 3c7cce2f168b..fe9f20b6ae53 100755 --- a/test/functional/rpc_misc.py +++ b/test/functional/rpc_misc.py @@ -15,6 +15,9 @@ from test_framework.authproxy import JSONRPCException +import http +import subprocess + class RpcMiscTest(BitcoinTestFramework): def set_test_params(self): @@ -24,11 +27,22 @@ def run_test(self): node = self.nodes[0] self.log.info("test CHECK_NONFATAL") - assert_raises_rpc_error( - -1, - 'Internal bug detected: request.params[9].get_str() != "trigger_internal_bug"', - lambda: node.echo(arg9='trigger_internal_bug'), - ) + msg_internal_bug = 'request.params[9].get_str() != "trigger_internal_bug"' + self.restart_node(0) # Required to flush the chainstate + try: + node.echo(arg9="trigger_internal_bug") + assert False # Must hit one of the exceptions below + except ( + subprocess.CalledProcessError, + http.client.CannotSendRequest, + http.client.RemoteDisconnected, + ): + self.log.info("Restart node after crash") + assert_equal(-6, node.process.wait(timeout=10)) + self.start_node(0) + except JSONRPCException as e: + assert_equal(e.error["code"], -1) + assert f"Internal bug detected: {msg_internal_bug}" in e.error["message"] self.log.info("test getmemoryinfo") memory = node.getmemoryinfo()['locked'] From ab145cb3b471d07a2e8ee79edde46ec67f47d580 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 12 Mar 2024 16:04:20 -0400 Subject: [PATCH 0029/2363] Updates CBlockIndexWorkComparator outdated comment --- src/node/blockstorage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 3c01ff2e8d62..81b55c2440f3 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -161,7 +161,7 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIn if (pa->nChainWork > pb->nChainWork) return false; if (pa->nChainWork < pb->nChainWork) return true; - // ... then by earliest time received, ... + // ... then by earliest activatable time, ... if (pa->nSequenceId < pb->nSequenceId) return false; if (pa->nSequenceId > pb->nSequenceId) return true; From 5370bed21e0b04feca6ec09738ecbe792095a338 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 19 Jan 2024 16:02:57 -0500 Subject: [PATCH 0030/2363] test: add functional test for complex reorgs --- test/functional/feature_chain_tiebreaks.py | 103 +++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 104 insertions(+) create mode 100755 test/functional/feature_chain_tiebreaks.py diff --git a/test/functional/feature_chain_tiebreaks.py b/test/functional/feature_chain_tiebreaks.py new file mode 100755 index 000000000000..cb1f9ebec9c3 --- /dev/null +++ b/test/functional/feature_chain_tiebreaks.py @@ -0,0 +1,103 @@ +#!/usr/bin/env python3 +# Copyright (c) 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 the correct active block is chosen in complex reorgs.""" + +from test_framework.blocktools import create_block +from test_framework.messages import CBlockHeader +from test_framework.p2p import P2PDataStore +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + +class ChainTiebreaksTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.setup_clean_chain = True + + @staticmethod + def send_headers(node, blocks): + """Submit headers for blocks to node.""" + for block in blocks: + # Use RPC rather than P2P, to prevent the message from being interpreted as a block + # announcement. + node.submitheader(hexdata=CBlockHeader(block).serialize().hex()) + + def run_test(self): + node = self.nodes[0] + # Add P2P connection to bitcoind + peer = node.add_p2p_connection(P2PDataStore()) + + self.log.info('Precomputing blocks') + # + # /- B3 -- B7 + # B1 \- B8 + # / \ + # / \ B4 -- B9 + # B0 \- B10 + # \ + # \ /- B5 + # B2 + # \- B6 + # + blocks = [] + + # Construct B0, building off genesis. + start_height = node.getblockcount() + blocks.append(create_block( + hashprev=int(node.getbestblockhash(), 16), + tmpl={"height": start_height + 1} + )) + blocks[-1].solve() + + # Construct B1-B10. + for i in range(1, 11): + blocks.append(create_block( + hashprev=blocks[(i - 1) >> 1].hash_int, + tmpl={ + "height": start_height + (i + 1).bit_length(), + # Make sure each block has a different hash. + "curtime": blocks[-1].nTime + 1, + } + )) + blocks[-1].solve() + + self.log.info('Make sure B0 is accepted normally') + peer.send_blocks_and_test([blocks[0]], node, success=True) + # B0 must be active chain now. + assert_equal(node.getbestblockhash(), blocks[0].hash_hex) + + self.log.info('Send B1 and B2 headers, and then blocks in opposite order') + self.send_headers(node, blocks[1:3]) + peer.send_blocks_and_test([blocks[2]], node, success=True) + peer.send_blocks_and_test([blocks[1]], node, success=False) + # B2 must be active chain now, as full data for B2 was received first. + assert_equal(node.getbestblockhash(), blocks[2].hash_hex) + + self.log.info('Send all further headers in order') + self.send_headers(node, blocks[3:]) + # B2 is still the active chain, headers don't change this. + assert_equal(node.getbestblockhash(), blocks[2].hash_hex) + + self.log.info('Send blocks B7-B10') + peer.send_blocks_and_test([blocks[7]], node, success=False) + peer.send_blocks_and_test([blocks[8]], node, success=False) + peer.send_blocks_and_test([blocks[9]], node, success=False) + peer.send_blocks_and_test([blocks[10]], node, success=False) + # B2 is still the active chain, as B7-B10 have missing parents. + assert_equal(node.getbestblockhash(), blocks[2].hash_hex) + + self.log.info('Send parents B3-B4 of B8-B10 in reverse order') + peer.send_blocks_and_test([blocks[4]], node, success=False, force_send=True) + peer.send_blocks_and_test([blocks[3]], node, success=False, force_send=True) + # B9 is now active. Despite B7 being received earlier, the missing parent. + assert_equal(node.getbestblockhash(), blocks[9].hash_hex) + + self.log.info('Invalidate B9-B10') + node.invalidateblock(blocks[9].hash_hex) + node.invalidateblock(blocks[10].hash_hex) + # B7 is now active. + assert_equal(node.getbestblockhash(), blocks[7].hash_hex) + +if __name__ == '__main__': + ChainTiebreaksTest(__file__).main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 44c2885bdf89..3cf3e243600f 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -320,6 +320,7 @@ 'feature_includeconf.py', 'feature_addrman.py', 'feature_asmap.py', + 'feature_chain_tiebreaks.py', 'feature_fastprune.py', 'feature_framework_miniwallet.py', 'mempool_unbroadcast.py', From 8b91883a23aac64a37d929eeae81325e221d177d Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Thu, 14 Mar 2024 13:48:43 -0400 Subject: [PATCH 0031/2363] Set the same best tip on restart if two candidates have the same work Before this, if we had two (or more) same work tip candidates and restarted our node, it could be the case that the block set as tip after bootstrap didn't match the one before stopping. That's because the work and `nSequenceId` of both block will be the same (the latter is only kept in memory), so the active chain after restart would have depended on what tip candidate was loaded first. This makes sure that we are consistent over reboots. --- src/chain.h | 4 +++- src/node/blockstorage.cpp | 5 +++-- src/validation.cpp | 18 ++++++++++++++++-- src/validation.h | 7 ++++--- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/chain.h b/src/chain.h index f5bfdb2fb4b0..d348fefe2a21 100644 --- a/src/chain.h +++ b/src/chain.h @@ -191,7 +191,9 @@ class CBlockIndex uint32_t nNonce{0}; //! (memory only) Sequential id assigned to distinguish order in which blocks are received. - int32_t nSequenceId{0}; + //! Initialized to 1 when loading blocks from disk, except for blocks belonging to the best chain + //! which overwrite it to 0. + int32_t nSequenceId{1}; //! (memory only) Maximum nTime in the chain up to and including this block. unsigned int nTimeMax{0}; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 81b55c2440f3..ac8839854b85 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -166,7 +166,8 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIn if (pa->nSequenceId > pb->nSequenceId) return true; // Use pointer address as tie breaker (should only happen with blocks - // loaded from disk, as those all have id 0). + // loaded from disk, as those share the same id: 0 for blocks on the + // best chain, 1 for all others). if (pa < pb) return false; if (pa > pb) return true; @@ -217,7 +218,7 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, CBlockInde // We assign the sequence id to blocks only when the full data is available, // to avoid miners withholding blocks but broadcasting headers, to get a // competitive advantage. - pindexNew->nSequenceId = 0; + pindexNew->nSequenceId = 1; pindexNew->phashBlock = &((*mi).first); BlockMap::iterator miPrev = m_block_index.find(block.hashPrevBlock); diff --git a/src/validation.cpp b/src/validation.cpp index b62691ca4c0a..f9a50d84e6b4 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4680,7 +4680,7 @@ bool Chainstate::LoadChainTip() AssertLockHeld(cs_main); const CCoinsViewCache& coins_cache = CoinsTip(); assert(!coins_cache.GetBestBlock().IsNull()); // Never called when the coins view is empty - const CBlockIndex* tip = m_chain.Tip(); + CBlockIndex* tip = m_chain.Tip(); if (tip && tip->GetBlockHash() == coins_cache.GetBestBlock()) { return true; @@ -4692,6 +4692,20 @@ bool Chainstate::LoadChainTip() return false; } m_chain.SetTip(*pindex); + tip = m_chain.Tip(); + + // Make sure our chain tip before shutting down scores better than any other candidate + // to maintain a consistent best tip over reboots in case of a tie. + auto target = tip; + while (target) { + target->nSequenceId = 0; + target = target->pprev; + } + + // Block index candidates are loaded before the chain tip, so we need to replace this entry + // Otherwise the scoring will be based on the memory address location instead of the nSequenceId + setBlockIndexCandidates.erase(tip); + TryAddBlockIndexCandidate(tip); PruneBlockIndexCandidates(); tip = m_chain.Tip(); @@ -5343,7 +5357,7 @@ void ChainstateManager::CheckBlockIndex() const } } } - if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= 0); // nSequenceId can't be set positive for blocks that aren't linked (negative is used for preciousblock) + if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= 1); // nSequenceId can't be set higher than 1 for blocks that aren't linked (negative is used for preciousblock, 0 for active chain) // VALID_TRANSACTIONS is equivalent to nTx > 0 for all nodes (whether or not pruning has occurred). // HAVE_DATA is only equivalent to nTx > 0 (or VALID_TRANSACTIONS) if no pruning has occurred. if (!m_blockman.m_have_pruned) { diff --git a/src/validation.h b/src/validation.h index c25dd2de2d96..de3711b5c8c6 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1034,8 +1034,9 @@ class ChainstateManager * Every received block is assigned a unique and increasing identifier, so we * know which one to give priority in case of a fork. */ - /** Blocks loaded from disk are assigned id 0, so start the counter at 1. */ - int32_t nBlockSequenceId GUARDED_BY(::cs_main) = 1; + /** Blocks loaded from disk are assigned id 1 (0 if they belong to the best + * chain loaded from disk), so start the counter at 2. **/ + int32_t nBlockSequenceId GUARDED_BY(::cs_main) = 2; /** Decreasing counter (used by subsequent preciousblock calls). */ int32_t nBlockReverseSequenceId = -1; /** chainwork for the last block that preciousblock has been applied to. */ @@ -1046,7 +1047,7 @@ class ChainstateManager void ResetBlockSequenceCounters() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - nBlockSequenceId = 1; + nBlockSequenceId = 2; nBlockReverseSequenceId = -1; } From 18524b072e6bdd590a9f6badd15d897b5ef5ce54 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 28 Jan 2025 12:12:36 -0500 Subject: [PATCH 0032/2363] Make nSequenceId init value constants Make it easier to follow what the values come without having to go over the comments, plus easier to maintain --- src/chain.h | 9 ++++++--- src/node/blockstorage.cpp | 2 +- src/validation.cpp | 6 ++++-- src/validation.h | 9 +++++---- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/chain.h b/src/chain.h index d348fefe2a21..8aa1ebb546a1 100644 --- a/src/chain.h +++ b/src/chain.h @@ -35,6 +35,9 @@ static constexpr int64_t MAX_FUTURE_BLOCK_TIME = 2 * 60 * 60; * MAX_FUTURE_BLOCK_TIME. */ static constexpr int64_t TIMESTAMP_WINDOW = MAX_FUTURE_BLOCK_TIME; +//! Init values for CBlockIndex nSequenceId when loaded from disk +static constexpr int32_t SEQ_ID_BEST_CHAIN_FROM_DISK = 0; +static constexpr int32_t SEQ_ID_INIT_FROM_DISK = 1; /** * Maximum gap between node time and block time used @@ -191,9 +194,9 @@ class CBlockIndex uint32_t nNonce{0}; //! (memory only) Sequential id assigned to distinguish order in which blocks are received. - //! Initialized to 1 when loading blocks from disk, except for blocks belonging to the best chain - //! which overwrite it to 0. - int32_t nSequenceId{1}; + //! Initialized to SEQ_ID_INIT_FROM_DISK{1} when loading blocks from disk, except for blocks + //! belonging to the best chain which overwrite it to SEQ_ID_BEST_CHAIN_FROM_DISK{0}. + int32_t nSequenceId{SEQ_ID_INIT_FROM_DISK}; //! (memory only) Maximum nTime in the chain up to and including this block. unsigned int nTimeMax{0}; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ac8839854b85..e1b54175dc22 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -218,7 +218,7 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, CBlockInde // We assign the sequence id to blocks only when the full data is available, // to avoid miners withholding blocks but broadcasting headers, to get a // competitive advantage. - pindexNew->nSequenceId = 1; + pindexNew->nSequenceId = SEQ_ID_INIT_FROM_DISK; pindexNew->phashBlock = &((*mi).first); BlockMap::iterator miPrev = m_block_index.find(block.hashPrevBlock); diff --git a/src/validation.cpp b/src/validation.cpp index f9a50d84e6b4..c6137ddb1374 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4698,7 +4698,7 @@ bool Chainstate::LoadChainTip() // to maintain a consistent best tip over reboots in case of a tie. auto target = tip; while (target) { - target->nSequenceId = 0; + target->nSequenceId = SEQ_ID_BEST_CHAIN_FROM_DISK; target = target->pprev; } @@ -5357,7 +5357,9 @@ void ChainstateManager::CheckBlockIndex() const } } } - if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= 1); // nSequenceId can't be set higher than 1 for blocks that aren't linked (negative is used for preciousblock, 0 for active chain) + // nSequenceId can't be set higher than SEQ_ID_INIT_FROM_DISK{1} for blocks that aren't linked + // (negative is used for preciousblock, SEQ_ID_BEST_CHAIN_FROM_DISK{0} for active chain when loaded from disk) + if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= SEQ_ID_INIT_FROM_DISK); // VALID_TRANSACTIONS is equivalent to nTx > 0 for all nodes (whether or not pruning has occurred). // HAVE_DATA is only equivalent to nTx > 0 (or VALID_TRANSACTIONS) if no pruning has occurred. if (!m_blockman.m_have_pruned) { diff --git a/src/validation.h b/src/validation.h index de3711b5c8c6..70c20ac135c7 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1034,9 +1034,10 @@ class ChainstateManager * Every received block is assigned a unique and increasing identifier, so we * know which one to give priority in case of a fork. */ - /** Blocks loaded from disk are assigned id 1 (0 if they belong to the best - * chain loaded from disk), so start the counter at 2. **/ - int32_t nBlockSequenceId GUARDED_BY(::cs_main) = 2; + /** Blocks loaded from disk are assigned id SEQ_ID_INIT_FROM_DISK{1} + * (SEQ_ID_BEST_CHAIN_FROM_DISK{0} if they belong to the best chain loaded from disk), + * so start the counter after that. **/ + int32_t nBlockSequenceId GUARDED_BY(::cs_main) = SEQ_ID_INIT_FROM_DISK + 1; /** Decreasing counter (used by subsequent preciousblock calls). */ int32_t nBlockReverseSequenceId = -1; /** chainwork for the last block that preciousblock has been applied to. */ @@ -1047,7 +1048,7 @@ class ChainstateManager void ResetBlockSequenceCounters() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - nBlockSequenceId = 2; + nBlockSequenceId = SEQ_ID_INIT_FROM_DISK + 1; nBlockReverseSequenceId = -1; } From 09c95f21e71d196120e6c9d0b1d1923a4927408d Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Thu, 14 Mar 2024 13:53:09 -0400 Subject: [PATCH 0033/2363] test: Adds block tiebreak over restarts tests Adds tests to make sure we are consistent on activating the same chain over a node restart if two or more candidates have the same work when the node is shutdown --- test/functional/feature_chain_tiebreaks.py | 50 +++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_chain_tiebreaks.py b/test/functional/feature_chain_tiebreaks.py index cb1f9ebec9c3..707c99473cf6 100755 --- a/test/functional/feature_chain_tiebreaks.py +++ b/test/functional/feature_chain_tiebreaks.py @@ -23,7 +23,7 @@ def send_headers(node, blocks): # announcement. node.submitheader(hexdata=CBlockHeader(block).serialize().hex()) - def run_test(self): + def test_chain_split_in_memory(self): node = self.nodes[0] # Add P2P connection to bitcoind peer = node.add_p2p_connection(P2PDataStore()) @@ -99,5 +99,53 @@ def run_test(self): # B7 is now active. assert_equal(node.getbestblockhash(), blocks[7].hash_hex) + # Invalidate blocks to start fresh on the next test + node.invalidateblock(blocks[0].hash_hex) + + def test_chain_split_from_disk(self): + node = self.nodes[0] + peer = node.add_p2p_connection(P2PDataStore()) + + self.log.info('Precomputing blocks') + # + # A1 + # / + # G + # \ + # A2 + # + blocks = [] + + # Construct two blocks building from genesis. + start_height = node.getblockcount() + genesis_block = node.getblock(node.getblockhash(start_height)) + prev_time = genesis_block["time"] + + for i in range(0, 2): + blocks.append(create_block( + hashprev=int(genesis_block["hash"], 16), + tmpl={"height": start_height + 1, + # Make sure each block has a different hash. + "curtime": prev_time + i + 1, + } + )) + blocks[-1].solve() + + # Send blocks and test the last one is not connected + self.log.info('Send A1 and A2. Make sure that only the former connects') + peer.send_blocks_and_test([blocks[0]], node, success=True) + peer.send_blocks_and_test([blocks[1]], node, success=False) + + self.log.info('Restart the node and check that the best tip before restarting matched the ones afterwards') + # Restart and check enough times for this to eventually fail if the logic is broken + for _ in range(10): + self.restart_node(0) + assert_equal(blocks[0].hash_hex, node.getbestblockhash()) + + def run_test(self): + self.test_chain_split_in_memory() + self.test_chain_split_from_disk() + + if __name__ == '__main__': ChainTiebreaksTest(__file__).main() From 0465574c127907df9b764055a585e8281bae8d1d Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Fri, 20 Jun 2025 10:56:26 -0400 Subject: [PATCH 0034/2363] test: Fixes send_blocks_and_test docs It's not true that if success=False the tip doesn't advance. It doesn'test advance to the provided tip, but it can advance to a competing one --- test/functional/test_framework/p2p.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index 610aa4ccca29..7aacf66d463f 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -866,8 +866,8 @@ def send_blocks_and_test(self, blocks, node, *, success=True, force_send=False, - the on_getheaders handler will ensure that any getheaders are responded to - if force_send is False: wait for getdata for each of the blocks. The on_getdata handler will ensure that any getdata messages are responded to. Otherwise send the full block unsolicited. - - if success is True: assert that the node's tip advances to the most recent block - - if success is False: assert that the node's tip doesn't advance + - if success is True: assert that the node's tip is the last block in blocks at the end of the operation. + - if success is False: assert that the node's tip isn't the last block in blocks at the end of the operation - if reject_reason is set: assert that the correct reject message is logged""" with p2p_lock: From 8a08eef645eeb3e1991a80480c5ee232bfceeb37 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 21 Jul 2025 13:41:36 -0700 Subject: [PATCH 0035/2363] tests: Check that the last hardened cache upgrade occurs When loading an older wallet without the last hardened cache, an automatic upgrade should be performed. Check this in wallet_backwards_compatibility.py When migrating a wallet, the migrated wallet should always have the last hardened cache, so verify in wallet_migration.py --- .../test_framework/test_framework.py | 12 ++++++ .../wallet_backwards_compatibility.py | 43 +++++++++++++------ test/functional/wallet_migration.py | 25 ++++++++++- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 639421d3aeac..e29815f5d296 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -1082,3 +1082,15 @@ def convert_to_json_for_cli(self, text): if self.options.usecli: return json.dumps(text) return text + + def inspect_sqlite_db(self, path, fn, *args, **kwargs): + try: + import sqlite3 # type: ignore[import] + conn = sqlite3.connect(path) + with conn: + result = fn(conn, *args, **kwargs) + conn.close() + return result + except ImportError: + self.log.warning("sqlite3 module not available, skipping tests that inspect the database") + diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index 43001fb3f30e..1f335398738c 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -21,9 +21,11 @@ from test_framework.blocktools import COINBASE_MATURITY from test_framework.test_framework import BitcoinTestFramework from test_framework.descriptors import descsum_create +from test_framework.messages import ser_string from test_framework.util import ( assert_equal, + assert_greater_than, assert_raises_rpc_error, ) @@ -149,18 +151,13 @@ def test_v22_inactivehdchain_path(self): assert_equal(bad_deriv_wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path) bad_deriv_wallet_master.unloadwallet() - # If we have sqlite3, verify that there are no keymeta records - try: - import sqlite3 - wallet_db = node_master.wallets_path / wallet_name / "wallet.dat" - conn = sqlite3.connect(wallet_db) - with conn: - # Retrieve all records that have the "keymeta" prefix. The remaining key data varies for each record. - keymeta_rec = conn.execute("SELECT value FROM main where key >= x'076b65796d657461' AND key < x'076b65796d657462'").fetchone() - assert_equal(keymeta_rec, None) - conn.close() - except ImportError: - self.log.warning("sqlite3 module not available, skipping lack of keymeta records check") + def check_keymeta(conn): + # Retrieve all records that have the "keymeta" prefix. The remaining key data varies for each record. + keymeta_rec = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'keymeta').hex()}' AND key < x'{ser_string(b'keymetb').hex()}'").fetchone() + assert_equal(keymeta_rec, None) + + wallet_db = node_master.wallets_path / wallet_name / "wallet.dat" + self.inspect_sqlite_db(wallet_db, check_keymeta) def test_ignore_legacy_during_startup(self, legacy_nodes, node_master): self.log.info("Test that legacy wallets are ignored during startup on v29+") @@ -342,6 +339,13 @@ def run_test(self): # Remove the wallet from old node wallet_prev.unloadwallet() + # Open backup with sqlite and get flags + def get_flags(conn): + flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone() + return int.from_bytes(flags_rec[0], byteorder="little") + + old_flags = self.inspect_sqlite_db(backup_path, get_flags) + # Restore the wallet to master load_res = node_master.restorewallet(wallet_name, backup_path) @@ -378,6 +382,21 @@ def run_test(self): wallet.unloadwallet() + # Open the wallet with sqlite and inspect the flags and records + def check_upgraded_records(conn, old_flags): + flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone() + new_flags = int.from_bytes(flags_rec[0], byteorder="little") + diff_flags = new_flags & ~old_flags + + # Check for last hardened xpubs if the flag is newly set + if diff_flags & (1 << 2): + self.log.debug("Checking descriptor cache was upgraded") + # Fetch all records with the walletdescriptorlhcache prefix + lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'walletdescriptorlhcache').hex()}' AND key < x'{ser_string(b'walletdescriptorlhcachf').hex()}'").fetchall() + assert_greater_than(len(lh_cache_recs), 0) + + self.inspect_sqlite_db(down_backup_path, check_upgraded_records, old_flags) + # Check that no automatic upgrade broke downgrading the wallet target_dir = node.wallets_path / down_wallet_name os.makedirs(target_dir, exist_ok=True) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 704204425c7e..a735a9c7d823 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -18,11 +18,12 @@ from test_framework.descriptors import descsum_create from test_framework.key import ECPubKey from test_framework.test_framework import BitcoinTestFramework -from test_framework.messages import COIN, CTransaction, CTxOut +from test_framework.messages import COIN, CTransaction, CTxOut, ser_string from test_framework.script import hash160 from test_framework.script_util import key_to_p2pkh_script, key_to_p2pk_script, script_to_p2sh_script, script_to_p2wsh_script from test_framework.util import ( assert_equal, + assert_greater_than, assert_raises_rpc_error, find_vout_for_address, sha256sum_file, @@ -139,7 +140,8 @@ def migrate_and_get_rpc(self, wallet_name, **kwargs): # (in which case the wallet name would be suffixed by the 'watchonly' term) migrated_wallet_name = migrate_info['wallet_name'] wallet = self.master_node.get_wallet_rpc(migrated_wallet_name) - assert_equal(wallet.getwalletinfo()["descriptors"], True) + wallet_info = wallet.getwalletinfo() + assert_equal(wallet_info["descriptors"], True) self.assert_is_sqlite(migrated_wallet_name) # Always verify the backup path exist after migration assert os.path.exists(migrate_info['backup_path']) @@ -151,6 +153,25 @@ def migrate_and_get_rpc(self, wallet_name, **kwargs): expected_backup_path = self.master_node.wallets_path / f"{backup_prefix}_{mocked_time}.legacy.bak" assert_equal(str(expected_backup_path), migrate_info['backup_path']) + # Open the wallet with sqlite and verify that the wallet has the last hardened cache flag + # set and the last hardened cache entries + def check_last_hardened(conn): + flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone() + flags = int.from_bytes(flags_rec[0], byteorder="little") + + # All wallets should have the upgrade flag set + assert_equal(bool(flags & (1 << 2)), True) + + # Fetch all records with the walletdescriptorlhcache prefix + # if the wallet has private keys and is not blank + if wallet_info["private_keys_enabled"] and not wallet_info["blank"]: + lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'walletdescriptorlhcache').hex()}' AND key < x'{ser_string(b'walletdescriptorlhcachf').hex()}'").fetchall() + assert_greater_than(len(lh_cache_recs), 0) + + inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat")) + wallet.backupwallet(inspect_path) + self.inspect_sqlite_db(inspect_path, check_last_hardened) + return migrate_info, wallet def test_basic(self): From 88b0647f027a608acb61ec32329d19f8e5b0a9fd Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 21 Jul 2025 12:59:15 -0700 Subject: [PATCH 0036/2363] wallet: Always write last hardened cache flag in migrated wallets --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 91a494c379fd..2373f30a2c06 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3851,7 +3851,7 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, m_internal_spk_managers.clear(); // Setup new descriptors (only if we are migrating any key material) - SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS); + SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS | WALLET_FLAG_LAST_HARDENED_XPUB_CACHED); if (has_spendable_material && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { // Use the existing master key if we have it if (data.master_key.key.IsValid()) { From 2320184d0ea87279558a8e6cbb3bccf5ba1bb781 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 30 Jul 2025 16:34:36 -0700 Subject: [PATCH 0037/2363] descriptors: Fix meaning of any_key_parsed Invert any_key_parsed so that the name matches the behavior. --- src/script/descriptor.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index bd819d365ae6..83a561a7e789 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -1843,20 +1843,20 @@ std::vector> ParsePubkey(uint32_t& key_exp_index bool any_ranged = false; bool all_bip32 = true; std::vector>> providers; - bool any_key_parsed = true; + bool any_key_parsed = false; size_t max_multipath_len = 0; while (expr.size()) { - if (!any_key_parsed && !Const(",", expr)) { + if (any_key_parsed && !Const(",", expr)) { error = strprintf("musig(): expected ',', got '%c'", expr[0]); return {}; } - any_key_parsed = false; auto arg = Expr(expr); auto pk = ParsePubkey(key_exp_index, arg, ParseScriptContext::MUSIG, out, error); if (pk.empty()) { error = strprintf("musig(): %s", error); return {}; } + any_key_parsed = true; any_ranged = any_ranged || pk.at(0)->IsRange(); all_bip32 = all_bip32 && pk.at(0)->IsBIP32(); @@ -1866,7 +1866,7 @@ std::vector> ParsePubkey(uint32_t& key_exp_index providers.emplace_back(std::move(pk)); key_exp_index++; } - if (any_key_parsed) { + if (!any_key_parsed) { error = "musig(): Must contain key expressions"; return {}; } From 39a63bf2e7e38dd3f30b5d1a8f6b2fff0e380d12 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 30 Jul 2025 16:36:12 -0700 Subject: [PATCH 0038/2363] descriptors: Add a doxygen comment for has_hardened output_parameter --- src/script/descriptor.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 83a561a7e789..3a402702178f 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -1647,6 +1647,7 @@ std::optional ParseKeyPathNum(std::span elem, bool& apostr * @param[out] apostrophe only updated if hardened derivation is found * @param[out] error parsing error message * @param[in] allow_multipath Allows the parsed path to use the multipath specifier + * @param[out] has_hardened Records whether the path contains any hardened derivation * @returns false if parsing failed **/ [[nodiscard]] bool ParseKeyPath(const std::vector>& split, std::vector& out, bool& apostrophe, std::string& error, bool allow_multipath, bool& has_hardened) From a4cfddda644f1fc9a815b2d16c997716cd63554a Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 30 Jul 2025 16:36:54 -0700 Subject: [PATCH 0039/2363] tests: Clarify why musig derivation adds a pubkey and xpub --- src/test/descriptor_tests.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 4dc94134278f..69ae51610eb9 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -52,7 +52,7 @@ constexpr int XONLY_KEYS = 1 << 6; // X-only pubkeys are in use (and thus inferr constexpr int MISSING_PRIVKEYS = 1 << 7; // Not all private keys are available, so ToPrivateString will fail. constexpr int SIGNABLE_FAILS = 1 << 8; // We can sign with this descriptor, but actually trying to sign will fail constexpr int MUSIG = 1 << 9; // This is a MuSig so key counts will have an extra key -constexpr int MUSIG_DERIVATION = 1 << 10; // MuSig with derivation from the aggregate key +constexpr int MUSIG_DERIVATION = 1 << 10; // MuSig with BIP 328 derivation from the aggregate key constexpr int MIXED_MUSIG = 1 << 11; // Both MuSig and normal key expressions are present constexpr int UNIQUE_XPUBS = 1 << 12; // Whether the xpub count should be of unique xpubs @@ -315,6 +315,7 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int size_t num_xpubs = CountXpubs(pub1); size_t num_unique_xpubs = CountUniqueXpubs(pub1); if (flags & MUSIG_DERIVATION) { + // Deriving from the aggregate will include the synthetic xpub of the aggregate in the caches and SigningProviders. num_xpubs++; num_unique_xpubs++; } From fb8720f1e09f4e41802f07be53fb220d6f6c127f Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 5 Feb 2024 15:09:40 -0500 Subject: [PATCH 0040/2363] sign: Refactor Schnorr sighash computation out of CreateSchnorrSig There will be other functions within MutableTransactionSignatureCreator that need to compute the same sighash, so make it a separate member function. --- src/script/sign.cpp | 26 ++++++++++++++++++-------- src/script/sign.h | 2 ++ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 33cbc38be41d..1e40d5328fc2 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -59,17 +59,14 @@ bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provid return true; } -bool MutableTransactionSignatureCreator::CreateSchnorrSig(const SigningProvider& provider, std::vector& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const +std::optional MutableTransactionSignatureCreator::ComputeSchnorrSignatureHash(const uint256* leaf_hash, SigVersion sigversion) const { assert(sigversion == SigVersion::TAPROOT || sigversion == SigVersion::TAPSCRIPT); - CKey key; - if (!provider.GetKeyByXOnly(pubkey, key)) return false; - // BIP341/BIP342 signing needs lots of precomputed transaction data. While some // (non-SIGHASH_DEFAULT) sighash modes exist that can work with just some subset // of data present, for now, only support signing when everything is provided. - if (!m_txdata || !m_txdata->m_bip341_taproot_ready || !m_txdata->m_spent_outputs_ready) return false; + if (!m_txdata || !m_txdata->m_bip341_taproot_ready || !m_txdata->m_spent_outputs_ready) return std::nullopt; ScriptExecutionData execdata; execdata.m_annex_init = true; @@ -77,15 +74,28 @@ bool MutableTransactionSignatureCreator::CreateSchnorrSig(const SigningProvider& if (sigversion == SigVersion::TAPSCRIPT) { execdata.m_codeseparator_pos_init = true; execdata.m_codeseparator_pos = 0xFFFFFFFF; // Only support non-OP_CODESEPARATOR BIP342 signing for now. - if (!leaf_hash) return false; // BIP342 signing needs leaf hash. + if (!leaf_hash) return std::nullopt; // BIP342 signing needs leaf hash. execdata.m_tapleaf_hash_init = true; execdata.m_tapleaf_hash = *leaf_hash; } uint256 hash; - if (!SignatureHashSchnorr(hash, execdata, m_txto, nIn, nHashType, sigversion, *m_txdata, MissingDataBehavior::FAIL)) return false; + if (!SignatureHashSchnorr(hash, execdata, m_txto, nIn, nHashType, sigversion, *m_txdata, MissingDataBehavior::FAIL)) return std::nullopt; + return hash; +} + +bool MutableTransactionSignatureCreator::CreateSchnorrSig(const SigningProvider& provider, std::vector& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const +{ + assert(sigversion == SigVersion::TAPROOT || sigversion == SigVersion::TAPSCRIPT); + + CKey key; + if (!provider.GetKeyByXOnly(pubkey, key)) return false; + + std::optional hash = ComputeSchnorrSignatureHash(leaf_hash, sigversion); + if (!hash.has_value()) return false; + sig.resize(64); // Use uint256{} as aux_rnd for now. - if (!key.SignSchnorr(hash, sig, merkle_root, {})) return false; + if (!key.SignSchnorr(*hash, sig, merkle_root, {})) return false; if (nHashType) sig.push_back(nHashType); return true; } diff --git a/src/script/sign.h b/src/script/sign.h index fe2c470bc644..5af6392b128f 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -45,6 +45,8 @@ class MutableTransactionSignatureCreator : public BaseSignatureCreator const MutableTransactionSignatureChecker checker; const PrecomputedTransactionData* m_txdata; + std::optional ComputeSchnorrSignatureHash(const uint256* leaf_hash, SigVersion sigversion) const; + public: MutableTransactionSignatureCreator(const CMutableTransaction& tx LIFETIMEBOUND, unsigned int input_idx, const CAmount& amount, int hash_type); MutableTransactionSignatureCreator(const CMutableTransaction& tx LIFETIMEBOUND, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, int hash_type); From a099655f2e1be313a6b51bba9799ea512c6eda3c Mon Sep 17 00:00:00 2001 From: rkrux Date: Fri, 13 Jun 2025 16:15:13 +0530 Subject: [PATCH 0041/2363] scripted-diff: Update `DeriveType` enum values to mention ranged derivations While reviewing the MuSig2 descriptors PR 31244, I realized that the enum `DeriveType` here logically refers to the derive type for ranged descriptors. This became evident to me while going through the implementations of `IsRange` & `IsHardened` functions of `BIP32PubkeyProvider`, and the `ParsePubkeyInner` function. Initially I got confused by reading `IsRange` translating to `!= DeriveType::NO`, but later realised it specifically referred to the presence of ranged derivations. I propose explicitly mentioning "RANGED" in the values of the `DeriveType` enum would make it easier to parse the descriptors code. This enum is used in one file only - `script/descriptors.cpp`. That's why I explicitly passed it as the argument in the `sed` commands in the script below. -BEGIN VERIFY SCRIPT- sed -i 's/HARDENED\b/HARDENED_RANGED/g' src/script/descriptor.cpp sed -i 's/\bNO\b/NON_RANGED/g' src/script/descriptor.cpp -END VERIFY SCRIPT- --- src/script/descriptor.cpp | 54 +++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index bd819d365ae6..aba92de6635b 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -359,9 +359,9 @@ class ConstPubkeyProvider final : public PubkeyProvider }; enum class DeriveType { - NO, - UNHARDENED, - HARDENED, + NON_RANGED, + UNHARDENED_RANGED, + HARDENED_RANGED, }; /** An object representing a parsed extended public key in a descriptor. */ @@ -401,7 +401,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider bool IsHardened() const { - if (m_derive == DeriveType::HARDENED) return true; + if (m_derive == DeriveType::HARDENED_RANGED) return true; for (auto entry : m_path) { if (entry >> 31) return true; } @@ -410,7 +410,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider public: BIP32PubkeyProvider(uint32_t exp_index, const CExtPubKey& extkey, KeyPath path, DeriveType derive, bool apostrophe) : PubkeyProvider(exp_index), m_root_extkey(extkey), m_path(std::move(path)), m_derive(derive), m_apostrophe(apostrophe) {} - bool IsRange() const override { return m_derive != DeriveType::NO; } + bool IsRange() const override { return m_derive != DeriveType::NON_RANGED; } size_t GetSize() const override { return 33; } bool IsBIP32() const override { return true; } std::optional GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override @@ -419,8 +419,8 @@ class BIP32PubkeyProvider final : public PubkeyProvider CKeyID keyid = m_root_extkey.pubkey.GetID(); std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint); info.path = m_path; - if (m_derive == DeriveType::UNHARDENED) info.path.push_back((uint32_t)pos); - if (m_derive == DeriveType::HARDENED) info.path.push_back(((uint32_t)pos) | 0x80000000L); + if (m_derive == DeriveType::UNHARDENED_RANGED) info.path.push_back((uint32_t)pos); + if (m_derive == DeriveType::HARDENED_RANGED) info.path.push_back(((uint32_t)pos) | 0x80000000L); // Derive keys or fetch them from cache CExtPubKey final_extkey = m_root_extkey; @@ -429,19 +429,19 @@ class BIP32PubkeyProvider final : public PubkeyProvider bool der = true; if (read_cache) { if (!read_cache->GetCachedDerivedExtPubKey(m_expr_index, pos, final_extkey)) { - if (m_derive == DeriveType::HARDENED) return std::nullopt; + if (m_derive == DeriveType::HARDENED_RANGED) return std::nullopt; // Try to get the derivation parent if (!read_cache->GetCachedParentExtPubKey(m_expr_index, parent_extkey)) return std::nullopt; final_extkey = parent_extkey; - if (m_derive == DeriveType::UNHARDENED) der = parent_extkey.Derive(final_extkey, pos); + if (m_derive == DeriveType::UNHARDENED_RANGED) der = parent_extkey.Derive(final_extkey, pos); } } else if (IsHardened()) { CExtKey xprv; CExtKey lh_xprv; if (!GetDerivedExtKey(arg, xprv, lh_xprv)) return std::nullopt; parent_extkey = xprv.Neuter(); - if (m_derive == DeriveType::UNHARDENED) der = xprv.Derive(xprv, pos); - if (m_derive == DeriveType::HARDENED) der = xprv.Derive(xprv, pos | 0x80000000UL); + if (m_derive == DeriveType::UNHARDENED_RANGED) der = xprv.Derive(xprv, pos); + if (m_derive == DeriveType::HARDENED_RANGED) der = xprv.Derive(xprv, pos | 0x80000000UL); final_extkey = xprv.Neuter(); if (lh_xprv.key.IsValid()) { last_hardened_extkey = lh_xprv.Neuter(); @@ -451,8 +451,8 @@ class BIP32PubkeyProvider final : public PubkeyProvider if (!parent_extkey.Derive(parent_extkey, entry)) return std::nullopt; } final_extkey = parent_extkey; - if (m_derive == DeriveType::UNHARDENED) der = parent_extkey.Derive(final_extkey, pos); - assert(m_derive != DeriveType::HARDENED); + if (m_derive == DeriveType::UNHARDENED_RANGED) der = parent_extkey.Derive(final_extkey, pos); + assert(m_derive != DeriveType::HARDENED_RANGED); } if (!der) return std::nullopt; @@ -461,7 +461,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider if (write_cache) { // Only cache parent if there is any unhardened derivation - if (m_derive != DeriveType::HARDENED) { + if (m_derive != DeriveType::HARDENED_RANGED) { write_cache->CacheParentExtPubKey(m_expr_index, parent_extkey); // Cache last hardened xpub if we have it if (last_hardened_extkey.pubkey.IsValid()) { @@ -481,7 +481,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider std::string ret = EncodeExtPubKey(m_root_extkey) + FormatHDKeypath(m_path, /*apostrophe=*/use_apostrophe); if (IsRange()) { ret += "/*"; - if (m_derive == DeriveType::HARDENED) ret += use_apostrophe ? '\'' : 'h'; + if (m_derive == DeriveType::HARDENED_RANGED) ret += use_apostrophe ? '\'' : 'h'; } return ret; } @@ -496,13 +496,13 @@ class BIP32PubkeyProvider final : public PubkeyProvider out = EncodeExtKey(key) + FormatHDKeypath(m_path, /*apostrophe=*/m_apostrophe); if (IsRange()) { out += "/*"; - if (m_derive == DeriveType::HARDENED) out += m_apostrophe ? '\'' : 'h'; + if (m_derive == DeriveType::HARDENED_RANGED) out += m_apostrophe ? '\'' : 'h'; } return true; } bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache) const override { - if (m_derive == DeriveType::HARDENED) { + if (m_derive == DeriveType::HARDENED_RANGED) { out = ToString(StringType::PUBLIC, /*normalized=*/true); return true; @@ -554,7 +554,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider out = "[" + origin_str + "]" + EncodeExtPubKey(xpub) + FormatHDKeypath(end_path); if (IsRange()) { out += "/*"; - assert(m_derive == DeriveType::UNHARDENED); + assert(m_derive == DeriveType::UNHARDENED_RANGED); } return true; } @@ -563,8 +563,8 @@ class BIP32PubkeyProvider final : public PubkeyProvider CExtKey extkey; CExtKey dummy; if (!GetDerivedExtKey(arg, extkey, dummy)) return; - if (m_derive == DeriveType::UNHARDENED && !extkey.Derive(extkey, pos)) return; - if (m_derive == DeriveType::HARDENED && !extkey.Derive(extkey, pos | 0x80000000UL)) return; + if (m_derive == DeriveType::UNHARDENED_RANGED && !extkey.Derive(extkey, pos)) return; + if (m_derive == DeriveType::HARDENED_RANGED && !extkey.Derive(extkey, pos | 0x80000000UL)) return; out.keys.emplace(extkey.key.GetPubKey().GetID(), extkey.key); } std::optional GetRootPubKey() const override @@ -595,7 +595,7 @@ class MuSigPubkeyProvider final : public PubkeyProvider const DeriveType m_derive; const bool m_ranged_participants; - bool IsRangedDerivation() const { return m_derive != DeriveType::NO; } + bool IsRangedDerivation() const { return m_derive != DeriveType::NON_RANGED; } public: MuSigPubkeyProvider( @@ -613,7 +613,7 @@ class MuSigPubkeyProvider final : public PubkeyProvider if (!Assume(!(m_ranged_participants && IsRangedDerivation()))) { throw std::runtime_error("musig(): Cannot have both ranged participants and ranged derivation"); } - if (!Assume(m_derive != DeriveType::HARDENED)) { + if (!Assume(m_derive != DeriveType::HARDENED_RANGED)) { throw std::runtime_error("musig(): Cannot have hardened derivation"); } } @@ -1723,14 +1723,14 @@ std::optional ParseKeyPathNum(std::span elem, bool& apostr static DeriveType ParseDeriveType(std::vector>& split, bool& apostrophe) { - DeriveType type = DeriveType::NO; + DeriveType type = DeriveType::NON_RANGED; if (std::ranges::equal(split.back(), std::span{"*"}.first(1))) { split.pop_back(); - type = DeriveType::UNHARDENED; + type = DeriveType::UNHARDENED_RANGED; } else if (std::ranges::equal(split.back(), std::span{"*'"}.first(2)) || std::ranges::equal(split.back(), std::span{"*h"}.first(2))) { apostrophe = std::ranges::equal(split.back(), std::span{"*'"}.first(2)); split.pop_back(); - type = DeriveType::HARDENED; + type = DeriveType::HARDENED_RANGED; } return type; } @@ -1872,7 +1872,7 @@ std::vector> ParsePubkey(uint32_t& key_exp_index } // Parse any derivation - DeriveType deriv_type = DeriveType::NO; + DeriveType deriv_type = DeriveType::NON_RANGED; std::vector derivation_multipaths; if (split.size() == 2 && Const("/", split.at(1), /*skip=*/false)) { if (!all_bip32) { @@ -1886,7 +1886,7 @@ std::vector> ParsePubkey(uint32_t& key_exp_index bool dummy = false; auto deriv_split = Split(split.at(1), '/'); deriv_type = ParseDeriveType(deriv_split, dummy); - if (deriv_type == DeriveType::HARDENED) { + if (deriv_type == DeriveType::HARDENED_RANGED) { error = "musig(): Cannot have hardened child derivation"; return {}; } From fa6497ba71e9573d341c1c051af09b3ec2fc8d74 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 6 Aug 2025 16:16:07 +0200 Subject: [PATCH 0042/2363] build: Set AUTHOR_WARNING on warnings --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 42552b9613eb..6017775fa78c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -708,6 +708,7 @@ if(configure_warnings) message(WARNING "${warning}") endforeach() message(" ******\n") + message(AUTHOR_WARNING "Warnings have been encountered!") endif() # We want all build properties to be encapsulated properly. From adefb51c5437667696cacaf163ea08b39e961358 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sun, 7 May 2023 12:34:40 +1000 Subject: [PATCH 0043/2363] rpc/net: add per-peer inv_to_send sizes --- src/net_processing.cpp | 2 ++ src/net_processing.h | 1 + src/rpc/net.cpp | 2 ++ test/functional/rpc_net.py | 1 + 4 files changed, 6 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ec6f55cfdf6c..373169c08bde 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1728,9 +1728,11 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) { stats.m_relay_txs = WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs); stats.m_fee_filter_received = tx_relay->m_fee_filter_received.load(); + stats.m_inv_to_send = WITH_LOCK(tx_relay->m_tx_inventory_mutex, return tx_relay->m_tx_inventory_to_send.size()); } else { stats.m_relay_txs = false; stats.m_fee_filter_received = 0; + stats.m_inv_to_send = 0; } stats.m_ping_wait = ping_wait; diff --git a/src/net_processing.h b/src/net_processing.h index 8c140d98ad66..ee12bd08d347 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -54,6 +54,7 @@ struct CNodeStateStats { std::chrono::microseconds m_ping_wait; std::vector vHeightInFlight; bool m_relay_txs; + int m_inv_to_send = 0; CAmount m_fee_filter_received; uint64_t m_addr_processed = 0; uint64_t m_addr_rate_limited = 0; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index fbb70d72161d..e5b9880ad965 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -142,6 +142,7 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::STR, "SERVICE_NAME", "the service name if it is recognised"} }}, {RPCResult::Type::BOOL, "relaytxes", "Whether we relay transactions to this peer"}, + {RPCResult::Type::NUM, "inv_to_send", "How many txs we have queued to announce to this peer"}, {RPCResult::Type::NUM_TIME, "lastsend", "The " + UNIX_EPOCH_TIME + " of the last send"}, {RPCResult::Type::NUM_TIME, "lastrecv", "The " + UNIX_EPOCH_TIME + " of the last receive"}, {RPCResult::Type::NUM_TIME, "last_transaction", "The " + UNIX_EPOCH_TIME + " of the last valid transaction received from this peer"}, @@ -238,6 +239,7 @@ static RPCHelpMan getpeerinfo() obj.pushKV("services", strprintf("%016x", services)); obj.pushKV("servicesnames", GetServicesNames(services)); obj.pushKV("relaytxes", statestats.m_relay_txs); + obj.pushKV("inv_to_send", statestats.m_inv_to_send); obj.pushKV("lastsend", count_seconds(stats.m_last_send)); obj.pushKV("lastrecv", count_seconds(stats.m_last_recv)); obj.pushKV("last_transaction", count_seconds(stats.m_last_tx_time)); diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 41ecbbed22d9..f52d59d7b3e8 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -166,6 +166,7 @@ def test_getpeerinfo(self): "permissions": [], "presynced_headers": -1, "relaytxes": False, + "inv_to_send": 0, "services": "0000000000000000", "servicesnames": [], "session_id": "" if not self.options.v2transport else no_version_peer.v2_state.peer['session_id'].hex(), From 50b63a5698e533376ef7a20bc0c440d3d6bf7a9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:18:54 -0700 Subject: [PATCH 0044/2363] refactor: inline constant return value of `CDBWrapper::WriteBatch` `WriteBatch` can only ever return `true` - its errors are handled by throwing a `throw dbwrapper_error` instead. The boolean return value is quite confusing, especially since it's symmetric with `CDBWrapper::Read`, which catches the exceptions and returns a boolean instead. We're removing the constant return value and inlining `true` for its usages. --- src/dbwrapper.cpp | 3 +-- src/dbwrapper.h | 8 +++++--- src/index/base.cpp | 2 +- src/index/blockfilterindex.cpp | 2 +- src/index/coinstatsindex.cpp | 2 +- src/index/txindex.cpp | 3 ++- src/node/blockstorage.cpp | 3 ++- src/test/dbwrapper_tests.cpp | 2 +- src/txdb.cpp | 4 ++-- 9 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index fe5f9cb0893d..cb9abee563fd 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -274,7 +274,7 @@ CDBWrapper::~CDBWrapper() DBContext().options.env = nullptr; } -bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync) +void CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync) { const bool log_memory = LogAcceptCategory(BCLog::LEVELDB, BCLog::Level::Debug); double mem_before = 0; @@ -288,7 +288,6 @@ bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync) LogDebug(BCLog::LEVELDB, "WriteBatch memory usage: db=%s, before=%.1fMiB, after=%.1fMiB\n", m_name, mem_before, mem_after); } - return true; } size_t CDBWrapper::DynamicMemoryUsage() const diff --git a/src/dbwrapper.h b/src/dbwrapper.h index b9b98bd96ade..94e452db76bb 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -234,7 +234,8 @@ class CDBWrapper { CDBBatch batch(*this); batch.Write(key, value); - return WriteBatch(batch, fSync); + WriteBatch(batch, fSync); + return true; } //! @returns filesystem path to the on-disk data. @@ -259,10 +260,11 @@ class CDBWrapper { CDBBatch batch(*this); batch.Erase(key); - return WriteBatch(batch, fSync); + WriteBatch(batch, fSync); + return true; } - bool WriteBatch(CDBBatch& batch, bool fSync = false); + void WriteBatch(CDBBatch& batch, bool fSync = false); // Get an estimate of LevelDB memory usage (in bytes). size_t DynamicMemoryUsage() const; diff --git a/src/index/base.cpp b/src/index/base.cpp index fdd0e0d8af2e..0fdb5af3ba10 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -262,7 +262,7 @@ bool BaseIndex::Commit() ok = CustomCommit(batch); if (ok) { GetDB().WriteBestBlock(batch, GetLocator(*m_chain, m_best_block_index.load()->GetBlockHash())); - ok = GetDB().WriteBatch(batch); + GetDB().WriteBatch(batch); } } if (!ok) { diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 2ccae3a221b2..c56d787907a6 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -338,7 +338,7 @@ bool BlockFilterIndex::CustomRemove(const interfaces::BlockInfo& block) // But since this creates new references to the filter, the position should get updated here // atomically as well in case Commit fails. batch.Write(DB_FILTER_POS, m_next_filter_pos); - if (!m_db->WriteBatch(batch)) return false; + m_db->WriteBatch(batch); // Update cached header to the previous block hash m_last_header = *Assert(ReadFilterHeader(block.height - 1, *Assert(block.prev_hash))); diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 96693f7f48a2..8f172d706a7d 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -263,7 +263,7 @@ bool CoinStatsIndex::CustomRemove(const interfaces::BlockInfo& block) return false; } - if (!m_db->WriteBatch(batch)) return false; + m_db->WriteBatch(batch); if (!ReverseBlock(block)) { return false; // failure cause logged internally diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 11dd856e1b67..16038a3a2b66 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -46,7 +46,8 @@ bool TxIndex::DB::WriteTxs(const std::vector>& v_pos for (const auto& [txid, pos] : v_pos) { batch.Write(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos); } - return WriteBatch(batch); + WriteBatch(batch); + return true; } TxIndex::TxIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory, bool f_wipe) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 3c01ff2e8d62..f1d4be6b7069 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -88,7 +88,8 @@ bool BlockTreeDB::WriteBatchSync(const std::vectorGetBlockHash()), CDiskBlockIndex{bi}); } - return WriteBatch(batch, true); + WriteBatch(batch, true); + return true; } bool BlockTreeDB::WriteFlag(const std::string& name, bool fValue) diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp index cd0f347b66e6..7066665751d5 100644 --- a/src/test/dbwrapper_tests.cpp +++ b/src/test/dbwrapper_tests.cpp @@ -173,7 +173,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch) // Remove key3 before it's even been written batch.Erase(key3); - BOOST_CHECK(dbw.WriteBatch(batch)); + dbw.WriteBatch(batch); BOOST_CHECK(dbw.Read(key, res)); BOOST_CHECK_EQUAL(res.ToString(), in.ToString()); diff --git a/src/txdb.cpp b/src/txdb.cpp index bb6ee2eb5243..23824f39c062 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -149,9 +149,9 @@ bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashB batch.Write(DB_BEST_BLOCK, hashBlock); LogDebug(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.ApproximateSize() * (1.0 / 1048576.0)); - bool ret = m_db->WriteBatch(batch); + m_db->WriteBatch(batch); LogDebug(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count); - return ret; + return true; } size_t CCoinsViewDB::EstimateSize() const From d1847cf5b5af232ad180f5d302361b72334952b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:23:07 -0700 Subject: [PATCH 0045/2363] refactor: inline constant return value of `TxIndex::DB::WriteTxs` --- src/index/txindex.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 16038a3a2b66..9554faf1e3af 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -28,7 +28,7 @@ class TxIndex::DB : public BaseIndex::DB bool ReadTxPos(const Txid& txid, CDiskTxPos& pos) const; /// Write a batch of transaction positions to the DB. - [[nodiscard]] bool WriteTxs(const std::vector>& v_pos); + void WriteTxs(const std::vector>& v_pos); }; TxIndex::DB::DB(size_t n_cache_size, bool f_memory, bool f_wipe) : @@ -40,14 +40,13 @@ bool TxIndex::DB::ReadTxPos(const Txid& txid, CDiskTxPos& pos) const return Read(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos); } -bool TxIndex::DB::WriteTxs(const std::vector>& v_pos) +void TxIndex::DB::WriteTxs(const std::vector>& v_pos) { CDBBatch batch(*this); for (const auto& [txid, pos] : v_pos) { batch.Write(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos); } WriteBatch(batch); - return true; } TxIndex::TxIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory, bool f_wipe) @@ -69,7 +68,8 @@ bool TxIndex::CustomAppend(const interfaces::BlockInfo& block) vPos.emplace_back(tx->GetHash(), pos); pos.nTxOffset += ::GetSerializeSize(TX_WITH_WITNESS(*tx)); } - return m_db->WriteTxs(vPos); + m_db->WriteTxs(vPos); + return true; } BaseIndex::DB& TxIndex::GetDB() const { return *m_db; } From cdab9480e9e35656f490878f92dab5427b36f21d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:28:44 -0700 Subject: [PATCH 0046/2363] refactor: inline constant return value of `CDBWrapper::Write` --- src/dbwrapper.h | 3 +-- src/index/blockfilterindex.cpp | 4 +--- src/index/coinstatsindex.cpp | 3 ++- src/node/blockstorage.cpp | 6 ++++-- src/test/dbwrapper_tests.cpp | 36 +++++++++++++++++----------------- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 94e452db76bb..50be26fb99e9 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -230,12 +230,11 @@ class CDBWrapper } template - bool Write(const K& key, const V& value, bool fSync = false) + void Write(const K& key, const V& value, bool fSync = false) { CDBBatch batch(*this); batch.Write(key, value); WriteBatch(batch, fSync); - return true; } //! @returns filesystem path to the on-disk data. diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index c56d787907a6..77bf931d9d09 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -291,9 +291,7 @@ bool BlockFilterIndex::Write(const BlockFilter& filter, uint32_t block_height, c value.second.header = filter_header; value.second.pos = m_next_filter_pos; - if (!m_db->Write(DBHeightKey(block_height), value)) { - return false; - } + m_db->Write(DBHeightKey(block_height), value); m_next_filter_pos.nPos += bytes_written; return true; diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 8f172d706a7d..8072167e4526 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -226,7 +226,8 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) // Intentionally do not update DB_MUHASH here so it stays in sync with // DB_BEST_BLOCK, and the index is not corrupted if there is an unclean shutdown. - return m_db->Write(DBHeightKey(block.height), value); + m_db->Write(DBHeightKey(block.height), value); + return true; } [[nodiscard]] static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch, diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index f1d4be6b7069..7371111ccbc5 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -62,7 +62,8 @@ bool BlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo& info) bool BlockTreeDB::WriteReindexing(bool fReindexing) { if (fReindexing) { - return Write(DB_REINDEX_FLAG, uint8_t{'1'}); + Write(DB_REINDEX_FLAG, uint8_t{'1'}); + return true; } else { return Erase(DB_REINDEX_FLAG); } @@ -94,7 +95,8 @@ bool BlockTreeDB::WriteBatchSync(const std::vector it(const_cast(dbw).NewIterator()); @@ -238,7 +238,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate) uint256 in = m_rng.rand256(); uint256 res; - BOOST_CHECK(dbw->Write(key, in)); + dbw->Write(key, in); BOOST_CHECK(dbw->Read(key, res)); BOOST_CHECK_EQUAL(res.ToString(), in.ToString()); @@ -261,7 +261,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate) uint256 res3; // Check that we can write successfully - BOOST_CHECK(odbw.Write(key, in2)); + odbw.Write(key, in2); BOOST_CHECK(odbw.Read(key, res3)); BOOST_CHECK_EQUAL(res3.ToString(), in2.ToString()); } @@ -279,7 +279,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex) uint256 in = m_rng.rand256(); uint256 res; - BOOST_CHECK(dbw->Write(key, in)); + dbw->Write(key, in); BOOST_CHECK(dbw->Read(key, res)); BOOST_CHECK_EQUAL(res.ToString(), in.ToString()); @@ -298,7 +298,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex) uint256 res3; // Check that we can write successfully - BOOST_CHECK(odbw.Write(key, in2)); + odbw.Write(key, in2); BOOST_CHECK(odbw.Read(key, res3)); BOOST_CHECK_EQUAL(res3.ToString(), in2.ToString()); } @@ -310,7 +310,7 @@ BOOST_AUTO_TEST_CASE(iterator_ordering) for (int x=0x00; x<256; ++x) { uint8_t key = x; uint32_t value = x*x; - if (!(x & 1)) BOOST_CHECK(dbw.Write(key, value)); + if (!(x & 1)) dbw.Write(key, value); } // Check that creating an iterator creates a snapshot @@ -319,7 +319,7 @@ BOOST_AUTO_TEST_CASE(iterator_ordering) for (unsigned int x=0x00; x<256; ++x) { uint8_t key = x; uint32_t value = x*x; - if (x & 1) BOOST_CHECK(dbw.Write(key, value)); + if (x & 1) dbw.Write(key, value); } for (const int seek_start : {0x00, 0x80}) { @@ -381,7 +381,7 @@ BOOST_AUTO_TEST_CASE(iterator_string_ordering) for (int z = 0; z < y; ++z) key += key; uint32_t value = x*x; - BOOST_CHECK(dbw.Write(StringContentsSerializer{key}, value)); + dbw.Write(StringContentsSerializer{key}, value); } } From e030240e909493549e24aa8bcd5b382cab6e2c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:33:39 -0700 Subject: [PATCH 0047/2363] refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` Did both in this commit, since the return value of `WriteReindexing` was ignored anyway - which existed only because of the constant `Erase` being called --- src/dbwrapper.h | 3 +-- src/node/blockstorage.cpp | 5 ++--- src/node/blockstorage.h | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 50be26fb99e9..c770df8e206e 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -255,12 +255,11 @@ class CDBWrapper } template - bool Erase(const K& key, bool fSync = false) + void Erase(const K& key, bool fSync = false) { CDBBatch batch(*this); batch.Erase(key); WriteBatch(batch, fSync); - return true; } void WriteBatch(CDBBatch& batch, bool fSync = false); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 7371111ccbc5..5a54baf2b5e2 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -59,13 +59,12 @@ bool BlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo& info) return Read(std::make_pair(DB_BLOCK_FILES, nFile), info); } -bool BlockTreeDB::WriteReindexing(bool fReindexing) +void BlockTreeDB::WriteReindexing(bool fReindexing) { if (fReindexing) { Write(DB_REINDEX_FLAG, uint8_t{'1'}); - return true; } else { - return Erase(DB_REINDEX_FLAG); + Erase(DB_REINDEX_FLAG); } } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index cee0eb61ed69..7d6f78f2532a 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -55,7 +55,7 @@ class BlockTreeDB : public CDBWrapper bool WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo); bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info); bool ReadLastBlockFile(int& nFile); - bool WriteReindexing(bool fReindexing); + void WriteReindexing(bool fReindexing); void ReadReindexing(bool& fReindexing); bool WriteFlag(const std::string& name, bool fValue); bool ReadFlag(const std::string& name, bool& fValue); From 743abbcbde9e5a2db489bca461c98df461eff7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 22 Jul 2025 12:37:40 -0700 Subject: [PATCH 0048/2363] refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` --- src/node/blockstorage.cpp | 13 ++++--------- src/node/blockstorage.h | 6 +++--- src/test/fuzz/block_index.cpp | 2 +- src/validation.cpp | 4 +--- 4 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 5a54baf2b5e2..a72292cb4a96 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -78,7 +78,7 @@ bool BlockTreeDB::ReadLastBlockFile(int& nFile) return Read(DB_LAST_BLOCK, nFile); } -bool BlockTreeDB::WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo) +void BlockTreeDB::WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo) { CDBBatch batch(*this); for (const auto& [file, info] : fileInfo) { @@ -89,13 +89,11 @@ bool BlockTreeDB::WriteBatchSync(const std::vectorGetBlockHash()), CDiskBlockIndex{bi}); } WriteBatch(batch, true); - return true; } -bool BlockTreeDB::WriteFlag(const std::string& name, bool fValue) +void BlockTreeDB::WriteFlag(const std::string& name, bool fValue) { Write(std::make_pair(DB_FLAG, name), fValue ? uint8_t{'1'} : uint8_t{'0'}); - return true; } bool BlockTreeDB::ReadFlag(const std::string& name, bool& fValue) @@ -478,7 +476,7 @@ bool BlockManager::LoadBlockIndex(const std::optional& snapshot_blockha return true; } -bool BlockManager::WriteBlockIndexDB() +void BlockManager::WriteBlockIndexDB() { AssertLockHeld(::cs_main); std::vector> vFiles; @@ -494,10 +492,7 @@ bool BlockManager::WriteBlockIndexDB() m_dirty_blockindex.erase(it++); } int max_blockfile = WITH_LOCK(cs_LastBlockFile, return this->MaxBlockfileNum()); - if (!m_block_tree_db->WriteBatchSync(vFiles, max_blockfile, vBlocks)) { - return false; - } - return true; + m_block_tree_db->WriteBatchSync(vFiles, max_blockfile, vBlocks); } bool BlockManager::LoadBlockIndexDB(const std::optional& snapshot_blockhash) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 7d6f78f2532a..caf291e9f2d5 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -52,12 +52,12 @@ class BlockTreeDB : public CDBWrapper { public: using CDBWrapper::CDBWrapper; - bool WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo); + void WriteBatchSync(const std::vector>& fileInfo, int nLastFile, const std::vector& blockinfo); bool ReadBlockFileInfo(int nFile, CBlockFileInfo& info); bool ReadLastBlockFile(int& nFile); void WriteReindexing(bool fReindexing); void ReadReindexing(bool& fReindexing); - bool WriteFlag(const std::string& name, bool fValue); + void WriteFlag(const std::string& name, bool fValue); bool ReadFlag(const std::string& name, bool& fValue); bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function insertBlockIndex, const util::SignalInterrupt& interrupt) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); @@ -300,7 +300,7 @@ class BlockManager std::unique_ptr m_block_tree_db GUARDED_BY(::cs_main); - bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + void WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool LoadBlockIndexDB(const std::optional& snapshot_blockhash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp index eef8c2efc808..5bc6240cabb7 100644 --- a/src/test/fuzz/block_index.cpp +++ b/src/test/fuzz/block_index.cpp @@ -89,7 +89,7 @@ FUZZ_TARGET(block_index, .init = init_block_index) } // Store these files and blocks in the block index. It should not fail. - assert(block_index.WriteBatchSync(files_info, files_count - 1, blocks_info)); + block_index.WriteBatchSync(files_info, files_count - 1, blocks_info); // We should be able to read every block file info we stored. Its value should correspond to // what we stored above. diff --git a/src/validation.cpp b/src/validation.cpp index 7b7c8b2d5bdb..c322118574b9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2851,9 +2851,7 @@ bool Chainstate::FlushStateToDisk( { LOG_TIME_MILLIS_WITH_CATEGORY("write block index to disk", BCLog::BENCH); - if (!m_blockman.WriteBlockIndexDB()) { - return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to block index database.")); - } + m_blockman.WriteBlockIndexDB(); } // Finally remove any pruned files if (fFlushForPrune) { From 5db8cd2d37eba3ca6abc66386a3b9dc2185fa3ce Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 14 Sep 2022 13:15:42 +1000 Subject: [PATCH 0049/2363] Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h Moves FormatScriptFlags logic into GetScriptFlagNames which returns a vector of strings. For completeness, also has GetScriptFlagNames report on any bits that do not match a known script flag. --- src/deploymentinfo.h | 2 ++ src/script/interpreter.cpp | 46 +++++++++++++++++++++++++++++++++ src/script/interpreter.h | 4 +++ src/test/script_tests.cpp | 17 +++++++++++- src/test/transaction_tests.cpp | 47 +++++----------------------------- 5 files changed, 74 insertions(+), 42 deletions(-) diff --git a/src/deploymentinfo.h b/src/deploymentinfo.h index 10029b0e30a5..3178ea742885 100644 --- a/src/deploymentinfo.h +++ b/src/deploymentinfo.h @@ -8,8 +8,10 @@ #include #include +#include #include #include +#include struct VBDeploymentInfo { /** Deployment name */ diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 07fda48c493f..85cb76873f51 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -10,6 +10,7 @@ #include #include #include