diff options
37 files changed, 718 insertions, 316 deletions
diff --git a/build-aux/m4/bitcoin_qt.m4 b/build-aux/m4/bitcoin_qt.m4 index 232a79bb36..5b5a8ed16e 100644 --- a/build-aux/m4/bitcoin_qt.m4 +++ b/build-aux/m4/bitcoin_qt.m4 @@ -150,7 +150,7 @@ AC_DEFUN([BITCOIN_QT_CONFIGURE],[ AC_DEFINE(QT_QPA_PLATFORM_WINDOWS, 1, [Define this symbol if the qt platform is windows]) elif test "x$TARGET_OS" = xlinux; then dnl workaround for https://bugreports.qt.io/browse/QTBUG-74874 - AX_CHECK_LINK_FLAG([-lxcb-shm], [QT_LIBS="-lxcb-shm $QT_LIBS"], [AC_MSG_ERROR([could not link against -lxcb-shm])]) + AX_CHECK_LINK_FLAG([-lxcb-shm], [QT_LIBS="$QT_LIBS -lxcb-shm"], [AC_MSG_ERROR([could not link against -lxcb-shm])]) _BITCOIN_QT_CHECK_STATIC_PLUGIN([QXcbIntegrationPlugin], [-lqxcb]) AC_DEFINE(QT_QPA_PLATFORM_XCB, 1, [Define this symbol if the qt platform is xcb]) elif test "x$TARGET_OS" = xdarwin; then diff --git a/contrib/guix/guix-build b/contrib/guix/guix-build index 69c244a6fa..29d6701b25 100755 --- a/contrib/guix/guix-build +++ b/contrib/guix/guix-build @@ -139,6 +139,28 @@ for host in $HOSTS; do done ################ +# VERSION_BASE should have enough space +################ + +avail_KiB="$(df -Pk "$VERSION_BASE" | sed 1d | tr -s ' ' | cut -d' ' -f4)" +total_required_KiB=0 +for host in $HOSTS; do + case "$host" in + *darwin*) required_KiB=440000 ;; + *mingw*) required_KiB=7600000 ;; + *) required_KiB=6400000 ;; + esac + total_required_KiB=$((total_required_KiB+required_KiB)) +done + +if (( total_required_KiB > avail_KiB )); then + total_required_GiB=$((total_required_KiB / 1048576)) + avail_GiB=$((avail_KiB / 1048576)) + echo "Performing a Bitcoin Core Guix build for the selected HOSTS requires ${total_required_GiB} GiB, however, only ${avail_GiB} GiB is available. Please free up some disk space before performing the build." + exit 1 +fi + +################ # Check that we can connect to the guix-daemon ################ diff --git a/contrib/guix/libexec/build.sh b/contrib/guix/libexec/build.sh index 46bfa29b74..3073b41baf 100755 --- a/contrib/guix/libexec/build.sh +++ b/contrib/guix/libexec/build.sh @@ -447,4 +447,6 @@ mkdir -p "$DISTSRC" esac ) # $DISTSRC -mv --no-target-directory "$OUTDIR" "$ACTUAL_OUTDIR" +rm -rf "$ACTUAL_OUTDIR" +mv --no-target-directory "$OUTDIR" "$ACTUAL_OUTDIR" \ + || ( rm -rf "$ACTUAL_OUTDIR" && exit 1 ) diff --git a/contrib/guix/libexec/codesign.sh b/contrib/guix/libexec/codesign.sh index 46b42a5712..1822da7ca4 100755 --- a/contrib/guix/libexec/codesign.sh +++ b/contrib/guix/libexec/codesign.sh @@ -100,4 +100,6 @@ mkdir -p "$DISTSRC" esac ) # $DISTSRC -mv --no-target-directory "$OUTDIR" "$ACTUAL_OUTDIR" +rm -rf "$ACTUAL_OUTDIR" +mv --no-target-directory "$OUTDIR" "$ACTUAL_OUTDIR" \ + || ( rm -rf "$ACTUAL_OUTDIR" && exit 1 ) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index c279a9af2f..5beb833b48 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -56,7 +56,7 @@ static void CoinSelection(benchmark::Bench& bench) bench.run([&] { std::set<CInputCoin> setCoinsRet; CAmount nValueRet; - bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params); + bool success = wallet.AttemptSelection(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params); assert(success); assert(nValueRet == 1003 * COIN); assert(setCoinsRet.size() == 2); diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 1dd1e92e2f..35b6160cea 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -6,6 +6,7 @@ #define BITCOIN_INTERFACES_NODE_H #include <amount.h> // For CAmount +#include <external_signer.h> #include <net.h> // For NodeId #include <net_types.h> // For banmap_t #include <netaddress.h> // For Network @@ -110,6 +111,11 @@ public: //! Disconnect node by id. virtual bool disconnectById(NodeId id) = 0; +#ifdef ENABLE_EXTERNAL_SIGNER + //! List external signers + virtual std::vector<ExternalSigner> externalSigners() = 0; +#endif + //! Get total bytes recv. virtual int64_t getTotalBytesRecv() = 0; diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 88f93321f9..a0cb2787b7 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -118,6 +118,9 @@ public: //! Save or remove receive request. virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; + //! Display address on external signer + virtual bool displayAddress(const CTxDestination& dest) = 0; + //! Lock coin. virtual void lockCoin(const COutPoint& output) = 0; @@ -252,6 +255,9 @@ public: // Return whether private keys enabled. virtual bool privateKeysDisabled() = 0; + // Return whether wallet uses an external signer. + virtual bool hasExternalSigner() = 0; + // Get default address type. virtual OutputType getDefaultAddressType() = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 8befbf5e30..171f15d4fb 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -170,6 +170,16 @@ public: } return false; } +#ifdef ENABLE_EXTERNAL_SIGNER + std::vector<ExternalSigner> externalSigners() override + { + std::vector<ExternalSigner> signers = {}; + const std::string command = gArgs.GetArg("-signer", ""); + if (command == "") return signers; + ExternalSigner::Enumerate(command, signers, Params().NetworkIDString()); + return signers; + } +#endif int64_t getTotalBytesRecv() override { return m_context->connman ? m_context->connman->GetTotalBytesRecv() : 0; } int64_t getTotalBytesSent() override { return m_context->connman ? m_context->connman->GetTotalBytesSent() : 0; } size_t getMempoolSize() override { return m_context->mempool ? m_context->mempool->size() : 0; } diff --git a/src/qt/createwalletdialog.cpp b/src/qt/createwalletdialog.cpp index 113bd30a0c..e593697b46 100644 --- a/src/qt/createwalletdialog.cpp +++ b/src/qt/createwalletdialog.cpp @@ -6,6 +6,7 @@ #include <config/bitcoin-config.h> #endif +#include <external_signer.h> #include <qt/createwalletdialog.h> #include <qt/forms/ui_createwalletdialog.h> @@ -27,14 +28,39 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : }); connect(ui->encrypt_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) { - // Disable the disable_privkeys_checkbox when isEncryptWalletChecked is + // Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is // set to true, enable it when isEncryptWalletChecked is false. ui->disable_privkeys_checkbox->setEnabled(!checked); + ui->external_signer_checkbox->setEnabled(!checked); // When the disable_privkeys_checkbox is disabled, uncheck it. if (!ui->disable_privkeys_checkbox->isEnabled()) { ui->disable_privkeys_checkbox->setChecked(false); } + + // When the external_signer_checkbox box is disabled, uncheck it. + if (!ui->external_signer_checkbox->isEnabled()) { + ui->external_signer_checkbox->setChecked(false); + } + + }); + + connect(ui->external_signer_checkbox, &QCheckBox::toggled, [this](bool checked) { + ui->encrypt_wallet_checkbox->setEnabled(!checked); + ui->blank_wallet_checkbox->setEnabled(!checked); + ui->disable_privkeys_checkbox->setEnabled(!checked); + ui->descriptor_checkbox->setEnabled(!checked); + + // The external signer checkbox is only enabled when a device is detected. + // In that case it is checked by default. Toggling it restores the other + // options to their default. + 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); }); connect(ui->disable_privkeys_checkbox, &QCheckBox::toggled, [this](bool checked) { @@ -63,11 +89,22 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) : ui->descriptor_checkbox->setToolTip(tr("Compiled without sqlite support (required for descriptor wallets)")); ui->descriptor_checkbox->setEnabled(false); ui->descriptor_checkbox->setChecked(false); + ui->external_signer_checkbox->setEnabled(false); + ui->external_signer_checkbox->setChecked(false); #endif + #ifndef USE_BDB ui->descriptor_checkbox->setEnabled(false); ui->descriptor_checkbox->setChecked(true); #endif + +#ifndef ENABLE_EXTERNAL_SIGNER + //: "External signing" means using devices such as hardware wallets. + ui->external_signer_checkbox->setToolTip(tr("Compiled without external signing support (required for external signing)")); + ui->external_signer_checkbox->setEnabled(false); + ui->external_signer_checkbox->setChecked(false); +#endif + } CreateWalletDialog::~CreateWalletDialog() @@ -75,6 +112,28 @@ CreateWalletDialog::~CreateWalletDialog() delete ui; } +#ifdef ENABLE_EXTERNAL_SIGNER +void CreateWalletDialog::setSigners(std::vector<ExternalSigner>& signers) +{ + if (!signers.empty()) { + ui->external_signer_checkbox->setEnabled(true); + ui->external_signer_checkbox->setChecked(true); + ui->encrypt_wallet_checkbox->setEnabled(false); + ui->encrypt_wallet_checkbox->setChecked(false); + // The order matters, because connect() is called when toggling a checkbox: + ui->blank_wallet_checkbox->setEnabled(false); + ui->blank_wallet_checkbox->setChecked(false); + ui->disable_privkeys_checkbox->setEnabled(false); + ui->disable_privkeys_checkbox->setChecked(true); + const std::string label = signers[0].m_name; + ui->wallet_name_line_edit->setText(QString::fromStdString(label)); + ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(true); + } else { + ui->external_signer_checkbox->setEnabled(false); + } +} +#endif + QString CreateWalletDialog::walletName() const { return ui->wallet_name_line_edit->text(); @@ -99,3 +158,8 @@ bool CreateWalletDialog::isDescriptorWalletChecked() const { return ui->descriptor_checkbox->isChecked(); } + +bool CreateWalletDialog::isExternalSignerChecked() const +{ + return ui->external_signer_checkbox->isChecked(); +} diff --git a/src/qt/createwalletdialog.h b/src/qt/createwalletdialog.h index 20cce937c8..585b1461f7 100644 --- a/src/qt/createwalletdialog.h +++ b/src/qt/createwalletdialog.h @@ -9,6 +9,10 @@ class WalletModel; +#ifdef ENABLE_EXTERNAL_SIGNER +class ExternalSigner; +#endif + namespace Ui { class CreateWalletDialog; } @@ -23,11 +27,16 @@ public: explicit CreateWalletDialog(QWidget* parent); virtual ~CreateWalletDialog(); +#ifdef ENABLE_EXTERNAL_SIGNER + void setSigners(std::vector<ExternalSigner>& signers); +#endif + QString walletName() const; bool isEncryptWalletChecked() const; bool isDisablePrivateKeysChecked() const; bool isMakeBlankWalletChecked() const; bool isDescriptorWalletChecked() const; + bool isExternalSignerChecked() const; private: Ui::CreateWalletDialog *ui; diff --git a/src/qt/forms/createwalletdialog.ui b/src/qt/forms/createwalletdialog.ui index 881869a46c..b11fb026b0 100644 --- a/src/qt/forms/createwalletdialog.ui +++ b/src/qt/forms/createwalletdialog.ui @@ -109,6 +109,16 @@ </property> </widget> </item> + <item> + <widget class="QCheckBox" name="external_signer_checkbox"> + <property name="toolTip"> + <string>Use an external signing device such as a hardware wallet. Configure the external signer script in wallet preferences first.</string> + </property> + <property name="text"> + <string>External signer</string> + </property> + </widget> + </item> </layout> </widget> </item> @@ -143,6 +153,7 @@ <tabstop>disable_privkeys_checkbox</tabstop> <tabstop>blank_wallet_checkbox</tabstop> <tabstop>descriptor_checkbox</tabstop> + <tabstop>external_signer_checkbox</tabstop> </tabstops> <resources/> <connections> diff --git a/src/qt/forms/optionsdialog.ui b/src/qt/forms/optionsdialog.ui index f199e8c1a1..bd72328c02 100644 --- a/src/qt/forms/optionsdialog.ui +++ b/src/qt/forms/optionsdialog.ui @@ -230,6 +230,36 @@ </widget> </item> <item> + <widget class="QGroupBox" name="groupBoxHww"> + <property name="title"> + <string>External Signer (e.g. hardware wallet)</string> + </property> + <layout class="QVBoxLayout" name="verticalLayoutHww"> + <item> + <layout class="QHBoxLayout" name="horizontalLayoutHww"> + <item> + <widget class="QLabel" name="externalSignerPathLabel"> + <property name="text"> + <string>&External signer script path</string> + </property> + <property name="buddy"> + <cstring>externalSignerPath</cstring> + </property> + </widget> + </item> + <item> + <widget class="QLineEdit" name="externalSignerPath"> + <property name="toolTip"> + <string>Full path to a Bitcoin Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins!</string> + </property> + </widget> + </item> + </layout> + </item> + </layout> + </widget> + </item> + <item> <spacer name="verticalSpacer_Wallet"> <property name="orientation"> <enum>Qt::Vertical</enum> diff --git a/src/qt/forms/receiverequestdialog.ui b/src/qt/forms/receiverequestdialog.ui index 7d95a8bc90..70a7cf71de 100644 --- a/src/qt/forms/receiverequestdialog.ui +++ b/src/qt/forms/receiverequestdialog.ui @@ -255,6 +255,19 @@ </widget> </item> <item> + <widget class="QPushButton" name="btnVerify"> + <property name="text"> + <string>&Verify</string> + </property> + <property name="toolTip"> + <string>Verify this address on e.g. a hardware wallet screen</string> + </property> + <property name="autoDefault"> + <bool>false</bool> + </property> + </widget> + </item> + <item> <widget class="QPushButton" name="btnSaveAs"> <property name="text"> <string>&Save Image…</string> diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp index 8a32994e3f..6ad8db4348 100644 --- a/src/qt/optionsdialog.cpp +++ b/src/qt/optionsdialog.cpp @@ -199,6 +199,7 @@ void OptionsDialog::setModel(OptionsModel *_model) connect(ui->prune, &QCheckBox::clicked, this, &OptionsDialog::togglePruneWarning); connect(ui->pruneSize, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); connect(ui->databaseCache, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); + connect(ui->externalSignerPath, &QLineEdit::textChanged, [this]{ showRestartWarning(); }); connect(ui->threadsScriptVerif, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning); /* Wallet */ connect(ui->spendZeroConfChange, &QCheckBox::clicked, this, &OptionsDialog::showRestartWarning); @@ -233,6 +234,7 @@ void OptionsDialog::setMapper() /* Wallet */ mapper->addMapping(ui->spendZeroConfChange, OptionsModel::SpendZeroConfChange); mapper->addMapping(ui->coinControlFeatures, OptionsModel::CoinControlFeatures); + mapper->addMapping(ui->externalSignerPath, OptionsModel::ExternalSignerPath); /* Network */ mapper->addMapping(ui->mapPortUpnp, OptionsModel::MapPortUPnP); diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index abdf9e9ae6..24a4e9ee96 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -117,6 +117,13 @@ void OptionsModel::Init(bool resetSettings) settings.setValue("bSpendZeroConfChange", true); if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool())) addOverriddenOption("-spendzeroconfchange"); + + if (!settings.contains("external_signer_path")) + settings.setValue("external_signer_path", ""); + + if (!gArgs.SoftSetArg("-signer", settings.value("external_signer_path").toString().toStdString())) { + addOverriddenOption("-signer"); + } #endif // Network @@ -326,6 +333,8 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const #ifdef ENABLE_WALLET case SpendZeroConfChange: return settings.value("bSpendZeroConfChange"); + case ExternalSignerPath: + return settings.value("external_signer_path"); #endif case DisplayUnit: return nDisplayUnit; @@ -445,6 +454,12 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in setRestartRequired(true); } break; + case ExternalSignerPath: + if (settings.value("external_signer_path") != value.toString()) { + settings.setValue("external_signer_path", value.toString()); + setRestartRequired(true); + } + break; #endif case DisplayUnit: setDisplayUnit(value); diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h index 4d012a9b8f..535843e8ba 100644 --- a/src/qt/optionsmodel.h +++ b/src/qt/optionsmodel.h @@ -65,6 +65,7 @@ public: Prune, // bool PruneSize, // int DatabaseCache, // int + ExternalSignerPath, // QString SpendZeroConfChange, // bool Listen, // bool OptionIDRowCount, diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp index 78ae5c07da..abe7de8f89 100644 --- a/src/qt/receiverequestdialog.cpp +++ b/src/qt/receiverequestdialog.cpp @@ -89,6 +89,12 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info) ui->wallet_tag->hide(); ui->wallet_content->hide(); } + + ui->btnVerify->setVisible(this->model->wallet().hasExternalSigner()); + + connect(ui->btnVerify, &QPushButton::clicked, [this] { + model->displayAddress(info.address.toStdString()); + }); } void ReceiveRequestDialog::updateDisplayUnit() diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 160b43324f..e87a2b97bc 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -199,7 +199,16 @@ void SendCoinsDialog::setModel(WalletModel *_model) // set default rbf checkbox state ui->optInRBF->setCheckState(Qt::Checked); - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().hasExternalSigner()) { + ui->sendButton->setText(tr("Sign on device")); + if (gArgs.GetArg("-signer", "") != "") { + ui->sendButton->setEnabled(true); + ui->sendButton->setToolTip(tr("Connect your hardware wallet first.")); + } else { + ui->sendButton->setEnabled(false); + ui->sendButton->setToolTip(tr("Set external signer script path in Options -> Wallet")); + } + } else if (model->wallet().privateKeysDisabled()) { ui->sendButton->setText(tr("Cr&eate Unsigned")); ui->sendButton->setToolTip(tr("Creates a Partially Signed Bitcoin Transaction (PSBT) for use with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); } @@ -313,14 +322,14 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa formatted.append(recipientElement); } - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { question_string.append(tr("Do you want to draft this transaction?")); } else { question_string.append(tr("Are you sure you want to send?")); } question_string.append("<br /><span style='font-size:10pt;'>"); - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can save or copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); } else { question_string.append(tr("Please, review your transaction.")); @@ -386,8 +395,8 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) if (!PrepareSendText(question_string, informative_text, detailed_text)) return; assert(m_current_transaction); - const QString confirmation = model->wallet().privateKeysDisabled() ? tr("Confirm transaction proposal") : tr("Confirm send coins"); - const QString confirmButtonText = model->wallet().privateKeysDisabled() ? tr("Create Unsigned") : tr("Send"); + const QString confirmation = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Confirm transaction proposal") : tr("Confirm send coins"); + const QString confirmButtonText = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Create Unsigned") : tr("Sign and send"); SendConfirmationDialog confirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, this); confirmationDialog.exec(); QMessageBox::StandardButton retval = static_cast<QMessageBox::StandardButton>(confirmationDialog.result()); @@ -403,9 +412,58 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())}; PartiallySignedTransaction psbtx(mtx); bool complete = false; - const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete, nullptr); + // Always fill without signing first. This prevents an external signer + // from being called prematurely and is not expensive. + TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete, nullptr); assert(!complete); assert(err == TransactionError::OK); + if (model->wallet().hasExternalSigner()) { + try { + err = model->wallet().fillPSBT(SIGHASH_ALL, true /* sign */, true /* bip32derivs */, psbtx, complete, nullptr); + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Sign failed"), e.what()); + send_failure = true; + return; + } + if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) { + QMessageBox::critical(nullptr, tr("External signer not found"), "External signer not found"); + send_failure = true; + return; + } + if (err == TransactionError::EXTERNAL_SIGNER_FAILED) { + QMessageBox::critical(nullptr, tr("External signer failure"), "External signer failure"); + send_failure = true; + return; + } + if (err != TransactionError::OK) { + tfm::format(std::cerr, "Failed to sign PSBT"); + processSendCoinsReturn(WalletModel::TransactionCreationFailed); + send_failure = true; + return; + } + // fillPSBT does not always properly finalize + complete = FinalizeAndExtractPSBT(psbtx, mtx); + } + + // Broadcast transaction if complete (even with an external signer this + // is not always the case, e.g. in a multisig wallet). + if (complete) { + const CTransactionRef tx = MakeTransactionRef(mtx); + m_current_transaction->setWtx(tx); + WalletModel::SendCoinsReturn sendStatus = model->sendCoins(*m_current_transaction); + // process sendStatus and on error generate message shown to user + processSendCoinsReturn(sendStatus); + + if (sendStatus.status == WalletModel::OK) { + Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash()); + } else { + send_failure = true; + } + return; + } + + // Copy PSBT to clipboard and offer to save + assert(!complete); // Serialize the PSBT CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << psbtx; @@ -447,7 +505,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) break; default: assert(false); - } + } // msgBox.exec() } else { // now send the prepared transaction WalletModel::SendCoinsReturn sendStatus = model->sendCoins(*m_current_transaction); @@ -614,7 +672,9 @@ void SendCoinsDialog::setBalance(const interfaces::WalletBalances& balances) if(model && model->getOptionsModel()) { CAmount balance = balances.balance; - if (model->wallet().privateKeysDisabled()) { + if (model->wallet().hasExternalSigner()) { + ui->labelBalanceName->setText(tr("External balance:")); + } else if (model->wallet().privateKeysDisabled()) { balance = balances.watch_only_balance; ui->labelBalanceName->setText(tr("Watch-only balance:")); } @@ -698,7 +758,7 @@ void SendCoinsDialog::on_buttonMinimizeFee_clicked() void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry) { // Include watch-only for wallets without private key - m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); // Calculate available amount to send. CAmount amount = model->wallet().getAvailableBalance(*m_coin_control); @@ -753,7 +813,7 @@ void SendCoinsDialog::updateCoinControlState() m_coin_control->m_confirm_target = getConfTargetForIndex(ui->confTargetSelector->currentIndex()); m_coin_control->m_signal_bip125_rbf = ui->optInRBF->isChecked(); // Include watch-only for wallets without private key - m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner(); } void SendCoinsDialog::updateNumberOfBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, bool headers, SynchronizationState sync_state) { diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index aa26a01541..7e5790fd87 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -263,6 +263,9 @@ void CreateWalletActivity::createWallet() if (m_create_wallet_dialog->isDescriptorWalletChecked()) { flags |= WALLET_FLAG_DESCRIPTORS; } + if (m_create_wallet_dialog->isExternalSignerChecked()) { + flags |= WALLET_FLAG_EXTERNAL_SIGNER; + } QTimer::singleShot(500, worker(), [this, name, flags] { std::unique_ptr<interfaces::Wallet> wallet = node().walletClient().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message); @@ -291,6 +294,17 @@ void CreateWalletActivity::finish() void CreateWalletActivity::create() { m_create_wallet_dialog = new CreateWalletDialog(m_parent_widget); + +#ifdef ENABLE_EXTERNAL_SIGNER + std::vector<ExternalSigner> signers; + try { + signers = node().externalSigners(); + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Can't list signers"), e.what()); + } + m_create_wallet_dialog->setSigners(signers); +#endif + m_create_wallet_dialog->setWindowModality(Qt::ApplicationModal); m_create_wallet_dialog->show(); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 7c58b8afd2..e32b7c2807 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -552,6 +552,18 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash) return true; } +bool WalletModel::displayAddress(std::string sAddress) +{ + CTxDestination dest = DecodeDestination(sAddress); + bool res = false; + try { + res = m_wallet->displayAddress(dest); + } catch (const std::runtime_error& e) { + QMessageBox::critical(nullptr, tr("Can't display address"), e.what()); + } + return res; +} + bool WalletModel::isWalletEnabled() { return !gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET); diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index b2ce5d69fb..47a21bcfcf 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -136,6 +136,7 @@ public: UnlockContext requestUnlock(); bool bumpFee(uint256 hash, uint256& new_hash); + bool displayAddress(std::string sAddress); static bool isWalletEnabled(); diff --git a/src/qt/walletmodeltransaction.cpp b/src/qt/walletmodeltransaction.cpp index 25172e774c..d185ddb7e8 100644 --- a/src/qt/walletmodeltransaction.cpp +++ b/src/qt/walletmodeltransaction.cpp @@ -26,6 +26,11 @@ CTransactionRef& WalletModelTransaction::getWtx() return wtx; } +void WalletModelTransaction::setWtx(const CTransactionRef& newTx) +{ + wtx = newTx; +} + unsigned int WalletModelTransaction::getTransactionSize() { return wtx ? GetVirtualTransactionSize(*wtx) : 0; diff --git a/src/qt/walletmodeltransaction.h b/src/qt/walletmodeltransaction.h index f9a95362c8..120d240d91 100644 --- a/src/qt/walletmodeltransaction.h +++ b/src/qt/walletmodeltransaction.h @@ -27,6 +27,8 @@ public: QList<SendCoinsRecipient> getRecipients() const; CTransactionRef& getWtx(); + void setWtx(const CTransactionRef&); + unsigned int getTransactionSize(); void setTransactionFee(const CAmount& newFee); diff --git a/src/test/fuzz/float.cpp b/src/test/fuzz/float.cpp index adef66a3ee..2f77c8949e 100644 --- a/src/test/fuzz/float.cpp +++ b/src/test/fuzz/float.cpp @@ -5,6 +5,7 @@ #include <memusage.h> #include <test/fuzz/FuzzedDataProvider.h> #include <test/fuzz/fuzz.h> +#include <test/fuzz/util.h> #include <util/serfloat.h> #include <version.h> @@ -17,7 +18,33 @@ FUZZ_TARGET(float) FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); { - const double d = fuzzed_data_provider.ConsumeFloatingPoint<double>(); + const double d{[&] { + double tmp; + CallOneOf( + fuzzed_data_provider, + // an actual number + [&] { tmp = fuzzed_data_provider.ConsumeFloatingPoint<double>(); }, + // special numbers and NANs + [&] { tmp = fuzzed_data_provider.PickValueInArray({ + std::numeric_limits<double>::infinity(), + -std::numeric_limits<double>::infinity(), + std::numeric_limits<double>::min(), + -std::numeric_limits<double>::min(), + std::numeric_limits<double>::max(), + -std::numeric_limits<double>::max(), + std::numeric_limits<double>::lowest(), + -std::numeric_limits<double>::lowest(), + std::numeric_limits<double>::quiet_NaN(), + -std::numeric_limits<double>::quiet_NaN(), + std::numeric_limits<double>::signaling_NaN(), + -std::numeric_limits<double>::signaling_NaN(), + std::numeric_limits<double>::denorm_min(), + -std::numeric_limits<double>::denorm_min(), + }); }, + // Anything from raw memory (also checks that DecodeDouble doesn't crash on any input) + [&] { tmp = DecodeDouble(fuzzed_data_provider.ConsumeIntegral<uint64_t>()); }); + return tmp; + }()}; (void)memusage::DynamicUsage(d); uint64_t encoded = EncodeDouble(d); diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 36b1d5035c..023dcdb3e5 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -44,7 +44,7 @@ void CallOneOf(FuzzedDataProvider& fuzzed_data_provider, Callables... callables) const size_t call_index{fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, call_size - 1)}; size_t i{0}; - return ((i++ == call_index ? callables() : void()), ...); + ((i++ == call_index ? callables() : void()), ...); } template <typename Collection> diff --git a/src/validation.cpp b/src/validation.cpp index e6e6aadb17..5e3d429c2e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -629,10 +629,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // is for the sake of multi-party protocols, where we don't // want a single party to be able to disable replacement. // - // The opt-out ignores descendants as anyone relying on - // first-seen mempool behavior should be checking all - // unconfirmed ancestors anyway; doing otherwise is hopelessly - // insecure. + // Transactions that don't explicitly signal replaceability are + // *not* replaceable with the current logic, even if one of their + // unconfirmed ancestors signals replaceability. This diverges + // from BIP125's inherited signaling description (see CVE-2021-31876). + // Applications relying on first-seen mempool behavior should + // check all unconfirmed ancestors; otherwise an opt-in ancestor + // might be replaced, causing removal of this descendant. bool fReplacementOptOut = true; for (const CTxIn &_txin : ptxConflicting->vin) { diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 5bf037b222..8d5316e0af 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -12,7 +12,7 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir) { - const size_t offset = wallet_dir.string().size() + 1; + const size_t offset = wallet_dir.string().size() + (wallet_dir == wallet_dir.root_name() ? 0 : 1); std::vector<fs::path> paths; boost::system::error_code ec; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index aca52964ee..ee92316b89 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -206,6 +206,11 @@ public: WalletBatch batch{m_wallet->GetDatabase()}; return m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } + bool displayAddress(const CTxDestination& dest) override + { + LOCK(m_wallet->cs_wallet); + return m_wallet->DisplayAddress(dest); + } void lockCoin(const COutPoint& output) override { LOCK(m_wallet->cs_wallet); @@ -446,6 +451,7 @@ public: unsigned int getConfirmTarget() override { return m_wallet->m_confirm_target; } bool hdEnabled() override { return m_wallet->IsHDEnabled(); } bool canGetAddresses() override { return m_wallet->CanGetAddresses(); } + bool hasExternalSigner() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); } bool privateKeysDisabled() override { return m_wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); } OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; } CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 97fc7acca5..c8ded4c51e 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -352,7 +352,7 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu return groups_out; } -bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, +bool CWallet::AttemptSelection(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params) const { setCoinsRet.clear(); @@ -456,32 +456,32 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six // confirmations on outputs received from other wallets and only spend confirmed change. - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; + if (AttemptSelection(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; + if (AttemptSelection(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; // Fall back to using zero confirmation change (but with as few ancestors in the mempool as // possible) if we cannot fund the transaction otherwise. if (m_spend_zero_conf_change) { - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params)) return true; + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; } - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; } // If partial groups are allowed, relax the requirement of spending OutputGroups (groups // of UTXOs sent to the same address, which are obviously controlled by a single wallet) // in their entirety. - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), + if (AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; } // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs // received from other wallets. if (coin_control.m_include_unsafe_inputs - && SelectCoinsMinConf(value_to_select, + && AttemptSelection(value_to_select, CoinEligibilityFilter(0 /* conf_mine */, 0 /* conf_theirs */, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; @@ -489,7 +489,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm // Try with unlimited ancestors/descendants. The transaction will still need to meet // mempool ancestor/descendant policy to be accepted to mempool and broadcasted, but // OutputGroups use heuristics that may overestimate ancestor/descendant counts. - if (!fRejectLongChains && SelectCoinsMinConf(value_to_select, + if (!fRejectLongChains && AttemptSelection(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max(), std::numeric_limits<uint64_t>::max(), true /* include_partial_groups */), vCoins, setCoinsRet, nValueRet, coin_selection_params)) { return true; @@ -499,7 +499,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm return false; }(); - // SelectCoinsMinConf clears setCoinsRet, so add the preset inputs from coin_control to the coinset + // AttemptSelection clears setCoinsRet, so add the preset inputs from coin_control to the coinset util::insert(setCoinsRet, setPresetCoins); // add preset inputs to the total value selected @@ -578,287 +578,266 @@ bool CWallet::CreateTransactionInternal( FeeCalculation& fee_calc_out, bool sign) { - CAmount nValue = 0; + AssertLockHeld(cs_wallet); + + CMutableTransaction txNew; // The resulting transaction that we make + txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); + + CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy + coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; + + CAmount recipients_sum = 0; const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend); ReserveDestination reservedest(this, change_type); - unsigned int nSubtractFeeFromAmount = 0; - for (const auto& recipient : vecSend) - { - if (nValue < 0 || recipient.nAmount < 0) - { - error = _("Transaction amounts must not be negative"); - return false; + unsigned int outputs_to_subtract_fee_from = 0; // The number of outputs which we are subtracting the fee from + for (const auto& recipient : vecSend) { + recipients_sum += recipient.nAmount; + + if (recipient.fSubtractFeeFromAmount) { + outputs_to_subtract_fee_from++; + coin_selection_params.m_subtract_fee_outputs = true; } - nValue += recipient.nAmount; + } - if (recipient.fSubtractFeeFromAmount) - nSubtractFeeFromAmount++; + // Create change script that will be used if we need change + // TODO: pass in scriptChange instead of reservedest so + // change transaction isn't always pay-to-bitcoin-address + CScript scriptChange; + + // coin control: send change to custom address + if (!std::get_if<CNoDestination>(&coin_control.destChange)) { + scriptChange = GetScriptForDestination(coin_control.destChange); + } else { // no coin control: send change to newly generated address + // Note: We use a new key here to keep it from being obvious which side is the change. + // The drawback is that by not reusing a previous key, the change may be lost if a + // backup is restored, if the backup doesn't have the new private key for the change. + // If we reused the old key, it would be possible to add code to look for and + // rediscover unknown transactions that were written with keys of ours to recover + // post-backup change. + + // Reserve a new key pair from key pool. If it fails, provide a dummy + // destination in case we don't need change. + CTxDestination dest; + if (!reservedest.GetReservedDestination(dest, true)) { + error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); + } + scriptChange = GetScriptForDestination(dest); + // A valid destination implies a change script (and + // vice-versa). An empty change script will abort later, if the + // change keypool ran out, but change is required. + CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); } - if (vecSend.empty()) - { - error = _("Transaction must have at least one recipient"); - return false; + CTxOut change_prototype_txout(0, scriptChange); + coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); + + // Get size of spending the change output + int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this); + // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh + // as lower-bound to allow BnB to do it's thing + if (change_spend_size == -1) { + coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE; + } else { + coin_selection_params.change_spend_size = (size_t)change_spend_size; } - CMutableTransaction txNew; + // Set discard feerate + coin_selection_params.m_discard_feerate = GetDiscardRate(*this); + + // Get the fee rate to use effective values in coin selection FeeCalculation feeCalc; - TxSize tx_sizes; - int nBytes; + coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); + // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly + // provided one + if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { + error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB)); + return false; + } + if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { + // eventually allow a fallback fee + error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); + return false; + } + + // Get long term estimate + CCoinControl cc_temp; + cc_temp.m_confirm_target = chain().estimateMaxBlocks(); + coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr); + + // Calculate the cost of change + // Cost of change is the cost of creating the change output + cost of spending the change output in the future. + // For creating the change output now, we use the effective feerate. + // For spending the change output in the future, we use the discard feerate for now. + // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate) + coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); + coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee; + + // vouts to the payees + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size) + } + for (const auto& recipient : vecSend) { - std::set<CInputCoin> setCoins; - LOCK(cs_wallet); - txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); - { - std::vector<COutput> vAvailableCoins; - AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); - CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy - coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; - - // Create change script that will be used if we need change - // TODO: pass in scriptChange instead of reservedest so - // change transaction isn't always pay-to-bitcoin-address - CScript scriptChange; - - // coin control: send change to custom address - if (!std::get_if<CNoDestination>(&coin_control.destChange)) { - scriptChange = GetScriptForDestination(coin_control.destChange); - } else { // no coin control: send change to newly generated address - // Note: We use a new key here to keep it from being obvious which side is the change. - // The drawback is that by not reusing a previous key, the change may be lost if a - // backup is restored, if the backup doesn't have the new private key for the change. - // If we reused the old key, it would be possible to add code to look for and - // rediscover unknown transactions that were written with keys of ours to recover - // post-backup change. - - // Reserve a new key pair from key pool. If it fails, provide a dummy - // destination in case we don't need change. - CTxDestination dest; - if (!reservedest.GetReservedDestination(dest, true)) { - error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); - } - scriptChange = GetScriptForDestination(dest); - // A valid destination implies a change script (and - // vice-versa). An empty change script will abort later, if the - // change keypool ran out, but change is required. - CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); - } - CTxOut change_prototype_txout(0, scriptChange); - coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); - - // Get size of spending the change output - int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this); - // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh - // as lower-bound to allow BnB to do it's thing - if (change_spend_size == -1) { - coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE; - } else { - coin_selection_params.change_spend_size = (size_t)change_spend_size; - } + CTxOut txout(recipient.nAmount, recipient.scriptPubKey); - // Set discard feerate - coin_selection_params.m_discard_feerate = GetDiscardRate(*this); + // Include the fee cost for outputs. + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); + } - // Get the fee rate to use effective values in coin selection - coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); - // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly - // provided one - if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { - error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB)); - return false; - } - if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { - // eventually allow a fallback fee - error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); - return false; - } + if (IsDust(txout, chain().relayDustFee())) + { + error = _("Transaction amount too small"); + return false; + } + txNew.vout.push_back(txout); + } - // Get long term estimate - CCoinControl cc_temp; - cc_temp.m_confirm_target = chain().estimateMaxBlocks(); - coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr); + // Include the fees for things that aren't inputs, excluding the change output + const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); + CAmount selection_target = recipients_sum + not_input_fees; - // Calculate the cost of change - // Cost of change is the cost of creating the change output + cost of spending the change output in the future. - // For creating the change output now, we use the effective feerate. - // For spending the change output in the future, we use the discard feerate for now. - // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate) - coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); - coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee; + // Get available coins + std::vector<COutput> vAvailableCoins; + AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); - coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values + // Choose coins to use + CAmount inputs_sum = 0; + std::set<CInputCoin> setCoins; + if (!SelectCoins(vAvailableCoins, /* nTargetValue */ selection_target, setCoins, inputs_sum, coin_control, coin_selection_params)) + { + error = _("Insufficient funds"); + return false; + } - // vouts to the payees - if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size) - } - for (const auto& recipient : vecSend) - { - CTxOut txout(recipient.nAmount, recipient.scriptPubKey); + // Always make a change output + // We will reduce the fee from this change output later, and remove the output if it is too small. + const CAmount change_and_fee = inputs_sum - recipients_sum; + assert(change_and_fee >= 0); + CTxOut newTxOut(change_and_fee, scriptChange); - // Include the fee cost for outputs. - if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); - } + if (nChangePosInOut == -1) + { + // Insert change txn at random position: + nChangePosInOut = GetRandInt(txNew.vout.size()+1); + } + else if ((unsigned int)nChangePosInOut > txNew.vout.size()) + { + error = _("Change index out of range"); + return false; + } - if (IsDust(txout, chain().relayDustFee())) - { - error = _("Transaction amount too small"); - return false; - } - txNew.vout.push_back(txout); - } + assert(nChangePosInOut != -1); + auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); - // Include the fees for things that aren't inputs, excluding the change output - const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); - CAmount nValueToSelect = nValue + not_input_fees; + // Shuffle selected coins and fill in final vin + std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end()); + Shuffle(selected_coins.begin(), selected_coins.end(), FastRandomContext()); - // Choose coins to use - CAmount inputs_sum = 0; - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, coin_control, coin_selection_params)) - { - error = _("Insufficient funds"); - return false; - } - - // Always make a change output - // We will reduce the fee from this change output later, and remove the output if it is too small. - const CAmount change_and_fee = inputs_sum - nValue; - assert(change_and_fee >= 0); - CTxOut newTxOut(change_and_fee, scriptChange); + // Note how the sequence number is set to non-maxint so that + // the nLockTime set above actually works. + // + // BIP125 defines opt-in RBF as any nSequence < maxint-1, so + // we use the highest possible value in that range (maxint-2) + // to avoid conflicting with other possible uses of nSequence, + // and in the spirit of "smallest possible change from prior + // behavior." + const uint32_t nSequence = coin_control.m_signal_bip125_rbf.value_or(m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : (CTxIn::SEQUENCE_FINAL - 1); + for (const auto& coin : selected_coins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); + } - if (nChangePosInOut == -1) - { - // Insert change txn at random position: - nChangePosInOut = GetRandInt(txNew.vout.size()+1); - } - else if ((unsigned int)nChangePosInOut > txNew.vout.size()) - { - error = _("Change index out of range"); - return false; - } + // Calculate the transaction fee + TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + int nBytes = tx_sizes.vsize; + if (nBytes < 0) { + error = _("Signing transaction failed"); + return false; + } + nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); - assert(nChangePosInOut != -1); - auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + // Subtract fee from the change output if not subtracting it from recipient outputs + CAmount fee_needed = nFeeRet; + if (!coin_selection_params.m_subtract_fee_outputs) { + change_position->nValue -= fee_needed; + } - // Dummy fill vin for maximum size estimation - // - for (const auto& coin : setCoins) { - txNew.vin.push_back(CTxIn(coin.outpoint,CScript())); - } + // We want to drop the change to fees if: + // 1. The change output would be dust + // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) + CAmount change_amount = change_position->nValue; + if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change) + { + nChangePosInOut = -1; + change_amount = 0; + txNew.vout.erase(change_position); + + // Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those + tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + nBytes = tx_sizes.vsize; + fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); + } - // Calculate the transaction fee - tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - nBytes = tx_sizes.vsize; - if (nBytes < 0) { - error = _("Signing transaction failed"); - return false; - } - nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); + // Update nFeeRet in case fee_needed changed due to dropping the change output + if (fee_needed <= change_and_fee - change_amount) { + nFeeRet = change_and_fee - change_amount; + } - // Subtract fee from the change output if not subtrating it from recipient outputs - CAmount fee_needed = nFeeRet; - if (nSubtractFeeFromAmount == 0) { - change_position->nValue -= fee_needed; + // Reduce output values for subtractFeeFromAmount + if (coin_selection_params.m_subtract_fee_outputs) { + CAmount to_reduce = fee_needed + change_amount - change_and_fee; + int i = 0; + bool fFirst = true; + for (const auto& recipient : vecSend) + { + if (i == nChangePosInOut) { + ++i; } + CTxOut& txout = txNew.vout[i]; - // We want to drop the change to fees if: - // 1. The change output would be dust - // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) - CAmount change_amount = change_position->nValue; - if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change) + if (recipient.fSubtractFeeFromAmount) { - nChangePosInOut = -1; - change_amount = 0; - txNew.vout.erase(change_position); - - // Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those - tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - nBytes = tx_sizes.vsize; - fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - } + txout.nValue -= to_reduce / outputs_to_subtract_fee_from; // Subtract fee equally from each selected recipient - // Update nFeeRet in case fee_needed changed due to dropping the change output - if (fee_needed <= change_and_fee - change_amount) { - nFeeRet = change_and_fee - change_amount; - } - - // Reduce output values for subtractFeeFromAmount - if (nSubtractFeeFromAmount != 0) { - CAmount to_reduce = fee_needed + change_amount - change_and_fee; - int i = 0; - bool fFirst = true; - for (const auto& recipient : vecSend) + if (fFirst) // first receiver pays the remainder not divisible by output count { - if (i == nChangePosInOut) { - ++i; - } - CTxOut& txout = txNew.vout[i]; - - if (recipient.fSubtractFeeFromAmount) - { - txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient - - if (fFirst) // first receiver pays the remainder not divisible by output count - { - fFirst = false; - txout.nValue -= to_reduce % nSubtractFeeFromAmount; - } - - // Error if this output is reduced to be below dust - if (IsDust(txout, chain().relayDustFee())) { - if (txout.nValue < 0) { - error = _("The transaction amount is too small to pay the fee"); - } else { - error = _("The transaction amount is too small to send after the fee has been deducted"); - } - return false; - } - } - ++i; + fFirst = false; + txout.nValue -= to_reduce % outputs_to_subtract_fee_from; } - nFeeRet = fee_needed; - } - // Give up if change keypool ran out and change is required - if (scriptChange.empty() && nChangePosInOut != -1) { - return false; + // Error if this output is reduced to be below dust + if (IsDust(txout, chain().relayDustFee())) { + if (txout.nValue < 0) { + error = _("The transaction amount is too small to pay the fee"); + } else { + error = _("The transaction amount is too small to send after the fee has been deducted"); + } + return false; + } } + ++i; } + nFeeRet = fee_needed; + } - // Shuffle selected coins and fill in final vin - txNew.vin.clear(); - std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end()); - Shuffle(selected_coins.begin(), selected_coins.end(), FastRandomContext()); - - // Note how the sequence number is set to non-maxint so that - // the nLockTime set above actually works. - // - // BIP125 defines opt-in RBF as any nSequence < maxint-1, so - // we use the highest possible value in that range (maxint-2) - // to avoid conflicting with other possible uses of nSequence, - // and in the spirit of "smallest possible change from prior - // behavior." - const uint32_t nSequence = coin_control.m_signal_bip125_rbf.value_or(m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : (CTxIn::SEQUENCE_FINAL - 1); - for (const auto& coin : selected_coins) { - txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); - } + // Give up if change keypool ran out and change is required + if (scriptChange.empty() && nChangePosInOut != -1) { + return false; + } - if (sign && !SignTransaction(txNew)) { - error = _("Signing transaction failed"); - return false; - } + if (sign && !SignTransaction(txNew)) { + error = _("Signing transaction failed"); + return false; + } - // Return the constructed transaction data. - tx = MakeTransactionRef(std::move(txNew)); + // Return the constructed transaction data. + tx = MakeTransactionRef(std::move(txNew)); - // Limit size - if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || - (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT)) - { - error = _("Transaction too large"); - return false; - } + // Limit size + if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || + (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT)) + { + error = _("Transaction too large"); + return false; } if (nFeeRet > m_default_max_tx_fee) { @@ -900,6 +879,18 @@ bool CWallet::CreateTransaction( FeeCalculation& fee_calc_out, bool sign) { + if (vecSend.empty()) { + error = _("Transaction must have at least one recipient"); + return false; + } + + if (std::any_of(vecSend.cbegin(), vecSend.cend(), [](const auto& recipient){ return recipient.nAmount < 0; })) { + error = _("Transaction amounts must not be negative"); + return false; + } + + LOCK(cs_wallet); + int nChangePosIn = nChangePosInOut; Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr) bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 14c3578473..c65ebad52f 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -270,7 +270,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret)); } - // Make sure that effective value is working in SelectCoinsMinConf when BnB is used + // Make sure that effective value is working in AttemptSelection when BnB is used CoinSelectionParams coin_selection_params_bnb(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000), /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), @@ -280,14 +280,14 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) empty_wallet(); add_coin(1); vCoins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); + BOOST_CHECK(!testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); // Test fees subtracted from output: empty_wallet(); add_coin(1 * CENT); vCoins.at(0).nInputBytes = 40; coin_selection_params_bnb.m_subtract_fee_outputs = true; - BOOST_CHECK(testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); + BOOST_CHECK(testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params_bnb)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); // Make sure that can use BnB when there are preset inputs @@ -322,24 +322,24 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) empty_wallet(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); add_coin(1*CENT, 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection( 1 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // but we can find a new 1 cent - BOOST_CHECK( testWallet.SelectCoinsMinConf( 1 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 1 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); add_coin(2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!testWallet.SelectCoinsMinConf( 3 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection( 3 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // we can make 3 cents of new coins - BOOST_CHECK( testWallet.SelectCoinsMinConf( 3 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 3 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 3 * CENT); add_coin(5*CENT); // add a mature 5 cent coin, @@ -349,33 +349,33 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection(38 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!testWallet.SelectCoinsMinConf(38 * CENT, filter_standard_extra, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection(38 * CENT, filter_standard_extra, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // but we can make 37 cents if we accept new coins from ourself - BOOST_CHECK( testWallet.SelectCoinsMinConf(37 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(37 * CENT, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 37 * CENT); // and we can make 38 cents if we accept all new coins - BOOST_CHECK( testWallet.SelectCoinsMinConf(38 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(38 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - BOOST_CHECK( testWallet.SelectCoinsMinConf(34 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(34 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - BOOST_CHECK( testWallet.SelectCoinsMinConf( 7 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 7 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 7 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - BOOST_CHECK( testWallet.SelectCoinsMinConf( 8 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 8 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK(nValueRet == 8 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - BOOST_CHECK( testWallet.SelectCoinsMinConf( 9 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection( 9 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 10 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -389,30 +389,30 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - BOOST_CHECK( testWallet.SelectCoinsMinConf(71 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); - BOOST_CHECK(!testWallet.SelectCoinsMinConf(72 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(71 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!testWallet.AttemptSelection(72 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); add_coin( 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); add_coin( 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - BOOST_CHECK( testWallet.SelectCoinsMinConf(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(16 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - BOOST_CHECK( testWallet.SelectCoinsMinConf(11 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(11 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 11 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); @@ -421,11 +421,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin( 2*COIN); add_coin( 3*COIN); add_coin( 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - BOOST_CHECK( testWallet.SelectCoinsMinConf(95 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(95 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); - BOOST_CHECK( testWallet.SelectCoinsMinConf(195 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(195 * CENT, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -440,14 +440,14 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * MIN_CHANGE from the 1.5 * MIN_CHANGE // we'll get change smaller than MIN_CHANGE whatever happens, so can expect MIN_CHANGE exactly - BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // but if we add a bigger coin, small change is avoided add_coin(1111*MIN_CHANGE); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // if we add more small coins: @@ -455,7 +455,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 7 / 10); // and try again to make 1.0 * MIN_CHANGE - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) @@ -464,7 +464,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(50000 * COIN); - BOOST_CHECK( testWallet.SelectCoinsMinConf(500000 * COIN, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(500000 * COIN, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 10U); // in ten coins @@ -477,7 +477,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 6 / 10); add_coin(MIN_CHANGE * 7 / 10); add_coin(1111 * MIN_CHANGE); - BOOST_CHECK( testWallet.SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(1 * MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1111 * MIN_CHANGE); // we get the bigger coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -487,7 +487,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 6 / 10); add_coin(MIN_CHANGE * 8 / 10); add_coin(1111 * MIN_CHANGE); - BOOST_CHECK( testWallet.SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK( testWallet.AttemptSelection(MIN_CHANGE, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // in two coins 0.4+0.6 @@ -498,12 +498,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(MIN_CHANGE * 100); // trying to make 100.01 from these three coins - BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(MIN_CHANGE * 10001 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - BOOST_CHECK(testWallet.SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(MIN_CHANGE * 9990 / 100, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 101 * MIN_CHANGE); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); } @@ -517,7 +517,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - BOOST_CHECK(testWallet.SelectCoinsMinConf(2000, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(2000, filter_confirmed, vCoins, setCoinsRet, nValueRet, coin_selection_params)); if (amt - 2000 < MIN_CHANGE) { // needs more than one input: @@ -602,7 +602,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(1000 * COIN); add_coin(3 * COIN); - BOOST_CHECK(testWallet.SelectCoinsMinConf(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(testWallet.AttemptSelection(1003 * COIN, filter_standard, vCoins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1003 * COIN); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 788a901f95..d0e26c416c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -326,7 +326,7 @@ private: // ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers; - bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign); + bool CreateTransactionInternal(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Catch wallet up to current chain, scanning new blocks, updating the best @@ -445,7 +445,7 @@ public: * param@[out] setCoinsRet Populated with the coins selected if successful. * param@[out] nValueRet Used to return the total value of selected coins. */ - bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, + bool AttemptSelection(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params) const; bool IsSpent(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c06b319b0b..24d5351945 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -712,6 +712,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) } } +#ifndef ENABLE_EXTERNAL_SIGNER + if (pwallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { + pwallet->WalletLogPrintf("Error: External signer wallet being loaded without external signer support compiled\n"); + return DBErrors::TOO_NEW; + } +#endif + // Get cursor if (!m_batch->StartCursor()) { diff --git a/src/zmq/zmqutil.cpp b/src/zmq/zmqutil.cpp index f07a4ae9fd..b0f12388e5 100644 --- a/src/zmq/zmqutil.cpp +++ b/src/zmq/zmqutil.cpp @@ -5,10 +5,12 @@ #include <zmq/zmqutil.h> #include <logging.h> - #include <zmq.h> -void zmqError(const char* str) +#include <cerrno> +#include <string> + +void zmqError(const std::string& str) { - LogPrint(BCLog::ZMQ, "zmq: Error: %s, errno=%s\n", str, zmq_strerror(errno)); + LogPrint(BCLog::ZMQ, "zmq: Error: %s, msg: %s\n", str, zmq_strerror(errno)); } diff --git a/src/zmq/zmqutil.h b/src/zmq/zmqutil.h index 4c1df5d6db..90c0b00edb 100644 --- a/src/zmq/zmqutil.h +++ b/src/zmq/zmqutil.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_ZMQ_ZMQUTIL_H #define BITCOIN_ZMQ_ZMQUTIL_H -void zmqError(const char* str); +#include <string> + +void zmqError(const std::string& str); #endif // BITCOIN_ZMQ_ZMQUTIL_H diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 344db5f652..0bb04ae267 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -116,6 +116,9 @@ class ReplaceByFeeTest(BitcoinTestFramework): self.log.info("Running test prioritised transactions...") self.test_prioritised_transactions() + self.log.info("Running test no inherited signaling...") + self.test_no_inherited_signaling() + self.log.info("Passed") def test_simple_doublespend(self): @@ -564,5 +567,69 @@ class ReplaceByFeeTest(BitcoinTestFramework): assert_equal(json0["vin"][0]["sequence"], 4294967293) assert_equal(json1["vin"][0]["sequence"], 4294967294) + def test_no_inherited_signaling(self): + # Send tx from which to conflict outputs later + base_txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) + self.nodes[0].generate(1) + self.sync_blocks() + + # Create an explicitly opt-in parent transaction + optin_parent_tx = self.nodes[0].createrawtransaction([{ + 'txid': base_txid, + 'vout': 0, + "sequence": 0xfffffffd, + }], {self.nodes[0].getnewaddress(): Decimal("9.99998")}) + + optin_parent_tx = self.nodes[0].signrawtransactionwithwallet(optin_parent_tx) + + # Broadcast parent tx + optin_parent_txid = self.nodes[0].sendrawtransaction(hexstring=optin_parent_tx["hex"], maxfeerate=0) + assert optin_parent_txid in self.nodes[0].getrawmempool() + + replacement_parent_tx = self.nodes[0].createrawtransaction([{ + 'txid': base_txid, + 'vout': 0, + "sequence": 0xfffffffd, + }], {self.nodes[0].getnewaddress(): Decimal("9.90000")}) + + replacement_parent_tx = self.nodes[0].signrawtransactionwithwallet(replacement_parent_tx) + + # Test if parent tx can be replaced. + res = self.nodes[0].testmempoolaccept(rawtxs=[replacement_parent_tx['hex']], maxfeerate=0)[0] + + # Parent can be replaced. + assert_equal(res['allowed'], True) + + # Create an opt-out child tx spending the opt-in parent + optout_child_tx = self.nodes[0].createrawtransaction([{ + 'txid': optin_parent_txid, + 'vout': 0, + "sequence": 0xffffffff, + }], {self.nodes[0].getnewaddress(): Decimal("9.99990")}) + + optout_child_tx = self.nodes[0].signrawtransactionwithwallet(optout_child_tx) + + # Broadcast child tx + optout_child_txid = self.nodes[0].sendrawtransaction(hexstring=optout_child_tx["hex"], maxfeerate=0) + assert optout_child_txid in self.nodes[0].getrawmempool() + + replacement_child_tx = self.nodes[0].createrawtransaction([{ + 'txid': optin_parent_txid, + 'vout': 0, + "sequence": 0xffffffff, + }], {self.nodes[0].getnewaddress(): Decimal("9.00000")}) + + replacement_child_tx = self.nodes[0].signrawtransactionwithwallet(replacement_child_tx) + + # Broadcast replacement child tx + # BIP 125 : + # 1. The original transactions signal replaceability explicitly or through inheritance as described in the above + # Summary section. + # The original transaction (`optout_child_tx`) doesn't signal RBF but its parent (`optin_parent_txid`) does. + # The replacement transaction (`replacement_child_tx`) should be able to replace the original transaction. + # See CVE-2021-31876 for further explanations. + assert optin_parent_txid in self.nodes[0].getrawmempool() + assert_raises_rpc_error(-26, 'txn-mempool-conflict', self.nodes[0].sendrawtransaction, replacement_child_tx["hex"], 0) + if __name__ == '__main__': ReplaceByFeeTest().main() diff --git a/test/functional/wallet_orphanedreward.py b/test/functional/wallet_orphanedreward.py index e1544cbb48..097df2cf41 100755 --- a/test/functional/wallet_orphanedreward.py +++ b/test/functional/wallet_orphanedreward.py @@ -31,6 +31,7 @@ class OrphanedBlockRewardTest(BitcoinTestFramework): # Let the block reward mature and send coins including both # the existing balance and the block reward. self.nodes[0].generate(150) + self.sync_blocks() assert_equal(self.nodes[1].getbalance(), 10 + 25) txid = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 30) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 2850cfcea5..877adaccec 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -34,6 +34,9 @@ unsigned-integer-overflow:crypto/ unsigned-integer-overflow:FuzzedDataProvider.h unsigned-integer-overflow:hash.cpp unsigned-integer-overflow:leveldb/ +# temporary coinstats suppressions (will be removed and fixed in https://github.com/bitcoin/bitcoin/pull/22146) +unsigned-integer-overflow:node/coinstats.cpp +signed-integer-overflow:node/coinstats.cpp unsigned-integer-overflow:policy/fees.cpp unsigned-integer-overflow:prevector.h unsigned-integer-overflow:pubkey.h |