Skip to content

Commit

Permalink
[wallet] Construct CWalletTx objects in CommitTransaction
Browse files Browse the repository at this point in the history 
Construct CWalletTx objects in CWallet::CommitTransaction, instead of having
callers do it. This ensures CWalletTx objects are constructed in a uniform way
and all fields are set.

This also makes it possible to avoid confusing and wasteful CWalletTx copies in
bitcoin#9381

There is no change in behavior.
  • Loading branch information
@ryanofsky
ryanofsky committed Nov 20, 2017
1 parent 901ba3e commit 0fdca91
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 73 deletions.
14 changes: 7 additions & 7 deletions src/qt/walletmodel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
int nChangePosRet = -1;
std::string strFailReason;

CWalletTx *newTx = transaction.getTransaction();
CTransactionRef* newTx = transaction.getTransaction();
CReserveKey *keyChange = transaction.getPossibleKeyChange();
bool fCreated = wallet->CreateTransaction(vecSend, *newTx, *keyChange, nFeeRequired, nChangePosRet, strFailReason, coinControl);
transaction.setTransactionFee(nFeeRequired);
Expand Down Expand Up @@ -308,8 +308,8 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran

{
LOCK2(cs_main, wallet->cs_wallet);
CWalletTx *newTx = transaction.getTransaction();

std::vector<std::pair<std::string, std::string>> vOrderForm;
for (const SendCoinsRecipient &rcp : transaction.getRecipients())
{
if (rcp.paymentRequest.IsInitialized())
Expand All @@ -320,22 +320,22 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran
}

// Store PaymentRequests in wtx.vOrderForm in wallet.
std::string key("PaymentRequest");
std::string value;
rcp.paymentRequest.SerializeToString(&value);
newTx->vOrderForm.push_back(make_pair(key, value));
vOrderForm.emplace_back("PaymentRequest", std::move(value));
}
else if (!rcp.message.isEmpty()) // Message from normal bitcoin:URI (bitcoin:123...?message=example)
newTx->vOrderForm.push_back(make_pair("Message", rcp.message.toStdString()));
vOrderForm.emplace_back("Message", rcp.message.toStdString());
}

CTransactionRef* newTx = transaction.getTransaction();
CReserveKey *keyChange = transaction.getPossibleKeyChange();
CValidationState state;
if(!wallet->CommitTransaction(*newTx, *keyChange, g_connman.get(), state))
if (!wallet->CommitTransaction(*newTx, {} /* mapValue */, std::move(vOrderForm), {} /* fromAccount */, *keyChange, g_connman.get(), state))
return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(state.GetRejectReason()));

CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
ssTx << *newTx->tx;
ssTx << **newTx;
transaction_array.append(&(ssTx[0]), ssTx.size());
}

Expand Down
16 changes: 5 additions & 11 deletions src/qt/walletmodeltransaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,21 @@ WalletModelTransaction::WalletModelTransaction(const QList<SendCoinsRecipient> &
walletTransaction(0),
fee(0)
{
walletTransaction = new CWalletTx();
}

WalletModelTransaction::~WalletModelTransaction()
{
delete walletTransaction;
}

QList<SendCoinsRecipient> WalletModelTransaction::getRecipients() const
{
return recipients;
}

CWalletTx *WalletModelTransaction::getTransaction() const
CTransactionRef* WalletModelTransaction::getTransaction()
{
return walletTransaction;
return &walletTransaction;
}

unsigned int WalletModelTransaction::getTransactionSize()
{
return (!walletTransaction ? 0 : ::GetVirtualTransactionSize(*walletTransaction->tx));
return (!walletTransaction ? 0 : ::GetVirtualTransactionSize(*walletTransaction));
}

CAmount WalletModelTransaction::getTransactionFee() const
Expand Down Expand Up @@ -62,7 +56,7 @@ void WalletModelTransaction::reassignAmounts(int nChangePosRet)
if (out.amount() <= 0) continue;
if (i == nChangePosRet)
i++;
subtotal += walletTransaction->tx->vout[i].nValue;
subtotal += walletTransaction->vout[i].nValue;
i++;
}
rcp.amount = subtotal;
Expand All @@ -71,7 +65,7 @@ void WalletModelTransaction::reassignAmounts(int nChangePosRet)
{
if (i == nChangePosRet)
i++;
rcp.amount = walletTransaction->tx->vout[i].nValue;
rcp.amount = walletTransaction->vout[i].nValue;
i++;
}
}
Expand Down
5 changes: 2 additions & 3 deletions src/qt/walletmodeltransaction.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,10 @@ class WalletModelTransaction
{
public:
explicit WalletModelTransaction(const QList<SendCoinsRecipient> &recipients);
~WalletModelTransaction();

QList<SendCoinsRecipient> getRecipients() const;

CWalletTx *getTransaction() const;
CTransactionRef* getTransaction();
unsigned int getTransactionSize();

void setTransactionFee(const CAmount& newFee);
Expand All @@ -39,7 +38,7 @@ class WalletModelTransaction

private:
QList<SendCoinsRecipient> recipients;
CWalletTx *walletTransaction;
CTransactionRef walletTransaction;
std::unique_ptr<CReserveKey> keyChange;
CAmount fee;
};
Expand Down
17 changes: 7 additions & 10 deletions src/wallet/feebumper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,31 +255,28 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
return result;
}

CWalletTx wtxBumped(wallet, MakeTransactionRef(std::move(mtx)));
// commit/broadcast the tx
CTransactionRef tx = MakeTransactionRef(std::move(mtx));
mapValue_t mapValue = oldWtx.mapValue;
mapValue["replaces_txid"] = oldWtx.GetHash().ToString();

CReserveKey reservekey(wallet);
wtxBumped.mapValue = oldWtx.mapValue;
wtxBumped.mapValue["replaces_txid"] = oldWtx.GetHash().ToString();
wtxBumped.vOrderForm = oldWtx.vOrderForm;
wtxBumped.strFromAccount = oldWtx.strFromAccount;
wtxBumped.fTimeReceivedIsTxTime = true;
wtxBumped.fFromMe = true;
CValidationState state;
if (!wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, oldWtx.strFromAccount, reservekey, g_connman.get(), state)) {
// NOTE: CommitTransaction never returns false, so this should never happen.
errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
return Result::WALLET_ERROR;
}

bumped_txid = wtxBumped.GetHash();
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)));
}

// mark the original tx as bumped
if (!wallet->MarkReplaced(oldWtx.GetHash(), wtxBumped.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
// wtxBumped to the caller even if marking the original transaction
Expand Down
43 changes: 21 additions & 22 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ UniValue getaddressesbyaccount(const JSONRPCRequest& request)
return ret;
}

static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, CWalletTx& wtxNew, const CCoinControl& coin_control)
static CTransactionRef SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue, std::string fromAccount)
{
CAmount curBalance = pwallet->GetBalance();

Expand All @@ -406,16 +406,18 @@ static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CA
int nChangePosRet = -1;
CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount};
vecSend.push_back(recipient);
if (!pwallet->CreateTransaction(vecSend, wtxNew, reservekey, nFeeRequired, nChangePosRet, strError, coin_control)) {
CTransactionRef tx;
if (!pwallet->CreateTransaction(vecSend, tx, reservekey, nFeeRequired, nChangePosRet, strError, coin_control)) {
if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance)
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(wtxNew, reservekey, g_connman.get(), state)) {
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, std::move(fromAccount), reservekey, g_connman.get(), state)) {
strError = strprintf("Error: The transaction was rejected! Reason given: %s", state.GetRejectReason());
throw JSONRPCError(RPC_WALLET_ERROR, strError);
}
return tx;
}

UniValue sendtoaddress(const JSONRPCRequest& request)
Expand Down Expand Up @@ -474,11 +476,11 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");

// Wallet comments
CWalletTx wtx;
mapValue_t mapValue;
if (!request.params[2].isNull() && !request.params[2].get_str().empty())
wtx.mapValue["comment"] = request.params[2].get_str();
mapValue["comment"] = request.params[2].get_str();
if (!request.params[3].isNull() && !request.params[3].get_str().empty())
wtx.mapValue["to"] = request.params[3].get_str();
mapValue["to"] = request.params[3].get_str();

bool fSubtractFeeFromAmount = false;
if (!request.params[4].isNull()) {
Expand All @@ -503,9 +505,8 @@ UniValue sendtoaddress(const JSONRPCRequest& request)

EnsureWalletIsUnlocked(pwallet);

SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, wtx, coin_control);

return wtx.GetHash().GetHex();
CTransactionRef tx = SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, coin_control, std::move(mapValue), {} /* fromAccount */);
return tx->GetHash().GetHex();
}

UniValue listaddressgroupings(const JSONRPCRequest& request)
Expand Down Expand Up @@ -969,12 +970,11 @@ UniValue sendfrom(const JSONRPCRequest& request)
if (!request.params[3].isNull())
nMinDepth = request.params[3].get_int();

CWalletTx wtx;
wtx.strFromAccount = strAccount;
mapValue_t mapValue;
if (!request.params[4].isNull() && !request.params[4].get_str().empty())
wtx.mapValue["comment"] = request.params[4].get_str();
mapValue["comment"] = request.params[4].get_str();
if (!request.params[5].isNull() && !request.params[5].get_str().empty())
wtx.mapValue["to"] = request.params[5].get_str();
mapValue["to"] = request.params[5].get_str();

EnsureWalletIsUnlocked(pwallet);

Expand All @@ -984,9 +984,8 @@ UniValue sendfrom(const JSONRPCRequest& request)
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds");

CCoinControl no_coin_control; // This is a deprecated API
SendMoney(pwallet, dest, nAmount, false, wtx, no_coin_control);

return wtx.GetHash().GetHex();
CTransactionRef tx = SendMoney(pwallet, dest, nAmount, false, no_coin_control, std::move(mapValue), std::move(strAccount));
return tx->GetHash().GetHex();
}


Expand Down Expand Up @@ -1057,10 +1056,9 @@ UniValue sendmany(const JSONRPCRequest& request)
if (!request.params[2].isNull())
nMinDepth = request.params[2].get_int();

CWalletTx wtx;
wtx.strFromAccount = strAccount;
mapValue_t mapValue;
if (!request.params[3].isNull() && !request.params[3].get_str().empty())
wtx.mapValue["comment"] = request.params[3].get_str();
mapValue["comment"] = request.params[3].get_str();

UniValue subtractFeeFromAmount(UniValue::VARR);
if (!request.params[4].isNull())
Expand Down Expand Up @@ -1126,16 +1124,17 @@ UniValue sendmany(const JSONRPCRequest& request)
CAmount nFeeRequired = 0;
int nChangePosRet = -1;
std::string strFailReason;
bool fCreated = pwallet->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control);
CTransactionRef tx;
bool fCreated = pwallet->CreateTransaction(vecSend, tx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control);
if (!fCreated)
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason);
CValidationState state;
if (!pwallet->CommitTransaction(wtx, keyChange, g_connman.get(), state)) {
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, std::move(strAccount), keyChange, g_connman.get(), state)) {
strFailReason = strprintf("Transaction commit failed:: %s", state.GetRejectReason());
throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
}

return wtx.GetHash().GetHex();
return tx->GetHash().GetHex();
}

// Defined in rpc/misc.cpp
Expand Down
8 changes: 4 additions & 4 deletions src/wallet/test/wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -618,17 +618,17 @@ class ListCoinsTestingSetup : public TestChain100Setup

CWalletTx& AddTx(CRecipient recipient)
{
CWalletTx wtx;
CTransactionRef tx;
CReserveKey reservekey(wallet.get());
CAmount fee;
int changePos = -1;
std::string error;
CCoinControl dummy;
BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy));
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, reservekey, fee, changePos, error, dummy));
CValidationState state;
BOOST_CHECK(wallet->CommitTransaction(wtx, reservekey, nullptr, state));
BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, {}, reservekey, nullptr, state));
LOCK(wallet->cs_wallet);
auto it = wallet->mapWallet.find(wtx.GetHash());
auto it = wallet->mapWallet.find(tx->GetHash());
BOOST_CHECK(it != wallet->mapWallet.end());
CreateAndProcessBlock({CMutableTransaction(*it->second.tx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
it->second.SetMerkleBranch(chainActive.Tip(), 1);
Expand Down
33 changes: 19 additions & 14 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2606,24 +2606,24 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
coinControl.Select(txin.prevout);

CReserveKey reservekey(this);
CWalletTx wtx;
if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
CTransactionRef tx_new;
if (!CreateTransaction(vecSend, tx_new, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
return false;
}

if (nChangePosInOut != -1) {
tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.tx->vout[nChangePosInOut]);
tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]);
// we don't have the normal Create/Commit cycle, and don't want to risk reusing change,
// so just remove the key from the keypool here.
reservekey.KeepKey();
}

// Copy output sizes from new transaction; they may have had the fee subtracted from them
for (unsigned int idx = 0; idx < tx.vout.size(); idx++)
tx.vout[idx].nValue = wtx.tx->vout[idx].nValue;
tx.vout[idx].nValue = tx_new->vout[idx].nValue;

// Add new txins (keeping original txin scriptSig/order)
for (const CTxIn& txin : wtx.tx->vin)
for (const CTxIn& txin : tx_new->vin)
{
if (!coinControl.IsSelected(txin.prevout))
{
Expand All @@ -2641,7 +2641,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
return true;
}

bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet,
bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CReserveKey& reservekey, CAmount& nFeeRet,
int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
{
CAmount nValue = 0;
Expand All @@ -2665,8 +2665,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
return false;
}

wtxNew.fTimeReceivedIsTxTime = true;
wtxNew.BindWallet(this);
CMutableTransaction txNew;

// Discourage fee sniping.
Expand Down Expand Up @@ -2751,7 +2749,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
nChangePosInOut = nChangePosRequest;
txNew.vin.clear();
txNew.vout.clear();
wtxNew.fFromMe = true;
bool fFirst = true;

CAmount nValueToSelect = nValue;
Expand Down Expand Up @@ -2961,11 +2958,11 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
}

// Embed the constructed transaction data in wtxNew.
wtxNew.SetTx(MakeTransactionRef(std::move(txNew)));
// Return the constructed transaction data.
tx = MakeTransactionRef(std::move(txNew));

// Limit size
if (GetTransactionWeight(*wtxNew.tx) >= MAX_STANDARD_TX_WEIGHT)
if (GetTransactionWeight(*tx) >= MAX_STANDARD_TX_WEIGHT)
{
strFailReason = _("Transaction too large");
return false;
Expand All @@ -2975,7 +2972,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
// Lastly, ensure this tx will pass the mempool's chain limits
LockPoints lp;
CTxMemPoolEntry entry(wtxNew.tx, 0, 0, 0, false, 0, lp);
CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp);
CTxMemPool::setEntries setAncestors;
size_t nLimitAncestors = gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
size_t nLimitAncestorSize = gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000;
Expand All @@ -3002,10 +2999,18 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
/**
* Call after CreateTransaction unless you want to abort
*/
bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey, CConnman* connman, CValidationState& state)
bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm, std::string fromAccount, CReserveKey& reservekey, CConnman* connman, CValidationState& state)
{
{
LOCK2(cs_main, cs_wallet);

CWalletTx wtxNew(this, std::move(tx));
wtxNew.mapValue = std::move(mapValue);
wtxNew.vOrderForm = std::move(orderForm);
wtxNew.strFromAccount = std::move(fromAccount);
wtxNew.fTimeReceivedIsTxTime = true;
wtxNew.fFromMe = true;

LogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString());
{
// Take key pair from key pool so it won't be used again
Expand Down
Loading

0 comments on commit 0fdca91

Please sign in to comment.