This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the Bitcoin DevWiki. A test which tested the previous behavior has also been updated.
Second attempt at #17543
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rajarshimaitra, achow101, glozow, furszy |
| Concept ACK | ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Concept ACK on refreshing state of conflicted transactions in the wallet when a block is disconnected. I think there is a potential problem with the implementation though because it isn’t recursively marking conflicted transactions as unconfirmed, only marking the top level transactions without recursing.
I think a good approach could be to steal from the previous implementation of this idea in commits b8b4592d985fa36dd0dddfd0e2bc6daf8df6e79c and 3c8bd6814ca88715d3d56994b10dc2c5f3e38e2d from #17543, which did recurse. That approach was also nice because it reused the existing MarkConflicted function in order to avoid code duplication. Commit ec5a89a26fa17e68b3e5928730e7d7c3e0370729 from that PR also added some more test coverage which could be used here.
I think there is a potential problem with the implementation though because it isn’t recursively marking conflicted transactions as unconfirmed, only marking the top level transactions without recursing.
Yes, I had forgotten about this, thanks for pointing it out!
I think a good approach could be to steal from the previous implementation of this idea in commits https://github.com/bitcoin/bitcoin/commit/b8b4592d985fa36dd0dddfd0e2bc6daf8df6e79c and https://github.com/bitcoin/bitcoin/commit/3c8bd6814ca88715d3d56994b10dc2c5f3e38e2d from #17543, which did recurse.
I have used ideas from that PR so that the transaction state updating will recurse.
Commit https://github.com/bitcoin/bitcoin/commit/ec5a89a26fa17e68b3e5928730e7d7c3e0370729 from that PR also added some more test coverage which could be used here.
I have cherry-picked that commit to this branch with a few adjustments.
0@@ -0,0 +1,113 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2023 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+"""Test conflicts tracking with multilple txn conflicting a conflicted tx."""
in 77a2e522d845eda270b2c03fd7e0a16bc4c00fbb:
I think this comment is a little bit hard to follow. Something more descriptive might be
0"""
1Test that wallet correctly tracks transactions that have been conflicted by blocks, particularly during reorgs.
2"""
83+ child_conflicted = self.nodes[0].gettransaction(child_conflicted_txid)
84+ conflicting = self.nodes[0].gettransaction(conflicting_txid_A)
85+ # Conflicted tx should have confirmations set to the confirmations of the most conflicting tx
86+ assert_equal(conflicted["confirmations"], -conflicting["confirmations"])
87+ # Child should inherit conflicted state from parent
88+ assert_equal(child_conflicted["confirmations"], -conflicting["confirmations"])
36+ self.log.info("Create a transaction with conflicted transactions spending outpoints A & B")
37+ nA = next(tx_out["vout"] for tx_out in self.nodes[0].gettransaction(txid_conflict_from_1)["details"] if tx_out["amount"] == Decimal("10"))
38+ nB = next(tx_out["vout"] for tx_out in self.nodes[0].gettransaction(txid_conflict_from_2)["details"] if tx_out["amount"] == Decimal("10"))
39+ inputs_conflicted_tx = []
40+ inputs_conflicted_tx.append({"txid": txid_conflict_from_1, "vout": nA})
41+ inputs_conflicted_tx.append({"txid": txid_conflict_from_2, "vout": nB})
nit in 77a2e522d845eda270b2c03fd7e0a16bc4c00fbb, I think this is clearer (same with the other arrays of inputs and outputs)
0 inputs_conflicted_tx = [{"txid": txid_conflict_from_1, "vout": nA}, {"txid": txid_conflict_from_2, "vout": nB}]
51+ outputs_conflicting_tx_B = {}
52+ outputs_conflicting_tx_B[self.nodes[0].getnewaddress()] = Decimal("9.99998")
53+
54+ conflicted = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs_conflicted_tx, outputs_conflicted_tx))
55+ conflicting_tx_A = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs_conflicting_tx_A, outputs_conflicting_tx_A))
56+ conflicting_tx_B = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs_conflicting_tx_B, outputs_conflicting_tx_B))
Sorry to bikeshed, but I think the naming is a bit hard to follow (“conflicted” vs “conflicting” don’t immediately make sense to me, especially since both sets of transactions get conflicted at some point during the test). Might I suggest names that indicate which input(s) they spend, which node broadcasts them, and what “generation” they are? For example
tx_AB_0_parent instead of conflicted
tx_A_1 instead of conflicting_tx_A
tx_B_1 instead of conflicting_tx_B
C instead of nA for the 19.99998BTC output, because nA is already the name of the 10BTC output from one of the funding transactions.
tx_C_0_child instead of child_conflicted
90+ # Node2 chain without conflicts
91+ self.generate(self.nodes[2], 15, sync_fun=self.no_op)
92+
93+ # Connect node0 and node2 and wait reorg
94+ self.connect_nodes(0, 2)
95+ self.sync_blocks([self.nodes[0], self.nodes[2]])
0 self.sync_blocks()
1464@@ -1465,8 +1465,32 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block)
1465 // future with a stickier abandoned state or even removing abandontransaction call.
1466 m_last_block_processed_height = block.height - 1;
1467 m_last_block_processed = *Assert(block.prev_hash);
1468+
1469+ const int& disconnect_height = block.height;
in commit 49f8af533f2e6370f4ae5220f5c706e30b90d06c
no need for const reference to integer
Concept ACK
There is a missing dash in “Co-authored-by:” in the commit messages
1349+
1350+ RecursiveUpdateTxState(hashTx, get_updated_state);
1351+
1352+}
1353+
1354+void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const GetUpdatedStateFn& get_updated_state) {
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 “wallet: introduce generic recursive tx state updating function”
This could also be used by AbandonTransaction.
1373- // Mark transaction as conflicted with this block.
1374- wtx.m_state = TxStateConflicted{hashBlock, conflicting_height};
1375+ if (get_updated_state(wtx)) {
1376 wtx.MarkDirty();
1377 batch.WriteTx(wtx);
1378 // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 “wallet: introduce generic recursive tx state updating function”
The comments should be updated to avoid mentioning specific transaction states.
1326+
1327+ RecursiveUpdateTxState(hashTx, get_updated_state);
1328+
1329+}
1330+
1331+void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const GetUpdatedStateFn& get_updated_state) {
GetUpdatedStateFn function isn’t really a getter, what about calling it TryUpdateState?.
GetUpdatedStateFn to TryUpdatingStateFn, and get_updated_state to try_updating_state.
1312@@ -1336,13 +1313,29 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
1313 if (conflictconfirms >= 0)
1314 return;
1315
1316- // Do not flush the wallet here for performance reasons
1317+ auto get_updated_state = [&](CWalletTx& wtx) {
1318+ LOCK(cs_wallet);
No need to re-acquire cs_wallet here (RecursiveUpdateTxState already locks cs_wallet) . Can add the exclusive lock annotation instead, e.g:
0const auto& try_update_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
1 if (conflictconfirms >= GetTxDepthInMainChain(wtx)) return false;
2
3 wtx.m_state = TxStateConflicted{hashBlock, conflicting_height};
4 return true;
5};
(same for the others)
1459+ for (TxSpends::const_iterator _it = range.first; _it != range.second; ++_it) {
1460+ CWalletTx& wtx = mapWallet.find(_it->second)->second;
1461+
1462+ if (!wtx.isConflicted()) continue;
1463+
1464+ auto try_updating_state = [&](CWalletTx& wtx) {
wtx shadows the function’s wtx. Could just provide disconnect_height.
The lambda function’s wtx shadows the function’s wtx.
I have renamed the lambda function’s wtx.
Could just provide disconnect_height.
By this do you mean providing the disconnect_height of the block to RecursiveUpdateTxState or providing the wallet tx’s conflicting_block_height to try_updating_state instead of the whole wallet tx by reference?
1325+ RecursiveUpdateTxState(hashTx, try_updating_state);
1326+
1327+}
1328+
1329+void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) {
1330+ LOCK(cs_wallet);
cs_wallet acquired. Could add the lock required annotation instead.
1296- // If the orig tx was not in block/mempool, none of its spends can be in mempool
1297 assert(!wtx.InMempool());
1298 wtx.m_state = TxStateInactive{/*abandoned=*/true};
1299- wtx.MarkDirty();
1300- batch.WriteTx(wtx);
1301 NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);
In f998cdbc:
Would be good to move this NotifyTransactionChanged to RecursiveUpdateTxState too. So MarkConflicted and the BlockDisconnected txes state changes are notified to the upper layers.
In commit “wallet: introduce generic recursive tx state updating function” (f998cdbc689cc65ff0aca98ce63758c59786c1db)
I think that this is a good idea, but would out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.
Agree we should avoid changing behavior, but I think the current commit might also be changing behavior by notifying before calling MarkDirty and WriteTx instead of after.
I think the best thing to do would be to change TryUpdatingStateFn to return an enum instead of a bool like:
0enum class TxUpdate { UNCHANGED, CHANGED, NOTIFY_CHANGED };
1using TryUpdatingStateFn = std::function<TxUpdate(CWalletTx& wtx)>;
Returning CHANGED/UNCHANGED should make the code clearer anyway because it’s less obvious what true and false mean.
Thanks for the review @achow101 and @furszy!
Haven’t finished checking the latest changes but, if you like it, https://github.com/furszy/bitcoin-core/commit/8bb14c1eaf09b0bca9efa78e1a7a7a00c61c0a29 is all yours. During the review, the functional test wasn’t clear enough for me so I spent a bit of time making it more friendly.
Thanks, I’ll use these improvements here once I review them. Edit: I’ve added the suggested changes to the functional tests.
1288- CWalletTx& wtx = it->second;
1289+ auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
1290 int currentconfirm = GetTxDepthInMainChain(wtx);
1291- // If the orig tx was not in block, none of its spends can be
1292 assert(currentconfirm <= 0);
1293- // if (currentconfirm < 0) {Tx and spends are already conflicted, no need to abandon}
In commit “wallet: introduce generic recursive tx state updating function” (f998cdbc689cc65ff0aca98ce63758c59786c1db)
I think as long as this code is still calling GetTxDepthInMainChain, it would be better to keep the 3 comments that were here instead of deleting them, because they explain why the asserts are valid and why conflicted transactions are ignored
1285- done.insert(now);
1286- auto it = mapWallet.find(now);
1287- assert(it != mapWallet.end());
1288- CWalletTx& wtx = it->second;
1289+ auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
1290 int currentconfirm = GetTxDepthInMainChain(wtx);
In commit “wallet: introduce generic recursive tx state updating function” (f998cdbc689cc65ff0aca98ce63758c59786c1db)
It seems like the code in this lambda would be simpler it avoided calling GetTxDepthInMainChain. The following should be equivalent:
0// If the orig tx was not in block/mempool, none of its spends can be.
1assert(!wtx.isConfirmed());
2assert(!wtx.InMempool());
3// If already conflicted or abandoned, no need to set abandoned.
4if (!wtx.isConflicted() && !wtx.isAbandoned()) {
5 wtx.m_state = TxStateInactive{/*abandoned=*/true};
6 ...
7}
Free to ignore this suggestion if you think it would complicate things, though. The cleanup is orthogonal and could be done separately.
1312@@ -1336,13 +1313,27 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
1313 if (conflictconfirms >= 0)
1314 return;
1315
1316- // Do not flush the wallet here for performance reasons
In commit “wallet: introduce generic recursive tx state updating function” (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This also seems like a useful comment. Would keep instead of deleting
1320+ return true;
1321+ }
1322+ return false;
1323+ };
1324+
1325+ RecursiveUpdateTxState(hashTx, try_updating_state);
In commit “wallet: introduce generic recursive tx state updating function” (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This code pretty opaque, would suggest copying the previous comment here “Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too” or something equivalent.
1312@@ -1336,13 +1313,27 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
1313 if (conflictconfirms >= 0)
1314 return;
1315
1316- // Do not flush the wallet here for performance reasons
1317+ auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
1318+ if (conflictconfirms < GetTxDepthInMainChain(wtx)) {
1319+ wtx.m_state = TxStateConflicted{hashBlock, conflicting_height};
In commit “wallet: introduce generic recursive tx state updating function” (f998cdbc689cc65ff0aca98ce63758c59786c1db)
Would suggest keeping the previous comment here instead of deleting it. It’s not obvious what the check means or what’s happening in the block:
0// Block is 'more conflicted' than current confirm; update.
1// Mark transaction as conflicted with this block.
1291- assert(currentconfirm <= 0);
1292- // if (currentconfirm < 0) {Tx and spends are already conflicted, no need to abandon}
1293- if (currentconfirm == 0 && !wtx.isAbandoned()) {
1294- // If the orig tx was not in block/mempool, none of its spends can be in mempool
1295- assert(!wtx.InMempool());
1296+ auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
nit: if you have to retouch. Can remove the context ref.
0 auto try_updating_state = [](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
This commit also changed `MarkConflicted` and `AbandonTransaction`
to use this new function
Co-authored-by: ariard <antoine.riard@gmail.com>
232 balance = newbalance
233
234- # There is currently a minor bug around this and so this test doesn't work. See Issue #7315
235- # Invalidate the block with the double spend and B's 10 BTC output should no longer be available
236- # Don't think C's should either
237+ # Invalidate the block with the double spend. B & C's 10 BTC outputs should no longer be available
Let me see if I’m following this:
Tx_AB1 spends B, and Tx_ABC2 (the child of Tx_AB1) spends C. So, when the double spend tx gets disconnected, Tx_AB1 state changes to inactive, marking output B and C as spent again.
Which ends up with the txes stuck now right?, and the user need to manually re-submit them.
In `blockDisconnected`, for each transaction in the block, look
for any wallet transactions spending the same inputs. If any of
these transactions were marked conflicted, they are now marked as
inactive.
Co-authored-by: ariard <antoine.riard@gmail.com>
Test the case of a tx being conflicted by multiple
txs with different depths. The conflicted tx is also spent by
a child tx for which confirmation status is tied to the parent's.
After a reorg of conflicting txs, the conflicted status should be
undone properly.
Co-authored-by: furszy <mfurszy@protonmail.com>
mapTxSpends.
tACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8.
Changes look very clean. We reviewed it in out weekly club and went over the new code addition.
Ran the tests on the master branch and saw it failing assert_equal(conflicted["confirmations"], 0). In the master conflicting transaction’s state doesn’t update after reorg.
I’ve fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in mapTxSpends.
Could you expand this?
Not sure if I follow the latest push, where only the inequality was changed.
If mapTxSpends.count(outpoint) == 1, the outpoint was spent only once, so the wallet shouldn’t contain any conflicting tx.
If mapTxSpends.count(outpoint) == 1, the outpoint was spent only once, so the wallet shouldn’t contain any conflicting tx.
This is not necessarily always true, because even though the conflicted wallet transaction must always have its spends added to mapTxSpends, this is not the case for all conflicting transactions.
For example, let’s say that Alice has a utxo. She creates a transaction, tx1, spending this utxo to Bob. AddToSpends will get called on Bob’s node for this transaction, because Bob is the recipient. Alice then creates a conflicting transaction to tx1, called tx2, which sends the funds back to herself. However, Bob’s node does not call AddToSpends on tx2, because Bob is not the recipient. If tx2 gets mined, but then that block is disconnected from our node, Bob’s mapTxSpends will only show a single spend for Alice’s utxo in mapTxSpends, even though there are technically two.
Tested ACK 89df7987
For example, let’s say that Alice has a utxo. She creates a transaction, tx1, spending this utxo to Bob. AddToSpends will get called on Bob’s node for this transaction, because Bob is the recipient. Alice then creates a conflicting transaction to tx1, called tx2, which sends the funds back to herself. However, Bob’s node does not call AddToSpends on tx2, because Bob is not the recipient. If tx2 gets mined, but then that block is disconnected from our node, Bob’s mapTxSpends will only show a single spend for Alice’s utxo in mapTxSpends, even though there are technically two.
Made a quick unit test and all good. Good finding. Would be good to have a real functional test in the future.