From 8bba91b22d22a8dfea7c947b542b1022bfc1c0ea Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 3 Apr 2019 17:29:21 -0400 Subject: [PATCH 1/4] [wallet] Fix whitespace in CWallet::CommitTransaction() Reviewer hint: use --ignore-all-space git diff option for review. --- src/wallet/wallet.cpp | 57 ++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1551c645d0..f77c413593 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3291,45 +3291,40 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std */ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state) { - { - auto locked_chain = chain().lock(); - LOCK(cs_wallet); + auto locked_chain = chain().lock(); + LOCK(cs_wallet); - CWalletTx wtxNew(this, std::move(tx)); - wtxNew.mapValue = std::move(mapValue); - wtxNew.vOrderForm = std::move(orderForm); - wtxNew.fTimeReceivedIsTxTime = true; - wtxNew.fFromMe = true; + CWalletTx wtxNew(this, std::move(tx)); + wtxNew.mapValue = std::move(mapValue); + wtxNew.vOrderForm = std::move(orderForm); + wtxNew.fTimeReceivedIsTxTime = true; + wtxNew.fFromMe = true; - WalletLogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */ - { + WalletLogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */ - // Add tx to wallet, because if it has change it's also ours, - // otherwise just for transaction history. - AddToWallet(wtxNew); + // Add tx to wallet, because if it has change it's also ours, + // otherwise just for transaction history. + AddToWallet(wtxNew); - // Notify that old coins are spent - for (const CTxIn& txin : wtxNew.tx->vin) - { - CWalletTx &coin = mapWallet.at(txin.prevout.hash); - coin.BindWallet(this); - NotifyTransactionChanged(this, coin.GetHash(), CT_UPDATED); - } - } + // Notify that old coins are spent + for (const CTxIn& txin : wtxNew.tx->vin) { + CWalletTx &coin = mapWallet.at(txin.prevout.hash); + coin.BindWallet(this); + NotifyTransactionChanged(this, coin.GetHash(), CT_UPDATED); + } - // Get the inserted-CWalletTx from mapWallet so that the - // fInMempool flag is cached properly - CWalletTx& wtx = mapWallet.at(wtxNew.GetHash()); + // Get the inserted-CWalletTx from mapWallet so that the + // 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) { + 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. } } + return true; } From b6f486a02b463ffeaf82ec11fc6f74f439c037ae Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 3 Apr 2019 17:51:23 -0400 Subject: [PATCH 2/4] [wallet] Add doxygen comment to CWallet::CommitTransaction() --- src/wallet/wallet.cpp | 3 --- src/wallet/wallet.h | 10 ++++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f77c413593..a9ea56b6ab 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3286,9 +3286,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std return true; } -/** - * Call after CreateTransaction unless you want to abort - */ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state) { auto locked_chain = chain().lock(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 006775e83b..d12c6077bf 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1147,6 +1147,16 @@ public: */ bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); + /** + * Submit the transaction to the node's mempool and then relay to peers. + * Should be called after CreateTransaction unless you want to abort + * broadcasting the transaction. + * + * @param tx[in] The transaction to be broadcast. + * @param mapValue[in] key-values to be set on the transaction. + * @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); bool DummySignTx(CMutableTransaction &txNew, const std::set &txouts, bool use_max_sig = false) const From d1734f9a3b138ab046f38ee44a09bc3847bf938a Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 3 Apr 2019 17:55:24 -0400 Subject: [PATCH 3/4] [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 aff08bfbea..1830de8a27 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 89e056b18b..a96b93b4c3 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 003a31b248..80ea6cd2e6 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 49a13330ec..5bc72125f6 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 54428aec08..d8dd6c74a3 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 fbcf8d4de7..c9cd042b03 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 debc3fecda..62066ddcc7 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 73523ca36d..f31cd1e1b6 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 a9ea56b6ab..9c5de45c28 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 d12c6077bf..8113e60aa6 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 { From 9e95931865186d7a9a6dc54b64bd96507e9fea4b Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 18 Oct 2019 09:37:40 -0400 Subject: [PATCH 4/4] [wallet] Remove `state` argument from CWallet::CommitTransaction The `state` return argument has not been set since commit 611291c198. Remove it (and the one place that it's used in a calling function). --- src/interfaces/wallet.cpp | 4 +--- src/wallet/feebumper.cpp | 12 ++---------- src/wallet/rpcwallet.cpp | 7 ++----- src/wallet/test/wallet_tests.cpp | 4 +--- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 3 +-- 6 files changed, 8 insertions(+), 24 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 1830de8a27..530f19e2d6 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -5,7 +5,6 @@ #include #include -#include #include #include #include @@ -224,8 +223,7 @@ public: { auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); - CValidationState state; - m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form), state); + m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form)); } bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); } bool abandonTransaction(const uint256& txid) override diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index c9cd042b03..0a4bb3f396 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -2,7 +2,6 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include #include #include #include @@ -393,17 +392,10 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti mapValue_t mapValue = oldWtx.mapValue; mapValue["replaces_txid"] = oldWtx.GetHash().ToString(); - CValidationState state; - wallet.CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, state); - - bumped_txid = tx->GetHash(); - if (state.IsInvalid()) { - // This can happen if the mempool rejected the transaction. Report - // what happened in the "errors" response. - errors.push_back(strprintf("Error: The transaction was rejected: %s", FormatStateMessage(state))); - } + wallet.CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm); // mark the original tx as bumped + bumped_txid = tx->GetHash(); if (!wallet.MarkReplaced(oldWtx.GetHash(), bumped_txid)) { // TODO: see if JSON-RPC has a standard way of returning a response // along with an exception. It would be good to return information about diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 62066ddcc7..35d550a2e3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4,7 +4,6 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include -#include #include #include #include @@ -342,8 +341,7 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired)); throw JSONRPCError(RPC_WALLET_ERROR, strError); } - CValidationState state; - pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state); + pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */); return tx; } @@ -924,8 +922,7 @@ static UniValue sendmany(const JSONRPCRequest& request) bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strFailReason, coin_control); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); - CValidationState state; - pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, state); + pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */); return tx->GetHash().GetHex(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index f31cd1e1b6..a2b2a7b227 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -8,7 +8,6 @@ #include #include -#include #include #include #include @@ -451,8 +450,7 @@ public: auto locked_chain = m_chain->lock(); BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, fee, changePos, error, dummy)); } - CValidationState state; - wallet->CommitTransaction(tx, {}, {}, state); + wallet->CommitTransaction(tx, {}, {}); CMutableTransaction blocktx; { LOCK(wallet->cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9c5de45c28..413013e32e 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; } -void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state) +void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm) { auto locked_chain = chain().lock(); LOCK(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8113e60aa6..c7834a5d02 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1155,9 +1155,8 @@ public: * @param tx[in] The transaction to be broadcast. * @param mapValue[in] key-values to be set on the transaction. * @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 */ - void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm, CValidationState& state); + void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm); bool DummySignTx(CMutableTransaction &txNew, const std::set &txouts, bool use_max_sig = false) const {