Skip to content

Commit ed457a3

Browse files
ryanofskyryanofsky
committed
Remove CWalletTx merging logic from AddToWallet
Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly. This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them. This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged. This is a pure refactoring, no behavior is changing.
1 parent f646275 commit ed457a3

File tree

8 files changed

+167
-127
lines changed

8 files changed

+167
-127
lines changed

‎src/qt/walletmodel.cpp‎

Lines changed: 14 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
Q_FOREACH(const SendCoinsRecipient &rcp, transaction.getRecipients())
315315
{
316316
if (rcp.paymentRequest.IsInitialized())
@@ -321,22 +321,28 @@ 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

333332
CReserveKey *keyChange = transaction.getPossibleKeyChange();
334333
CValidationState state;
335-
if(!wallet->CommitTransaction(*newTx, *keyChange, g_connman.get(), state))
334+
auto updateEntry = [&](TxEntry& entry, bool fNew) {
335+
assert(fNew);
336+
entry.second.fFromMe = true;
337+
entry.second.fTimeReceivedIsTxTime = true;
338+
entry.second.vOrderForm = std::move(vOrderForm);
339+
return true;
340+
};
341+
if (!wallet->CommitTransaction(transaction.getTransaction(), updateEntry, *keyChange, g_connman.get(), state))
336342
return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(state.GetRejectReason()));
337343

338344
CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
339-
ssTx << *newTx->tx;
345+
ssTx << *transaction.getTransaction();
340346
transaction_array.append(&(ssTx[0]), ssTx.size());
341347
}
342348

‎src/qt/walletmodeltransaction.cpp‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,19 @@ WalletModelTransaction::WalletModelTransaction(const QList<SendCoinsRecipient> &
1313
keyChange(0),
1414
fee(0)
1515
{
16-
walletTransaction = new CWalletTx();
1716
}
1817

1918
WalletModelTransaction::~WalletModelTransaction()
2019
{
2120
delete keyChange;
22-
delete walletTransaction;
2321
}
2422

2523
QList<SendCoinsRecipient> WalletModelTransaction::getRecipients()
2624
{
2725
return recipients;
2826
}
2927

30-
CWalletTx *WalletModelTransaction::getTransaction()
28+
CTransactionRef& WalletModelTransaction::getTransaction()
3129
{
3230
return walletTransaction;
3331
}
@@ -64,7 +62,7 @@ void WalletModelTransaction::reassignAmounts(int nChangePosRet)
6462
if (out.amount() <= 0) continue;
6563
if (i == nChangePosRet)
6664
i++;
67-
subtotal += walletTransaction->tx->vout[i].nValue;
65+
subtotal += walletTransaction->vout[i].nValue;
6866
i++;
6967
}
7068
rcp.amount = subtotal;
@@ -73,7 +71,7 @@ void WalletModelTransaction::reassignAmounts(int nChangePosRet)
7371
{
7472
if (i == nChangePosRet)
7573
i++;
76-
rcp.amount = walletTransaction->tx->vout[i].nValue;
74+
rcp.amount = walletTransaction->vout[i].nValue;
7775
i++;
7876
}
7977
}

‎src/qt/walletmodeltransaction.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ class WalletModelTransaction
2424

2525
QList<SendCoinsRecipient> getRecipients();
2626

27-
CWalletTx *getTransaction();
27+
CTransactionRef& getTransaction();
2828
unsigned int getTransactionSize();
2929

3030
void setTransactionFee(const CAmount& newFee);
@@ -39,7 +39,7 @@ class WalletModelTransaction
3939

4040
private:
4141
QList<SendCoinsRecipient> recipients;
42-
CWalletTx *walletTransaction;
42+
CTransactionRef walletTransaction;
4343
CReserveKey *keyChange;
4444
CAmount fee;
4545
};

‎src/wallet/rpcdump.cpp‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,6 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
271271
if (!DecodeHexTx(tx, request.params[0].get_str()))
272272
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
273273
uint256 hashTx = tx.GetHash();
274-
CWalletTx wtx(pwalletMain, MakeTransactionRef(std::move(tx)));
275274

276275
CDataStream ssMB(ParseHexV(request.params[1], "proof"), SER_NETWORK, PROTOCOL_VERSION);
277276
CMerkleBlock merkleBlock;
@@ -299,13 +298,28 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
299298
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock");
300299
}
301300

302-
wtx.nIndex = txnIndex;
303-
wtx.hashBlock = merkleBlock.header.GetHash();
301+
int nIndex = txnIndex;
302+
uint256 hashBlock = merkleBlock.header.GetHash();
304303

305304
LOCK2(cs_main, pwalletMain->cs_wallet);
306305

307-
if (pwalletMain->IsMine(wtx)) {
308-
pwalletMain->AddToWallet(wtx, false);
306+
if (pwalletMain->IsMine(tx)) {
307+
auto updateEntry = [&](TxEntry& entry, bool fNew) {
308+
bool fWriteTx = fNew;
309+
310+
// Update transaction block position if it has changed.
311+
if (entry.second.nIndex != nIndex || entry.second.hashBlock != hashBlock) {
312+
entry.second.nIndex = nIndex;
313+
entry.second.hashBlock = hashBlock;
314+
fWriteTx = true;
315+
}
316+
317+
// Break debit/credit balance caches.
318+
entry.second.MarkDirty();
319+
320+
return fWriteTx;
321+
};
322+
pwalletMain->AddToWallet(MakeTransactionRef(std::move(tx)), updateEntry, false);
309323
return NullUniValue;
310324
}
311325

‎src/wallet/rpcwallet.cpp‎

Lines changed: 50 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ UniValue getaddressesbyaccount(const JSONRPCRequest& request)
336336
return ret;
337337
}
338338

339-
static void SendMoney(const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, CWalletTx& wtxNew)
339+
static void SendMoney(const CTxDestination& address, CAmount nValue, bool fSubtractFeeFromAmount, const string& comment, const string& to, const string& strFromAccount, CTransactionRef& txNew)
340340
{
341341
CAmount curBalance = pwalletMain->GetBalance();
342342

@@ -361,13 +361,26 @@ static void SendMoney(const CTxDestination &address, CAmount nValue, bool fSubtr
361361
int nChangePosRet = -1;
362362
CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount};
363363
vecSend.push_back(recipient);
364-
if (!pwalletMain->CreateTransaction(vecSend, wtxNew, reservekey, nFeeRequired, nChangePosRet, strError)) {
364+
if (!pwalletMain->CreateTransaction(vecSend, txNew, reservekey, nFeeRequired, nChangePosRet, strError)) {
365365
if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance)
366366
strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired));
367367
throw JSONRPCError(RPC_WALLET_ERROR, strError);
368368
}
369+
370+
auto updateEntry = [&](TxEntry& entry, bool fNew) {
371+
assert(fNew);
372+
entry.second.fFromMe = true;
373+
entry.second.fTimeReceivedIsTxTime = true;
374+
entry.second.strFromAccount = strFromAccount;
375+
if (!comment.empty())
376+
entry.second.mapValue["comment"] = comment;
377+
if (!to.empty())
378+
entry.second.mapValue["to"] = to;
379+
return true;
380+
};
381+
369382
CValidationState state;
370-
if (!pwalletMain->CommitTransaction(wtxNew, reservekey, g_connman.get(), state)) {
383+
if (!pwalletMain->CommitTransaction(txNew, updateEntry, reservekey, g_connman.get(), state)) {
371384
strError = strprintf("Error: The transaction was rejected! Reason given: %s", state.GetRejectReason());
372385
throw JSONRPCError(RPC_WALLET_ERROR, strError);
373386
}
@@ -414,21 +427,23 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
414427
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
415428

416429
// Wallet comments
417-
CWalletTx wtx;
418-
if (request.params.size() > 2 && !request.params[2].isNull() && !request.params[2].get_str().empty())
419-
wtx.mapValue["comment"] = request.params[2].get_str();
420-
if (request.params.size() > 3 && !request.params[3].isNull() && !request.params[3].get_str().empty())
421-
wtx.mapValue["to"] = request.params[3].get_str();
430+
string comment;
431+
if (request.params.size() > 2 && !request.params[2].isNull())
432+
comment = request.params[2].get_str();
433+
string to;
434+
if (request.params.size() > 3 && !request.params[3].isNull())
435+
to = request.params[3].get_str();
422436

423437
bool fSubtractFeeFromAmount = false;
424438
if (request.params.size() > 4)
425439
fSubtractFeeFromAmount = request.params[4].get_bool();
426440

427441
EnsureWalletIsUnlocked();
428442

429-
SendMoney(address.Get(), nAmount, fSubtractFeeFromAmount, wtx);
443+
CTransactionRef txNew;
444+
SendMoney(address.Get(), nAmount, fSubtractFeeFromAmount, comment, to, {}, txNew);
430445

431-
return wtx.GetHash().GetHex();
446+
return txNew->GetHash().GetHex();
432447
}
433448

434449
UniValue listaddressgroupings(const JSONRPCRequest& request)
@@ -835,12 +850,12 @@ UniValue sendfrom(const JSONRPCRequest& request)
835850
if (request.params.size() > 3)
836851
nMinDepth = request.params[3].get_int();
837852

838-
CWalletTx wtx;
839-
wtx.strFromAccount = strAccount;
840-
if (request.params.size() > 4 && !request.params[4].isNull() && !request.params[4].get_str().empty())
841-
wtx.mapValue["comment"] = request.params[4].get_str();
842-
if (request.params.size() > 5 && !request.params[5].isNull() && !request.params[5].get_str().empty())
843-
wtx.mapValue["to"] = request.params[5].get_str();
853+
string comment;
854+
if (request.params.size() > 4 && !request.params[4].isNull())
855+
comment = request.params[4].get_str();
856+
string to;
857+
if (request.params.size() > 5 && !request.params[5].isNull())
858+
to = request.params[5].get_str();
844859

845860
EnsureWalletIsUnlocked();
846861

@@ -849,9 +864,10 @@ UniValue sendfrom(const JSONRPCRequest& request)
849864
if (nAmount > nBalance)
850865
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Account has insufficient funds");
851866

852-
SendMoney(address.Get(), nAmount, false, wtx);
867+
CTransactionRef txNew;
868+
SendMoney(address.Get(), nAmount, false, comment, to, strAccount, txNew);
853869

854-
return wtx.GetHash().GetHex();
870+
return txNew->GetHash().GetHex();
855871
}
856872

857873

@@ -907,10 +923,9 @@ UniValue sendmany(const JSONRPCRequest& request)
907923
if (request.params.size() > 2)
908924
nMinDepth = request.params[2].get_int();
909925

910-
CWalletTx wtx;
911-
wtx.strFromAccount = strAccount;
912-
if (request.params.size() > 3 && !request.params[3].isNull() && !request.params[3].get_str().empty())
913-
wtx.mapValue["comment"] = request.params[3].get_str();
926+
string comment;
927+
if (request.params.size() > 3 && !request.params[3].isNull())
928+
comment = request.params[3].get_str();
914929

915930
UniValue subtractFeeFromAmount(UniValue::VARR);
916931
if (request.params.size() > 4)
@@ -960,16 +975,26 @@ UniValue sendmany(const JSONRPCRequest& request)
960975
CAmount nFeeRequired = 0;
961976
int nChangePosRet = -1;
962977
string strFailReason;
963-
bool fCreated = pwalletMain->CreateTransaction(vecSend, wtx, keyChange, nFeeRequired, nChangePosRet, strFailReason);
978+
CTransactionRef txNew;
979+
bool fCreated = pwalletMain->CreateTransaction(vecSend, txNew, keyChange, nFeeRequired, nChangePosRet, strFailReason);
964980
if (!fCreated)
965981
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason);
966982
CValidationState state;
967-
if (!pwalletMain->CommitTransaction(wtx, keyChange, g_connman.get(), state)) {
983+
auto updateEntry = [&](TxEntry& entry, bool fNew) {
984+
assert(fNew);
985+
entry.second.fFromMe = true;
986+
entry.second.fTimeReceivedIsTxTime = true;
987+
entry.second.strFromAccount = strAccount;
988+
if (!comment.empty())
989+
entry.second.mapValue["comment"] = comment;
990+
return true;
991+
};
992+
if (!pwalletMain->CommitTransaction(txNew, updateEntry, keyChange, g_connman.get(), state)) {
968993
strFailReason = strprintf("Transaction commit failed:: %s", state.GetRejectReason());
969994
throw JSONRPCError(RPC_WALLET_ERROR, strFailReason);
970995
}
971996

972-
return wtx.GetHash().GetHex();
997+
return txNew->GetHash().GetHex();
973998
}
974999

9751000
// Defined in rpc/misc.cpp

‎src/wallet/test/accounting_tests.cpp‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ GetResults(std::map<CAmount, CAccountingEntry>& results)
3232
BOOST_AUTO_TEST_CASE(acc_orderupgrade)
3333
{
3434
std::vector<CWalletTx*> vpwtx;
35-
CWalletTx wtx;
3635
CAccountingEntry ae;
3736
std::map<CAmount, CAccountingEntry> results;
3837

@@ -45,9 +44,12 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
4544
ae.strComment = "";
4645
pwalletMain->AddAccountingEntry(ae);
4746

48-
wtx.mapValue["comment"] = "z";
49-
pwalletMain->AddToWallet(wtx);
50-
vpwtx.push_back(&pwalletMain->mapWallet[wtx.GetHash()]);
47+
CTransactionRef txNew = MakeTransactionRef();
48+
pwalletMain->AddToWallet(txNew, [&](TxEntry& entry, bool fNew) {
49+
entry.second.mapValue["comment"] = "z";
50+
vpwtx.push_back(&entry.second);
51+
return true;
52+
});
5153
vpwtx[0]->nTimeReceived = (unsigned int)1333333335;
5254
vpwtx[0]->nOrderPos = -1;
5355

@@ -82,24 +84,28 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
8284
BOOST_CHECK(results[3].strComment.empty());
8385

8486

85-
wtx.mapValue["comment"] = "y";
8687
{
87-
CMutableTransaction tx(wtx);
88+
CMutableTransaction tx(*txNew);
8889
--tx.nLockTime; // Just to change the hash :)
89-
wtx.SetTx(MakeTransactionRef(std::move(tx)));
90+
txNew = MakeTransactionRef(std::move(tx));
9091
}
91-
pwalletMain->AddToWallet(wtx);
92-
vpwtx.push_back(&pwalletMain->mapWallet[wtx.GetHash()]);
92+
pwalletMain->AddToWallet(txNew, [&](TxEntry& entry, bool fNew) {
93+
entry.second.mapValue["comment"] = "y";
94+
vpwtx.push_back(&entry.second);
95+
return true;
96+
});
9397
vpwtx[1]->nTimeReceived = (unsigned int)1333333336;
9498

95-
wtx.mapValue["comment"] = "x";
9699
{
97-
CMutableTransaction tx(wtx);
100+
CMutableTransaction tx(*txNew);
98101
--tx.nLockTime; // Just to change the hash :)
99-
wtx.SetTx(MakeTransactionRef(std::move(tx)));
102+
txNew = MakeTransactionRef(std::move(tx));
100103
}
101-
pwalletMain->AddToWallet(wtx);
102-
vpwtx.push_back(&pwalletMain->mapWallet[wtx.GetHash()]);
104+
pwalletMain->AddToWallet(txNew, [&](TxEntry& entry, bool fNew) {
105+
entry.second.mapValue["comment"] = "x";
106+
vpwtx.push_back(&entry.second);
107+
return true;
108+
});
103109
vpwtx[2]->nTimeReceived = (unsigned int)1333333329;
104110
vpwtx[2]->nOrderPos = -1;
105111

0 commit comments

Comments
 (0)