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
577@@ -578,9 +578,9 @@ void WalletModel::getOutputs(const std::vector<COutPoint>& vOutpoints, std::vect
578 for (const COutPoint& outpoint : vOutpoints)
579 {
580 if (!wallet->mapWallet.count(outpoint.hash)) continue;
581- int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain();
582+ int nDepth = wallet->mapWallet.at(outpoint.hash).GetDepthInMainChain();
583 if (nDepth < 0) continue;
CWalletTx? If so replace .at() with .find() and test the iterator.
Isn’t this condition taking advantage of the dummy CWalletTx? If so replace .at() with .find() and test the iterator.
Dummy constructor isn’t called here because of the count check above.
577@@ -578,9 +578,9 @@ void WalletModel::getOutputs(const std::vector<COutPoint>& vOutpoints, std::vect
578 for (const COutPoint& outpoint : vOutpoints)
579 {
580 if (!wallet->mapWallet.count(outpoint.hash)) continue;
581- int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain();
582+ int nDepth = wallet->mapWallet.at(outpoint.hash).GetDepthInMainChain();
583 if (nDepth < 0) continue;
584- COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);
585+ COutput out(&wallet->mapWallet.at(outpoint.hash), outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);
245@@ -246,7 +246,7 @@ bool CFeeBumper::commit(CWallet *pWallet)
246 currentResult = BumpFeeResult::MISC_ERROR;
247 return false;
248 }
249- CWalletTx& oldWtx = pWallet->mapWallet[txid];
250+ CWalletTx& oldWtx = pWallet->mapWallet.at(txid);
.find(txid) above (before .count()) and test if exists, also avoids 2nd lookup.
mapWallet.at() can throw so I guess it should be avoided and replaced with mapWallet.find(), to test the returned iterator.
Needs rebase.
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.
Great! I’ve always been wary of this projects’ gratuitous use of [] for retrieval, as we don’t want to accidentally create empty objects and because it generates more code. The second is true even when it’s shielded with .count() or such. .at/.find is better.
So concept ACK at least - haven’t been able to review all the changes in detail.
378@@ -379,7 +379,7 @@ UniValue getaddressesbyaccount(const JSONRPCRequest& request)
379 return ret;
380 }
381
382-static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, CWalletTx& wtxNew, const CCoinControl& coin_control)
383+static void SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue, std::string fromAccount, CTransactionRef& txNew)
txNew seems to be only used to store the resulting output in, wouldn’t it be more convenient to make it the return type rather than an argument?
Given that the argument txNew seems to be only used to store the resulting output in, wouldn’t it be more convenient to make it the return type rather than an argument?
Forgot to respond to this comment, but this was a good idea, and I implemented it in a previous update to the PR: a0a4d8205498c38e53fb85e25cf6428ead1e93e1 -> f16d28cc4cbab3b1c8d05d9e327676afafefdbdf (pr/makewtx.11 -> pr/makewtx.12)
338+ if (!wallet->CommitTransaction(*newTx, {} /* mapValue */, std::move(vOrderForm), {} /* fromAccount */, *keyChange, g_connman.get(), state))
339 return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(state.GetRejectReason()));
340
341 CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
342- ssTx << *newTx->tx;
343+ ssTx << **newTx;
std::shared_ptr<X> should work, if X is serializable.
43@@ -44,7 +44,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
44
45 wtx.mapValue["comment"] = "z";
46 pwalletMain->AddToWallet(wtx);
47- vpwtx.push_back(&pwalletMain->mapWallet[wtx.GetHash()]);
48+ vpwtx.push_back(&pwalletMain->mapWallet.at(wtx.GetHash()));
.at() will cause compile-time errors for non-existent keys, but from what I understand at() will throw runtime out_of_range exceptions. This change in behavior seems okay, but I don’t have great context here – are we alright with these lines now being liable to throw runtime exceptions?
ACK https://github.com/bitcoin/bitcoin/commit/7bae52b4f5d7893a951c38af032a09b12c4ccebf
Cloned this branch, compiled, and ran bitcoin-qt.
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
https://github.com/bitcoin/bitcoin/pull/9381
There is no change in behavior.
No change in behavior in the normal case. But buggy mapWallet lookups with
invalid txids will now throw exceptions instead of inserting dummy entries into
the map, and potentially causing segfaults and other failures.
This also makes it a compiler error to use the mapWallet[hash] syntax which
could create dummy entries.
Needs rebase
Rebased 8fcd24ece6720d09a22758d6addafcf3539057a2 -> 2c08d0b47843de4aab10e21db7a417791e34be2e (pr/makewtx.24 -> pr/makewtx.25) for #12607.
