Remove CWalletTx merging logic from AddToWallet #9381

pull ryanofsky wants to merge 5 commits into bitcoin:master from ryanofsky:pr/atw-nomerge changing 9 files +126 −134
  1. ryanofsky commented at 11:00 pm on December 19, 2016: member

    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.

  2. fanquake added the label Refactoring on Dec 19, 2016
  3. fanquake added the label Wallet on Dec 19, 2016
  4. ryanofsky force-pushed on Jan 6, 2017
  5. ryanofsky force-pushed on Jan 25, 2017
  6. ryanofsky commented at 6:30 pm on January 25, 2017: member
    Rebased ed457a39ebaa672cce21622d3af422fe50730580 -> b1ac3cff00a51c0b8965b7a85fa489c98409ddff to resolve merge conflicts with bumpfee (#8456).
  7. ryanofsky commented at 6:48 pm on January 25, 2017: member

    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.

  8. TheBlueMatt commented at 7:26 pm on January 25, 2017: member

    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 https://github.com/bitcoin/bitcoin/pull/9381#issuecomment-275196684, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnoHu0xZqwtESQp3_rXWoR3OurDnNzgks5rV5kPgaJpZM4LRR4W.

  9. TheBlueMatt commented at 10:17 pm on January 25, 2017: member

    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.

  10. ryanofsky force-pushed on Feb 2, 2017
  11. ryanofsky referenced this in commit 53e5acc717 on Feb 2, 2017
  12. ryanofsky referenced this in commit dd12997a4f on Feb 2, 2017
  13. ryanofsky force-pushed on Feb 2, 2017
  14. ryanofsky renamed this:
    Remove CWalletTx merging logic from AddToWallet
    Remove CWalletTx merging logic from AddToWallet (on top of #9673)
    on Feb 2, 2017
  15. ryanofsky commented at 11:51 pm on February 2, 2017: member

    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).

  16. ryanofsky force-pushed on Feb 3, 2017
  17. TheBlueMatt commented at 6:46 pm on February 3, 2017: member

    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.

  18. ryanofsky renamed this:
    Remove CWalletTx merging logic from AddToWallet (on top of #9673)
    Remove CWalletTx merging logic from AddToWallet (on top of #9680)
    on Feb 3, 2017
  19. ryanofsky commented at 7:10 pm on February 3, 2017: member

    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.

  20. ryanofsky force-pushed on Feb 6, 2017
  21. ryanofsky renamed this:
    Remove CWalletTx merging logic from AddToWallet (on top of #9680)
    WIP: Remove CWalletTx merging logic from AddToWallet (on top of #9680)
    on Feb 6, 2017
  22. ryanofsky force-pushed on Mar 3, 2017
  23. ryanofsky force-pushed on Jun 2, 2017
  24. ryanofsky force-pushed on Jun 19, 2017
  25. ryanofsky force-pushed on Jul 18, 2017
  26. ryanofsky force-pushed on Aug 14, 2017
  27. ryanofsky force-pushed on Aug 16, 2017
  28. ryanofsky force-pushed on Aug 23, 2017
  29. ryanofsky force-pushed on Aug 25, 2017
  30. ryanofsky force-pushed on Sep 20, 2017
  31. ryanofsky force-pushed on Oct 19, 2017
  32. ryanofsky referenced this in commit 242bf31d84 on Nov 20, 2017
  33. ryanofsky force-pushed on Nov 20, 2017
  34. ryanofsky referenced this in commit 0fdca91f4b on Nov 20, 2017
  35. ryanofsky force-pushed on Nov 20, 2017
  36. ryanofsky force-pushed on Nov 20, 2017
  37. ryanofsky force-pushed on Nov 20, 2017
  38. ryanofsky force-pushed on Nov 21, 2017
  39. ryanofsky referenced this in commit e6cb4b0193 on Dec 1, 2017
  40. ryanofsky force-pushed on Dec 1, 2017
  41. ryanofsky renamed this:
    WIP: Remove CWalletTx merging logic from AddToWallet (on top of #9680)
    Remove CWalletTx merging logic from AddToWallet (on top of #9680)
    on Dec 1, 2017
  42. ryanofsky renamed this:
    Remove CWalletTx merging logic from AddToWallet (on top of #9680)
    Remove CWalletTx merging logic from AddToWallet
    on Dec 1, 2017
  43. ryanofsky commented at 10:31 pm on December 1, 2017: member

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

    Implemented this suggestion. (New function is CWallet::AddTransaction). I also went ahead and disabled the CWalletTx copy constructor. Interestingly this uncovered a bunch of cases where we were unwittingly copying wallet transactions in loops due to writing:

    0for (const std::pair<uint256, CWalletTx>& pairWtx : pwallet->mapWallet)

    instead of:

    0for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet)
  44. ryanofsky referenced this in commit 7b44eb8e15 on Dec 14, 2017
  45. ryanofsky force-pushed on Dec 14, 2017
  46. ryanofsky referenced this in commit d17857b2fb on Dec 15, 2017
  47. ryanofsky force-pushed on Dec 15, 2017
  48. ryanofsky referenced this in commit cba74b9b48 on Dec 31, 2017
  49. ryanofsky force-pushed on Dec 31, 2017
  50. ryanofsky referenced this in commit 2458b102f7 on Dec 31, 2017
  51. ryanofsky force-pushed on Dec 31, 2017
  52. ryanofsky referenced this in commit a9db0f911c on Jan 25, 2018
  53. ryanofsky force-pushed on Jan 25, 2018
  54. ryanofsky referenced this in commit 8e3dd3be1a on Feb 2, 2018
  55. ryanofsky force-pushed on Feb 2, 2018
  56. ryanofsky force-pushed on Feb 12, 2018
  57. in src/qt/walletmodeltransaction.cpp:22 in 8e3dd3be1a outdated 
    24 {
25     return recipients;
26 }
27 
28-CWalletTx *WalletModelTransaction::getTransaction() const
29+CTransactionRef* WalletModelTransaction::getTransaction()
    kallewoof commented at 8:33 am on February 22, 2018:
    CTransactionRef is itself a pointer, so unless you intend for the caller to be able to modify what it points to, I think simply returning a CTransactionRef is better here. Every use case also seems to *-dereference it.
    ryanofsky commented at 7:39 pm on March 13, 2018:

    #9381 (review)

    CTransactionRef is itself a pointer, so unless you intend for the caller to be able to modify what it points to, I think simply returning a CTransactionRef is better here. Every use case also seems to *-dereference it.

    Good idea, done in 64f182fa587f35081ed8026b44e2a36010364153.

  58. ryanofsky referenced this in commit ae8fd3792b on Feb 23, 2018
  59. ryanofsky force-pushed on Feb 23, 2018
  60. ryanofsky force-pushed on Mar 5, 2018
  61. ryanofsky referenced this in commit e54fbd8c00 on Mar 7, 2018
  62. ryanofsky force-pushed on Mar 7, 2018
  63. ryanofsky referenced this in commit ecad5c9240 on Mar 7, 2018
  64. ryanofsky force-pushed on Mar 7, 2018
  65. MarcoFalke commented at 3:21 pm on March 8, 2018: member
    Needs rebase to fix travis (sorry)
  66. ryanofsky force-pushed on Mar 12, 2018
  67. ryanofsky commented at 6:55 pm on March 12, 2018: member

    Needs rebase to fix travis (sorry)

    Rebased 71bce6b0f4aee487026afd31ada41a4e6028abd3 -> 31297f007b2424ab5036258ed782aacc70237a7f (pr/atw-nomerge.33 -> pr/atw-nomerge.34) for #12607.

  68. kallewoof commented at 1:06 am on March 13, 2018: member
    Light utACK 59ba4deec713669504d284b415bd31f049866f4c
  69. ryanofsky force-pushed on Mar 13, 2018
  70. ryanofsky referenced this in commit a128bdc9e1 on Mar 13, 2018
  71. ryanofsky commented at 8:14 pm on March 13, 2018: member

    @kallewoof, this is based on #9680, so perhaps you could add your ACK there, since it looks like you reviewed commits from that PR.

    Added 1 commits 31297f007b2424ab5036258ed782aacc70237a7f -> 64f182fa587f35081ed8026b44e2a36010364153 (pr/atw-nomerge.34 -> pr/atw-nomerge.35, compare) Squashed 64f182fa587f35081ed8026b44e2a36010364153 -> 01ab386a6957de4b97ed52f2b1b2d45aa7ba4e0f (pr/atw-nomerge.35 -> pr/atw-nomerge.36) Rebased 01ab386a6957de4b97ed52f2b1b2d45aa7ba4e0f -> e15246fc310599770332021d14b96ce3f213a82b (pr/atw-nomerge.36 -> pr/atw-nomerge.37) due to conflict with #12658

  72. kallewoof commented at 11:27 pm on March 13, 2018: member
    Re-slight-utACK 8a6ec70
  73. sipa referenced this in commit 6acd8700bc on Mar 14, 2018
  74. Flowdalic referenced this in commit 7fd96dac7d on Mar 14, 2018
  75. ryanofsky force-pushed on Mar 14, 2018
  76. ryanofsky commented at 7:41 pm on March 15, 2018: member
    Rebased e15246fc310599770332021d14b96ce3f213a82b -> d62357a7c7c8c34a55580a31ddbfd04a9fa72591 (pr/atw-nomerge.37 -> pr/atw-nomerge.38) due to conflict with #12681 and after merge of base PR #9680.
  77. in src/wallet/walletdb.cpp:780 in bf59f2135d outdated 
    672-                CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef());
673-                ssValue >> wtx;
674-
675                 vTxHash.push_back(hash);
676-                vWtx.push_back(wtx);
677+                vWtx.emplace_back(nullptr /* wallet */, nullptr /* tx */);
    promag commented at 10:59 pm on March 15, 2018:

    In commit “Disable CWalletTx copy constructor”,

    Could add an empty constructor (or add default values)? I believe that would allow keeping vWtx as vector?

    ryanofsky commented at 4:49 pm on March 16, 2018:

    #9381 (review)

    In commit “Disable CWalletTx copy constructor”,

    Could add an empty constructor (or add default values)? I believe that would allow keeping vWtx as vector?

    It isn’t enough because a vector needs its elements to be copyable or movable (otherwise it couldn’t reallocate during expansion).

    Sjors commented at 11:48 am on February 26, 2020:
    Maybe briefly mention this in the commit message, as the change from std::vector to std::list looks a bit out of the blue (until you try compiling without).
  78. in src/wallet/wallet.h:501 in 62afca0ac3 outdated 
    347@@ -348,14 +348,13 @@ class CWalletTx : public CMerkleTx
348     mutable CAmount nAvailableWatchCreditCached;
349     mutable CAmount nChangeCached;
350 
351-    CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg))
352+    CWalletTx(const CWallet* wallet, CTransactionRef arg) : CMerkleTx(std::move(arg)), pwallet(wallet)
    promag commented at 11:02 pm on March 15, 2018:

    In commit “Avoid copying CWalletTx in LoadToWallet”,

    This and the Init() change could be in a different (trivial) commit.

    ryanofsky commented at 4:49 pm on March 16, 2018:

    #9381 (review)

    In commit “Avoid copying CWalletTx in LoadToWallet”,

    This and the Init() change could be in a different (trivial) commit.

    Good catch, split 62afca0ac31ee6db6beab800cc349157809e87da into 5f10dfceef542ec19030e02d4954295bc0fa0428 and 9b178932e532bc507bc4e825a404e42deab726f9

  79. in src/wallet/test/wallet_tests.cpp:288 in 98ff78d5a2 outdated 
    555-    CWalletTx wtx(&wallet, MakeTransactionRef(tx));
556-    if (block) {
557-        wtx.SetMerkleBranch(block, 0);
558-    }
559-    {
560-        LOCK(cs_main);
    promag commented at 11:18 pm on March 15, 2018:

    In commit “Remove CWalletTx merging logic from AddToWallet”,

    Should stay? Related to #12681.

    Looks like this code is not hit, otherwise the lock held assertion in LookupBlockIndex would cause travis failure here?

    ryanofsky commented at 4:51 pm on March 16, 2018:

    #9381 (review)

    In commit “Remove CWalletTx merging logic from AddToWallet”,

    Should stay? Related to #12681.

    It’s not needed because of the lock above:

    https://github.com/bitcoin/bitcoin/blob/98ff78d5a272f9fef0057ebd8a4553cd2bead5ee/src/wallet/test/wallet_tests.cpp#L543

    This is the reason there’s no assert failure on travis.

  80. promag commented at 11:34 pm on March 15, 2018: member

    I’m just generally not a fan of callbacks

    +1

    The merging code could still be removed but instead of callbacks, the caller would first lookup the transaction and if it exists, it would update accordingly, otherwise add it.

    Anyway, this sounds better than the current implementation.

    Interestingly this uncovered a bunch of cases where we were unwittingly copying wallet transactions in loops due to writing:

    Use auto there?

  81. ryanofsky force-pushed on Mar 16, 2018
  82. ryanofsky commented at 5:57 pm on March 16, 2018: member

    Split up commits d62357a7c7c8c34a55580a31ddbfd04a9fa72591 -> 65a15b7752e0ca0af3c381a26b07833a81523d30 (pr/atw-nomerge.38 -> pr/atw-nomerge.39) as suggested #9381 (review), no other code changes. Rebased 65a15b7752e0ca0af3c381a26b07833a81523d30 -> aa967a383a491ac2f90becaf998e91ac684b2d1f (pr/atw-nomerge.39 -> pr/atw-nomerge.40) due to conflict with #12891 Rebased aa967a383a491ac2f90becaf998e91ac684b2d1f -> 1d6c0ba1159d67c7b6dec8891e41fc20ec66370e (pr/atw-nomerge.40 -> pr/atw-nomerge.41) due to conflict with #11851 Rebased 1d6c0ba1159d67c7b6dec8891e41fc20ec66370e -> cad8b14857d8e310ebad8a850c9e6450e13360fc (pr/atw-nomerge.41 -> pr/atw-nomerge.42) due to conflict with #13081

    I’m just generally not a fan of callbacks

    +1

    The merging code could still be removed but instead of callbacks, the caller would first lookup the transaction and if it exists, it would update accordingly, otherwise add it.

    Callbacks can be used badly, and callbacks can also be used well. I think it’s better to look at advantages and disadvantages of using them in specific cases than to talk about callbacks in general. The reason I think callbacks make sense here is that AddToWallet gives callers a way to add and update transactions to the database, and takes care of the locking, serialization, i/o, and notification on callers behalf. The callback gives AddToWallet just enough flexibility to allow its callers to be able to mutate the CWalletTx object in the ways they need, without having to handle the other low level details.

    It seems to me like the suggestion above essentially amounts to inlining AddToWallet in the various places it is called, which would be more verbose and complicated for callers. But if there are advantages to doing this, I’d be interested to know about them.

    Interestingly this uncovered a bunch of cases where we were unwittingly copying wallet transactions in loops due to writing:

    Use auto there?

    Using auto would avoid the copying in these particular cases, but disabling the copy constructor avoids it in all cases, and prevents new unintended copies in the future.

  83. ryanofsky force-pushed on Apr 9, 2018
  84. ryanofsky force-pushed on Apr 10, 2018
  85. ryanofsky force-pushed on May 14, 2018
  86. DrahtBot added the label Needs rebase on Jun 15, 2018
  87. ryanofsky force-pushed on Jun 19, 2018
  88. ryanofsky commented at 9:22 pm on June 19, 2018: member

    Needs rebase

    Rebased cad8b14857d8e310ebad8a850c9e6450e13360fc -> a494d2a4472ea63620b1f6cce20cb726590eb75b (pr/atw-nomerge.42 -> pr/atw-nomerge.43) due to minor conflict with #13437. Rebased a494d2a4472ea63620b1f6cce20cb726590eb75b -> 497f8a035ca2788564266f426033f52f982075ed (pr/atw-nomerge.43 -> pr/atw-nomerge.44) due to conflict with #13622 Rebased 497f8a035ca2788564266f426033f52f982075ed -> ba8ade94a4291b761f6a0395a9d6345782388c91 (pr/atw-nomerge.44 -> pr/atw-nomerge.45) due to conflict with #13774 Rebased ba8ade94a4291b761f6a0395a9d6345782388c91 -> 876d885730e8572e4616b0067595c91f0545dd55 (pr/atw-nomerge.45 -> pr/atw-nomerge.46) due to conflict with #12992 Rebased 876d885730e8572e4616b0067595c91f0545dd55 -> 9274458aedb68919618b13568432998b35430e6f (pr/atw-nomerge.46 -> pr/atw-nomerge.47) due to conflict with #14023 Rebased 9274458aedb68919618b13568432998b35430e6f -> a0d1323d7ef977b0ed78ec675d8bb14fa5e672d5 (pr/atw-nomerge.47 -> pr/atw-nomerge.48) due to conflict with #13825 Rebased a0d1323d7ef977b0ed78ec675d8bb14fa5e672d5 -> 4b077293cec57f0b5f3bc010beb5ce1a9820a9ef (pr/atw-nomerge.48 -> pr/atw-nomerge.49) due to conflict with #14287 Updated 4b077293cec57f0b5f3bc010beb5ce1a9820a9ef -> 15adcd19e49399590b5224a6a5e0a11e003eac53 (pr/atw-nomerge.49 -> pr/atw-nomerge.50) with suggested lint fixes Updated 15adcd19e49399590b5224a6a5e0a11e003eac53 -> ca7b4e24324464f04053145e58c25bfa5e0c5a51 (pr/atw-nomerge.50 -> pr/atw-nomerge.51) with suggested lint fixes Rebased ca7b4e24324464f04053145e58c25bfa5e0c5a51 -> 8cbc44f92f0932c87abadbd7c85e246d5b37b707 (pr/atw-nomerge.51 -> pr/atw-nomerge.52) due to conflict with #11634 Added 1 commits 8cbc44f92f0932c87abadbd7c85e246d5b37b707 -> 845345c70c42411373a480406686b67870a235ad (pr/atw-nomerge.52 -> pr/atw-nomerge.53, compare) with MeshCollider suggestions. Rebased 845345c70c42411373a480406686b67870a235ad -> b35d6cd39f383af5c8d07312e7c7b8d2d3c12641 (pr/atw-nomerge.53 -> pr/atw-nomerge.54) due to conflict with #14437 Rebased b35d6cd39f383af5c8d07312e7c7b8d2d3c12641 -> da86f39d50de1ca559c96c87e17228f73ee1afef (pr/atw-nomerge.54 -> pr/atw-nomerge.55) due to conflict with #14711 Rebased da86f39d50de1ca559c96c87e17228f73ee1afef -> 596f85075d21c474f4ae5a98ad5c89da7ace1484 (pr/atw-nomerge.55 -> pr/atw-nomerge.56) due to conflict with #15288 Rebased 596f85075d21c474f4ae5a98ad5c89da7ace1484 -> ab2bd422f77c37d232467c45cdfa57a581c8008b (pr/atw-nomerge.56 -> pr/atw-nomerge.57) due to conflict with #15948

  89. DrahtBot removed the label Needs rebase on Jun 19, 2018
  90. sipa commented at 1:24 am on June 27, 2018: member
    utACK a494d2a4472ea63620b1f6cce20cb726590eb75b. I think the “callback” approach is perfectly fine here; they’re not actual callbacks (in that they’re not invoked at a different time or from a different thread); they’re just functions to fill in values at the time the object is created.
  91. HashUnlimited referenced this in commit debd93030d on Jun 29, 2018
  92. DrahtBot added the label Needs rebase on Jul 11, 2018
  93. ryanofsky force-pushed on Jul 11, 2018
  94. DrahtBot removed the label Needs rebase on Jul 11, 2018
  95. DrahtBot added the label Needs rebase on Jul 29, 2018
  96. ryanofsky force-pushed on Aug 6, 2018
  97. ryanofsky force-pushed on Aug 6, 2018
  98. DrahtBot removed the label Needs rebase on Aug 6, 2018
  99. DrahtBot added the label Needs rebase on Aug 27, 2018
  100. ryanofsky force-pushed on Aug 28, 2018
  101. DrahtBot removed the label Needs rebase on Aug 28, 2018
  102. ryanofsky closed this on Aug 29, 2018
  103. ryanofsky reopened this on Aug 29, 2018
  104. DrahtBot added the label Needs rebase on Aug 30, 2018
  105. ryanofsky force-pushed on Aug 30, 2018
  106. DrahtBot removed the label Needs rebase on Aug 30, 2018
  107. in src/wallet/wallet.cpp:975 in a0d1323d7e outdated 
    977+                    wtx.nIndex = posInBlock;
978+                    wtx.hashBlock = pIndex->GetBlockHash();
979+                    updated = true;
980+                }
981+                // Clear abandoned state assuming notification means it isn't
982+                // safe to consider the tranaction abandoned (see TODO above).
    practicalswift commented at 6:36 pm on September 2, 2018:
    Typo found by codespell: tranaction
  108. in src/wallet/wallet.cpp:843 in a0d1323d7e outdated 
    832@@ -833,65 +833,41 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
833     return success;
834 }
835 
836-bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
837+bool CWallet::AddToWallet(CTransactionRef tx, UpdateWalletTxFn update_wtx, bool fFlushOnClose)
    practicalswift commented at 8:45 am on September 21, 2018: 
    02018-09-20 06:04:29 clang-tidy(pr=9381): src/wallet/wallet.cpp:843:64: warning: the parameter 'update_wtx' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
12018-09-20 06:04:29 clang-tidy(pr=9381): src/wallet/wallet.cpp:903:66: warning: the parameter 'update_wtx' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
  109. in src/wallet/test/wallet_tests.cpp:225 in a0d1323d7e outdated 
    216+        wallet.AddTransaction(MakeTransactionRef(tx));
217     }
218-
219-    CWalletTx wtx(&wallet, MakeTransactionRef(tx));
220-    if (block) {
221-        wtx.SetMerkleBranch(block, 0);
    practicalswift commented at 8:45 am on September 21, 2018:

    Seems like SetMerkleBranch can be removed.

    02018-09-20 06:29:02 cppcheck(pr=9381): [src/wallet/wallet.cpp:4237]: (style) The function 'SetMerkleBranch' is never used.
  110. DrahtBot commented at 1:34 pm on September 21, 2018: 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:

    • #18592 (rpc: replace raw pointers with shared_ptrs by brakmic)
    • #16910 (wallet: reduce loading time by using unordered maps 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.

  111. ryanofsky force-pushed on Sep 21, 2018
  112. ryanofsky force-pushed on Sep 25, 2018
  113. in src/wallet/walletdb.cpp:199 in 15adcd19e4 outdated 
    195@@ -196,37 +196,41 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
196         {
197             uint256 hash;
198             ssKey >> hash;
199-            CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef());
200-            ssValue >> wtx;
201-            CValidationState state;
202-            if (!(CheckTransaction(*wtx.tx, state) && (wtx.GetHash() == hash) && state.IsValid()))
203-                return false;
204+            auto update_wtx = [&](CWalletTx& wtx, bool new_tx) {
    practicalswift commented at 5:47 am on September 26, 2018:
    new_tx not used?
  114. in src/wallet/wallet.cpp:973 in 15adcd19e4 outdated 
    975-            // Get merkle branch if transaction was found in a block
976-            if (pIndex != nullptr)
977-                wtx.SetMerkleBranch(pIndex, posInBlock);
978-
979-            return AddToWallet(wtx, false);
980+            auto update_wtx = [&](CWalletTx& wtx, bool new_tx) {
    practicalswift commented at 5:48 am on September 26, 2018:
    new_tx not used?
  115. in src/wallet/wallet.cpp:1069 in 15adcd19e4 outdated 
    1063@@ -1076,6 +1064,20 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
1064     return true;
1065 }
1066 
1067+bool CWallet::AddTransaction(CTransactionRef tx, const uint256& block_hash, int block_position)
1068+{
1069+    auto update_wtx = [&](CWalletTx& wtx, bool new_tx) {
    practicalswift commented at 5:48 am on September 26, 2018:
    new_tx not used?
  116. ryanofsky force-pushed on Sep 27, 2018
  117. DrahtBot added the label Needs rebase on Oct 24, 2018
  118. ryanofsky force-pushed on Oct 25, 2018
  119. DrahtBot removed the label Needs rebase on Oct 25, 2018
  120. lionello referenced this in commit c76dac8f53 on Nov 7, 2018
  121. DrahtBot added the label Needs rebase on Nov 9, 2018
  122. in src/wallet/wallet.cpp:981 in 2c7c443718 outdated 
    983+                    wtx.nIndex = posInBlock;
984+                    wtx.hashBlock = pIndex->GetBlockHash();
985+                    updated = true;
986+                }
987+                // Clear abandoned state assuming notification means it isn't
988+                // safe to consider the transaction abandoned (see TODO above).
    meshcollider commented at 1:47 am on November 12, 2018:
    Which TODO is this referring to?
    ryanofsky commented at 5:17 pm on November 12, 2018:

    re: #9381 (review)

    Which TODO is this referring to?

    This is referring to the abandoned transaction TODO in AddToWalletIfInvolvingMe:

    https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.h#L654-L658

    The TODO was moved in #13651 to where it doesn’t really make a lot of sense, so I’m moving it back here in 845345c70c42411373a480406686b67870a235ad.

  123. in src/wallet/wallet.cpp:988 in 2c7c443718 outdated 
    990+                    wtx.hashBlock.SetNull();
991+                    updated = true;
992+                }
993+                return updated;
994+            };
995+            return AddToWallet(MakeTransactionRef(tx), update_wtx);
    meshcollider commented at 1:48 am on November 12, 2018:
    Should this flush on close? Default is true but previously it didn’t
    ryanofsky commented at 5:18 pm on November 12, 2018:

    re: #9381 (review)

    Should this flush on close? Default is true but previously it didn’t

    Good catch. It should be safe to flush on close, but this change was not intentional. It’s now set back to false in 845345c70c42411373a480406686b67870a235ad.

  124. meshcollider commented at 2:02 am on November 12, 2018: contributor

    Light utACK https://github.com/bitcoin/bitcoin/pull/9381/commits/8cbc44f92f0932c87abadbd7c85e246d5b37b707

    Just a couple of small questions inline

  125. ryanofsky force-pushed on Nov 12, 2018
  126. DrahtBot removed the label Needs rebase on Nov 12, 2018
  127. DrahtBot added the label Needs rebase on Jan 30, 2019
  128. ryanofsky force-pushed on Jan 30, 2019
  129. DrahtBot removed the label Needs rebase on Jan 30, 2019
  130. DrahtBot added the label Needs rebase on Mar 4, 2019
  131. ryanofsky force-pushed on Mar 4, 2019
  132. DrahtBot removed the label Needs rebase on Mar 4, 2019
  133. jasonbcox referenced this in commit 80cfe7eac1 on Mar 26, 2019
  134. jonspock referenced this in commit 24788984c7 on Apr 3, 2019
  135. jonspock referenced this in commit 03f50c5f15 on Apr 4, 2019
  136. DrahtBot added the label Needs rebase on May 7, 2019
  137. ryanofsky force-pushed on May 7, 2019
  138. DrahtBot removed the label Needs rebase on May 7, 2019
  139. DrahtBot added the label Needs rebase on Jun 19, 2019
  140. ryanofsky force-pushed on Jun 19, 2019
  141. DrahtBot removed the label Needs rebase on Jun 19, 2019
  142. DrahtBot added the label Needs rebase on Jul 30, 2019
  143. ryanofsky force-pushed on Aug 1, 2019
  144. DrahtBot removed the label Needs rebase on Aug 1, 2019
  145. DrahtBot added the label Needs rebase on Aug 2, 2019
  146. ryanofsky force-pushed on Aug 2, 2019
  147. DrahtBot removed the label Needs rebase on Aug 2, 2019
  148. DrahtBot added the label Needs rebase on Aug 12, 2019
  149. ryanofsky force-pushed on Aug 13, 2019
  150. DrahtBot removed the label Needs rebase on Aug 13, 2019
  151. in src/wallet/wallet.h:1046 in ea715a2625 outdated 
    1035@@ -1041,7 +1036,8 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
1036     DBErrors ReorderTransactions();
1037 
1038     void MarkDirty();
1039-    bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true);
1040+    typedef std::function<bool(CWalletTx& wtx, bool new_tx)> UpdateWalletTxFn;
1041+    bool AddToWallet(CTransactionRef tx, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose=true);
    Sjors commented at 1:10 pm on August 15, 2019:
    Maybe make AddToWallet( private to avoid exposing the update_wtx callback stuff?
    ryanofsky commented at 3:52 pm on August 15, 2019:

    re: #9381 (review)

    Maybe make AddToWallet( private to avoid exposing the update_wtx callback stuff?

    I added a doxygen comment to avoid confusion around the update_wtx callback (which is also used in a later commit by LoadToWallet, not just AddWallet).

    I would change AddToWallet from public to private here too, but this is awkward because AddToWallet is called from a static test function which can’t be declared as a friend:

    https://github.com/bitcoin/bitcoin/blob/b8f3870c6a46052a80c694c0744a3eb56a9116ac/src/wallet/test/coinselector_tests.cpp#L68

    In general I think trying to have better encapsulation in the wallet with a CWallet public / private boundary is probably never going to work out too well. Really CWallet needs to get broken up into smaller classes which can’t access each others internal state, not just have a single big boundary.

  152. Sjors approved
  153. Sjors commented at 2:27 pm on August 15, 2019: member

    @achow101 how does this jive with wallet boxes #16341?

    Code review light-ACK b8f3870c6a46052a80c694c0744a3eb56a9116ac (some things go over my head, but do look cleaner). Tested sending coins between wallets, RBF and then abandoning the old one and waiting for confirmation.

  154. ryanofsky force-pushed on Aug 15, 2019
  155. ryanofsky commented at 4:58 pm on August 15, 2019: member

    Thanks for the review!

    Updated b8f3870c6a46052a80c694c0744a3eb56a9116ac -> 825a514481dd69dc7038c7f4c974a76cb2becd87 (pr/atw-nomerge.61 -> pr/atw-nomerge.62, compare) adding a update_wtx comment.

    @achow101 how does this jive with wallet boxes #16341?

    Not achow, but I’ve been slogging through #16341, and I think there is not too much overlap since this PR is concerned with transactions, while the other is concerned with scripts and keys. I definitely think this PR has much less priority than #16341 and shouldn’t interfere with it. I’ve been rebasing this PR a while anyway and it’s been easy to maintain.

  156. achow101 commented at 6:15 pm on August 15, 2019: member

    @achow101 how does this jive with wallet boxes #16341?

    AFAICT They don’t interact at all. The only part of #16341 that touches transactions is a small section in AddToWalletIfInvolvingMe which this PR does not touch.

  157. Sjors commented at 6:44 pm on August 15, 2019: member
    re-ACK 825a514481dd69dc7038c7f4c974a76cb2becd87, but Travis is in a state of despair, perhaps due to #16582 (@MarcoFalke?)
  158. in src/wallet/wallet.cpp:848 in 6731aac447 outdated 
    1161     //// debug print
1162-    WalletLogPrintf("AddToWallet %s  %s%s\n", wtxIn.GetHash().ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : ""));
1163+    WalletLogPrintf("AddToWallet %s  %s%s\n", hash.ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : ""));
1164 
1165     // Write to disk
1166     if (fInsertedNew || fUpdated)
    ariard commented at 4:49 pm on August 16, 2019:
    If tx isn’t already in mapWallet, it’s going to be a fInsertedNew one, if it’s already there we surely want to get them fUpdated on disk, and I think both callback return true, excepts for a transaction being disconnected ? Maybe all this flags could be simplified in a further work.
    ryanofsky commented at 7:24 pm on September 10, 2019:

    re: #9381 (review)

    If tx isn’t already in mapWallet, it’s going to be a fInsertedNew one, if it’s already there we surely want to get them fUpdated on disk, and I think both callback return true, excepts for a transaction being disconnected ? Maybe all this flags could be simplified in a further work.

    I think this is basically never false right now, but it seems reasonable to me that there should be a way to avoid writing transaction metadata to disk when it is unchanged. I’d also be happy to simplify this if you see a way to do that, but for the now the PR isn’t really changing anything here.

  159. in src/wallet/wallet.cpp:1161 in 8c4c8cc538 outdated 
    1156@@ -1157,12 +1157,13 @@ bool CWallet::AddToWallet(CTransactionRef tx, const UpdateWalletTxFn& update_wtx
1157     return true;
1158 }
1159 
1160-void CWallet::LoadToWallet(const CWalletTx& wtxIn)
1161+bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& update_wtx)
1162 {
1163-    uint256 hash = wtxIn.GetHash();
1164-    const auto& ins = mapWallet.emplace(hash, wtxIn);
1165+    auto ins = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(this, nullptr));
    ariard commented at 5:18 pm on August 16, 2019:
    Hmmm new execution flow doesn’t make it easier to understand. Maybe mapWallet should be access directly in ReadKeyValue, when as you said in another comment wallet components are broken up in smaller classes. I think, but need to test, that MarkConflicted and its loop is useless here.
    ryanofsky commented at 7:25 pm on September 10, 2019:

    re: #9381 (review)

    Hmmm new execution flow doesn’t make it easier to understand. Maybe mapWallet should be access directly in ReadKeyValue, when as you said in another comment wallet components are broken up in smaller classes. I think, but need to test, that MarkConflicted and its loop is useless here.

    These improvements seem like good ideas, they’re just larger changes than this one is. I agree that the local control flow is a more complicated here, but I think it’s a good tradeoff to be able to get rid of the CWalletTx copy constructor, and get rid of all code that is filling a temporary CWalletTx and then merging two CWalletTx’s together instead of directly updating a single CWalletTx.

    This emplace on this line is a good example of how fragile the temporary CWalletTx merging logic can be. If the hash already exists the data in wtxIn is just silently discarded in a different part of the code from where the data was loaded. In the new flow there is no temporary CWalletTx, and the code responsible for loading data can directly control what goes into the final CWalletTx.

  160. ariard commented at 5:24 pm on August 16, 2019: member

    Concept ACK, did only a light code review, I think changes are going in the right way to let caller manage the scope of transactions changes, at same time it points way in which CWallet could be refactored in better way.

    It’s overlap with #9381 but rebased in any order shouldn’t be ugly

  161. ryanofsky force-pushed on Aug 21, 2019
  162. DrahtBot added the label Needs rebase on Sep 5, 2019
  163. ryanofsky force-pushed on Sep 10, 2019
  164. ryanofsky commented at 8:05 pm on September 10, 2019: member
    Rebased cb4fce1f5c4b83d08704a0423f88add021405aae -> 47d95638c682d8efd1e46dfcb300b75aa970f7a0 (pr/atw-nomerge.63 -> pr/atw-nomerge.64) due to conflict with #16624
  165. ryanofsky force-pushed on Sep 10, 2019
  166. DrahtBot removed the label Needs rebase on Sep 10, 2019
  167. ryanofsky force-pushed on Sep 23, 2019
  168. ryanofsky force-pushed on Oct 11, 2019
  169. ryanofsky force-pushed on Oct 16, 2019
  170. ryanofsky force-pushed on Oct 22, 2019
  171. DrahtBot added the label Needs rebase on Oct 23, 2019
  172. ryanofsky force-pushed on Oct 23, 2019
  173. DrahtBot removed the label Needs rebase on Oct 23, 2019
  174. DrahtBot added the label Needs rebase on Oct 24, 2019
  175. ryanofsky force-pushed on Oct 24, 2019
  176. DrahtBot removed the label Needs rebase on Oct 24, 2019
  177. DrahtBot added the label Needs rebase on Nov 8, 2019
  178. ryanofsky force-pushed on Nov 8, 2019
  179. DrahtBot removed the label Needs rebase on Nov 8, 2019
  180. DrahtBot added the label Needs rebase on Nov 22, 2019
  181. meshcollider commented at 9:15 pm on November 22, 2019: contributor
    Sorry about all the rebases, Concept ACK
  182. ryanofsky force-pushed on Nov 25, 2019
  183. DrahtBot removed the label Needs rebase on Nov 25, 2019
  184. DrahtBot added the label Needs rebase on Dec 4, 2019
  185. laanwj commented at 1:17 pm on December 4, 2019: member
    Concept ACK
  186. ryanofsky force-pushed on Dec 4, 2019
  187. DrahtBot removed the label Needs rebase on Dec 4, 2019
  188. DrahtBot added the label Needs rebase on Jan 15, 2020
  189. ryanofsky force-pushed on Jan 16, 2020
  190. DrahtBot removed the label Needs rebase on Jan 16, 2020
  191. DrahtBot added the label Needs rebase on Feb 17, 2020
  192. ryanofsky force-pushed on Feb 25, 2020
  193. ryanofsky commented at 5:40 pm on February 25, 2020: member
    Rebased dff3c12515773bef07756bb2420da5a1ac8e1438 -> 93510bcfe80385203880d1211cb94f621411839e (pr/atw-nomerge.74 -> pr/atw-nomerge.75, compare) due to minor conflict with #13339
  194. DrahtBot removed the label Needs rebase on Feb 25, 2020
  195. in src/wallet/wallet.cpp:766 in 03c5ac045f outdated 
    762@@ -763,19 +763,19 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
763     return false;
764 }
765 
766-bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
767+bool CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose)
    Sjors commented at 9:23 am on February 26, 2020:
    In 03c5ac045f94ab965d79300712a70a0357397d5e Remove CWalletTx merging logic from AddToWallet: it might make sense to have a seperate commit that adds the confirm argument. (maybe not worth confusing existing reviewers)
    ryanofsky commented at 2:43 pm on February 26, 2020:

    re: #9381 (review)

    In 03c5ac0 Remove CWalletTx merging logic from AddToWallet: it might make sense to have a seperate commit that adds the confirm argument. (maybe not worth confusing existing reviewers)

    This is a good thought, and I started to implement it, but the problem is if I just make a commit adding the confirm argument, then there are two confirm objects being passed to AddToWallet (the other one embedded in the CWalletTx object), which is confusing because it’s unclear which confirmation should take precedence. If I change the CWalletTx argument to a TransactionRef to solve this problem, then there is no way for callers to set to the other CWalletTx fields (mapValue, vOrderForm, etc), and I have to add the update_wtx argument too, which is already the whole current commit

  196. in src/wallet/wallet.h:878 in 03c5ac045f outdated 
    870@@ -871,7 +871,16 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
871     DBErrors ReorderTransactions();
872 
873     void MarkDirty();
874-    bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true);
875+
876+    //! Callback for updating transaction metadata in mapWallet.
877+    //!
878+    //! @param wtx - reference to mapWallet transaction to update
879+    //! @param new_tx - true if wtx is newly inserted, false it it previously existed
    Sjors commented at 9:37 am on February 26, 2020:
    Nit: “false if it”
    ryanofsky commented at 2:44 pm on February 26, 2020:

    re: #9381 (review)

    Nit: “false if it”

    Thank you, fixed!

  197. in src/wallet/wallet.cpp:3003 in 03c5ac045f outdated 
    2941@@ -2951,37 +2942,38 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
2942 {
2943     auto locked_chain = chain().lock();
2944     LOCK(cs_wallet);
2945+    WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); /* Continued */
    Sjors commented at 9:54 am on February 26, 2020:
    Why the /* Continued */ comment?
    ryanofsky commented at 2:44 pm on February 26, 2020:

    re: #9381 (review)

    Why the /* Continued */ comment?

    This is unchanged from previous code, but it’s needed to print a lint error since the log format string doesn’t end in \n

  198. in src/wallet/wallet.cpp:2954 in 03c5ac045f outdated 
    2957 
2958     // Add tx to wallet, because if it has change it's also ours,
2959     // otherwise just for transaction history.
2960-    AddToWallet(wtxNew);
2961+    AddToWallet(std::move(tx), {}, [&](CWalletTx& new_wtx, bool new_tx) {
2962+        assert(new_tx);
    Sjors commented at 10:06 am on February 26, 2020:

    Make this a runtime exception? Perhaps there’s some weird edge case, I haven’t tried, where a user restores their wallet from a backup on a node with a fresh mempool. The user then recreates the same* transaction in the UI and as they look at the confirmation screen, the transaction re-enters the mempool. Once they click OK this assert should be hit.

    * = IIUC it’s only a duplicate if they use the same inputs, outputs, fee, if we shuffle them the same way and if the block height hasn’t changed

    ryanofsky commented at 2:44 pm on February 26, 2020:

    re: #9381 (review)

    Make this a runtime exception? Perhaps there’s some weird edge case, I haven’t tried, where a user restores their wallet from a backup on a node with a fresh mempool. The user then recreates the same* transaction in the UI and as they look at the confirmation screen, the transaction re-enters the mempool. Once they click OK this assert should be hit.

    Good point. Since there can be an arbitrary delay before the GUI commits the transaction, it isn’t safe to assert assert the transaction is new. I removed the assert and just replaced it with nonfatal checks to make sure there aren’t conflicting mapValue and order form values.

  199. in src/wallet/walletdb.cpp:310 in 754eb2112a outdated 
    269 
270-            pwallet->LoadToWallet(wtx);
271+                return true;
272+            };
273+            if (!pwallet->LoadToWallet(hash, update_wtx)) {
274+                return false;
    Sjors commented at 10:57 am on February 26, 2020:

    In 754eb2112aac4729805dcc547bef1de956c3c14d Avoid copying CWalletTx in LoadToWallet

    The “result” of (formerly void) LoadToWallet has always been ignored. Perhaps that’s because before https://github.com/bitcoin/bitcoin/pull/3671/files#diff-3b5a9b7d780ff672241548edf2888fcdL385 we just updated the transaction map directly.

    Can you think of any silent failures that would now result in ReadKeyValue failing, and IIUC trigger a rescan? (cc @achow101)

    https://github.com/bitcoin/bitcoin/blob/2d6e76af240969aa284cd4c3d376493988e218c2/src/wallet/walletdb.cpp#L469-L483

    ryanofsky commented at 2:44 pm on February 26, 2020:

    re: #9381 (review)

    In 754eb21 Avoid copying CWalletTx in LoadToWallet

    The “result” of (formerly void) LoadToWallet has always been ignored. Perhaps that’s because before https://github.com/bitcoin/bitcoin/pull/3671/files#diff-3b5a9b7d780ff672241548edf2888fcdL385 we just updated the transaction map directly.

    Can you think of any silent failures that would now result in ReadKeyValue failing, and IIUC trigger a rescan? (cc @achow101)

    It might be worth thinking about how to improve error handling in this code, but behavior shouldn’t be changing here. LoadToWallet will only return false if fill_wtx returns false, and fill_wtx only returns false if wtx.GetHash() != hash. So ReadKeyValue should be returning true all the times it used to return true and false and the times it used to return false

  200. in src/wallet/walletdb.cpp:217 in 754eb2112a outdated 
    213@@ -214,36 +214,40 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
214         } else if (strType == DBKeys::TX) {
215             uint256 hash;
216             ssKey >> hash;
217-            CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef());
218-            ssValue >> wtx;
219-            if (wtx.GetHash() != hash)
220-                return false;
221+            auto update_wtx = [&](CWalletTx& wtx, bool /* new_tx */) {
    Sjors commented at 11:19 am on February 26, 2020:

    In 754eb2112aac4729805dcc547bef1de956c3c14d Avoid copying CWalletTx in LoadToWallet

    Maybe add a comment here that LoadToWallet creates a fresh CWalletTx and that we fill it with ssValue (hence my rename to fill_wtx suggestion above).

    Since a wallet doesn’t (shouldn’t? otherwise add assert(new_tx)) have duplicate transactions new_tx should be true, but if some reason a wallet entry does exists it will be overridden.

    ryanofsky commented at 2:44 pm on February 26, 2020:

    re: #9381 (review)

    In 754eb21 Avoid copying CWalletTx in LoadToWallet

    Maybe add a comment here that LoadToWallet creates a fresh CWalletTx and that we fill it with ssValue (hence my rename to fill_wtx suggestion above).

    Since a wallet doesn’t (shouldn’t? otherwise add assert(new_tx)) have duplicate transactions new_tx should be true, but if some reason a wallet entry does exists it will be overridden.

    Added comment and assert

  201. in src/wallet/wallet.cpp:861 in 754eb2112a outdated 
    857@@ -858,33 +858,34 @@ bool CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& con
858     return true;
859 }
860 
861-void CWallet::LoadToWallet(CWalletTx& wtxIn)
862+bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& update_wtx)
    Sjors commented at 11:27 am on February 26, 2020:
    Maybe rename update_wtx to fill_wtx .
    ryanofsky commented at 2:44 pm on February 26, 2020:

    re: #9381 (review)

    Maybe rename update_wtx to fill_wtx .

    Done

  202. Sjors approved
  203. Sjors commented at 11:50 am on February 26, 2020: member
    ACK 93510bcfe80385203880d1211cb94f621411839e
  204. ryanofsky force-pushed on Feb 26, 2020
  205. ryanofsky commented at 4:19 pm on February 26, 2020: member

    Thanks for reviewing!

    Updated 93510bcfe80385203880d1211cb94f621411839e -> fc4df9dc2910c4a57d5e81573faf7952e1c3ee42 (pr/atw-nomerge.75 -> pr/atw-nomerge.76, compare) with suggestions and a few tweaks to make diffs a little smaller

  206. DrahtBot added the label Needs rebase on Mar 23, 2020
  207. ryanofsky force-pushed on Mar 24, 2020
  208. ryanofsky commented at 8:34 pm on March 24, 2020: member
    Rebased fc4df9dc2910c4a57d5e81573faf7952e1c3ee42 -> f1fc7892608fb258da86017c79ffb4b1a477014a (pr/atw-nomerge.76 -> pr/atw-nomerge.77, compare) due to conflict with #18278
  209. DrahtBot removed the label Needs rebase on Mar 24, 2020
  210. Sjors commented at 3:16 pm on March 31, 2020: member
    utACK f1fc7892608fb258da86017c79ffb4b1a477014a
  211. DrahtBot added the label Needs rebase on Apr 14, 2020
  212. ryanofsky force-pushed on Apr 15, 2020
  213. ryanofsky commented at 9:41 pm on April 15, 2020: member
    Rebased f1fc7892608fb258da86017c79ffb4b1a477014a -> ece5d1cf46cf8c335dee4bb6affe514d7b33f306 (pr/atw-nomerge.77 -> pr/atw-nomerge.78, compare) due to conflict with #17954
  214. DrahtBot removed the label Needs rebase on Apr 15, 2020
  215. Sjors commented at 2:17 pm on April 19, 2020: member
    re-utACK ece5d1c (rebased)
  216. Remove CWalletTx merging logic from AddToWallet 
    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.
    2b9cba2065
  217. Get rid of unneeded CWalletTx::Init parameter bd2fbc7cdb
  218. Avoid copying CWalletTx in LoadToWallet 
    The change in walletdb.cpp is easier to review ignoring whitespace.

This change is need to get rid of CWalletTx copy constructor.
    65b9d8f8dd
  219. Disable CWalletTx copy constructor 
    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.
    d002f9d15d
  220. Get rid of BindWallet 
    CWalletTx initialization has been fixed so it's no longer necessary to change
which wallet a transaction is bound to.
    28b112e9bd
  221. DrahtBot added the label Needs rebase on May 1, 2020
  222. ryanofsky force-pushed on May 1, 2020
  223. ryanofsky commented at 3:00 pm on May 1, 2020: member
    Rebased ece5d1cf46cf8c335dee4bb6affe514d7b33f306 -> f2892d8eb5404a34f3b09d3b9ac65d58c965e362 (pr/atw-nomerge.78 -> pr/atw-nomerge.79, compare) due to conflict with #16426
  224. DrahtBot removed the label Needs rebase on May 1, 2020
  225. Sjors commented at 10:21 am on May 4, 2020: member
    Light re-utACK f2892d8eb5404a34f3b09d3b9ac65d58c965e362, looks like a reasonably straight-forward rebase.
  226. meshcollider commented at 2:52 am on May 5, 2020: contributor
    utACK f2892d8eb5404a34f3b09d3b9ac65d58c965e362
  227. in src/wallet/wallet.cpp:3010 in 677e1e0e0e outdated 
    3013     // otherwise just for transaction history.
3014-    AddToWallet(wtxNew);
3015+    CWalletTx* wtx = AddToWallet(std::move(tx), {}, [&](CWalletTx& wtx, bool new_tx) {
3016+        CHECK_NONFATAL(wtx.mapValue.empty());
3017+        CHECK_NONFATAL(wtx.vOrderForm.empty());
3018+        wtx.mapValue = std::move(mapValue);
    MarcoFalke commented at 3:12 pm on May 5, 2020:
    Does the std::move still work after going through a lambda capture by reference?
    sipa commented at 4:22 pm on May 5, 2020:
    Yes. mapValue is a reference to the same thing it was outside of the lambda. std::move casts it to an rvalue reference.
    MarcoFalke commented at 4:33 pm on May 5, 2020:
    Oh, this has nothing to do with lambdas. I didn’t know that references (as long as they are not const) can be moved.
    ryanofsky commented at 4:39 pm on May 5, 2020:

    re: #9381 (review)

    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 =

    sipa commented at 4:41 pm on May 5, 2020:
    Even const ones, actually - though unless they have mutable fields, there is no effect.
    MarcoFalke commented at 5:34 pm on May 5, 2020:

    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:

    0#include <memory>
1
2struct Test{
3    const std::unique_ptr<int>b;
4    Test(const std::unique_ptr<int>& a):b{std::move(a)}{}
5};
    sipa commented at 5:37 pm on May 5, 2020:
    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.
    MarcoFalke commented at 5:43 pm on May 5, 2020:
    TIL that const T&& exists.
  228. in src/wallet/wallet.cpp:3017 in 677e1e0e0e outdated 
    3022+        return true;
3023+    });
3024 
3025     // Notify that old coins are spent
3026-    for (const CTxIn& txin : wtxNew.tx->vin) {
3027+    for (const CTxIn& txin : wtx->tx->vin) {
    MarcoFalke commented at 3:13 pm on May 5, 2020:
    Doesn’t this line crash the node when the wallet can not write the tx to disk?
    ryanofsky commented at 4:30 pm on May 5, 2020:

    re: #9381 (review)

    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

  229. MarcoFalke approved
  230. MarcoFalke commented at 3:14 pm on May 5, 2020: member

    ACK f2892d8

    Just some questions

  231. ryanofsky force-pushed on May 5, 2020
  232. ryanofsky commented at 4:51 pm on May 5, 2020: member
    Updated f2892d8eb5404a34f3b09d3b9ac65d58c965e362 -> 28b112e9bd3fd1181c0720306051ba7efca8b43 (pr/atw-nomerge.79 -> pr/atw-nomerge.80, compare) reverting some CommitTransaction changes to avoid changing behavior and crashing on failure
  233. MarcoFalke commented at 5:29 pm on May 5, 2020: member
    Ah sorry, no need to revert your changes. If the wallet can’t write to disk, a simple CHECK_NONFATAL(wtx) should be sufficient
  234. ryanofsky commented at 5:54 pm on May 5, 2020: member

    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.

  235. MarcoFalke commented at 5:58 pm on May 5, 2020: member

    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 28b112e9bd3fd1181c0720306051ba7efca8b436

  236. meshcollider commented at 11:37 pm on May 5, 2020: contributor
    re-utACK 28b112e9bd3fd1181c0720306051ba7efca8b436
  237. meshcollider merged this on May 5, 2020
  238. meshcollider closed this on May 5, 2020
  239. sidhujag referenced this in commit 04bea9fcac on May 12, 2020
  240. xdustinface referenced this in commit 272a288258 on Aug 12, 2020
  241. xdustinface referenced this in commit fff497de59 on Aug 12, 2020
  242. xdustinface referenced this in commit 2edc79f5ec on Aug 27, 2020
  243. xdustinface referenced this in commit 2251e61a50 on Aug 28, 2020
  244. xdustinface referenced this in commit a65556bdff on Aug 29, 2020
  245. xdustinface referenced this in commit a759f99845 on Sep 1, 2020
  246. UdjinM6 referenced this in commit ebe7e80a49 on Sep 21, 2020
  247. Fabcien referenced this in commit 66ddfb30ed on Jan 27, 2021
  248. Fabcien referenced this in commit 2e0a03b737 on Jan 27, 2021
  249. Fabcien referenced this in commit 6bde49b668 on Jan 27, 2021
  250. Fabcien referenced this in commit 19dfc49276 on Jan 27, 2021
  251. Fabcien referenced this in commit 846f417721 on Jan 27, 2021
  252. gades referenced this in commit 06ff74715d on Mar 8, 2022
  253. DrahtBot locked this on Aug 18, 2022


ryanofsky TheBlueMatt kallewoof MarcoFalke promag Sjors sipa practicalswift DrahtBot meshcollider achow101 ariard laanwj

Labels
Refactoring 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-11-17 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me