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

ryanofsky commented at 11:09 am on May 30, 2021: member

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

fanquake added the label Refactoring on May 30, 2021

fanquake added the label Wallet on May 30, 2021

hobhsy approved

DrahtBot commented at 4:05 pm on May 30, 2021: member

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.

fanquake requested review from fjahr on May 31, 2021

fanquake requested review from meshcollider on May 31, 2021

fanquake requested review from promag on May 31, 2021

fanquake requested review from Sjors on May 31, 2021

doianmai2020 approved

in src/wallet/wallet.h:700 in a3f623035a outdated

655     /** should probably be renamed to IsRelevantToMe */
656     bool IsFromMe(const CTransaction& tx) const;
657     CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
658-    /** Returns whether all of the inputs match the filter */
659-    bool IsAllFromMe(const CTransaction& tx, const isminefilter& filter) const;
660-    CAmount GetCredit(const CTransaction& tx, const isminefilter& filter) const;

Sjors commented at 4:29 pm on May 31, 2021:

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

ryanofsky commented at 5:30 pm on May 31, 2021:

re: #22100 (review)

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.

achow101 commented at 8:10 pm on September 1, 2021:

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

Sjors commented at 9:48 am on September 2, 2021:

Or maybe in a (more compact) followup.

ryanofsky commented at 11:16 am on September 2, 2021:

re: #22100 (review)

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 approved

Sjors commented at 4:50 pm on May 31, 2021: member

utACK a3f623035a2653049d098f43e55b9e01850cb16c

Review hint:

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

ryanofsky commented at 5:53 pm on May 31, 2021: member

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

in src/wallet/receive.h:26 in a3f623035a outdated

21+bool ScriptIsChange(const CWallet& wallet, const CScript& script) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
22+bool OutputIsChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
23+CAmount OutputGetChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
24+CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx);
25+
26+CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter);

promag commented at 7:39 pm on May 31, 2021:

Comparing to 4e11f88320b644b67db55fe737815451ca7d0681 (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?

ryanofsky commented at 8:21 pm on May 31, 2021:

re: #22100 (review)

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.

promag commented at 9:02 pm on May 31, 2021:

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.

ryanofsky commented at 10:54 pm on May 31, 2021:

re: #22100 (review)

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.

promag commented at 11:01 pm on May 31, 2021:

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

promag commented at 7:44 pm on May 31, 2021: member

Code review ACK a3f623035a2653049d098f43e55b9e01850cb16c.

Maybe @achow101 should review this.

DrahtBot added the label Needs rebase on Jun 9, 2021

ryanofsky force-pushed on Jun 11, 2021

ryanofsky commented at 8:00 pm on June 11, 2021: member

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

DrahtBot removed the label Needs rebase on Jun 11, 2021

fjahr commented at 8:02 pm on June 13, 2021: member

Code review ACK 642843a193c237fc7a21dffc9fbbb3e10ab8e50d

DrahtBot added the label Needs rebase on Jun 17, 2021

ryanofsky force-pushed on Jun 17, 2021

ryanofsky commented at 1:18 pm on June 17, 2021: member

Rebased 642843a193c237fc7a21dffc9fbbb3e10ab8e50d -> b69d82094bccdf929f7d483c5031c55e6e40f103 (pr/txfun.2 -> pr/txfun.3, compare) for no reason Rebased b69d82094bccdf929f7d483c5031c55e6e40f103 -> 33af67edbd902e9b9c3862c3a3066798cac1a33d (pr/txfun.3 -> pr/txfun.4, compare) due to conflict with #21935

DrahtBot removed the label Needs rebase on Jun 17, 2021

theStack commented at 11:05 am on July 18, 2021: member

Concept ACK

DrahtBot added the label Needs rebase on Jul 27, 2021

ryanofsky force-pushed on Aug 13, 2021

ryanofsky commented at 1:57 am on August 13, 2021: member

Rebased 33af67edbd902e9b9c3862c3a3066798cac1a33d -> 67f8c262281feb3599ec2dab13439edd520bb5e6 (pr/txfun.4 -> pr/txfun.5, compare) due to conflict with #22155

DrahtBot removed the label Needs rebase on Aug 13, 2021

Zero-1729 commented at 9:40 am on August 13, 2021: contributor

crACK 67f8c262281feb3599ec2dab13439edd520bb5e6

fanquake deleted a comment on Aug 14, 2021

fanquake deleted a comment on Aug 16, 2021

DrahtBot added the label Needs rebase on Aug 19, 2021

ryanofsky force-pushed on Aug 19, 2021

ryanofsky commented at 1:24 pm on August 19, 2021: member

Rebased 67f8c262281feb3599ec2dab13439edd520bb5e6 -> 54faec818254453f8c0813f60be0164afb26558a (pr/txfun.5 -> pr/txfun.6, compare) due to conflict with #19101

DrahtBot removed the label Needs rebase on Aug 19, 2021

fjahr commented at 10:08 pm on August 23, 2021: member

Code review re-ACK 54faec818254453f8c0813f60be0164afb26558a

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

Zero-1729 commented at 7:08 am on August 24, 2021: contributor

re-crACK 54faec8

LGTM, only resolved merge conflicts since last review.

meshcollider commented at 5:12 am on September 1, 2021: contributor

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

meshcollider requested review from achow101 on Sep 1, 2021

DrahtBot added the label Needs rebase on Sep 1, 2021

refactor: Detach wallet transaction methods (followup for move-only)

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.

b11a195ef4

ryanofsky force-pushed on Sep 1, 2021

ryanofsky commented at 2:59 pm on September 1, 2021: member

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

Thanks! And no problem, conflicts were trivial

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

DrahtBot removed the label Needs rebase on Sep 1, 2021

achow101 commented at 8:09 pm on September 1, 2021: member

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 commented at 9:34 pm on September 1, 2021: member

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 commented at 10:11 pm on September 1, 2021: member

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 commented at 11:13 pm on September 1, 2021: member

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 commented at 1:43 am on September 2, 2021: member

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 commented at 2:35 am on September 2, 2021: member

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 commented at 2:52 am on September 2, 2021: member

ACK b11a195ef450bd138aa03204a5e74fdd3ddced26

fanquake removed review request from fjahr on Sep 2, 2021

fanquake removed review request from meshcollider on Sep 2, 2021

fanquake requested review from fjahr on Sep 2, 2021

fanquake requested review from meshcollider on Sep 2, 2021

Sjors approved

Sjors commented at 9:58 am on September 2, 2021: member

utACK b11a195ef450bd138aa03204a5e74fdd3ddced26

ryanofsky commented at 11:54 am on September 2, 2021: member

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.

Sjors commented at 12:00 pm on September 2, 2021: member

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 commented at 9:21 am on September 3, 2021: contributor

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 b11a195ef450bd138aa03204a5e74fdd3ddced26

meshcollider merged this on Sep 3, 2021

meshcollider closed this on Sep 3, 2021

sidhujag referenced this in commit 59e0e11547 on Sep 4, 2021

domob1812 referenced this in commit 8ab1f6d49a on Sep 7, 2021

DrahtBot locked this on Sep 3, 2022

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

Conflicts