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

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.

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?

Great idea, done.

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.

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.

Should add that I adopted the ISMINE_ENUM_ELEMENTS idea.

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.

Oh, you're right. Good catch.

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.

Yep, you're correct! Fixing.

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

nit, either align all = or none?

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

Add explicit. I'd remove = CREDIT.

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

Could be an enum class?

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

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

Ah, I see.

std::bitset is already initialized with zeros.

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

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

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

m_cached.set(filter)

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.

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.

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

Good point! Fixed.

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:

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

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.

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

enum CacheType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT };
CAmount 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.

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.

@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

-    mutable CachableAccount m_debit{CachableAccount::Type::DEBIT};
-    mutable CachableAccount m_credit{CachableAccount::Type::CREDIT};
-    mutable CachableAccount m_credit_immature{CachableAccount::Type::CREDIT};
-    mutable CachableAccount m_credit_available{CachableAccount::Type::CREDIT};
+    mutable CachableAccount accounts[CACHETYPE_ENUM_ELEMENTS];

which seems like it will degrade readability.

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:

auto& 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.

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

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

Makes sense, changed.

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.

Good point, fixed.

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

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.

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.

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

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

Yeah, I agree. Changing to AmountType.

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

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

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

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.

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

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.

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.

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

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

The two comments contradict each other:

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

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