[Wallet] add cacheable amounts for caching credit/debit values

[furszy]

Built on top of #1755 and #1666 .

The goal of this PR has been to improve the CWalletTx to be able to add shielded cached balances in a much cleaner way.

Including the following changes:

• Adapted IsSolvable function from 0c8ea63 (without the witness check. Only used to check regular outputs to be able to decouple the output solvability from ismine.h enum)

• Solvability decoupled from ismine.h. Back ported 6d714c3 and 4e91820 from Separate IsMine from solvability bitcoin/bitcoin#13142.

• And lastly, adapted wallet: add cachable amounts for caching credit/debit values bitcoin/bitcoin#15780 beauty cleanup to our sources.

[furszy]

rebased after get 1755 and #1666 merged. Ready for review.

[random-zebra]

Very nice refactoring. Love the concept.
Few things (some of which can be tackled in a followup PR):

• !fUseCache = recalculate (see comment inlined for GetColdStakingDebit/GetStakeDelegationDebit).
• The change to GetUnlockedCredit/GetLockedCredit is wrong. But we should probably just remove those methods.
• GetAvailableWatchOnlyCredit can be refactored in terms of GetCachableAmount or, this too, simply removed.
• GetUnspentCredit should also be removed (as the maturity should not be checked here) and just replaced with GetCredit.

[furszy]

Great catches :) .
Agree on removing the methods that are not used (plus, not tested anywhere).

Some thoughts:
GetUnlockedCredit --> yes, this one can be removed.
GetLockedCredit --> not sure, will need to think on a replacement for this. Maybe a cached locked amount like what we have with the change amount and mark the tx dirty if it's unlocked/locked.
GetUnspentCredit --> not sure on removing this one. It's a simple wrapper that checks for maturity before get the cached amount. Side from this, totally agree on unifying it with GetCredit for sure.

[random-zebra]

Yeah I don't think we should check for IsSpent in GetCredit. That should only be done in GetAvailableCredit / GetAvailableWatchOnlyCredit, and maturity should be checked there as well (no need for a separate GetUnspentCredit wrapper imo). But well... this can be tackled/discussed in a separate PR.

[Fuzzbawls]

ACK 1bf9dc4

[random-zebra]

I think that this PR highlights a problem with CWalletTx::GetCredit / CWalletTx::GetDebit and cold staking scripts.
These functions don't take into account that isminefilter is actually a bitflag and the conditions could overlap.

For example, calling tx.GetCredit(ISMINE_ALL) (which we do, e.g., from the GUI, for the tx description) would result in a bug with a P2CS output, if the wallet has both cold staking keys, where credit is added twice (third and fourth if block):

    CAmount credit = 0;
    if (filter & ISMINE_SPENDABLE) {
        // GetBalance can assume transactions in mapWallet won't change
        credit += GetCachableAmount(CREDIT, ISMINE_SPENDABLE, recalculate, fUnspent);
    }
    if (filter & ISMINE_WATCH_ONLY) {
        credit += GetCachableAmount(CREDIT, ISMINE_WATCH_ONLY, recalculate, fUnspent);
    }
    if (filter & ISMINE_COLD) {
        credit += GetCachableAmount(CREDIT, ISMINE_COLD, recalculate, fUnspent);
    }
    if (filter & ISMINE_SPENDABLE_DELEGATED) {
        credit += GetCachableAmount(CREDIT, ISMINE_SPENDABLE_DELEGATED, recalculate, fUnspent);
    }
    return credit;

Also, this way, ISMINE_SPENDABLE_STAKEABLE is never cached (same as with ISMINE_ALL).

Fixing this would mean rewriting them as just:

CAmount CWalletTx::GetCredit(const isminefilter& filter, bool recalculate, bool fUnspent) const
{
    return GetCachableAmount(CREDIT, filter, recalculate, fUnspent);
}

CAmount CWalletTx::GetDebit(const isminefilter& filter) const
{
    if (vin.empty())
        return 0;

    return GetCachableAmount(DEBIT, filter);
}

(note that to make the the wallet_tests pass and fake out IsFromMe(), with this change, you would need to .Set(ISMINE_ALL, 1) inside add_coin).

I'm not sure why there is a comment suggesting that ISMINE_ALL should never be cached.
It seems it's just for efficiency, as in bitcoin it's just

ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE

and ISMINE_WATCH_ONLY/ISMINE_SPENDABLE types do not overlap (an output cannot be both at the same time).
But in our case it's different.
ISMINE_SPENDABLE_STAKEABLE (which is not a "power-of-2" element of isminetype) can be returned by IsMine() .

Alternatively, for now, we can just add some condition/hack for this particular situation.

But in any case, in the future, we should properly reorganize this, as it seems highly inefficient as it is: we keep a vector of 16 cacheable amounts, to actually use only 4 of them. In upstream this is less relevant (they use 2 out of 4).

[random-zebra]

Thinking more about it, for the problem explained above, we are forced to actually use the ISMINE_SPENDABLE_STAKEABLE (separate) cached amount. There is no way to distinguish the cold staking types otherwise. So, I guess, we should do something like this:

CAmount CWalletTx::GetCredit(const isminefilter& filter, bool recalculate, bool fUnspent) const
{
    CAmount credit = 0;
    if (filter & ISMINE_SPENDABLE) {
        // GetBalance can assume transactions in mapWallet won't change
        credit += GetCachableAmount(CREDIT, ISMINE_SPENDABLE, recalculate, fUnspent);
    }
    if (filter & ISMINE_WATCH_ONLY) {
        credit += GetCachableAmount(CREDIT, ISMINE_WATCH_ONLY, recalculate, fUnspent);
    }

    bool fOnlyCold = (filter & ISMINE_COLD) && !(filter & ISMINE_SPENDABLE_DELEGATED);
    bool fOnlyDelegated = !(filter & ISMINE_COLD) && (filter & ISMINE_SPENDABLE_DELEGATED);
    if (fOnlyCold) {
        credit += GetCachableAmount(CREDIT, ISMINE_COLD, recalculate, fUnspent);
    } else if (fOnlyDelegated) {
        credit += GetCachableAmount(CREDIT, ISMINE_SPENDABLE_DELEGATED, recalculate, fUnspent);
    } else if (filter & ISMINE_SPENDABLE_STAKEABLE)  {
        // cached mixed balance (sum of outputs that are either COLD, or SPENDABLE_DELEGATED,
        // counting them only once if the wallet has both coldstaking keys).
        credit += GetCachableAmount(CREDIT, ISMINE_SPENDABLE_STAKEABLE, recalculate, fUnspent);
    }
    return credit;
}

In the future, would be better to refactor m_amounts, giving up on the simplicity of the direct mapping, and using a vector of only 5 cached amounts.

[furszy]

Good catch zebra :) , cool that this work popped up few hidden issues and good discussions.

Have something in mind, do we really need the ISMINE_SPENDABLE_DELEGATED type at all?. It seems to be just a redundant representation of a ISMINE_SPENDABLE sub type that can be simplified.

When the wallet wants to check any output credit, it already know whether is a P2CS or not. If the P2CS is spendable then it's a ISMINE_SPENDABLE (the wallet has the owner priv key), if not then is ISMINE_COLD or a ISMINE_WATCH_ONLY.

Going more general to describe this a little bit better. IsMine() shouldn't return the type of the output script (can already be done outside of the function). It should purely return the wallet spending authority type.
Example:

if (output.IsPayToColdStaking() && IsMine(output) == ISMINE_SPENDABLE) {
 // we know here that the output is a `ISMINE_SPENDABLE_DELEGATED`
 // without having the extra enum type and any other logic complication.
}

Doing this structural modification, we would be erasing the problem's root and making the flow simpler.

[random-zebra]

Yes, we can do that, but then you would lose the mapping between cached amounts and ismine types.
And it wouldn't remove the problem: if you want to cache delegated/cold balance per-tx, for efficiency, you need to consider the situation of having both keys (so, an additional cached amount).

What we could do, though, is return ISMINE_SPENDABLE_DELEGATED also in the case of a wallet with both keys. This would change slightly the semantic of "cold balance" (as it would not include self-delegated outputs anymore) but it would remove the problem of having "overlapping" cached amounts: an output could be SPENDABLE_DELEGATED or COLD, but not both.

Merging SPENDABLE_DELEGATED with SPENDABLE, as you suggest, would be an additional step, but then, again, we would lose the mapping (and I think it's convenient to have a separate cached balance just for delegations).

Anyway, this kind of refactoring would require its own PR, so for now we can either add a comment in GetCredit/GetDebit, or maybe temporarily use an hack like the one I suggested above.

[furszy]

loose the one-to-one isminetype -> value cache mapping is good actually. As you said in the first comment, we are having there more balance types than what we are really using. We just need to store the basic types there, not all the possible combinations (those would be duplicating amounts, adding few other values already cached into other cached space).

ofc not planning to do it here. Just continuing the nice convo where we started it :) .
Will add the quick hack to continue moving on and we can back to this talk after #1810 (which adds the test coverage for the cache balances).

[random-zebra]

loose the one-to-one isminetype -> value cache mapping is good actually.

I agree.

[furszy]

Added a commit following up the convo and zebra's last suggestion.
ISMINE_SPENDABLE_DELEGATED will be now taking precedence over ISMINE_COLD, simple and clean solution.
Ready for review.

[random-zebra]

wallet_import_stakingaddress needs to be updated for the semantic change to cold balance (node 0 does self-delegations at the beginning). We should check the delegated balance on it now (line 50), instead of the cold staking balance.

[furszy]

Done 👍

[random-zebra]

utACK e02edd3