The PR branch HEAD was 100d5d03f at the time of this review club meeting.
Notes
Motivation
The goal of this PR is to refactor the code to enable removing the mempool’s
NotifyEntryAdded and NotifyEntryRemoved signals.
These boost signals were added in
PR #9371 “Notify on removal”,
before Bitcoin Core had an asynchronous validation interface.
The NotifyEntryAdded callback was used by validation to build a vector of
conflicted transactions when connecting a block, which the wallet was notified
of in the BlockConnectedCValidationInterface callback.
Now that we have an asynchronous TransactionRemovedFromMempool callback, we
can fire that signal directly from the mempool for conflicted transactions
without having to worry about the performance impact of synchronising wallet
transactions during block connection.
Background
A conflicted transaction is one whose inputs have been spent by a different
transaction which has been confirmed in the block chain.
Prior to PR #3669 “Handle
‘conflicted’ transactions properly”, the Bitcoin Core wallet did not track
conflicted transactions. If a transaction sending to or from the wallet was
conflicted, it would continue to be displayed in the wallet as unconfirmed
forever. PR #3669 states:
This introduces the notion of a "conflicted" transaction -- a transaction
created by the wallet that is not in either the blockchain or the memory
pool, and that (therefore) is unlikely to ever be confirmed.
In the RPC interface, these transactions were previously reported as
confirmations: 0.
With this change, they are reported as confirmations: -1 and category:
"conflicted".
So if a transaction is mutated or double-spent, and the mutated version ends
up being mined, listtransactions will show both. Transactions can go from
category conflicted to sent/received if a blockchain re-org happens.
Note that this early version of marking a transaction as conflicted is not capturing
what we want to know, which is whether the inputs to the transaction have been
double-spent in a different transaction.
Accurately tracking when a transaction is conflicted is difficult. Consider the case
where a transaction leaves the mempool because of expiry, limiting or replacement,
and then later a conflicting transaction is included in a block. The wallet
contains some conflict-tracking code to try to deal with cases like this. See
the AddToWalletIfInvolvingMe and CWallet::AvailableCoins functions
(and/or search for “conflict”) in src/wallet/wallet.cpp.
Visit
To see the Notifications class in src/interfaces/chain.h:
git grep -A11 "Chain notifications"
To see the reasons, passed to the notifications, why a transaction can be
removed from the mempool from the perspective of the codebase:
git grep -A7 "enum class MemPoolRemovalReason"
Commit notes
This PR makes changes in a methodical, step-by-step manner that is logical and
clear to follow to facilitate review.
In commit 1 of this PR, the same reasons we saw above for evicting a
transaction from the mempool are added (and more clearly written) in
src/validationinterface.h:91: virtual void TransactionAddedToMempool:
Commit 1 aligns the TransactionAddedToMempool function with the
MemPoolRemovalReason enum class (except one of the reasons… which one
and why?) and removes the transaction conflict checks from the conditional in
src/txmempool.cpp:404: void CTxMemPool::removeUnchecked and from
wallet/wallet.cpp:1099:void CWallet::BlockConnected.
Commit 2 logically proceeds to remove vtxConflicted from the parameters
of all the BlockConnected functions.
Commit 3 removes the conflictedTxs code from class PerBlockConnectTrace
and class ConnectTrace in src/validation.cpp.
Commit 4 removes the NotifyEntryRemoved callback code from class
ConnectTrace in src/validation.cpp. Since PerBlockConnectTrace no longer
tracks conflicted transactions, ConnectTrace no longer requires these
notifications.
Commit 5 completes the local cleanup by removing the pool member from
ConnectTrace in src/validation.cpp.
Finally, the last commit builds on the preparatory work in the preceding
commits, which collectively enable the end goal of the PR: removing
NotifyEntryAdded and NotifyEntryRemoved.
Related open issues of possible interest
#10656
“listsinceblock incorrectly showing some conflicted transactions.”
As an aside, note that the PR author tags contributors for review in a
separate comment and not in the PR description itself. Why?
What are conflicted transactions? Give some examples of how a transaction can
be conflicted. How are they handled by Bitcoin Core?
What does the wallet mean by a conflicted transaction, and why does it care
about tracking these? How is the wallet’s definition of a conflicted
transaction different from MemPoolRemovalReason::CONFLICT?
Does this PR introduce a change of behavior of consequence? Do you see any
race conditions that may arise? Could these be verified with a new test (if
so, try writing one)?
What are different ways a transaction might be removed by a reorg?
When was an asynchronous validation interface added to Bitcoin Core?
Bonus: did you also review the follow-up PR
#17562
“Validation: Remove ConnectTrace and PerBlockConnectTrace”?
<pinheadmz> Last week we were talking about code coverage -- I notice bitcoin isn't using Coveralls from Travis to report coverage on PRs - is there a reason why not?
<jonatack> curious what difftools everyone is using for reviewing... i've been using gitk (with dracula for dark mode) but wish i could switch from red/green to nicer syntax highlighting sometimes
<jonatack> i mean, sure, coding is more fun than reviewing -- but reviewing is too vital here, and when i see trivial PRs i keep thinking, maybe too much, about the review resources they might be taking away from catching a regression or CVE
<jonatack> hopefully this review club can help encourage more reviewing but i'm thinking about how else we can encourage it, add incentives or social norms maybe, etc.
<jonatack> FWIW I have begun building a personal website (as a sort of online resume for funding or employment) and migrating my articles here: https://jonatack.github.io/articles/
<jonatack> We usually start Bitcoin Core IRC meetings with a 'hi' so it's clear who's at keyboard. Feel free to say hi, even if you arrive in the middle of the meeting!
<jonatack> amiti: saame, if anything it seems better with these changes, but i'd like to try to write a test to check that (not sure how feasible that would be)
<pinheadmz> jnewbery: right, since those signals replace the vector you removed, they couldve been part of that PR? or at least a follow up after merge
<jnewbery> pinheadmz: exactly. The first commit changes the way the wallet is notified about conflicted txs, and the rest of the commits are just removing the now-dead code
<michaelfolkson> I think the answer to raj_ question is it depends (on the supposed dead code). Sometimes just checking whether it is defined, initialized is enough?
<pinheadmz> conflicted TX can happen in the mempool for lots of reasons, easiest to consider is a TX in a block that double spends something in the mempool
<pinheadmz> fjahr: imagine spending a coinbase output after exactly 100 blocks. then there is a chain reorganiztion. the chain tip block becomes unconfirmed and in that moment, the TX is invalid
<lightlike> yesterday i got confused by the splitup to #17562. After the split, there is still a comment in GetBlocksConnected() mentioning the (now removed) conflicted txes vector, which will be addressed only in the follow-up. So maybe it would make sense to mention the follow-up in the description (or change the comment).
<jnewbery> pinheadmz: I dno't think I'd refer to that as 'unconfirmed'. By unconfirmed, we really mean something that can't be included in the blockchain because its ancestors have been spent somehow
<pinheadmz> jnewbery: yeah true, but woudlnt that fire the removeed signel anyway? in my example, i imageine the coinbase spend is till in the mempool, witing for confirmation in the next block
<jnewbery> pinheadmz: I don't know if we have a name. When there's a reorg we try to put the transactions from that block back in the mempool, but as you've pointed out with the coinbase maturity example, that's not always possible
<pinheadmz> jnewbery: and in the coinbase spend case, if there's a reorg - the tx will likely be valid after the reorg is done. (the chain height will be equal or greater than it was when the tx was first broadcast) but still in the midst of the reorg process, it will be evicted from the mempool (IIUC)
<jnewbery> I think it's quite important to distinguish between what we mean by MemPoolRemovalReason::CONFLICT and what the wallet means by conflicted transactions. Can anyone try to explain that?
<jnewbery> raj_: it's usually that, but imagine if we have a chain of transactions A->B1->C and then B2 double spends B1. Is C a conflicted transaction?
<amiti> I think.. in the wallet you could mark a txn as `abandoned` because its no longer in your mempool and you think its not going to be mined, but some miner or mempool kept it around and it later gets mined, so then you'd update to `conflicted`
<jnewbery> C is also conflicted. Usually the case is that a transaction's inputs are double spent, but it could be the transaction's ancestor's inputs that are double spent
<jnewbery> raj_: exactly. Imagine Alice pays Bob pays me, and then the payment from Alice to Bob is double-spent. I was notified of the payment from Bob to me when it entered the mempool, but it can never confirm. I need to mark that tx as conflicted
<jnewbery> you can imagine an arbitrarily long chain of unconfirmed txs that ends in a payment to you, and then something in that chain is double-spent
<jnewbery> raj_: yes. MemPoolRemovalReason::CONFLICT is the reason given to a transaction being removed from the mempool when a block connects because the tx's inputs or one of the tx's ancestor's inputs is spent by a different transaction included in the block
<jnewbery> so this notification from the mempool to the wallet o fMemPoolRemovalReason::CONFLICT is one way for the wallet to learn about conflicted transactions, but it doesn't capture everything the wallet needs to know to mark its transactions as conflicted
<raj_> just to see if i got it. The wallet would also like to know about in-mempool conflicts, but this signal is only for in-block conflicts. is that correct?
<jnewbery> raj_: almost. The wallet wants to know about arbitrarily deep conflicts, even when the transaction is no longer in the mempool. The mempool can only tell the wallet about txs that are in the mempool at the point the block is connected
<lightlike> I’m sure that this is unrelated to this PR, but did anyone else experience non-reproducible strange output of program code to the console when running the unit tests via test_bitcoin, like “pindexNew->GetBlockHash()” or “m_expected_tip” even though all tests succeed?
<pinheadmz> im a bit curious about boost signals in general, might be out of scope or we can talk after the meeting. Im familiar with libuv "events" in nodejs and just wonder how comprable it is
<fjahr> So, for a short window the tx is marked not in mempool but also not in a block yet. What if there are high fees, a full mempool, and we do some automatical RBFing, so we replace a tx with a higher fee one if our original is evicted from the Mempool because of low fees. We might try to RBF again although it is already in the block. That's the only scenario I could come up with and I did not have time to
<jonatack> lightlike: right, running just test_bitcoin or src/test/test_bitcoin? sometimes i've thought i'm seeing strange behavior there but i'm not sure yet
<michaelfolkson> amiti: What do you mean what would happen? It wouldn't get into a block unless there was a re-org. And it would leave our mempool too unless there was a re-org?
<fjahr> amiti: no, I did look into it further because I had the idea 20min before the meeting. I think there would be no real consequences, I was just thinking if anythink could go wrong after the change, even if consequences are insignificant
<jonatack> fjahr: amiti: agree, i think going through the existing tests on this topic and noodling with adding any that seem missing could be time well-spent