From d1734f9a3b138ab046f38ee44a09bc3847bf938a Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 3 Apr 2019 17:55:24 -0400 Subject: [PATCH] [wallet] Remove return value from CommitTransaction() CommitTransaction returns a bool to indicate success, but since commit b3a74100b8 it only returns true, even if the transaction was not successfully broadcast. This commit changes CommitTransaction() to return void. All dead code in `if (!CommitTransaction())` branches has been removed. --- src/interfaces/wallet.cpp | 11 +++-------- src/interfaces/wallet.h | 5 ++--- src/qt/sendcoinsdialog.cpp | 7 +------ src/qt/walletmodel.cpp | 4 +--- src/qt/walletmodel.h | 1 - src/wallet/feebumper.cpp | 6 +----- src/wallet/rpcwallet.cpp | 11 ++--------- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 17 +++++++++-------- src/wallet/wallet.h | 2 +- 10 files changed, 21 insertions(+), 45 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index aff08bfbea1..1830de8a27d 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -218,19 +218,14 @@ public: } return tx; } - bool commitTransaction(CTransactionRef tx, + void commitTransaction(CTransactionRef tx, WalletValueMap value_map, - WalletOrderForm order_form, - std::string& reject_reason) override + WalletOrderForm order_form) override { auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); CValidationState state; - if (!m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form), state)) { - reject_reason = state.GetRejectReason(); - return false; - } - return true; + m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form), state); } bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); } bool abandonTransaction(const uint256& txid) override diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 89e056b18b8..a96b93b4c3d 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -141,10 +141,9 @@ public: std::string& fail_reason) = 0; //! Commit transaction. - virtual bool commitTransaction(CTransactionRef tx, + virtual void commitTransaction(CTransactionRef tx, WalletValueMap value_map, - WalletOrderForm order_form, - std::string& reject_reason) = 0; + WalletOrderForm order_form) = 0; //! Return whether transaction can be abandoned. virtual bool transactionCanBeAbandoned(const uint256& txid) = 0; diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 003a31b2488..80ea6cd2e60 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -558,8 +558,7 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn msgParams.second = CClientUIInterface::MSG_WARNING; // This comment is specific to SendCoinsDialog usage of WalletModel::SendCoinsReturn. - // WalletModel::TransactionCommitFailed is used only in WalletModel::sendCoins() - // all others are used only in WalletModel::prepareTransaction() + // All status values are used only in WalletModel::prepareTransaction() switch(sendCoinsReturn.status) { case WalletModel::InvalidAddress: @@ -581,10 +580,6 @@ 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 rejected with the following reason: %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/walletmodel.cpp b/src/qt/walletmodel.cpp index 49a13330ec0..5bc72125f68 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -260,9 +260,7 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran } auto& newTx = transaction.getWtx(); - std::string rejectReason; - if (!wallet().commitTransaction(newTx, {} /* mapValue */, std::move(vOrderForm), rejectReason)) - return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(rejectReason)); + wallet().commitTransaction(newTx, {} /* mapValue */, std::move(vOrderForm)); CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << *newTx; diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 54428aec082..d8dd6c74a3f 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -139,7 +139,6 @@ public: AmountWithFeeExceedsBalance, DuplicateAddress, TransactionCreationFailed, // Error returned when wallet is still locked - TransactionCommitFailed, AbsurdFee, PaymentRequestExpired }; diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index fbcf8d4de75..c9cd042b031 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -394,11 +394,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti mapValue["replaces_txid"] = oldWtx.GetHash().ToString(); CValidationState state; - if (!wallet.CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state)) { - // NOTE: CommitTransaction never returns false, so this should never happen. - errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state))); - return Result::WALLET_ERROR; - } + wallet.CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state); bumped_txid = tx->GetHash(); if (state.IsInvalid()) { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index debc3fecda4..62066ddcc78 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -343,10 +343,7 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet throw JSONRPCError(RPC_WALLET_ERROR, strError); } CValidationState state; - if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state)) { - strError = strprintf("Error: The transaction was rejected! Reason given: %s", FormatStateMessage(state)); - throw JSONRPCError(RPC_WALLET_ERROR, strError); - } + pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state); return tx; } @@ -928,11 +925,7 @@ static UniValue sendmany(const JSONRPCRequest& request) if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); CValidationState state; - if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state)) { - strFailReason = strprintf("Transaction commit failed:: %s", FormatStateMessage(state)); - throw JSONRPCError(RPC_WALLET_ERROR, strFailReason); - } - + pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state); return tx->GetHash().GetHex(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 73523ca36df..f31cd1e1b63 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -452,7 +452,7 @@ public: BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, fee, changePos, error, dummy)); } CValidationState state; - BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, state)); + wallet->CommitTransaction(tx, {}, {}, state); CMutableTransaction blocktx; { LOCK(wallet->cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a9ea56b6ab1..9c5de45c280 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3286,7 +3286,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std return true; } -bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state) +void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state) { auto locked_chain = chain().lock(); LOCK(cs_wallet); @@ -3314,15 +3314,16 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve // fInMempool flag is cached properly CWalletTx& wtx = mapWallet.at(wtxNew.GetHash()); - if (fBroadcastTransactions) { - std::string err_string; - if (!wtx.SubmitMemoryPoolAndRelay(err_string, true, *locked_chain)) { - WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string); - // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. - } + if (!fBroadcastTransactions) { + // Don't submit tx to the mempool + return; } - return true; + std::string err_string; + if (!wtx.SubmitMemoryPoolAndRelay(err_string, true, *locked_chain)) { + WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string); + // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. + } } DBErrors CWallet::LoadWallet(bool& fFirstRunRet) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d12c6077bf5..8113e60aa61 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1157,7 +1157,7 @@ public: * @param orderForm[in] BIP 70 / BIP 21 order form details to be set on the transaction. * @param state[in,out] CValidationState object returning information about whether the transaction was accepted */ - bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state); + void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state); bool DummySignTx(CMutableTransaction &txNew, const std::set &txouts, bool use_max_sig = false) const {