aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorWladimir J. van der Laan <laanwj@protonmail.com>2019-10-24 10:14:55 +0200
committerWladimir J. van der Laan <laanwj@protonmail.com>2019-10-24 10:16:12 +0200
commit8a191148db3f01f2a345fa2db6bc26d4d88e0e3b (patch)
treee33e738095ff624974706869a6027f1e8a081602
parentc5ac7af7793a70d73d6bab09a7bc29dc4ddc7ea2 (diff)
parent9e95931865186d7a9a6dc54b64bd96507e9fea4b (diff)
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.cpp13
-rw-r--r--src/interfaces/wallet.h5
-rw-r--r--src/qt/sendcoinsdialog.cpp7
-rw-r--r--src/qt/walletmodel.cpp4
-rw-r--r--src/qt/walletmodel.h1
-rw-r--r--src/wallet/feebumper.cpp16
-rw-r--r--src/wallet/rpcwallet.cpp14
-rw-r--r--src/wallet/test/wallet_tests.cpp4
-rw-r--r--src/wallet/wallet.cpp67
-rw-r--r--src/wallet/wallet.h11
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
{