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
|
I think the earlier The unconditional Collecting the inputs in Suggested test: diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py
index bd08819fdc..0b2c05c0f7 100755
--- a/test/functional/wallet_importprunedfunds.py
+++ b/test/functional/wallet_importprunedfunds.py
@@ -157,6 +157,36 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
available_utxos = [u["txid"] for u in node.listunspent(minconf=0)]
assert utxo["txid"] not in available_utxos, "UTXO should still be spent by conflicting tx"
+ self.log.info("Test removeprunedfunds preserves inputs spent by another confirmed tx")
+ parent_txid = node.sendtoaddress(node.getnewaddress(), 1)
+ self.generate(node, 1)
+ parent_utxo = next(
+ utxo for utxo in node.listunspent() if utxo["txid"] == parent_txid
+ )
+ parent_outpoint = (parent_txid, parent_utxo["vout"])
+
+ child_txid = node.send(outputs=[{node.getnewaddress(): Decimal("0.5")}], inputs=[parent_utxo])["txid"]
+ child_fee = node.gettransaction(child_txid)["fee"]
+
+ # Replace child_txid with a conflicting tx and confirm it. Removing
+ # child_txid must not make parent_outpoint spendable again.
+ output_value = parent_utxo["amount"] + child_fee - Decimal("0.00001")
+ raw_conflict_tx = node.createrawtransaction(inputs=[parent_utxo], outputs=[{node.getnewaddress(): output_value}])
+ signed_conflict_tx = node.signrawtransactionwithwallet(raw_conflict_tx)
+ conflict_txid = node.sendrawtransaction(signed_conflict_tx["hex"])
+ assert_not_equal(conflict_txid, child_txid)
+ self.generate(node, 1)
+
+ def confirmed_parent_is_unspent():
+ return any(
+ (utxo["txid"], utxo["vout"]) == parent_outpoint
+ for utxo in node.listunspent(minconf=0)
+ )
+
+ assert_equal(confirmed_parent_is_unspent(), False)
+ node.removeprunedfunds(child_txid)
+ assert_equal(confirmed_parent_is_unspent(), False)
+
self.log.info("Test removeprunedfunds restores inputs of removed confirmed spend")
parent_txid = node.sendtoaddress(node.getnewaddress(), 1)
self.generate(node, 1) |
Checking the states to revert them was starting to get a bit complicated, so I ended up doing the overkill thing of recomputing all of the TXOs after removing any transaction. That should resolve any issues here.
Included the test. Also ended up reworking how the tx states are set so that |
|
🚧 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. |
w0xlt
left a comment
There was a problem hiding this comment.
AddToWallet()'s isInactive() branch marks the inputs of an inactive tx usable again without checking whether another confirmed tx still spends them, which lets a confirmed-spent TXO back into m_txos and trips the new Assert(!wallet.IsSpent(outpoint, 1)) in GetBalance(), aborting the node on every subsequent balance query.
Sequence:
- UTXO
Ois spent byT1, which confirms →Omoves tom_unusable_txos. sendwithOas a preset input buildsT2:FetchSelectedInputsresolves it viaGetTXO()(which now also returns unusable TXOs) and has noIsSpentcheck on the preset path.CommitTransactioncallsAddToWallet(T2, TxStateInactive{})before broadcasting; theisInactive()branch runsMarkTXOUsable(O). The broadcast then fails withbad-txns-inputs-missingorspent, but the wallet state was already mutated.- Next
getbalances:
wallet/receive.cpp:256 GetBalance: Assertion `!wallet.IsSpent(outpoint, 1)' failed.
Suggestion:
} else if (wtx.isInactive()) {
- // When a transaction becomes inactive, we need to mark its inputs as usable again
+ // When a transaction becomes inactive, we need to mark its inputs as usable again,
+ // unless an input is still spent by a different confirmed transaction
for (const CTxIn& txin : wtx.tx->vin) {
- MarkTXOUsable(txin.prevout);
+ if (!IsSpent(txin.prevout, /*min_depth=*/1)) {
+ MarkTXOUsable(txin.prevout);
+ }
}
}(Alternatively the guard could live inside MarkTXOUsable() itself, which would protect the other/future callers too.)
The test below crashes the node at getbalances on the current head and passes with the patch:
diff
#!/usr/bin/env python3
"""Test that committing a tx that spends an already confirmed-spent input
does not resurrect the spent TXO into the usable TXO set."""
from decimal import Decimal
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal
class WalletSpentTXOResurrectTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
def run_test(self):
node = self.nodes[0]
node.createwallet("resurrect")
wallet = node.get_wallet_rpc("resurrect")
default = node.get_wallet_rpc(self.default_wallet_name)
self.log.info("Fund the wallet with UTXO O")
txid = default.sendtoaddress(wallet.getnewaddress(), 1)
self.generate(node, 1)
utxo = next(u for u in wallet.listunspent() if u["txid"] == txid)
outpoint = {"txid": utxo["txid"], "vout": utxo["vout"]}
self.log.info("Spend O in T1 and confirm it -> O becomes definitely unspendable")
t1 = wallet.send(outputs=[{default.getnewaddress(): Decimal("0.9")}],
inputs=[outpoint], add_inputs=False)
assert t1["complete"]
self.generate(node, 1)
self.log.info("Try to double-spend O via a preset input")
# The broadcast fails, but the tx is committed to the wallet first.
try:
wallet.send(outputs=[{default.getnewaddress(): Decimal("0.8")}],
inputs=[outpoint], add_inputs=False)
except Exception as e:
self.log.info(f"send reported: {e}")
self.log.info("Balance queries must still work and O must remain spent")
wallet.getbalances() # aborts the node without the fix
assert_equal(utxo["txid"] in [u["txid"] for u in wallet.listunspent()], False)
if __name__ == "__main__":
WalletSpentTXOResurrectTest(__file__).main()
I've changed |
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.
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.