When a block is disconnected, update transactions that are no longer conflicted (
wallet) May 31, 2023
The PR branch HEAD was 89df798 at the time of this review club meeting.
A transaction is considered
conflicted when one or more of its inputs has been spent by another
confirmed transaction. A conflicted transaction is marked with negative depth equal to the number of
confirmations on the conflicting transaction.
It’s possible for a transaction to have previously been in a block that used to be part of the
most-work chain but has since been reorged out.
The wallet keeps track of relevant transactions and their confirmation status. This information is
used to calculate the wallet’s balance(s). For example:
If a transaction that is 100 blocks deep in the most-work chain, the wallet can
reasonably include its UTXOs in the balance displayed to the user.
If a transaction conflicts with another transaction 100 blocks deep in the most-work chain,
the wallet can be equally sure that, even though the transaction may have a valid signature,
its UTXOs do not count towards the user’s balance.
If a transaction is unconfirmed and in the node’s mempool, the wallet should account for its
UTXOs, but not consider them as safe as confirmed ones.
The author has provided more notes on transaction states and their effects on balance calculation
Wallet Transaction Conflict
across chainstate and mempool events is tricky. As described in Issue #7315,
when a block is disconnected, the wallet should be marking conflicted transactions as inactive, but
isn’t currently doing so. PR #27145 updates the behavior to mark
transactions that are no longer conflicting after a reorg as inactive. Questions
Did you review the PR?
Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
What is the issue this PR addresses? Can you reproduce the problem described? (Hint: try running
wallet_conflicts.py on master. What error do you get?)
What are the different
CWalletTx can be in?
Which, if any, of the
TxStates are “final,” i.e. once a transaction reaches this
state, it will never change again?
Where in net_processing.cpp is
CWallet::blockDisconnected() triggered to be executed (Hint: it is
not directly called. See
Which thread executes this wallet function?
What does the wallet do with
? (Who is notifying whom, of what?)
RecursiveUpdateTxState() do and why is it “recursive”? What are its callsites?
(Before you grep, where do you think this function should be called?)
What is tested in wallet_conflicts.py? Can you think of any other cases that should be tested?
1 17:00 <josie> #startmeeting
3 17:00 <abubakarsadiq> Hi
9 17:00 <pablomartin> hello
10 17:00 <hernanmarino> Hi
11 17:00 <effexzi> Hi every1
12 17:01 <SebastianvStaa> hi
13 17:01 <josie> welcome to this week's PR review club. any first timers here?
15 17:01 <josie> Pins: welcome!
17 17:01 <glozow> Pins: welcome!
18 17:02 <josie> just a general reminder: don't hesitate to ask a question or speak up (no need to ask if its okay to ask)
19 17:02 <josie> first question: did you get a chance to review the PR?
20 17:03 <josie> (can respond with a y/n)
21 17:03 <SebastianvStaa> y
23 17:03 <abubakarsadiq> y
24 17:03 <hernanmarino> n, just lurking today
25 17:03 <pablomartin> same
27 17:03 <josie> if you did review it, what was your approach? and what's your conclusion? concept/approach ack, or nack?
28 17:03 <josie> lurking is always fine :)
29 17:04 <hernanmarino> From a light reading , approach ACK .
30 17:05 <abubakarsadiq> Tested Ack I, run the test on the PR it passed, and also run the functional test on master to ensure the test fail.
31 17:05 <SebastianvStaa> same here
33 17:05 <LarryRuane> abubakarsadiq: same
34 17:05 <josie> abubakarsadiq: nice! I was curious if anyone got a chance to run the test on master before the PR got merged
36 17:07 <josie> stickies-v: thanks for the link!
37 17:07 <abubakarsadiq> Yeah, thats what I did, thanks for the link and help stickies-v
38 17:07 <josie> so question 2: what issue does this PR address? can you reproduce the problem?
39 17:08 <josie> (for those who ran the test on master pre-merge, this should be an easy question :D)
40 17:09 <abubakarsadiq> This PR address issue whereby if a block is disconnected, the state of all the transaction in the block that our node/wallet know will change to inactive and have 0 confirmations.
42 17:10 <SebastianvStaa> On master, formerly conflicted txns are not set from conflicted to inactive after reorg
43 17:10 <SebastianvStaa> (reorginging out of the conflicted txn)
44 17:13 <josie> SebastianvStaa: yep! before this PR, txs marked as conflicted were not being updated to the correct state of inactive when relevant
45 17:15 <josie> abubakarsadiq: I didn't actually verify what the behavior was before this PR, so you maybe correct. I'd say the main issue this PR attempts to address is *not* marking txs as inactive on blockDisconnect, when they should be marked inactive
46 17:15 <josie> unrelated question: did anyone get a chance to read the wiki or the gist linked in the notes? if not, I'd recommend it! I learned a lot about re-orgs and transaction states by reading them
49 17:17 <josie> okay, moving on to question 3: what are the different states a CWalletTx (a transaction known by the wallet) can be in?
50 17:17 <josie> glozow: thanks!
51 17:17 <abubakarsadiq> thanks josie
52 17:17 <SebastianvStaa> TxStateConfirmed: Contains corresponding block information
53 17:17 <SebastianvStaa> TxStateInMempool
54 17:17 <SebastianvStaa> TxStateConflicted: Contains corresponding conflicting block information
55 17:17 <SebastianvStaa> TxStateInactive: Can be abandoned or not
56 17:17 <SebastianvStaa> TxStateUnrecognized: Treated as inactive
57 17:18 <abubakarsadiq> Inactive, Confirmed, Conflicted, InMempool, and Unrecognized
58 17:19 <josie> SebastianvStaa, abubakarsadiq: yep! curious what you think about Unrecognized? I was trying to think of an example of an Unrecognized state and couldn't come up with one
59 17:20 <SebastianvStaa> josie don't know yet. Just looked up the state definitons in the code
60 17:20 <abubakarsadiq> Josie, Are they transaction whose inputs are known to the node?
61 17:21 <josie> yeah, same. Unrecognized stuck out to me tho, as it seems like this is an area we should be able to account for everything
62 17:21 <glozow> what does a tx state start as if you created but couldnt broadcast it (yet)?
63 17:22 <glozow> or would that never go to mapwallet?
64 17:22 <josie> abubakarsadiq: it's certainly possible to have external inputs in a tx, but I don't think this would apply here as the inputs would either be in a confirmed or unconfirmed state. if its confirmed the node definitely knows about it since it appears in a block
65 17:22 <ishaana> glozow: TxStateInactive
66 17:22 <glozow> ishaana: ah thanks
67 17:22 <ishaana> and I think it would be added to mapWallet
68 17:23 <josie> glozow, ishaana: interesting! my gut reaction was to say it wouldn't be added to mapWallet yet
69 17:24 <josie> seems like Unrecognized might be a better state than inactive for a tx yet to be broadcasted? anyways, don't want to go on too far a tangent
70 17:25 <josie> question 4: what transaction states are considered "final" ?
71 17:26 <josie> by final, we mean once a tx reaches this state it will never (or it is extremely unlikely) change states again
72 17:26 <SebastianvStaa> josie since there is only statistical finality in Bitcoin, probably no txns state is final forever
73 17:26 <abubakarsadiq> considering reorg happens i dont think there is
74 17:26 <SebastianvStaa> abubakarsadiq +1
76 17:27 <josie> SebastianvStaa, abubakarsadiq: reading the question from the notes, I agree :) which is why I added the "extremely unlikely" qualifier
78 17:29 <glozow> thanks for the link!
79 17:29 <ishaana> I don't think that an "abandoned" transaction can change tx state, but technically that would just be a TxStateInactive transaction
80 17:29 <josie> but I think we have to assume some notion of probabilistic finality, otherwise something like showing a wallet balance would be impossible
81 17:30 <josie> so given that we accept some notion of probabilistic finality, what states would you consider "final"/
84 17:30 <abubakarsadiq> on master before this pr was merged, just asking conceptually the real problem that will require the state to change to inactive
85 17:32 <SebastianvStaa> josie TxStateConfirmed could be considered final (unless reorg)
86 17:33 <josie> SebastianvStaa: correct! TxStateConfirmed is considered (increasingly) final the more confirmations it has. The PR specifically mentions 100 blocks as a number where a TxState is definitely considered final
87 17:34 <Pins> +1 (considering it is extremely unlikely the reorg)
88 17:34 <josie> any other states we might consider "final" (especially if we assume > 100 confirmations)
90 17:35 <Pins> TxStateConflicted
91 17:35 <SebastianvStaa> yes, that is also the time span after which the coinbase transaction becomes spendable
92 17:35 <abubakarsadiq> +1 Josie
93 17:36 <josie> abubakarsadiq: I'm not sure I follow your question? The conflicted transaction shouldn't be mine-able as it would be spending inputs that are already confirmed spent in the longest chain
94 17:36 <josie> wim96: I would consider "spent" as TxConfirmed
95 17:37 <SebastianvStaa> josie maybe TxStateConflicted could be considered final after conflicting txn is mined 100 blocks deep
96 17:38 <josie> Pins, SebastianvStaa: ah! so it seems we agree that TxConflicted is not really a "final" state. based on this PR, what state does a TxConflicted tx get updated to? (e.g on blockDisconnect)
97 17:39 <abubakarsadiq> Josie the conflicting transaction's block was disconnected.
98 17:39 <Pins> TxStateInactive
99 17:40 <abubakarsadiq> TxStateInactive
100 17:40 <SebastianvStaa> josie TxStateInactive
101 17:40 <josie> Pins: yep! from my understanding of the PR, I'd say TxStateInactive is a "final" state for a tx in our wallet
103 17:41 <josie> so we have TxConfirmed (the tx in a block that is part of the heaviest chain), and TxInactive (txs that were at one point in a conflicting block, but that block is no longer part of the longest chain)
104 17:42 <josie> okay, question 5: where in `net_processing.cpp` is `CWallet::blockDisconnected()` triggered to be executed?
105 17:42 <josie> which thread executes this wallet function?
106 17:43 <SebastianvStaa> josie: didn'T we state earlier that TxStateInactive is also the inital state for a txn after creation?
107 17:43 <SebastianvStaa> [7:21:45 PM] <glozow> what does a tx state start as if you created but couldnt broadcast it (yet)?
108 17:43 <SebastianvStaa> [7:22:13 PM] <glozow> or would that never go to mapwallet?
109 17:43 <SebastianvStaa> [7:22:32 PM] <ishaana> glozow: TxStateInactive
110 17:43 <SebastianvStaa> [7:22:55 PM] <glozow> ishaana: ah thanks
111 17:44 <SebastianvStaa> so I don't see why this state could be considered 'final'
112 17:45 <josie> SebastianvStaa: yep, which is a bit surprising to me, tbh. I suppose "final" almost always is accompanied with some notion of confirmations
113 17:46 <SebastianvStaa> josie ok makes sense. the state by itself is not final, the txn need more properties for that
114 17:47 <josie> yep! that's how I think about it. An Inactive tx which spends inputs in the UTXO set wouldn't be considered final
115 17:47 <josie> but an Inactive tx which spends inputs that were spent many blocks back by a different transaction I would definitely consider to be in a final state
116 17:50 <josie> feel free to throw out any ideas regarding question 5. I'll admit, I don't think I know the answer to this one
117 17:50 <SebastianvStaa> I'm curious about question 5 as well, as I don't understand thread concurrency in Bitcoin Core (yet)
118 17:50 <josie> (altho, I did learn that subscribers to the ValidationInterface can assume that events happen in a sequential order, which is kinda cool)
119 17:53 <josie> ishaana: curious if you have any insights for question 5?
120 17:54 <josie> SebastianvStaa: threading in bitcoin core is quite the beast! I'm still stumbling my way through understanding it
122 17:54 <SebastianvStaa> josie +1
123 17:55 <josie> let's move on to question 6: what does the wallet do with TxUpdate::NOTIFY_CHANGED?
125 17:55 <josie> (who is notifying whom, of what?)
126 17:56 <josie> SebastianvStaa: great link! thanks for sharing. https://obc.256k1.dev/ is also a great architecture overview, which might be slightly more up to date
127 17:57 <SebastianvStaa> very nice resource! Thanks
128 17:57 <lightlike> as for q5: looks like ActivateBestChain() is called in various places in net_processing, which can lead to DisconnectTip() in validation being called, which then creates the BlockDisconnected() signal which is picked up by the wallet later.
129 17:58 <abubakarsadiq> Josie: After running the test on master before this pr was merged the error was
132 17:59 <josie> lightlike: thanks! I got as far as DisconnectTip() -> BlockDisconnected() but didn't realize it was originating with ActivateBestChain()
133 18:00 <ishaana> josie: yeah it looks like it is called during ActivateBestChain()
134 18:00 <josie> that's time, so we'll stop here, but I'd encourage everyone to work through the remaining questions!
135 18:00 <josie> #endmeeting