wallet: Improve CWallet:MarkDestinationsDirty #17889

pull promag wants to merge 1 commits into bitcoin:master from promag:2020-01-wallettx-iscacheempty changing 3 files +12 −2
  1. promag commented at 1:56 PM on January 7, 2020: member

    Improve CWallet:MarkDestinationsDirty by skipping transactions that already have the cache invalidated. Skipping a transaction avoids at worst case extracting all output destinations.

  2. fanquake added the label Wallet on Jan 7, 2020
  3. in src/wallet/wallet.h:322 in 182fe73410 outdated 
     309 | @@ -310,6 +310,7 @@ class CWalletTx
 310 |      enum AmountType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
 311 |      CAmount GetCachableAmount(AmountType type, const isminefilter& filter, bool recalculate = false) const;
 312 |      mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
 313 | +    mutable bool m_is_cache_empty{true};
    instagibbs commented at 2:01 PM on January 7, 2020:

    a comment for semantics and uses would be nice

    promag commented at 10:43 AM on January 9, 2020:

    @instagibbs done (I think?)

  4. DrahtBot commented at 6:34 PM on January 7, 2020: 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:

    • #17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple 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.

  5. DrahtBot added the label Needs rebase on Jan 7, 2020
  6. promag cross-referenced this on Jan 8, 2020 from issue wallet: Reset reused transactions cache by fjahr
  7. in src/wallet/wallet.h:439 in 182fe73410 outdated 
     431 | @@ -431,11 +432,13 @@ class CWalletTx
 432 |      //! make sure balances are recalculated
 433 |      void MarkDirty()
 434 |      {
 435 | +        fChangeCached = false;
 436 | +        if (m_is_cache_empty) return;
    achow101 commented at 6:25 PM on January 8, 2020:

    I don't think we should have an early return. The reset shouldn't be very expensive, and I think it's better to be sure that the cache is cleared when m_is_cache_empty = true

    promag commented at 8:41 PM on January 8, 2020:

    Done.

  8. promag force-pushed on Jan 8, 2020
  9. promag force-pushed on Jan 8, 2020
  10. DrahtBot removed the label Needs rebase on Jan 8, 2020
  11. promag force-pushed on Jan 9, 2020
  12. fjahr commented at 6:12 PM on January 9, 2020: contributor

    Code review ACK c9a314c6dd3bf48f5bc58e606470b39a3c963724

  13. in src/wallet/wallet.h:319 in c9a314c6dd outdated 
     312 | @@ -313,6 +313,13 @@ class CWalletTx
 313 |      enum AmountType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
 314 |      CAmount GetCachableAmount(AmountType type, const isminefilter& filter, bool recalculate = false) const;
 315 |      mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
 316 | +    /**
 317 | +     * This flag is true if all m_amounts caches are empty. This is particularly
 318 | +     * usefull in places where MarkDirty is conditionally called and the
 319 | +     * conditiont can be expensive and thus can be skipped if the flag is true.
    instagibbs commented at 6:24 PM on January 9, 2020:

    conditiont?

    promag commented at 6:25 PM on January 9, 2020:

    Yes :trollface:

  14. achow101 commented at 6:52 PM on January 9, 2020: member

    ACK c9a314c6dd3bf48f5bc58e606470b39a3c963724

  15. promag force-pushed on Jan 9, 2020
  16. promag commented at 6:57 PM on January 9, 2020: member

    Fixed typo "conditiont" as reported by @instagibbs.

  17. promag force-pushed on Jan 15, 2020
  18. promag commented at 9:30 AM on January 15, 2020: member

    Rebased now that base PR #17843 is merged.

  19. promag requested review from meshcollider on Jan 15, 2020
  20. fjahr commented at 1:02 PM on January 15, 2020: contributor

    Re-ACK fd6d067

  21. fanquake requested review from achow101 on Jan 15, 2020
  22. achow101 commented at 6:18 PM on January 15, 2020: member

    ACK fd6d0679e7c3a956afc273fa38f64dab3f0ed223

  23. achow101 approved
  24. fanquake requested review from instagibbs on Jan 16, 2020
  25. instagibbs commented at 5:05 PM on January 16, 2020: member

    utACK https://github.com/bitcoin/bitcoin/pull/17889/commits/fd6d0679e7c3a956afc273fa38f64dab3f0ed223

    though I think better encapsulation of these concepts would be a plus so we can't accidentally mark dirty or set the cache without knowing.

  26. in src/wallet/wallet.h:318 in fd6d0679e7 outdated 
     312 | @@ -313,6 +313,13 @@ class CWalletTx
 313 |      enum AmountType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
 314 |      CAmount GetCachableAmount(AmountType type, const isminefilter& filter, bool recalculate = false) const;
 315 |      mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
 316 | +    /**
 317 | +     * This flag is true if all m_amounts caches are empty. This is particularly
 318 | +     * usefull in places where MarkDirty is conditionally called and the
    meshcollider commented at 1:08 AM on January 17, 2020:

    nit: typo in usefull. Not going to let it hold up merge though.

    promag commented at 1:13 AM on January 17, 2020:

    Sorry 😞

  27. meshcollider commented at 1:09 AM on January 17, 2020: contributor

    utACK fd6d0679e7c3a956afc273fa38f64dab3f0ed223

  28. wallet: Improve CWallet:MarkDestinationsDirty 2b1641492f
  29. promag force-pushed on Jan 17, 2020
  30. meshcollider commented at 1:14 AM on January 17, 2020: contributor

    re-utACK 2b1641492fbf81e2c5a95f3e580811ca8700adc5

    Only change is typo fix

  31. meshcollider referenced this in commit 7fb94c0ed4 on Jan 17, 2020
  32. meshcollider merged this on Jan 17, 2020
  33. meshcollider closed this on Jan 17, 2020
  34. promag deleted the branch on Jan 17, 2020
  35. MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
  36. jasonbcox referenced this in commit 7cec11d3ed on Oct 7, 2020
  37. bitcoin locked this on Feb 15, 2022
Contributors
promaginstagibbsDrahtBotachow101fjahrmeshcollider


instagibbs

Labels
Wallet
Linked (view graph)
#17843 wallet: Reset reused transactions cache
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:54 UTC