Unify CWalletTx construction #9680

ryanofsky commented at 7:07 PM on February 3, 2017: member

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

fanquake added the label Wallet on Feb 3, 2017

ryanofsky force-pushed on Feb 6, 2017

ryanofsky renamed this:
~~Unify CWalletTx construction (on top of #9673)~~
Unify CWalletTx construction
on Feb 6, 2017

ryanofsky force-pushed on Mar 3, 2017

ryanofsky force-pushed on Jun 2, 2017

ryanofsky force-pushed on Jun 19, 2017

ryanofsky force-pushed on Jul 18, 2017

in src/qt/walletmodel.cpp:583 in 2245b37ea9 outdated

 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;

promag commented at 10:44 PM on August 12, 2017:

Isn't this condition taking advantage of the dummy CWalletTx? If so replace .at() with .find() and test the iterator.

ryanofsky commented at 6:27 PM on August 14, 2017:

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.

in src/qt/walletmodel.cpp:583 in 2245b37ea9 outdated

 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 */);

promag commented at 10:45 PM on August 12, 2017:

Another lookup? With the above suggestion it can be avoided.

ryanofsky commented at 6:30 PM on August 14, 2017:

Another lookup? With the above suggestion it can be avoided.

It looks like this is done in your PR #11039. I'll review that PR, so this PR can be limited to replacing [] lookups with at() lookups.

in src/wallet/feebumper.cpp:249 in 2245b37ea9 outdated

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

promag commented at 10:48 PM on August 12, 2017:

Replace with .find(txid) above (before .count()) and test if exists, also avoids 2nd lookup.

ryanofsky commented at 6:31 PM on August 14, 2017:

Replace with .find(txid) above (before .count()) and test if exists, also avoids 2nd lookup.

Also seems to be taken care of in #11039.

promag commented at 10:50 PM on August 12, 2017: member

mapWallet.at() can throw so I guess it should be avoided and replaced with mapWallet.find(), to test the returned iterator.

Needs rebase.

ryanofsky force-pushed on Aug 14, 2017

ryanofsky commented at 6:33 PM on August 14, 2017: member

Thanks very much for the review. Rebased 2245b37ea967ca0047766479cd8e85409399fe0a -> 51b93a90a403abed3716782d86dada3d4e92e38e (pr/makewtx.4 -> pr/makewtx.5), just fixing minor merge conflicts.

ryanofsky force-pushed on Aug 16, 2017

laanwj commented at 7:16 AM on August 18, 2017: member

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.

ryanofsky force-pushed on Aug 23, 2017

ryanofsky force-pushed on Aug 25, 2017

ryanofsky force-pushed on Sep 20, 2017

in src/wallet/rpcwallet.cpp:383 in e0bad0a8ce outdated

 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)

sipa commented at 12:39 AM on September 21, 2017:

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?

ryanofsky commented at 10:15 PM on December 1, 2017:

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)

jonasschnelli commented at 5:19 AM on September 22, 2017: contributor

Nice! Concept ACK.

ryanofsky force-pushed on Nov 16, 2017

ryanofsky force-pushed on Nov 20, 2017

in src/qt/walletmodel.cpp:338 in 0fdca91f4b outdated

 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;

sipa commented at 12:11 AM on November 22, 2017:

I think the extra dereference isn't needed here; serializing a std::shared_ptr<X> should work, if X is serializable.

ryanofsky commented at 10:13 PM on December 1, 2017:

Done in d070a69e9734b83e2defc29aa97bf4dab8953151

sipa commented at 12:21 AM on November 22, 2017: member

utACK f16d28cc4cbab3b1c8d05d9e327676afafefdbdf

ryanofsky force-pushed on Dec 1, 2017

ryanofsky commented at 10:15 PM on December 1, 2017: member

Added 1 commit f16d28cc4cbab3b1c8d05d9e327676afafefdbdf -> d070a69e9734b83e2defc29aa97bf4dab8953151 (pr/makewtx.12 -> pr/makewtx.13, compare) Squashed d070a69e9734b83e2defc29aa97bf4dab8953151 -> 1dc99fb74210e511287e1b2c0406ab336ffa241f (pr/makewtx.13 -> pr/makewtx.14) Rebased 1dc99fb74210e511287e1b2c0406ab336ffa241f -> fe84e57ad18f7e5518a191aa30d70fc37ff3f73e (pr/makewtx.14 -> pr/makewtx.15) Rebased fe84e57ad18f7e5518a191aa30d70fc37ff3f73e -> 857a50a93fc64c8c7af1d7dd05fc4992dc170c0d (pr/makewtx.15 -> pr/makewtx.16) Rebased 857a50a93fc64c8c7af1d7dd05fc4992dc170c0d -> d8f17e6388787da71ca502d74abdfe0240c315c0 (pr/makewtx.16 -> pr/makewtx.17) Rebased d8f17e6388787da71ca502d74abdfe0240c315c0 -> 1531d0df4bd727c5c30d4685ab9abeb0810936db (pr/makewtx.17 -> pr/makewtx.18) Rebased 1531d0df4bd727c5c30d4685ab9abeb0810936db -> 0c6ee54c94f2972e084022c539ffc52345daf48f (pr/makewtx.18 -> pr/makewtx.19) Rebased 0c6ee54c94f2972e084022c539ffc52345daf48f -> b110df7d9548566c428a2449f1e5b92633b98878 (pr/makewtx.19 -> pr/makewtx.20) Rebased b110df7d9548566c428a2449f1e5b92633b98878 -> 7bae52b4f5d7893a951c38af032a09b12c4ccebf (pr/makewtx.20 -> pr/makewtx.21)

ryanofsky force-pushed on Dec 14, 2017

ryanofsky force-pushed on Dec 15, 2017

ryanofsky force-pushed on Dec 31, 2017

ryanofsky force-pushed on Jan 25, 2018

ryanofsky force-pushed on Feb 2, 2018

ryanofsky force-pushed on Feb 23, 2018

in src/wallet/test/accounting_tests.cpp:47 in 7bae52b4f5 outdated

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

jamesob commented at 6:48 PM on March 6, 2018:

The commit message seems to suggest .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?

ryanofsky commented at 7:24 PM on March 6, 2018:

Oops, commit message is mistaken, it should say "runtime exceptions" not "compile errors". But the change is intentional because it should be preferable to throw exceptions if invalid txids are looked up instead of creating empty map entries with null CTransactionRef pointers that might later get dereferenced.

jamesob commented at 6:49 PM on March 6, 2018: member

Concept ACK https://github.com/bitcoin/bitcoin/pull/9680/commits/7bae52b4f5d7893a951c38af032a09b12c4ccebf

jamesob commented at 7:39 PM on March 6, 2018: member

ACK https://github.com/bitcoin/bitcoin/commit/7bae52b4f5d7893a951c38af032a09b12c4ccebf

Cloned this branch, compiled, and ran bitcoin-qt.

ryanofsky force-pushed on Mar 6, 2018

ryanofsky commented at 7:50 PM on March 6, 2018: member

Updated commit message 7bae52b4f5d7893a951c38af032a09b12c4ccebf -> a5a2e7eddeaa3c2323da83f1b40b1c382718cc3a (pr/makewtx.21 -> pr/makewtx.22), no other changes.

jamesob commented at 7:56 PM on March 6, 2018: member

ACK https://github.com/bitcoin/bitcoin/commit/a5a2e7eddeaa3c2323da83f1b40b1c382718cc3a

ryanofsky force-pushed on Mar 7, 2018

ryanofsky commented at 1:26 PM on March 7, 2018: member

Rebased a5a2e7eddeaa3c2323da83f1b40b1c382718cc3a -> da94a51fa50766d4bb528a16fee2b6e7a1fb2146 (pr/makewtx.22 -> pr/makewtx.23) due to conflict with #12421 causing travis build failures. Rebased da94a51fa50766d4bb528a16fee2b6e7a1fb2146 -> 8fcd24ece6720d09a22758d6addafcf3539057a2 (pr/makewtx.23 -> pr/makewtx.24) due to conflict with #11687.

ryanofsky force-pushed on Mar 7, 2018

[wallet] Construct CWalletTx objects in CommitTransaction

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.

a128bdc9e1

[wallet] Get rid of CWalletTx default constructor

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.

b4bc32a451

MarcoFalke commented at 3:20 PM on March 8, 2018: member

Needs rebase

ryanofsky commented at 10:11 PM on March 12, 2018: member

Needs rebase

Rebased 8fcd24ece6720d09a22758d6addafcf3539057a2 -> 2c08d0b47843de4aab10e21db7a417791e34be2e (pr/makewtx.24 -> pr/makewtx.25) for #12607.

ryanofsky force-pushed on Mar 13, 2018

ryanofsky commented at 8:13 PM on March 13, 2018: member

Added 1 commits 2c08d0b47843de4aab10e21db7a417791e34be2e -> d0b85362d3f620cf397b00992fddc48a5fc4ee79 (pr/makewtx.25 -> pr/makewtx.26, compare) implementing suggestion from #9381 (review). Squashed d0b85362d3f620cf397b00992fddc48a5fc4ee79 -> b4bc32a451720167000f59dd73ab07990f9c6b92 (pr/makewtx.26 -> pr/makewtx.27)

kallewoof approved

kallewoof commented at 11:26 PM on March 13, 2018: member

utACK a128bdc9e15dec5cd9aed1e4922c938edf31eb9a

sipa commented at 1:32 AM on March 14, 2018: member

utACK b4bc32a451720167000f59dd73ab07990f9c6b92

sipa merged this on Mar 14, 2018

sipa closed this on Mar 14, 2018

sipa referenced this in commit 6acd8700bc on Mar 14, 2018

UdjinM6 referenced this in commit ebe7e80a49 on Sep 21, 2020

MarcoFalke locked this on Sep 8, 2021