Unify CWalletTx construction #9680

ryanofsky · 2017-02-03T19:07:36Z

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

promag

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

Needs rebase.

promag · 2017-08-12T22:44:22Z

src/qt/walletmodel.cpp

@@ -578,9 +578,9 @@ void WalletModel::getOutputs(const std::vector<COutPoint>& vOutpoints, std::vect
    for (const COutPoint& outpoint : vOutpoints)
    {
        if (!wallet->mapWallet.count(outpoint.hash)) continue;
-        int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain();
+        int nDepth = wallet->mapWallet.at(outpoint.hash).GetDepthInMainChain();
        if (nDepth < 0) continue;


Isn't this condition taking advantage of the dummy 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.

promag · 2017-08-12T22:45:15Z

src/qt/walletmodel.cpp

        if (nDepth < 0) continue;
-        COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);
+        COutput out(&wallet->mapWallet.at(outpoint.hash), outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);


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

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.

promag · 2017-08-12T22:48:55Z

src/wallet/feebumper.cpp

@@ -246,7 +246,7 @@ bool CFeeBumper::commit(CWallet *pWallet)
        currentResult = BumpFeeResult::MISC_ERROR;
        return false;
    }
-    CWalletTx& oldWtx = pWallet->mapWallet[txid];
+    CWalletTx& oldWtx = pWallet->mapWallet.at(txid);


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

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

Also seems to be taken care of in #11039.

ryanofsky

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

ryanofsky · 2017-08-14T18:27:19Z

src/qt/walletmodel.cpp

@@ -578,9 +578,9 @@ void WalletModel::getOutputs(const std::vector<COutPoint>& vOutpoints, std::vect
    for (const COutPoint& outpoint : vOutpoints)
    {
        if (!wallet->mapWallet.count(outpoint.hash)) continue;
-        int nDepth = wallet->mapWallet[outpoint.hash].GetDepthInMainChain();
+        int nDepth = wallet->mapWallet.at(outpoint.hash).GetDepthInMainChain();
        if (nDepth < 0) continue;


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.

ryanofsky · 2017-08-14T18:30:10Z

src/qt/walletmodel.cpp

        if (nDepth < 0) continue;
-        COutput out(&wallet->mapWallet[outpoint.hash], outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);
+        COutput out(&wallet->mapWallet.at(outpoint.hash), outpoint.n, nDepth, true /* spendable */, true /* solvable */, true /* safe */);


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.

ryanofsky · 2017-08-14T18:31:05Z

src/wallet/feebumper.cpp

@@ -246,7 +246,7 @@ bool CFeeBumper::commit(CWallet *pWallet)
        currentResult = BumpFeeResult::MISC_ERROR;
        return false;
    }
-    CWalletTx& oldWtx = pWallet->mapWallet[txid];
+    CWalletTx& oldWtx = pWallet->mapWallet.at(txid);


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

Also seems to be taken care of in #11039.

laanwj · 2017-08-18T07:16:20Z

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.

sipa · 2017-09-21T00:39:22Z

src/wallet/rpcwallet.cpp

@@ -379,7 +379,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 void SendMoney(CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue, std::string fromAccount, CTransactionRef& txNew)


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?

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: a0a4d82 -> f16d28c (pr/makewtx.11 -> pr/makewtx.12)

jonasschnelli · 2017-09-22T05:19:45Z

Nice!
Concept ACK.

sipa

utACK f16d28c

sipa · 2017-11-22T00:11:37Z

src/qt/walletmodel.cpp

            return SendCoinsReturn(TransactionCommitFailed, QString::fromStdString(state.GetRejectReason()));

        CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
-        ssTx << *newTx->tx;
+        ssTx << **newTx;


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

Done in d070a69

ryanofsky

Added 1 commit f16d28c -> d070a69 (pr/makewtx.12 -> pr/makewtx.13, compare)
Squashed d070a69 -> 1dc99fb (pr/makewtx.13 -> pr/makewtx.14)
Rebased 1dc99fb -> fe84e57 (pr/makewtx.14 -> pr/makewtx.15)
Rebased fe84e57 -> 857a50a (pr/makewtx.15 -> pr/makewtx.16)
Rebased 857a50a -> d8f17e6 (pr/makewtx.16 -> pr/makewtx.17)
Rebased d8f17e6 -> 1531d0d (pr/makewtx.17 -> pr/makewtx.18)
Rebased 1531d0d -> 0c6ee54 (pr/makewtx.18 -> pr/makewtx.19)
Rebased 0c6ee54 -> b110df7 (pr/makewtx.19 -> pr/makewtx.20)
Rebased b110df7 -> 7bae52b (pr/makewtx.20 -> pr/makewtx.21)

jamesob

Concept ACK 7bae52b

jamesob · 2018-03-06T18:48:37Z

src/wallet/test/accounting_tests.cpp

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

    wtx.mapValue["comment"] = "z";
    pwalletMain->AddToWallet(wtx);
-    vpwtx.push_back(&pwalletMain->mapWallet[wtx.GetHash()]);
+    vpwtx.push_back(&pwalletMain->mapWallet.at(wtx.GetHash()));


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?

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 · 2018-03-06T19:39:04Z

ACK 7bae52b

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

ryanofsky · 2018-03-06T19:50:17Z

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

jamesob · 2018-03-06T19:56:43Z

ACK a5a2e7e

ryanofsky · 2018-03-07T13:26:39Z

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

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.

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.

maflcko · 2018-03-08T15:20:13Z

Needs rebase

ryanofsky · 2018-03-12T22:11:12Z

Needs rebase

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

ryanofsky · 2018-03-13T20:13:56Z

Added 1 commits 2c08d0b -> d0b8536 (pr/makewtx.25 -> pr/makewtx.26, compare) implementing suggestion from #9381 (comment).
Squashed d0b8536 -> b4bc32a (pr/makewtx.26 -> pr/makewtx.27)

kallewoof

utACK a128bdc

sipa · 2018-03-14T01:32:21Z

utACK b4bc32a

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

* [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 bitcoin#9381 There is no change in behavior. * [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. * Apply suggestions from code review Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: Russell Yanofsky <russ@yanofsky.org> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

fanquake added the Wallet label Feb 3, 2017

This was referenced Feb 3, 2017

Remove CWalletTx merging logic from AddToWallet #9381

Merged

Set correct metadata on bumpfee wallet transactions #9673

Merged

ryanofsky force-pushed the pr/makewtx branch from 8480832 to d7510af Compare February 6, 2017 17:02

ryanofsky changed the title ~~Unify CWalletTx construction (on top of #9673)~~ Unify CWalletTx construction Feb 6, 2017

ryanofsky force-pushed the pr/makewtx branch from d7510af to c472c5a Compare March 3, 2017 19:24

ryanofsky force-pushed the pr/makewtx branch from c472c5a to 6fd4fdc Compare June 2, 2017 16:04

ryanofsky force-pushed the pr/makewtx branch from 6fd4fdc to fa27fbf Compare June 19, 2017 19:23

ryanofsky force-pushed the pr/makewtx branch from fa27fbf to 2245b37 Compare July 18, 2017 18:49

promag reviewed Aug 12, 2017

View reviewed changes

ryanofsky force-pushed the pr/makewtx branch from 2245b37 to 51b93a9 Compare August 14, 2017 18:33

ryanofsky commented Aug 14, 2017

View reviewed changes

ryanofsky force-pushed the pr/makewtx branch from 51b93a9 to 635f4ef Compare August 16, 2017 17:36

ryanofsky force-pushed the pr/makewtx branch 2 times, most recently from 8fc4343 to 0a8204b Compare August 25, 2017 21:36

ryanofsky force-pushed the pr/makewtx branch from 0a8204b to 1cf7f53 Compare September 20, 2017 21:44

sipa reviewed Sep 21, 2017

View reviewed changes

ryanofsky force-pushed the pr/makewtx branch 3 times, most recently from a0a4d82 to f16d28c Compare November 20, 2017 22:08

sipa reviewed Nov 22, 2017

View reviewed changes

ryanofsky force-pushed the pr/makewtx branch from f16d28c to 1dc99fb Compare December 1, 2017 22:13

ryanofsky commented Dec 1, 2017

View reviewed changes

ryanofsky force-pushed the pr/makewtx branch from 1dc99fb to fe84e57 Compare December 14, 2017 17:59

ryanofsky mentioned this pull request Dec 15, 2017

Travis x86_64-apple-darwin11 errors: "Configuring boost... /bin/sh: 1: requires: not found" #11906

Closed

ryanofsky force-pushed the pr/makewtx branch from fe84e57 to 857a50a Compare December 15, 2017 12:37

ryanofsky force-pushed the pr/makewtx branch 2 times, most recently from d8f17e6 to 1531d0d Compare December 31, 2017 19:00

ryanofsky force-pushed the pr/makewtx branch from 1531d0d to 0c6ee54 Compare January 25, 2018 16:24

ryanofsky force-pushed the pr/makewtx branch from 0c6ee54 to b110df7 Compare February 2, 2018 20:49

ryanofsky force-pushed the pr/makewtx branch from b110df7 to 7bae52b Compare February 23, 2018 14:42

jamesob reviewed Mar 6, 2018

View reviewed changes

ryanofsky force-pushed the pr/makewtx branch from 7bae52b to a5a2e7e Compare March 6, 2018 19:49

ryanofsky force-pushed the pr/makewtx branch from a5a2e7e to da94a51 Compare March 7, 2018 13:24

ryanofsky force-pushed the pr/makewtx branch from da94a51 to 8fcd24e Compare March 7, 2018 18:15

ryanofsky added 2 commits March 7, 2018 21:12

ryanofsky force-pushed the pr/makewtx branch from 2c08d0b to b4bc32a Compare March 13, 2018 20:13

kallewoof approved these changes Mar 13, 2018

View reviewed changes

sipa merged commit b4bc32a into bitcoin:master Mar 14, 2018

xdustinface mentioned this pull request Aug 29, 2020

bitcoin#9680: Unify CWalletTx construction dashpay/dash#3680

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Unify CWalletTx construction #9680

Unify CWalletTx construction #9680

Uh oh!

Conversation

ryanofsky commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Sep 22, 2017

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Mar 6, 2018

Uh oh!

ryanofsky commented Mar 6, 2018

Uh oh!

jamesob commented Mar 6, 2018

Uh oh!

ryanofsky commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 8, 2018

Uh oh!

ryanofsky commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 13, 2018

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Feb 3, 2017 •

edited

Loading

promag left a comment •

edited

Loading

laanwj commented Aug 18, 2017 •

edited

Loading

ryanofsky Dec 1, 2017 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky commented Mar 7, 2018 •

edited

Loading

ryanofsky commented Mar 12, 2018 •

edited

Loading