MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/txmove changing 10 files +1990 −1902
  1. ryanofsky commented at 5:30 am on February 17, 2021: member

    This commit just moves function without making any changes. It can be reviewed with git log -p -n1 --color-moved=dimmed_zebra

    Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems.

    This moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files:

    • transaction.cpp/.h - CWalletTx and CMerkleTx class definitions
    • receive.cpp/.h - functions checking received transactions and computing balances
    • spend.cpp/.h - functions creating transactions and finding spendable coins

    After #20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to:

    • sync.cpp/.h - functions handling chain notifications and rescanning

    This commit arranges receive.cpp and spend.cpp functions in dependency order so it’s possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated code.

    Followup commit “refactor: Detach wallet transaction methods” in #21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.

  2. fanquake added the label Wallet on Feb 17, 2021
  3. DrahtBot commented at 10:23 am on February 17, 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:

    • #22009 (wallet: Decide which coin selection solution to use based on waste metric by achow101)
    • #22008 (wallet: Cleanup and refactor CreateTransactionInternal by achow101)
    • #21365 (Basic Taproot signing support for descriptor wallets by sipa)
    • #21312 (wallet: remove lock during listaddressgroupings by vladyslavstartsev)
    • #21283 (Implement BIP 370 PSBTv2 by achow101)
    • #20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)
    • #20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)
    • #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
    • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

    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.

  4. jonatack commented at 11:11 am on February 18, 2021: member
    ACK 2a855bf0927908c7c1100ef763a44e3b82432c8f light verification of move-only and review of headers and makefile, local debug build and unit tests clean, the re-organisation choices of which code to move where look coherent
  5. ryanofsky commented at 6:14 pm on February 18, 2021: member

    This is from a hacky, stolen script[1], but here are commands that can be used verify the change is move only. It just prints all the lines that were NOT moved (comment lines and #include lines)

    0diff_lines=$(git diff HEAD^..HEAD | grep -v '^\(---\|+++\|@@ \)'| grep '^\([><] \)\|[+-]' | sed 's/^+/> /;s/^-/< /')
1while IFS= read -r line || [ -n "$line" ]
2do
3    contents="${line:2}"
4    count_removes="$(grep -cFxe "< $contents" <<< "$diff_lines" || true)"
5    count_adds="$(grep -cFxe "> $contents" <<< "$diff_lines" || true)"
6    test -z "$contents" || test "$count_removes" -eq "$count_adds" || echo "$line"
7done <<< "$diff_lines"

    [1] script stolen from https://github.com/l0b0/diff-ignore-moved-lines/blob/master/diff-ignore-moved-lines.sh

  6. DrahtBot added the label Needs rebase on Feb 23, 2021
  7. ryanofsky force-pushed on Mar 4, 2021
  8. ryanofsky commented at 0:22 am on March 4, 2021: member
    Rebased 2a855bf0927908c7c1100ef763a44e3b82432c8f -> 01f4bb26935e431d44503a31006300196e0d1360 (pr/txmove.1 -> pr/txmove.2, compare) due to conflict with #16546 Rebased 01f4bb26935e431d44503a31006300196e0d1360 -> 74d45298e59dc15dd708c68bac502c12a5d93ab5 (pr/txmove.2 -> pr/txmove.3, compare) due to conflict with #20536
  9. DrahtBot removed the label Needs rebase on Mar 4, 2021
  10. DrahtBot added the label Needs rebase on Mar 8, 2021
  11. ryanofsky force-pushed on Mar 8, 2021
  12. DrahtBot removed the label Needs rebase on Mar 9, 2021
  13. ryanofsky commented at 2:27 pm on March 9, 2021: member

    Requesting concept ACKs for this PR!

    No are no code changes, only moves, so the review should be trivial. Also, this is easy for me to rebase, so if anyone is concerned about conflicts and wants this delayed until another change is merged, that is no problem. I’m going to try to review PRs on the conflict list #21207 (comment), but it’d be good to know if any PRs in particular are a concern!

  14. promag commented at 2:30 pm on March 9, 2021: member
    I’ve ACK this change on the follow up PR.
  15. practicalswift commented at 10:41 pm on March 9, 2021: contributor
    Concept ACK: less monolithic is more
  16. DrahtBot added the label Needs rebase on Mar 17, 2021
  17. ryanofsky force-pushed on Mar 21, 2021
  18. DrahtBot removed the label Needs rebase on Mar 21, 2021
  19. ryanofsky commented at 9:40 pm on March 22, 2021: member
    Rebased 74d45298e59dc15dd708c68bac502c12a5d93ab5 -> 7c188f3508cfca4cf4ca8bf3ca3b266b147bb854 (pr/txmove.3 -> pr/txmove.4, compare) due to conflict with #21083
  20. DrahtBot added the label Needs rebase on Apr 13, 2021
  21. fanquake commented at 1:40 pm on April 13, 2021: member
    @achow101 @Sjors @instagibbs @Xekyo @meshcollider @S3RK do any of you have a (conceptual) opinion here?
  22. ryanofsky force-pushed on Apr 13, 2021
  23. ryanofsky commented at 2:40 pm on April 13, 2021: member
    Rebased 7c188f3508cfca4cf4ca8bf3ca3b266b147bb854 -> a71a7db9a9eca72a391426acd8555d97980b018d (pr/txmove.4 -> pr/txmove.5, compare) due to conflict with #21467
  24. DrahtBot removed the label Needs rebase on Apr 13, 2021
  25. Sjors commented at 7:42 pm on April 13, 2021: member
    I’m always happy to see large files split up. Will give it some more thought.
  26. instagibbs commented at 11:02 pm on April 13, 2021: member
    I like the effort being put in, would have to think more about it before approach acking
  27. ryanofsky commented at 4:46 am on April 14, 2021: member

    Note that commit 3e0a7ba411a01160aa411c7aa78f718c2feed056 from #21206 is the spiritual successor to this PR and I’ll probably break it off from that PR if this one gets merged.

    This PR does obvious high-level moves that can be trivially reviewed, and then that commit does tedious renames to reflect the moved-to locations.

    Since I requested approach ACKS, I can summarize what the approach taken here was: Just starting from a big file full of functions, then moving groups of functions that call each other into smaller files. I put a lot of thought and effort into the moves, grouping related functions together while keeping the functions strictly sorted in dependency order within each file, avoiding any circular dependencies between files, and not changing any code at all. Things can move around freely after this, but I think the result is a great starting point for less monolithic and more comprehensible wallet code.

  28. S3RK commented at 7:38 am on April 14, 2021: member
    I’m in favour of splitting big files/classes. Will take a closer look and come back with more thoughts
  29. S3RK commented at 7:50 am on April 20, 2021: member

    grouping related functions together while keeping the functions strictly sorted in dependency order within each file, avoiding any circular dependencies between files

    This is an instrumental ground work, greatly appreciate the effort.

    What I’m missing here is an ideal end state and #21206 doesn’t help with this either. A purely procedural style as proposed in 3e0a7ba is fine as an intermediate state, but it’s not good enough for the long run.

    Let’s take CWallet::GetBalance as an example for no particular reason. On the first look, it seems like it should stay in CWallet class IMHO. And in that case there is no need to move to receive.cpp. Of course, there are multiple ways to design this, so I’m curious how do you envision this.

    Do you think a class diagram for an ideal final state would be useful here?

  30. ryanofsky commented at 1:50 pm on April 21, 2021: member

    re: #21207 (comment)

    Thanks for looking into this.

    What I’m missing here is an ideal end state

    The idea is instead of implementing all wallet functionality in one sprawling and disorganized class, CWallet, CWallet is just a collection of keys and transactions, and higher level functionality like spending, receiving, and syncing implemented on top of it separately. This PR and followup commit 3e0a7ba411a01160aa411c7aa78f718c2feed056 refactor: Detach wallet transaction methods which is currently part of #21206 achieve that goal, except for the syncing, and #15719 begins to address the syncing.

    I’m not someone with strong opinions about object oriented style, but if you are interested in a justification for this approach the “Non-Member Functions Improve Encapsulation” idea https://www.drdobbs.com/cpp/how-non-member-functions-improve-encapsu/184401197, http://www.gotw.ca/gotw/084.htm could be helpful. #21207 and 3e0a7ba411a01160aa411c7aa78f718c2feed056 are just grouping related functions together instead mixing them up, though, so no need to get very lofty about them.

    A purely procedural style as proposed in 3e0a7ba is fine as an intermediate state, but it’s not good enough for the long run.

    It doesn’t matter to me if wallet spending, receiving, and syncing code is object oriented or procedural. Currently it’s procedural code that’s thrown into a giant class. The followup commit 3e0a7ba411a01160aa411c7aa78f718c2feed056 makes the procedural form more explicit, but there’s nothing substantive about the commit, it’s pure renaming.

    Let’s take CWallet::GetBalance as an example for no particular reason. On the first look, it seems like it should stay in CWallet class IMHO. And in that case there is no need to move to receive.cpp. Of course, there are multiple ways to design this, so I’m curious how do you envision this.

    Do you think a class diagram for an ideal final state would be useful here?

    I don’t have any objections to someone rewriting GetBalance code in an object oriented style, but I don’t have plans to change GetBalance. I just want to consolidate it in wallet/receive.cpp so it’s not scattered all over wallet/wallet.cpp.

  31. Sjors commented at 8:50 am on April 22, 2021: member
    utACK a71a7db9a9eca72a391426acd8555d97980b018d
  32. promag commented at 12:23 pm on April 22, 2021: member
    Concept ACK, will review and #21206 again.
  33. S3RK commented at 7:48 am on April 26, 2021: member

    @ryanofsky Thanks for the pointers and expanded explanation of your reasoning. I’m now convinced this a clear improvement. Approach ACK, will do code review soon.

    UPD 2021-05-19: have limited time availability now, but I intend to get back to the review next month

  34. DrahtBot added the label Needs rebase on Apr 29, 2021
  35. ryanofsky force-pushed on Apr 30, 2021
  36. ryanofsky commented at 4:30 pm on April 30, 2021: member
    Rebased a71a7db9a9eca72a391426acd8555d97980b018d -> 4984c4548cd3d6fb0296cad39bcec7ff3be7cd76 (pr/txmove.5 -> pr/txmove.6, compare) due to conflict with #21759
  37. ryanofsky commented at 4:35 pm on April 30, 2021: member

    Approach ACK, will do code review soon.

    Thanks all reviewers, and just a reminder using --color-moved=dimmed_zebra mentioned in the description should make this a lot easier to review and the moveonly verify script #21207 (comment) could also help as another check.

  38. DrahtBot removed the label Needs rebase on Apr 30, 2021
  39. Sjors commented at 5:14 pm on April 30, 2021: member
    re-utACK 4984c4548cd3d6fb0296cad39bcec7ff3be7cd76
  40. DrahtBot added the label Needs rebase on May 10, 2021
  41. ryanofsky force-pushed on May 20, 2021
  42. ryanofsky commented at 9:57 pm on May 20, 2021: member
    Rebased 4984c4548cd3d6fb0296cad39bcec7ff3be7cd76 -> 264b945fa7f10b418d8acaeebe90c766789720c2 (pr/txmove.6 -> pr/txmove.7, compare) due to conflicts with #21359 and #21910
  43. DrahtBot removed the label Needs rebase on May 20, 2021
  44. Sjors commented at 1:07 pm on May 21, 2021: member
    re-utACK 264b945fa7f10b418d8acaeebe90c766789720c2
  45. DrahtBot added the label Needs rebase on May 25, 2021
  46. ryanofsky force-pushed on May 25, 2021
  47. ryanofsky commented at 4:45 pm on May 25, 2021: member
    Rebased 264b945fa7f10b418d8acaeebe90c766789720c2 -> ae93b95a6822be5360544b5c2bb6dacbd8d3b10d (pr/txmove.7 -> pr/txmove.8, compare) due to conflict with #17331
  48. Sjors commented at 4:50 pm on May 25, 2021: member
    re-utACK ae93b95 (checked that the range-diff is just changes inside the conflicting functions and that git show --color-moved=dimmed_zebra still only moves entire functions).
  49. DrahtBot removed the label Needs rebase on May 25, 2021
  50. practicalswift commented at 8:56 pm on May 25, 2021: contributor
    cr ACK ae93b95a6822be5360544b5c2bb6dacbd8d3b10d: dimmed zebra looks correct
  51. DrahtBot added the label Needs rebase on May 26, 2021
  52. promag commented at 8:22 am on May 26, 2021: member
    ACK after rebase due to #22042.
  53. MOVEONLY: CWallet transaction code out of wallet.cpp/.h 
    This commit just moves functions without making any changes. It can be
reviewed with `git log -p -n1 --color-moved=dimmed_zebra`

Motivation for this change is to make wallet.cpp/h less monolithic and
start to make wallet transaction state tracking comprehensible so bugs
in
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
can be fixed safely without introducing new problems.

This commit moves wallet classes and methods that deal with transactions
out of wallet.cpp/.h into better organized files:

- transaction.cpp/.h - CWalletTx and CMerkleTx class definitions
- receive.cpp/.h - functions checking received transactions and computing balances
- spend.cpp/.h - functions creating transactions and finding spendable coins

After #20773, when loading is separated from syncing it will also be
possible to move more wallet.cpp/.h functions to:

- sync.cpp/.h - functions handling chain notifications and rescanning

This commit arranges receive.cpp and spend.cpp functions in dependency
order so it's possible to skim receive.cpp and get an idea of how
computing balances works, and skim spend.cpp and get an idea of how
transactions are created, without having to jump all over wallet.cpp
where functions are not in order and there is a lot of unrelated code.

Followup commit "refactor: Detach wallet transaction methods" in
https://github.com/bitcoin/bitcoin/pull/21206 follows up this PR and
tweaks function names and arguments to reflect new locations. The two
commits are split into separate PRs because this commit is more work to
maintain and less work to review, while the other commit is less work to
maintain and more work to review, so hopefully this commit can be merged
earlier.
    c7bd5842e4
  54. ryanofsky force-pushed on May 26, 2021
  55. ryanofsky commented at 1:09 pm on May 26, 2021: member
    Rebased ae93b95a6822be5360544b5c2bb6dacbd8d3b10d -> c7bd5842e467c4fc286399379572bbdec6b26a4f (pr/txmove.8 -> pr/txmove.9, compare) due to conflicts with #22042 and #18418
  56. DrahtBot removed the label Needs rebase on May 26, 2021
  57. promag commented at 5:43 pm on May 26, 2021: member

    Code review ACK c7bd5842e467c4fc286399379572bbdec6b26a4f, verified move only claim.

    This is base for #21206 which IMO is an awesome change. Reviewers might want to check that to get motivation for this one.

  58. Sjors commented at 5:50 pm on May 26, 2021: member

    re-utACK c7bd5842e467c4fc286399379572bbdec6b26a4f

    In case we want more review / concept ACKs: cc @achow101, @instagibbs, @fjahr

  59. fjahr commented at 9:43 pm on May 26, 2021: member

    utACK c7bd5842e467c4fc286399379572bbdec6b26a4f

    I reviewed using git show --color-moved=dimmed_zebra and manually confirmed the few instances were git was acting funny with the diffs. Move-only confirmed.

  60. fanquake requested review from meshcollider on May 27, 2021
  61. meshcollider commented at 9:57 am on May 30, 2021: contributor
    Dimmed-zebra-check and functional test run ACK c7bd5842e467c4fc286399379572bbdec6b26a4f
  62. meshcollider merged this on May 30, 2021
  63. meshcollider closed this on May 30, 2021
  64. domob1812 referenced this in commit c750aba537 on May 31, 2021
  65. doianmai2020 commented at 12:55 pm on May 31, 2021: none
    .
  66. fanquake locked this on May 31, 2021


ryanofsky DrahtBot jonatack promag practicalswift fanquake Sjors instagibbs S3RK fjahr meshcollider doianmai2020

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me