Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 23, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/script/ismine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,43 @@
#include <script/standard.h>

#include <stdint.h>
#include <bitset>

class CKeyStore;
class CScript;

/** IsMine() return codes */
enum isminetype
{
ISMINE_NO = 0,
ISMINE_WATCH_ONLY = 1,
ISMINE_SPENDABLE = 2,
ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE
ISMINE_NO = 0,
ISMINE_WATCH_ONLY = 1 << 0,
ISMINE_SPENDABLE = 1 << 1,
ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
ISMINE_ENUM_ELEMENTS,
};
/** used for bitflags of isminetype */
typedef uint8_t isminefilter;

isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey);
isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest);

/**
* Cachable amount subdivided into watchonly and spendable parts.
*/
struct CachableAmount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

@kallewoof kallewoof Apr 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// NO and ALL are never (supposed to be) cached
std::bitset<ISMINE_ENUM_ELEMENTS> m_cached;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CAmount m_value[ISMINE_ENUM_ELEMENTS];
inline void Reset()
{
m_cached.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
void Set(isminefilter filter, CAmount value)
{
m_cached.set(filter);
m_value[filter] = value;
}
};

#endif // BITCOIN_SCRIPT_ISMINE_H
5 changes: 2 additions & 3 deletions src/wallet/test/coinselector_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa
std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(&testWallet, MakeTransactionRef(std::move(tx)));
if (fIsFromMe)
{
wtx->fDebitCached = true;
wtx->nDebitCached = 1;
wtx->m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1);
}
COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */);
vCoins.push_back(output);
Expand Down Expand Up @@ -115,7 +114,7 @@ inline std::vector<OutputGroup>& GroupCoins(const std::vector<COutput>& coins)
{
static std::vector<OutputGroup> static_groups;
static_groups.clear();
for (auto& coin : coins) static_groups.emplace_back(coin.GetInputCoin(), coin.nDepth, coin.tx->fDebitCached && coin.tx->nDebitCached == 1 /* HACK: we can't figure out the is_me flag so we use the conditions defined above; perhaps set safe to false for !fIsFromMe in add_coin() */, 0, 0);
for (auto& coin : coins) static_groups.emplace_back(coin.GetInputCoin(), coin.nDepth, coin.tx->m_amounts[CWalletTx::DEBIT].m_cached[ISMINE_SPENDABLE] && coin.tx->m_amounts[CWalletTx::DEBIT].m_value[ISMINE_SPENDABLE] == 1 /* HACK: we can't figure out the is_me flag so we use the conditions defined above; perhaps set safe to false for !fIsFromMe in add_coin() */, 0, 0);
return static_groups;
}

Expand Down
94 changes: 27 additions & 67 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1932,33 +1932,26 @@ std::set<uint256> CWalletTx::GetConflicts() const
return result;
}

CAmount CWalletTx::GetCachableAmount(AmountType type, const isminefilter& filter, bool recalculate) const
{
auto& amount = m_amounts[type];
if (recalculate || !amount.m_cached[filter]) {
amount.Set(filter, type == DEBIT ? pwallet->GetDebit(*tx, filter) : pwallet->GetCredit(*tx, filter));
}
return amount.m_value[filter];
}

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

CAmount debit = 0;
if(filter & ISMINE_SPENDABLE)
{
if (fDebitCached)
debit += nDebitCached;
else
{
nDebitCached = pwallet->GetDebit(*tx, ISMINE_SPENDABLE);
fDebitCached = true;
debit += nDebitCached;
}
if (filter & ISMINE_SPENDABLE) {
debit += GetCachableAmount(DEBIT, ISMINE_SPENDABLE);
}
if(filter & ISMINE_WATCH_ONLY)
{
if(fWatchDebitCached)
debit += nWatchDebitCached;
else
{
nWatchDebitCached = pwallet->GetDebit(*tx, ISMINE_WATCH_ONLY);
fWatchDebitCached = true;
debit += nWatchDebitCached;
}
if (filter & ISMINE_WATCH_ONLY) {
debit += GetCachableAmount(DEBIT, ISMINE_WATCH_ONLY);
}
return debit;
}
Expand All @@ -1970,40 +1963,20 @@ CAmount CWalletTx::GetCredit(interfaces::Chain::Lock& locked_chain, const ismine
return 0;

CAmount credit = 0;
if (filter & ISMINE_SPENDABLE)
{
if (filter & ISMINE_SPENDABLE) {
// GetBalance can assume transactions in mapWallet won't change
if (fCreditCached)
credit += nCreditCached;
else
{
nCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
fCreditCached = true;
credit += nCreditCached;
}
credit += GetCachableAmount(CREDIT, ISMINE_SPENDABLE);
}
if (filter & ISMINE_WATCH_ONLY)
{
if (fWatchCreditCached)
credit += nWatchCreditCached;
else
{
nWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);
fWatchCreditCached = true;
credit += nWatchCreditCached;
}
if (filter & ISMINE_WATCH_ONLY) {
credit += GetCachableAmount(CREDIT, ISMINE_WATCH_ONLY);
}
return credit;
}

CAmount CWalletTx::GetImmatureCredit(interfaces::Chain::Lock& locked_chain, bool fUseCache) const
{
if (IsImmatureCoinBase(locked_chain) && IsInMainChain(locked_chain)) {
if (fUseCache && fImmatureCreditCached)
return nImmatureCreditCached;
nImmatureCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
fImmatureCreditCached = true;
return nImmatureCreditCached;
return GetCachableAmount(IMMATURE_CREDIT, ISMINE_SPENDABLE, !fUseCache);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return 0;
Expand All @@ -2014,23 +1987,15 @@ CAmount CWalletTx::GetAvailableCredit(interfaces::Chain::Lock& locked_chain, boo
if (pwallet == nullptr)
return 0;

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

// Must wait until coinbase is safely deep enough in the chain before valuing it
if (IsImmatureCoinBase(locked_chain))
return 0;

CAmount* cache = nullptr;
bool* cache_used = nullptr;

if (filter == ISMINE_SPENDABLE) {
cache = &nAvailableCreditCached;
cache_used = &fAvailableCreditCached;
} else if (filter == ISMINE_WATCH_ONLY) {
cache = &nAvailableWatchCreditCached;
cache_used = &fAvailableWatchCreditCached;
}

if (fUseCache && cache_used && *cache_used) {
return *cache;
if (fUseCache && allow_cache && m_amounts[AVAILABLE_CREDIT].m_cached[filter]) {
return m_amounts[AVAILABLE_CREDIT].m_value[filter];
}

CAmount nCredit = 0;
Expand All @@ -2046,22 +2011,17 @@ CAmount CWalletTx::GetAvailableCredit(interfaces::Chain::Lock& locked_chain, boo
}
}

if (cache) {
*cache = nCredit;
assert(cache_used);
*cache_used = true;
if (allow_cache) {
m_amounts[AVAILABLE_CREDIT].Set(filter, nCredit);
}

return nCredit;
}

CAmount CWalletTx::GetImmatureWatchOnlyCredit(interfaces::Chain::Lock& locked_chain, const bool fUseCache) const
{
if (IsImmatureCoinBase(locked_chain) && IsInMainChain(locked_chain)) {
if (fUseCache && fImmatureWatchCreditCached)
return nImmatureWatchCreditCached;
nImmatureWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);
fImmatureWatchCreditCached = true;
return nImmatureWatchCreditCached;
return GetCachableAmount(IMMATURE_CREDIT, ISMINE_WATCH_ONLY, !fUseCache);
}

return 0;
Expand Down
47 changes: 7 additions & 40 deletions src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,24 +369,11 @@ class CWalletTx : public CMerkleTx
std::multimap<int64_t, CWalletTx*>::const_iterator m_it_wtxOrdered;

// memory only
mutable bool fDebitCached;
mutable bool fCreditCached;
mutable bool fImmatureCreditCached;
mutable bool fAvailableCreditCached;
mutable bool fWatchDebitCached;
mutable bool fWatchCreditCached;
mutable bool fImmatureWatchCreditCached;
mutable bool fAvailableWatchCreditCached;
enum AmountType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
CAmount GetCachableAmount(AmountType type, const isminefilter& filter, bool recalculate = false) const;
mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
mutable bool fChangeCached;
mutable bool fInMempool;
mutable CAmount nDebitCached;
mutable CAmount nCreditCached;
mutable CAmount nImmatureCreditCached;
mutable CAmount nAvailableCreditCached;
mutable CAmount nWatchDebitCached;
mutable CAmount nWatchCreditCached;
mutable CAmount nImmatureWatchCreditCached;
mutable CAmount nAvailableWatchCreditCached;
mutable CAmount nChangeCached;

CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg))
Expand All @@ -403,24 +390,8 @@ class CWalletTx : public CMerkleTx
nTimeReceived = 0;
nTimeSmart = 0;
fFromMe = false;
fDebitCached = false;
fCreditCached = false;
fImmatureCreditCached = false;
fAvailableCreditCached = false;
fWatchDebitCached = false;
fWatchCreditCached = false;
fImmatureWatchCreditCached = false;
fAvailableWatchCreditCached = false;
fChangeCached = false;
fInMempool = false;
nDebitCached = 0;
nCreditCached = 0;
nImmatureCreditCached = 0;
nAvailableCreditCached = 0;
nWatchDebitCached = 0;
nWatchCreditCached = 0;
nAvailableWatchCreditCached = 0;
nImmatureWatchCreditCached = 0;
nChangeCached = 0;
nOrderPos = -1;
}
Expand Down Expand Up @@ -464,14 +435,10 @@ class CWalletTx : public CMerkleTx
//! make sure balances are recalculated
void MarkDirty()
{
fCreditCached = false;
fAvailableCreditCached = false;
fImmatureCreditCached = false;
fWatchDebitCached = false;
fWatchCreditCached = false;
fAvailableWatchCreditCached = false;
fImmatureWatchCreditCached = false;
fDebitCached = false;
m_amounts[DEBIT].Reset();
m_amounts[CREDIT].Reset();
m_amounts[IMMATURE_CREDIT].Reset();
m_amounts[AVAILABLE_CREDIT].Reset();
fChangeCached = false;
}

Expand Down