wallet: Improve CWallet:MarkDestinationsDirty #17889

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.

fanquake added the label Wallet on Jan 7, 2020

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

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

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

DrahtBot added the label Needs rebase on Jan 7, 2020

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.

promag force-pushed on Jan 8, 2020

DrahtBot removed the label Needs rebase on Jan 8, 2020

promag force-pushed on Jan 9, 2020

fjahr commented at 6:12 PM on January 9, 2020: member

Code review ACK c9a314c6dd3bf48f5bc58e606470b39a3c963724

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:

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

ACK c9a314c6dd3bf48f5bc58e606470b39a3c963724

promag force-pushed on Jan 9, 2020

promag commented at 6:57 PM on January 9, 2020: member

Fixed typo "conditiont" as reported by @instagibbs.

promag force-pushed on Jan 15, 2020

promag commented at 9:30 AM on January 15, 2020: member

Rebased now that base PR #17843 is merged.

promag requested review from meshcollider on Jan 15, 2020

fjahr commented at 1:02 PM on January 15, 2020: member

Re-ACK fd6d067

fanquake requested review from achow101 on Jan 15, 2020

achow101 commented at 6:18 PM on January 15, 2020: member

ACK fd6d0679e7c3a956afc273fa38f64dab3f0ed223

achow101 approved

fanquake requested review from instagibbs on Jan 16, 2020

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.

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 😞

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

utACK fd6d0679e7c3a956afc273fa38f64dab3f0ed223

wallet: Improve CWallet:MarkDestinationsDirty 2b1641492f

promag force-pushed on Jan 17, 2020

meshcollider commented at 1:14 AM on January 17, 2020: contributor

re-utACK 2b1641492fbf81e2c5a95f3e580811ca8700adc5

Only change is typo fix

meshcollider referenced this in commit 7fb94c0ed4 on Jan 17, 2020

meshcollider merged this on Jan 17, 2020

meshcollider closed this on Jan 17, 2020

promag deleted the branch on Jan 17, 2020

jasonbcox referenced this in commit 7cec11d3ed on Oct 7, 2020

DrahtBot locked this on Feb 15, 2022