wallet: Track no-longer-spendable TXOs separately - #27865
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/27865. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
6925928 to
7a50755
Compare
8c4b04a to
9730ec0
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
The send RPC can be used to create a double spend of a confirmed transaction which is added unconditionally to the wallet as an inactive transaction. The test ensures that the transaction is in the wallet and that the balance is still being counted correctly.
m_from_me is used to track whether a transaction is "from me", i.e. has any inputs which belong to the wallet.
Instead of looking at the cached amounts or searching every input of a transaction each time we want to determine whether it is "from me", use m_from_me which stores this value for us.
The states will be updated whenever CWaleltTx::SetState is called too. This is achieved by having CWalletTx::SetState take a function that applies the new state to the specified TXO. This ensures that CWalletTx states and WalletTXO states are kept in sync.
Perform the transaction reorder upgrade immediately after loading txs instead of waiting for the end of loading.
Since we need to know whether the transaction that creates a WalletTXO is "from me", we should store this state in the WalletTXO too, copied from its parent CWalletTx.
WalletTXOs need to know their parent tx's timestamp for AvailableCoins to work.
A min_conf parameter is added to IsSpent so that it can set a confirmation threshold for whether something is considered spent.
When a block is disconnected, we need to process the transactions in reverse order so that the wallet's TXO set is updated in the correct order.
CWallet::Create will properly connect the wallet to the chain, so we should be doing that rather than ad-hoc chain connection.
Definitely unusable TXOs are those that are spent by a confirmed transaction or were produced by a now conflicted transaction. However, we still need them for GetDebit, so we store them in a separate m_unusable_txos container. MarkConflicted, AbandonTransaction, and loading (via PruneSpentTXOs) will ensure that these unusable TXOs are properly moved.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Could turn into draft while CI is red? |
jeanpablojp
left a comment
jeanpablojp
left a comment
There was a problem hiding this comment.
Concept ACK
After a reorg and abandontransaction, an output that should be spendable again is missing from the balance and from listunspent until the wallet is reloaded. Trusted 150 BTC against 200 on 512dc9af1b. And after removeprunedfunds, a block-conflicted output can take the node down on the next getbalances, tripping Assert(tx_depth >= 0). Both are inline, and neither happens on the base.
test
The reorg has to be deep enough for the coinbase to be immature at the new tip, otherwise the spend goes back to the mempool. Restarting the node rebuilds the containers and hides it.
self.generatetoaddress(node, 1, w.getnewaddress())
c_txid = node.getblock(node.getblockhash(1))["tx"][0]
self.generatetoaddress(node, COINBASE_MATURITY, w.getnewaddress())
o = next(u for u in w.listunspent() if u["txid"] == c_txid)
t1 = w.send(outputs=[{w.getnewaddress(): o["amount"] - Decimal("0.001")}],
inputs=[{"txid": o["txid"], "vout": o["vout"]}], add_inputs=False)["txid"]
self.generatetoaddress(node, 1, w.getnewaddress())
node.invalidateblock(node.getblockhash(COINBASE_MATURITY))
assert_equal(node.getrawmempool(), [])
assert_equal(w.gettransaction(t1)["confirmations"], 0)
self.generatetoaddress(node, 5, w.getnewaddress())
w.abandontransaction(t1)
assert any(u["txid"] == c_txid for u in w.listunspent(minconf=0))Built this and the base and ran the wallet tests.
| std::pair<CWallet::TXOMap::iterator, CWallet::TXOMap::iterator> CWallet::MaybeMarkTXOUsable(const COutPoint& outpoint) | ||
| { | ||
| AssertLockHeld(cs_wallet); | ||
| if (IsSpent(outpoint)) { |
There was a problem hiding this comment.
min_depth left at the default instead of the 1 that matches m_unusable_txos's definition. An output spent by a transaction that confirms and is then unconfirmed by a reorg doesn't come back, and abandontransaction doesn't either, since RecursiveUpdateTxState only walks the transaction's outputs and never its inputs. With 1 the wallet functional tests still pass and the case goes away.
| it->second.SetTxFromMe(*wtx.m_from_me); | ||
| } else { | ||
| WalletTXO txo{txout, wtx.GetState(), wtx.IsCoinBase(), *wtx.m_from_me, wtx.GetTxTime(), wtx.GetTx()->version}; | ||
| bool is_unusable = m_last_block_processed_height >= 0 && IsSpent(outpoint, /*min_depth=*/1); |
There was a problem hiding this comment.
This is missing the TxStateBlockConflicted check that PruneSpentTXOs does. RemoveTxs exposes it, since it clears the containers before rebuilding. Mirroring PruneSpentTXOs here makes the balance match the base again.
| result = std::max(result, tx_res.m_result); | ||
|
|
||
| // Upgrade each CWalletTx missing m_from_me | ||
| if (any_missing_from_me) { |
There was a problem hiding this comment.
ReorderTransactions used to sit after LoadWallet's if (result != DBErrors::LOAD_OK) return result;, and now both it and the new upgrade loop run before that return. With one corrupt tx record, loadwallet fails and the intact records still get rewritten, 310 to 311 bytes here.
| it->second.SetState(wtx.GetState()); | ||
| it->second.SetTxFromMe(*wtx.m_from_me); | ||
| } else { | ||
| WalletTXO txo{txout, wtx.GetState(), wtx.IsCoinBase(), *wtx.m_from_me, wtx.GetTxTime(), wtx.GetTx()->version}; |
There was a problem hiding this comment.
m_from_me is an optional<bool> and CWalletTx(tx, state) doesn't set it. That's the three red fuzz jobs, all on wallet_create_transaction, with UBSan reporting invalid-enum-load and MSan the libc++ hardening assertion on operator*. The RefreshTXOsFromTx call there came in with #35790. LoadToWallet guards with has_value(), but the precondition isn't documented anywhere and the method is public.
12 participants
In #27286, the wallet keeps track of all of its transaction outputs, even if they are already spent or are otherwise unspendable. This TXO set is iterated for balance checking and coin selection preparation, which can still be slow for wallets that have had a lot of activity. This PR aims to improve the performance of such wallets by moving UTXOs that are definitely no longer spendable to a different map in the wallet so that far fewer TXOs need to be iterated for the aforementioned functions.
Unspendable TXOs (not to be confused with Unspent TXOs) are those which have a spending transaction that has been confirmed, or are no longer valid due to reorgs. TXOs that are spent in unconfirmed transactions remain in the primary TXO set, and are filtered out of balance and coin selection as before.