#27286 wallet: Keep track of the wallet's own transaction outputs in memory
All code comments in [] are my own.
CWallet's track transactions of interest in an unordered map (mapWallet) from txid
(uint256) to CWalletTx:
/**
* A CWallet maintains a set of transactions and balances, and provides the ability to create new transactions.
*/
class CWallet final : public WalletStorage, public interfaces::Chain::Notifications
{
public:
/** Map from txid to CWalletTx for all transactions this wallet is
* interested in, including received and sent transactions. */
std::unordered_map<uint256, CWalletTx, SaltedTxidHasher> mapWallet GUARDED_BY(cs_wallet);
}CWalletTx is a CTransactionRef (shared pointer to a CTransaction), with
some metadata the wallet cares about, like the "comment/to" field which I think
the GUI displays, and some feebumping relationships for the bumpfee if they
exist, like the txid this replaces, or the txid this was replaced by, and
something called nTimeSmart, which seems to be a topic worth returning to but
which I'll avoid for now.
One big picture question I have about CWalletTx: Is any of this metadata
relevant to only the output we care about? If so, how are metadata for
transactions containing multiple outputs we care about handled?
//! All possible CWalletTx states
using TxState = std::variant<TxStateConfirmed, TxStateInMempool, TxStateBlockConflicted, TxStateInactive, TxStateUnrecognized>;
// [ Used to imitate the key-value map in which
// transaction data is serialized in the DB. ]
typedef std::map<std::string, std::string> mapValue_t;
/**
* A transaction with a bunch of additional info that only the owner cares about.
* It includes any unrecorded transactions needed to link it back to the block chain.
*/
class CWalletTx
{
public:
/**
* Key/value map with information about the transaction.
*
* The following keys can be read and written through the map and are
* serialized in the wallet database:
*
* "comment", "to" - comment strings provided to sendtoaddress,
* and sendmany wallet RPCs
* "replaces_txid" - txid (as HexStr) of transaction replaced by
* bumpfee on transaction created by bumpfee
* "replaced_by_txid" - txid (as HexStr) of transaction created by
* bumpfee on transaction replaced by bumpfee
* "from", "message" - obsolete fields that could be set in UI prior to
* 2011 (removed in commit 4d9b223)
*
* The following keys are serialized in the wallet database, but shouldn't
* be read or written through the map (they will be temporarily added and
* removed from the map during serialization):
*
* "fromaccount" - serialized strFromAccount value
* "n" - serialized nOrderPos value
* "timesmart" - serialized nTimeSmart value
* "spent" - serialized vfSpent value that existed prior to
* 2014 (removed in commit 93a18a3)
*/
mapValue_t mapValue;
// [ Mysterious! Added in [#521](https://github.com/bitcoin/bitcoin/pull/521),
// has something to do with address URI's. ]
std::vector<std::pair<std::string, std::string> > vOrderForm;
unsigned int fTimeReceivedIsTxTime;
unsigned int nTimeReceived; //!< time received by this node
/**
* Stable timestamp that never changes, and reflects the order a transaction
* was added to the wallet. Timestamp is based on the block time for a
* transaction added as part of a block, or else the time when the
* transaction was received if it wasn't part of a block, with the timestamp
* adjusted in both cases so timestamp order matches the order transactions
* were added to the wallet. More details can be found in
* CWallet::ComputeTimeSmart().
*/
unsigned int nTimeSmart;
// [ Q: Why do we want to know this?
// A: Turns out, we don't! We used to leak privacy by aggressively
// broadcasting our own transactions, this was removed in #3115 (https://github.com/bitcoin/bitcoin/pull/3115/commits/fe5234645036178a540fdd4166b26493b0b40529)
// Attempted to be removed in #9351, but author closed, I've reopened: https://github.com/bitcoin/bitcoin/pull/32502
// ]
/**
* From me flag is set to 1 for transactions that were created by the wallet
* on this bitcoin node, and set to 0 for transactions that were created
* externally and came in through the network or sendrawtransaction RPC.
*/
bool fFromMe;
int64_t nOrderPos; //!< position in ordered transaction list
std::multimap<int64_t, CWalletTx*>::const_iterator m_it_wtxOrdered;
// memory only
enum AmountType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
// [ clever trick, now the enum members can be used as array indices. ]
mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
/**
* This flag is true if all m_amounts caches are empty. This is particularly
* useful in places where MarkDirty is conditionally called and the
* condition can be expensive and thus can be skipped if the flag is true.
* See MarkDestinationsDirty.
*/
mutable bool m_is_cache_empty{true};
mutable bool fChangeCached;
mutable CAmount nChangeCached;
CWalletTx(CTransactionRef tx, const TxState& state) : tx(std::move(tx)), m_state(state)
{
Init();
}
void Init()
{
mapValue.clear();
vOrderForm.clear();
fTimeReceivedIsTxTime = false;
nTimeReceived = 0;
nTimeSmart = 0;
fFromMe = false;
fChangeCached = false;
nChangeCached = 0;
nOrderPos = -1;
}
// [ The two most important members of the WalletTXO ]
CTransactionRef tx;
TxState m_state;
// [ Why do we care? https://github.com/bitcoin/bitcoin/pull/27307 ]
// Set of mempool transactions that conflict
// directly with the transaction, or that conflict
// with an ancestor transaction. This set will be
// empty if state is InMempool or Confirmed, but
// can be nonempty if state is Inactive or
// BlockConflicted.
std::set<Txid> mempool_conflicts;
//! make sure balances are recalculated
void MarkDirty()
{
m_amounts[DEBIT].Reset();
m_amounts[CREDIT].Reset();
m_amounts[IMMATURE_CREDIT].Reset();
m_amounts[AVAILABLE_CREDIT].Reset();
// [ for some mysterious reason, change is handled differently. ]
fChangeCached = false;
m_is_cache_empty = true;
}The only way we track block data that's interesting to CWallet's is at the
transaction level with mapWallet, which tracks the transactions that are of
interest to the wallet. (Those that spend from or send to a script or key that
the wallet cares about) But, when computing balance
(GetBalance)
or retrieving available outputs for coin selection
(AvailableCoins),
we don't care about the transactions this wallet is involved in, we care about
the transaction outputs this wallet is involved in.
Because of this, when we do either of these presently, we iterate through every output in the relevant transactions to find the output(s) of interest.
Keep track of the transaction outputs of interest to a wallet, this is done by
checking which outputs, IsMine() on wallet load.
Keep track of the transaction outputs of interest to a wallet in a new memory
structure, this is done by checking which outputs, IsMine() on wallet load.
After adding a wallet descriptor (typically by import), mark all balance
caches dirty. This allows transactions that the wallet already knows
about that have outputs that are now ISMINE_SPENDABLE after the import
to actually be shown in balance calculations. Legacy wallet imports
would do this, but importdescriptors did not.
Really this comes in a pair with the next commit which tests the behavior that previously was not handled. I kind of understand this commit, but I'm not 100% sure I have a map of the inbound routes where balance cache needs to be invalidated, I'm OK to accept this commit as-is though, since worst case, we will be invalidating our balance cache when we didn't need to, this commit cannot cause serious harm, and superficially, is correct.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 4a4aa837eaad7..cc604ed27986a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3780,6 +3780,9 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
// Save the descriptor to DB
spk_man->WriteDescriptor();
+ // Break balance caches so that outputs that are now IsMine in already known txs will be included in the balance
+ MarkDirty();
+
return std::reference_wrapper(*spk_man);
}The whole trick of this test is that marking the transaction balance caches as dirty will result in the balance being recomputed for every output. So, if we can import a descriptor for a not-yet-tracked output of a transaction that we care about one of the outputs of with triggering a rescan of the block, we should still see the available balance from this newly imported output.
IMO, is this really a functional test, as in, does it test behavior we/users
expect to work? Or is this an indirect way of testing an implementation detail
of CWallet? I'll wait till the end..
self.log.info("Test that the balance is updated by an import that makes an untracked output in an existing tx \"mine\"")
# [ The defot walit's gotta tonna ca🦨h ]
default = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
self.nodes[0].createwallet("importupdate")
wallet = self.nodes[0].get_wallet_rpc("importupdate")
# [ Make two keys, only import the first one. ]
import_key1 = get_generate_key()
import_key2 = get_generate_key()
wallet.importdescriptors([{"desc": descsum_create(f"wpkh({import_key1.privkey})"), "timestamp": "now"}])
# [ default wallet (rich uncle) sends 15 to key1 and 15 to key2... ]
amount = 15
default.send([{import_key1.p2wpkh_addr: amount},{import_key2.p2wpkh_addr: amount}])
# [ Mine a block with the tx, I assume send doesn't do that by default. ]
self.generate(self.nodes[0], 1)
# [ This business is a bit awkward, "now" aka the default rescan time
# is MTP of the most recent block, presumably to handle a lengthy
# import where the chaintip advances.(I should look at how mtp time works)
# but as long as we advance the mtp past that block it won't be part of
# the "now" that gets rescanned when we import the next descriptor ]
# Mock the time forward by 1 day so that "now" will exclude the block we just mined
self.nodes[0].setmocktime(int(time.time()) + 86400)
# Mine 11 blocks to move the MTP past the block we just mined
self.generate(self.nodes[0], 11, sync_fun=self.no_op)
balances = wallet.getbalances()
assert_equal(balances["mine"]["trusted"], amount)
# [ Now import the key, this doesn't trigger a rescan of the block, but
# the balance for the tx is still updated because the transaction level
# balance cache is invalidated, presumably this is of more interest for
# later. ]
# Don't rescan to make sure that the import updates the wallet txos
wallet.importdescriptors([{"desc": descsum_create(f"wpkh({import_key2.privkey})"), "timestamp": "now"}])
balances = wallet.getbalances()
assert_equal(balances["mine"]["trusted"], amount * 2)Need to load txs last so that IsMine works.
This gives a good premonition of the change to come, by the time we get to
LoadTxRecords(), we will want to know about everything the wallet cares about.
I do have a small complaint about this though, this PR introduces a constraint to the order in which records are loaded, but this constraint doesn't appear to be documented (meh) or enforced (what we really want) in any way. Is there a way to have this restriction be enforced either by the compiler or by tests?
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 3bb5db71f5e8e..7ec2208daa004 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -1186,14 +1186,14 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
// Load address book
result = std::max(LoadAddressBookRecords(pwallet, *m_batch), result);
- // Load tx records
- result = std::max(LoadTxRecords(pwallet, *m_batch, upgraded_txs, any_unordered), result);
-
// Load SPKMs
result = std::max(LoadActiveSPKMs(pwallet, *m_batch), result);
// Load decryption keys
result = std::max(LoadDecryptionKeys(pwallet, *m_batch), result);
+
+ // Load tx records
+ result = std::max(LoadTxRecords(pwallet, *m_batch, upgraded_txs, any_unordered), result);
} catch (...) {
// Exceptions that can be ignored or treated as non-critical are handled by the individual loading functions.
// Any uncaught exceptions will be caught here and treated as critical.When loading transactions to the wallet, check the outputs, and keep
track of the ones that are IsMine.
Introduces the key data/functionality of this PR.
This is a map from outpoint to "WalletTXO". We'll look at WalletTXO's in a second, but
class CWallet
{
//! Set of both spent and unspent transaction outputs owned by this wallet
std::unordered_map<COutPoint, WalletTXO, SaltedOutpointHasher> m_txos GUARDED_BY(cs_wallet);
}Introduces a class WalletTXO which in short represents a transaction output
the wallet cares about, and what the IsMine() of the output is. Three members,
a const reference to a CWalletTx, a const reference to a CTxOut, and a
mutable isminetype m_ismine (used to represent the IsMine() result on this
output.
The const members have getters, the mutable member has a getter and setter:
In src/wallet/transaction.h:
class WalletTXO
{
private:
const CWalletTx& m_wtx;
const CTxOut& m_output;
isminetype m_ismine;
public:
WalletTXO(const CWalletTx& wtx, const CTxOut& output, const isminetype ismine)
: m_wtx(wtx),
m_output(output),
m_ismine(ismine)
{
Assume(std::ranges::find(wtx.tx->vout, output) != wtx.tx->vout.end());
}
const CWalletTx& GetWalletTx() const { return m_wtx; }
const CTxOut& GetTxOut() const { return m_output; }
isminetype GetIsMine() const { return m_ismine; }
void SetIsMine(isminetype ismine) { m_ismine = ismine; }
};void CWallet::RefreshTXOsFromTx(const CWalletTx& wtx)
{
// [ We need the `cs_wallet` lock for `IsMine()` below, and we don't want
// `IsMine()` to change in the middle of a call to this function. ]
AssertLockHeld(cs_wallet);
for (uint32_t i = 0; i < wtx.tx->vout.size(); ++i) {
// [ a CTxOut is a ScriptPubKey, and nValue ]
const CTxOut& txout = wtx.tx->vout.at(i);
// [ Get the ismine result for the txout ]
isminetype ismine = IsMine(txout);
if (ismine == ISMINE_NO) {
continue;
}
// [ See https://github.com/bitcoin/bitcoin/pull/27286/commits/41c82afdfc94d7e0e1daa5ac4f4e5814e113c398#r2089882326 ]
COutPoint outpoint(wtx.GetHash(), i);
if (m_txos.contains(outpoint)) {
m_txos.at(outpoint).SetIsMine(ismine);
} else {
m_txos.emplace(outpoint, WalletTXO{wtx, txout, ismine});
}
}
}For now, just updating the m_txos map happens, it doesn't get read anywhere.
To make sure m_txos stays up-to-date, it needs to change any time:
- A transaction comes into or leaves the wallet.
mapWalletcan be used as a signal for this.
- The wallet's interpretation of what is
IsMine()changes.
In this commit, we just handle the first case, which happens to be anywhere mapWallet.emplace() is used. 1
We want to use RefreshTXOsFromTx anywhere that transactions can come into the wallet, and we'll manually remove them wherever transactions leave the wallet.
Basically, it's AddToWallet() and LoadToWallet(), these are the two places
mapWallet is emplaced into, LoadToWallet() is when loading tx'es from disk,
AddToWallet() is every other way tx'es come in to the wallet (rescans, new
blocks, etc. )
CWallet::AddToWallet() annotated
CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx, bool rescanning_old_block)
{
LOCK(cs_wallet);
WalletBatch batch(GetDatabase());
Txid hash = tx->GetHash();
// [ this flag is on-by-default. ]
if (IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) {
// Mark used destinations
std::set<CTxDestination> tx_destinations;
for (const CTxIn& txin : tx->vin) {
const COutPoint& op = txin.prevout;
SetSpentKeyState(batch, op.hash, op.n, true, tx_destinations);
}
MarkDestinationsDirty(tx_destinations);
}
// [ Why we care about addWallet, mapWallet gets emplaced into, this means
// our wallet's view of relevant tx'es has changed, which means our txo's
// we care about also has changed. ]
// Inserts only if not already there, returns tx inserted or tx found
auto ret = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(tx, state));
CWalletTx& wtx = (*ret.first).second;
bool fInsertedNew = ret.second;
// [ Booleans for control-flow, ay ay ay 🥙 !!!! ]
bool fUpdated = update_wtx && update_wtx(wtx, fInsertedNew);
if (fInsertedNew) {
wtx.nTimeReceived = GetTime();
wtx.nOrderPos = IncOrderPosNext(&batch);
wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
wtx.nTimeSmart = ComputeTimeSmart(wtx, rescanning_old_block);
AddToSpends(wtx, &batch);
// Update birth time when tx time is older than it.
MaybeUpdateBirthTime(wtx.GetTxTime());
}
// [ If it's already inserted.... ]
if (!fInsertedNew)
{
// [ If the state passed in don't match the state already on the tx... ]
if (state.index() != wtx.m_state.index()) {
// [ Update it...]
wtx.m_state = state;
fUpdated = true;
// [ Otherwise if they do match... ]
} else {
// [ Assert that their serializations match?? what? why?
// Investigate: https://github.com/bitcoin/bitcoin/pull/16624 ]
assert(TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state));
assert(TxStateSerializedBlockHash(wtx.m_state) == TxStateSerializedBlockHash(state));
}
// If we have a witness-stripped version of this transaction, and we
// see a new version with a witness, then we must be upgrading a pre-segwit
// wallet. Store the new version of the transaction with the witness,
// as the stripped-version must be invalid.
// TODO: Store all versions of the transaction, instead of just one.
if (tx->HasWitness() && !wtx.tx->HasWitness()) {
wtx.SetTx(tx);
fUpdated = true;
}
}
// Mark inactive coinbase transactions and their descendants as abandoned
if (wtx.IsCoinBase() && wtx.isInactive()) {
// [ This segment is *way* too long, IMO, worth looking at a refactor
// here. ]
// [...]
}
//// debug print
WalletLogPrintf("AddToWallet %s %s%s %s\n", hash.ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : ""), TxStateString(state));
// Write to disk
if (fInsertedNew || fUpdated)
if (!batch.WriteTx(wtx))
return nullptr;
// Break debit/credit balance caches:
wtx.MarkDirty();
// Cache the outputs that belong to the wallet
RefreshTXOsFromTx(wtx);
// Notify UI of new or updated transaction
NotifyTransactionChanged(hash, fInsertedNew ? CT_NEW : CT_UPDATED);
return &wtx;
}This is a strange commit:
Elsewhere (in (GetBalance())[https://github.com/bitcoin/bitcoin/blob/72378b2bf42714b67fb3f0272b5fbb7c37a08898/src/wallet/receive.cpp#L307] and AvailableCoins() this gets called on every TX and after this PR would end
up getting called on every TXO and hurting performance, see:
https://github.com/bitcoin/bitcoin/pull/27286/commits/72378b2bf42714b67fb3f0272b5fbb7c37a08898#r2078987925
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
index f0d7d00763ffc..620d2bb12c65d 100644
--- a/src/wallet/receive.cpp
+++ b/src/wallet/receive.cpp
@@ -257,6 +257,10 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef
bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<Txid>& trusted_parents)
{
AssertLockHeld(wallet.cs_wallet);
+
+ // This wtx is already trusted
+ if (trusted_parents.contains(wtx.GetHash())) return true;
+
if (wtx.isConfirmed()) return true;
if (wtx.isBlockConflicted()) return false;
// using wtx's cached debitLet's look at CachedTxIsTrusted():
// NOLINTNEXTLINE(misc-no-recursion)
bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<Txid>& trusted_parents)
{
AssertLockHeld(wallet.cs_wallet);
// This wtx is already trusted
if (trusted_parents.contains(wtx.GetHash())) return true;
if (wtx.isConfirmed()) return true;
if (wtx.isBlockConflicted()) return false;
// using wtx's cached debit
if (!wallet.m_spend_zero_conf_change || !CachedTxIsFromMe(wallet, wtx, ISMINE_ALL)) return false;
// Don't trust unconfirmed transactions from us unless they are in the mempool.
if (!wtx.InMempool()) return false;
// Trusted if all inputs are from us and are in the mempool:
for (const CTxIn& txin : wtx.tx->vin)
{
// Transactions not sent by us: not trusted
const CWalletTx* parent = wallet.GetWalletTx(txin.prevout.hash);
if (parent == nullptr) return false;
const CTxOut& parentOut = parent->tx->vout[txin.prevout.n];
// Check that this specific input being spent is trusted
if (wallet.IsMine(parentOut) != ISMINE_SPENDABLE) return false;
// If we've already trusted this parent, continue
if (trusted_parents.count(parent->GetHash())) continue;
// Recurse to check that the parent is also trusted
if (!CachedTxIsTrusted(wallet, *parent, trusted_parents)) return false;
trusted_parents.insert(parent->GetHash());
}
return true;
}This PR fixes part 2 of what I mentioned 2 commits above, which is that we need
to refresh m_txos anywhere a wallet's view of what is IsMine() changes.
Commit diff
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 39df2166bef10..eece0815111cb 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -250,6 +250,7 @@ RPCHelpMan keypoolrefill()
if (pwallet->GetKeyPoolSize() < kpSize) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error refreshing keypool.");
}
+ pwallet->RefreshAllTXOs();
return UniValue::VNULL;
},
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 3ec4285a3f9cf..c7e477d727023 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -399,6 +399,7 @@ RPCHelpMan importdescriptors()
}
}
pwallet->ConnectScriptPubKeyManNotifiers();
+ pwallet->RefreshAllTXOs();
}
// Rescan the blockchain using the lowest timestamp
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index b133753de944e..dbb2a5b1e0761 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4496,4 +4496,12 @@ void CWallet::RefreshTXOsFromTx(const CWalletTx& wtx)
}
}
}
+
+void CWallet::RefreshAllTXOs()
+{
+ AssertLockHeld(cs_wallet);
+ for (const auto& [_, wtx] : mapWallet) {
+ RefreshTXOsFromTx(wtx);
+ }
+}
} // namespace wallet
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 2845b5450db0f..36083d99d1559 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -515,6 +515,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
/** Cache outputs that belong to the wallet from a single transaction */
void RefreshTXOsFromTx(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+ /** Cache outputs that belong to the wallet for all transactions in the wallet */
+ void RefreshAllTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/**
* Return depth of transaction in blockchain:Since we track the outputs owned by the wallet with m_txos, we can now
calculate the balance of the wallet by iterating m_txos and summing up
the amounts of the unspent txos.
Commit diff
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
index 620d2bb12c65d..5639586ea9011 100644
--- a/src/wallet/receive.cpp
+++ b/src/wallet/receive.cpp
@@ -4,6 +4,7 @@
#include <consensus/amount.h>
#include <consensus/consensus.h>
+#include <util/check.h>
#include <wallet/receive.h>
#include <wallet/transaction.h>
#include <wallet/wallet.h>
@@ -297,27 +298,41 @@ bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx)
Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse)
{
Balance ret;
- isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
+ bool allow_used_addresses = !avoid_reuse || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
{
LOCK(wallet.cs_wallet);
std::set<Txid> trusted_parents;
- for (const auto& entry : wallet.mapWallet)
- {
- const CWalletTx& wtx = entry.second;
- const bool is_trusted{CachedTxIsTrusted(wallet, wtx, trusted_parents)};
- const int tx_depth{wallet.GetTxDepthInMainChain(wtx)};
- const CAmount tx_credit_mine{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | reuse_filter)};
- const CAmount tx_credit_watchonly{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_WATCH_ONLY | reuse_filter)};
- if (is_trusted && tx_depth >= min_depth) {
- ret.m_mine_trusted += tx_credit_mine;
- ret.m_watchonly_trusted += tx_credit_watchonly;
- }
- if (!is_trusted && tx_depth == 0 && wtx.InMempool()) {
- ret.m_mine_untrusted_pending += tx_credit_mine;
- ret.m_watchonly_untrusted_pending += tx_credit_watchonly;
+ for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
+ Assert(MoneyRange(txo.GetTxOut().nValue));
+
+ const bool is_trusted{CachedTxIsTrusted(wallet, txo.GetWalletTx(), trusted_parents)};
+ const int tx_depth{wallet.GetTxDepthInMainChain(txo.GetWalletTx())};
+
+ if (!wallet.IsSpent(outpoint) && (allow_used_addresses || !wallet.IsSpentKey(txo.GetTxOut().scriptPubKey))) {
+ // Get the amounts for mine and watchonly
+ CAmount credit_mine = 0;
+ CAmount credit_watchonly = 0;
+ if (txo.GetIsMine() == ISMINE_SPENDABLE) {
+ credit_mine = txo.GetTxOut().nValue;
+ } else if (txo.GetIsMine() == ISMINE_WATCH_ONLY) {
+ credit_watchonly = txo.GetTxOut().nValue;
+ } else {
+ // We shouldn't see any other isminetypes
+ Assume(false);
+ }
+
+ // Set the amounts in the return object
+ if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed()) {
+ ret.m_mine_immature += credit_mine;
+ ret.m_watchonly_immature += credit_watchonly;
+ } else if (is_trusted && tx_depth >= min_depth) {
+ ret.m_mine_trusted += credit_mine;
+ ret.m_watchonly_trusted += credit_watchonly;
+ } else if (!is_trusted && txo.GetWalletTx().InMempool()) {
+ ret.m_mine_untrusted_pending += credit_mine;
+ ret.m_watchonly_untrusted_pending += credit_watchonly;
+ }
}
- ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE);
- ret.m_watchonly_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_WATCH_ONLY);
}
}
return ret;
@@ -330,31 +345,19 @@ std::map<CTxDestination, CAmount> GetAddressBalances(const CWallet& wallet)
{
LOCK(wallet.cs_wallet);
std::set<Txid> trusted_parents;
- for (const auto& walletEntry : wallet.mapWallet)
- {
- const CWalletTx& wtx = walletEntry.second;
+ for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
+ if (!CachedTxIsTrusted(wallet, txo.GetWalletTx(), trusted_parents)) continue;
+ if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx())) continue;
- if (!CachedTxIsTrusted(wallet, wtx, trusted_parents))
- continue;
+ int nDepth = wallet.GetTxDepthInMainChain(txo.GetWalletTx());
+ if (nDepth < (CachedTxIsFromMe(wallet, txo.GetWalletTx(), ISMINE_ALL) ? 0 : 1)) continue;
- if (wallet.IsTxImmatureCoinBase(wtx))
- continue;
+ CTxDestination addr;
+ Assume(wallet.IsMine(txo.GetTxOut()));
+ if(!ExtractDestination(txo.GetTxOut().scriptPubKey, addr)) continue;
- int nDepth = wallet.GetTxDepthInMainChain(wtx);
- if (nDepth < (CachedTxIsFromMe(wallet, wtx, ISMINE_ALL) ? 0 : 1))
- continue;
-
- for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
- const auto& output = wtx.tx->vout[i];
- CTxDestination addr;
- if (!wallet.IsMine(output))
- continue;
- if(!ExtractDestination(output.scriptPubKey, addr))
- continue;
-
- CAmount n = wallet.IsSpent(COutPoint(walletEntry.first, i)) ? 0 : output.nValue;
- balances[addr] += n;
- }
+ CAmount n = wallet.IsSpent(outpoint) ? 0 : txo.GetTxOut().nValue;
+ balances[addr] += n;
}
}
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 36083d99d1559..4cac92aff4e72 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -513,6 +513,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
std::set<Txid> GetTxConflicts(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+ const std::unordered_map<COutPoint, WalletTXO, SaltedOutpointHasher>& GetTXOs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return m_txos; };
+
/** Cache outputs that belong to the wallet from a single transaction */
void RefreshTXOsFromTx(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/** Cache outputs that belong to the wallet for all transactions in the wallet */Instead of iterating every transaction and every output stored in wallet
when trying to figure out what outputs can be spent, iterate the TXO set
which should be a lot smaller.
Commit diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index a330c31a87a79..63ff6d6ee280d 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -330,137 +330,146 @@ CoinsResult AvailableCoins(const CWallet& wallet,
std::vector<COutPoint> outpoints;
std::set<Txid> trusted_parents;
- for (const auto& entry : wallet.mapWallet)
- {
- const Txid& txid = entry.first;
- const CWalletTx& wtx = entry.second;
+ // Cache for whether each tx passes the tx level checks (first bool), and whether the transaction is "safe" (second bool)
+ std::unordered_map<uint256, std::pair<bool, bool>, SaltedTxidHasher> tx_safe_cache;
+ for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
+ const CWalletTx& wtx = txo.GetWalletTx();
+ const CTxOut& output = txo.GetTxOut();
- if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase)
+ if (tx_safe_cache.contains(outpoint.hash) && !tx_safe_cache.at(outpoint.hash).first) {
continue;
+ }
- int nDepth = wallet.GetTxDepthInMainChain(wtx);
- if (nDepth < 0)
+ // Skip manually selected coins (the caller can fetch them directly)
+ if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint))
continue;
- // We should not consider coins which aren't at least in our mempool
- // It's possible for these to be conflicted via ancestors which we may never be able to detect
- if (nDepth == 0 && !wtx.InMempool())
+ if (wallet.IsLockedCoin(outpoint) && params.skip_locked)
continue;
- bool safeTx = CachedTxIsTrusted(wallet, wtx, trusted_parents);
-
- // We should not consider coins from transactions that are replacing
- // other transactions.
- //
- // Example: There is a transaction A which is replaced by bumpfee
- // transaction B. In this case, we want to prevent creation of
- // a transaction B' which spends an output of B.
- //
- // Reason: If transaction A were initially confirmed, transactions B
- // and B' would no longer be valid, so the user would have to create
- // a new transaction C to replace B'. However, in the case of a
- // one-block reorg, transactions B' and C might BOTH be accepted,
- // when the user only wanted one of them. Specifically, there could
- // be a 1-block reorg away from the chain where transactions A and C
- // were accepted to another chain where B, B', and C were all
- // accepted.
- if (nDepth == 0 && wtx.mapValue.count("replaces_txid")) {
- safeTx = false;
- }
+ if (wallet.IsSpent(outpoint))
+ continue;
- // Similarly, we should not consider coins from transactions that
- // have been replaced. In the example above, we would want to prevent
- // creation of a transaction A' spending an output of A, because if
- // transaction B were initially confirmed, conflicting with A and
- // A', we wouldn't want to the user to create a transaction D
- // intending to replace A', but potentially resulting in a scenario
- // where A, A', and D could all be accepted (instead of just B and
- // D, or just A and A' like the user would want).
- if (nDepth == 0 && wtx.mapValue.count("replaced_by_txid")) {
- safeTx = false;
- }
+ if (output.nValue < params.min_amount || output.nValue > params.max_amount)
+ continue;
- if (only_safe && !safeTx) {
+ if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) {
continue;
}
- if (nDepth < min_depth || nDepth > max_depth) {
+ if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase)
continue;
- }
- bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL);
+ isminetype mine = wallet.IsMine(output);
- for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
- const CTxOut& output = wtx.tx->vout[i];
- const COutPoint outpoint(txid, i);
+ assert(mine != ISMINE_NO);
- if (output.nValue < params.min_amount || output.nValue > params.max_amount)
- continue;
+ int nDepth = wallet.GetTxDepthInMainChain(wtx);
+ if (nDepth < 0)
+ continue;
- // Skip manually selected coins (the caller can fetch them directly)
- if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint))
- continue;
+ // Perform tx level checks if we haven't already come across outputs from this tx before.
+ if (!tx_safe_cache.contains(outpoint.hash)) {
+ tx_safe_cache[outpoint.hash] = {false, false};
- if (wallet.IsLockedCoin(outpoint) && params.skip_locked)
+ // We should not consider coins which aren't at least in our mempool
+ // It's possible for these to be conflicted via ancestors which we may never be able to detect
+ if (nDepth == 0 && !wtx.InMempool())
continue;
- if (wallet.IsSpent(outpoint))
- continue;
+ bool safeTx = CachedTxIsTrusted(wallet, wtx, trusted_parents);
+
+ // We should not consider coins from transactions that are replacing
+ // other transactions.
+ //
+ // Example: There is a transaction A which is replaced by bumpfee
+ // transaction B. In this case, we want to prevent creation of
+ // a transaction B' which spends an output of B.
+ //
+ // Reason: If transaction A were initially confirmed, transactions B
+ // and B' would no longer be valid, so the user would have to create
+ // a new transaction C to replace B'. However, in the case of a
+ // one-block reorg, transactions B' and C might BOTH be accepted,
+ // when the user only wanted one of them. Specifically, there could
+ // be a 1-block reorg away from the chain where transactions A and C
+ // were accepted to another chain where B, B', and C were all
+ // accepted.
+ if (nDepth == 0 && wtx.mapValue.count("replaces_txid")) {
+ safeTx = false;
+ }
- isminetype mine = wallet.IsMine(output);
+ // Similarly, we should not consider coins from transactions that
+ // have been replaced. In the example above, we would want to prevent
+ // creation of a transaction A' spending an output of A, because if
+ // transaction B were initially confirmed, conflicting with A and
+ // A', we wouldn't want to the user to create a transaction D
+ // intending to replace A', but potentially resulting in a scenario
+ // where A, A', and D could all be accepted (instead of just B and
+ // D, or just A and A' like the user would want).
+ if (nDepth == 0 && wtx.mapValue.count("replaced_by_txid")) {
+ safeTx = false;
+ }
- if (mine == ISMINE_NO) {
+ if (only_safe && !safeTx) {
continue;
}
- if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) {
+ if (nDepth < min_depth || nDepth > max_depth) {
continue;
}
- std::unique_ptr<SigningProvider> provider = wallet.GetSolvingProvider(output.scriptPubKey);
-
- int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), can_grind_r, coinControl);
- // Because CalculateMaximumSignedInputSize infers a solvable descriptor to get the satisfaction size,
- // it is safe to assume that this input is solvable if input_bytes is greater than -1.
- bool solvable = input_bytes > -1;
- bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
-
- // Filter by spendable outputs only
- if (!spendable && params.only_spendable) continue;
-
- // Obtain script type
- std::vector<std::vector<uint8_t>> script_solutions;
- TxoutType type = Solver(output.scriptPubKey, script_solutions);
-
- // If the output is P2SH and solvable, we want to know if it is
- // a P2SH (legacy) or one of P2SH-P2WPKH, P2SH-P2WSH (P2SH-Segwit). We can determine
- // this from the redeemScript. If the output is not solvable, it will be classified
- // as a P2SH (legacy), since we have no way of knowing otherwise without the redeemScript
- bool is_from_p2sh{false};
- if (type == TxoutType::SCRIPTHASH && solvable) {
- CScript script;
- if (!provider->GetCScript(CScriptID(uint160(script_solutions[0])), script)) continue;
- type = Solver(script, script_solutions);
- is_from_p2sh = true;
- }
+ tx_safe_cache[outpoint.hash] = {true, safeTx};
+ }
+ const auto& [tx_ok, tx_safe] = tx_safe_cache.at(outpoint.hash);
+ if (!Assume(tx_ok)) {
+ continue;
+ }
- result.Add(GetOutputType(type, is_from_p2sh),
- COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate));
+ bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL);
- outpoints.push_back(outpoint);
+ std::unique_ptr<SigningProvider> provider = wallet.GetSolvingProvider(output.scriptPubKey);
+
+ int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), can_grind_r, coinControl);
+ // Because CalculateMaximumSignedInputSize infers a solvable descriptor to get the satisfaction size,
+ // it is safe to assume that this input is solvable if input_bytes is greater than -1.
+ bool solvable = input_bytes > -1;
+ bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
+
+ // Filter by spendable outputs only
+ if (!spendable && params.only_spendable) continue;
+
+ // Obtain script type
+ std::vector<std::vector<uint8_t>> script_solutions;
+ TxoutType type = Solver(output.scriptPubKey, script_solutions);
+
+ // If the output is P2SH and solvable, we want to know if it is
+ // a P2SH (legacy) or one of P2SH-P2WPKH, P2SH-P2WSH (P2SH-Segwit). We can determine
+ // this from the redeemScript. If the output is not solvable, it will be classified
+ // as a P2SH (legacy), since we have no way of knowing otherwise without the redeemScript
+ bool is_from_p2sh{false};
+ if (type == TxoutType::SCRIPTHASH && solvable) {
+ CScript script;
+ if (!provider->GetCScript(CScriptID(uint160(script_solutions[0])), script)) continue;
+ type = Solver(script, script_solutions);
+ is_from_p2sh = true;
+ }
- // Checks the sum amount of all UTXO's.
- if (params.min_sum_amount != MAX_MONEY) {
- if (result.GetTotalAmount() >= params.min_sum_amount) {
- return result;
- }
- }
+ result.Add(GetOutputType(type, is_from_p2sh),
+ COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));
- // Checks the maximum number of UTXO's.
- if (params.max_count > 0 && result.Size() >= params.max_count) {
+ outpoints.push_back(outpoint);
+
+ // Checks the sum amount of all UTXO's.
+ if (params.min_sum_amount != MAX_MONEY) {
+ if (result.GetTotalAmount() >= params.min_sum_amount) {
return result;
}
}
+
+ // Checks the maximum number of UTXO's.
+ if (params.max_count > 0 && result.Size() >= params.max_count) {
+ return result;
+ }
}
if (feerate.has_value()) {Instead of searching mapWallet for the preselected inputs, search
m_txos.
Commit diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 63ff6d6ee280d..e8e90cb3a7135 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -277,12 +277,8 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
input_bytes = GetVirtualTransactionSize(input_bytes, 0, 0);
}
CTxOut txout;
- if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) {
- // Clearly invalid input, fail
- if (ptr_wtx->tx->vout.size() <= outpoint.n) {
- return util::Error{strprintf(_("Invalid pre-selected input %s"), outpoint.ToString())};
- }
- txout = ptr_wtx->tx->vout.at(outpoint.n);
+ if (auto txo = wallet.GetTXO(outpoint)) {
+ txout = txo->GetTxOut();
if (input_bytes == -1) {
input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control);
}
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index dbb2a5b1e0761..ab9026ab7289a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4504,4 +4504,14 @@ void CWallet::RefreshAllTXOs()
RefreshTXOsFromTx(wtx);
}
}
+
+std::optional<WalletTXO> CWallet::GetTXO(const COutPoint& outpoint) const
+{
+ AssertLockHeld(cs_wallet);
+ const auto& it = m_txos.find(outpoint);
+ if (it == m_txos.end()) {
+ return std::nullopt;
+ }
+ return it->second;
+}
} // namespace wallet
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 4cac92aff4e72..0f07388ef7621 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -514,6 +514,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
std::set<Txid> GetTxConflicts(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
const std::unordered_map<COutPoint, WalletTXO, SaltedOutpointHasher>& GetTXOs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return m_txos; };
+ std::optional<WalletTXO> GetTXO(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/** Cache outputs that belong to the wallet from a single transaction */
void RefreshTXOsFromTx(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);When a legacy wallet has been migrated to contain descriptors, but
before the transactions have been updated to match, we need to recompute
the wallet TXOs so that the transaction update will work correctly.
Commit diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index ab9026ab7289a..7b760435c1d1b 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3946,6 +3946,10 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
return util::Error{_("Error: Unable to read wallet's best block locator record")};
}
+ // Update m_txos to match the descriptors remaining in this wallet
+ m_txos.clear();
+ RefreshAllTXOs();
+
// Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet.
// We need to go through these in the tx insertion order so that lookups to spends works.
std::vector<Txid> txids_to_delete;Instead of looking up the previous tx in mapWallet, and then calling
IsMine on the specified output, use the TXO set and its cached IsMine
value.
Commit diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7b760435c1d1b..5d385065ceb3b 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1571,16 +1571,10 @@ void CWallet::BlockUntilSyncedToCurrentChain() const {
// and a not-"is mine" (according to the filter) input.
CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const
{
- {
- LOCK(cs_wallet);
- const auto mi = mapWallet.find(txin.prevout.hash);
- if (mi != mapWallet.end())
- {
- const CWalletTx& prev = (*mi).second;
- if (txin.prevout.n < prev.tx->vout.size())
- if (IsMine(prev.tx->vout[txin.prevout.n]) & filter)
- return prev.tx->vout[txin.prevout.n].nValue;
- }
+ LOCK(cs_wallet);
+ auto txo = GetTXO(txin.prevout);
+ if (txo && (txo->GetIsMine() & filter)) {
+ return txo->GetTxOut().nValue;
}
return 0;
}These two functions are no longer used as GetBalances now uses the TXO
set rather than per-tx cached balances
Commit diff
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
index 5639586ea9011..f94b80fce63d9 100644
--- a/src/wallet/receive.cpp
+++ b/src/wallet/receive.cpp
@@ -146,52 +146,6 @@ CAmount CachedTxGetChange(const CWallet& wallet, const CWalletTx& wtx)
return wtx.nChangeCached;
}
-CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)
-{
- AssertLockHeld(wallet.cs_wallet);
-
- if (wallet.IsTxImmatureCoinBase(wtx) && wtx.isConfirmed()) {
- return GetCachableAmount(wallet, wtx, CWalletTx::IMMATURE_CREDIT, filter);
- }
-
- return 0;
-}
-
-CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)
-{
- AssertLockHeld(wallet.cs_wallet);
-
- // Avoid caching ismine for NO or ALL cases (could remove this check and simplify in the future).
- bool allow_cache = (filter & ISMINE_ALL) && (filter & ISMINE_ALL) != ISMINE_ALL;
-
- // Must wait until coinbase is safely deep enough in the chain before valuing it
- if (wallet.IsTxImmatureCoinBase(wtx))
- return 0;
-
- if (allow_cache && wtx.m_amounts[CWalletTx::AVAILABLE_CREDIT].m_cached[filter]) {
- return wtx.m_amounts[CWalletTx::AVAILABLE_CREDIT].m_value[filter];
- }
-
- bool allow_used_addresses = (filter & ISMINE_USED) || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
- CAmount nCredit = 0;
- Txid hashTx = wtx.GetHash();
- for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
- const CTxOut& txout = wtx.tx->vout[i];
- if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(txout.scriptPubKey))) {
- nCredit += OutputGetCredit(wallet, txout, filter);
- if (!MoneyRange(nCredit))
- throw std::runtime_error(std::string(__func__) + " : value out of range");
- }
- }
-
- if (allow_cache) {
- wtx.m_amounts[CWalletTx::AVAILABLE_CREDIT].Set(filter, nCredit);
- wtx.m_is_cache_empty = false;
- }
-
- return nCredit;
-}
-
void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
std::list<COutputEntry>& listReceived,
std::list<COutputEntry>& listSent, CAmount& nFee, const isminefilter& filter,
diff --git a/src/wallet/receive.h b/src/wallet/receive.h
index b795689de51e4..330fbf21a65ca 100644
--- a/src/wallet/receive.h
+++ b/src/wallet/receive.h
@@ -30,10 +30,6 @@ CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const ism
//! filter decides which addresses will count towards the debit
CAmount CachedTxGetDebit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter);
CAmount CachedTxGetChange(const CWallet& wallet, const CWalletTx& wtx);
-CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)
- EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
-CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter = ISMINE_SPENDABLE)
- EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
struct COutputEntry
{
CTxDestination destination;
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 966c6d2c4ba12..640fd063fef2e 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -244,35 +244,6 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
/*num_expected_wallets=*/0);
}
-// Check that GetImmatureCredit() returns a newly calculated value instead of
-// the cached value after a MarkDirty() call.
-//
-// This is a regression test written to verify a bugfix for the immature credit
-// function. Similar tests probably should be written for the other credit and
-// debit functions.
-BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
-{
- CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
-
- LOCK(wallet.cs_wallet);
- LOCK(Assert(m_node.chainman)->GetMutex());
- CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/0}};
- wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
- wallet.SetupDescriptorScriptPubKeyMans();
-
- wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
-
- // Call GetImmatureCredit() once before adding the key to the wallet to
- // cache the current immature credit amount, which is 0.
- BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE), 0);
-
- // Invalidate the cached value, add the key, and make sure a new immature
- // credit amount is calculated.
- wtx.MarkDirty();
- AddKey(wallet, coinbaseKey);
- BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE), 50*COIN);
-}
-
static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime)
{
CMutableTransaction tx;
@@ -751,65 +722,5 @@ BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup)
TestUnloadWallet(std::move(wallet));
}
-/**
- * Checks a wallet invalid state where the inputs (prev-txs) of a new arriving transaction are not marked dirty,
- * while the transaction that spends them exist inside the in-memory wallet tx map (not stored on db due a db write failure).
- */
-BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
-{
- CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
- {
- LOCK(wallet.cs_wallet);
- wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
- wallet.SetupDescriptorScriptPubKeyMans();
- }
-
- // Add tx to wallet
- const auto op_dest{*Assert(wallet.GetNewDestination(OutputType::BECH32M, ""))};
-
- CMutableTransaction mtx;
- mtx.vout.emplace_back(COIN, GetScriptForDestination(op_dest));
- mtx.vin.emplace_back(Txid::FromUint256(m_rng.rand256()), 0);
- const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();
-
- {
- // Cache and verify available balance for the wtx
- LOCK(wallet.cs_wallet);
- const CWalletTx* wtx_to_spend = wallet.GetWalletTx(tx_id_to_spend);
- BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *wtx_to_spend), 1 * COIN);
- }
-
- // Now the good case:
- // 1) Add a transaction that spends the previously created transaction
- // 2) Verify that the available balance of this new tx and the old one is updated (prev tx is marked dirty)
-
- mtx.vin.clear();
- mtx.vin.emplace_back(tx_id_to_spend, 0);
- wallet.transactionAddedToMempool(MakeTransactionRef(mtx));
- const auto good_tx_id{mtx.GetHash()};
-
- {
- // Verify balance update for the new tx and the old one
- LOCK(wallet.cs_wallet);
- const CWalletTx* new_wtx = wallet.GetWalletTx(good_tx_id);
- BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *new_wtx), 1 * COIN);
-
- // Now the old wtx
- const CWalletTx* wtx_to_spend = wallet.GetWalletTx(tx_id_to_spend);
- BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *wtx_to_spend), 0 * COIN);
- }
-
- // Now the bad case:
- // 1) Make db always fail
- // 2) Try to add a transaction that spends the previously created transaction and
- // verify that we are not moving forward if the wallet cannot store it
- GetMockableDatabase(wallet).m_pass = false;
- mtx.vin.clear();
- mtx.vin.emplace_back(good_tx_id, 0);
- BOOST_CHECK_EXCEPTION(wallet.transactionAddedToMempool(MakeTransactionRef(mtx)),
- std::runtime_error,
- HasReason("DB error adding transaction to wallet, write failed"));
-}
-
BOOST_AUTO_TEST_SUITE_END()
} // namespace wallet
diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
index 3e40714ae145c..a7cb68bac9ae2 100644
--- a/src/wallet/transaction.h
+++ b/src/wallet/transaction.h
@@ -220,7 +220,7 @@ class CWalletTx
std::multimap<int64_t, CWalletTx*>::const_iterator m_it_wtxOrdered;
// memory only
- enum AmountType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
+ enum AmountType { DEBIT, CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
/**
* This flag is true if all m_amounts caches are empty. This is particularly
@@ -316,8 +316,6 @@ class CWalletTx
{
m_amounts[DEBIT].Reset();
m_amounts[CREDIT].Reset();
- m_amounts[IMMATURE_CREDIT].Reset();
- m_amounts[AVAILABLE_CREDIT].Reset();
fChangeCached = false;
m_is_cache_empty = true;
}One of the main issues with AvailableCoins is its performance when txs
have unrelated outputs, so update the benchmark to check the performance
of that.
Commit diff
diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp
index 3c4b2f4f83a51..d6173191a1652 100644
--- a/src/bench/wallet_create_tx.cpp
+++ b/src/bench/wallet_create_tx.cpp
@@ -71,10 +71,17 @@ void generateFakeBlock(const CChainParams& params,
coinbase_tx.vin[0].prevout.SetNull();
coinbase_tx.vout.resize(2);
coinbase_tx.vout[0].scriptPubKey = coinbase_out_script;
- coinbase_tx.vout[0].nValue = 49 * COIN;
+ coinbase_tx.vout[0].nValue = 48 * COIN;
coinbase_tx.vin[0].scriptSig = CScript() << ++tip.tip_height << OP_0;
coinbase_tx.vout[1].scriptPubKey = coinbase_out_script; // extra output
coinbase_tx.vout[1].nValue = 1 * COIN;
+
+ // Fill the coinbase with outputs that don't belong to the wallet in order to benchmark
+ // AvailableCoins' behavior with unnecessary TXOs
+ for (int i = 0; i < 50; ++i) {
+ coinbase_tx.vout.emplace_back(1 * COIN / 50, CScript(OP_TRUE));
+ }
+
block.vtx = {MakeTransactionRef(std::move(coinbase_tx))};
block.nVersion = VERSIONBITS_LAST_OLD_BLOCK_VERSION;
@@ -129,14 +136,14 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type
// Check available balance
auto bal = WITH_LOCK(wallet.cs_wallet, return wallet::AvailableCoins(wallet).GetTotalAmount()); // Cache
- assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY));
+ assert(bal == 49 * COIN * (chain_size - COINBASE_MATURITY));
wallet::CCoinControl coin_control;
coin_control.m_allow_other_inputs = allow_other_inputs;
CAmount target = 0;
if (preset_inputs) {
- // Select inputs, each has 49 BTC
+ // Select inputs, each has 48 BTC
wallet::CoinFilterParams filter_coins;
filter_coins.max_count = preset_inputs->num_of_internal_inputs;
const auto& res = WITH_LOCK(wallet.cs_wallet,
@@ -189,7 +196,7 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector<OutputType
// Check available balance
auto bal = WITH_LOCK(wallet.cs_wallet, return wallet::AvailableCoins(wallet).GetTotalAmount()); // Cache
- assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY));
+ assert(bal == 49 * COIN * (chain_size - COINBASE_MATURITY));
bench.epochIterations(2).run([&] {
LOCK(wallet.cs_wallet);Footnotes
-
Access around
mapWalletshould be changed in my opinion, so that it's really obvious where it might get modified, right now, there's a whole bunch of non-const iterators reaching into it, that AFAICT non modify any tx'es in mapWallet. ↩