Remove CWalletTx merging logic from AddToWallet #9381

ryanofsky · 2016-12-19T23:00:20Z

This is a pure refactoring, no behavior is changing.

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.

Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function.

This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying.

ryanofsky · 2017-01-25T18:30:33Z

Rebased ed457a3 -> b1ac3cf to resolve merge conflicts with bumpfee (#8456).

ryanofsky · 2017-01-25T18:48:42Z

I'm thinking of splitting this up into two commits to make it easier to review. First commit would change CreateTransaction to return a CTransactionRef instead of CWalletTx. Second commit would change AddToWallet to accept a CTransactionRef instead of a CWalletTx.

If any reviewers would prefer this you can let me know.

TheBlueMatt · 2017-01-25T19:26:39Z

Splitting commits into logical breaks is always appreciated.

…

On 01/25/17 18:48, Russell Yanofsky wrote: I'm thinking of splitting this up into two commits to make it easier to review. First commit would change CreateTransaction to return a CTransactionRef instead of CWalletTx. Second commit would change AddToWallet to accept a CTransactionRef instead of a CWalletTx. If any reviewers would prefer this you can let me know. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9381 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnoHu0xZqwtESQp3_rXWoR3OurDnNzgks5rV5kPgaJpZM4LRR4W>.

TheBlueMatt · 2017-01-25T22:17:34Z

Hum, reading this (finally) now, I'm not sure if I'm a big fan of this approach. The call-function-which-callsback-to-let-you-fill-in-things-that-really-should-have-been-arguments pattern really sucks. On the flip side, passing in CWalletTxes, and copying those into mapWallet can also lead to issues if you try to do anything to the object you just passed in thinking it is what got stored in the wallet (a mistake I made recently).

Maybe we should adapt some of these functions (AddToWallet/CommitTransaction/whatever) to just always take an rvalue to a CWalletTx. This leaves only AddToWalletIfInvolvingMe (I think) which has to update-or-add, and that can just use an internal version of AddToWallet which takes the CWalletDB as an argument instead of creating its own.

Preserves comment strings, order form and account strings from the original wallet transaction. Also sets fTimeReceivedIsTxTime and fFromMe fields (which are set in CWallet::CreateTransaction) which don't actually influence wallet behavior, but record that the transaction originated in the wallet (instead of coming from the network or sendrawtransaction). Setting these fields also simplifies CWalletTx cleanup in bitcoin#9381.

ryanofsky · 2017-02-02T23:51:36Z

The call-function-which-callsback-to-let-you-fill-in-things-that-really-should-have-been-arguments pattern really sucks.

If you are adamant about this, could you say more about why it sucks? I think a callback is exactly the thing you want when you are doing an in-place update to a data structure.

In any case, I agree that CommitTransaction shouldn't take a callback, so I changed it to just take a transaction ref. So now there are way fewer callbacks (I think only 3 left in non-test code).

TheBlueMatt · 2017-02-03T18:46:10Z

I'm just generally not a fan of callbacks making ownership models inconsistent. eg this kind of thing is where people always fuck up lockordering (though admittedly less so in this particular case, more the general case of callbacks going in both directions between modules).

So I do like this version much better, but still not sure the call back is required to be publicly exposed...Can we just leave MarkReplaced and add a similar function for importpruned funds to use (I dont believe the double-disk-sync in importprunedfunds will make for inconsistent/bad on-disk state?). Then we can make AddToWallet(..., callback) private and use AddToWalletIfInvolvingMe (which should probably also take a CTransactionRef, not a CTransaction, though maybe thats for a separate PR) publicly.

ryanofsky · 2017-02-03T19:10:56Z

Can we just leave MarkReplaced and add a similar function for importpruned funds to use

Will try that. In the meantime I moved the non-AddToWallet cleanup to #9680 so it can be considered separately. By the way #9369 is another PR which significantly simplifies AddToWallet.

CWalletTx initialization has been fixed so it's no longer necessary to change which wallet a transaction is bound to.

ryanofsky · 2020-05-01T15:00:34Z

Rebased ece5d1c -> f2892d8 (pr/atw-nomerge.78 -> pr/atw-nomerge.79, compare) due to conflict with #16426

Sjors · 2020-05-04T10:21:31Z

Light re-utACK f2892d8, looks like a reasonably straight-forward rebase.

meshcollider

utACK f2892d8

maflcko

ACK f2892d8

Just some questions

maflcko · 2020-05-05T15:12:48Z

src/wallet/wallet.cpp

+    CWalletTx* wtx = AddToWallet(std::move(tx), {}, [&](CWalletTx& wtx, bool new_tx) {
+        CHECK_NONFATAL(wtx.mapValue.empty());
+        CHECK_NONFATAL(wtx.vOrderForm.empty());
+        wtx.mapValue = std::move(mapValue);


Does the std::move still work after going through a lambda capture by reference?

Yes. mapValue is a reference to the same thing it was outside of the lambda. std::move casts it to an rvalue reference.

Oh, this has nothing to do with lambdas. I didn't know that references (as long as they are not const) can be moved.

re: #9381 (comment)

Oh, this has nothing to do with lambdas. I didn't know that references (as long as they are not const) can be moved.

Yep, std::move isn't affected by outside things like this. std::move(mapValue) is only a type cast from mapValue_t& to mapValue_t&& that makes the compiler favor the operator=(mapValue_t&&) assignment overload instead of the operator=(const mapValue_t&) one for wtx.mapValue =

Even const ones, actually - though unless they have mutable fields, there is no effect.

With "no effect" you mean const& will stay const&, right? See also the example, which does not compile, because the const& copy constructor is deleted:

#include <memory> struct Test{ const std::unique_ptr<int>b; Test(const std::unique_ptr<int>& a):b{std::move(a)}{} };

std::move(a) where a is const T&, returns something of type const T&&. If T has for example a operator=(const T&&), then that one would be selected in assignment. If there is no such operator, operator=(const T&) will be selected instead. In general a const T&& assignment/constructor operator only makes sense if a class has mutable fields.

TIL that const T&& exists.

maflcko · 2020-05-05T15:13:25Z

src/wallet/wallet.cpp


    // Notify that old coins are spent
-    for (const CTxIn& txin : wtxNew.tx->vin) {
+    for (const CTxIn& txin : wtx->tx->vin) {


Doesn't this line crash the node when the wallet can not write the tx to disk?

re: #9381 (comment)

Doesn't this line crash the node when the wallet can not write the tx to disk?

Wow, good catch! I'm not sure current behavior is ideal either but it wasn't my intention to change it. Updated PR

ryanofsky

Updated f2892d8 -> 28b112e (pr/atw-nomerge.79 -> pr/atw-nomerge.80, compare) reverting some CommitTransaction changes to avoid changing behavior and crashing on failure

ryanofsky · 2020-05-05T16:30:13Z

src/wallet/wallet.cpp


    // Notify that old coins are spent
-    for (const CTxIn& txin : wtxNew.tx->vin) {
+    for (const CTxIn& txin : wtx->tx->vin) {


re: #9381 (comment)

Doesn't this line crash the node when the wallet can not write the tx to disk?

Wow, good catch! I'm not sure current behavior is ideal either but it wasn't my intention to change it. Updated PR

ryanofsky · 2020-05-05T16:39:28Z

src/wallet/wallet.cpp

+    CWalletTx* wtx = AddToWallet(std::move(tx), {}, [&](CWalletTx& wtx, bool new_tx) {
+        CHECK_NONFATAL(wtx.mapValue.empty());
+        CHECK_NONFATAL(wtx.vOrderForm.empty());
+        wtx.mapValue = std::move(mapValue);


re: #9381 (comment)

Oh, this has nothing to do with lambdas. I didn't know that references (as long as they are not const) can be moved.

Yep, std::move isn't affected by outside things like this. std::move(mapValue) is only a type cast from mapValue_t& to mapValue_t&& that makes the compiler favor the operator=(mapValue_t&&) assignment overload instead of the operator=(const mapValue_t&) one for wtx.mapValue =

maflcko · 2020-05-05T17:29:37Z

Ah sorry, no need to revert your changes. If the wallet can't write to disk, a simple CHECK_NONFATAL(wtx) should be sufficient

ryanofsky · 2020-05-05T17:54:24Z

Ah sorry, no need to revert your changes. If the wallet can't write to disk, a simple CHECK_NONFATAL(wtx) should be sufficient

I don't think CHECK_NONFATAL would be right, because this is checking a runtime error, not an internal assertion about how code works. I can go back to the earlier version and add this if that's your preference. But actually I like the update better because it makes the overall diff smaller and avoids changing behavior in a refactoring PR. I think ideally if different error handling is needed here, it would be handled in a standalone PR with a unit test.

maflcko · 2020-05-05T17:58:36Z

Well, our assumption in the code is that the wallet can write to disk. If that assumption is violated it seems fine to assert or do that CHECK thingy. But I also see your point to make it a separate runtime error.

Anyway, re-ACK 28b112e

meshcollider · 2020-05-05T23:37:03Z

re-utACK 28b112e

28b112e Get rid of BindWallet (Russell Yanofsky) d002f9d Disable CWalletTx copy constructor (Russell Yanofsky) 65b9d8f Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky) bd2fbc7 Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky) 2b9cba2 Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky) Pull request description: This is a pure refactoring, no behavior is changing. 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. Motivation for this change came from the bumpfee PR bitcoin#8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function. This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying. ACKs for top commit: MarcoFalke: Anyway, re-ACK 28b112e Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef

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] 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>

Summary: > 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. This is a backport of Core [[bitcoin/bitcoin#9381 | PR9381]] [1/5] bitcoin/bitcoin@2b9cba2 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9074

Summary: This is a backport of Core [[bitcoin/bitcoin#9381 | PR9381]] [2/5] bitcoin/bitcoin@bd2fbc7 Depends on D9074 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9075

Summary: The change in walletdb.cpp is easier to review ignoring whitespace. This change is need to get rid of CWalletTx copy constructor. This is a backport of Core [[bitcoin/bitcoin#9381 | PR9381]] [3/5] bitcoin/bitcoin@65b9d8f Depends on D9075 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9076

Summary: Disable copying of CWalletTx objects to prevent bugs where instances get copied in and out of the mapWallet map and fields are updated in the wrong copy. This is a backport of Core [[bitcoin/bitcoin#9381 | PR9381]] [4/5] bitcoin/bitcoin@d002f9d Depends on D9076 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9077

Summary: CWalletTx initialization has been fixed so it's no longer necessary to change which wallet a transaction is bound to. This concludes backport of Core [[bitcoin/bitcoin#9381 | PR9381]] [5/5] bitcoin/bitcoin@28b112e Depends on D9077 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9078

* [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 Refactoring Wallet labels Dec 19, 2016

ryanofsky force-pushed the pr/atw-nomerge branch from d988ec9 to ed457a3 Compare January 6, 2017 17:08

ryanofsky mentioned this pull request Jan 13, 2017

[RPC] Simplified bumpfee command. #8456

Merged

ryanofsky force-pushed the pr/atw-nomerge branch from ed457a3 to b1ac3cf Compare January 25, 2017 18:29

ryanofsky force-pushed the pr/atw-nomerge branch from b1ac3cf to b78f2eb Compare February 2, 2017 23:23

ryanofsky mentioned this pull request Feb 2, 2017

Set correct metadata on bumpfee wallet transactions #9673

Merged

ryanofsky force-pushed the pr/atw-nomerge branch from b78f2eb to d45e95e Compare February 2, 2017 23:34

ryanofsky changed the title ~~Remove CWalletTx merging logic from AddToWallet~~ Remove CWalletTx merging logic from AddToWallet (on top of #9673) Feb 2, 2017

ryanofsky force-pushed the pr/atw-nomerge branch from d45e95e to 31ec1e6 Compare February 3, 2017 16:26

ryanofsky mentioned this pull request Feb 3, 2017

Unify CWalletTx construction #9680

Merged

ryanofsky changed the title ~~Remove CWalletTx merging logic from AddToWallet (on top of #9673)~~ Remove CWalletTx merging logic from AddToWallet (on top of #9680) Feb 3, 2017

ryanofsky force-pushed the pr/atw-nomerge branch from 31ec1e6 to 98cf53c Compare February 6, 2017 17:47

ryanofsky changed the title ~~Remove CWalletTx merging logic from AddToWallet (on top of #9680)~~ WIP: Remove CWalletTx merging logic from AddToWallet (on top of #9680) Feb 6, 2017

ryanofsky force-pushed the pr/atw-nomerge branch from 98cf53c to 3b022f6 Compare March 3, 2017 19:38

ryanofsky force-pushed the pr/atw-nomerge branch from 3b022f6 to 2764551 Compare June 2, 2017 16:05

ryanofsky force-pushed the pr/atw-nomerge branch from 2764551 to c80a2bf Compare June 19, 2017 19:24

ryanofsky force-pushed the pr/atw-nomerge branch from c80a2bf to 90847c4 Compare July 18, 2017 18:50

ryanofsky force-pushed the pr/atw-nomerge branch 2 times, most recently from 777c297 to 4a20b3c Compare August 16, 2017 17:36

ryanofsky force-pushed the pr/atw-nomerge branch from 4a20b3c to 4e8ce01 Compare August 23, 2017 19:46

Get rid of BindWallet

28b112e

CWalletTx initialization has been fixed so it's no longer necessary to change which wallet a transaction is bound to.

DrahtBot added the Needs rebase label May 1, 2020

ryanofsky force-pushed the pr/atw-nomerge branch from ece5d1c to f2892d8 Compare May 1, 2020 14:58

DrahtBot removed the Needs rebase label May 1, 2020

meshcollider reviewed May 5, 2020

View reviewed changes

maflcko approved these changes May 5, 2020

View reviewed changes

ryanofsky force-pushed the pr/atw-nomerge branch from f2892d8 to 28b112e Compare May 5, 2020 16:50

ryanofsky commented May 5, 2020

View reviewed changes

meshcollider merged commit 60091d2 into bitcoin:master May 5, 2020

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Remove CWalletTx merging logic from AddToWallet #9381

Remove CWalletTx merging logic from AddToWallet #9381

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ryanofsky commented Jan 25, 2017

Uh oh!

ryanofsky commented Jan 25, 2017

Uh oh!

TheBlueMatt commented Jan 25, 2017 via email

Uh oh!

TheBlueMatt commented Jan 25, 2017

Uh oh!

ryanofsky commented Feb 2, 2017

Uh oh!

TheBlueMatt commented Feb 3, 2017

Uh oh!

ryanofsky commented Feb 3, 2017

Uh oh!

ryanofsky commented May 1, 2020

Uh oh!

Sjors commented May 4, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko 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!

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!

sipa May 5, 2020 • 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!

maflcko May 5, 2020 • 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!

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!

maflcko commented May 5, 2020

Uh oh!

ryanofsky commented May 5, 2020

Uh oh!

maflcko commented May 5, 2020

Uh oh!

meshcollider commented May 5, 2020

Uh oh!

Uh oh!

ryanofsky commented Dec 19, 2016 •

edited

Loading

sipa May 5, 2020 •

edited

Loading

maflcko May 5, 2020 •

edited

Loading