Skip to content

Commit 6acd870

Browse files
sipasipa
committed
Merge #9680: Unify CWalletTx construction
b4bc32a [wallet] Get rid of CWalletTx default constructor (Russell Yanofsky) a128bdc [wallet] Construct CWalletTx objects in CommitTransaction (Russell Yanofsky) Pull request description: Two commits: - `Construct CWalletTx objects in CommitTransaction` moves a bunch of CWalletTx initialization into CWallet::CommitTransaction to dedup some code and avoid future inconsistencies in how wallet transactions are created. - `Get rid of CWalletTx default constructor` does what is described and eliminates the possibility of empty transaction entries being inadvertently created by mapWallet[hash] accesses. Both of these changes were originally part of #9381 Tree-SHA512: af3841c4f0539e0662d81b33c5369fc70aa06ddde1c59cb00fb21c9e4c7d9ff47f1edc5040cb463af1333838802c56b3ef875b939e2b804ee45b8e0294a4371c
2 parents 1846296 + b4bc32a commit 6acd870

File tree

10 files changed

+80
-91
lines changed

10 files changed

+80
-91
lines changed

‎src/qt/walletmodel.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -275,9 +275,9 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
275275
int nChangePosRet = -1;
276276
std::string strFailReason;
277277

278-
CWalletTx *newTx = transaction.getTransaction();
278+
CTransactionRef& newTx = transaction.getTransaction();
279279
CReserveKey *keyChange = transaction.getPossibleKeyChange();
280-
bool fCreated = wallet->CreateTransaction(vecSend, *newTx, *keyChange, nFeeRequired, nChangePosRet, strFailReason, coinControl);
280+
bool fCreated = wallet->CreateTransaction(vecSend, newTx, *keyChange, nFeeRequired, nChangePosRet, strFailReason, coinControl);
281281
transaction.setTransactionFee(nFeeRequired);
282282
if (fSubtractFeeFromAmount && fCreated)
283283
transaction.reassignAmounts(nChangePosRet);
@@ -309,8 +309,8 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran
309309

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

313+
std::vector<std::pair<std::string, std::string>> vOrderForm;
314314
for (const SendCoinsRecipient &rcp : transaction.getRecipients())
315315
{
316316
if (rcp.paymentRequest.IsInitialized())
@@ -321,22 +321,22 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran
321321
}
322322

323323
// Store PaymentRequests in wtx.vOrderForm in wallet.
324-
std::string key("PaymentRequest");
325324
std::string value;
326325
rcp.paymentRequest.SerializeToString(&value);
327-
newTx->vOrderForm.push_back(make_pair(key, value));
326+
vOrderForm.emplace_back("PaymentRequest", std::move(value));
328327
}
329328
else if (!rcp.message.isEmpty()) // Message from normal bitcoin:URI (bitcoin:123...?message=example)
330-
newTx->vOrderForm.push_back(make_pair("Message", rcp.message.toStdString()));
329+
vOrderForm.emplace_back("Message", rcp.message.toStdString());
331330
}
332331

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

338338
CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
339-
ssTx << *newTx->tx;
339+
ssTx << newTx;
340340
transaction_array.append(&(ssTx[0]), ssTx.size());
341341
}
342342

‎src/qt/walletmodeltransaction.cpp

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,27 +12,21 @@ WalletModelTransaction::WalletModelTransaction(const QList<SendCoinsRecipient> &
1212
walletTransaction(0),
1313
fee(0)
1414
{
15-
walletTransaction = new CWalletTx();
16-
}
17-
18-
WalletModelTransaction::~WalletModelTransaction()
19-
{
20-
delete walletTransaction;
2115
}
2216

2317
QList<SendCoinsRecipient> WalletModelTransaction::getRecipients() const
2418
{
2519
return recipients;
2620
}
2721

28-
CWalletTx *WalletModelTransaction::getTransaction() const
22+
CTransactionRef& WalletModelTransaction::getTransaction()
2923
{
3024
return walletTransaction;
3125
}
3226

3327
unsigned int WalletModelTransaction::getTransactionSize()
3428
{
35-
return (!walletTransaction ? 0 : ::GetVirtualTransactionSize(*walletTransaction->tx));
29+
return (!walletTransaction ? 0 : ::GetVirtualTransactionSize(*walletTransaction));
3630
}
3731

3832
CAmount WalletModelTransaction::getTransactionFee() const
@@ -62,7 +56,7 @@ void WalletModelTransaction::reassignAmounts(int nChangePosRet)
6256
if (out.amount() <= 0) continue;
6357
if (i == nChangePosRet)
6458
i++;
65-
subtotal += walletTransaction->tx->vout[i].nValue;
59+
subtotal += walletTransaction->vout[i].nValue;
6660
i++;
6761
}
6862
rcp.amount = subtotal;
@@ -71,7 +65,7 @@ void WalletModelTransaction::reassignAmounts(int nChangePosRet)
7165
{
7266
if (i == nChangePosRet)
7367
i++;
74-
rcp.amount = walletTransaction->tx->vout[i].nValue;
68+
rcp.amount = walletTransaction->vout[i].nValue;
7569
i++;
7670
}
7771
}

‎src/qt/walletmodeltransaction.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,10 @@ class WalletModelTransaction
2020
{
2121
public:
2222
explicit WalletModelTransaction(const QList<SendCoinsRecipient> &recipients);
23-
~WalletModelTransaction();
2423

2524
QList<SendCoinsRecipient> getRecipients() const;
2625

27-
CWalletTx *getTransaction() const;
26+
CTransactionRef& getTransaction();
2827
unsigned int getTransactionSize();
2928

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

4039
private:
4140
QList<SendCoinsRecipient> recipients;
42-
CWalletTx *walletTransaction;
41+
CTransactionRef walletTransaction;
4342
std::unique_ptr<CReserveKey> keyChange;
4443
CAmount fee;
4544
};

‎src/wallet/feebumper.cpp

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -262,31 +262,28 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
262262
return result;
263263
}
264264

265-
CWalletTx wtxBumped(wallet, MakeTransactionRef(std::move(mtx)));
266265
// commit/broadcast the tx
266+
CTransactionRef tx = MakeTransactionRef(std::move(mtx));
267+
mapValue_t mapValue = oldWtx.mapValue;
268+
mapValue["replaces_txid"] = oldWtx.GetHash().ToString();
269+
267270
CReserveKey reservekey(wallet);
268-
wtxBumped.mapValue = oldWtx.mapValue;
269-
wtxBumped.mapValue["replaces_txid"] = oldWtx.GetHash().ToString();
270-
wtxBumped.vOrderForm = oldWtx.vOrderForm;
271-
wtxBumped.strFromAccount = oldWtx.strFromAccount;
272-
wtxBumped.fTimeReceivedIsTxTime = true;
273-
wtxBumped.fFromMe = true;
274271
CValidationState state;
275-
if (!wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
272+
if (!wallet->CommitTransaction(tx, std::move(mapValue), oldWtx.vOrderForm, oldWtx.strFromAccount, reservekey, g_connman.get(), state)) {
276273
// NOTE: CommitTransaction never returns false, so this should never happen.
277274
errors.push_back(strprintf("The transaction was rejected: %s", FormatStateMessage(state)));
278275
return Result::WALLET_ERROR;
279276
}
280277

281-
bumped_txid = wtxBumped.GetHash();
278+
bumped_txid = tx->GetHash();
282279
if (state.IsInvalid()) {
283280
// This can happen if the mempool rejected the transaction. Report
284281
// what happened in the "errors" response.
285282
errors.push_back(strprintf("Error: The transaction was rejected: %s", FormatStateMessage(state)));
286283
}
287284

288285
// mark the original tx as bumped
289-
if (!wallet->MarkReplaced(oldWtx.GetHash(), wtxBumped.GetHash())) {
286+
if (!wallet->MarkReplaced(oldWtx.GetHash(), bumped_txid)) {
290287
// TODO: see if JSON-RPC has a standard way of returning a response
291288
// along with an exception. It would be good to return information about
292289
// wtxBumped to the caller even if marking the original transaction

‎src/wallet/rpcwallet.cpp

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ UniValue getaddressesbyaccount(const JSONRPCRequest& request)
404404
return ret;
405405
}
406406

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

@@ -430,16 +430,18 @@ static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CA
430430
int nChangePosRet = -1;
431431
CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount};
432432
vecSend.push_back(recipient);
433-
if (!pwallet->CreateTransaction(vecSend, wtxNew, reservekey, nFeeRequired, nChangePosRet, strError, coin_control)) {
433+
CTransactionRef tx;
434+
if (!pwallet->CreateTransaction(vecSend, tx, reservekey, nFeeRequired, nChangePosRet, strError, coin_control)) {
434435
if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance)
435436
strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired));
436437
throw JSONRPCError(RPC_WALLET_ERROR, strError);
437438
}
438439
CValidationState state;
439-
if (!pwallet->CommitTransaction(wtxNew, reservekey, g_connman.get(), state)) {
440+
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, std::move(fromAccount), reservekey, g_connman.get(), state)) {
440441
strError = strprintf("Error: The transaction was rejected! Reason given: %s", FormatStateMessage(state));
441442
throw JSONRPCError(RPC_WALLET_ERROR, strError);
442443
}
444+
return tx;
443445
}
444446

445447
UniValue sendtoaddress(const JSONRPCRequest& request)
@@ -498,11 +500,11 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
498500
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
499501

500502
// Wallet comments
501-
CWalletTx wtx;
503+
mapValue_t mapValue;
502504
if (!request.params[2].isNull() && !request.params[2].get_str().empty())
503-
wtx.mapValue["comment"] = request.params[2].get_str();
505+
mapValue["comment"] = request.params[2].get_str();
504506
if (!request.params[3].isNull() && !request.params[3].get_str().empty())
505-
wtx.mapValue["to"] = request.params[3].get_str();
507+
mapValue["to"] = request.params[3].get_str();
506508

507509
bool fSubtractFeeFromAmount = false;
508510
if (!request.params[4].isNull()) {
@@ -527,9 +529,8 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
527529

528530
EnsureWalletIsUnlocked(pwallet);
529531

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

535536
UniValue listaddressgroupings(const JSONRPCRequest& request)
@@ -995,12 +996,11 @@ UniValue sendfrom(const JSONRPCRequest& request)
995996
if (!request.params[3].isNull())
996997
nMinDepth = request.params[3].get_int();
997998

998-
CWalletTx wtx;
999-
wtx.strFromAccount = strAccount;
999+
mapValue_t mapValue;
10001000
if (!request.params[4].isNull() && !request.params[4].get_str().empty())
1001-
wtx.mapValue["comment"] = request.params[4].get_str();
1001+
mapValue["comment"] = request.params[4].get_str();
10021002
if (!request.params[5].isNull() && !request.params[5].get_str().empty())
1003-
wtx.mapValue["to"] = request.params[5].get_str();
1003+
mapValue["to"] = request.params[5].get_str();
10041004

10051005
EnsureWalletIsUnlocked(pwallet);
10061006

@@ -1010,9 +1010,8 @@ UniValue sendfrom(const JSONRPCRequest& request)
10101010
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds");
10111011

10121012
CCoinControl no_coin_control; // This is a deprecated API
1013-
SendMoney(pwallet, dest, nAmount, false, wtx, no_coin_control);
1014-
1015-
return wtx.GetHash().GetHex();
1013+
CTransactionRef tx = SendMoney(pwallet, dest, nAmount, false, no_coin_control, std::move(mapValue), std::move(strAccount));
1014+
return tx->GetHash().GetHex();
10161015
}
10171016

10181017

@@ -1083,10 +1082,9 @@ UniValue sendmany(const JSONRPCRequest& request)
10831082
if (!request.params[2].isNull())
10841083
nMinDepth = request.params[2].get_int();
10851084

1086-
CWalletTx wtx;
1087-
wtx.strFromAccount = strAccount;
1085+
mapValue_t mapValue;
10881086
if (!request.params[3].isNull() && !request.params[3].get_str().empty())
1089-
wtx.mapValue["comment"] = request.params[3].get_str();
1087+
mapValue["comment"] = request.params[3].get_str();
10901088

10911089
UniValue subtractFeeFromAmount(UniValue::VARR);
10921090
if (!request.params[4].isNull())
@@ -1152,16 +1150,17 @@ UniValue sendmany(const JSONRPCRequest& request)
11521150
CAmount nFeeRequired = 0;
11531151
int nChangePosRet = -1;
11541152
std::string strFailReason;
1155-
bool fCreated = pwallet->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control);
1153+
CTransactionRef tx;
1154+
bool fCreated = pwallet->CreateTransaction(vecSend, tx, keyChange, nFeeRequired, nChangePosRet, strFailReason, coin_control);
11561155
if (!fCreated)
11571156
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason);
11581157
CValidationState state;
1159-
if (!pwallet->CommitTransaction(wtx, keyChange, g_connman.get(), state)) {
1158+
if (!pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */, std::move(strAccount), keyChange, g_connman.get(), state)) {
11601159
strFailReason = strprintf("Transaction commit failed:: %s", FormatStateMessage(state));
11611160
throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
11621161
}
11631162

1164-
return wtx.GetHash().GetHex();
1163+
return tx->GetHash().GetHex();
11651164
}
11661165

11671166
UniValue addmultisigaddress(const JSONRPCRequest& request)

‎src/wallet/test/accounting_tests.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ GetResults(CWallet& wallet, std::map<CAmount, CAccountingEntry>& results)
2929
BOOST_AUTO_TEST_CASE(acc_orderupgrade)
3030
{
3131
std::vector<CWalletTx*> vpwtx;
32-
CWalletTx wtx;
32+
CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef());
3333
CAccountingEntry ae;
3434
std::map<CAmount, CAccountingEntry> results;
3535

@@ -44,7 +44,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
4444

4545
wtx.mapValue["comment"] = "z";
4646
m_wallet.AddToWallet(wtx);
47-
vpwtx.push_back(&m_wallet.mapWallet[wtx.GetHash()]);
47+
vpwtx.push_back(&m_wallet.mapWallet.at(wtx.GetHash()));
4848
vpwtx[0]->nTimeReceived = (unsigned int)1333333335;
4949
vpwtx[0]->nOrderPos = -1;
5050

@@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
8686
wtx.SetTx(MakeTransactionRef(std::move(tx)));
8787
}
8888
m_wallet.AddToWallet(wtx);
89-
vpwtx.push_back(&m_wallet.mapWallet[wtx.GetHash()]);
89+
vpwtx.push_back(&m_wallet.mapWallet.at(wtx.GetHash()));
9090
vpwtx[1]->nTimeReceived = (unsigned int)1333333336;
9191

9292
wtx.mapValue["comment"] = "x";
@@ -96,7 +96,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
9696
wtx.SetTx(MakeTransactionRef(std::move(tx)));
9797
}
9898
m_wallet.AddToWallet(wtx);
99-
vpwtx.push_back(&m_wallet.mapWallet[wtx.GetHash()]);
99+
vpwtx.push_back(&m_wallet.mapWallet.at(wtx.GetHash()));
100100
vpwtx[2]->nTimeReceived = (unsigned int)1333333329;
101101
vpwtx[2]->nOrderPos = -1;
102102

‎src/wallet/test/wallet_tests.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -627,23 +627,23 @@ class ListCoinsTestingSetup : public TestChain100Setup
627627

628628
CWalletTx& AddTx(CRecipient recipient)
629629
{
630-
CWalletTx wtx;
630+
CTransactionRef tx;
631631
CReserveKey reservekey(wallet.get());
632632
CAmount fee;
633633
int changePos = -1;
634634
std::string error;
635635
CCoinControl dummy;
636-
BOOST_CHECK(wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy));
636+
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, reservekey, fee, changePos, error, dummy));
637637
CValidationState state;
638-
BOOST_CHECK(wallet->CommitTransaction(wtx, reservekey, nullptr, state));
638+
BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, {}, reservekey, nullptr, state));
639639
CMutableTransaction blocktx;
640640
{
641641
LOCK(wallet->cs_wallet);
642-
blocktx = CMutableTransaction(*wallet->mapWallet.at(wtx.GetHash()).tx);
642+
blocktx = CMutableTransaction(*wallet->mapWallet.at(tx->GetHash()).tx);
643643
}
644644
CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
645645
LOCK(wallet->cs_wallet);
646-
auto it = wallet->mapWallet.find(wtx.GetHash());
646+
auto it = wallet->mapWallet.find(tx->GetHash());
647647
BOOST_CHECK(it != wallet->mapWallet.end());
648648
it->second.SetMerkleBranch(chainActive.Tip(), 1);
649649
return it->second;

0 commit comments

Comments
 (0)