diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index fe711c74d265..bf23fb88db6f 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -7,6 +7,7 @@ #include #include // For CTransactionRef +#include // For TransactionError #include // For util::SettingsValue #include @@ -238,8 +239,9 @@ class Chain //! Transaction is added to memory pool, if the transaction fee is below the //! amount specified by max_tx_fee, and broadcast to all peers if relay is set to true. - //! Return false if the transaction could not be added due to the fee or for another reason. - virtual bool broadcastTransaction(const CTransactionRef& tx, + //! Return the node's result: OK when it is in the mempool, ALREADY_IN_CHAIN when + //! its outputs are already in the chain, or why it could not be added. + virtual TransactionError broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, bilingual_str& err_string) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 15787b5bf021..7f14eaf0a3a0 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1567,13 +1567,9 @@ class ChainImpl : public Chain auto it = m_node.mempool->GetIter(txid); return it && (*it)->GetCountWithDescendants() > 1; } - bool broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, bilingual_str& err_string) override + TransactionError broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, bilingual_str& err_string) override { - const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback=*/false); - // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures. - // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures - // that Chain clients do not need to know about. - return TransactionError::OK == err; + return BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback=*/false); } void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize, CAmount* ancestorfees) override { diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 9be0ba7b22f7..b4a16cdd8145 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -646,8 +646,13 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked) // failed, or more signatures are needed. if (broadcast) { // now send the prepared transaction - model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin()); - Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash()); + const auto send_status{model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin())}; + if (send_status.status == WalletModel::OK) { + Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash()); + } else { + processSendCoinsReturn(send_status); + send_failure = true; + } } } if (!send_failure) { @@ -841,7 +846,7 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn msgParams.second = CClientUIInterface::MSG_WARNING; // This comment is specific to SendCoinsDialog usage of WalletModel::SendCoinsReturn. - // All status values are used only in WalletModel::prepareTransaction() + // All status values except TransactionCommitFailed are used only in WalletModel::prepareTransaction() switch(sendCoinsReturn.status) { case WalletModel::InvalidAddress: @@ -860,6 +865,10 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn msgParams.first = tr("Transaction creation failed!"); msgParams.second = CClientUIInterface::MSG_ERROR; break; + case WalletModel::TransactionCommitFailed: + msgParams.first = tr("The transaction was not sent: %1").arg(sendCoinsReturn.reasonCommitFailed); + msgParams.second = CClientUIInterface::MSG_ERROR; + break; case WalletModel::AbsurdFee: msgParams.first = tr("A fee higher than %1 is considered an absurdly high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), model->wallet().getDefaultMaxTxFee())); break; diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 4fce62430de2..bb65763fa3bc 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -19,10 +19,18 @@ #include #include #include +#include #include +#include +#include #include +#include +#include #include #include +#include +#include +#include #include #include #include @@ -31,7 +39,9 @@ #include #include +#include #include +#include #include #include @@ -40,6 +50,7 @@ #include #include #include +#include #include #include #include @@ -48,7 +59,9 @@ using wallet::AddWallet; using wallet::CWallet; using wallet::CreateMockableWalletDatabase; +using wallet::GetBalance; using wallet::RemoveWallet; +using wallet::WALLET_FLAG_AVOID_REUSE; using wallet::WALLET_FLAG_DESCRIPTORS; using wallet::WalletContext; using wallet::WalletDescriptor; @@ -56,14 +69,16 @@ using wallet::WalletRescanReserver; namespace { -//! Press "Yes" or "Cancel" buttons in modal send confirmation dialog. -void ConfirmSend(QString* text = nullptr, bool cancel = false) +//! Press "Yes" or "Cancel" buttons in modal send confirmation dialog, +//! after running before_answer. +void ConfirmSend(QString* text = nullptr, bool cancel = false, std::function before_answer = {}) { - QTimer::singleShot(0, [text, cancel]() { + QTimer::singleShot(0, [text, cancel, before_answer]() { for (QWidget* widget : QApplication::topLevelWidgets()) { if (widget->inherits("SendConfirmationDialog")) { SendConfirmationDialog* dialog = qobject_cast(widget); if (text) *text = dialog->text(); + if (before_answer) before_answer(); QAbstractButton* button = dialog->button(cancel ? QMessageBox::Cancel : QMessageBox::Yes); button->setEnabled(true); button->click(); @@ -72,8 +87,10 @@ void ConfirmSend(QString* text = nullptr, bool cancel = false) }); } -//! Send coins to address and return txid. -uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDestination& address, CAmount amount) +//! Send coins to address and return txid. before_confirm runs once the +//! transaction is prepared, before the send is confirmed. +uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDestination& address, CAmount amount, + std::function before_confirm = {}) { QVBoxLayout* entries = sendCoinsDialog.findChild("entries"); SendCoinsEntry* entry = qobject_cast(entries->itemAt(0)->widget()); @@ -83,7 +100,7 @@ uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDe boost::signals2::scoped_connection c(wallet.NotifyTransactionChanged.connect([&txid](const uint256& hash, ChangeType status) { if (status == CT_NEW) txid = hash; })); - ConfirmSend(); + ConfirmSend(/*text=*/nullptr, /*cancel=*/false, std::move(before_confirm)); bool invoked = QMetaObject::invokeMethod(&sendCoinsDialog, "sendButtonClicked", Q_ARG(bool, false)); assert(invoked); return txid; @@ -173,6 +190,9 @@ void TestGUI(interfaces::Node& node) WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel); sendCoinsDialog.setModel(&walletModel); transactionView.setModel(&walletModel); + WalletView walletView(&walletModel, nullptr); + walletView.setClientModel(&clientModel); + QSignalSpy notified(&walletView, &WalletView::incomingTransaction); // Update walletModel cached balance which will trigger an update for the 'labelBalance' QLabel. walletModel.pollBalanceChanged(); @@ -187,9 +207,170 @@ void TestGUI(interfaces::Node& node) // Transaction table model updates on a QueuedConnection, so process events to ensure it's updated. qApp->processEvents(); QCOMPARE(transactionTableModel->rowCount({}), 107); + QCOMPARE(notified.count(), 2); QVERIFY(FindTx(*transactionTableModel, txid1).isValid()); QVERIFY(FindTx(*transactionTableModel, txid2).isValid()); + // A send that cannot be broadcast is reported as an error instead of as + // sent, and is abandoned so its coins are spendable again. The fee ceiling + // is lowered after the transaction is prepared, so only the broadcast + // refuses it. + { + QSignalSpy sent(&sendCoinsDialog, &SendCoinsDialog::coinsSent); + QSignalSpy messages(&sendCoinsDialog, &SendCoinsDialog::message); + const CAmount balance{walletModel.wallet().getBalance()}; + const CAmount max_fee{wallet->m_default_max_tx_fee}; + const uint256 refused{SendCoins(*wallet, sendCoinsDialog, PKHash(), 2 * COIN, + [&wallet] { wallet->m_default_max_tx_fee = 1; })}; + wallet->m_default_max_tx_fee = max_fee; + QVERIFY(!refused.IsNull()); + QCOMPARE(sent.count(), 0); + QCOMPARE(messages.count(), 1); + QCOMPARE(messages.first().at(2).toUInt(), static_cast(CClientUIInterface::MSG_ERROR)); + QVERIFY(!node.context()->mempool->exists(refused)); + QCOMPARE(walletModel.wallet().getBalance(), balance); + // Its new row raises no "Sent transaction" notification. + qApp->processEvents(); + QVERIFY(FindTx(*transactionTableModel, refused).isValid()); + QCOMPARE(notified.count(), 2); + // The form is kept for a retry. + QVBoxLayout* entries{sendCoinsDialog.findChild("entries")}; + SendCoinsEntry* entry{qobject_cast(entries->itemAt(0)->widget())}; + QCOMPARE(entry->findChild("payAmount")->value(), 2 * COIN); + } + + // Retrying a refused send can rebuild the identical transaction, which the + // wallet holds abandoned: it is committed and sent like a new one. + { + const QList recipients{ + SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", 3 * COIN, ""}}; + WalletModelTransaction retried{recipients}; + QCOMPARE(walletModel.prepareTransaction(retried, wallet::CCoinControl{}).status, WalletModel::OK); + const CAmount balance{walletModel.wallet().getBalance()}; + const CAmount max_fee{wallet->m_default_max_tx_fee}; + wallet->m_default_max_tx_fee = 1; + const auto over_max_fee{walletModel.sendCoins(retried, /*fIsCoinJoin=*/false)}; + wallet->m_default_max_tx_fee = max_fee; + QCOMPARE(over_max_fee.status, WalletModel::TransactionCommitFailed); + QCOMPARE(over_max_fee.reasonCommitFailed, + QString::fromStdString(TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED).original)); + QCOMPARE(walletModel.wallet().getBalance(), balance); + QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::OK); + QVERIFY(node.context()->mempool->exists(retried.getWtx()->GetHash())); + QVERIFY(walletModel.wallet().getBalance() < balance); + + // Mined through another path before the wallet processed the block + // (this wallet does not follow the chain yet), it was sent. + WalletModelTransaction mined{ + {SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", 4 * COIN, ""}}}; + QCOMPARE(walletModel.prepareTransaction(mined, wallet::CCoinControl{}).status, WalletModel::OK); + test.CreateAndProcessBlock({CMutableTransaction{*mined.getWtx()}}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey())); + QCOMPARE(walletModel.sendCoins(mined, /*fIsCoinJoin=*/false).status, WalletModel::OK); + + // Committed again once mined, it stays confirmed and its inputs spent. + // The wallet follows the chain for this, as a loaded wallet does. + wallet->m_chain_notifications_handler = node.context()->chain->handleNotifications(wallet); + test.CreateAndProcessBlock({CMutableTransaction{*retried.getWtx()}}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey())); + SyncWithValidationInterfaceQueue(); + const CAmount mined_balance{walletModel.wallet().getBalance()}; + QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::OK); + interfaces::WalletTxStatus status{}; + interfaces::WalletOrderForm order_form; + bool in_mempool{false}; + int num_blocks{0}; + walletModel.wallet().getWalletTxDetails(retried.getWtx()->GetHash(), status, order_form, in_mempool, num_blocks); + QCOMPARE(status.depth_in_main_chain, 1); + QCOMPARE(walletModel.wallet().getBalance(), mined_balance); + + // Relayed through another path and seen by the wallet first, it still + // gets what the send records about it, such as the payment message. + WalletModelTransaction relayed{ + {SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", 1 * COIN, "relayed elsewhere"}}}; + QCOMPARE(walletModel.prepareTransaction(relayed, wallet::CCoinControl{}).status, WalletModel::OK); + bilingual_str relay_error; + QCOMPARE(node.broadcastTransaction(relayed.getWtx(), /*max_tx_fee=*/0, relay_error), TransactionError::OK); + SyncWithValidationInterfaceQueue(); + QCOMPARE(walletModel.sendCoins(relayed, /*fIsCoinJoin=*/false).status, WalletModel::OK); + walletModel.wallet().getWalletTxDetails(relayed.getWtx()->GetHash(), status, order_form, in_mempool, num_blocks); + QVERIFY(in_mempool); + QCOMPARE(order_form, (interfaces::WalletOrderForm{{"Message", "relayed elsewhere"}})); + } + + // The mempool's reason reaches the caller: of two sends prepared over the + // same coin, the second is refused as a conflict of the first. + { + std::optional coin; + for (const auto& [dest, coins] : walletModel.wallet().listCoins()) { + for (const auto& [outpoint, out] : coins) { + if (!coin && !out.is_spent && out.depth_in_main_chain > 0) coin = outpoint; + } + } + QVERIFY(coin); + wallet::CCoinControl same_coin; + same_coin.Select(*coin); + same_coin.m_allow_other_inputs = false; + const QList recipients{ + SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", COIN, ""}}; + WalletModelTransaction first{recipients}; + WalletModelTransaction conflict{recipients}; + QCOMPARE(walletModel.prepareTransaction(first, same_coin).status, WalletModel::OK); + QCOMPARE(walletModel.prepareTransaction(conflict, same_coin).status, WalletModel::OK); + QCOMPARE(walletModel.sendCoins(first, /*fIsCoinJoin=*/false).status, WalletModel::OK); + const auto refused{walletModel.sendCoins(conflict, /*fIsCoinJoin=*/false)}; + QCOMPARE(refused.status, WalletModel::TransactionCommitFailed); + QVERIFY2(refused.reasonCommitFailed.contains("txn-mempool-conflict"), qPrintable(refused.reasonCommitFailed)); + } + + // With avoid_reuse, a refused send's input address stays marked as used + // after the abandon, as for any abandoned transaction. The balance leaves + // out every coin at that address, so a send above what remains fails the + // balance check, although the coins are spendable again. + { + // The wallet follows the chain by now. Let it process the earlier sends' + // mempool notifications first, or they would mark their inputs as used. + SyncWithValidationInterfaceQueue(); + wallet->SetWalletFlag(WALLET_FLAG_AVOID_REUSE); + std::optional coin; + for (const auto& [dest, coins] : walletModel.wallet().listCoins()) { + for (const auto& [outpoint, out] : coins) { + if (!coin && out.depth_in_main_chain > 0 && + out.txout.scriptPubKey == GetScriptForRawPubKey(test.coinbaseKey.GetPubKey())) { + coin = outpoint; + } + } + } + QVERIFY(coin); + wallet::CCoinControl one_coin; + one_coin.Select(*coin); + one_coin.m_allow_other_inputs = false; + const QList recipients{ + SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", COIN, ""}}; + WalletModelTransaction refused{recipients}; + QCOMPARE(walletModel.prepareTransaction(refused, one_coin).status, WalletModel::OK); + const CAmount max_fee{wallet->m_default_max_tx_fee}; + wallet->m_default_max_tx_fee = 1; + QCOMPARE(walletModel.sendCoins(refused, /*fIsCoinJoin=*/false).status, WalletModel::TransactionCommitFailed); + wallet->m_default_max_tx_fee = max_fee; + const CAmount unused{walletModel.wallet().getBalance()}; + const CAmount spendable{GetBalance(*wallet, /*min_depth=*/0, /*avoid_reuse=*/false).m_mine_trusted}; + QVERIFY(unused + COIN < spendable); + qApp->processEvents(); + walletModel.pollBalanceChanged(); + QCOMPARE(walletModel.getCachedBalance().balance, unused); + WalletModelTransaction over_unused{ + {SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", unused + COIN, ""}}}; + QCOMPARE(walletModel.prepareTransaction(over_unused, wallet::CCoinControl{}).status, WalletModel::AmountExceedsBalance); + // Coin control can still pick the refused send's coin. + WalletModelTransaction retried{recipients}; + QCOMPARE(walletModel.prepareTransaction(retried, one_coin).status, WalletModel::OK); + QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::OK); + QVERIFY(node.context()->mempool->exists(retried.getWtx()->GetHash())); + // Cached balances were computed under the flag. + wallet->UnsetWalletFlag(WALLET_FLAG_AVOID_REUSE); + wallet->MarkDirty(); + qApp->processEvents(); + } + // Check current balance on OverviewPage OverviewPage overviewPage; overviewPage.setClientModel(&clientModel); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 5e146b6106dc..f8c77be578eb 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -324,7 +324,20 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact return SendCoinsReturn(OK); } -void WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin) +namespace { +bool WasSent(interfaces::Wallet& wallet, const uint256& txid) +{ + interfaces::WalletTxStatus status{}; + interfaces::WalletOrderForm order_form; + bool in_mempool{false}; + int num_blocks{0}; + wallet.getWalletTxDetails(txid, status, order_form, in_mempool, num_blocks); + return status.depth_in_main_chain > 0 || + (status.depth_in_main_chain == 0 && (in_mempool || status.is_islocked)); +} +} // namespace + +WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin) { QByteArray transaction_array; /* store serialized transaction */ @@ -342,7 +355,18 @@ void WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoi } auto& newTx = transaction.getWtx(); - wallet().commitTransaction(newTx, /*value_map=*/std::move(mapValue), std::move(vOrderForm)); + // A refused transaction was never relayed. Abandon it, so the wallet + // does not rebroadcast a payment the user is told failed and its inputs + // are free for a retry. It cannot be abandoned if a resend got it into + // the mempool or a block, or it was InstantSend-locked, meanwhile, and + // then it was sent, or if a spend of the same inputs has confirmed, and + // then it was not. + const auto res{wallet().commitTransaction(newTx, /*value_map=*/std::move(mapValue), std::move(vOrderForm))}; + if (!res && (wallet().abandonTransaction(newTx->GetHash()) || !WasSent(wallet(), newTx->GetHash()))) { + const bilingual_str error{util::ErrorString(res)}; + return SendCoinsReturn(TransactionCommitFailed, + QString::fromStdString(error.translated.empty() ? error.original : error.translated)); + } CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << *newTx; @@ -375,6 +399,7 @@ void WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoi } checkBalanceChanged(m_wallet->getBalances()); // update balance immediately, otherwise there could be a short noticeable delay until pollBalanceChanged hits + return SendCoinsReturn(OK); } OptionsModel* WalletModel::getOptionsModel() const diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index c540b0a0a2e4..48c837d8ae15 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -64,6 +64,7 @@ class WalletModel : public QObject AmountWithFeeExceedsBalance, DuplicateAddress, TransactionCreationFailed, // Error returned when wallet is still locked + TransactionCommitFailed, AbsurdFee }; @@ -101,8 +102,9 @@ class WalletModel : public QObject // prepare transaction for getting txfee before sending coins SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const wallet::CCoinControl& coinControl); - // Send coins to a list of recipients - void sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin); + // Send coins to a list of recipients; TransactionCommitFailed when the + // transaction could not be broadcast, in which case it is abandoned + SendCoinsReturn sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin); // Wallet encryption bool setWalletEncrypted(const SecureString& passphrase); diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index bbc07ec0a3d0..792f584aee95 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -188,6 +188,10 @@ void WalletView::processNewTransaction(const QModelIndex& parent, int start, int return; QModelIndex index = ttm->index(start, 0, parent); + // WalletModel::sendCoins abandons a send whose broadcast was refused + // before its row arrives here. It was not sent. + if (ttm->data(index, TransactionTableModel::StatusRole).toInt() == TransactionStatus::Abandoned) return; + QSettings settings; if (!settings.value("fShowCoinJoinPopups").toBool()) { QVariant nType = ttm->data(index, TransactionTableModel::TypeRole); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 8f41cc393806..20db3c28a0c5 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -1008,7 +1008,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); - BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error) == TransactionError::OK); // Reload wallet and make sure new transactions are detected despite events @@ -1049,7 +1049,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); - BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error) == TransactionError::OK); SyncWithValidationInterfaceQueue(); }); wallet = TestLoadWallet(context); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8d151593bc46..1acf2f03b77d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1424,7 +1424,7 @@ bool CWallet::ResendTransaction(const uint256& hashTx) CWalletTx& wtx = it->second; bilingual_str unused_err_string; - return SubmitTxMemoryPoolAndRelay(wtx, unused_err_string, true); + return SubmitTxMemoryPoolAndRelay(wtx, unused_err_string, true) == TransactionError::OK; } void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx) @@ -2200,11 +2200,11 @@ bool CWallet::CanTxBeResent(const CWalletTx& wtx) const !IsTxLockedByInstantSend(wtx); } -bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const +std::optional CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const { AssertLockHeld(cs_wallet); - if (!CanTxBeResent(wtx)) return false; + if (!CanTxBeResent(wtx)) return std::nullopt; // Submit transaction to mempool for relay WalletLogPrintf("Submitting wtx %s to mempool for relay\n", wtx.GetHash().ToString()); @@ -2217,8 +2217,8 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_stri // If broadcast fails for any reason, trying to set wtx.m_state here would be incorrect. // If transaction was previously in the mempool, it should be updated when // TransactionRemovedFromMempool fires. - bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string); - if (ret) wtx.m_state = TxStateInMempool{}; + const TransactionError ret{chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string)}; + if (ret == TransactionError::OK) wtx.m_state = TxStateInMempool{}; return ret; } @@ -2302,7 +2302,7 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force) // Now try submitting the transactions to the memory pool and (optionally) relay them. for (auto wtx : to_submit) { bilingual_str unused_err_string; - if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, relay)) ++submitted_tx_count; + if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, relay) == TransactionError::OK) ++submitted_tx_count; } } // cs_wallet @@ -2569,22 +2569,15 @@ util::Result CWallet::CommitTransaction(CTransactionRef tx, mapValue_t map } bilingual_str err_string; - if (!SubmitTxMemoryPoolAndRelay(*wtx, err_string, true)) { - // A transaction already in the chain, e.g. mined through another path - // before this wallet processed its block, was sent. BroadcastTransaction - // refuses it without a reason, having found its outputs. - if (err_string.empty()) { - std::map outputs; - for (uint32_t i{0}; i < tx->vout.size(); ++i) outputs.emplace(COutPoint{tx->GetHash(), i}, Coin{}); - chain().findCoins(outputs); - if (std::any_of(outputs.begin(), outputs.end(), [](const auto& output) { return !output.second.IsSpent(); })) return {}; - } - WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string.original); - // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. - // Not every refusal carries a reason, e.g. the -maxtxfee check in BroadcastTransaction. - return util::Error{err_string.empty() ? _("Transaction could not be broadcast") : err_string}; - } - return {}; + const auto broadcast{SubmitTxMemoryPoolAndRelay(*wtx, err_string, true)}; + // A transaction already in the chain, e.g. mined through another path + // before this wallet processed its block, was sent. + if (broadcast == TransactionError::OK || broadcast == TransactionError::ALREADY_IN_CHAIN) return {}; + // Not every refusal carries the mempool's reason, e.g. the -maxtxfee check in BroadcastTransaction. + if (err_string.empty()) err_string = broadcast ? TransactionErrorString(*broadcast) : _("Transaction could not be broadcast"); + WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string.original); + // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. + return util::Error{err_string}; } DBErrors CWallet::LoadWallet() diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 02af32e21391..45699678003b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -854,8 +854,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Will SubmitTxMemoryPoolAndRelay() consider wtx if supplied */ bool CanTxBeResent(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** Pass this transaction to node for mempool insertion and relay to peers if flag set to true */ - bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const + /** + * Pass this transaction to node for mempool insertion and relay to peers if flag set to true. + * @return the node's broadcast result, or nullopt if CanTxBeResent() kept wtx from the node. + */ + std::optional SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool DummySignTx(CMutableTransaction &txNew, const std::vector &txouts, const CCoinControl* coin_control = nullptr) const;