maxmustermann/dogecoin
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Merge #17154: wallet: Remove return value from CommitTransaction

Browse source 
9e95931865 [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery)
d1734f9a3b [wallet] Remove return value from CommitTransaction() (John Newbery)
b6f486a02b [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery)
8bba91b22d [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 9e95931865

Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6
This commit is contained in:
Wladimir J. van der Laan 2019-10-24 10:14:55 +02:00
parent c5ac7af779 9e95931865
commit 8a191148db
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
10 changed files with 54 additions and 92 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
src/interfaces/wallet.cpp
View file

@ -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

5
src/interfaces/wallet.h
View file

@ -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;

7
src/qt/sendcoinsdialog.cpp
View file

@ -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;

4
src/qt/walletmodel.cpp
View file

@ -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;

1
src/qt/walletmodel.h
View file

@ -138,7 +138,6 @@ public:
AmountWithFeeExceedsBalance,
DuplicateAddress,
TransactionCreationFailed, // Error returned when wallet is still locked
TransactionCommitFailed,
AbsurdFee,
PaymentRequestExpired
};

16
src/wallet/feebumper.cpp
View file

@ -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

14
src/wallet/rpcwallet.cpp
View file

@ -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();
}

4
src/wallet/test/wallet_tests.cpp
View file

@ -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);

71
src/wallet/wallet.cpp
View file

@ -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);
}
}
// 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.
}
}
// 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());
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)

11
src/wallet/wallet.h
View file

@ -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
{