refactor: Clean up new wallet spend, receive files added #21207 #22100

ryanofsky · 2021-05-30T11:09:45Z

This makes CWallet and CWalletTx methods in spend.cpp and receive.cpp files into standalone functions.

It's a followup to #21207 MOVEONLY: CWallet transaction code out of wallet.cpp/.h, which moved code from wallet.cpp to new spend.cpp and receive.cpp files.

There are no changes in behavior. This is just making methods into functions and removing circular dependencies created by #21207. There are no comment or documentation changes, either. Removed comments from transaction.h are just migrated to spend.h, receive.h, and wallet.h.

This commit was split off from #21206 so there are a few earlier review comments there

DrahtBot · 2021-05-30T16:05:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22805 (refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack)
#22019 (wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101)
#21260 (wallet: indicate whether a transaction is in the mempool by danben)
#20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)
#20205 (wallet: Properly support a wallet id by achow101)
#17355 (gui: grey out used address in address book by za-kk)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#15719 (Wallet passive startup by ryanofsky)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors

utACK a3f6230

Review hint:

git show --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change

Sjors · 2021-05-31T16:29:58Z

src/wallet/wallet.h

    bool IsMine(const CTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    /** should probably be renamed to IsRelevantToMe */
    bool IsFromMe(const CTransaction& tx) const;
    CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
-    /** Returns whether all of the inputs match the filter */
-    bool IsAllFromMe(const CTransaction& tx, const isminefilter& filter) const;
-    CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const;


How come you're able to move GetCredit but not GetDebit?

re: #22100 (comment)

How come you're able to move GetCredit but not GetDebit?

I would like to move it, but it's kept in place for now to avoid circular dependencies. The problem is that the CWallet::SyncTransaction method needs to call CWallet::AddToWalletIfInvolvingMe which needs to call CWallet::IsFromMe which needs to call CWallet::GetDebit. These functions can't be moved out of wallet.cpp to receive.cpp, because wallet.cpp can't depend on receive.cpp (by design, higher level receive.cpp spend.cpp and a future sync.cpp are all meant to depend on the lower level wallet.cpp CWallet object).

I think a future fix for this will move CWallet::SyncTransaction along with chain attach and rescan code from wallet.cpp to sync.cpp. It should then be no problem for sync.cpp to depend on receive.cpp and for CWallet::AddToWalletIfInvolvingMe and related code to move to receive.cpp. The reason I didn't create sync.cpp and move these things already is that I was waiting for #20773 to split up the monolithic CWallet::Create function to make this easier.

Given that #20773 is merged, could the change to GetDebit be done now?

Or maybe in a (more compact) followup.

re: #22100 (comment)

How come you're able to move GetCredit but not GetDebit?

I would like to move it, but it's kept in place for now to avoid circular dependencies.

Given that #20773 is merged, could the change to GetDebit be done now?

Or maybe in a (more compact) followup.

Yes #20733 does make this easier, and yes this would make sense to do as a followup. Doing it in this PR wouldn't make great sense because this PR is not moving code just doing naming/declaration cleanups after a previous move. (Also this is not a tiny change, just in terms of number of lines). Will see what this looks like and post a followup.

ryanofsky

Thanks for the review and good color-moved-ws tip!

ryanofsky · 2021-05-31T17:30:05Z

src/wallet/wallet.h

    bool IsMine(const CTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    /** should probably be renamed to IsRelevantToMe */
    bool IsFromMe(const CTransaction& tx) const;
    CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
-    /** Returns whether all of the inputs match the filter */
-    bool IsAllFromMe(const CTransaction& tx, const isminefilter& filter) const;
-    CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const;


re: #22100 (comment)

How come you're able to move GetCredit but not GetDebit?

I would like to move it, but it's kept in place for now to avoid circular dependencies. The problem is that the CWallet::SyncTransaction method needs to call CWallet::AddToWalletIfInvolvingMe which needs to call CWallet::IsFromMe which needs to call CWallet::GetDebit. These functions can't be moved out of wallet.cpp to receive.cpp, because wallet.cpp can't depend on receive.cpp (by design, higher level receive.cpp spend.cpp and a future sync.cpp are all meant to depend on the lower level wallet.cpp CWallet object).

I think a future fix for this will move CWallet::SyncTransaction along with chain attach and rescan code from wallet.cpp to sync.cpp. It should then be no problem for sync.cpp to depend on receive.cpp and for CWallet::AddToWalletIfInvolvingMe and related code to move to receive.cpp. The reason I didn't create sync.cpp and move these things already is that I was waiting for #20773 to split up the monolithic CWallet::Create function to make this easier.

promag

Code review ACK a3f6230.

Maybe @achow101 should review this.

promag · 2021-05-31T19:39:48Z

src/wallet/receive.h

+CAmount OutputGetChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
+CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx);
+
+CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter);


Comparing to 4e11f88 (which I reviewed in #21206) what is the motivation to prefix with Cached? Is it relevant for the caller that some caching is involved? Is it a way to remember that concurrent syncs can happen and so the return value can be out of date?

re: #22100 (comment)

Comparing to 4e11f88 (which I reviewed in #21206) what is the motivation to prefix with Cached? Is it relevant for the caller that some caching is involved? Is it a way to remember that concurrent syncs can happen and so the return value can be out of date?

There should be no changes since #21206, but CachedTx is in debit/credit/change function names when the functions take CWalletTx arguments, since the point of CWalletTx class is to encapsulate CTransaction plus cached balance information. Functions named Tx instead of CachedTx just take plain CTransaction arguments by comparison. And functions named Input, Output, or Script take those things as arguments.

I think it makes calling code more readable if function names explicitly say what they compute values from and whether values may be stale due to caching. But this is just a naming convention I settled on because I didnt like the previous convention where there are so many slightly different functions all having the same name. There could be better approaches I didn't think of though.

Re: concurrency, I think cs_wallet doesn't really allow too much and there shouldn't be caching problems related to that. Caching more affects state transitions when blocks are connected disconnected and transactions are abandoned or replaced or added or removed from the mempool.

Functions named Tx instead of CachedTx just take plain CTransaction arguments by comparison

Thanks for the clarification. An alternative prefix could be Wallet, so in this case, could be WalletTxGetCredit. But it's fine as is and agree with the convention.

Re: concurrency, I think cs_wallet doesn't really allow too much and there shouldn't be caching problems related to that. Caching more affects state transitions when blocks are connected disconnected and transactions are abandoned or replaced or added or removed from the mempool.

Right, didn't mean to imply otherwise.

re: #22100 (comment)

Functions named Tx instead of CachedTx just take plain CTransaction arguments by comparison

Thanks for the clarification. An alternative prefix could be Wallet, so in this case, could be WalletTxGetCredit. But it's fine as is and agree with the convention.

That suggestion is pretty good too. Maybe I should s/CachedTx/WalletTx/g in this PR. Will do if reviewers want that. I was anticipating renaming CWalletTx to CachedTransaction later (and namespacing so its full name would be wallet::CachedTransaction) but maybe I am jumping the gun a little bit and should stick with WalletTx here.

Right, didn't mean to imply otherwise.

Sorry about that, I just misinterpreted "concurrent syncs" it sounds like.

Both are just fine to me, as long as the motivation and convention are clear.

ryanofsky · 2021-06-11T20:00:51Z

Rebased a3f6230 -> 642843a (pr/txfun.1 -> pr/txfun.2, compare) due to conflict with #22008

fjahr · 2021-06-13T20:02:53Z

Code review ACK 642843a

fjahr · 2021-08-23T22:08:41Z

Code review re-ACK 54faec8

Confirmed changes since my last review did not change behavior and only addressed mentioned merge conflicts.

Zero-1729

re-crACK 54faec8

LGTM, only resolved merge conflicts since last review.

meshcollider · 2021-09-01T05:12:40Z

Sorry about another rebase Russ, I'll review this next 😄

Followup to commit "MOVEONLY: CWallet transaction code out of wallet.cpp/.h" that detaches and renames some CWalletTx methods, making into them into standalone functions or CWallet methods instead. There are no changes in behavior and no code changes that aren't purely mechanical. It just gives spend and receive functions more consistent names and removes the circular dependencies added by the earlier MOVEONLY commit. There are also no comment or documentation changes. Removed comments from transaction.h are just migrated to spend.h, receive.h, and wallet.h.

ryanofsky · 2021-09-01T14:59:50Z

Sorry about another rebase Russ, I'll review this next smile

Thanks! And no problem, conflicts were trivial

Rebased 54faec8 -> b11a195 (pr/txfun.6 -> pr/txfun.7, compare) due to conflict with #22009

achow101 · 2021-09-01T20:09:44Z

I get that the point is to remove circular dependencies, but I am not sure about the new code layout which removes a lot of the object-oriented-ness of the wallet. It doesn't quite make sense to me why so many functions need or should be changed to take CWallet or CWalletTx (or both) as arguments when, conceptually, it makes more sense to have them remain as member functions.

ryanofsky · 2021-09-01T21:34:57Z

removes a lot of the object-oriented-ness of the wallet.

This was discussed more here #22100 (comment), but the general idea is that saying object.function() or function(object) does not substantively affect the object orientedness off the code, that big classes with too many interacting parts are hard to understand and maintain, and that the way you can break apart a big class without rewriting the world is to detach parts of it (possibly reorganizing related parts into smaller classes when it makes sense).

achow101 · 2021-09-01T22:11:01Z

Is there a description of where this is going and what the end state is going to look like? To me, it feels like many of the changes made here are not well motivated (other than removing the circular dependency). I've read through #21206, #21207, and https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, but it's not clear to me how these particular changes are related to the end goals stated in those places.

ryanofsky · 2021-09-01T23:13:06Z

This PR is my end state for detaching methods and breaking the monolithic wallet.h/cpp into wallet.h/cpp, transaction.h/cpp, receive.h/cpp, and spend.h/spend.cpp modules. Obviously if more work is done on balance tracking and spending code more improvements can be made in these places. This PR is only fixing inconsistencies in naming and moving function declarations out of wallet.h to the new transaction/spend/receive modules where the functions are implemented,

Separately, I would like add a sync.h/cpp module and move chain notification and rescan handling code from wallet.cpp there. This should only affect a handful of CWallet methods, and my presumption is that it would be easier implement after #15719 which move some sync code out of the wallet entirely, to the node where it could be shared with other chain clients like indexes. sync.h/cpp module creation would be basically tangential to everything in this except it might detach a few more CWallet methods and follow a similar pattern.

#21206 mostly concerns transaction.h/cpp and is basically the end state of transaction representation for all the potential changes described https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking. There would just be pretty small tweaks to the representation after #21206 required to implement the specific changes outlined on the wiki page. #21206 makes these changes safer to implement by taking away the ability to represent invalid transaction states.

Definitely you should ask if any of motivations I'm giving in PR descriptions are unclear. Or if any changes in any PR do not seem motivated. This particular PR is just tweaking function names to reflect the places those functions are currently located. So I think there is not very much to get excited about here.

achow101 · 2021-09-02T01:43:49Z

Ok, I think I get it now. #21207 moved several things, including members of CWallet and CWalletTx to spend.cpp and receive.cpp. Then this PR completes that move by making the moved functions no longer members of CWallet and CWalletTx so that we don't have implementations of class members across different files. My last question is how is this refactor useful to us? I'm not convinced this change was particularly beneficial especially since it forces a rebase of ~every wallet PR. But it's a bit too late to be asking that and should've asked that prior to #21207 being merged, which also means that I should've reviewed it.

I was confused by the mention of #21206 in the OP and thought this was related to that, but I think it just makes the implementation a bit easier and isn't strictly necessary.

ryanofsky · 2021-09-02T02:35:49Z

My last question is how is this refactor useful to us?

Like you mentioned, this PR post-moveonly cleanup after #21207, and the overall motivation is described there. The goal of that PR and this one were to increase legibility and improve organization of wallet code. Before #21207 balance computing code was mixed with syncing code and coin selection code and all of this was in random order. After #21207 the code is grouped and organized into different files, separating spending and balance tracking code in different files, with the lower level functions in each file organized first and higher level functions following the lower level functions they call. In this renaming PR after the MOVEONLY PR, circular dependencies are dropped, functions with overloaded names are given distinct names, pointless CWalletTx wallet backpointers are dropped, and functions are named and arranged based on what inputs they are operate on, what they are used for, whether they cache state, how they related to other functions, and things like that, instead of arbitrary things like what convention somebody felt like using in some isolated PR 6 years ago or whether someone flipped a coin and felt like attaching a method to the CWallet class or the CWalletTx class.

All of the changes made in these two PRs are very conservative and all are very minor. In almost every case a function name is changing it is just getting longer and more descriptive, and in cases where words are actually different just they are becoming more consistent with other functions. Also, wallet.function() is becoming function(wallet) in many cases. In short, this a boring collection of renames that shouldn't affect you very much if you are a wallet expert, but should make the wallet more modular and understandable if you are trying to understand the code and don't have it memorized. This is a result of me spending a few days looking at dependencies between wallet functions, moving the functions that are related and depend on each other into different files, and then smoothing out a few obvious inconsistencies in naming.

achow101 · 2021-09-02T02:52:34Z

ACK b11a195

Sjors

utACK b11a195

Sjors · 2021-09-02T09:48:28Z

src/wallet/wallet.h

    bool IsMine(const CTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    /** should probably be renamed to IsRelevantToMe */
    bool IsFromMe(const CTransaction& tx) const;
    CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
-    /** Returns whether all of the inputs match the filter */
-    bool IsAllFromMe(const CTransaction& tx, const isminefilter& filter) const;
-    CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const;


Or maybe in a (more compact) followup.

ryanofsky

Thanks for reviews!

re: #22100 (comment)

I'm not convinced this change was particularly beneficial especially since it forces a rebase of ~every wallet PR.

I'm struggling with this comment (which I guess was added in editing) because I don't have a great perspective on this problem. The list of conflicts here does not seem very long #22100 (comment), and this seems like the easiest type of rebase conflict to resolve since there are no code logic changes or even moves, just renames.

But maybe the list of conflicts is not a great metric because with function renames there could be silent conflicts in case PRs are adding new calls to a renamed function. Also my perspective on rebases is totally warped because I do them so regularly and follow a procedure to resolve them (manually apply the change from one side of a three way conflict section to the other two sides, and then run a script to collapse identical sides of conflict sections) to ensure changes weren't missed. Also I rely very heavily on rerere. Rebases for me seem mindless and not very important, but I know that makes me a weirdo because rebases are everybody else's favorite thing to complain about.

In defense of rebases for this particular change, I've been hearing for years wallet code is a giant hairball, everything is crammed into a single giant class in one wallet.cpp file. #17261 made a major dent in this, pulling key management out into a separate file. And #21207 untangles the rest of the hairball pulling balance tracking and spending code out, just leaving syncing behind. If feels like if changes like these are done thoughtfully and infrequently then benefits will outweigh the costs, but if I am mispricing costs of rebases, who know what other costs I'm mispricing too. I don't know. That is my thinking about this.

ryanofsky · 2021-09-02T11:16:54Z

src/wallet/wallet.h

    bool IsMine(const CTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    /** should probably be renamed to IsRelevantToMe */
    bool IsFromMe(const CTransaction& tx) const;
    CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
-    /** Returns whether all of the inputs match the filter */
-    bool IsAllFromMe(const CTransaction& tx, const isminefilter& filter) const;
-    CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const;


re: #22100 (comment)

How come you're able to move GetCredit but not GetDebit?

I would like to move it, but it's kept in place for now to avoid circular dependencies.

Given that #20773 is merged, could the change to GetDebit be done now?

Or maybe in a (more compact) followup.

Yes #20733 does make this easier, and yes this would make sense to do as a followup. Doing it in this PR wouldn't make great sense because this PR is not moving code just doing naming/declaration cleanups after a previous move. (Also this is not a tiny change, just in terms of number of lines). Will see what this looks like and post a followup.

Sjors · 2021-09-02T12:00:08Z

It would be useful to see which PR's are impacted by silent merge conflicts. Afaik most of the big upcoming work, like taproot, doesn't touch this. I also think this reorganization makes future wallet improvements less scary.

meshcollider

I haven't given the code a really in-depth review, but have read through it and everything looks good and sane to me. I've also built and run the unit + functional tests.

light ACK b11a195

…es added …

Adjustments to the wallet code with name changes (like GetNameCredit/Debit, or CreateTransaction with input) made necessary with the upstream change bitcoin/bitcoin#22100. Fixed some of the regtests that were broken by namecoin#453.

fanquake added Refactoring Wallet labels May 30, 2021

hobhsy approved these changes May 30, 2021

View reviewed changes

ryanofsky mentioned this pull request May 30, 2021

refactor: Make CWalletTx sync state type-safe #21206

Merged

This was referenced May 30, 2021

wallet: indicate whether a transaction is in the mempool #21260

Closed

wallet: Properly support a wallet id #20205

Closed

Wallet passive startup #15719

Closed

[RPC] Include coinbase transactions in receivedby RPCs #14707

Merged

fanquake requested review from fjahr, meshcollider, promag and Sjors May 31, 2021 02:22

doianmai2020 approved these changes May 31, 2021

View reviewed changes

Sjors approved these changes May 31, 2021

View reviewed changes

ryanofsky commented May 31, 2021

View reviewed changes

promag reviewed May 31, 2021

View reviewed changes

This was referenced Jun 1, 2021

gui: grey out used address in address book #17355

Closed

wallet: Introduce SelectionResult for encapsulating a coin selection solution #22019

Merged

refactor: remove ::vpwallets and related global variables #19101

Merged

DrahtBot added the Needs rebase label Jun 9, 2021

ryanofsky force-pushed the pr/txfun branch from a3f6230 to 642843a Compare June 11, 2021 19:59

DrahtBot removed the Needs rebase label Jun 11, 2021

DrahtBot mentioned this pull request Jun 12, 2021

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

Zero-1729 reviewed Aug 24, 2021

View reviewed changes

DrahtBot mentioned this pull request Aug 26, 2021

refactor: use CWallet const shared pointers in dump{privkey,wallet} #22805

Merged

meshcollider requested a review from achow101 September 1, 2021 05:12

DrahtBot added the Needs rebase label Sep 1, 2021

ryanofsky force-pushed the pr/txfun branch from 54faec8 to b11a195 Compare September 1, 2021 14:59

DrahtBot removed the Needs rebase label Sep 1, 2021

fanquake requested review from fjahr and meshcollider and removed request for fjahr and meshcollider September 2, 2021 02:53

Sjors approved these changes Sep 2, 2021

View reviewed changes

ryanofsky commented Sep 2, 2021

View reviewed changes

meshcollider reviewed Sep 3, 2021

View reviewed changes

meshcollider merged commit 629c4ab into bitcoin:master Sep 3, 2021

promag mentioned this pull request Sep 4, 2021

wallet: Avoid locking cs_wallet recursively #19833

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 4, 2021

Merge bitcoin#22100: refactor: Clean up new wallet spend, receive fil…

59e0e11

…es added …

bitcoin locked as resolved and limited conversation to collaborators Sep 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Clean up new wallet spend, receive files added #21207 #22100

refactor: Clean up new wallet spend, receive files added #21207 #22100

ryanofsky commented May 30, 2021 •

edited

DrahtBot commented May 30, 2021 •

edited

Sjors left a comment

Sjors May 31, 2021

ryanofsky May 31, 2021

achow101 Sep 1, 2021

Sjors Sep 2, 2021

ryanofsky Sep 2, 2021

ryanofsky left a comment

ryanofsky May 31, 2021

promag left a comment

promag May 31, 2021

ryanofsky May 31, 2021

promag May 31, 2021

ryanofsky May 31, 2021

promag May 31, 2021

ryanofsky commented Jun 11, 2021

fjahr commented Jun 13, 2021

fjahr commented Aug 23, 2021

Zero-1729 left a comment

meshcollider commented Sep 1, 2021

ryanofsky commented Sep 1, 2021

achow101 commented Sep 1, 2021

ryanofsky commented Sep 1, 2021 •

edited

achow101 commented Sep 1, 2021

ryanofsky commented Sep 1, 2021

achow101 commented Sep 2, 2021 •

edited

ryanofsky commented Sep 2, 2021

achow101 commented Sep 2, 2021

Sjors left a comment

Sjors Sep 2, 2021

ryanofsky left a comment •

edited

ryanofsky Sep 2, 2021

Sjors commented Sep 2, 2021

meshcollider left a comment

refactor: Clean up new wallet spend, receive files added #21207 #22100

refactor: Clean up new wallet spend, receive files added #21207 #22100

Conversation

ryanofsky commented May 30, 2021 • edited

DrahtBot commented May 30, 2021 • edited

Conflicts

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky commented Jun 11, 2021

fjahr commented Jun 13, 2021

fjahr commented Aug 23, 2021

Zero-1729 left a comment

Choose a reason for hiding this comment

meshcollider commented Sep 1, 2021

ryanofsky commented Sep 1, 2021

achow101 commented Sep 1, 2021

ryanofsky commented Sep 1, 2021 • edited

achow101 commented Sep 1, 2021

ryanofsky commented Sep 1, 2021

achow101 commented Sep 2, 2021 • edited

ryanofsky commented Sep 2, 2021

achow101 commented Sep 2, 2021

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Sep 2, 2021

meshcollider left a comment

Choose a reason for hiding this comment

ryanofsky commented May 30, 2021 •

edited

DrahtBot commented May 30, 2021 •

edited

ryanofsky commented Sep 1, 2021 •

edited

achow101 commented Sep 2, 2021 •

edited

ryanofsky left a comment •

edited