wallet: add cachable amounts for caching credit/debit values

kallewoof commented at 2:42 am on April 10, 2019: member

This is a refactoring that will make #13756 a lot cleaner and straight-forward, since it adds another combination to the pile (watch-only * spendable * reused).

It’s also a nice change in general.

kallewoof force-pushed on Apr 10, 2019

fanquake added the label Wallet on Apr 10, 2019

kallewoof force-pushed on Apr 10, 2019

in src/script/ismine.h:36 in 17d2688093 outdated

35+{
36+    bool m_cached[4] = {0,0,0,0};
37+    CAmount m_value[4];
38+    void Reset()
39+    {
40+        memset(m_cached, 0, 4);

gwillen commented at 4:46 am on April 10, 2019:

4 * sizeof(bool) – sizeof(bool) is not required by the standard to be 1. (And even though that would be stupid, the standard is pretty explicit about it.)

Or perhaps instead, more typesafe / c++ style: std::fill(std::begin(m_cached), std::end(m_cached), false);, which I have not tested. This version would also fix the need for the constant “4” here.

in src/wallet/wallet.cpp:1927 in 17d2688093 outdated

1923@@ -1924,33 +1924,25 @@ std::set<uint256> CWalletTx::GetConflicts() const
1924     return result;
1925 }
1926 
1927+CAmount CWalletTx::GetCachable(CachableAccount& acct, bool credit, const isminefilter& filter, bool recalculate) const

gwillen commented at 5:02 am on April 10, 2019:

I know that various people have different feelings about this practice, but invocations would be more readable and less error-prone if bool credit were an enum class of CREDIT or DEBIT.

(I feel less strongly about recalculate because it’s unambiguous which direction is “off” vs “on”, and it defaults to “off”.)

In fact, better still – since any given CachableAccount is either credit or debit, why not have the account carry the flag, and remove the error-proneness of passing it by hand entirely?

kallewoof commented at 7:11 am on April 10, 2019:

Great idea, done.

in src/script/ismine.h:32 in 17d2688093 outdated

31 isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey);
32 isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest);
33 
34+struct CachableAccount
35+{
36+    bool m_cached[4] = {0,0,0,0};

gwillen commented at 5:04 am on April 10, 2019:

I know it seems obvious now, but it would probably help future readers a bit to comment the fact that these correspond to the four values of isminetype. (Or you could add ISMINE_ENUM_ELEMENTS or the like and use that instead of the constant 4.)

In fact, actually, after studying the code – we aren’t really using a bitfield of IsMine types here. Only ISMINE_SPENDABLE and ISMINE_WATCHONLY are valid. Neither ISMINE_NO nor ISMINE_ALL are things we would ever want to cache here. Although I see that in #13756 you are maybe making more use of the fact that this is a bitfield, and I haven’t read that one, so I assume that informs your choices here.

kallewoof commented at 6:51 am on April 10, 2019:

Yes, there is a tiny bit of waste due to ISMINE_NO and _ALL, and yep I am preparing to use this more extensively for the avoid_reuse feature in #13756. I could make a “bridge” that makes use of these more efficiently, but I kind of enjoy the simplicity of the direct mapping.

kallewoof commented at 7:12 am on April 10, 2019:

Should add that I adopted the ISMINE_ENUM_ELEMENTS idea.

in src/wallet/wallet.cpp:2007 in 17d2688093 outdated

2003@@ -2038,22 +2004,20 @@ CAmount CWalletTx::GetAvailableCredit(interfaces::Chain::Lock& locked_chain, boo
2004         }
2005     }
2006 
2007-    if (cache) {
2008-        *cache = nCredit;
2009-        assert(cache_used);
2010-        *cache_used = true;
2011+    if (fUseCache) {

gwillen commented at 5:11 am on April 10, 2019:

The old code appears to always store into the cache, as long as filter is ISMINE_SPENDABLE or ISMINE_WATCHONLY, regardless of the value of fUseCache.

kallewoof commented at 6:53 am on April 10, 2019:

Oh, you’re right. Good catch.

in src/wallet/wallet.cpp:1991 in 17d2688093 outdated

1999-    }
2000-
2001-    if (fUseCache && cache_used && *cache_used) {
2002-        return *cache;
2003+    if (fUseCache && m_credit.m_cached[filter]) {
2004+        return m_credit.m_value[filter];

gwillen commented at 5:16 am on April 10, 2019:

I don’t know this code well enough to speak to the existing semantics of it. But it seems like, in the old code, GetCredit and GetAvailableCredit had separate caches, whereas now they share one.

kallewoof commented at 6:55 am on April 10, 2019:

Yep, you’re correct! Fixing.

kallewoof force-pushed on Apr 10, 2019

kallewoof commented at 7:12 am on April 10, 2019: member

@gwillen Thank you for reviewing! I have addressed your comments, I believe.

DrahtBot commented at 7:21 am on April 10, 2019: 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:

#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)

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.

in src/script/ismine.h:23 in 5b4ba10713 outdated

22-    ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE
23+    ISMINE_NO         = 0,
24+    ISMINE_WATCH_ONLY = 1 << 0,
25+    ISMINE_SPENDABLE  = 1 << 1,
26+    ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
27+    ISMINE_ENUM_ELEMENTS = ISMINE_ALL + 1,

promag commented at 7:45 am on April 10, 2019:

I may be wrong but I think you can drop = ISMINE_ALL + 1.

nit, either align all = or none?

in src/script/ismine.h:37 in 5b4ba10713 outdated

36+{
37+    enum Type {
38+        CREDIT,
39+        DEBIT,
40+    } m_type{CREDIT};
41+    bool m_cached[ISMINE_ENUM_ELEMENTS];

promag commented at 7:49 am on April 10, 2019:

nit, m_cached could be an int, or better std::bitset.

laanwj commented at 8:50 am on April 10, 2019: member

Concept ACK! this groups similar functionality together making it, overall, easier to understand. I was afraid for a bit with the word “accounts” though (having just finally deprecated that).

kallewoof force-pushed on Apr 10, 2019

kallewoof commented at 9:36 am on April 10, 2019: member

Thanks for review @promag. I believe I addressed all your comments.

kallewoof commented at 9:37 am on April 10, 2019: member

@laanwj Maybe I should have picked a different name! I think “account” works in this case, though… :)

kallewoof force-pushed on Apr 10, 2019

laanwj commented at 12:11 pm on April 10, 2019: member

@laanwj Maybe I should have picked a different name! I think “account” works in this case, though… :)

Yes to be clear it was not a suggestion to change it :slightly_smiling_face: Would only have been a potential concern if it would end up in the RPC API.

gwillen commented at 9:13 pm on April 10, 2019: contributor

utACK 3b9a9a4

in src/script/ismine.h:40 in 3b9a9a44f0 outdated

39+        CREDIT,
40+        DEBIT,
41+    } m_type{CREDIT};
42+    std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;
43+    CAmount m_value[ISMINE_ENUM_ELEMENTS];
44+    CachableAccount(Type type = CREDIT) : m_type(type)

promag commented at 9:29 am on April 11, 2019:

Add explicit. I’d remove = CREDIT.

in src/script/ismine.h:37 in 3b9a9a44f0 outdated

36+struct CachableAccount
37+{
38+    enum Type {
39+        CREDIT,
40+        DEBIT,
41+    } m_type{CREDIT};

promag commented at 9:29 am on April 11, 2019:

Remove, it is initialized in the constructor. Also, should be const.

MarcoFalke commented at 1:38 pm on April 11, 2019:

Could be an enum class?

kallewoof commented at 2:44 pm on April 11, 2019:

Made enum class but not const. It doesn’t look like ‘const enum class’ ever occurs anywhere else, so assuming it’s not a thing you do..

promag commented at 2:53 pm on April 11, 2019:

I mean ... const m_type, meaning it can’t change after initialization.

kallewoof commented at 3:10 pm on April 11, 2019:

Ah, I see.

in src/script/ismine.h:42 in 3b9a9a44f0 outdated

41+    } m_type{CREDIT};
42+    std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;
43+    CAmount m_value[ISMINE_ENUM_ELEMENTS];
44+    CachableAccount(Type type = CREDIT) : m_type(type)
45+    {
46+        Reset();

promag commented at 9:31 am on April 11, 2019:

std::bitset is already initialized with zeros.

in src/script/ismine.h:48 in 3b9a9a44f0 outdated

47+    }
48+    void Reset()
49+    {
50+        m_cached.reset();
51+    }
52+    CachableAccount& Set(const isminefilter& filter, CAmount value)

promag commented at 9:31 am on April 11, 2019:

I don’t see a reason to pass enum values by const reference? (despite the fact there’s some occurrences in the code).

promag commented at 9:34 am on April 11, 2019:

I don’t see the return value being used so I’d drop it.

kallewoof force-pushed on Apr 11, 2019

kallewoof commented at 2:34 pm on April 11, 2019: member

@promag @MarcoFalke Thank you for the review! I believe I addressed all your feedback.

kallewoof force-pushed on Apr 11, 2019

jnewbery added this to the "Blockers" column in a project

in src/script/ismine.h:47 in fda0ade007 outdated

46+    {
47+        m_cached.reset();
48+    }
49+    void Set(isminefilter filter, CAmount value)
50+    {
51+        m_cached[filter] = true;

promag commented at 10:09 am on April 14, 2019:

nit, use http://www.cplusplus.com/reference/bitset/bitset/set/:

0m_cached.set(filter)

in src/script/ismine.h:42 in fda0ade007 outdated

42+    std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;
43+    CAmount m_value[ISMINE_ENUM_ELEMENTS];
44+    explicit CachableAccount(Type type) : m_type(type) {}
45+    void Reset()
46+    {
47+        m_cached.reset();

promag commented at 10:11 am on April 14, 2019:

Since CachableAccount members are public you could ditch Reset and call m_cached.reset() directly. Otherwise you could make members private and add IsCached(), Get() and Type() accessors.

kallewoof commented at 0:36 am on April 15, 2019:

I think leaving it public is ok, but I can be convinced to make it private. For now, I made Reset inline, which basically has the same effect as what you are suggesting without exposing too many innards.

promag commented at 10:12 am on April 14, 2019: member

utACK fda0ade00739f30bdfa9d576371a350c1c4ade5f.

kallewoof force-pushed on Apr 15, 2019

in src/wallet/wallet.cpp:1970 in 1603501eaa outdated

1990-        if (fUseCache && fImmatureCreditCached)
1991-            return nImmatureCreditCached;
1992-        nImmatureCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
1993-        fImmatureCreditCached = true;
1994-        return nImmatureCreditCached;
1995+        if (fUseCache && m_credit_immature.m_cached[ISMINE_SPENDABLE]) {

meshcollider commented at 6:51 am on April 15, 2019:

Why not simplify to just return GetCachable(m_credit_immature, ISMINE_SPENDABLE, !fUseCache);? (same in GetImmatureWatchOnlyCredit)

kallewoof commented at 7:08 am on April 15, 2019:

Good point! Fixed.

meshcollider commented at 6:58 am on April 15, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15780/commits/1603501eaa6b292b3dc75e96836568ecc7cc73f6, I like this cleanup :)

kallewoof force-pushed on Apr 15, 2019

promag commented at 7:16 am on April 15, 2019: member

utACK 18f23ae0c5599f2246e58e5293f7426471b73e9c, only change is to use GetCachable in GetImmatureCredit and GetImmatureWatchOnlyCredit`.

in src/script/ismine.h:34 in 18f23ae0c5 outdated

33 isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey);
34 isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest);
35 
36+struct CachableAccount
37+{
38+    enum class Type : uint8_t {

ryanofsky commented at 5:54 pm on April 15, 2019:

It doesn’t make sense for CachableAccount to contain a Type enum, or an m_type runtime member. The type of cache variable isn’t a runtime value that needs to be stored. This is kind of like declaring a fixed width variable to store an int, and then declaring another variable to store the fixed width.

The only place m_type is used is inside the GetCachable() function, so type could just be an argument to that function. This would make GetCachable() calls more straightforward to understand, because you wouldn’t have to look in a separate part of the code for CachableAccount object construction to see the type of call. You could also simplify the GetCachable() by dropping the cached variable reference argument. So I guess I’m suggesting:

0enum CacheType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT };
1CAmount GetCachable(CacheType type, const isminefilter& filter, bool recalculate) const;

gwillen commented at 6:59 pm on April 15, 2019:

The code is this way because I specifically objected to passing the CacheType to GetCacheable, which is how it was written before, because the CacheType is in fact a property of the CachableAccount object: any given such object is always either a credit account or a debit account, and it is always an error to call GetCachable with any CacheType other than the correct one for the particular CachableAccount object. Associating the type with the object prevents this error.

What I thought you were getting at in the first paragraph was that the type of a CachableAccount is a static property that is known at compile time. This I agree with, and you could certainly make it a type parameter, although I think that’s unusually esoteric for C++ code, and making it an ordinary member is perfectly defensible. But I think not storing it at all, and leaving it implicit, is not defensible.

ryanofsky commented at 7:24 pm on April 15, 2019:

@gwillen it sounds like you are worried about an error that could have occurred in a previous or hypothetical version of the code where GetCacheable function took both CachableAmount and Type arguments. I agree that this would be more error prone than the current code, but it’s not what I’m suggesting.

The current code avoids one problem (type being inconsistent with CachableAmount variable) but stores extra runtime data for no reason and adds a weird action-at-distance thing where you have to look 3 different places to see what GetCacheable will do (call site, function definition, cache variable construction). I don’t there is anything wrong with actual interface I suggested:

0enum CacheType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT };
1CAmount GetCachable(CacheType type, const isminefilter& filter, bool recalculate) const;

If you don’t like it for some reason, that’s fine. I already acked this PR in its current form. But if you want to call my suggestion “not defensible” you should be able to point out some actual error or mistake or problem that could it could cause.

gwillen commented at 8:30 pm on April 15, 2019:

It was not hypothetical, it was indeed the previous version of the code, which I think you can see in the history of the PR (but maybe a rebase wiped it out.) I responded too quickly because I was frustrated that you appeared to be suggesting to go back to a previous version, without addressing my comments discussing why I was opposed to the previous version. In fact I did misunderstand your suggestion, and the thing you actually suggested makes sense to me. Sorry for responding the way I did.

kallewoof commented at 0:13 am on April 16, 2019:

@ryanofsky That sounds reasonable, but how were you envisioning accessing the corresponding cache type from inside GetCachable? I started rewriting the code and ended up with

0-    mutable CachableAccount m_debit{CachableAccount::Type::DEBIT};
1-    mutable CachableAccount m_credit{CachableAccount::Type::CREDIT};
2-    mutable CachableAccount m_credit_immature{CachableAccount::Type::CREDIT};
3-    mutable CachableAccount m_credit_available{CachableAccount::Type::CREDIT};
4+    mutable CachableAccount accounts[CACHETYPE_ENUM_ELEMENTS];

which seems like it will degrade readability.

ryanofsky commented at 1:01 am on April 16, 2019:

re: https://github.com/bitcoin/bitcoin/pull/15780/files#r275588760 from kallewoof

I started rewriting the code and ended up with

Yes, I would probably implement this with an array. But a smaller change would be to use a conditional:

0auto& cache = type == CREDIT ? m_credit : type == DEBIT ? m_debit : ...;

which seems like it will degrade readability.

Where specifically do you think this decreases readability?

Readability outside the GetCached function is improved by this suggestion because it replaces a complicated in/out parameter with a simple enum mode argument.
Readability inside the GetCached function should be improved even if the code is longer by a line or two because a layer of indirection is gone and you can see the actual objects that are being read and written to instead of having to trace through aliases.
Overall readability should be improved because instead of cache struct instances being accessed from many parts of wallet code they are accessed only in one place.

in src/wallet/wallet.cpp:1981 in 18f23ae0c5 outdated

1977@@ -2006,23 +1978,15 @@ CAmount CWalletTx::GetAvailableCredit(interfaces::Chain::Lock& locked_chain, boo
1978     if (pwallet == nullptr)
1979         return 0;
1980 
1981+    bool got_cache = filter == ISMINE_SPENDABLE || filter == ISMINE_WATCH_ONLY;

ryanofsky commented at 6:11 pm on April 15, 2019:

Is purpose of the got_cache variable just to avoid changing behavior in this PR? If so, would suggest renaming and commenting:

0// Avoid caching ismine or NONE or ALL cases (could remove this check and simplify in the future).
1const bool allow_cache = filter == ISMINE_SPENDABLE || filter == ISMINE_WATCH_ONLY;

kallewoof commented at 0:17 am on April 16, 2019:

Makes sense, changed.

in src/wallet/wallet.cpp:1982 in 18f23ae0c5 outdated

1977@@ -2006,23 +1978,15 @@ CAmount CWalletTx::GetAvailableCredit(interfaces::Chain::Lock& locked_chain, boo
1978     if (pwallet == nullptr)
1979         return 0;
1980 
1981+    bool got_cache = filter == ISMINE_SPENDABLE || filter == ISMINE_WATCH_ONLY;
1982+    fUseCache &= got_cache;

ryanofsky commented at 6:20 pm on April 15, 2019:

It seems pointless to overwrite the fUseCache argument. It would seem clearer just to write fUseCache && allow_cache in the one place the argument is used below to avoid having its value change in the middle of the function.

kallewoof commented at 0:17 am on April 16, 2019:

Good point, fixed.

ryanofsky approved

ryanofsky commented at 6:41 pm on April 15, 2019: member

utACK 18f23ae0c5599f2246e58e5293f7426471b73e9c. Overall this is an improvement and looks correct.

in src/script/ismine.h:32 in d7e1144005 outdated

31 typedef uint8_t isminefilter;
32 
33 isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey);
34 isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest);
35 
36+struct CachableAccount

ryanofsky commented at 1:11 am on April 16, 2019:

I just noticed the struct is called CachableAccount and not CachableAmount. This seems like an unusual choice, because while it is clear this struct is a wrapper around an amount (subdivided into spendable and watchonly parts), it’s at all not clear why we’d ever think of instances of these structs as “accounts.” Not even to mention in the past history of accounts in the wallet…

I’d suggest at least renaming CachableAccount to CachableAmount, though I think more ideally this would:

Rename CacheableAccount to CachedAmount and add doxygen comment: “Cached amount subdivided into watchonly and spendable parts.”
Rename GetCachable to GetAmount

kallewoof commented at 3:15 am on April 16, 2019:

It is not always cached (hence the bitset), so I think “cachable” sounds better in this case. I agree on the amount vs account deal though. “An amount subdivided into spendable and watchonly parts” is exactly what this is.

ryanofsky commented at 4:16 pm on April 17, 2019:

re: #15780 (review) from kallewoof

It is not always cached (hence the bitset), so I think “cachable” sounds better in this case.

Ok, but this is kind of funny logic. I wonder if you also prefer to call pointers “pointables” because sometimes they don’t point to valid objects.

kallewoof commented at 1:11 am on April 18, 2019:

If they would automatically deduce and/or instantiate the appropriate value/object when they didn’t point to valid objects, they would definitely no longer just be “pointers”.

ryanofsky approved

ryanofsky commented at 1:22 am on April 16, 2019: member

utACK d7e11440052f44507a1b658350ee899986d1721d. Thanks for considering my suggestions. Would suggest squashing commits so the change is more readable when this is merged.

I left some new suggestions, but feel free to ignore them if not helpful.

kallewoof force-pushed on Apr 16, 2019

kallewoof renamed this:
~~wallet: add cachable accounts for caching credit/debit values~~
wallet: add cachable amounts for caching credit/debit values
on Apr 16, 2019

kallewoof commented at 3:20 am on April 16, 2019: member

@ryanofsky Thanks for the review! I tried using an array rather than 4 ivars, and it looks OK to me so sticking with that.

kallewoof force-pushed on Apr 16, 2019

jonasschnelli commented at 7:40 am on April 16, 2019: contributor

Finally someone did this. utACK f801212c7170fc56acdc0c61a0f33b5b103753a2 Looks like almost ready to merge. Would be great if @ryanofsky and @promag could accept/comment (on) the newest changes.

in src/wallet/wallet.h:372 in f801212c71 outdated

375-    mutable bool fAvailableCreditCached;
376-    mutable bool fWatchDebitCached;
377-    mutable bool fWatchCreditCached;
378-    mutable bool fImmatureWatchCreditCached;
379-    mutable bool fAvailableWatchCreditCached;
380+    enum CacheType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, CACHETYPE_ENUM_ELEMENTS };

promag commented at 2:05 pm on April 16, 2019:

This should be AmountType/Kind/Category/...?

kallewoof commented at 2:44 pm on April 16, 2019:

Yeah, I agree. Changing to AmountType.

promag commented at 2:09 pm on April 16, 2019: member

utACK f801212, nice simplifications over last review. Refactor looks correct, behavior is the same.

kallewoof force-pushed on Apr 16, 2019

MarcoFalke commented at 3:46 pm on April 17, 2019: member

Would be nice if the benchmark could make it in first, so that we are not accidentally regressing on performance.

test: Add wallet_balance benchmark #15779

in src/script/ismine.h:35 in d8fdf6b670 outdated

31 typedef uint8_t isminefilter;
32 
33 isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey);
34 isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest);
35 
36+struct CachableAmount

ryanofsky commented at 3:55 pm on April 17, 2019:

Note: Could add doxygen comment suggested previously #15780 (review)

0//! Cached amount subdivided into watchonly and spendable parts.

kallewoof commented at 1:15 am on April 18, 2019:

Oops, I missed that part. Thanks, done. (I used /**...*/)

in src/wallet/wallet.cpp:1935 in d8fdf6b670 outdated

1931@@ -1932,33 +1932,26 @@ std::set<uint256> CWalletTx::GetConflicts() const
1932     return result;
1933 }
1934 
1935+CAmount CWalletTx::GetCachable(AmountType type, const isminefilter& filter, bool recalculate) const

ryanofsky commented at 4:01 pm on April 17, 2019:

Note: Could call this GetAmount instead of GetCachable as suggested previously #15780 (review) because this returns an amount and now handles caching internally instead of taking a cache reference.

kallewoof commented at 1:16 am on April 18, 2019:

The recalculate bool is for the cache, though. Maybe GetCachableAmount is better. Using that.

ryanofsky approved

ryanofsky commented at 4:12 pm on April 17, 2019: member

utACK d8fdf6b670e179282faff4ff57e37a6047efac9c. Thanks for the changes!

Would be nice if the benchmark could make it in first, so that we are not accidentally regressing on performance.

I’ll review the benchmark, but I’d be pretty surprised if this affects performance significantly, since it is a basically a fancy variable rename, and doesn’t really change existing logic.

kallewoof force-pushed on Apr 18, 2019

ryanofsky approved

ryanofsky commented at 4:03 pm on April 18, 2019: member

utACK 46a7d7b1983ebf0a98b96a09d83d9f63ab81dbcc. Just method rename and new code comment since last review.

in src/wallet/wallet.cpp:1979 in 46a7d7b198 outdated

1999-        if (fUseCache && fImmatureCreditCached)
2000-            return nImmatureCreditCached;
2001-        nImmatureCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
2002-        fImmatureCreditCached = true;
2003-        return nImmatureCreditCached;
2004+        return GetCachableAmount(IMMATURE_CREDIT, ISMINE_SPENDABLE, !fUseCache);

ryanofsky commented at 4:18 pm on April 18, 2019:

I don’t want to suggest a change for this PR, since it’s good that this just rearranges variables and doesn’t change any computation.

But in the future, it seems like IMMATURE_CREDIT could be replaced with CREDIT here without affecting anything, and IMMATURE_CREDIT and AVAILABLE_CREDIT could go away. Unless I’m missing something, it seems like a lot of the previous variables like nImmatureCreditCached were just redundant.

kallewoof commented at 10:57 pm on April 18, 2019:

Nice! That would definitely improve things. I agree that’s beyond this PR but will gladly dig into it if nobody else does after merge.

promag commented at 7:01 pm on April 18, 2019: member

utACK 46a7d7b.

in src/script/ismine.h:38 in 46a7d7b198 outdated

36+/**
37+ * Cachable amount subdivided into watchonly and spendable parts.
38+ */
39+struct CachableAmount
40+{
41+    std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;

MarcoFalke commented at 7:18 pm on April 22, 2019:

Could add a comment to say that NO and ALL are never (supposed to be) cached?

kallewoof commented at 11:19 pm on April 22, 2019:

Thanks, done. I also fixed typo in wallet.cpp#1990 (said “Avoid caching ismine or NONE or all cases” before).

ryanofsky commented at 2:38 pm on April 23, 2019:

The two comments contradict each other:

0// NO and ALL are never (supposed to be) cached

0// Avoid caching ismine for NO or ALL cases (could remove this check and simplify in the future).

If it’s harmful to cache these values, we should drop the second comment and explain the harm. If it’s not harmful, we should drop the first comment because it’s misleading and irrelevant.

In the future, it would be good to get rid of all unused variables and special cases by switching to a 2 part cache or just caching the 4 parts unconditionally.

MarcoFalke approved

MarcoFalke commented at 7:25 pm on April 22, 2019: member

utACK 46a7d7b1983ebf0a98b96a09d83d9f63ab81dbcc

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3utACK 46a7d7b1983ebf0a98b96a09d83d9f63ab81dbcc
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjt+Qv/ZmeTg9qRNFbvQvlhEqwC8YTlz79XldXWXALt5wZpPFVL721OfLpB0Dq3
 8aGVwaxUsIICmWY7McMpixs/Q563+GX5+mnwAnq7R+lLMfz3ESAqExnfsouxVVdII
 9PZaNXh9jvXbrDGzz4MkyJKgMa7hx54vcg180wy5MQHCB1DTxT5iDrFeSxVKLlGk2
10cQOqqeob93Xa7pABisE7FnCZkPo2YsRJAY8D57XUb+9W3uomJFUlVeJn6UjveMNI
11CyHMmO2zmICeWTYdlGn7BcGcEpMUM2zZs3fNgp2AnhixgeSCcL19HPq5kL/ASH7l
12ln3uguLDEspSUD7u7vLEoqQcfYT0QO4YwJkhvbFwhKUA2qFvnqQg61bVee+C2fro
13RrN2uodY6nhGa55ZtV+JmJDu4FqNqK3sBydGnz7LSGhNcsvJcW7f+DN/YoSssyHK
14fpNkz6Wyilsm0i1xw6lZ+XJn8hfk87G6Jvo/D+mK3howtKebC0u7npQ0WGp4N+rF
15Amjiv43T
16=ONE8
17-----END PGP SIGNATURE-----

MarcoFalke added the label Refactoring on Apr 22, 2019

wallet: add cachable amounts for caching credit/debit values c9e6e7ed79

kallewoof force-pushed on Apr 22, 2019

promag commented at 9:53 am on April 23, 2019: member

utACK c9e6e7e.

MarcoFalke commented at 11:38 am on April 23, 2019: member

re-utACK c9e6e7ed79886b702614e627a966fc9a4850f8f0 (Only change is comments)

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-utACK c9e6e7ed79886b702614e627a966fc9a4850f8f0 (Only change is comments)
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgtWwv+OiZ5fOCqUEVQRgsFHUXeXu+mFPLRT9TMP4HkO/fZbMgFg3gZUAMw9FCV
 8R8973LZ2iSwK/IpWhrU/VA+jCvgQwIT0RP52qopIJKIb6JN+Bguf9Cw4Dx7nL+2u
 9P393qaa6KIGcV/9REelCmWyIE3EgTnkTA/AJBAaDqvWqghMQAYrM3r19bDb9dHAe
105Xx+UF/FmmG/3xIuoh6EV75g2njVT/LbXRYYXOwjyF5nFbYN89CkZt1pENmbgF0l
11Fw7KRb/4LdhTGP72BIEz5lM645uXDXUZHDxKUWnE18hDP0AU2udGKaa+2vMvhxGh
12NmD1+J7/XEvvzpmLI/QhQH20HTQuNgxRVrxApxRWhrNaBcf6GBmF8MjhqX+1ekSL
13pSHbt35NfsnPGiwVVenqkBeP8DDt5S7tKioHTLyno7w1XuwOr750q2iCPo0ECVj+
143NXrmd/3kCM86CKn6l0p2EiuqpGs88Gb0asHL7P1nWN2BNnMGz1lss3U448uSHFa
15ealUr82R
16=9v90
17-----END PGP SIGNATURE-----

ryanofsky approved

ryanofsky commented at 2:43 pm on April 23, 2019: member

utACK c9e6e7ed79886b702614e627a966fc9a4850f8f0. Just comment changes since last review.

laanwj merged this on Apr 23, 2019

laanwj closed this on Apr 23, 2019

laanwj referenced this in commit 2d5419feed on Apr 23, 2019

kallewoof deleted the branch on Apr 23, 2019

fanquake removed this from the "Blockers" column in a project

meshcollider referenced this in commit 44d8172323 on Jun 18, 2019

sidhujag referenced this in commit 6bcaa95d9d on Jun 19, 2019

deadalnix referenced this in commit 71716907bf on Jun 4, 2020

furszy referenced this in commit e2cc4aa411 on Aug 21, 2020

Munkybooty referenced this in commit 5ca34f593d on Oct 1, 2021

kittywhiskers referenced this in commit bb2d19507e on Dec 4, 2021

kittywhiskers referenced this in commit 6472a7ed01 on Dec 8, 2021

kittywhiskers referenced this in commit ed7240800c on Dec 12, 2021

kittywhiskers referenced this in commit 317145e2ce on Dec 13, 2021

kittywhiskers referenced this in commit aeeba338a4 on Dec 13, 2021

kittywhiskers referenced this in commit 8d843714c5 on Dec 13, 2021

DrahtBot locked this on Dec 16, 2021

wallet: add cachable amounts for caching credit/debit values #15780

Conflicts