From 5b3a85b4c6ffd1f29a917d4c1af4bff6c0ea2ef5 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 13 Jun 2023 15:04:22 -0400 Subject: [PATCH 01/42] interfaces, wallet: Expose migrate wallet --- src/interfaces/wallet.h | 13 +++++++++++++ src/wallet/interfaces.cpp | 13 +++++++++++++ src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 1 + 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 8c31112fc9485..54eb720d02d86 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -50,6 +50,7 @@ struct WalletBalances; struct WalletTx; struct WalletTxOut; struct WalletTxStatus; +struct WalletMigrationResult; using WalletOrderForm = std::vector>; using WalletValueMap = std::map; @@ -332,6 +333,9 @@ class WalletLoader : public ChainClient //! Restore backup wallet virtual util::Result> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector& warnings) = 0; + //! Migrate a wallet + virtual util::Result migrateWallet(const std::string& name, const SecureString& passphrase) = 0; + //! Return available wallets in wallet directory. virtual std::vector listWalletDir() = 0; @@ -423,6 +427,15 @@ struct WalletTxOut bool is_spent = false; }; +//! Migrated wallet info +struct WalletMigrationResult +{ + std::unique_ptr wallet; + std::optional watchonly_wallet_name; + std::optional solvables_wallet_name; + fs::path backup_path; +}; + //! Return implementation of Wallet interface. This function is defined in //! dummywallet.cpp and throws if the wallet component is not compiled. std::unique_ptr MakeWallet(wallet::WalletContext& context, const std::shared_ptr& wallet); diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index cd438cfe2f399..463d169b46ebd 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -42,6 +42,7 @@ using interfaces::Wallet; using interfaces::WalletAddress; using interfaces::WalletBalances; using interfaces::WalletLoader; +using interfaces::WalletMigrationResult; using interfaces::WalletOrderForm; using interfaces::WalletTx; using interfaces::WalletTxOut; @@ -631,6 +632,18 @@ class WalletLoaderImpl : public WalletLoader return util::Error{error}; } } + util::Result migrateWallet(const std::string& name, const SecureString& passphrase) override + { + auto res = wallet::MigrateLegacyToDescriptor(name, passphrase, m_context); + if (!res) return util::Error{util::ErrorString(res)}; + WalletMigrationResult out{ + .wallet = MakeWallet(m_context, res->wallet), + .watchonly_wallet_name = res->watchonly_wallet ? std::make_optional(res->watchonly_wallet->GetName()) : std::nullopt, + .solvables_wallet_name = res->solvables_wallet ? std::make_optional(res->solvables_wallet->GetName()) : std::nullopt, + .backup_path = res->backup_path, + }; + return {std::move(out)}; // std::move to work around clang bug + } std::string getWalletDir() override { return fs::PathToString(GetWalletDir()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 62f0f53b016ef..b153c3f60a376 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4253,7 +4253,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // Migration successful, unload the wallet locally, then reload it. assert(local_wallet.use_count() == 1); local_wallet.reset(); - LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + res.wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); res.wallet_name = wallet_name; } else { // Migration failed, cleanup diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cbd5008366903..25752a3849248 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1068,6 +1068,7 @@ bool FillInputToWeight(CTxIn& txin, int64_t target_weight); struct MigrationResult { std::string wallet_name; + std::shared_ptr wallet; std::shared_ptr watchonly_wallet; std::shared_ptr solvables_wallet; fs::path backup_path; From d05be124dbc0b24fb69d0c28ba2d6b297d243751 Mon Sep 17 00:00:00 2001 From: kevkevin Date: Thu, 22 Jun 2023 08:27:38 -0500 Subject: [PATCH 02/42] test: added coverage to estimatefee Added a assert for an rpc error when we try to estimate fee for the max conf_target --- test/functional/rpc_estimatefee.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/functional/rpc_estimatefee.py b/test/functional/rpc_estimatefee.py index dad3cbcf0cb2a..6643799a76700 100755 --- a/test/functional/rpc_estimatefee.py +++ b/test/functional/rpc_estimatefee.py @@ -36,6 +36,9 @@ def run_test(self): assert_raises_rpc_error(-1, "estimatesmartfee", self.nodes[0].estimatesmartfee, 1, 'ECONOMICAL', 1) assert_raises_rpc_error(-1, "estimaterawfee", self.nodes[0].estimaterawfee, 1, 1, 1) + # max value of 1008 per src/policy/fees.h + assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", self.nodes[0].estimaterawfee, 1009) + # valid calls self.nodes[0].estimatesmartfee(1) # self.nodes[0].estimatesmartfee(1, None) From b9765ba1d67d7b74c17f9ce70cad5487715208a0 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 15 Dec 2018 04:20:46 +0000 Subject: [PATCH 03/42] GUI: TransactionRecord: Use "any from me" as the criteria for deciding whether a transaction is a send or receive This changes behaviour (IMO for the better) in the case where some but not all inputs are from us, and the net amount is positive. --- src/qt/transactionrecord.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 26144ba197e19..4b48b124d478f 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -13,6 +13,7 @@ #include +using wallet::ISMINE_NO; using wallet::ISMINE_SPENDABLE; using wallet::ISMINE_WATCH_ONLY; using wallet::isminetype; @@ -39,8 +40,21 @@ QList TransactionRecord::decomposeTransaction(const interface uint256 hash = wtx.tx->GetHash(); std::map mapValue = wtx.value_map; - if (nNet > 0 || wtx.is_coinbase) - { + bool involvesWatchAddress = false; + isminetype fAllFromMe = ISMINE_SPENDABLE; + bool any_from_me = false; + if (wtx.is_coinbase) { + fAllFromMe = ISMINE_NO; + } else { + for (const isminetype mine : wtx.txin_is_mine) + { + if(mine & ISMINE_WATCH_ONLY) involvesWatchAddress = true; + if(fAllFromMe > mine) fAllFromMe = mine; + if (mine) any_from_me = true; + } + } + + if (!any_from_me) { // // Credit // @@ -78,14 +92,6 @@ QList TransactionRecord::decomposeTransaction(const interface } else { - bool involvesWatchAddress = false; - isminetype fAllFromMe = ISMINE_SPENDABLE; - for (const isminetype mine : wtx.txin_is_mine) - { - if(mine & ISMINE_WATCH_ONLY) involvesWatchAddress = true; - if(fAllFromMe > mine) fAllFromMe = mine; - } - isminetype fAllToMe = ISMINE_SPENDABLE; for (const isminetype mine : wtx.txout_is_mine) { From f3fbe99fcf90daec79d49fd5d868102dc99feb23 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 15 Dec 2018 19:52:03 +0000 Subject: [PATCH 04/42] GUI: TransactionRecord: Refactor to turn send-to-self into send+receive pairs --- src/interfaces/wallet.h | 1 + src/qt/transactionrecord.cpp | 130 ++++++++++++++--------------------- src/wallet/interfaces.cpp | 1 + 3 files changed, 55 insertions(+), 77 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index f26ac866dcf95..b5d98e36fe6d4 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -387,6 +387,7 @@ struct WalletTx CTransactionRef tx; std::vector txin_is_mine; std::vector txout_is_mine; + std::vector txout_is_change; std::vector txout_address; std::vector txout_address_is_mine; CAmount credit; diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 4b48b124d478f..8a373451eb607 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -54,90 +54,36 @@ QList TransactionRecord::decomposeTransaction(const interface } } - if (!any_from_me) { - // - // Credit - // - for(unsigned int i = 0; i < wtx.tx->vout.size(); i++) - { - const CTxOut& txout = wtx.tx->vout[i]; - isminetype mine = wtx.txout_is_mine[i]; - if(mine) - { - TransactionRecord sub(hash, nTime); - sub.idx = i; // vout index - sub.credit = txout.nValue; - sub.involvesWatchAddress = mine & ISMINE_WATCH_ONLY; - if (wtx.txout_address_is_mine[i]) - { - // Received by Bitcoin Address - sub.type = TransactionRecord::RecvWithAddress; - sub.address = EncodeDestination(wtx.txout_address[i]); - } - else - { - // Received by IP connection (deprecated features), or a multisignature or other non-simple transaction - sub.type = TransactionRecord::RecvFromOther; - sub.address = mapValue["from"]; - } - if (wtx.is_coinbase) - { - // Generated - sub.type = TransactionRecord::Generated; - } - - parts.append(sub); - } - } - } - else - { - isminetype fAllToMe = ISMINE_SPENDABLE; + if (fAllFromMe || !any_from_me) { for (const isminetype mine : wtx.txout_is_mine) { if(mine & ISMINE_WATCH_ONLY) involvesWatchAddress = true; - if(fAllToMe > mine) fAllToMe = mine; } - if (fAllFromMe && fAllToMe) + CAmount nTxFee = nDebit - wtx.tx->GetValueOut(); + + for(unsigned int i = 0; i < wtx.tx->vout.size(); i++) { - // Payment to self - std::string address; - for (auto it = wtx.txout_address.begin(); it != wtx.txout_address.end(); ++it) { - if (it != wtx.txout_address.begin()) address += ", "; - address += EncodeDestination(*it); + const CTxOut& txout = wtx.tx->vout[i]; + + if (wtx.txout_is_change[i]) { + continue; } - CAmount nChange = wtx.change; - parts.append(TransactionRecord(hash, nTime, TransactionRecord::SendToSelf, address, -(nDebit - nChange), nCredit - nChange)); - parts.last().involvesWatchAddress = involvesWatchAddress; // maybe pass to TransactionRecord as constructor argument - } - else if (fAllFromMe) - { - // - // Debit - // - CAmount nTxFee = nDebit - wtx.tx->GetValueOut(); + if (fAllFromMe) { + // + // Debit + // - for (unsigned int nOut = 0; nOut < wtx.tx->vout.size(); nOut++) - { - const CTxOut& txout = wtx.tx->vout[nOut]; TransactionRecord sub(hash, nTime); - sub.idx = nOut; + sub.idx = i; sub.involvesWatchAddress = involvesWatchAddress; - if(wtx.txout_is_mine[nOut]) - { - // Ignore parts sent to self, as this is usually the change - // from a transaction sent back to our own address. - continue; - } - - if (!std::get_if(&wtx.txout_address[nOut])) + if (!std::get_if(&wtx.txout_address[i])) { // Sent to Bitcoin Address sub.type = TransactionRecord::SendToAddress; - sub.address = EncodeDestination(wtx.txout_address[nOut]); + sub.address = EncodeDestination(wtx.txout_address[i]); } else { @@ -157,15 +103,45 @@ QList TransactionRecord::decomposeTransaction(const interface parts.append(sub); } + + isminetype mine = wtx.txout_is_mine[i]; + if(mine) + { + // + // Credit + // + + TransactionRecord sub(hash, nTime); + sub.idx = i; // vout index + sub.credit = txout.nValue; + sub.involvesWatchAddress = mine & ISMINE_WATCH_ONLY; + if (wtx.txout_address_is_mine[i]) + { + // Received by Bitcoin Address + sub.type = TransactionRecord::RecvWithAddress; + sub.address = EncodeDestination(wtx.txout_address[i]); + } + else + { + // Received by IP connection (deprecated features), or a multisignature or other non-simple transaction + sub.type = TransactionRecord::RecvFromOther; + sub.address = mapValue["from"]; + } + if (wtx.is_coinbase) + { + // Generated + sub.type = TransactionRecord::Generated; + } + + parts.append(sub); + } } - else - { - // - // Mixed debit transaction, can't break down payees - // - parts.append(TransactionRecord(hash, nTime, TransactionRecord::Other, "", nNet, 0)); - parts.last().involvesWatchAddress = involvesWatchAddress; - } + } else { + // + // Mixed debit transaction, can't break down payees + // + parts.append(TransactionRecord(hash, nTime, TransactionRecord::Other, "", nNet, 0)); + parts.last().involvesWatchAddress = involvesWatchAddress; } return parts; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 9083c304b2cef..c027ff1240165 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -64,6 +64,7 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx) result.txout_address_is_mine.reserve(wtx.tx->vout.size()); for (const auto& txout : wtx.tx->vout) { result.txout_is_mine.emplace_back(wallet.IsMine(txout)); + result.txout_is_change.push_back(OutputIsChange(wallet, txout)); result.txout_address.emplace_back(); result.txout_address_is_mine.emplace_back(ExtractDestination(txout.scriptPubKey, result.txout_address.back()) ? wallet.IsMine(result.txout_address.back()) : From 71fbdb7f403e673877be94a79cd4c6b13b0bbcd6 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sun, 6 Jan 2019 12:45:52 +0000 Subject: [PATCH 05/42] GUI: Remove SendToSelf TransactionRecord type --- src/qt/transactionrecord.h | 1 - src/qt/transactiontablemodel.cpp | 6 ------ src/qt/transactionview.cpp | 1 - 3 files changed, 8 deletions(-) diff --git a/src/qt/transactionrecord.h b/src/qt/transactionrecord.h index dd34656d5fdb0..3ebcac8ab10c6 100644 --- a/src/qt/transactionrecord.h +++ b/src/qt/transactionrecord.h @@ -78,7 +78,6 @@ class TransactionRecord SendToOther, RecvWithAddress, RecvFromOther, - SendToSelf }; /** Number of confirmation recommended for accepting a transaction */ diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index 44b4fee2e7902..c551456bda38a 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -380,8 +380,6 @@ QString TransactionTableModel::formatTxType(const TransactionRecord *wtx) const case TransactionRecord::SendToAddress: case TransactionRecord::SendToOther: return tr("Sent to"); - case TransactionRecord::SendToSelf: - return tr("Payment to yourself"); case TransactionRecord::Generated: return tr("Mined"); default: @@ -424,8 +422,6 @@ QString TransactionTableModel::formatTxToAddress(const TransactionRecord *wtx, b return lookupAddress(wtx->address, tooltip) + watchAddress; case TransactionRecord::SendToOther: return QString::fromStdString(wtx->address) + watchAddress; - case TransactionRecord::SendToSelf: - return lookupAddress(wtx->address, tooltip) + watchAddress; default: return tr("(n/a)") + watchAddress; } @@ -444,8 +440,6 @@ QVariant TransactionTableModel::addressColor(const TransactionRecord *wtx) const if(label.isEmpty()) return COLOR_BAREADDRESS; } break; - case TransactionRecord::SendToSelf: - return COLOR_BAREADDRESS; default: break; } diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 778ef04b77378..4e771258eb8a4 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -91,7 +91,6 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa TransactionFilterProxy::TYPE(TransactionRecord::RecvFromOther)); typeWidget->addItem(tr("Sent to"), TransactionFilterProxy::TYPE(TransactionRecord::SendToAddress) | TransactionFilterProxy::TYPE(TransactionRecord::SendToOther)); - typeWidget->addItem(tr("To yourself"), TransactionFilterProxy::TYPE(TransactionRecord::SendToSelf)); typeWidget->addItem(tr("Mined"), TransactionFilterProxy::TYPE(TransactionRecord::Generated)); typeWidget->addItem(tr("Other"), TransactionFilterProxy::TYPE(TransactionRecord::Other)); From 2d182f77cd8100395cf47a721bd01dc8620c9718 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Mon, 7 Jan 2019 09:37:43 +0000 Subject: [PATCH 06/42] Bugfix: Ignore ischange flag when we're not the sender If we didn't send it, it can't possibly be change, even if that's the key's purpose --- src/qt/transactionrecord.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 8a373451eb607..85b7e701874c3 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -66,11 +66,13 @@ QList TransactionRecord::decomposeTransaction(const interface { const CTxOut& txout = wtx.tx->vout[i]; - if (wtx.txout_is_change[i]) { - continue; - } - if (fAllFromMe) { + // Change is only really possible if we're the sender + // Otherwise, someone just sent bitcoins to a change address, which should be shown + if (wtx.txout_is_change[i]) { + continue; + } + // // Debit // From 099dbe4224e0e896604e7f6901d0fc302b0bd3a0 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 15 Dec 2018 22:49:26 +0000 Subject: [PATCH 07/42] GUI: TransactionRecord: When time/index/etc match, sort send before receive --- src/qt/transactionrecord.cpp | 14 ++++++++++++-- src/qt/transactiontablemodel.cpp | 3 ++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 85b7e701874c3..f08c0d17c3758 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -154,11 +154,21 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, cons // Determine transaction status // Sort order, unrecorded transactions sort to the top - status.sortKey = strprintf("%010d-%01d-%010u-%03d", + int typesort; + switch (type) { + case SendToAddress: case SendToOther: + typesort = 2; break; + case RecvWithAddress: case RecvFromOther: + typesort = 3; break; + default: + typesort = 9; + } + status.sortKey = strprintf("%010d-%01d-%010u-%03d-%d", wtx.block_height, wtx.is_coinbase ? 1 : 0, wtx.time_received, - idx); + idx, + typesort); status.countsForBalance = wtx.is_trusted && !(wtx.blocks_to_maturity > 0); status.depth = wtx.depth_in_main_chain; status.m_cur_block_hash = block_hash; diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index c551456bda38a..8ae0a3c3abe7b 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -557,7 +558,7 @@ QVariant TransactionTableModel::data(const QModelIndex &index, int role) const case Status: return QString::fromStdString(rec->status.sortKey); case Date: - return rec->time; + return QString::fromStdString(strprintf("%020s-%s", rec->time, rec->status.sortKey)); case Type: return formatTxType(rec); case Watchonly: From 9ea31eba04ff8dcb9d7d993ce28bb10731a35177 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 13 Jun 2023 15:52:03 -0400 Subject: [PATCH 08/42] gui: Disable and uncheck blank when private keys are disabled Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled. --- src/qt/createwalletdialog.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/qt/createwalletdialog.cpp b/src/qt/createwalletdialog.cpp index 5b3c8bcf48124..3e8be3e6754dd 100644 --- a/src/qt/createwalletdialog.cpp +++ b/src/qt/createwalletdialog.cpp @@ -58,10 +58,7 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : ui->descriptor_checkbox->setChecked(checked); ui->encrypt_wallet_checkbox->setChecked(false); ui->disable_privkeys_checkbox->setChecked(checked); - // The blank check box is ambiguous. This flag is always true for a - // watch-only wallet, even though we immedidately fetch keys from the - // external signer. - ui->blank_wallet_checkbox->setChecked(checked); + ui->blank_wallet_checkbox->setChecked(false); }); connect(ui->disable_privkeys_checkbox, &QCheckBox::toggled, [this](bool checked) { @@ -69,9 +66,10 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : // set to true, enable it when isDisablePrivateKeysChecked is false. ui->encrypt_wallet_checkbox->setEnabled(!checked); - // Wallets without private keys start out blank + // Wallets without private keys cannot set blank + ui->blank_wallet_checkbox->setEnabled(!checked); if (checked) { - ui->blank_wallet_checkbox->setChecked(true); + ui->blank_wallet_checkbox->setChecked(false); } // When the encrypt_wallet_checkbox is disabled, uncheck it. @@ -81,8 +79,11 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : }); connect(ui->blank_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) { - if (!checked) { - ui->disable_privkeys_checkbox->setChecked(false); + // Disable the disable_privkeys_checkbox when blank_wallet_checkbox is checked + // as blank-ness only pertains to wallets with private keys. + ui->disable_privkeys_checkbox->setEnabled(!checked); + if (checked) { + ui->disable_privkeys_checkbox->setChecked(false); } }); From 577be889cd52fc2d896a5f39c66bc2cadb8622e4 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 23 Jun 2023 14:23:27 -0400 Subject: [PATCH 09/42] gui: Optionally return passphrase after unlocking AskPassphraseDialog has an optional parameter for the caller to get the passphrase. Make this available for Unlocking. --- src/qt/askpassphrasedialog.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index 0a96be038b22a..246dff0069215 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -167,6 +167,9 @@ void AskPassphraseDialog::accept() "passphrase to avoid this issue in the future.")); } } else { + if (m_passphrase_out) { + m_passphrase_out->assign(oldpass); + } QDialog::accept(); // Success } } catch (const std::runtime_error& e) { From 48aae2cffeb91add75a70ac4d5075c38054452fa Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 13 Jun 2023 15:04:43 -0400 Subject: [PATCH 10/42] gui: Add File > Migrate Wallet --- src/qt/bitcoingui.cpp | 12 +++++++ src/qt/bitcoingui.h | 2 ++ src/qt/walletcontroller.cpp | 64 +++++++++++++++++++++++++++++++++++++ src/qt/walletcontroller.h | 22 +++++++++++++ 4 files changed, 100 insertions(+) diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index f201d8fa011c2..6147d986d0ea0 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -359,6 +359,10 @@ void BitcoinGUI::createActions() m_close_all_wallets_action = new QAction(tr("Close All Wallets…"), this); m_close_all_wallets_action->setStatusTip(tr("Close all wallets")); + m_migrate_wallet_action = new QAction(tr("Migrate Wallet"), this); + m_migrate_wallet_action->setEnabled(false); + m_migrate_wallet_action->setStatusTip(tr("Migrate a wallet")); + showHelpMessageAction = new QAction(tr("&Command-line options"), this); showHelpMessageAction->setMenuRole(QAction::NoRole); showHelpMessageAction->setStatusTip(tr("Show the %1 help message to get a list with possible Bitcoin command-line options").arg(PACKAGE_NAME)); @@ -456,6 +460,11 @@ void BitcoinGUI::createActions() connect(m_close_all_wallets_action, &QAction::triggered, [this] { m_wallet_controller->closeAllWallets(this); }); + connect(m_migrate_wallet_action, &QAction::triggered, [this] { + auto activity = new MigrateWalletActivity(m_wallet_controller, this); + connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet); + activity->migrate(walletFrame->currentWalletModel()); + }); connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::setPrivacy); connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::enableHistoryAction); } @@ -483,6 +492,7 @@ void BitcoinGUI::createMenuBar() file->addAction(m_open_wallet_action); file->addAction(m_close_wallet_action); file->addAction(m_close_all_wallets_action); + file->addAction(m_migrate_wallet_action); file->addSeparator(); file->addAction(backupWalletAction); file->addAction(m_restore_wallet_action); @@ -767,6 +777,7 @@ void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model) } } updateWindowTitle(); + m_migrate_wallet_action->setEnabled(wallet_model->wallet().isLegacy()); } void BitcoinGUI::setCurrentWalletBySelectorIndex(int index) @@ -800,6 +811,7 @@ void BitcoinGUI::setWalletActionsEnabled(bool enabled) openAction->setEnabled(enabled); m_close_wallet_action->setEnabled(enabled); m_close_all_wallets_action->setEnabled(enabled); + m_migrate_wallet_action->setEnabled(enabled); } void BitcoinGUI::createTrayIcon() diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h index 92b889263ba61..0887ebd8cc48e 100644 --- a/src/qt/bitcoingui.h +++ b/src/qt/bitcoingui.h @@ -163,6 +163,8 @@ class BitcoinGUI : public QMainWindow QAction* m_wallet_selector_label_action = nullptr; QAction* m_wallet_selector_action = nullptr; QAction* m_mask_values_action{nullptr}; + QAction* m_migrate_wallet_action{nullptr}; + QMenu* m_migrate_wallet_menu{nullptr}; QLabel *m_wallet_selector_label = nullptr; QComboBox* m_wallet_selector = nullptr; diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index d782838d6ff98..7b2ac4f90a06f 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -432,3 +432,67 @@ void RestoreWalletActivity::finish() Q_EMIT finished(); } + +void MigrateWalletActivity::migrate(WalletModel* wallet_model) +{ + // Warn the user about migration + QMessageBox box(m_parent_widget); + box.setWindowTitle(tr("Migrate wallet")); + box.setText(tr("Are you sure you wish to migrate the wallet %1?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName()))); + box.setInformativeText(tr("Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.\n" + "If this wallet contains any watchonly scripts, a new wallet will be created which contains those watchonly scripts.\n" + "If this wallet contains any solvable but not watched scripts, a different and new wallet will be created which contains those scripts.\n\n" + "The migration process will create a backup of the wallet before migrating. This backup file will be named " + "-.legacy.bak and can be found in the directory for this wallet. In the event of " + "an incorrect migration, the backup can be restored with the \"Restore Wallet\" functionality.")); + box.setStandardButtons(QMessageBox::Yes|QMessageBox::Cancel); + box.setDefaultButton(QMessageBox::Yes); + if (box.exec() != QMessageBox::Yes) return; + + // Get the passphrase if it is encrypted regardless of it is locked or unlocked. We need the passphrase itself. + SecureString passphrase; + WalletModel::EncryptionStatus enc_status = wallet_model->getEncryptionStatus(); + if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) { + AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase); + dlg.setModel(wallet_model); + dlg.exec(); + } + + // GUI needs to remove the wallet so that it can actually be unloaded by migration + const std::string name = wallet_model->wallet().getWalletName(); + m_wallet_controller->removeAndDeleteWallet(wallet_model); + + showProgressDialog(tr("Migrate Wallet"), tr("Migrating Wallet %1…").arg(GUIUtil::HtmlEscape(name))); + + QTimer::singleShot(0, worker(), [this, name, passphrase] { + auto res{node().walletLoader().migrateWallet(name, passphrase)}; + + if (res) { + m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(res->wallet->getWalletName())); + if (res->watchonly_wallet_name) { + m_success_message += tr(" Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->watchonly_wallet_name.value())); + } + if (res->solvables_wallet_name) { + m_success_message += tr(" Solvable but not watched scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->solvables_wallet_name.value())); + } + m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(res->wallet)); + } else { + m_error_message = util::ErrorString(res); + } + + QTimer::singleShot(0, this, &MigrateWalletActivity::finish); + }); +} + +void MigrateWalletActivity::finish() +{ + if (!m_error_message.empty()) { + QMessageBox::critical(m_parent_widget, tr("Migration failed"), QString::fromStdString(m_error_message.translated)); + } else { + QMessageBox::information(m_parent_widget, tr("Migration Successful"), m_success_message); + } + + if (m_wallet_model) Q_EMIT migrated(m_wallet_model); + + Q_EMIT finished(); +} diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h index fcd65756c67c7..4f2443a727c23 100644 --- a/src/qt/walletcontroller.h +++ b/src/qt/walletcontroller.h @@ -40,6 +40,7 @@ class path; class AskPassphraseDialog; class CreateWalletActivity; class CreateWalletDialog; +class MigrateWalletActivity; class OpenWalletActivity; class WalletControllerActivity; @@ -65,6 +66,8 @@ class WalletController : public QObject void closeWallet(WalletModel* wallet_model, QWidget* parent = nullptr); void closeAllWallets(QWidget* parent = nullptr); + void migrateWallet(WalletModel* wallet_model, QWidget* parent = nullptr); + Q_SIGNALS: void walletAdded(WalletModel* wallet_model); void walletRemoved(WalletModel* wallet_model); @@ -83,6 +86,7 @@ class WalletController : public QObject std::unique_ptr m_handler_load_wallet; friend class WalletControllerActivity; + friend class MigrateWalletActivity; }; class WalletControllerActivity : public QObject @@ -175,4 +179,22 @@ class RestoreWalletActivity : public WalletControllerActivity void finish(); }; +class MigrateWalletActivity : public WalletControllerActivity +{ + Q_OBJECT + +public: + MigrateWalletActivity(WalletController* wallet_controller, QWidget* parent) : WalletControllerActivity(wallet_controller, parent) {} + + void migrate(WalletModel* wallet_model); + +Q_SIGNALS: + void migrated(WalletModel* wallet_model); + +private: + QString m_success_message; + + void finish(); +}; + #endif // BITCOIN_QT_WALLETCONTROLLER_H From deccf1c4848620cfff2a9642c5a6b5acdfbc33bd Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 13 Jul 2023 13:01:54 -0600 Subject: [PATCH 11/42] Move IsPeerAddrLocalGood() declaration from header to implementation --- src/net.cpp | 2 +- src/net.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 1b1b5404177b5..e8567348f66f1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -223,7 +223,7 @@ static int GetnScore(const CService& addr) } // Is our peer's addrLocal potentially useful as an external IP source? -bool IsPeerAddrLocalGood(CNode *pnode) +[[nodiscard]] static bool IsPeerAddrLocalGood(CNode *pnode) { CService addrLocal = pnode->GetAddrLocal(); return fDiscover && pnode->addr.IsRoutable() && addrLocal.IsRoutable() && diff --git a/src/net.h b/src/net.h index 7427d0f45b759..e26bf523e8b61 100644 --- a/src/net.h +++ b/src/net.h @@ -145,7 +145,6 @@ enum LOCAL_MAX }; -bool IsPeerAddrLocalGood(CNode *pnode); /** Returns a local address that we should advertise to this peer. */ std::optional GetLocalAddrForPeer(CNode& node); From 11426f6557ac09489d5e13bf3b9d94fbd5073c8e Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 13 Jul 2023 14:30:30 -0600 Subject: [PATCH 12/42] Move CaptureMessageToFile() declaration from header to implementation --- src/net.cpp | 9 +++++---- src/net.h | 6 ------ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index e8567348f66f1..ee4faeed9fde7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2915,10 +2915,11 @@ uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& address) const return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize(); } -void CaptureMessageToFile(const CAddress& addr, - const std::string& msg_type, - Span data, - bool is_incoming) +// Dump binary message to file, with timestamp. +static void CaptureMessageToFile(const CAddress& addr, + const std::string& msg_type, + Span data, + bool is_incoming) { // Note: This function captures the message at the time of processing, // not at socket receive/send time. diff --git a/src/net.h b/src/net.h index e26bf523e8b61..1c2b60a30f3cb 100644 --- a/src/net.h +++ b/src/net.h @@ -1221,12 +1221,6 @@ class CConnman friend struct ConnmanTestMsg; }; -/** Dump binary message to file, with timestamp */ -void CaptureMessageToFile(const CAddress& addr, - const std::string& msg_type, - Span data, - bool is_incoming); - /** Defaults to `CaptureMessageToFile()`, but can be overridden by unit tests. */ extern std::function Date: Thu, 13 Jul 2023 12:14:16 -0600 Subject: [PATCH 13/42] Move GetLocal() declaration from header to implementation --- src/net.cpp | 2 +- src/net.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index ee4faeed9fde7..654e0ac9fd2cc 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -146,7 +146,7 @@ uint16_t GetListenPort() } // find 'best' local address for a particular peer -bool GetLocal(CService& addr, const CNode& peer) +[[nodiscard]] static bool GetLocal(CService& addr, const CNode& peer) { if (!fListen) return false; diff --git a/src/net.h b/src/net.h index 1c2b60a30f3cb..54c3839fcc919 100644 --- a/src/net.h +++ b/src/net.h @@ -163,7 +163,6 @@ bool AddLocal(const CNetAddr& addr, int nScore = LOCAL_NONE); void RemoveLocal(const CService& addr); bool SeenLocal(const CService& addr); bool IsLocal(const CService& addr); -bool GetLocal(CService& addr, const CNode& peer); CService GetLocalAddress(const CNode& peer); CService MaybeFlipIPv6toCJDNS(const CService& service); From fb4265747c9eb022d80c6f2988e574c689130348 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 14 Jul 2023 10:48:30 -0600 Subject: [PATCH 14/42] Add and use CNetAddr::HasCJDNSPrefix() helper --- src/net.cpp | 2 +- src/netaddress.cpp | 3 +-- src/netaddress.h | 1 + 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 654e0ac9fd2cc..d76874b2338af 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -275,7 +275,7 @@ std::optional GetLocalAddrForPeer(CNode& node) CService MaybeFlipIPv6toCJDNS(const CService& service) { CService ret{service}; - if (ret.m_net == NET_IPV6 && ret.m_addr[0] == 0xfc && IsReachable(NET_CJDNS)) { + if (ret.m_net == NET_IPV6 && ret.HasCJDNSPrefix() && IsReachable(NET_CJDNS)) { ret.m_net = NET_CJDNS; } return ret; diff --git a/src/netaddress.cpp b/src/netaddress.cpp index 4758f2468087e..8196a6a48c660 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -450,8 +450,7 @@ bool CNetAddr::IsValid() const return false; } - // CJDNS addresses always start with 0xfc - if (IsCJDNS() && (m_addr[0] != 0xFC)) { + if (IsCJDNS() && !HasCJDNSPrefix()) { return false; } diff --git a/src/netaddress.h b/src/netaddress.h index 36dc8864068ff..5e67d843e2507 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -182,6 +182,7 @@ class CNetAddr bool IsTor() const; bool IsI2P() const; bool IsCJDNS() const; + [[nodiscard]] bool HasCJDNSPrefix() const { return m_addr[0] == 0xfc; } bool IsLocal() const; bool IsRoutable() const; bool IsInternal() const; From df488563b280c63f5d74d5ac0fcb1a2cad489d55 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Tue, 18 Jul 2023 10:07:41 -0600 Subject: [PATCH 15/42] GetLocal() type-safety, naming, const, and formatting cleanups Co-authored-by: Jon Atack --- src/net.cpp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index d76874b2338af..83440c2497fcb 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -148,30 +148,27 @@ uint16_t GetListenPort() // find 'best' local address for a particular peer [[nodiscard]] static bool GetLocal(CService& addr, const CNode& peer) { - if (!fListen) - return false; + if (!fListen) return false; int nBestScore = -1; int nBestReachability = -1; { LOCK(g_maplocalhost_mutex); - for (const auto& entry : mapLocalHost) - { + for (const auto& [local_addr, local_service_info] : mapLocalHost) { // For privacy reasons, don't advertise our privacy-network address // to other networks and don't advertise our other-network address // to privacy networks. - const Network our_net{entry.first.GetNetwork()}; + const Network our_net{local_addr.GetNetwork()}; const Network peers_net{peer.ConnectedThroughNetwork()}; if (our_net != peers_net && (our_net == NET_ONION || our_net == NET_I2P || peers_net == NET_ONION || peers_net == NET_I2P)) { continue; } - int nScore = entry.second.nScore; - int nReachability = entry.first.GetReachabilityFrom(peer.addr); - if (nReachability > nBestReachability || (nReachability == nBestReachability && nScore > nBestScore)) - { - addr = CService(entry.first, entry.second.nPort); + const int nScore{local_service_info.nScore}; + const int nReachability{local_addr.GetReachabilityFrom(peer.addr)}; + if (nReachability > nBestReachability || (nReachability == nBestReachability && nScore > nBestScore)) { + addr = CService{local_addr, local_service_info.nPort}; nBestReachability = nReachability; nBestScore = nScore; } From 07f589158835151a7613e4b2a508c0dd61a18fd7 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Wed, 19 Jul 2023 11:11:06 -0600 Subject: [PATCH 16/42] Add CNetAddr::IsPrivacyNet() and CNode::IsConnectedThroughPrivacyNet() Co-authored-by: Vasil Dimov --- src/net.cpp | 5 +++++ src/net.h | 3 +++ src/netaddress.h | 7 +++++++ 3 files changed, 15 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index 83440c2497fcb..e1f3040485efa 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -620,6 +620,11 @@ Network CNode::ConnectedThroughNetwork() const return m_inbound_onion ? NET_ONION : addr.GetNetClass(); } +bool CNode::IsConnectedThroughPrivacyNet() const +{ + return m_inbound_onion || addr.IsPrivacyNet(); +} + #undef X #define X(name) stats.name = name void CNode::CopyStats(CNodeStats& stats) diff --git a/src/net.h b/src/net.h index 54c3839fcc919..8cf203b309e53 100644 --- a/src/net.h +++ b/src/net.h @@ -506,6 +506,9 @@ class CNode */ Network ConnectedThroughNetwork() const; + /** Whether this peer connected through a privacy network. */ + [[nodiscard]] bool IsConnectedThroughPrivacyNet() const; + // We selected peer as (compact blocks) high-bandwidth peer (BIP152) std::atomic m_bip152_highbandwidth_to{false}; // Peer selected us as (compact blocks) high-bandwidth peer (BIP152) diff --git a/src/netaddress.h b/src/netaddress.h index 5e67d843e2507..428ab87423b5b 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -188,6 +188,13 @@ class CNetAddr bool IsInternal() const; bool IsValid() const; + /** + * Whether this object is a privacy network. + * TODO: consider adding IsCJDNS() here when more peers adopt CJDNS, see: + * https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155 + */ + [[nodiscard]] bool IsPrivacyNet() const { return IsTor() || IsI2P(); } + /** * Check if the current object can be serialized in pre-ADDRv2/BIP155 format. */ From f1304db136bbeac70b52522b5a4522165bfb3fc0 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 14 Jul 2023 06:51:21 -0600 Subject: [PATCH 17/42] Use higher-level CNetAddr and CNode helpers in net.cpp rather than low-level comparisons with Network enum values. --- src/net.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index e1f3040485efa..a230b64a74db5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -158,11 +158,8 @@ uint16_t GetListenPort() // For privacy reasons, don't advertise our privacy-network address // to other networks and don't advertise our other-network address // to privacy networks. - const Network our_net{local_addr.GetNetwork()}; - const Network peers_net{peer.ConnectedThroughNetwork()}; - if (our_net != peers_net && - (our_net == NET_ONION || our_net == NET_I2P || - peers_net == NET_ONION || peers_net == NET_I2P)) { + if (local_addr.GetNetwork() != peer.ConnectedThroughNetwork() + && (local_addr.IsPrivacyNet() || peer.IsConnectedThroughPrivacyNet())) { continue; } const int nScore{local_service_info.nScore}; @@ -272,7 +269,7 @@ std::optional GetLocalAddrForPeer(CNode& node) CService MaybeFlipIPv6toCJDNS(const CService& service) { CService ret{service}; - if (ret.m_net == NET_IPV6 && ret.HasCJDNSPrefix() && IsReachable(NET_CJDNS)) { + if (ret.IsIPv6() && ret.HasCJDNSPrefix() && IsReachable(NET_CJDNS)) { ret.m_net = NET_CJDNS; } return ret; @@ -488,7 +485,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo const bool use_proxy{GetProxy(addrConnect.GetNetwork(), proxy)}; bool proxyConnectionFailed = false; - if (addrConnect.GetNetwork() == NET_I2P && use_proxy) { + if (addrConnect.IsI2P() && use_proxy) { i2p::Connection conn; if (m_i2p_sam_session) { From 5316ae5dd8d90623f9bb883bb253fa6463ee4d34 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 13 Jul 2023 12:38:06 -0600 Subject: [PATCH 18/42] Convert GetLocal() to std::optional and remove out-param Co-authored-by: stickies-v --- src/net.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index a230b64a74db5..a12c7a2e1cbde 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -145,11 +145,12 @@ uint16_t GetListenPort() return static_cast(gArgs.GetIntArg("-port", Params().GetDefaultPort())); } -// find 'best' local address for a particular peer -[[nodiscard]] static bool GetLocal(CService& addr, const CNode& peer) +// Determine the "best" local address for a particular peer. +[[nodiscard]] static std::optional GetLocal(const CNode& peer) { - if (!fListen) return false; + if (!fListen) return std::nullopt; + std::optional addr; int nBestScore = -1; int nBestReachability = -1; { @@ -165,13 +166,13 @@ uint16_t GetListenPort() const int nScore{local_service_info.nScore}; const int nReachability{local_addr.GetReachabilityFrom(peer.addr)}; if (nReachability > nBestReachability || (nReachability == nBestReachability && nScore > nBestScore)) { - addr = CService{local_addr, local_service_info.nPort}; + addr.emplace(CService{local_addr, local_service_info.nPort}); nBestReachability = nReachability; nBestScore = nScore; } } } - return nBestScore >= 0; + return addr; } //! Convert the serialized seeds into usable address objects. @@ -196,17 +197,13 @@ static std::vector ConvertSeeds(const std::vector &vSeedsIn) return vSeedsOut; } -// get best local address for a particular peer as a CAddress -// Otherwise, return the unroutable 0.0.0.0 but filled in with +// Determine the "best" local address for a particular peer. +// If none, return the unroutable 0.0.0.0 but filled in with // the normal parameters, since the IP may be changed to a useful // one by discovery. CService GetLocalAddress(const CNode& peer) { - CService addr; - if (GetLocal(addr, peer)) { - return addr; - } - return CService{CNetAddr(), GetListenPort()}; + return GetLocal(peer).value_or(CService{CNetAddr(), GetListenPort()}); } static int GetnScore(const CService& addr) From 4ecfd3eaf434d868455466e001adae4b68515ab8 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 14 Jul 2023 07:08:23 -0600 Subject: [PATCH 19/42] Inline short, often-called, rarely-changed basic CNetAddr getters and make them nodiscard. Member functions containing a few lines of code are usually inlined, either implicitly by defining them in the declaration as done here, or declared inline. References https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-inline https://google.github.io/styleguide/cppguide#Inline_Functions https://www.ibm.com/docs/en/i/7.1?topic=only-inline-member-functions-c --- src/netaddress.cpp | 20 -------------------- src/netaddress.h | 10 +++++----- 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/src/netaddress.cpp b/src/netaddress.cpp index 8196a6a48c660..7530334db1da3 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -309,10 +309,6 @@ bool CNetAddr::IsBindAny() const return std::all_of(m_addr.begin(), m_addr.end(), [](uint8_t b) { return b == 0; }); } -bool CNetAddr::IsIPv4() const { return m_net == NET_IPV4; } - -bool CNetAddr::IsIPv6() const { return m_net == NET_IPV6; } - bool CNetAddr::IsRFC1918() const { return IsIPv4() && ( @@ -400,22 +396,6 @@ bool CNetAddr::IsHeNet() const return IsIPv6() && HasPrefix(m_addr, std::array{0x20, 0x01, 0x04, 0x70}); } -/** - * Check whether this object represents a TOR address. - * @see CNetAddr::SetSpecial(const std::string &) - */ -bool CNetAddr::IsTor() const { return m_net == NET_ONION; } - -/** - * Check whether this object represents an I2P address. - */ -bool CNetAddr::IsI2P() const { return m_net == NET_I2P; } - -/** - * Check whether this object represents a CJDNS address. - */ -bool CNetAddr::IsCJDNS() const { return m_net == NET_CJDNS; } - bool CNetAddr::IsLocal() const { // IPv4 loopback (127.0.0.0/8 or 0.0.0.0/8) diff --git a/src/netaddress.h b/src/netaddress.h index 428ab87423b5b..867042b387997 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -162,8 +162,8 @@ class CNetAddr bool SetSpecial(const std::string& addr); bool IsBindAny() const; // INADDR_ANY equivalent - bool IsIPv4() const; // IPv4 mapped address (::FFFF:0:0/96, 0.0.0.0/0) - bool IsIPv6() const; // IPv6 address (not mapped IPv4, not Tor) + [[nodiscard]] bool IsIPv4() const { return m_net == NET_IPV4; } // IPv4 mapped address (::FFFF:0:0/96, 0.0.0.0/0) + [[nodiscard]] bool IsIPv6() const { return m_net == NET_IPV6; } // IPv6 address (not mapped IPv4, not Tor) bool IsRFC1918() const; // IPv4 private networks (10.0.0.0/8, 192.168.0.0/16, 172.16.0.0/12) bool IsRFC2544() const; // IPv4 inter-network communications (198.18.0.0/15) bool IsRFC6598() const; // IPv4 ISP-level NAT (100.64.0.0/10) @@ -179,9 +179,9 @@ class CNetAddr bool IsRFC6052() const; // IPv6 well-known prefix for IPv4-embedded address (64:FF9B::/96) bool IsRFC6145() const; // IPv6 IPv4-translated address (::FFFF:0:0:0/96) (actually defined in RFC2765) bool IsHeNet() const; // IPv6 Hurricane Electric - https://he.net (2001:0470::/36) - bool IsTor() const; - bool IsI2P() const; - bool IsCJDNS() const; + [[nodiscard]] bool IsTor() const { return m_net == NET_ONION; } + [[nodiscard]] bool IsI2P() const { return m_net == NET_I2P; } + [[nodiscard]] bool IsCJDNS() const { return m_net == NET_CJDNS; } [[nodiscard]] bool HasCJDNSPrefix() const { return m_addr[0] == 0xfc; } bool IsLocal() const; bool IsRoutable() const; From 83d7cfd5429b0be7755bc48145032956f5f56dae Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 25 Jul 2023 22:15:56 +0200 Subject: [PATCH 20/42] test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs Follow-up for #28025. --- test/functional/p2p_segwit.py | 20 +++++++++----------- test/functional/test_framework/script.py | 9 +++++++++ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 1e7bc95a63ec9..51ec0fc8e7fc3 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -70,9 +70,9 @@ SIGHASH_ANYONECANPAY, SIGHASH_NONE, SIGHASH_SINGLE, - SegwitV0SignatureHash, hash160, sign_input_legacy, + sign_input_segwitv0, ) from test_framework.script_util import ( key_to_p2pk_script, @@ -121,10 +121,8 @@ def func_wrapper(self, *args, **kwargs): def sign_p2pk_witness_input(script, tx_to, in_idx, hashtype, value, key): """Add signature for a P2PK witness script.""" - tx_hash = SegwitV0SignatureHash(script, tx_to, in_idx, hashtype, value) - signature = key.sign_ecdsa(tx_hash) + chr(hashtype).encode('latin-1') - tx_to.wit.vtxinwit[in_idx].scriptWitness.stack = [signature, script] - tx_to.rehash() + tx_to.wit.vtxinwit[in_idx].scriptWitness.stack = [script] + sign_input_segwitv0(tx_to, in_idx, script, value, key, hashtype) def test_transaction_acceptance(node, p2p, tx, with_witness, accepted, reason=None): """Send a transaction to the node and check that it's accepted to the mempool @@ -1476,11 +1474,9 @@ def test_uncompressed_pubkey(self): tx2.vin.append(CTxIn(COutPoint(tx.sha256, 0), b"")) tx2.vout.append(CTxOut(tx.vout[0].nValue - 1000, script_wsh)) script = keyhash_to_p2pkh_script(pubkeyhash) - sig_hash = SegwitV0SignatureHash(script, tx2, 0, SIGHASH_ALL, tx.vout[0].nValue) - signature = key.sign_ecdsa(sig_hash) + b'\x01' # 0x1 is SIGHASH_ALL tx2.wit.vtxinwit.append(CTxInWitness()) - tx2.wit.vtxinwit[0].scriptWitness.stack = [signature, pubkey] - tx2.rehash() + tx2.wit.vtxinwit[0].scriptWitness.stack = [pubkey] + sign_input_segwitv0(tx2, 0, script, tx.vout[0].nValue, key) # Should fail policy test. test_transaction_acceptance(self.nodes[0], self.test_node, tx2, True, False, 'non-mandatory-script-verify-flag (Using non-compressed keys in segwit)') @@ -1676,11 +1672,13 @@ def test_signature_version_1(self): tx2.vout.append(CTxOut(tx.vout[0].nValue, CScript([OP_TRUE]))) script = keyhash_to_p2pkh_script(pubkeyhash) - sig_hash = SegwitV0SignatureHash(script, tx2, 0, SIGHASH_ALL, tx.vout[0].nValue) - signature = key.sign_ecdsa(sig_hash) + b'\x01' # 0x1 is SIGHASH_ALL + tx2.wit.vtxinwit.append(CTxInWitness()) + sign_input_segwitv0(tx2, 0, script, tx.vout[0].nValue, key) + signature = tx2.wit.vtxinwit[0].scriptWitness.stack.pop() # Check that we can't have a scriptSig tx2.vin[0].scriptSig = CScript([signature, pubkey]) + tx2.rehash() block = self.build_next_block() self.update_witness_block_with_transactions(block, [tx, tx2]) test_witness_block(self.nodes[0], self.test_node, block, accepted=False, diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py index 78f58cf11fcd3..17a954cb22c76 100644 --- a/test/functional/test_framework/script.py +++ b/test/functional/test_framework/script.py @@ -699,6 +699,15 @@ def sign_input_legacy(tx, input_index, input_scriptpubkey, privkey, sighash_type tx.vin[input_index].scriptSig = bytes(CScript([der_sig + bytes([sighash_type])])) + tx.vin[input_index].scriptSig tx.rehash() +def sign_input_segwitv0(tx, input_index, input_scriptpubkey, input_amount, privkey, sighash_type=SIGHASH_ALL): + """Add segwitv0 ECDSA signature for a given transaction input. Note that the signature + is inserted at the bottom of the witness stack, i.e. additional witness data + needed (e.g. pubkey for P2WPKH) can already be set before.""" + sighash = SegwitV0SignatureHash(input_scriptpubkey, tx, input_index, sighash_type, input_amount) + der_sig = privkey.sign_ecdsa(sighash) + tx.wit.vtxinwit[input_index].scriptWitness.stack.insert(0, der_sig + bytes([sighash_type])) + tx.rehash() + # TODO: Allow cached hashPrevouts/hashSequence/hashOutputs to be provided. # Performance optimization probably not necessary for python tests, however. # Note that this corresponds to sigversion == 1 in EvalScript, which is used From 56b27b84877376ffc32b3bad09f1047b23de4ba1 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 21 Oct 2022 15:03:40 -0300 Subject: [PATCH 21/42] rpc, refactor: clean-up `addnode` 1. Use const where possible; 2. Rename variables to make them clearer; 3. There is no need to check whether `command` is null since it's a non-optional field. --- src/rpc/net.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index a2a46ef32f84b..da511d1ed940d 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -297,10 +297,8 @@ static RPCHelpMan addnode() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::string strCommand; - if (!request.params[1].isNull()) - strCommand = request.params[1].get_str(); - if (strCommand != "onetry" && strCommand != "add" && strCommand != "remove") { + const std::string command{request.params[1].get_str()}; + if (command != "onetry" && command != "add" && command != "remove") { throw std::runtime_error( self.ToString()); } @@ -308,24 +306,24 @@ static RPCHelpMan addnode() NodeContext& node = EnsureAnyNodeContext(request.context); CConnman& connman = EnsureConnman(node); - std::string strNode = request.params[0].get_str(); + const std::string node_arg{request.params[0].get_str()}; - if (strCommand == "onetry") + if (command == "onetry") { CAddress addr; - connman.OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), ConnectionType::MANUAL); + connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grantOutbound=*/nullptr, node_arg.c_str(), ConnectionType::MANUAL); return UniValue::VNULL; } - if (strCommand == "add") + if (command == "add") { - if (!connman.AddNode(strNode)) { + if (!connman.AddNode(node_arg)) { throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: Node already added"); } } - else if(strCommand == "remove") + else if (command == "remove") { - if (!connman.RemoveAddedNode(strNode)) { + if (!connman.RemoveAddedNode(node_arg)) { throw JSONRPCError(RPC_CLIENT_NODE_NOT_ADDED, "Error: Node could not be removed. It has not been added previously."); } } From effd1efefb53c58f0e43fec4f019a19f97795553 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 21 Oct 2022 15:41:35 -0300 Subject: [PATCH 22/42] test: `addnode` with an invalid command should throw an error --- test/functional/rpc_net.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 5fdd5daddf784..5260656c63f91 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -61,7 +61,7 @@ def run_test(self): self.test_getpeerinfo() self.test_getnettotals() self.test_getnetworkinfo() - self.test_getaddednodeinfo() + self.test_addnode_getaddednodeinfo() self.test_service_flags() self.test_getnodeaddresses() self.test_addpeeraddress() @@ -203,8 +203,8 @@ def test_getnetworkinfo(self): # Check dynamically generated networks list in getnetworkinfo help output. assert "(ipv4, ipv6, onion, i2p, cjdns)" in self.nodes[0].help("getnetworkinfo") - def test_getaddednodeinfo(self): - self.log.info("Test getaddednodeinfo") + def test_addnode_getaddednodeinfo(self): + self.log.info("Test addnode and getaddednodeinfo") assert_equal(self.nodes[0].getaddednodeinfo(), []) # add a node (node2) to node0 ip_port = "127.0.0.1:{}".format(p2p_port(2)) @@ -218,6 +218,8 @@ def test_getaddednodeinfo(self): # check that node can be removed self.nodes[0].addnode(node=ip_port, command='remove') assert_equal(self.nodes[0].getaddednodeinfo(), []) + # check that an invalid command returns an error + assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc') # check that trying to remove the node again returns an error assert_raises_rpc_error(-24, "Node could not be removed", self.nodes[0].addnode, node=ip_port, command='remove') # check that a non-existent node returns an error From f52cb02f700b58bca921a7aa24bfeee04760262b Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 21 Oct 2022 15:56:18 -0300 Subject: [PATCH 23/42] doc: make it clear that `node` in `addnode` refers to the node's address --- src/rpc/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index da511d1ed940d..4a24e772e3b11 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -287,7 +287,7 @@ static RPCHelpMan addnode() strprintf("Addnode connections are limited to %u at a time", MAX_ADDNODE_CONNECTIONS) + " and are counted separately from the -maxconnections limit.\n", { - {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node (see getpeerinfo for nodes)"}, + {"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The address of the peer to connect to"}, {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "'add' to add a node to the list, 'remove' to remove a node from the list, 'onetry' to try a connection to the node once"}, }, RPCResult{RPCResult::Type::NONE, "", ""}, From 27b4084e16a1cb210ce27119416ee34625781052 Mon Sep 17 00:00:00 2001 From: Tim Neubauer Date: Thu, 31 Aug 2023 18:59:43 +0200 Subject: [PATCH 24/42] Refactor: Remove m_is_test_chain --- src/kernel/chainparams.cpp | 4 ---- src/kernel/chainparams.h | 3 +-- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index 733a3339b38b3..9968e0fa09f5b 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -151,7 +151,6 @@ class CMainParams : public CChainParams { vFixedSeeds = std::vector(std::begin(chainparams_seed_main), std::end(chainparams_seed_main)); fDefaultConsistencyChecks = false; - m_is_test_chain = false; m_is_mockable_chain = false; checkpointData = { @@ -258,7 +257,6 @@ class CTestNetParams : public CChainParams { vFixedSeeds = std::vector(std::begin(chainparams_seed_test), std::end(chainparams_seed_test)); fDefaultConsistencyChecks = false; - m_is_test_chain = true; m_is_mockable_chain = false; checkpointData = { @@ -380,7 +378,6 @@ class SigNetParams : public CChainParams { bech32_hrp = "tb"; fDefaultConsistencyChecks = false; - m_is_test_chain = true; m_is_mockable_chain = false; } }; @@ -471,7 +468,6 @@ class CRegTestParams : public CChainParams vSeeds.emplace_back("dummySeed.invalid."); fDefaultConsistencyChecks = true; - m_is_test_chain = true; m_is_mockable_chain = true; checkpointData = { diff --git a/src/kernel/chainparams.h b/src/kernel/chainparams.h index 2d38af609c981..285d76b623f82 100644 --- a/src/kernel/chainparams.h +++ b/src/kernel/chainparams.h @@ -103,7 +103,7 @@ class CChainParams /** Default value for -checkmempool and -checkblockindex argument */ bool DefaultConsistencyChecks() const { return fDefaultConsistencyChecks; } /** If this chain is exclusively used for testing */ - bool IsTestChain() const { return m_is_test_chain; } + bool IsTestChain() const { return m_chain_type != ChainType::MAIN; } /** If this chain allows time to be mocked */ bool IsMockableChain() const { return m_is_mockable_chain; } uint64_t PruneAfterHeight() const { return nPruneAfterHeight; } @@ -177,7 +177,6 @@ class CChainParams CBlock genesis; std::vector vFixedSeeds; bool fDefaultConsistencyChecks; - bool m_is_test_chain; bool m_is_mockable_chain; CCheckpointData checkpointData; MapAssumeutxo m_assumeutxo_data; From befb42f1462f886bf5bed562ba1dae00853cecde Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 11 Sep 2023 16:30:58 +0100 Subject: [PATCH 25/42] qt: Silence `-Wcast-function-type` warning Required for https://github.com/bitcoin/bitcoin/pull/25972. Picked from https://trac.nginx.org/nginx/ticket/1865. --- src/qt/winshutdownmonitor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/winshutdownmonitor.cpp b/src/qt/winshutdownmonitor.cpp index 386d593eeaab0..97a9ec318c4f5 100644 --- a/src/qt/winshutdownmonitor.cpp +++ b/src/qt/winshutdownmonitor.cpp @@ -43,7 +43,7 @@ bool WinShutdownMonitor::nativeEventFilter(const QByteArray &eventType, void *pM void WinShutdownMonitor::registerShutdownBlockReason(const QString& strReason, const HWND& mainWinId) { typedef BOOL (WINAPI *PSHUTDOWNBRCREATE)(HWND, LPCWSTR); - PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate"); + PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)(void*)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate"); if (shutdownBRCreate == nullptr) { qWarning() << "registerShutdownBlockReason: GetProcAddress for ShutdownBlockReasonCreate failed"; return; From fa5b6d29ee90911271d4304a6f39c38743a84f33 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 13 Sep 2023 16:07:54 +0200 Subject: [PATCH 26/42] fuzz: Drop unused params from serialize helpers With the ser-type and ser-version going away, it seems unlikely that there is need for them in the future, so just remove them. --- src/test/fuzz/deserialize.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index 100a6b4ee45fb..5809db31bc762 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -91,9 +91,9 @@ void DeserializeFromFuzzingInput(FuzzBufferType buffer, T&& obj, const P& params } template -CDataStream Serialize(const T& obj, const int version = INIT_PROTO_VERSION, const int ser_type = SER_NETWORK) +CDataStream Serialize(const T& obj) { - CDataStream ds(ser_type, version); + CDataStream ds{SER_NETWORK, INIT_PROTO_VERSION}; ds << obj; return ds; } @@ -107,12 +107,10 @@ T Deserialize(CDataStream ds) } template -void DeserializeFromFuzzingInput(FuzzBufferType buffer, T&& obj, const std::optional protocol_version = std::nullopt, const int ser_type = SER_NETWORK) +void DeserializeFromFuzzingInput(FuzzBufferType buffer, T&& obj) { - CDataStream ds(buffer, ser_type, INIT_PROTO_VERSION); - if (protocol_version) { - ds.SetVersion(*protocol_version); - } else { + CDataStream ds{buffer, SER_NETWORK, INIT_PROTO_VERSION}; + { try { int version; ds >> version; @@ -135,9 +133,9 @@ void AssertEqualAfterSerializeDeserialize(const T& obj, const P& params) assert(Deserialize(Serialize(obj, params), params) == obj); } template -void AssertEqualAfterSerializeDeserialize(const T& obj, const int version = INIT_PROTO_VERSION, const int ser_type = SER_NETWORK) +void AssertEqualAfterSerializeDeserialize(const T& obj) { - assert(Deserialize(Serialize(obj, version, ser_type)) == obj); + assert(Deserialize(Serialize(obj)) == obj); } } // namespace From fad52baf1e9bf9d55a300922e73d3bc3169a8843 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 13 Sep 2023 16:14:40 +0200 Subject: [PATCH 27/42] fuzz: Rework addr fuzzing * Replace ConsumeDeserializationParams with V1, because V2 is unconditionally checked as well. * Also fuzz CAddress::Format::Disk in the address_deserialize fuzz target. --- src/test/fuzz/deserialize.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index 5809db31bc762..510ee7fb5b06a 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -252,7 +252,7 @@ FUZZ_TARGET(netaddr_deserialize, .init = initialize_deserialize) if (!maybe_na) return; const CNetAddr& na{*maybe_na}; if (na.IsAddrV1Compatible()) { - AssertEqualAfterSerializeDeserialize(na, ConsumeDeserializationParams(fdp)); + AssertEqualAfterSerializeDeserialize(na, CNetAddr::V1); } AssertEqualAfterSerializeDeserialize(na, CNetAddr::V2); } @@ -264,7 +264,7 @@ FUZZ_TARGET(service_deserialize, .init = initialize_deserialize) if (!maybe_s) return; const CService& s{*maybe_s}; if (s.IsAddrV1Compatible()) { - AssertEqualAfterSerializeDeserialize(s, ConsumeDeserializationParams(fdp)); + AssertEqualAfterSerializeDeserialize(s, CNetAddr::V1); } AssertEqualAfterSerializeDeserialize(s, CNetAddr::V2); if (ser_params.enc == CNetAddr::Encoding::V1) { @@ -279,8 +279,8 @@ FUZZ_TARGET_DESERIALIZE(messageheader_deserialize, { FUZZ_TARGET(address_deserialize, .init = initialize_deserialize) { FuzzedDataProvider fdp{buffer.data(), buffer.size()}; - const auto ser_enc{ConsumeDeserializationParams(fdp)}; - const auto maybe_a{ConsumeDeserializable(fdp, CAddress::SerParams{{ser_enc}, CAddress::Format::Network})}; + const auto ser_enc{ConsumeDeserializationParams(fdp)}; + const auto maybe_a{ConsumeDeserializable(fdp, ser_enc)}; if (!maybe_a) return; const CAddress& a{*maybe_a}; // A CAddress in V1 mode will roundtrip From 275579d8c133c066212a26423639956e2576e97a Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Wed, 13 Sep 2023 14:20:14 -0400 Subject: [PATCH 28/42] Remove MemPoolAccept::m_limits, only have local copies for carveouts --- src/txmempool.cpp | 7 +++---- src/txmempool.h | 2 -- src/validation.cpp | 26 ++++++++++++++------------ 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 92379484e3a46..bedb57c13cc38 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -197,7 +197,6 @@ util::Result CTxMemPool::CalculateAncestorsAndCheckLimit } bool CTxMemPool::CheckPackageLimits(const Package& package, - const Limits& limits, std::string &errString) const { CTxMemPoolEntry::Parents staged_ancestors; @@ -208,8 +207,8 @@ bool CTxMemPool::CheckPackageLimits(const Package& package, std::optional piter = GetIter(input.prevout.hash); if (piter) { staged_ancestors.insert(**piter); - if (staged_ancestors.size() + package.size() > static_cast(limits.ancestor_count)) { - errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count); + if (staged_ancestors.size() + package.size() > static_cast(m_limits.ancestor_count)) { + errString = strprintf("too many unconfirmed parents [limit: %u]", m_limits.ancestor_count); return false; } } @@ -219,7 +218,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package, // considered together must be within limits even if they are not interdependent. This may be // stricter than the limits for each individual transaction. const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(), - staged_ancestors, limits)}; + staged_ancestors, m_limits)}; // It's possible to overestimate the ancestor/descendant totals. if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original; return ancestors.has_value(); diff --git a/src/txmempool.h b/src/txmempool.h index fcef19e8070fe..b26cd4efa6fef 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -606,11 +606,9 @@ class CTxMemPool * @param[in] package Transaction package being evaluated for acceptance * to mempool. The transactions need not be direct * ancestors/descendants of each other. - * @param[in] limits Maximum number and size of ancestors and descendants * @param[out] errString Populated with error reason if a limit is hit. */ bool CheckPackageLimits(const Package& package, - const Limits& limits, std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Populate setDescendants with all in-mempool descendants of hash. diff --git a/src/validation.cpp b/src/validation.cpp index 6e0addc877dd6..cf4728c214e89 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -432,8 +432,7 @@ class MemPoolAccept m_pool(mempool), m_view(&m_dummy), m_viewmempool(&active_chainstate.CoinsTip(), m_pool), - m_active_chainstate(active_chainstate), - m_limits{m_pool.m_limits} + m_active_chainstate(active_chainstate) { } @@ -683,8 +682,6 @@ class MemPoolAccept Chainstate& m_active_chainstate; - CTxMemPool::Limits m_limits; - /** Whether the transaction(s) would replace any mempool transactions. If so, RBF rules apply. */ bool m_rbf{false}; }; @@ -873,6 +870,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (!bypass_limits && !args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); + + // Note that these modifications are only applicable to single transaction scenarios; + // carve-outs and package RBF are disabled for multi-transaction evaluations. + CTxMemPool::Limits maybe_rbf_limits = m_pool.m_limits; + // Calculate in-mempool ancestors, up to a limit. if (ws.m_conflicts.size() == 1) { // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we @@ -905,11 +907,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) assert(ws.m_iters_conflicting.size() == 1); CTxMemPool::txiter conflict = *ws.m_iters_conflicting.begin(); - m_limits.descendant_count += 1; - m_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); + maybe_rbf_limits.descendant_count += 1; + maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, m_limits)}; + auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}; if (!ancestors) { // If CalculateMemPoolAncestors fails second time, we want the original error string. // Contracting/payment channels CPFP carve-out: @@ -925,9 +927,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html CTxMemPool::Limits cpfp_carve_out_limits{ .ancestor_count = 2, - .ancestor_size_vbytes = m_limits.ancestor_size_vbytes, - .descendant_count = m_limits.descendant_count + 1, - .descendant_size_vbytes = m_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, + .ancestor_size_vbytes = maybe_rbf_limits.ancestor_size_vbytes, + .descendant_count = maybe_rbf_limits.descendant_count + 1, + .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, }; const auto error_message{util::ErrorString(ancestors).original}; if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) { @@ -1010,7 +1012,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn { return !m_pool.exists(GenTxid::Txid(tx->GetHash()));})); std::string err_string; - if (!m_pool.CheckPackageLimits(txns, m_limits, err_string)) { + if (!m_pool.CheckPackageLimits(txns, err_string)) { // This is a package-wide error, separate from an individual transaction error. return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string); } @@ -1165,7 +1167,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the // last calculation done in PreChecks, since package ancestors have already been submitted. { - auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_limits)}; + auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_pool.m_limits)}; if(!ancestors) { results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. From c38561d6b1de954b712a92cb8a198ed42d73caea Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 26 May 2020 09:11:05 +0800 Subject: [PATCH 29/42] build: add -zip option to macdeployqtplus This zips the app bundle in /dist. --- contrib/macdeploy/macdeployqtplus | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contrib/macdeploy/macdeployqtplus b/contrib/macdeploy/macdeployqtplus index f8677ba7b8808..0c4ca86b8711c 100755 --- a/contrib/macdeploy/macdeployqtplus +++ b/contrib/macdeploy/macdeployqtplus @@ -397,6 +397,7 @@ ap.add_argument("-no-plugins", dest="plugins", action="store_false", default=Tru ap.add_argument("-no-strip", dest="strip", action="store_false", default=True, help="don't run 'strip' on the binaries") ap.add_argument("-dmg", nargs="?", const="", metavar="basename", help="create a .dmg disk image") ap.add_argument("-translations-dir", nargs=1, metavar="path", default=None, help="Path to Qt's translations. Base translations will automatically be added to the bundle's resources.") +ap.add_argument("-zip", nargs="?", const="", metavar="zip", help="create a .zip containing the app bundle") config = ap.parse_args() @@ -593,6 +594,11 @@ if config.dmg is not None: # ------------------------------------------------ +if config.zip is not None: + shutil.make_archive('{}'.format(appname), format='zip', root_dir='dist', base_dir='Bitcoin-Qt.app') + +# ------------------------------------------------ + print("+ Done +") sys.exit(0) From a128111c29ba0c31763ccbcd316268bfa9c029cd Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 15 Sep 2023 13:46:39 +0100 Subject: [PATCH 30/42] build: produce a .zip for macOS distribution Instead of a .dmg. Co-authored-by: fanquake --- .gitignore | 2 +- Makefile.am | 17 +++++++++-------- ci/test/00_setup_env_mac.sh | 2 +- configure.ac | 1 + contrib/guix/libexec/build.sh | 2 +- contrib/guix/libexec/codesign.sh | 7 ++----- contrib/guix/manifest.scm | 2 +- depends/README.md | 2 +- 8 files changed, 17 insertions(+), 18 deletions(-) diff --git a/.gitignore b/.gitignore index 6ca9d39a16b36..c77303f50e2ee 100644 --- a/.gitignore +++ b/.gitignore @@ -74,7 +74,7 @@ src/qt/bitcoin-qt.includes *.log *.trs -*.dmg +*.zip *.json.h *.raw.h diff --git a/Makefile.am b/Makefile.am index 8b61763ae4d6c..7c3cd82e7f0b9 100644 --- a/Makefile.am +++ b/Makefile.am @@ -37,7 +37,7 @@ space := $(empty) $(empty) OSX_APP=Bitcoin-Qt.app OSX_VOLNAME = $(subst $(space),-,$(PACKAGE_NAME)) -OSX_DMG = $(OSX_VOLNAME).dmg +OSX_ZIP = $(OSX_VOLNAME).zip OSX_DEPLOY_SCRIPT=$(top_srcdir)/contrib/macdeploy/macdeployqtplus OSX_INSTALLER_ICONS=$(top_srcdir)/src/qt/res/icons/bitcoin.icns OSX_PLIST=$(top_builddir)/share/qt/Info.plist #not installed @@ -124,15 +124,16 @@ osx_volname: echo $(OSX_VOLNAME) >$@ if BUILD_DARWIN -$(OSX_DMG): $(OSX_APP_BUILT) $(OSX_PACKAGING) - $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) -dmg +$(OSX_ZIP): $(OSX_APP_BUILT) $(OSX_PACKAGING) + $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) -zip -deploydir: $(OSX_DMG) +deploydir: $(OSX_ZIP) else !BUILD_DARWIN APP_DIST_DIR=$(top_builddir)/dist -$(OSX_DMG): deploydir - $(XORRISOFS) -D -l -V "$(OSX_VOLNAME)" -no-pad -r -dir-mode 0755 -o $@ $(APP_DIST_DIR) -- $(if $(SOURCE_DATE_EPOCH),-volume_date all_file_dates =$(SOURCE_DATE_EPOCH)) +$(OSX_ZIP): deploydir + if [ -n "$(SOURCE_DATE_EPOCH)" ]; then find $(APP_DIST_DIR) -exec touch -d @$(SOURCE_DATE_EPOCH) {} +; fi + cd $(APP_DIST_DIR) && find . | sort | $(ZIP) -X@ $@ $(APP_DIST_DIR)/$(OSX_APP)/Contents/MacOS/Bitcoin-Qt: $(OSX_APP_BUILT) $(OSX_PACKAGING) INSTALL_NAME_TOOL=$(INSTALL_NAME_TOOL) OTOOL=$(OTOOL) STRIP=$(STRIP) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) @@ -140,7 +141,7 @@ $(APP_DIST_DIR)/$(OSX_APP)/Contents/MacOS/Bitcoin-Qt: $(OSX_APP_BUILT) $(OSX_PAC deploydir: $(APP_DIST_DIR)/$(OSX_APP)/Contents/MacOS/Bitcoin-Qt endif !BUILD_DARWIN -deploy: $(OSX_DMG) +deploy: $(OSX_ZIP) endif $(BITCOIN_QT_BIN): FORCE @@ -313,7 +314,7 @@ EXTRA_DIST += \ test/util/data/txcreatesignv2.hex \ test/util/rpcauth-test.py -CLEANFILES = $(OSX_DMG) $(BITCOIN_WIN_INSTALLER) +CLEANFILES = $(OSX_ZIP) $(BITCOIN_WIN_INSTALLER) DISTCHECK_CONFIGURE_FLAGS = --enable-man diff --git a/ci/test/00_setup_env_mac.sh b/ci/test/00_setup_env_mac.sh index 85d30b758f427..4790f305aa52a 100755 --- a/ci/test/00_setup_env_mac.sh +++ b/ci/test/00_setup_env_mac.sh @@ -11,7 +11,7 @@ export SDK_URL=${SDK_URL:-https://bitcoincore.org/depends-sources/sdks} export CONTAINER_NAME=ci_macos_cross export CI_IMAGE_NAME_TAG="docker.io/ubuntu:22.04" export HOST=x86_64-apple-darwin -export PACKAGES="cmake libz-dev python3-setuptools xorriso" +export PACKAGES="cmake libz-dev python3-setuptools xorriso zip" export XCODE_VERSION=12.2 export XCODE_BUILD_ID=12B45b export RUN_UNIT_TESTS=false diff --git a/configure.ac b/configure.ac index 449d4f9ed145b..a5b1d2ed70128 100644 --- a/configure.ac +++ b/configure.ac @@ -800,6 +800,7 @@ case $host in AC_PATH_TOOL([INSTALL_NAME_TOOL], [install_name_tool], [install_name_tool]) AC_PATH_TOOL([OTOOL], [otool], [otool]) AC_PATH_PROGS([XORRISOFS], [xorrisofs], [xorrisofs]) + AC_PATH_PROG([ZIP], [zip], [zip]) dnl libtool will try to strip the static lib, which is a problem for dnl cross-builds because strip attempts to call a hard-coded ld, diff --git a/contrib/guix/libexec/build.sh b/contrib/guix/libexec/build.sh index 7ca272209805b..c49553bec3ce4 100755 --- a/contrib/guix/libexec/build.sh +++ b/contrib/guix/libexec/build.sh @@ -315,7 +315,7 @@ mkdir -p "$DISTSRC" | gzip -9n > "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.tar.gz" \ || ( rm -f "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.tar.gz" && exit 1 ) ) - make deploy ${V:+V=1} OSX_DMG="${OUTDIR}/${DISTNAME}-${HOST}-unsigned.dmg" + make deploy ${V:+V=1} OSX_ZIP="${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip" ;; esac ( diff --git a/contrib/guix/libexec/codesign.sh b/contrib/guix/libexec/codesign.sh index 6ffa0f07b2e90..0b5f77d01ea2b 100755 --- a/contrib/guix/libexec/codesign.sh +++ b/contrib/guix/libexec/codesign.sh @@ -85,11 +85,8 @@ mkdir -p "$DISTSRC" # Apply detached codesignatures to dist/ (in-place) signapple apply dist/Bitcoin-Qt.app codesignatures/osx/dist - # Make a DMG from dist/ - xorrisofs -D -l -V "$(< osx_volname)" -no-pad -r -dir-mode 0755 \ - -o "${OUTDIR}/${DISTNAME}-${HOST}.dmg" \ - dist \ - -- -volume_date all_file_dates ="$SOURCE_DATE_EPOCH" + # Make a .zip from dist/ + zip "${OUTDIR}/${DISTNAME}-${HOST}.zip" dist/* ;; *) exit 1 diff --git a/contrib/guix/manifest.scm b/contrib/guix/manifest.scm index 01018263997de..0c273dbb0353e 100644 --- a/contrib/guix/manifest.scm +++ b/contrib/guix/manifest.scm @@ -606,5 +606,5 @@ inspecting signatures in Mach-O binaries.") ((string-contains target "-linux-") (list (make-bitcoin-cross-toolchain target))) ((string-contains target "darwin") - (list clang-toolchain-15 binutils cmake-minimal xorriso python-signapple)) + (list clang-toolchain-15 binutils cmake-minimal xorriso python-signapple zip)) (else '()))))) diff --git a/depends/README.md b/depends/README.md index b5992a3feff06..75750aff21d15 100644 --- a/depends/README.md +++ b/depends/README.md @@ -48,7 +48,7 @@ The paths are automatically configured and no other options are needed unless ta #### For macOS cross compilation - sudo apt-get install curl bsdmainutils cmake libz-dev python3-setuptools xorriso + sudo apt-get install curl bsdmainutils cmake libz-dev python3-setuptools xorriso zip Note: You must obtain the macOS SDK before proceeding with a cross-compile. Under the depends directory, create a subdirectory named `SDKs`. From 33ae0bd1e4756ca0f180ac4b3c32c9eb83b88cfd Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 18 Nov 2022 12:04:51 +0000 Subject: [PATCH 31/42] macdeploy: remove DMG generation from deploy script --- configure.ac | 1 - contrib/macdeploy/background.tiff | Bin 18464 -> 0 bytes contrib/macdeploy/macdeployqtplus | 105 ++---------------------------- 3 files changed, 4 insertions(+), 102 deletions(-) delete mode 100644 contrib/macdeploy/background.tiff diff --git a/configure.ac b/configure.ac index a5b1d2ed70128..3b3edab6de95d 100644 --- a/configure.ac +++ b/configure.ac @@ -1937,7 +1937,6 @@ AC_CONFIG_FILES([contrib/devtools/split-debug.sh],[chmod +x contrib/devtools/spl AM_COND_IF([HAVE_DOXYGEN], [AC_CONFIG_FILES([doc/Doxyfile])]) AC_CONFIG_LINKS([contrib/devtools/iwyu/bitcoin.core.imp:contrib/devtools/iwyu/bitcoin.core.imp]) AC_CONFIG_LINKS([contrib/filter-lcov.py:contrib/filter-lcov.py]) -AC_CONFIG_LINKS([contrib/macdeploy/background.tiff:contrib/macdeploy/background.tiff]) AC_CONFIG_LINKS([src/.bear-tidy-config:src/.bear-tidy-config]) AC_CONFIG_LINKS([src/.clang-tidy:src/.clang-tidy]) AC_CONFIG_LINKS([test/functional/test_runner.py:test/functional/test_runner.py]) diff --git a/contrib/macdeploy/background.tiff b/contrib/macdeploy/background.tiff deleted file mode 100644 index 1fb088c8374ac3acd5c4f4eb6ea6b5869723fd2f..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 18464 zcmeHtc|4Tg`}d3)OV&_Cwvs)vWEs22l08M1LQG8d8Pv#9DJ_a*9mW!(5=xdVQA#p2 zM1@G}%v82AA^UUgTYW!$zMt>&{Fdk6c#Zoy*SXI7dSA;qbJy!wSt%mcaUu{$UybK> zC#LTed;CW2fvl}M=Nkasjk(NKXM?5+jZUu0F8O3r#LZt=@%D&HseG>t8+Uipo& zN}Uz}FMAN4(iuFL&_T1q%f^#a$;NUI-@HDY%Uvs>qaN|`WWYmJ;w!1SCYQh%l#{4z zru#BjDKYmq15ePl70p`Qsh#C)%n*F-Vz{1&@yqS!>NPNB@1mp$=H!~NB?TU6+DQ$g zw{ur6w^Tr0FFBk1LiMPx!Y1}&V?ae#RxR$4D^EYES4@h%IMt3V{Gu)HGnnkOW4mN) zckr2X`^yM>T(AxGm1SM(yEiu3dA++eF;cnV>c<0p1gz1RvMbFg7!6E-^i{irH?g>Y zUDiV%kK5FxB7GB6_wAPT(|oR0b?PjtDi3gxBUkq|vD+UJ*#BduKO)C`op9F9#3t`^ zvB&1ZGYlF+F22nOS(DwwE20v-JeR+6aUyo_B*N>qRETTgWSxpWF?jOYD^lw^TSKGy z_cdkq{dxP8A}`;mKHD$*?3+Qqd7VV`j?pcCDbgXk3QZ*!XHTbyhiu)g+u>RTdb6Xe z`D=iCNeDj}dpvPZ^`Cz@oY~)<6vijfFPk4HHjv+PV<=gFE8+4TyObk-Sxf9u6#?I# zYo(`>WxS{6Wxn|2VTVeD0>~e?hNWia79S^S5q;|)Sy2yeCvpjh%X{U$j+;VGJos?^ zbVo8L$*)d)uTF<6`bpzHp0=(#ZO0CLi@SX1l+A%sGgOw};Ecj-E*YaaN7H8PIxt$2 zoGFu9{b8x8UuDZHO_zDRcGv4RgQEh0-gfs6ygTU<*ZerbTIMvW^qiqnC~>J^?N{{2%z=tmuT`}by$=kIZgzW} zd89i%nt$89@uhyxazg46>1W@=za`$qF?i1?+c@RgxC!uhHGhA5s1#X#B04YItgXu> zR((RV;00CR;2bBZGJij9Jl$qHaNGG-yMyD+V zrufZ{kUOHuHtXqD*6Y+gKaKbrZs-9mxT5ooT2rdTYs-nQ7R@SEd6ubHX*d)bZnLRi z@1*bL%ui>8h4S=QCIo8ir|X1lB~s-x^BSh=sn&0z9Jdtg9rN|D{Jt>L=c+ZgeE305 zL{?!(<>Ku7BP|zPX$PFqnRPwqzbk+PbE94FsyDv=h1axHd;A((+<3Ow3+b%h zU7IWzX{FWXwn<;q>~L8g!J7MWW!vMeY;e>37Sm=W)tpbBVV){nJT9vZEzdlR-hNx* zjJn0BCRaP@VP zxs~{PPPXS;7W4P8saixJQyHY9JB;JbkHlWYuvfFt$sft=)rRy9k7JBSy_3G|kX^~d zEZo^)%tb0{FgNCsU^u>Ri*?Sx=<4zMpYC#e9m1t?93zK$gDlU-+&ISPAJgS%@M7ua zRzkQChd+^iZKC<~XK!i?$sb+)HjMX$Cd(@X{hEr`lo%mAQoQGUR#C$Tp^{4gVG>!M ziIMm&*%JkHmpUp{{o*n>{5KKy^n=#_z_-W#kWs*K%qM;J>Ky>J?zF-osuh)JJnEP9 zWd!B6jj$&<$|&F@<`bI5EeUgDA;dc>>kjEK#hwS}+(x=RVbeWUx2gEugeblV%I=iEl!}9QLm-!AXeZE6T?LWx)9=33I8H zwS2o=d@pOcI(x75$1+U#7uFyj-r^ajta8jHHdf`FRFg9&g`S@H&PmMS%Xb^w-xJw& zt|rt(>FJ4T4&wGRc8vu=Pfw)9aypTi(}~e0qhQv=#F}pGc6u?8H`l_1i-71YDj3gV zQXffu>>%&67Orj;dlDxwB{%#T0p>@ao_M~^KNoXJV&%0n;b4$sPa)>gflVPymA#1y z{(RY}&ugcAd5d>GiN}P$XAKJAEpETvoDF=%nsm}1y;ArnOpq$yF+%6jt-Lf#Hx@lB zM;IZ=3s+Of`;MK`&qCn@raS?x;8YIg(maR2FoS}_@fN#HT=%)UNwE(D6JZ1&LIuI+?L!DPad5DKtb;>>vC#AHKQk|3 zCx|{}2Jzy@RSI3Fnsyf;zG!aFF)TS{F;UCIF;atIgrA6P?+{(pXg0wW;a z(2peea1#QYtie+S@IU42Z3fA@K(6xf-suEn9UzZI;+*Y(Yz(?7a)rQ*(M;w8d|#<$cKUt+Svmc_(Gla zA(*-WSsuu({#YxhW?O*#1s}2-vQb1J(7e7_vzI4z36=oI3&w05;r@Fp zfnFWRd&2R1ptXPxTf!lnxfPJjfqcUsZwcu^ylh`X1K~acIbm1v#_zNMdXN+Lqxgv3 zP#!>x?8VsNy-+?ukFbv*oRQuLEFuJP5d8H)1S8%dq!0uI4iO1Ueh6T0N(lHsBMA8u zjO_E%qYCh7DsdGN0p|S?V_@DNNVbT#u;wUW7zDIesz?J5Uo z9`f?}r@Pj~Xy_UNV!2&tDB?sp)hZmz9=0>=YoM(PFnu16ntd-VUK zs()4U7q`Dx)&BRsfji0fAAJLSvG%g|u#T`kM@WI^N7fgtpMW&PIt+dX|HbF88bCdT z1NzmTSnW0+a7h2+U;;7|2+szAY(SO5!gaC$>%2jJ2_SnBpzoksD}xUBy%vA(5U8hr zpNBeP)w>_ST^;?^@jkEy>JZ33)Ghza&;MkH{I)+11z!>k4Lwj2y?_l}tshi7>>0Zm zJH*MYs<%|fREt$_sxnna;4HxPQ|(tBSAC&+16oc1Hyr-0onh`;o+Y<`}baj zGX8rHLAm_zG5S5%e?LK1s}Ju|UTI!6UUptZUQJ#TUL@}(@T<;i3?ymZ&Ac|e!eG{t zSB}?=cjLeE@vCBiKm&CU)OmmP;_A8Q3RwQ?G$=lQP<=dn%6b2c_upfb`dgIH35ffB zpCNg`PG0r2dWQV+`75t*h5vSf?ggH3h$Dap9^@4|9fHBxBlR~M=j*?O%2ksZDZR7WDI15mO!hat-z^gkB~x}pf%CP zKV|47)Ca%NIk=i5Wwa5PHAI`Cb)Yly_h`x_lz=k*hj1^k7ABf_JcW z2oZz?LK=LlD+8|D2tDvUyA!-O*dz8JToDHVJAY71JjiV{;shcAk%+j2NJnHN@)5;| z>xc?O6`~H&gm{2>jOax4AYLGbz&8*LF^c$#m_aNckw|tV4^j{*f|NwcAeE5nNFAgh zawpOTxf|(<^g{X}gOL%)XkW0SoX8{v4pWiv7BN_V##7DWVy*w%kqGwljQ}=Tb5ClX%qs* zg%Uz-LMfwkP^Kt*lpE?03Xh6KokwM$3Q-l{6lh2Fq28j#P;+QDv>;jvtqf{oiQbF$ zL5HDZ(HGF!=xgX&^dod1`W^Z!dYP4nRg6`MRhQL@bswuA>k-xj)(qAX)*99}aB9<7 zXV}=-gg{?tgMM*kJIr>B?E+gaTP53lwr6Y~*rwUp*+tlu*bUh2*}d7r*%R1F>^Iq4 z*n8P&>_0fTI3zjLIm|iSID$D&a%6Da;ArOP(am{fH za4T|~a(i$`a3^t>a^L43jl1|$t|{9 zqPLW7c_p((Mn}e9CR3(eW>!{C)MdG@<#Gu@`dux71$JX z6ao}-6?zm|6g3t76>}83l~|OtlyFM9O3${kZpCa3-deb|UzuCkL^)EqO!=LPkcy4U zNtHU4iEUf9xou0?*1m07RZ}$(e7_B;38?K-JE_*7Hl?np?xUWg{#=7c!(1ay1yeQ>sIMb>M835=-tr!q%WtB)i2inV6fT1+u(}9TSF;BFT(=Ew?>NYX^iQ8GnAQy*?BYSPJx|!cV_P#G2d+NXI^Ih z)k4$an8kfdR!eKk6w7`qaVsCI>sDWPY43{J^~jpr+Q~ZG`n`>!4c?}~mc@3LZL00C zos3(Jyxq5LU(CKvS5a4->s>c?H)ppJw`upC?&?8vDki;>TfXLEgXd-II*D)Pni z6Z7W_a0LTbEUwfR$`)o6u@yxY(Tkmn+pp?ey&tDg+b^rEt6J~q+^MLRsV3J5)|{_J z))H&K*7?`Hz3Y1SS^cj1)&{+Xnnsnz(xy#KIrrAzOK#?DKGnR^64f$&KlJ|C1HT6! zT0L8b9`1Yi{E_1$YMXW2lgH+dAGVvcw>&X;($t~bQBTpK)OBig)^=%j)jZXFT0_;M z)^=-m-|fNlG(6LP*4%5<`=HOP@9}f1=bbO?UOaob_vJvpNB`Ra-+@nqfrH;(MZQ`X zIu2g!&%a*#I%7m^r0|Wxo65IZZ|}V`d)M{e>HW|LpAVxS2_F|}r|3NN42C4*`X}{I zO`pv__k3~xLK_VoT^LIk=N~6e$WK&H8c%k8-S?ID4gYOr>H<@QS^9nZ_tt5L>5-YB znT6T&KSX|9o70(VpLdyOEF4*6U(8xkSZY|dULIZvT3G@Ab2#j1H>$dBrz72HkQW)7 zvbKZ=H#*ixolGy=5~b9x)7uzxb}YNwv}mfTsO2a#*{Y&cjK-cBrb5v-A!6Qpq!CX} z;UCw_Gto5KS%M>dNt%UmyS#9Lppb^4jPf(oBRdU0^@%ffk5nyi zc7ru*SJ$kebc!$vy9CoRrKvkfa`M~!$cgHclZraR*b$@6Z#~S@=X&}KsaKup&8;h9 zVJgB~32Bnx=~O$GYVjhH`$huAK}xrC}XQKF}a zMhpQ(LjFXoNSI2$A|WmLf==c*MU2)LmqNe=4#qUNU)0I$3>t%C<0WBo-KRihc= zG-d_}wJEOZKFH*92Mfe0ho6d#BH0gI2vtE*q8B%9V)V)bz%qUYo}_Lu_w_7c@OqbH zEOi@RWt$2i?akLja_9QmG5M7cfY|{suh80t5>EP~B-G7pgS zwtVyuz$Q(ZN>j1@0I|vQqsPcTx_rwgz>blDq3C&VFsj}Gq*Tx&lY>=7ixezWO;00; zU)|GsZWX3lh${8!e!88IPNo^DXpwQ2FlUTuIrGlLVPeu-M>uWa*aj*qZCN6L;9Ui& zv&L4{{oVBi!!1G3To6G;einF;pPf}Db5WLrB^f92D{bw-AnqHa_KYTTrT$dUV*qB| zQw$ASl7yiw4bjQ?w1rNcb}`y=Vlsi^IMMRs-v;Ckc!o3vWuP;~j(kQ`{C zkup`yqJE_sHtKsXMo3$*Py9KNj!9iAZNtzqra>m%BRl6o`05t*Ef)ybB>YM(#$^=+ z8sNezx?r$$K(Dk7G6R?{beoxcAqHrszs|bZJ0Ng|2Er3=E_~*aKuGK=Jmf@5EIi zzz+xq45+>T<(qiO|$-%UfXiF03#mc zDEZ=3;%G;3N!v2+9F&0nOOS9}_Fw~+lJl8I0gx8soVtbUSWAY^8*nBGEmUm7kHk~~ z2gGyC^8B$wZewM&aEJV!{kT}TqC@YYf*2Vw;z2%d-^PT1G9O&2el!<}O{yALIq*KZ z4Z?1ZO<{JWzP3qR8WmvZ84NzH9;gfY@Q+$9!u?U#HrJzWLEIBaZG=D=GSoW2wypW_ zIp&K_G3@QhmCXz;aHt=YT--@L5>u6fyIuL_@m#BYq=xcL{O?o&ub*YE_Y|6EFrv*# zMf!SL3~(H|j(KM4yaDHt&_b;YRT%VTo8j9;vQZ198T9-ZohsKz5?V+KB3CkfwfhcA z)?EA#Iqg;=v~2B4d`hf>mURj+8o?=a(6i;yTq{JKZ+r51BVYjKg!rK!lg+G-tz2LI0?rfuPnZq?Xvyc zMYLa@jcM_tl1%-~*VLKT#hXTPh}0JCYmpL5yB!p}6iT)68EOvF@hiC<0=}P^V7CXe_fgfy-ntKFFlL^4(&6O!vDMA<2oY$E#w7SO}gEiI3hbz8PwojwlEm zsVx}p4~@KcNw98EUgH4YeiHE)EH0Eo6Ttdd_}(QYe>L5a=c= zKdaf7x!ltqKb~X9itSWCQC~?i^mEDQ%YUhXO5*{{n-4WicQ@C`8^*HZ25Xd6XEn*n zanp6rT1(FtV_(1?wI(NyG|ni?rDiz7lrTj5759u?*gLjk}X^lCb&&8jPpuc2u(2`3H z=~^;CaZ`x9-w@Q~-BUX>XcBz(@r1k!(eus@TC%A=UHc{SWHSU`wHl7}vryFd&JFLL z`2KZ|5ToO!t=y#ynW3rk_5JSx<+s(Bj4KyopVUV0o;ae@`B5Je#*`_>w%2+boH(k} zNz+%T@UU?^6xg|mR&nOel|_z84>gvwi#I)&Rc8&z>qGOpVu)MihxWA;3}@aHpr+Oa zEe4T!NYRc|4db0;xoLy(fn29WwHoacRY;PUBb9WBMwUBGJ#*L?6Gd9| zqLSziI`)@G^CVG}+kEGm9D>LcS=DriRLA7C1~)`@TbJbB?b>n^Y6r<<=g$-g%q@pkrl>y@)z#$ z9mhv=rI>V$OZbV-p5|-*VZ|kHQ5Or&;i<4gy~|?0aSN#hV!1XiRkdTtIGwEcukMB@ z+9zMUX+5i}icbDl8F>r2R2jyqsBcLeO|q*LbEoEt)b3+%KXRBzvSyj*`=33{cm|6GVxm}+uQt&P_N zZNB??&!i8I=p!cI?(fbfKTBBhi+VeI)o#dD}2bll#e2Qc$HEhR%YX@pslblb0ajw*R|m$g7S>A zPL0Of$rEY3t)f{ZO*%v2K$k(rv9HTV;-5P#^m;fX&54=c2E9Dr-F(cDx98!H@t^{+ zY0QPqZNuZ!`AgPDT}@|d$3K{0CI}mLTGcZ~jRxZl1DwR!nNNb0%)9EBHNste+IDV@ zF65xx9&(OOi=PVhO~)I)KOs{fwlcq7oL->6)%7&FKe&k{jpwFkiX$)OVcOou#1Bf~ z^e`#8tIAG$M^Svgcv4v|zG@BWfj?e=*=QM;30FKw*_k z3;n@@8&4?orkS}ilfkY_@&Xi-`dw9cjAZ{YE%_l4fwmJEc^7MESw>$SnT3*&V5a84 zOL>u+PEquEko~L}+flpuJ~`utywVxw4i}db)i#696AGnQZ%`<2n`##nd(~@WY2pO8 z8x?_}RO@@?#n`8{Q??l8z3y7_4B?;(b}T`iCJN@G+)~-+4%3qM?sTY?b0VinZ~P*W z!@l@v;QdrvL7-#F@NPTn>tkUQW6vwGy`35+gC8%+S7!C?)~)KB{oaCgpWNb5i`hbv zqgAJ6+^kutbZCk85trX6E^jI z$RMRSfR$avJThI6!!bpQu}7CE8v$0uSk>f0!;y&uLCemnAR0Hoa;<$^hV;yiud*fK zcVd*Us<<~Xvl_p=+>fF>#!>|7)oHPAvn_Go4T2109BNgZ)}=|8y^`?UPyBvv@rznP zV60`DFa@WOb9yndgFjB*Df&CFL!{~!${MgTeXORPuh9=bTVF8dZJEXe>`Vs}N0*1L z^?V!H;1H>{g@OWR*T=Suw2?GriBS*K*s?dq@`QY; z43GxL9lce7B;6UhiPHq5?447sp00qPSDy;%r8t*#DU{0ewnn~G$6j0o%~Wuw;LcUw z5;rcfQG@3=Nb1Gp3`?#dZ(j;8;k~SKAp0(BOo5n4t;pr@*=vETevq@*Y+yfJ^=q-l zu%A{+nj!3mbF0&DG7`f?ni?c16V z@ILV}VE2&mIy3LdxKb<2eJnpth`Wle`S5^l*I(MA;HX=-P`uPI8a2|qkl)1SZq+=y zO(L(K;Xa``uxqNiWnjEM&Ff)BYYpzvVEZF+KY#$(s5{Q+reE&T+A z&)dhs9xvK;UoGyt#V*Qh)X%(Z_fX<+*W@>kUAjESZmO*`Fn0Ii28u4uWg0+pDiC6> zd#Sf@i*p@g_eq|{`I-iUOo2c};gw5}uc3e!EiI6*$a3%YMe|i;3Fgo$GO;UU6?sgp zlkhf<$bLJRM;093-LyPqm+^1-Tlp z;gpL#nMGBmlCu;deq0q#2=_{*|h%@gnsO5dP zlsraLVB(zpQP%*jE$waoSUaOK&FfhC{WFVckSjNSpJp2D;c-^wYKqk9WqHvieEFms>w#W6Tp6IuCiYV0JKMX;l1KFx6lib0FrX^-xMGm=0Xh0yvp$5w?9 zRb&P#($(n!Fg^`t2Lb@>zC)d=bog)VK*v_Ad9G$go2~(SM!`9IkRXnsgI)sdekH?G zAM|7;cG(w7A|Rv>X@0Vkf;hWNQW77Z+E&=sdwqV zW;!O_`yp>{(|CQTw-e^cR0%_uXDGliqfWmc2T)7qS_!!c<8HW&ZLlJ}bgl-ns9{lV zneiy~ZgeiD3wB4im3sTR8GiG?M2JS-p!>n#?QYBOAaJ{vpv55Y!Kru{d@y6u00K|1 zn474xdl*?B+Nr(jZL%I3hIdDZ)&{g4tLOt%UlVB0aRZ<8oJ6$7ZZU%10i=Z$Lpvr3 zh9YKkuhO~(Y6kI4$*_m9*WZ|HgB907qF`rbAT5x^gN%Hj09{u;Hv(e=I*~2cxHm@L zrMc&gZV&X4D4d%RuYL5S6FQ>YgTWDncASXIp*%dWQ@}N8%Wm?V6vz3#*-zRVjz>7A z&oG|Y`P4KO!_)Y^W=h?2bX9e0 zDt%S8StIWAs_M=Zmhthskuh%Ud6!?VwMZmxY=GjIOAw9xyfR5piu(f0E)oyZXTidu ztWxLY76Lf0V#>dFg+NBMfHs%q7noA-%x32kT`Lh(+(3He-YZkM`M2B0DE&85h@}L4 zYf_7I3S|hgH;g28xkG|}WG-bR5Ng%6mnV_ogE8s=4*UMpiD?tT%EFGkP~vllu1W)0m#{a>rH(ajJKbM zHGQ1g@!9q9Y#C61p7ZK#dJUc%nbj0eU}$JBl8lRxLUR3j;3d@TQZ!t8J+&i zbF~2M7&+j2v=4A+K&|m@E(h6`V7Mv1d5OUoZ2Yu>; zF<!90s8E?^Z%&)&IRu0uQEur{A)#JtevZ0M8`wIYM&~@K$C&wL<~x zBjG$}4Krn`d#-k3ENpnV#itAPpi zohIMnYs~2C?p=kH0E)c1R09Jhk?}FDN-f#9;S$7L;>)%dMw&9e-K_nD0(T#O-cmiz z0`4{bl%Wf#|J|FltiU!@>%YEP8xQQDo3)U=2eAKjv(^U4zi!s<0rLO$X6-4k7W#p% zH6nj|)`1w`f{z0Tst8=(&;*|fF$4lDg+T1wf@y8@J$I Date: Fri, 18 Nov 2022 12:36:20 +0000 Subject: [PATCH 32/42] build: remove dmg dependencies --- ci/test/00_setup_env_mac.sh | 2 +- configure.ac | 1 - contrib/guix/manifest.scm | 3 +-- contrib/macdeploy/README.md | 19 +++++++------------ depends/README.md | 2 +- depends/packages/native_ds_store.mk | 15 --------------- depends/packages/native_mac_alias.mk | 15 --------------- depends/packages/packages.mk | 2 +- doc/build-osx.md | 12 +++--------- doc/release-process.md | 2 +- 10 files changed, 15 insertions(+), 58 deletions(-) delete mode 100644 depends/packages/native_ds_store.mk delete mode 100644 depends/packages/native_mac_alias.mk diff --git a/ci/test/00_setup_env_mac.sh b/ci/test/00_setup_env_mac.sh index 4790f305aa52a..6fabb4ab96d78 100755 --- a/ci/test/00_setup_env_mac.sh +++ b/ci/test/00_setup_env_mac.sh @@ -11,7 +11,7 @@ export SDK_URL=${SDK_URL:-https://bitcoincore.org/depends-sources/sdks} export CONTAINER_NAME=ci_macos_cross export CI_IMAGE_NAME_TAG="docker.io/ubuntu:22.04" export HOST=x86_64-apple-darwin -export PACKAGES="cmake libz-dev python3-setuptools xorriso zip" +export PACKAGES="cmake libz-dev python3-setuptools zip" export XCODE_VERSION=12.2 export XCODE_BUILD_ID=12B45b export RUN_UNIT_TESTS=false diff --git a/configure.ac b/configure.ac index 3b3edab6de95d..9b4b9bd42bbbe 100644 --- a/configure.ac +++ b/configure.ac @@ -799,7 +799,6 @@ case $host in AC_PATH_TOOL([DSYMUTIL], [dsymutil], [dsymutil]) AC_PATH_TOOL([INSTALL_NAME_TOOL], [install_name_tool], [install_name_tool]) AC_PATH_TOOL([OTOOL], [otool], [otool]) - AC_PATH_PROGS([XORRISOFS], [xorrisofs], [xorrisofs]) AC_PATH_PROG([ZIP], [zip], [zip]) dnl libtool will try to strip the static lib, which is a problem for diff --git a/contrib/guix/manifest.scm b/contrib/guix/manifest.scm index 0c273dbb0353e..3c4ad6cdbc9fc 100644 --- a/contrib/guix/manifest.scm +++ b/contrib/guix/manifest.scm @@ -3,7 +3,6 @@ ((gnu packages bash) #:select (bash-minimal)) (gnu packages bison) ((gnu packages certs) #:select (nss-certs)) - ((gnu packages cdrom) #:select (xorriso)) ((gnu packages cmake) #:select (cmake-minimal)) (gnu packages commencement) (gnu packages compression) @@ -606,5 +605,5 @@ inspecting signatures in Mach-O binaries.") ((string-contains target "-linux-") (list (make-bitcoin-cross-toolchain target))) ((string-contains target "darwin") - (list clang-toolchain-15 binutils cmake-minimal xorriso python-signapple zip)) + (list clang-toolchain-15 binutils cmake-minimal python-signapple zip)) (else '()))))) diff --git a/contrib/macdeploy/README.md b/contrib/macdeploy/README.md index 599a0bfa6ce49..16fb0dad219d6 100644 --- a/contrib/macdeploy/README.md +++ b/contrib/macdeploy/README.md @@ -6,7 +6,7 @@ The `macdeployqtplus` script should not be run manually. Instead, after building make deploy ``` -When complete, it will have produced `Bitcoin-Core.dmg`. +When complete, it will have produced `Bitcoin-Core.zip`. ## SDK Extraction @@ -60,10 +60,10 @@ previous stage) as the first argument. The `sha256sum` of the generated TAR.GZ archive should be `df75d30ecafc429e905134333aeae56ac65fac67cb4182622398fd717df77619`. -## Deterministic macOS DMG Notes +## Deterministic macOS App Notes -Working macOS DMGs are created in Linux by combining a recent `clang`, the Apple -`binutils` (`ld`, `ar`, etc) and DMG authoring tools. +macOS Applications are created in Linux by combining a recent `clang` and the Apple +`binutils` (`ld`, `ar`, etc). Apple uses `clang` extensively for development and has upstreamed the necessary functionality so that a vanilla clang can take advantage. It supports the use of `-F`, @@ -93,20 +93,15 @@ created using these tools. The build process has been designed to avoid includin SDK's files in Guix's outputs. All interim tarballs are fully deterministic and may be freely redistributed. -[`xorrisofs`](https://www.gnu.org/software/xorriso/) is used to create the DMG. - -A background image is added to DMG files by inserting a `.DS_Store` during creation. - As of OS X 10.9 Mavericks, using an Apple-blessed key to sign binaries is a requirement in order to satisfy the new Gatekeeper requirements. Because this private key cannot be shared, we'll have to be a bit creative in order for the build process to remain somewhat deterministic. Here's how it works: -- Builders use Guix to create an unsigned release. This outputs an unsigned DMG which +- Builders use Guix to create an unsigned release. This outputs an unsigned ZIP which users may choose to bless and run. It also outputs an unsigned app structure in the form - of a tarball, which also contains all of the tools that have been previously (deterministically) - built in order to create a final DMG. + of a tarball. - The Apple keyholder uses this unsigned app to create a detached signature, using the script that is also included there. Detached signatures are available from this [repository](https://github.com/bitcoin-core/bitcoin-detached-sigs). - Builders feed the unsigned app + detached signature back into Guix. It uses the - pre-built tools to recombine the pieces into a deterministic DMG. + pre-built tools to recombine the pieces into a deterministic ZIP. diff --git a/depends/README.md b/depends/README.md index 75750aff21d15..8a964b25770dc 100644 --- a/depends/README.md +++ b/depends/README.md @@ -48,7 +48,7 @@ The paths are automatically configured and no other options are needed unless ta #### For macOS cross compilation - sudo apt-get install curl bsdmainutils cmake libz-dev python3-setuptools xorriso zip + sudo apt-get install curl bsdmainutils cmake libz-dev python3-setuptools zip Note: You must obtain the macOS SDK before proceeding with a cross-compile. Under the depends directory, create a subdirectory named `SDKs`. diff --git a/depends/packages/native_ds_store.mk b/depends/packages/native_ds_store.mk deleted file mode 100644 index 51a95f48ef7b1..0000000000000 --- a/depends/packages/native_ds_store.mk +++ /dev/null @@ -1,15 +0,0 @@ -package=native_ds_store -$(package)_version=1.3.0 -$(package)_download_path=https://github.com/dmgbuild/ds_store/archive/ -$(package)_file_name=v$($(package)_version).tar.gz -$(package)_sha256_hash=76b3280cd4e19e5179defa23fb594a9dd32643b0c80d774bd3108361d94fb46d -$(package)_install_libdir=$(build_prefix)/lib/python3/dist-packages - -define $(package)_build_cmds - python3 setup.py build -endef - -define $(package)_stage_cmds - mkdir -p $($(package)_install_libdir) && \ - python3 setup.py install --root=$($(package)_staging_dir) --prefix=$(build_prefix) --install-lib=$($(package)_install_libdir) -endef diff --git a/depends/packages/native_mac_alias.mk b/depends/packages/native_mac_alias.mk deleted file mode 100644 index ddd631186edf7..0000000000000 --- a/depends/packages/native_mac_alias.mk +++ /dev/null @@ -1,15 +0,0 @@ -package=native_mac_alias -$(package)_version=2.2.0 -$(package)_download_path=https://github.com/dmgbuild/mac_alias/archive/ -$(package)_file_name=v$($(package)_version).tar.gz -$(package)_sha256_hash=421e6d7586d1f155c7db3e7da01ca0dacc9649a509a253ad7077b70174426499 -$(package)_install_libdir=$(build_prefix)/lib/python3/dist-packages - -define $(package)_build_cmds - python3 setup.py build -endef - -define $(package)_stage_cmds - mkdir -p $($(package)_install_libdir) && \ - python3 setup.py install --root=$($(package)_staging_dir) --prefix=$(build_prefix) --install-lib=$($(package)_install_libdir) -endef diff --git a/depends/packages/packages.mk b/depends/packages/packages.mk index b3600b72d0b54..dd55c939cbb1d 100644 --- a/depends/packages/packages.mk +++ b/depends/packages/packages.mk @@ -27,7 +27,7 @@ multiprocess_native_packages = native_libmultiprocess native_capnp usdt_linux_packages=systemtap -darwin_native_packages = native_ds_store native_mac_alias +darwin_native_packages = ifneq ($(build_os),darwin) darwin_native_packages += native_cctools native_libtapi diff --git a/doc/build-osx.md b/doc/build-osx.md index f11ed97e098ae..76fc3579569bd 100644 --- a/doc/build-osx.md +++ b/doc/build-osx.md @@ -163,14 +163,8 @@ brew install python #### Deploy Dependencies -You can deploy a `.dmg` containing the Bitcoin Core application using `make deploy`. -This command depends on a couple of python packages, so it is required that you have `python` installed. - -Ensuring that `python` is installed, you can install the deploy dependencies by running the following commands in your terminal: - -``` bash -pip3 install ds_store mac_alias -``` +You can deploy a `.zip` containing the Bitcoin Core application using `make deploy`. +It is required that you have `python` installed. ## Building Bitcoin Core @@ -230,7 +224,7 @@ make check # Run tests if Python 3 is available ### 3. Deploy (optional) -You can also create a `.dmg` containing the `.app` bundle by running the following command: +You can also create a `.zip` containing the `.app` bundle by running the following command: ``` bash make deploy diff --git a/doc/release-process.md b/doc/release-process.md index c7a8998b8bf8e..bdef57243bffa 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -123,7 +123,7 @@ git -C ./guix.sigs pull ### Create the macOS SDK tarball (first time, or when SDK version changes) Create the macOS SDK tarball, see the [macdeploy -instructions](/contrib/macdeploy/README.md#deterministic-macos-dmg-notes) for +instructions](/contrib/macdeploy/README.md#deterministic-macos-app-notes) for details. ### Build and attest to build outputs From 78c2707b2aefa9e0aee5ddceaeab31997085a241 Mon Sep 17 00:00:00 2001 From: Tim Neubauer Date: Fri, 1 Sep 2023 14:26:18 +0200 Subject: [PATCH 33/42] Refactor: Replace 'isMockableChain' with inline 'ChainType' check for 'submitpackage' --- src/rpc/mempool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 377e9de0e84b3..705608bd476a3 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -862,7 +862,7 @@ static RPCHelpMan submitpackage() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - if (!Params().IsMockableChain()) { + if (Params().GetChainType() != ChainType::REGTEST) { throw std::runtime_error("submitpackage is for regression testing (-regtest mode) only"); } const UniValue raw_transactions = request.params[0].get_array(); From c8eb8dae51039aa1938e7040001a149210e87275 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Sun, 29 Jan 2023 18:59:00 +0530 Subject: [PATCH 34/42] rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table --- src/rpc/net.cpp | 50 +++++++++++++++++++++++++++++++++++++++++++ src/test/fuzz/rpc.cpp | 1 + 2 files changed, 51 insertions(+) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index f7b6c68344667..e9e9ad62aeb33 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -1016,6 +1016,55 @@ static RPCHelpMan sendmsgtopeer() }; } +static RPCHelpMan getaddrmaninfo() +{ + return RPCHelpMan{"getaddrmaninfo", + "\nProvides information about the node's address manager by returning the number of " + "addresses in the `new` and `tried` tables and their sum for all networks.\n" + "This RPC is for testing only.\n", + {}, + RPCResult{ + RPCResult::Type::OBJ_DYN, "", "json object with network type as keys", + { + {RPCResult::Type::OBJ, "network", "the network (" + Join(GetNetworkNames(), ", ") + ")", + { + {RPCResult::Type::NUM, "new", "number of addresses in the new table, which represent potential peers the node has discovered but hasn't yet successfully connected to."}, + {RPCResult::Type::NUM, "tried", "number of addresses in the tried table, which represent peers the node has successfully connected to in the past."}, + {RPCResult::Type::NUM, "total", "total number of addresses in both new/tried tables"}, + }}, + } + }, + RPCExamples{ + HelpExampleCli("getaddrmaninfo", "") + + HelpExampleRpc("getaddrmaninfo", "") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue + { + NodeContext& node = EnsureAnyNodeContext(request.context); + if (!node.addrman) { + throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Address manager functionality missing or disabled"); + } + + UniValue ret(UniValue::VOBJ); + for (int n = 0; n < NET_MAX; ++n) { + enum Network network = static_cast(n); + if (network == NET_UNROUTABLE || network == NET_INTERNAL) continue; + UniValue obj(UniValue::VOBJ); + obj.pushKV("new", node.addrman->Size(network, true)); + obj.pushKV("tried", node.addrman->Size(network, false)); + obj.pushKV("total", node.addrman->Size(network)); + ret.pushKV(GetNetworkName(network), obj); + } + UniValue obj(UniValue::VOBJ); + obj.pushKV("new", node.addrman->Size(std::nullopt, true)); + obj.pushKV("tried", node.addrman->Size(std::nullopt, false)); + obj.pushKV("total", node.addrman->Size()); + ret.pushKV("all_networks", obj); + return ret; + }, + }; +} + void RegisterNetRPCCommands(CRPCTable& t) { static const CRPCCommand commands[]{ @@ -1035,6 +1084,7 @@ void RegisterNetRPCCommands(CRPCTable& t) {"hidden", &addconnection}, {"hidden", &addpeeraddress}, {"hidden", &sendmsgtopeer}, + {"hidden", &getaddrmaninfo}, }; for (const auto& c : commands) { t.appendCommand(c.name, &c); diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 24ec0e4a73722..543c9933bb935 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -110,6 +110,7 @@ const std::vector RPC_COMMANDS_SAFE_FOR_FUZZING{ "generate", "generateblock", "getaddednodeinfo", + "getaddrmaninfo", "getbestblockhash", "getblock", "getblockchaininfo", From ee589d4466bb0548a6f2215afe8abd0735768dab Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Thu, 14 Sep 2023 13:13:57 -0400 Subject: [PATCH 35/42] Add regression test for m_limit mutation --- test/functional/mempool_limit.py | 51 ++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 0abebbec02583..a1147f70f33e1 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -78,6 +78,56 @@ def fill_mempool(self): assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) + def test_rbf_carveout_disallowed(self): + node = self.nodes[0] + + self.log.info("Check that individually-evaluated transactions in a package don't increase package limits for other subpackage parts") + + # We set chain limits to 2 ancestors, 1 descendant, then try to get a parents-and-child chain of 2 in mempool + # + # A: Solo transaction to be RBF'd (to bump descendant limit for package later) + # B: First transaction in package, RBFs A by itself under individual evaluation, which would give it +1 descendant limit + # C: Second transaction in package, spends B. If the +1 descendant limit persisted, would make it into mempool + + self.restart_node(0, extra_args=self.extra_args[0] + ["-limitancestorcount=2", "-limitdescendantcount=1"]) + + # Generate a confirmed utxo we will double-spend + rbf_utxo = self.wallet.send_self_transfer( + from_node=node, + confirmed_only=True + )["new_utxo"] + self.generate(node, 1) + + # tx_A needs to be RBF'd, set minfee at set size + A_weight = 1000 + mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"] + tx_A = self.wallet.send_self_transfer( + from_node=node, + fee=(mempoolmin_feerate / 1000) * (A_weight // 4) + Decimal('0.000001'), + target_weight=A_weight, + utxo_to_spend=rbf_utxo, + confirmed_only=True + ) + + # RBF's tx_A, is not yet submitted + tx_B = self.wallet.create_self_transfer( + fee=tx_A["fee"] * 4, + target_weight=A_weight, + utxo_to_spend=rbf_utxo, + confirmed_only=True + ) + + # Spends tx_B's output, too big for cpfp carveout (because that would also increase the descendant limit by 1) + non_cpfp_carveout_weight = 40001 # EXTRA_DESCENDANT_TX_SIZE_LIMIT + 1 + tx_C = self.wallet.create_self_transfer( + target_weight=non_cpfp_carveout_weight, + fee = (mempoolmin_feerate / 1000) * (non_cpfp_carveout_weight // 4) + Decimal('0.000001'), + utxo_to_spend=tx_B["new_utxo"], + confirmed_only=True + ) + + assert_raises_rpc_error(-26, "too-long-mempool-chain", node.submitpackage, [tx_B["hex"], tx_C["hex"]]) + def test_mid_package_eviction(self): node = self.nodes[0] self.log.info("Check a package where each parent passes the current mempoolminfee but would cause eviction before package submission terminates") @@ -324,6 +374,7 @@ def run_test(self): self.test_mid_package_replacement() self.test_mid_package_eviction() + self.test_rbf_carveout_disallowed() if __name__ == '__main__': From fa3b5e5e57cd4649d577e06a3aca798b55a75fd5 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 19 Sep 2023 09:36:44 +0000 Subject: [PATCH 36/42] ci: Use nproc over MAKEJOBS in 01_base_install --- ci/test/01_base_install.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh index b05d7547c8dfd..424dca52dc6c0 100755 --- a/ci/test/01_base_install.sh +++ b/ci/test/01_base_install.sh @@ -51,7 +51,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then -DLLVM_ENABLE_RUNTIMES="compiler-rt;libcxx;libcxxabi;libunwind" \ -S /msan/llvm-project/llvm - ninja -C /msan/clang_build/ "$MAKEJOBS" + ninja -C /msan/clang_build/ "-j$( nproc )" # Use nproc, because MAKEJOBS is the default in docker image builds ninja -C /msan/clang_build/ install-runtimes update-alternatives --install /usr/bin/clang++ clang++ /msan/clang_build/bin/clang++ 100 @@ -69,13 +69,13 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then -DLIBCXX_HARDENING_MODE=debug \ -S /msan/llvm-project/runtimes - ninja -C /msan/cxx_build/ "$MAKEJOBS" + ninja -C /msan/cxx_build/ "-j$( nproc )" # Use nproc, because MAKEJOBS is the default in docker image builds fi if [[ "${RUN_TIDY}" == "true" ]]; then git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 /include-what-you-use cmake -B /iwyu-build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S /include-what-you-use - make -C /iwyu-build/ install "$MAKEJOBS" + make -C /iwyu-build/ install "-j$( nproc )" # Use nproc, because MAKEJOBS is the default in docker image builds fi mkdir -p "${DEPENDS_DIR}/SDKs" "${DEPENDS_DIR}/sdk-sources" From 28bac81a346c0b68273fa73af924f7096cb3f41d Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Sun, 29 Jan 2023 23:54:43 +0530 Subject: [PATCH 37/42] test: add functional test for getaddrmaninfo --- test/functional/rpc_net.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 255f5108a2556..4fbe22a8462af 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -66,6 +66,7 @@ def run_test(self): self.test_getnodeaddresses() self.test_addpeeraddress() self.test_sendmsgtopeer() + self.test_getaddrmaninfo() def test_connection_count(self): self.log.info("Test getconnectioncount") @@ -360,6 +361,28 @@ def test_sendmsgtopeer(self): node.sendmsgtopeer(peer_id=0, msg_type="addr", msg=zero_byte_string.hex()) self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0, timeout=10) + def test_getaddrmaninfo(self): + self.log.info("Test getaddrmaninfo") + node = self.nodes[1] + + self.log.debug("Test that getaddrmaninfo is a hidden RPC") + # It is hidden from general help, but its detailed help may be called directly. + assert "getaddrmaninfo" not in node.help() + assert "getaddrmaninfo" in node.help("getaddrmaninfo") + + # current count of ipv4 addresses in addrman is {'new':1, 'tried':1} + self.log.info("Test that count of addresses in addrman match expected values") + res = node.getaddrmaninfo() + assert_equal(res["ipv4"]["new"], 1) + assert_equal(res["ipv4"]["tried"], 1) + assert_equal(res["ipv4"]["total"], 2) + assert_equal(res["all_networks"]["new"], 1) + assert_equal(res["all_networks"]["tried"], 1) + assert_equal(res["all_networks"]["total"], 2) + for net in ["ipv6", "onion", "i2p", "cjdns"]: + assert_equal(res[net]["new"], 0) + assert_equal(res[net]["tried"], 0) + assert_equal(res[net]["total"], 0) if __name__ == '__main__': NetTest().main() From 533660c58ad5a218671a9bc1537299b1d67bb55d Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Wed, 13 Sep 2023 13:13:57 -0400 Subject: [PATCH 38/42] Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion While allowing submitted packages to be slightly larger than what may be allowed in the mempool to allow simpler reasoning about contextual-less checks vs chain limits. --- doc/policy/packages.md | 11 +++++++---- src/policy/packages.cpp | 8 ++++---- src/policy/packages.h | 20 ++++++++++++-------- src/test/txpackage_tests.cpp | 12 ++++++------ 4 files changed, 29 insertions(+), 22 deletions(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 2a5758318a9b6..399ae945d52db 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -18,16 +18,19 @@ tip or some preceding transaction in the package. The following rules are enforced for all packages: -* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KvB` total size +* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_WEIGHT=404000` total weight (#20833) - - *Rationale*: This is already enforced as mempool ancestor/descendant limits. If - transactions in a package are all related, exceeding this limit would mean that the package - can either be split up or it wouldn't pass individual mempool policy. + - *Rationale*: We want package size to be as small as possible to mitigate DoS via package + validation. However, we want to make sure that the limit does not restrict ancestor + packages that would be allowed if submitted individually. - Note that, if these mempool limits change, package limits should be reconsidered. Users may also configure their mempool limits differently. + - Note that the this is transaction weight, not "virtual" size as with other limits to allow + simpler context-less checks. + * Packages must be topologically sorted. (#20833) * Packages cannot have conflicting transactions, i.e. no two transactions in a package can spend diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index a901ef8f38c7a..fd272a2642e4c 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -23,10 +23,10 @@ bool CheckPackage(const Package& txns, PackageValidationState& state) return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); } - const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, - [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); - // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. - if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { + const int64_t total_weight = std::accumulate(txns.cbegin(), txns.cend(), 0, + [](int64_t sum, const auto& tx) { return sum + GetTransactionWeight(*tx); }); + // If the package only contains 1 tx, it's better to report the policy violation on individual tx weight. + if (package_count > 1 && total_weight > MAX_PACKAGE_WEIGHT) { return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); } diff --git a/src/policy/packages.h b/src/policy/packages.h index 0a0e7cf6bb85e..702667b8ade8d 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -15,18 +15,22 @@ /** Default maximum number of transactions in a package. */ static constexpr uint32_t MAX_PACKAGE_COUNT{25}; -/** Default maximum total virtual size of transactions in a package in KvB. */ -static constexpr uint32_t MAX_PACKAGE_SIZE{101}; -static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_WEIGHT); +/** Default maximum total weight of transactions in a package in weight + to allow for context-less checks. This must allow a superset of sigops + weighted vsize limited transactions to not disallow transactions we would + have otherwise accepted individually. */ +static constexpr uint32_t MAX_PACKAGE_WEIGHT = 404'000; +static_assert(MAX_PACKAGE_WEIGHT >= MAX_STANDARD_TX_WEIGHT); -// If a package is submitted, it must be within the mempool's ancestor/descendant limits. Since a -// submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor +// If a package is to be evaluated, it must be at least as large as the mempool's ancestor/descendant limits, +// otherwise transactions that would be individually accepted may be rejected in a package erroneously. +// Since a submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor // of the child), package limits are ultimately bounded by mempool package limits. Ensure that the // defaults reflect this constraint. static_assert(DEFAULT_DESCENDANT_LIMIT >= MAX_PACKAGE_COUNT); static_assert(DEFAULT_ANCESTOR_LIMIT >= MAX_PACKAGE_COUNT); -static_assert(DEFAULT_ANCESTOR_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE); -static_assert(DEFAULT_DESCENDANT_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE); +static_assert(MAX_PACKAGE_WEIGHT >= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000); +static_assert(MAX_PACKAGE_WEIGHT >= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000); /** A "reason" why a package was invalid. It may be that one or more of the included * transactions is invalid or the package itself violates our rules. @@ -47,7 +51,7 @@ class PackageValidationState : public ValidationState { /** Context-free package policy checks: * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT. - * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE. + * 2. The total weight cannot exceed MAX_PACKAGE_WEIGHT. * 3. If any dependencies exist between transactions, parents must appear before children. * 4. Transactions cannot conflict, i.e., spend the same inputs. */ diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 10ab656d38821..571b58156f737 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -51,14 +51,14 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) BOOST_CHECK_EQUAL(state_too_many.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state_too_many.GetRejectReason(), "package-too-many-transactions"); - // Packages can't have a total size of more than 101KvB. + // Packages can't have a total weight of more than 404'000WU. CTransactionRef large_ptx = create_placeholder_tx(150, 150); Package package_too_large; - auto size_large = GetVirtualTransactionSize(*large_ptx); - size_t total_size{0}; - while (total_size <= MAX_PACKAGE_SIZE * 1000) { + auto size_large = GetTransactionWeight(*large_ptx); + size_t total_weight{0}; + while (total_weight <= MAX_PACKAGE_WEIGHT) { package_too_large.push_back(large_ptx); - total_size += size_large; + total_weight += size_large; } BOOST_CHECK(package_too_large.size() <= MAX_PACKAGE_COUNT); PackageValidationState state_too_large; @@ -122,7 +122,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. CTransactionRef giant_ptx = create_placeholder_tx(999, 999); - BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > MAX_PACKAGE_SIZE * 1000); + BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000); auto result_single_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {giant_ptx}, /*test_accept=*/true); BOOST_CHECK(result_single_large.m_state.IsInvalid()); BOOST_CHECK_EQUAL(result_single_large.m_state.GetResult(), PackageValidationResult::PCKG_TX); From bc013fe8e3d0bae2ab766a8248219aa3c9e344df Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 26 Aug 2023 00:09:16 +0000 Subject: [PATCH 39/42] Bugfix: Pass correct virtual size to CheckPackageLimits --- src/txmempool.cpp | 5 ++--- src/txmempool.h | 2 ++ src/validation.cpp | 6 ++++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index bedb57c13cc38..960be428c6501 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -197,12 +197,11 @@ util::Result CTxMemPool::CalculateAncestorsAndCheckLimit } bool CTxMemPool::CheckPackageLimits(const Package& package, + const int64_t total_vsize, std::string &errString) const { CTxMemPoolEntry::Parents staged_ancestors; - int64_t total_size = 0; for (const auto& tx : package) { - total_size += GetVirtualTransactionSize(*tx); for (const auto& input : tx->vin) { std::optional piter = GetIter(input.prevout.hash); if (piter) { @@ -217,7 +216,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package, // When multiple transactions are passed in, the ancestors and descendants of all transactions // considered together must be within limits even if they are not interdependent. This may be // stricter than the limits for each individual transaction. - const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(), + const auto ancestors{CalculateAncestorsAndCheckLimits(total_vsize, package.size(), staged_ancestors, m_limits)}; // It's possible to overestimate the ancestor/descendant totals. if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original; diff --git a/src/txmempool.h b/src/txmempool.h index b26cd4efa6fef..8ea3373cb47be 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -606,9 +606,11 @@ class CTxMemPool * @param[in] package Transaction package being evaluated for acceptance * to mempool. The transactions need not be direct * ancestors/descendants of each other. + * @param[in] total_vsize Sum of virtual sizes for all transactions in package. * @param[out] errString Populated with error reason if a limit is hit. */ bool CheckPackageLimits(const Package& package, + int64_t total_vsize, std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Populate setDescendants with all in-mempool descendants of hash. diff --git a/src/validation.cpp b/src/validation.cpp index 8b5acf9ad173b..db3310c2c365d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -634,6 +634,7 @@ class MemPoolAccept // Enforce package mempool ancestor/descendant limits (distinct from individual // ancestor/descendant limits done in PreChecks). bool PackageMempoolChecks(const std::vector& txns, + int64_t total_vsize, PackageValidationState& package_state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Run the script checks using our policy flags. As this can be slow, we should @@ -1003,6 +1004,7 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) } bool MemPoolAccept::PackageMempoolChecks(const std::vector& txns, + const int64_t total_vsize, PackageValidationState& package_state) { AssertLockHeld(cs_main); @@ -1013,7 +1015,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn { return !m_pool.exists(GenTxid::Txid(tx->GetHash()));})); std::string err_string; - if (!m_pool.CheckPackageLimits(txns, err_string)) { + if (!m_pool.CheckPackageLimits(txns, total_vsize, err_string)) { // This is a package-wide error, separate from an individual transaction error. return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string); } @@ -1298,7 +1300,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // because it's unnecessary. Also, CPFP carve out can increase the limit for individual // transactions, but this exemption is not extended to packages in CheckPackageLimits(). std::string err_string; - if (txns.size() > 1 && !PackageMempoolChecks(txns, package_state)) { + if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } From 1a579f9d01256b0b2570168496d129a8b2009b35 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 11 Sep 2023 09:55:39 -0400 Subject: [PATCH 40/42] Handle over-sized (in virtual bytes) packages with no in-mempool ancestors --- src/txmempool.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 960be428c6501..e021cfb06e376 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -200,6 +200,23 @@ bool CTxMemPool::CheckPackageLimits(const Package& package, const int64_t total_vsize, std::string &errString) const { + size_t pack_count = package.size(); + + // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist + if (pack_count > static_cast(m_limits.ancestor_count)) { + errString = strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_count, m_limits.ancestor_count); + return false; + } else if (pack_count > static_cast(m_limits.descendant_count)) { + errString = strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_count, m_limits.descendant_count); + return false; + } else if (total_vsize > m_limits.ancestor_size_vbytes) { + errString = strprintf("package size %u exceeds ancestor size limit [limit: %u]", total_vsize, m_limits.ancestor_size_vbytes); + return false; + } else if (total_vsize > m_limits.descendant_size_vbytes) { + errString = strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes); + return false; + } + CTxMemPoolEntry::Parents staged_ancestors; for (const auto& tx : package) { for (const auto& input : tx->vin) { From eb8f58f5e4a249d55338304099e8238896d2316d Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Thu, 14 Sep 2023 16:29:55 -0400 Subject: [PATCH 41/42] Add functional test to catch too large vsize packages --- test/functional/mempool_sigoplimit.py | 46 ++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index 962b2b19bd8ef..fbec6d0dc8b7b 100755 --- a/test/functional/mempool_sigoplimit.py +++ b/test/functional/mempool_sigoplimit.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 sigop limit mempool policy (`-bytespersigop` parameter)""" +from decimal import Decimal from math import ceil from test_framework.messages import ( @@ -25,6 +26,7 @@ OP_TRUE, ) from test_framework.script_util import ( + keys_to_multisig_script, script_to_p2wsh_script, ) from test_framework.test_framework import BitcoinTestFramework @@ -32,9 +34,10 @@ assert_equal, assert_greater_than, assert_greater_than_or_equal, + assert_raises_rpc_error, ) from test_framework.wallet import MiniWallet - +from test_framework.wallet_util import generate_keypair DEFAULT_BYTES_PER_SIGOP = 20 # default setting @@ -133,6 +136,45 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops): assert_equal(entry_parent['descendantcount'], 2) assert_equal(entry_parent['descendantsize'], parent_tx.get_vsize() + sigop_equivalent_vsize) + def test_sigops_package(self): + self.log.info("Test a overly-large sigops-vbyte hits package limits") + # Make a 2-transaction package which fails vbyte checks even though + # separately they would work. + self.restart_node(0, extra_args=["-bytespersigop=5000"] + self.extra_args[0]) + + def create_bare_multisig_tx(utxo_to_spend=None): + _, pubkey = generate_keypair() + amount_for_bare = 50000 + tx_dict = self.wallet.create_self_transfer(fee=Decimal("3"), utxo_to_spend=utxo_to_spend) + tx_utxo = tx_dict["new_utxo"] + tx = tx_dict["tx"] + tx.vout.append(CTxOut(amount_for_bare, keys_to_multisig_script([pubkey], k=1))) + tx.vout[0].nValue -= amount_for_bare + tx_utxo["txid"] = tx.rehash() + tx_utxo["value"] -= Decimal("0.00005000") + return (tx_utxo, tx) + + tx_parent_utxo, tx_parent = create_bare_multisig_tx() + tx_child_utxo, tx_child = create_bare_multisig_tx(tx_parent_utxo) + + # Separately, the parent tx is ok + parent_individual_testres = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0] + assert parent_individual_testres["allowed"] + # Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops + assert_equal(parent_individual_testres["vsize"], 5000 * 20) + + # But together, it's exceeding limits in the *package* context. If sigops adjusted vsize wasn't being checked + # here, it would get further in validation and give too-long-mempool-chain error instead. + packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()]) + assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"]) + + # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits + assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()]) + assert tx_parent.rehash() in self.nodes[0].getrawmempool() + + # Transactions are tiny in weight + assert_greater_than(2000, tx_parent.get_weight() + tx_child.get_weight()) + def run_test(self): self.wallet = MiniWallet(self.nodes[0]) @@ -149,6 +191,8 @@ def run_test(self): self.generate(self.wallet, 1) + self.test_sigops_package() + if __name__ == '__main__': BytesPerSigOpTest().main() From 43cd8029fa39e0bd4bf6fb896952952bcae16160 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 20 Sep 2023 21:49:57 +0100 Subject: [PATCH 42/42] ci: Install Homebrew's `pkg-config` package Some versions of macOS images lack the 'pkg-config' package, which is required for the build process. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0ef705250d7ec..28a027b780639 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,7 +68,7 @@ jobs: run: clang --version - name: Install Homebrew packages - run: brew install boost libevent qt@5 miniupnpc libnatpmp ccache zeromq qrencode libtool automake gnu-getopt + run: brew install automake libtool pkg-config gnu-getopt ccache boost libevent miniupnpc libnatpmp zeromq qt@5 qrencode - name: Set Ccache directory run: echo "CCACHE_DIR=${RUNNER_TEMP}/ccache_dir" >> "$GITHUB_ENV"