From 61a01403629d5756bb479814dd10c063495c61e9 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 2 Dec 2021 13:56:51 +0100 Subject: [PATCH 1/9] Merge bitcoin/bitcoin#23642: refactor: Call type-solver earlier in decodescript 33330702081f67cb05fd86e00b252f6355249513 refactor: Call type-solver earlier in decodescript (MarcoFalke) fab0d998f4bf0f3f09afa51845d91408dd484408 style: Remove whitespace (MarcoFalke) Pull request description: The current logic is a bit confusing. First creating the `UniValue` return dict, then parsing it again to get the type as `std::string`. Clean this up by using a strong type `TxoutType`. Also, remove whitespace. ACKs for top commit: shaavan: ACK 33330702081f67cb05fd86e00b252f6355249513 theStack: Code-review ACK 33330702081f67cb05fd86e00b252f6355249513 Tree-SHA512: 49db7bc614d2491cd3ec0177d21ad1e9924dbece1eb5635290cd7fd18cb30adf4711b891daf522e7c4f6baab3033b66393bbfcd1d4726f24f90a433124f925d6 --- src/rpc/rawtransaction.cpp | 52 +++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 09e7c7d862..b4aab57602 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -869,29 +869,30 @@ static RPCHelpMan decoderawtransaction() static RPCHelpMan decodescript() { - return RPCHelpMan{"decodescript", - "\nDecode a hex-encoded script.\n", - { - {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "the hex-encoded script"}, - }, - RPCResult{ - RPCResult::Type::OBJ, "", "", + return RPCHelpMan{ + "decodescript", + "\nDecode a hex-encoded script.\n", + { + {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "the hex-encoded script"}, + }, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + {RPCResult::Type::STR, "asm", "Script public key"}, + {RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"}, + {RPCResult::Type::STR, "address", /* optional */ true, "Dash address (only if a well-defined address exists)"}, + {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Number of required signatures"}, + {RPCResult::Type::ARR, "addresses", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Array of Dash addresses", { - {RPCResult::Type::STR, "asm", "Script public key"}, - {RPCResult::Type::STR, "type", "The output type (e.g. "+GetAllOutputTypes()+")"}, - {RPCResult::Type::STR, "address", /* optional */ true, "Dash address (only if a well-defined address exists)"}, - {RPCResult::Type::NUM, "reqSigs", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Number of required signatures"}, - {RPCResult::Type::ARR, "addresses", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Array of Dash addresses", - { - {RPCResult::Type::STR, "address", "Dash address"}, - }}, - {RPCResult::Type::STR, "p2sh", "address of P2SH script wrapping this redeem script (not returned if the script is already a P2SH)"}, - } - }, - RPCExamples{ - HelpExampleCli("decodescript", "\"hexstring\"") - + HelpExampleRpc("decodescript", "\"hexstring\"") - }, + {RPCResult::Type::STR, "address", "Dash address"}, + }}, + {RPCResult::Type::STR, "p2sh", "address of P2SH script wrapping this redeem script (not returned if the script is already a P2SH)"}, + }, + }, + RPCExamples{ + HelpExampleCli("decodescript", "\"hexstring\"") + + HelpExampleRpc("decodescript", "\"hexstring\"") + }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { RPCTypeCheck(request.params, {UniValue::VSTR}); @@ -906,11 +907,10 @@ static RPCHelpMan decodescript() } ScriptPubKeyToUniv(script, r, /* fIncludeHex */ false); - UniValue type; + std::vector> solutions_data; + const TxoutType which_type{Solver(script, solutions_data)}; - type = find_value(r, "type"); - - if (type.isStr() && type.get_str() != "scripthash") { + if (which_type != TxoutType::SCRIPTHASH) { // P2SH cannot be wrapped in a P2SH. If this script is already a P2SH, // don't return the address for a P2SH of the P2SH. r.pushKV("p2sh", EncodeDestination(ScriptHash(script))); From 995cae46af3e0360dea678ab1fdd58b8e7f68b17 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 11 Oct 2021 11:27:34 +0200 Subject: [PATCH 2/9] Merge bitcoin/bitcoin#22794: test: Verify if wallet is compiled in rpc_invalid_address_message.py test c2fbdca54915e85ffafe1a88858d3c70c2b1afe8 Add BECH32_INVALID_VERSION test (lsilva01) b142f79ddb91a44f29fcb2afb7f2edf3ca17e168 skip test_getaddressinfo() if wallet is disabled (lsilva01) Pull request description: Most of `test/functional/rpc_invalid_address_message.py` does not requires wallet. But if the project is compiled in disable-wallet mode, the entire test will be skipped. This PR changes the test to run the RPC tests first and then checks if the wallet is compiled. ACKs for top commit: stratospher: tested ACK c2fbdca Tree-SHA512: 11fa2fedf4a15aa45e3f12490df8e22290a867d5de594247211499533c32289c68c0b60bd42dbf8305e43dbcc042789e7139317ef5c9f8cf386f2d84c91b9ac2 --- test/functional/rpc_invalid_address_message.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/functional/rpc_invalid_address_message.py b/test/functional/rpc_invalid_address_message.py index bb0180a0d3..59d2f55fc1 100755 --- a/test/functional/rpc_invalid_address_message.py +++ b/test/functional/rpc_invalid_address_message.py @@ -22,9 +22,6 @@ class InvalidAddressErrorMessageTest(BitcoinTestFramework): self.setup_clean_chain = True self.num_nodes = 1 - def skip_test_if_missing_module(self): - self.skip_if_no_wallet() - def test_validateaddress(self): node = self.nodes[0] @@ -50,7 +47,10 @@ class InvalidAddressErrorMessageTest(BitcoinTestFramework): def run_test(self): self.test_validateaddress() - self.test_getaddressinfo() + + if self.is_wallet_compiled(): + self.init_wallet(0) + self.test_getaddressinfo() if __name__ == '__main__': From 66e77f78790b2a301133a4fa01cf9d6847b00438 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 20 Oct 2021 17:43:51 +0200 Subject: [PATCH 3/9] Merge bitcoin/bitcoin#23316: test: make the node param explicit in init_wallet() 7b3c9e4ee8feb552dc0fc4347db2d06e60894a9f Make explicit the node param in init_wallet() (lsilva01) Pull request description: This PR changes the definition of `def init_wallet(self, i)` to `def init_wallet(self, *, node)` to make the node parameter explicit, as suggested in https://github.com/bitcoin/bitcoin/pull/22794#discussion_r713287448 . ACKs for top commit: stratospher: tested ACK 7b3c9e4. Tree-SHA512: 2ef036f4c2110b2f7dc893dc6eea8faa0a18edd7f8f59b25460a6c544df7238175ddd6a0d766e2bb206326b1c9afc84238c75613a0f01eeda89a8ccb7d86a4f1 --- test/functional/rpc_invalid_address_message.py | 2 +- test/functional/test_framework/test_framework.py | 8 ++++---- test/functional/wallet_backup.py | 6 +++--- test/functional/wallet_listdescriptors.py | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/test/functional/rpc_invalid_address_message.py b/test/functional/rpc_invalid_address_message.py index 59d2f55fc1..18d85d6770 100755 --- a/test/functional/rpc_invalid_address_message.py +++ b/test/functional/rpc_invalid_address_message.py @@ -49,7 +49,7 @@ class InvalidAddressErrorMessageTest(BitcoinTestFramework): self.test_validateaddress() if self.is_wallet_compiled(): - self.init_wallet(0) + self.init_wallet(node=0) self.test_getaddressinfo() diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 6aa7a4a53c..76d7a5bfed 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -469,12 +469,12 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): def import_deterministic_coinbase_privkeys(self): for i in range(len(self.nodes)): - self.init_wallet(i) + self.init_wallet(node=i) - def init_wallet(self, i): - wallet_name = self.default_wallet_name if self.wallet_names is None else self.wallet_names[i] if i < len(self.wallet_names) else False + def init_wallet(self, *, node): + wallet_name = self.default_wallet_name if self.wallet_names is None else self.wallet_names[node] if node < len(self.wallet_names) else False if wallet_name is not False: - n = self.nodes[i] + n = self.nodes[node] if wallet_name is not None: n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True) n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase', rescan=True) diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 9371cee0f5..5e188dce4c 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -135,9 +135,9 @@ class WalletBackupTest(BitcoinTestFramework): assert os.path.exists(wallet_file) def init_three(self): - self.init_wallet(0) - self.init_wallet(1) - self.init_wallet(2) + self.init_wallet(node=0) + self.init_wallet(node=1) + self.init_wallet(node=2) def run_test(self): self.log.info("Generating initial blockchain") diff --git a/test/functional/wallet_listdescriptors.py b/test/functional/wallet_listdescriptors.py index b6c82e64e9..166443c525 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -23,7 +23,7 @@ class ListDescriptorsTest(BitcoinTestFramework): self.skip_if_no_sqlite() # do not create any wallet by default - def init_wallet(self, i): + def init_wallet(self, *, node): return def run_test(self): From b212ca051542ba3c4a336b1d31c92a51342d32a2 Mon Sep 17 00:00:00 2001 From: laanwj <126646+laanwj@users.noreply.github.com> Date: Mon, 28 Feb 2022 13:00:14 +0100 Subject: [PATCH 4/9] Merge bitcoin/bitcoin#24365: wallet: Don't generate keys for wallets with private keys disabled during upgradewallet c7376cc8d728f3a7c40f79bd57e7cef685def723 tests: Test upgrading wallet with privkeys disabled (Andrew Chow) 3d985d4f43b5344f998bcf6db22d02782e647a2a wallet: Don't generate keys when privkeys disabled when upgrading (Andrew Chow) Pull request description: When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled. Fixes #23610 ACKs for top commit: laanwj: Code review ACK c7376cc8d728f3a7c40f79bd57e7cef685def723 benthecarman: tACK c7376cc8d728f3a7c40f79bd57e7cef685def723 this fixed the issue for me Tree-SHA512: fa07cf37df9196ff98671bb1ce5c9aa0bab46495066b4dab796d7e8e5d5c7adb414ff56adae4fd3e15658a610995bd19a9e1edb00c46144b0df635c5b343f3a6 --- test/functional/wallet_upgradewallet.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index cc2b4168c2..41547353ec 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -239,5 +239,16 @@ class UpgradeWalletTest(BitcoinTestFramework): desc_wallet = self.nodes[0].get_wallet_rpc("desc_upgrade") self.test_upgradewallet(desc_wallet, previous_version=120200, expected_version=120200) + self.log.info("Checking that descriptor wallets without privkeys do nothing, successfully") + self.nodes[0].createwallet(wallet_name="desc_upgrade_nopriv", descriptors=True, disable_private_keys=True) + desc_wallet = self.nodes[0].get_wallet_rpc("desc_upgrade_nopriv") + self.test_upgradewallet(desc_wallet, previous_version=120200, expected_version=120200) + + if self.is_bdb_compiled(): + self.log.info("Upgrading a wallet with private keys disabled") + self.nodes[0].createwallet(wallet_name="privkeys_disabled_upgrade", disable_private_keys=True, descriptors=False) + disabled_wallet = self.nodes[0].get_wallet_rpc("privkeys_disabled_upgrade") + self.test_upgradewallet(disabled_wallet, previous_version=120200, expected_version=120200) + if __name__ == '__main__': UpgradeWalletTest().main() From f16265dd50ffc46998d082fd4fa397f34da93406 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 12 Jan 2022 14:56:52 +0200 Subject: [PATCH 5/9] Merge bitcoin-core/gui#517: refactor, qt: Use std::chrono for parameters of QTimer methods 51250b0906e56b39488304208ad119c951b4ae7d refactor, qt: Use std::chrono for input_filter_delay constant (Hennadii Stepanov) f3bdc143b67e8a5e763071a0774f6d994ca35c57 refactor, qt: Add SHUTDOWN_POLLING_DELAY constant (Hennadii Stepanov) 0e193deb523a4fa04e0ee69bd66f917895802ac9 refactor, qt: Use std::chrono for non-zero arguments in QTimer methods (Hennadii Stepanov) 6f0da958116ecc0e06332fad2f490e37b6884166 refactor, qt: Use std::chrono in ConfirmMessage parameter (Hennadii Stepanov) 33d520ac538fcd6285fd958578f1bd26295592e4 refactor, qt: Use std::chrono for MODEL_UPDATE_DELAY constant (Hennadii Stepanov) Pull request description: Since Qt 5.8 `QTimer` methods have overloads that accept `std::chrono::milliseconds` arguments: - [`QTimer::singleShot`](https://doc.qt.io/archives/qt-5.9/qtimer.html#singleShot-8) - [`QTimer::start`](https://doc.qt.io/archives/qt-5.9/qtimer.html#start-2) ACKs for top commit: promag: Code review ACK 51250b0906e56b39488304208ad119c951b4ae7d. shaavan: reACK 51250b0906e56b39488304208ad119c951b4ae7d Tree-SHA512: aa843bb2322a84c0c2bb113d3b48d7bf02d7f09a770779dcde312c32887f973ef9445cdef42f39edaa599ff0f3d0457454f6153aa130efadd989e413d39c6062 --- src/qt/bitcoin.cpp | 5 +++-- src/qt/clientmodel.cpp | 3 ++- src/qt/guiconstants.h | 10 ++++++++-- src/qt/optionsdialog.cpp | 4 +++- src/qt/sendcoinsdialog.cpp | 5 +++-- src/qt/test/addressbooktests.cpp | 4 +++- src/qt/test/util.cpp | 4 +++- src/qt/test/util.h | 8 ++++++-- src/qt/test/wallettests.cpp | 1 + src/qt/transactionview.cpp | 5 +++-- src/qt/walletcontroller.cpp | 5 +++-- 11 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index debab9e2a2..b251da5d02 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -44,6 +44,7 @@ #endif // ENABLE_WALLET #include +#include #include #include @@ -398,10 +399,10 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead connect(paymentServer, &PaymentServer::message, [this](const QString& title, const QString& message, unsigned int style) { window->message(title, message, style); }); - QTimer::singleShot(100, paymentServer, &PaymentServer::uiReady); + QTimer::singleShot(100ms, paymentServer, &PaymentServer::uiReady); } #endif - pollShutdownTimer->start(200); + pollShutdownTimer->start(SHUTDOWN_POLLING_DELAY); } else { Q_EMIT splashFinished(); // Make sure splash screen doesn't stick around during shutdown quit(); // Exit first main loop invocation diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index d604e3ac79..5dd3e8198a 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -323,7 +324,7 @@ static void BlockTipChanged(ClientModel* clientmodel, SynchronizationState sync_ const bool throttle = (sync_state != SynchronizationState::POST_INIT && !fHeader) || sync_state == SynchronizationState::INIT_REINDEX; const int64_t now = throttle ? GetTimeMillis() : 0; int64_t& nLastUpdateNotification = fHeader ? nLastHeaderTipUpdateNotification : nLastBlockTipUpdateNotification; - if (throttle && now < nLastUpdateNotification + MODEL_UPDATE_DELAY) { + if (throttle && now < nLastUpdateNotification + count_milliseconds(MODEL_UPDATE_DELAY)) { return; } diff --git a/src/qt/guiconstants.h b/src/qt/guiconstants.h index 87f47b023f..4dd734b4d4 100644 --- a/src/qt/guiconstants.h +++ b/src/qt/guiconstants.h @@ -6,10 +6,16 @@ #ifndef BITCOIN_QT_GUICONSTANTS_H #define BITCOIN_QT_GUICONSTANTS_H +#include #include -/* Milliseconds between model updates */ -static const int MODEL_UPDATE_DELAY = 250; +using namespace std::chrono_literals; + +/* A delay between model updates */ +static constexpr auto MODEL_UPDATE_DELAY{250ms}; + +/* A delay between shutdown pollings */ +static constexpr auto SHUTDOWN_POLLING_DELAY{200ms}; /* AskPassphraseDialog -- Maximum passphrase length */ static const int MAX_PASSPHRASE_SIZE = 1024; diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp index 08da36cff8..1b537a9411 100644 --- a/src/qt/optionsdialog.cpp +++ b/src/qt/optionsdialog.cpp @@ -24,6 +24,8 @@ #include #include +#include + #include #include #include @@ -457,7 +459,7 @@ void OptionsDialog::showRestartWarning(bool fPersistent) ui->statusLabel->setText(tr("This change would require a client restart.")); // clear non-persistent status label after 10 seconds // Todo: should perhaps be a class attribute, if we extend the use of statusLabel - QTimer::singleShot(10000, this, &OptionsDialog::clearStatusLabel); + QTimer::singleShot(10s, this, &OptionsDialog::clearStatusLabel); } } diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 1319d1d836..79526f9381 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -24,10 +24,11 @@ #include #include #include +#include #include #include #include -#include +#include #include #include @@ -1080,7 +1081,7 @@ SendConfirmationDialog::SendConfirmationDialog(const QString& title, const QStri int SendConfirmationDialog::exec() { updateYesButton(); - countDownTimer.start(1000); + countDownTimer.start(1s); return QMessageBox::exec(); } diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 5eb00a1c32..3e0be6da11 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -19,6 +19,8 @@ #include #include +#include + #include #include #include @@ -39,7 +41,7 @@ void EditAddressAndSubmit( dialog->findChild("labelEdit")->setText(label); dialog->findChild("addressEdit")->setText(address); - ConfirmMessage(&warning_text, 5); + ConfirmMessage(&warning_text, 5ms); dialog->accept(); QCOMPARE(warning_text, expected_msg); } diff --git a/src/qt/test/util.cpp b/src/qt/test/util.cpp index 987d921f03..635dbcd1c5 100644 --- a/src/qt/test/util.cpp +++ b/src/qt/test/util.cpp @@ -2,6 +2,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include + #include #include #include @@ -9,7 +11,7 @@ #include #include -void ConfirmMessage(QString* text, int msec) +void ConfirmMessage(QString* text, std::chrono::milliseconds msec) { QTimer::singleShot(msec, [text]() { for (QWidget* widget : QApplication::topLevelWidgets()) { diff --git a/src/qt/test/util.h b/src/qt/test/util.h index df5931a032..f50a6b6c61 100644 --- a/src/qt/test/util.h +++ b/src/qt/test/util.h @@ -5,7 +5,11 @@ #ifndef BITCOIN_QT_TEST_UTIL_H #define BITCOIN_QT_TEST_UTIL_H -#include +#include + +QT_BEGIN_NAMESPACE +class QString; +QT_END_NAMESPACE /** * Press "Ok" button in message box dialog. @@ -13,6 +17,6 @@ * @param text - Optionally store dialog text. * @param msec - Number of milliseconds to pause before triggering the callback. */ -void ConfirmMessage(QString* text = nullptr, int msec = 0); +void ConfirmMessage(QString* text, std::chrono::milliseconds msec); #endif // BITCOIN_QT_TEST_UTIL_H diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 3a8b297037..fe16db6c4e 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 55760147c5..fdc2e5cb85 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -114,8 +115,8 @@ TransactionView::TransactionView(QWidget* parent) : amountWidget->setObjectName("amountWidget"); hlayout->addWidget(amountWidget); - // Delay before filtering transactions in ms - static const int input_filter_delay = 200; + // Delay before filtering transactions + static constexpr auto input_filter_delay{200ms}; QTimer* amount_typing_delay = new QTimer(this); amount_typing_delay->setSingleShot(true); diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 2133999b9b..b39f630ccb 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -271,12 +272,12 @@ void CreateWalletActivity::createWallet() flags |= WALLET_FLAG_DESCRIPTORS; } - QTimer::singleShot(500, worker(), [this, name, flags] { + QTimer::singleShot(500ms, worker(), [this, name, flags] { std::unique_ptr wallet = node().walletLoader().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message); if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet)); - QTimer::singleShot(500, this, &CreateWalletActivity::finish); + QTimer::singleShot(500ms, this, &CreateWalletActivity::finish); }); } From 11eeae2ab9fcf97f355bd73d9194708d6e79a136 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 2 Feb 2022 15:00:19 +0100 Subject: [PATCH 6/9] Merge bitcoin/bitcoin#24219: Fix implicit-integer-sign-change in bloom fad84a25956ec081f22aebbda309d168a3dc0004 refactor: Fixup uint64_t-cast style in touched line (MarcoFalke) fa041878de786f5be74ec74a06ec407c99ca8656 Fix implicit-integer-sign-change in bloom (MarcoFalke) Pull request description: Signed values don't really make sense when using `std::vector::operator[]`. Fix that and remove the suppression. ACKs for top commit: PastaPastaPasta: utACK fad84a25956ec081f22aebbda309d168a3dc0004 theStack: Code-review ACK fad84a25956ec081f22aebbda309d168a3dc0004 Tree-SHA512: 7139dd9aa098c41e4af1b6e63dd80e71a92b0a98062d1676b01fe550ffa8e21a5f84a578afa7a536d70dad1b8a5017625e3a9e2dda6f864b452ec77b130ddf2a --- src/common/bloom.cpp | 6 +++--- test/sanitizer_suppressions/ubsan | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/common/bloom.cpp b/src/common/bloom.cpp index 6fc86cf72a..315258fb14 100644 --- a/src/common/bloom.cpp +++ b/src/common/bloom.cpp @@ -314,8 +314,8 @@ void CRollingBloomFilter::insert(Span vKey) /* FastMod works with the upper bits of h, so it is safe to ignore that the lower bits of h are already used for bit. */ uint32_t pos = FastRange32(h, data.size()); /* The lowest bit of pos is ignored, and set to zero for the first bit, and to one for the second. */ - data[pos & ~1] = (data[pos & ~1] & ~(((uint64_t)1) << bit)) | ((uint64_t)(nGeneration & 1)) << bit; - data[pos | 1] = (data[pos | 1] & ~(((uint64_t)1) << bit)) | ((uint64_t)(nGeneration >> 1)) << bit; + data[pos & ~1U] = (data[pos & ~1U] & ~(uint64_t{1} << bit)) | (uint64_t(nGeneration & 1)) << bit; + data[pos | 1] = (data[pos | 1] & ~(uint64_t{1} << bit)) | (uint64_t(nGeneration >> 1)) << bit; } } @@ -326,7 +326,7 @@ bool CRollingBloomFilter::contains(Span vKey) const int bit = h & 0x3F; uint32_t pos = FastRange32(h, data.size()); /* If the relevant bit is not set in either data[pos & ~1] or data[pos | 1], the filter does not contain vKey */ - if (!(((data[pos & ~1] | data[pos | 1]) >> bit) & 1)) { + if (!(((data[pos & ~1U] | data[pos | 1]) >> bit) & 1)) { return false; } } diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 42df86bbdd..ba08cef801 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -48,7 +48,6 @@ implicit-integer-sign-change:*/include/c++/ implicit-integer-sign-change:*/new_allocator.h implicit-integer-sign-change:addrman.h implicit-integer-sign-change:bech32.cpp -implicit-integer-sign-change:common/bloom.cpp implicit-integer-sign-change:chain.cpp implicit-integer-sign-change:chain.h implicit-integer-sign-change:compat/stdin.cpp From 2a2a2693d0fd429f6aa193809b7905e77aada25e Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Wed, 13 Oct 2021 13:48:36 +0200 Subject: [PATCH 7/9] Merge bitcoin/bitcoin#23253: bitcoin-tx: Reject non-integral and out of range int strings fa6f29de516c7af5206b91b59ada466032329250 bitcoin-tx: Reject non-integral and out of range multisig numbers (MarcoFalke) fafab8ea5e6ed6b87fac57a5cd16a8135236cdd6 bitcoin-tx: Reject non-integral and out of range sequence ids (MarcoFalke) fa53d3d8266ad0257315d07b71b4f8a711134622 test: Check that bitcoin-tx accepts whitespace around sequence id and multisig numbers (MarcoFalke) Pull request description: Seems odd to silently accept arbitrary strings that don't even represent integral values. Fix that. ACKs for top commit: practicalswift: cr ACK fa6f29de516c7af5206b91b59ada466032329250 laanwj: Code review ACK fa6f29de516c7af5206b91b59ada466032329250 Empact: Code review ACK https://github.com/bitcoin/bitcoin/pull/23253/commits/fa6f29de516c7af5206b91b59ada466032329250 promag: Code review ACK fa6f29de516c7af5206b91b59ada466032329250. Tree-SHA512: e31f7f21fe55ac069e755557bdbcae8d5d29e20ff82e441ebdfc65153e3a31a4edd46ad3e6dea5190ecbd1b8ea5a8f94daa5d59a3b7558e46e794e30db0e6c79 --- src/bitcoin-tx.cpp | 19 ++++++++--- test/util/data/bitcoin-util-test.json | 48 +++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 0eb122f086..6f98a71622 100644 --- a/src/bitcoin-tx.cpp +++ b/src/bitcoin-tx.cpp @@ -220,6 +220,16 @@ static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal) tx.nLockTime = (unsigned int) newLocktime; } +template +static T TrimAndParse(const std::string& int_str, const std::string& err) +{ + const auto parsed{ToIntegral(TrimString(int_str))}; + if (!parsed.has_value()) { + throw std::runtime_error(err + " '" + int_str + "'"); + } + return parsed.value(); +} + static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInput) { std::vector vStrInputParts = SplitString(strInput, ':'); @@ -245,8 +255,9 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu // extract the optional sequence number uint32_t nSequenceIn = CTxIn::SEQUENCE_FINAL; - if (vStrInputParts.size() > 2) - nSequenceIn = std::stoul(vStrInputParts[2]); + if (vStrInputParts.size() > 2) { + nSequenceIn = TrimAndParse(vStrInputParts.at(2), "invalid TX sequence id"); + } // append to transaction input list CTxIn txin(txid, vout, CScript(), nSequenceIn); @@ -324,10 +335,10 @@ static void MutateTxAddOutMultiSig(CMutableTransaction& tx, const std::string& s CAmount value = ExtractAndValidateValue(vStrInputParts[0]); // Extract REQUIRED - uint32_t required = stoul(vStrInputParts[1]); + const uint32_t required{TrimAndParse(vStrInputParts.at(1), "invalid multisig required number")}; // Extract NUMKEYS - uint32_t numkeys = stoul(vStrInputParts[2]); + const uint32_t numkeys{TrimAndParse(vStrInputParts.at(2), "invalid multisig total number")}; // Validate there are the correct number of pubkeys if (vStrInputParts.size() < numkeys + 3) diff --git a/test/util/data/bitcoin-util-test.json b/test/util/data/bitcoin-util-test.json index 5d78d2edb1..4722f684f1 100644 --- a/test/util/data/bitcoin-util-test.json +++ b/test/util/data/bitcoin-util-test.json @@ -425,6 +425,30 @@ "output_cmp": "txcreatedata2.json", "description": "Creates a new transaction with one input, one address output and one data (zero value) output (output in json)" }, + { "exec": "./dash-tx", + "args": + ["-create", + "in=5897de6bd6027a475eadd57019d4e6872c396d0716c4875a5f1a6fcfdf385c1f:0:11aa"], + "return_code": 1, + "error_txt": "error: invalid TX sequence id '11aa'", + "description": "Try to parse a sequence number outside the allowed range" + }, + { "exec": "./dash-tx", + "args": + ["-create", + "in=5897de6bd6027a475eadd57019d4e6872c396d0716c4875a5f1a6fcfdf385c1f:0:-1"], + "return_code": 1, + "error_txt": "error: invalid TX sequence id '-1'", + "description": "Try to parse a sequence number outside the allowed range" + }, + { "exec": "./dash-tx", + "args": + ["-create", + "in=5897de6bd6027a475eadd57019d4e6872c396d0716c4875a5f1a6fcfdf385c1f:0:4294967296"], + "return_code": 1, + "error_txt": "error: invalid TX sequence id '4294967296'", + "description": "Try to parse a sequence number outside the allowed range" + }, { "exec": "./dash-tx", "args": ["-create", @@ -433,6 +457,14 @@ "output_cmp": "txcreatedata_seq0.hex", "description": "Creates a new transaction with one input with sequence number and one address output" }, + { "exec": "./dash-tx", + "args": + ["-create", + "in=5897de6bd6027a475eadd57019d4e6872c396d0716c4875a5f1a6fcfdf385c1f:0: 4294967293 ", + "outaddr=0.18:XijDvbYpPmznwgpWD3DkdYNfGmRP2KoVSk"], + "output_cmp": "txcreatedata_seq0.hex", + "description": "Creates a new transaction with one input with sequence number (+whitespace) and one address output" + }, { "exec": "./dash-tx", "args": ["-json", @@ -457,15 +489,27 @@ "output_cmp": "txcreatedata_seq1.json", "description": "Adds a new input with sequence number to a transaction (output in json)" }, + { "exec": "./dash-tx", + "args": ["-create", "outmultisig=1:-2:3:02a5:021:02df", "nversion=1"], + "return_code": 1, + "error_txt": "error: invalid multisig required number '-2'", + "description": "Try to parse a multisig number outside the allowed range" + }, + { "exec": "./dash-tx", + "args": ["-create", "outmultisig=1:2:3a:02a5:021:02df", "nversion=1"], + "return_code": 1, + "error_txt": "error: invalid multisig total number '3a'", + "description": "Try to parse a multisig number outside the allowed range" + }, { "exec": "./dash-tx", "args": ["-create", "outmultisig=1:2:3:02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397:021ac43c7ff740014c3b33737ede99c967e4764553d1b2b83db77c83b8715fa72d:02df2089105c77f266fa11a9d33f05c735234075f2e8780824c6b709415f9fb485", "nversion=1"], "output_cmp": "txcreatemultisig1.hex", "description": "Creates a new transaction with a single 2-of-3 multisig output" }, { "exec": "./dash-tx", - "args": ["-json", "-create", "outmultisig=1:2:3:02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397:021ac43c7ff740014c3b33737ede99c967e4764553d1b2b83db77c83b8715fa72d:02df2089105c77f266fa11a9d33f05c735234075f2e8780824c6b709415f9fb485", "nversion=1"], + "args": ["-json", "-create", "outmultisig=1: 2: 3:02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397:021ac43c7ff740014c3b33737ede99c967e4764553d1b2b83db77c83b8715fa72d:02df2089105c77f266fa11a9d33f05c735234075f2e8780824c6b709415f9fb485", "nversion=1"], "output_cmp": "txcreatemultisig1.json", - "description": "Creates a new transaction with a single 2-of-3 multisig output (output in json)" + "description": "Creates a new transaction with a single 2-of-3 multisig output (with whitespace, output in json)" }, { "exec": "./dash-tx", "args": ["-create", "outmultisig=1:2:3:02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397:021ac43c7ff740014c3b33737ede99c967e4764553d1b2b83db77c83b8715fa72d:02df2089105c77f266fa11a9d33f05c735234075f2e8780824c6b709415f9fb485:S", "nversion=1"], From f147373a324a22383f86684ef87c3ea0f3ea5da2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 28 Feb 2022 08:34:27 +0100 Subject: [PATCH 8/9] Merge bitcoin/bitcoin#24449: fuzz: FuzzedFileProvider::write should not return negative value fc471814dc34abb4d5479803ebb1033b572eda43 fuzz: FuzzedFileProvider::write should not return negative value (eugene) Pull request description: Doing so can lead to a glibc crash (from 2005 but I think it's relevant https://sourceware.org/bugzilla/show_bug.cgi?id=2074). Also the manpage for fopencookie warns against this: https://man7.org/linux/man-pages/man3/fopencookie.3.html. This would invalidate the autofile seeds (and maybe others?) in qa-assets. On another note, I noticed that FuzzedFileProvider::seek has some confusing behavior with SEEK_END. It seems to me that if these handlers are supposed to mimic the real functions, that SEEK_END would use the offset from the end of the stream, rather than changing the offset with a random value between 0 and 4096. I could also open a PR to fix SEEK_END, but it would invalidate the seeds. ACKs for top commit: MarcoFalke: cr ACK fc471814dc34abb4d5479803ebb1033b572eda43 Tree-SHA512: 9db41637f0df7f2b2407b82531cbc34f4ba9393063b63ec6786372e808fe991f7f24df45936c203fe0f9fc49686180c65ad57c2ce7d49e0c5402240616bcfede --- src/test/fuzz/util.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index bd55d99f97..ddfbc97dbf 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -456,7 +456,7 @@ ssize_t FuzzedFileProvider::write(void* cookie, const char* buf, size_t size) SetFuzzedErrNo(fuzzed_file->m_fuzzed_data_provider); const ssize_t n = fuzzed_file->m_fuzzed_data_provider.ConsumeIntegralInRange(0, size); if (AdditionOverflow(fuzzed_file->m_offset, (int64_t)n)) { - return fuzzed_file->m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; + return 0; } fuzzed_file->m_offset += n; return n; From 39316088585984faf4ea422745e64f0adef916f8 Mon Sep 17 00:00:00 2001 From: merge-script Date: Tue, 14 Sep 2021 11:10:08 +0200 Subject: [PATCH 9/9] Merge bitcoin/bitcoin#22543: test: Use MiniWallet in mempool_limit.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 08634e82c68ea1be79e1395f4f551082f497023f fix typos in logging messages (ShubhamPalriwala) d447ded6babebe7c7948e585c9e78bf34dbef226 replace: self.nodes[0] with node (ShubhamPalriwala) dddca3899c4738e512313a85aeb006310e34e31f test: use MiniWallet in mempool_limit.py (ShubhamPalriwala) Pull request description: This is a PR proposed in #20078 This PR enables running another non-wallet functional test even when the wallet is disabled thanks to the MiniWallet, i.e. it can be run even when bitcoin-core is compiled with --disable-wallet. It also includes changes in wallet.py in the form of a new method, `create_large_transactions()` for the MiniWallet to create large transactions. Efforts for this feature started in #20874 but were not continued and that PR was closed hence I picked this up. To test this PR locally, compile and build bitcoin-core without the wallet and run: ``` $ test/functional/mempool_limit.py ``` ACKs for top commit: amitiuttarwar: ACK 08634e8, only git changes since last push (and one new line). Zero-1729: ACK 08634e82c68ea1be79e1395f4f551082f497023f 🧉 Tree-SHA512: 0f744ad26bf7a5a784aac1ed5077b59c95a36d1ff3ad0087ffd10ac8d5979f7362c63c20c2ce2bfa650fda02dfbcd60b1fceee049a2465c8d221cce51c20369f --- test/functional/mempool_limit.py | 81 +++++++++++++++++--------------- 1 file changed, 44 insertions(+), 37 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 3229537737..01e2add039 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -6,8 +6,11 @@ from decimal import Decimal +from test_framework.blocktools import COINBASE_MATURITY from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, assert_greater_than, assert_raises_rpc_error, create_confirmed_utxos, create_lots_of_big_transactions, gen_return_txouts +from test_framework.util import assert_equal, assert_greater_than, assert_raises_rpc_error, gen_return_txouts +from test_framework.wallet import MiniWallet + class MempoolLimitTest(BitcoinTestFramework): def set_test_params(self): @@ -20,55 +23,59 @@ class MempoolLimitTest(BitcoinTestFramework): ]] self.supports_cli = False - def skip_test_if_missing_module(self): - self.skip_if_no_wallet() + def send_large_txs(self, node, miniwallet, txouts, fee_rate, tx_batch_size): + for _ in range(tx_batch_size): + tx = miniwallet.create_self_transfer(from_node=node, fee_rate=fee_rate)['tx'] + for txout in txouts: + tx.vout.append(txout) + miniwallet.sendrawtransaction(from_node=node, tx_hex=tx.serialize().hex()) def run_test(self): txouts = gen_return_txouts() - relayfee = self.nodes[0].getnetworkinfo()['relayfee'] + node=self.nodes[0] + miniwallet = MiniWallet(node) + relayfee = node.getnetworkinfo()['relayfee'] - self.log.info('Check that mempoolminfee is minrelytxfee') - assert_equal(self.nodes[0].getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) - assert_equal(self.nodes[0].getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + self.log.info('Check that mempoolminfee is minrelaytxfee') + assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) + assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - txids = [] - utxos = create_confirmed_utxos(self, relayfee, self.nodes[0], 491) + tx_batch_size = 25 + num_of_batches = 3 + # Generate UTXOs to flood the mempool + # 1 to create a tx initially that will be evicted from the mempool later + # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO + # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee + self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size) + 1) + + # Mine 99 blocks so that the UTXOs are allowed to be spent + self.generate(node, COINBASE_MATURITY - 1) self.log.info('Create a mempool tx that will be evicted') - us0 = utxos.pop() - inputs = [{ "txid" : us0["txid"], "vout" : us0["vout"]}] - outputs = {self.nodes[0].getnewaddress() : 0.0001} - tx = self.nodes[0].createrawtransaction(inputs, outputs) - self.nodes[0].settxfee(relayfee) # specifically fund this tx with low fee - txF = self.nodes[0].fundrawtransaction(tx) - self.nodes[0].settxfee(0) # return to automatic fee selection - txFS = self.nodes[0].signrawtransactionwithwallet(txF['hex']) - txid = self.nodes[0].sendrawtransaction(txFS['hex']) + tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"] - relayfee = self.nodes[0].getnetworkinfo()['relayfee'] - base_fee = relayfee*100 - for i in range (3): - txids.append([]) - txids[i] = create_lots_of_big_transactions(self.nodes[0], txouts, utxos[30*i:30*i+30], 30, (i+1)*base_fee) + # Increase the tx fee rate massively to give the subsequent transactions a higher priority in the mempool + base_fee = relayfee * 1000 + + self.log.info("Fill up the mempool with txs with higher fee rate") + for batch_of_txid in range(num_of_batches): + fee_rate=(batch_of_txid + 1) * base_fee + self.send_large_txs(node, miniwallet, txouts, fee_rate, tx_batch_size) self.log.info('The tx should be evicted by now') - assert txid not in self.nodes[0].getrawmempool() - txdata = self.nodes[0].gettransaction(txid) - assert txdata['confirmations'] == 0 #confirmation should still be 0 + # The number of transactions created should be greater than the ones present in the mempool + assert_greater_than(tx_batch_size * num_of_batches, len(node.getrawmempool())) + # Initial tx created should not be present in the mempool anymore as it had a lower fee rate + assert tx_to_be_evicted_id not in node.getrawmempool() - self.log.info('Check that mempoolminfee is larger than minrelytxfee') - assert_equal(self.nodes[0].getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) - assert_greater_than(self.nodes[0].getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + self.log.info('Check that mempoolminfee is larger than minrelaytxfee') + assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) + assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + # Deliberately try to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool self.log.info('Create a mempool tx that will not pass mempoolminfee') - us0 = utxos.pop() - inputs = [{ "txid" : us0["txid"], "vout" : us0["vout"]}] - outputs = {self.nodes[0].getnewaddress() : 0.0001} - tx = self.nodes[0].createrawtransaction(inputs, outputs) - # specifically fund this tx with a fee < mempoolminfee, >= than minrelaytxfee - txF = self.nodes[0].fundrawtransaction(tx, {'feeRate': relayfee}) - txFS = self.nodes[0].signrawtransactionwithwallet(txF['hex']) - assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, txFS['hex']) + assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee, mempool_valid=False) + if __name__ == '__main__': MempoolLimitTest().main()