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: contributor

    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 cross-referenced this on Jan 13, 2017 from issue [RPC] Simplified bumpfee command. by mrbandrews
  6. ryanofsky force-pushed on Jan 25, 2017
  7. ryanofsky commented at 6:30 PM on January 25, 2017: contributor

    Rebased ed457a39ebaa672cce21622d3af422fe50730580 -> b1ac3cff00a51c0b8965b7a85fa489c98409ddff to resolve merge conflicts with bumpfee (#8456).

  8. ryanofsky commented at 6:48 PM on January 25, 2017: contributor

    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.

  9. TheBlueMatt commented at 7:26 PM on January 25, 2017: contributor

    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.

  10. TheBlueMatt commented at 10:17 PM on January 25, 2017: contributor

    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.

  11. ryanofsky force-pushed on Feb 2, 2017
  12. ryanofsky referenced this in commit 53e5acc717 on Feb 2, 2017
  13. ryanofsky referenced this in commit dd12997a4f on Feb 2, 2017
  14. ryanofsky cross-referenced this on Feb 2, 2017 from issue Set correct metadata on bumpfee wallet transactions by ryanofsky
  15. ryanofsky force-pushed on Feb 2, 2017
  16. ryanofsky renamed this:
    Remove CWalletTx merging logic from AddToWallet
    Remove CWalletTx merging logic from AddToWallet (on top of #9673)
    on Feb 2, 2017
  17. ryanofsky commented at 11:51 PM on February 2, 2017: contributor

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

  18. ryanofsky force-pushed on Feb 3, 2017
  19. TheBlueMatt commented at 6:46 PM on February 3, 2017: contributor

    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.

  20. ryanofsky cross-referenced this on Feb 3, 2017 from issue Unify CWalletTx construction by ryanofsky
  21. 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
  22. ryanofsky commented at 7:10 PM on February 3, 2017: contributor

    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.

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

    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:

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

    instead of:

    for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet)
  47. ryanofsky referenced this in commit 7b44eb8e15 on Dec 14, 2017
  48. ryanofsky force-pushed on Dec 14, 2017
  49. ryanofsky cross-referenced this on Dec 15, 2017 from issue Travis x86_64-apple-darwin11 errors: "Configuring boost... /bin/sh: 1: requires: not found" by ryanofsky
  50. ryanofsky referenced this in commit d17857b2fb on Dec 15, 2017
  51. ryanofsky force-pushed on Dec 15, 2017
  52. ryanofsky referenced this in commit cba74b9b48 on Dec 31, 2017
  53. ryanofsky force-pushed on Dec 31, 2017
  54. ryanofsky referenced this in commit 2458b102f7 on Dec 31, 2017
  55. ryanofsky force-pushed on Dec 31, 2017
  56. ryanofsky referenced this in commit a9db0f911c on Jan 25, 2018
  57. ryanofsky force-pushed on Jan 25, 2018
  58. ryanofsky referenced this in commit 8e3dd3be1a on Feb 2, 2018
  59. ryanofsky force-pushed on Feb 2, 2018
  60. ryanofsky force-pushed on Feb 12, 2018
  61. 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.

  62. ryanofsky referenced this in commit ae8fd3792b on Feb 23, 2018
  63. ryanofsky force-pushed on Feb 23, 2018
  64. ryanofsky force-pushed on Mar 5, 2018
  65. ryanofsky referenced this in commit e54fbd8c00 on Mar 7, 2018
  66. ryanofsky force-pushed on Mar 7, 2018
  67. ryanofsky referenced this in commit ecad5c9240 on Mar 7, 2018
  68. ryanofsky force-pushed on Mar 7, 2018
  69. MarcoFalke commented at 3:21 PM on March 8, 2018: member

    Needs rebase to fix travis (sorry)

  70. ryanofsky force-pushed on Mar 12, 2018
  71. ryanofsky commented at 6:55 PM on March 12, 2018: contributor

    Needs rebase to fix travis (sorry)

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

  72. kallewoof commented at 1:06 AM on March 13, 2018: member

    Light utACK 59ba4deec713669504d284b415bd31f049866f4c

  73. ryanofsky force-pushed on Mar 13, 2018
  74. ryanofsky referenced this in commit a128bdc9e1 on Mar 13, 2018
  75. ryanofsky commented at 8:14 PM on March 13, 2018: contributor

    @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

  76. kallewoof commented at 11:27 PM on March 13, 2018: member

    Re-slight-utACK 8a6ec70

  77. sipa referenced this in commit 6acd8700bc on Mar 14, 2018
  78. Flowdalic referenced this in commit 7fd96dac7d on Mar 14, 2018
  79. ryanofsky force-pushed on Mar 14, 2018
  80. ryanofsky commented at 7:41 PM on March 15, 2018: contributor

    Rebased e15246fc310599770332021d14b96ce3f213a82b -> d62357a7c7c8c34a55580a31ddbfd04a9fa72591 (pr/atw-nomerge.37 -> pr/atw-nomerge.38) due to conflict with #12681 and after merge of base PR #9680.

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

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

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

  84. 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?

  85. ryanofsky force-pushed on Mar 16, 2018
  86. ryanofsky commented at 5:57 PM on March 16, 2018: contributor

    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.

  87. 2a5A1Ghu1 cross-referenced this on Apr 5, 2018 from issue doc: add qrencode to brew install instructions (#1) by 2a5A1Ghu1
  88. ryanofsky force-pushed on Apr 9, 2018
  89. ryanofsky force-pushed on Apr 10, 2018
  90. ryanofsky force-pushed on May 14, 2018
  91. DrahtBot cross-referenced this on Jun 14, 2018 from issue wallet: Erase wtxOrderd wtx pointer on removeprunedfunds by MarcoFalke
  92. DrahtBot added the label Needs rebase on Jun 15, 2018
  93. ryanofsky force-pushed on Jun 19, 2018
  94. ryanofsky commented at 9:22 PM on June 19, 2018: contributor

    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

  95. DrahtBot removed the label Needs rebase on Jun 19, 2018
  96. 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.

  97. HashUnlimited referenced this in commit debd93030d on Jun 29, 2018
  98. HashUnlimited cross-referenced this on Jun 29, 2018 from issue [wallet] Construct CWalletTx objects in CommitTransaction by HashUnlimited
  99. DrahtBot cross-referenced this on Jul 10, 2018 from issue Remove mapRequest tracking that just effects Qt display. by TheBlueMatt
  100. DrahtBot added the label Needs rebase on Jul 11, 2018
  101. ryanofsky force-pushed on Jul 11, 2018
  102. DrahtBot removed the label Needs rebase on Jul 11, 2018
  103. DrahtBot cross-referenced this on Jul 26, 2018 from issue wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof
  104. DrahtBot cross-referenced this on Jul 27, 2018 from issue Return void instead of bool for functions that cannot fail by practicalswift
  105. DrahtBot added the label Needs rebase on Jul 29, 2018
  106. ryanofsky force-pushed on Aug 6, 2018
  107. ryanofsky force-pushed on Aug 6, 2018
  108. DrahtBot removed the label Needs rebase on Aug 6, 2018
  109. DrahtBot cross-referenced this on Aug 6, 2018 from issue [wallet] Kill accounts by jnewbery
  110. DrahtBot cross-referenced this on Aug 22, 2018 from issue Remove accounts rpcs by jnewbery
  111. DrahtBot added the label Needs rebase on Aug 27, 2018
  112. ryanofsky force-pushed on Aug 28, 2018
  113. DrahtBot removed the label Needs rebase on Aug 28, 2018
  114. ryanofsky closed this on Aug 29, 2018
  115. ryanofsky reopened this on Aug 29, 2018
  116. DrahtBot added the label Needs rebase on Aug 30, 2018
  117. ryanofsky force-pushed on Aug 30, 2018
  118. DrahtBot removed the label Needs rebase on Aug 30, 2018
  119. 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

  120. DrahtBot cross-referenced this on Sep 7, 2018 from issue Replace coin selection fallback strategy with Single Random Draw by achow101
  121. DrahtBot cross-referenced this on Sep 13, 2018 from issue Use MakeUnique to construct objects owned by unique_ptrs by practicalswift
  122. 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: 
    2018-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]
2018-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]
  123. 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.

    2018-09-20 06:29:02 cppcheck(pr=9381): [src/wallet/wallet.cpp:4237]: (style) The function 'SetMerkleBranch' is never used.
  124. DrahtBot cross-referenced this on Sep 21, 2018 from issue tests: Use MakeUnique to construct objects owned by unique_ptrs by practicalswift
  125. DrahtBot cross-referenced this on Sep 21, 2018 from issue wallet: Add missing cs_wallet/cs_KeyStore locks to wallet by practicalswift
  126. DrahtBot cross-referenced this on Sep 21, 2018 from issue Refactor: separate wallet from node by ryanofsky
  127. DrahtBot commented at 1:34 PM on September 21, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  128. ryanofsky force-pushed on Sep 21, 2018
  129. DrahtBot cross-referenced this on Sep 24, 2018 from issue rpc: Early call once CWallet::MarkDirty in import calls by promag
  130. ryanofsky force-pushed on Sep 25, 2018
  131. DrahtBot cross-referenced this on Sep 26, 2018 from issue Add missing locks: validation.cpp + related by practicalswift
  132. 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?

  133. 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?

  134. 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?

  135. ryanofsky force-pushed on Sep 27, 2018
  136. DrahtBot cross-referenced this on Sep 27, 2018 from issue Multiprocess bitcoin by ryanofsky
  137. DrahtBot cross-referenced this on Oct 9, 2018 from issue Refactor: Start to separate wallet from node by ryanofsky
  138. DrahtBot added the label Needs rebase on Oct 24, 2018
  139. ryanofsky force-pushed on Oct 25, 2018
  140. DrahtBot removed the label Needs rebase on Oct 25, 2018
  141. lionello referenced this in commit c76dac8f53 on Nov 7, 2018
  142. DrahtBot added the label Needs rebase on Nov 9, 2018
  143. 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.

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

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

  146. ryanofsky force-pushed on Nov 12, 2018
  147. DrahtBot removed the label Needs rebase on Nov 12, 2018
  148. DrahtBot added the label Needs rebase on Jan 30, 2019
  149. ryanofsky force-pushed on Jan 30, 2019
  150. DrahtBot removed the label Needs rebase on Jan 30, 2019
  151. DrahtBot added the label Needs rebase on Mar 4, 2019
  152. ryanofsky force-pushed on Mar 4, 2019
  153. DrahtBot removed the label Needs rebase on Mar 4, 2019
  154. jasonbcox referenced this in commit 80cfe7eac1 on Mar 26, 2019
  155. jonspock referenced this in commit 24788984c7 on Apr 3, 2019
  156. jonspock referenced this in commit 03f50c5f15 on Apr 4, 2019
  157. DrahtBot added the label Needs rebase on May 7, 2019
  158. ryanofsky force-pushed on May 7, 2019
  159. DrahtBot removed the label Needs rebase on May 7, 2019
  160. DrahtBot added the label Needs rebase on Jun 19, 2019
  161. ryanofsky force-pushed on Jun 19, 2019
  162. DrahtBot removed the label Needs rebase on Jun 19, 2019
  163. DrahtBot added the label Needs rebase on Jul 30, 2019
  164. ryanofsky force-pushed on Aug 1, 2019
  165. DrahtBot removed the label Needs rebase on Aug 1, 2019
  166. DrahtBot added the label Needs rebase on Aug 2, 2019
  167. ryanofsky force-pushed on Aug 2, 2019
  168. DrahtBot removed the label Needs rebase on Aug 2, 2019
  169. DrahtBot added the label Needs rebase on Aug 12, 2019
  170. ryanofsky force-pushed on Aug 13, 2019
  171. DrahtBot removed the label Needs rebase on Aug 13, 2019
  172. 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.

  173. Sjors approved
  174. 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.

  175. ryanofsky force-pushed on Aug 15, 2019
  176. ryanofsky commented at 4:58 PM on August 15, 2019: contributor

    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.

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

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

  179. ryanofsky cross-referenced this on Aug 16, 2019 from issue wallet: encapsulate transactions state by ariard
  180. 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.

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

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

  183. ryanofsky force-pushed on Aug 21, 2019
  184. DrahtBot added the label Needs rebase on Sep 5, 2019
  185. ryanofsky force-pushed on Sep 10, 2019
  186. ryanofsky commented at 8:05 PM on September 10, 2019: contributor

    Rebased cb4fce1f5c4b83d08704a0423f88add021405aae -> 47d95638c682d8efd1e46dfcb300b75aa970f7a0 (pr/atw-nomerge.63 -> pr/atw-nomerge.64) due to conflict with #16624

  187. ryanofsky force-pushed on Sep 10, 2019
  188. DrahtBot removed the label Needs rebase on Sep 10, 2019
  189. ryanofsky force-pushed on Sep 23, 2019
  190. ryanofsky force-pushed on Oct 11, 2019
  191. ryanofsky force-pushed on Oct 16, 2019
  192. ryanofsky force-pushed on Oct 22, 2019
  193. DrahtBot added the label Needs rebase on Oct 23, 2019
  194. ryanofsky force-pushed on Oct 23, 2019
  195. DrahtBot removed the label Needs rebase on Oct 23, 2019
  196. DrahtBot added the label Needs rebase on Oct 24, 2019
  197. ryanofsky force-pushed on Oct 24, 2019
  198. DrahtBot removed the label Needs rebase on Oct 24, 2019
  199. DrahtBot added the label Needs rebase on Nov 8, 2019
  200. ryanofsky force-pushed on Nov 8, 2019
  201. DrahtBot removed the label Needs rebase on Nov 8, 2019
  202. DrahtBot added the label Needs rebase on Nov 22, 2019
  203. meshcollider commented at 9:15 PM on November 22, 2019: contributor

    Sorry about all the rebases, Concept ACK

  204. ryanofsky force-pushed on Nov 25, 2019
  205. DrahtBot removed the label Needs rebase on Nov 25, 2019
  206. DrahtBot added the label Needs rebase on Dec 4, 2019
  207. laanwj commented at 1:17 PM on December 4, 2019: member

    Concept ACK

  208. ryanofsky force-pushed on Dec 4, 2019
  209. DrahtBot removed the label Needs rebase on Dec 4, 2019
  210. DrahtBot added the label Needs rebase on Jan 15, 2020
  211. ryanofsky force-pushed on Jan 16, 2020
  212. DrahtBot removed the label Needs rebase on Jan 16, 2020
  213. DrahtBot cross-referenced this on Feb 11, 2020 from issue wallet: Remove calls to Chain::Lock methods by ryanofsky
  214. DrahtBot cross-referenced this on Feb 11, 2020 from issue wallet: undo conflicts properly in case of blocks disconnection by ariard
  215. DrahtBot cross-referenced this on Feb 11, 2020 from issue gui: grey out used address in address book by za-kk
  216. DrahtBot cross-referenced this on Feb 11, 2020 from issue wallet: reduce loading time by using unordered maps by achow101
  217. DrahtBot cross-referenced this on Feb 12, 2020 from issue wallet: Replace %w by wallet name in -walletnotify script by promag
  218. DrahtBot cross-referenced this on Feb 17, 2020 from issue Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard
  219. DrahtBot added the label Needs rebase on Feb 17, 2020
  220. ryanofsky force-pushed on Feb 25, 2020
  221. ryanofsky commented at 5:40 PM on February 25, 2020: contributor

    Rebased dff3c12515773bef07756bb2420da5a1ac8e1438 -> 93510bcfe80385203880d1211cb94f621411839e (pr/atw-nomerge.74 -> pr/atw-nomerge.75, compare) due to minor conflict with #13339

  222. DrahtBot removed the label Needs rebase on Feb 25, 2020
  223. 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

  224. 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!

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

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

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

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

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

  230. Sjors approved
  231. Sjors commented at 11:50 AM on February 26, 2020: member

    ACK 93510bcfe80385203880d1211cb94f621411839e

  232. ryanofsky force-pushed on Feb 26, 2020
  233. ryanofsky commented at 4:19 PM on February 26, 2020: contributor

    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

  234. DrahtBot cross-referenced this on Mar 6, 2020 from issue interfaces: Describe and follow some code conventions by ryanofsky
  235. DrahtBot added the label Needs rebase on Mar 23, 2020
  236. ryanofsky force-pushed on Mar 24, 2020
  237. ryanofsky commented at 8:34 PM on March 24, 2020: contributor

    Rebased fc4df9dc2910c4a57d5e81573faf7952e1c3ee42 -> f1fc7892608fb258da86017c79ffb4b1a477014a (pr/atw-nomerge.76 -> pr/atw-nomerge.77, compare) due to conflict with #18278

  238. DrahtBot removed the label Needs rebase on Mar 24, 2020
  239. DrahtBot cross-referenced this on Mar 25, 2020 from issue Use shared pointers only in validation interface by bvbfan
  240. Sjors commented at 3:16 PM on March 31, 2020: member

    utACK f1fc7892608fb258da86017c79ffb4b1a477014a

  241. DrahtBot cross-referenced this on Apr 1, 2020 from issue Wallet passive startup by ryanofsky
  242. DrahtBot cross-referenced this on Apr 11, 2020 from issue rpc: replace raw pointers with shared_ptrs by brakmic
  243. DrahtBot added the label Needs rebase on Apr 14, 2020
  244. ryanofsky force-pushed on Apr 15, 2020
  245. ryanofsky commented at 9:41 PM on April 15, 2020: contributor

    Rebased f1fc7892608fb258da86017c79ffb4b1a477014a -> ece5d1cf46cf8c335dee4bb6affe514d7b33f306 (pr/atw-nomerge.77 -> pr/atw-nomerge.78, compare) due to conflict with #17954

  246. DrahtBot removed the label Needs rebase on Apr 15, 2020
  247. DrahtBot cross-referenced this on Apr 17, 2020 from issue Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard
  248. Sjors commented at 2:17 PM on April 19, 2020: member

    re-utACK ece5d1c (rebased)

  249. 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
  250. Get rid of unneeded CWalletTx::Init parameter bd2fbc7cdb
  251. 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
  252. 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
  253. 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
  254. DrahtBot added the label Needs rebase on May 1, 2020
  255. ryanofsky force-pushed on May 1, 2020
  256. ryanofsky commented at 3:00 PM on May 1, 2020: contributor

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

  257. DrahtBot removed the label Needs rebase on May 1, 2020
  258. Sjors commented at 10:21 AM on May 4, 2020: member

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

  259. meshcollider commented at 2:52 AM on May 5, 2020: contributor

    utACK f2892d8eb5404a34f3b09d3b9ac65d58c965e362

  260. 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:

    #include <memory>

struct Test{
    const std::unique_ptr<int>b;
    Test(const std::unique_ptr<int>& a):b{std::move(a)}{}
};
    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.

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

  262. MarcoFalke approved
  263. MarcoFalke commented at 3:14 PM on May 5, 2020: member

    ACK f2892d8

    Just some questions

  264. ryanofsky force-pushed on May 5, 2020
  265. ryanofsky commented at 4:51 PM on May 5, 2020: contributor

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

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

  267. ryanofsky commented at 5:54 PM on May 5, 2020: contributor

    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.

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

  269. meshcollider commented at 11:37 PM on May 5, 2020: contributor

    re-utACK 28b112e9bd3fd1181c0720306051ba7efca8b436

  270. meshcollider merged this on May 5, 2020
  271. meshcollider closed this on May 5, 2020
  272. sidhujag referenced this in commit 04bea9fcac on May 12, 2020
  273. xdustinface referenced this in commit 272a288258 on Aug 12, 2020
  274. xdustinface referenced this in commit fff497de59 on Aug 12, 2020
  275. xdustinface referenced this in commit 2edc79f5ec on Aug 27, 2020
  276. xdustinface referenced this in commit 2251e61a50 on Aug 28, 2020
  277. xdustinface referenced this in commit a65556bdff on Aug 29, 2020
  278. xdustinface referenced this in commit a759f99845 on Sep 1, 2020
  279. UdjinM6 referenced this in commit ebe7e80a49 on Sep 21, 2020
  280. Fabcien referenced this in commit 66ddfb30ed on Jan 27, 2021
  281. Fabcien referenced this in commit 2e0a03b737 on Jan 27, 2021
  282. Fabcien referenced this in commit 6bde49b668 on Jan 27, 2021
  283. Fabcien referenced this in commit 19dfc49276 on Jan 27, 2021
  284. Fabcien referenced this in commit 846f417721 on Jan 27, 2021
  285. gades referenced this in commit 06ff74715d on Mar 8, 2022
  286. bitcoin locked this on Aug 18, 2022
Contributors
ryanofskyTheBlueMattkallewoofMarcoFalkepromagSjorssipapracticalswiftDrahtBotmeshcolliderachow101ariardlaanwj
Labels
RefactoringWallet
Linked (view graph)
#8456 [RPC] Simplified bumpfee command.#9369 Factor out CWallet::nTimeSmart computation into a method.#9673 Set correct metadata on bumpfee wallet transactions#9680 Unify CWalletTx construction#11906 Travis x86_64-apple-darwin11 errors: "Configuring boost... /bin/sh: 1: requires: not found"#12607 depends: Remove ccache#12681 Fix ComputeTimeSmart test failure with -DDEBUG_LOCKORDER#12889 doc: add qrencode to brew install instructions (#1)#12992 [wallet] Add wallet name to log messages#13437 wallet: Erase wtxOrderd wtx pointer on removeprunedfunds#13622 Remove mapRequest tracking that just effects Qt display.#13651 [moveonly] Extract CWallet::MarkInputsDirty, and privatize AddToWalletIfInvolvingMe#13774 Return void instead of bool for functions that cannot fail#13825 [wallet] Kill accounts#14023 Remove accounts rpcs#14711 Remove uses of chainActive and mapBlockIndex in wallet code#15288 Remove wallet -> node global function calls#15948 refactor: rename chainActive#16341 Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)#16582 test: Rework ci (Use travis only as fallback env)#16624 wallet: encapsulate transactions state
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: 2026-05-20 06:55 UTC