diff options
author | Wladimir J. van der Laan <laanwj@protonmail.com> | 2019-10-24 10:14:55 +0200 |
---|---|---|
committer | Wladimir J. van der Laan <laanwj@protonmail.com> | 2019-10-24 10:16:12 +0200 |
commit | 8a191148db3f01f2a345fa2db6bc26d4d88e0e3b (patch) | |
tree | e33e738095ff624974706869a6027f1e8a081602 | |
parent | c5ac7af7793a70d73d6bab09a7bc29dc4ddc7ea2 (diff) | |
parent | 9e95931865186d7a9a6dc54b64bd96507e9fea4b (diff) | |
download | bitcoin-8a191148db3f01f2a345fa2db6bc26d4d88e0e3b.tar.xz |
Merge #17154: wallet: Remove return value from CommitTransaction
9e95931865186d7a9a6dc54b64bd96507e9fea4b [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery)
d1734f9a3b138ab046f38ee44a09bc3847bf938a [wallet] Remove return value from CommitTransaction() (John Newbery)
b6f486a02b463ffeaf82ec11fc6f74f439c037ae [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery)
8bba91b22d22a8dfea7c947b542b1022bfc1c0ea [wallet] Fix whitespace in CWallet::CommitTransaction() (John Newbery)
Pull request description:
`CommitTransaction()` returns a bool to indicate success, but since commit
b3a7410 (#9302) 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.
Two additional commits fix up the idiosyncratic whitespace in `CommitTransaction` and add a doxygen comment for the function.
ACKs for top commit:
laanwj:
ACK 9e95931865186d7a9a6dc54b64bd96507e9fea4b
Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6
-rw-r--r-- | src/interfaces/wallet.cpp | 13 | ||||
-rw-r--r-- | src/interfaces/wallet.h | 5 | ||||
-rw-r--r-- | src/qt/sendcoinsdialog.cpp | 7 | ||||
-rw-r--r-- | src/qt/walletmodel.cpp | 4 | ||||
-rw-r--r-- | src/qt/walletmodel.h | 1 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 16 | ||||
-rw-r--r-- | src/wallet/rpcwallet.cpp | 14 | ||||
-rw-r--r-- | src/wallet/test/wallet_tests.cpp | 4 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 67 | ||||
-rw-r--r-- | src/wallet/wallet.h | 11 |
10 files changed, 52 insertions, 90 deletions
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 2353fe0934..9b0a8b64c9 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -5,7 +5,6 @@ #include <interfaces/wallet.h> #include <amount.h> -#include <consensus/validation.h> #include <interfaces/chain.h> #include <interfaces/handler.h> #include <policy/fees.h> @@ -216,19 +215,13 @@ 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)); } 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 f16fe81bd9..d180c9f8c4 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -138,7 +138,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..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 <consensus/validation.h> #include <interfaces/chain.h> #include <wallet/coincontrol.h> #include <wallet/feebumper.h> @@ -393,21 +392,10 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti mapValue_t mapValue = oldWtx.mapValue; 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; - } - - 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 e159866921..e571501221 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 <amount.h> -#include <consensus/validation.h> #include <core_io.h> #include <init.h> #include <interfaces/chain.h> @@ -341,11 +340,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; - 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 */); return tx; } @@ -926,12 +921,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; - 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 */); return tx->GetHash().GetHex(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 73523ca36d..a2b2a7b227 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -8,7 +8,6 @@ #include <stdint.h> #include <vector> -#include <consensus/validation.h> #include <interfaces/chain.h> #include <policy/policy.h> #include <rpc/server.h> @@ -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; - BOOST_CHECK(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 6adcf15167..159d4f78c6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3284,51 +3284,44 @@ 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<std::pair<std::string, std::string>> orderForm, CValidationState& state) +void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm) { - { - 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) { + // Don't submit tx to the mempool + return; + } + + 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; } DBErrors CWallet::LoadWallet(bool& fFirstRunRet) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f9e2230a6f..85c277ff50 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1146,7 +1146,16 @@ public: */ bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); - bool CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, CValidationState& state); + /** + * 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. + */ + void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm); bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts, bool use_max_sig = false) const { |