From ca35efc777ab558bf5e0e29207e17c018c0eaacd 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 | 57 ++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 1b01eb3195d9e..9695e4fee2b00 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -869,29 +869,33 @@ 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, "", "", - { - {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\"") - }, + 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, "The Bitcoin address (only if a well-defined address exists)"}, + {RPCResult::Type::STR, "p2sh", /* optional */ true, "address of P2SH script wrapping this redeem script (not returned if the script is already a P2SH)"}, + {RPCResult::Type::OBJ, "segwit", /* optional */ true, "Result of a witness script public key wrapping this redeem script (not returned if the script is a P2SH or witness)", + { + {RPCResult::Type::STR, "asm", "String representation of the script public key"}, + {RPCResult::Type::STR_HEX, "hex", "Hex string of the script public key"}, + {RPCResult::Type::STR, "type", "The type of the script public key (e.g. witness_v0_keyhash or witness_v0_scripthash)"}, + {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"}, + {RPCResult::Type::STR, "p2sh-segwit", "address of the P2SH script wrapping this witness redeem script"}, + }}, + }, + }, + RPCExamples{ + HelpExampleCli("decodescript", "\"hexstring\"") + + HelpExampleRpc("decodescript", "\"hexstring\"") + }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { RPCTypeCheck(request.params, {UniValue::VSTR}); @@ -906,11 +910,10 @@ static RPCHelpMan decodescript() } ScriptPubKeyToUniv(script, r, /* fIncludeHex */ false); - UniValue type; - - type = find_value(r, "type"); + std::vector> solutions_data; + const TxoutType which_type{Solver(script, solutions_data)}; - 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 b7a66fc40ff6ed5e345b1a819334e079a3d415fc 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 bb0180a0d3725..59d2f55fc1d3f 100755 --- a/test/functional/rpc_invalid_address_message.py +++ b/test/functional/rpc_invalid_address_message.py @@ -22,9 +22,6 @@ def set_test_params(self): 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 @@ def test_getaddressinfo(self): 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 eb0f5a9e2bfaf6cdf10b465bbb19b4ac3429263a 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 59d2f55fc1d3f..18d85d6770306 100755 --- a/test/functional/rpc_invalid_address_message.py +++ b/test/functional/rpc_invalid_address_message.py @@ -49,7 +49,7 @@ def run_test(self): 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 3669f7f7cdac3..ece5401560304 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -468,12 +468,12 @@ def setup_nodes(self): 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 9371cee0f5223..5e188dce4ca5a 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -135,9 +135,9 @@ def restore_wallet_existent_name(self): 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 8896a765f0918..640a3793a6cef 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -23,7 +23,7 @@ def skip_test_if_missing_module(self): 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 270e87bd91aeba21bf46ad47aebb762faade8887 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 a662fbd387697..7ab06dfefaf6d 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -233,5 +233,16 @@ def copy_non_hd(): assert_equal(120200, wallet.getwalletinfo()['walletversion']) + 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=169900, expected_version=169900) + + 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=169900, expected_version=169900) + if __name__ == '__main__': UpgradeWalletTest().main() From 7b4af86fe0f60cd337bccf2e98bd919a5f53fcdb 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 | 6 +++--- src/qt/walletcontroller.cpp | 5 +++-- 11 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index af50ae0129b48..4134a61153903 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -44,6 +44,7 @@ #include #include +#include #include #include @@ -472,10 +473,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 ea1df1ac6b0e0..5243aec011523 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 87f47b023fe41..4dd734b4d4d43 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 a5587373bb90e..136973d9b1da1 100644 --- a/src/qt/optionsdialog.cpp +++ b/src/qt/optionsdialog.cpp @@ -24,6 +24,8 @@ #include #include +#include + #include #include #include @@ -454,7 +456,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 1319d1d83684d..79526f938174d 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 5eb00a1c32ce8..3e0be6da11a69 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 987d921f036d4..635dbcd1c584e 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 df5931a0329ee..f50a6b6c6178a 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 3a8b297037ca1..fe16db6c4e7e7 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 80e030c24f1be..0f5bf99786af3 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -21,9 +21,9 @@ #include #include +#include #include #include -#include #include #include #include @@ -112,8 +112,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 2133999b9bb57..b39f630ccb256 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 7d8d2f682780ceb9a96a81a23ddbf3ddbb145cd1 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/bloom.cpp | 6 +++--- test/sanitizer_suppressions/ubsan | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/bloom.cpp b/src/bloom.cpp index 50cf7194953bd..c9afe7e4fdb8c 100644 --- a/src/bloom.cpp +++ b/src/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 1d84b89904e68..352cb0ebcd1b5 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -47,7 +47,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:bloom.cpp implicit-integer-sign-change:chain.cpp implicit-integer-sign-change:chain.h implicit-integer-sign-change:coins.h From d884ff6d67015cc667afbbe2d1fc31db6406036b 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/lint/lint-locale-dependence.sh | 1 - test/util/data/bitcoin-util-test.json | 29 +++++++++++++++++++++++++-- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 0eb122f086cc5..6f98a71622e70 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/lint/lint-locale-dependence.sh b/test/lint/lint-locale-dependence.sh index 18f0a793f020d..13f4725528ba7 100755 --- a/test/lint/lint-locale-dependence.sh +++ b/test/lint/lint-locale-dependence.sh @@ -42,7 +42,6 @@ export LC_ALL=C # https://stackoverflow.com/a/34878283 for more details. KNOWN_VIOLATIONS=( - "src/bitcoin-tx.cpp.*stoul" "src/dbwrapper.cpp:.*vsnprintf" "src/test/dbwrapper_tests.cpp:.*snprintf" "src/test/fuzz/locale.cpp" diff --git a/test/util/data/bitcoin-util-test.json b/test/util/data/bitcoin-util-test.json index 5d78d2edb166f..85d1dfb494d40 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", @@ -463,9 +487,10 @@ "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 fbaccdd5252ed714adb2af9fc8e5f7c467aa4a73 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 3cfa9467fb9f1..35e57c83c9a91 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -477,7 +477,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 1cce271b1aa58962570c5ba6c143d1a8bca9e875 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 | 83 +++++++++++++++++--------------- 1 file changed, 45 insertions(+), 38 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 3229537737008..01e2add03922b 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 @@ def set_test_params(self): ]] 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 minrelaytxfee') + assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) + assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - 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')) + 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) - txids = [] - utxos = create_confirmed_utxos(self, relayfee, self.nodes[0], 491) + # 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']) - - 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) + tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"] + + # 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()