Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals (validation)

https://github.com/bitcoin/bitcoin/17477

Host: jonatack

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 BlockConnected CValidationInterface 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.

  • PR #3694 “Remove CWalletTx::vfSpent” notified the wallet directly when a transaction left the mempool because it became conflicted with a transaction in a newly connected block.

  • 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:
    git grep -A17 "virtual void TransactionAddedToMempool(const CTransactionRef "
    
  • 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.
  • #10656 “listsinceblock incorrectly showing some conflicted transactions.”

  • #11853 “listsinceblock shows conflicted transactions forever.”

  • #12883 “Transactions in mempool that conflict with wallet transactions are not (always) shown in GUI or RPC.”

Bonus: follow-up PR

  • If you have time, review the follow-up PR #17562 “Validation: Remove ConnectTrace and PerBlockConnectTrace.”

Questions

  • Did you review the PR? Concept ACK, approach ACK, ACK <commit>, or NACK?  Don’t forget to put your PR review on GitHub or ask questions.

  • How did you test this PR?

  • 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”?

Meeting Log

17:03 <jonatack> We'll get started in a little under 2 hours for this week's Review Club episode
18:09 <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?
18:18 <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
18:20 <pinheadmz> jonatack: honestly on macos theres a piece of junk called filemerge that is the default for git difftool
18:21 <pinheadmz> i like it because i can scroll through the entire file (not just snippets like github) and i prefer side-by-side comaprison
18:22 <jonatack> we shouldn't be reviewing on GitHub anyway, only using for commenting
18:24 <jonatack> pinheadmz: interesting, i no longer use macOS but back then i was using things like opendiff and meld for macOS
18:25 <jonatack> i have a note-to-self blog post coming on about how to choose PRs to review
18:27 <jonatack> which PRs we review with our limited time, of the 300+ open ones on the stack, is an important choice
18:28 <jonatack> and the trivial/newest/easy ones were receiving more attention (at least from me) than they would warrant
18:30 <jonatack> so i'm actively trying to pay less attention to new/easy/trivial ones in favor of high-prio, higher-value, harder ones
18:30 <pinheadmz> Looking forward to review club for Taproot :-)
18:30 <jonatack> we see really important PRs sometimes sitting for months or even years without enough reviewers
18:32 <jonatack> so the daily choice of what to review and what to ignore is maybe underappreciated... it was by me for the first months
18:33 <jonatack> i'm also actively trying to review PRs by people who also review PRs
18:33 <jonatack> and less by people who add PRs to the stack but don't review PRs or test issuens
18:34 <jonatack> heh, enough ranting, will make a blog post methinks
18:35 <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
18:36 <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.
18:53 <jnewbery> jonatack: those are good things to think about. I'm looking forward to reading that blog post!
18:54 <jnewbery> pinheadmz: I expect we'll do a series of review clubs on the schnorr/taproot PR. There's too much to cover in one meeting
18:56 <jonatack> jnewbery: +1
18:59 <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/
18:59 <kanzure> 404
19:00 <jonatack> it's WIP and thanks to lisa neigut for helping with the styling
19:00 <jonatack> #startmeeting
19:00 <jnewbery> hi
19:00 <ajonas> hi
19:00 <pinheadmz> hi
19:00 <kanzure> hi
19:00 <lightlike> hi
19:00 <amiti> hi
19:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club
19:00 <raj_> hi
19:00 <andrewtoth> hi
19:00 <michaelfolkson> hi
19:01 <jonatack> topic Today we are looking at PR 17477, "Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals" (validation)
19:01 <fjahr> hi
19:01 <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!
19:01 <jonatack> Do jump in at any point with thoughts and questions.
19:01 <jonatack> I am personally here above all to learn -- and look forward to everyone sharing their thoughts.
19:01 <jonatack> Don't be shy! This discussion is about your thoughts and input.
19:02 <gr0kchain> hi
19:02 <jonatack> To start, everyone please give a quick y or n: did you have the chance to read the notes and questions for this meeting?
19:02 <raj_> y
19:02 <gr0kchain> n
19:02 <fjahr> y
19:02 <amiti> y
19:02 <lightlike> y
19:02 <andrewtoth> y
19:02 <jnewbery> y
19:03 <pinheadmz> y
19:03 <jonatack> Nice. Now y or n: did you have the chance to review the PR?
19:03 <gr0kchain> n
19:03 <raj_> y
19:03 <fjahr> y
19:04 <lightlike> y
19:04 <amiti> I started reviewing but am not done
19:04 <andrewtoth> n
19:04 <pinheadmz> yeah
19:04 <jnewbery> y
19:05 <jonatack> Great. raj_ fjahr lightlike pinheadmz: Concept ACK, approach ACK, ACK, or NACK?
19:05 <pinheadmz> "limited knowledge ACK" haha
19:05 <jonatack> (I think jnewbery is an ACK)
19:05 <pinheadmz> I dont know if this breaks anythign
19:06 <jonatack> amiti: (initial thoughts yay or nay?)
19:06 <raj_> Concept ACK. Ran ubit and functional test, standard tests passing. Havent tested manually.
19:06 <fjahr> still need to think about it a little more, 90% there to ACK
19:06 <jonatack> Would anyone like to describe what these mempool notifications do?
19:07 <jonatack> As an aside, note that the PR author tags contributors for review in a separate comment and not in the PR description itself.
19:07 <lightlike> concept ack for me, not 100% sure about the subtle differences to the new handling of conflicted txs
19:07 <fjahr> The wallet needs information about its txs so it needs to be informed when the mempool changes
19:08 <jonatack> (anyone want to describe why?)
19:08 <raj_> They notify tx entry/removal events in mempool to subscribing instances.
19:08 <amiti> seems reasonable. tried to think through concrete examples of any issues from separating the notifications, but I can't come up with anything.
19:09 <andrewtoth> I believe the description becomes part of the commit description, so any upstream projects will notify the tagged users. Is that right?
19:09 <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)
19:09 <andrewtoth> *downstream
19:10 <jonatack> or at least go through the existing tests
19:10 <fjahr> andrewtoth: +1
19:10 <jonatack> existing test files: git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*'
19:10 <jonatack> andrewtoth: right, anyone tagged with get a bunch of notifs
19:10 <jonatack> will*
19:11 <jonatack> recently iiuc the merge script was updated to remove the @-tags
19:11 <pinheadmz> this PR removes code that is essentially dead since mempool notifiactions were intorduced
19:11 <jonatack> but it's a good practice to not tag in the PR description and in the commits
19:11 <pinheadmz> is there a rason why it wasnt pulled out by that PR?
19:12 <jonatack> pinheadmz: i'm guessing here but can imagine at least two good reasons:
19:12 <jnewbery> pinheadmz: which PR?
19:13 <jonatack> - smaller, more focused PRs are generally easier to review and have merged
19:13 <michaelfolkson> A recent example (non-related to this PR) of code that was assumed to be dead but wasn't.... https://github.com/bitcoin/bitcoin/pull/17965
19:13 <pinheadmz> jnewbery: 9371 for example
19:13 <jonatack> - anything involving validation needs a high degree of review and careful scrutiny
19:13 <pinheadmz> jonatack: makes sense
19:14 <jonatack> as maintaining consensus is the overarching mission of bitcoin core
19:14 <pinheadmz> it just means this unused vector was still getting written to memory for about 3 years of release :-) (IIUC)
19:14 <jnewbery> You're talking about the NotifyEntryAdded and NotifyEntryRemoved signals?
19:15 <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
19:15 <pinheadmz> but i may be missing other uses for it that kept it around until now
19:15 <jnewbery> NotifyEntryRemoved is used to add conflicted txs to the conflictedTxs vector in PerBlockConnectTrace
19:16 <jonatack> kanzure: thank you, the link had an extra / ... it's https://jonatack.github.io/articles (and WIP)
19:16 <jnewbery> that's changed by this PR17477
19:17 <pinheadmz> jnewbery: oh i see! so that is the last reference to the conflicted Txs vector
19:17 <jonatack> right, this code isn't dead per se
19:17 <pinheadmz> ty.
19:18 <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
19:18 <jonatack> I liked how jnewbery structured the commits with step-by-step sequentially logical changes that made it much easier to review
19:19 <jonatack> The first commit is the most impactful one
19:19 <raj_> is there any process through which we can determine which parts of the code base are dead? or its just by experienced telling?
19:19 <jonatack> followed by almost mechanical changes that lead to the last commit removing the callbacks
19:19 <raj_> jonatack: +1, really loved the organization of this commit. made things much easier.
19:20 <jonatack> yes!
19:20 <pinheadmz> yes +1 for long commit messages and including the module name in [brackets]
19:20 <jonatack> imagine if it had been a single commit, and pinheadmz yes +1 on the messages
19:21 <jonatack> the commit messages are so clear that i didn't see what else to add to describing them in the notes
19:21 <jonatack> Anyone: What are conflicted transactions?
19:22 <pinheadmz> you can tell when an author spends a lot of time reviewing :-) "what would make this easier for everyone else to review?"
19:22 <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?
19:22 <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
19:22 <jonatack> pinheadmz: I agree, this is an underrated quality
19:22 <pinheadmz> but more interesting cases inolve premature coinbase spends when a block is UN condifmred
19:23 <pinheadmz> and relative timelocks
19:23 <jonatack> raj_: one dumb trick, other than git grepping for things, is to rename or remove a call site and see if building fails
19:24 <michaelfolkson> Call site?
19:24 <fjahr> pinheadmz: "when a block is UN condifmred" => I don't understand
19:24 <jonatack> raj_: this is what i did, for instance, with the boost signals include in my review... i removed it to see what would happen
19:24 <raj_> jonatack: nice trick, thnaks.
19:25 <jonatack> Is a double spend and a conflicted transaction the same thing?
19:25 <jnewbery> michaelfolkson: where the function is called from somewhere else
19:25 <michaelfolkson> fjahr Unconfirmed
19:26 <jonatack> For example, to try to see what code covers conflicted transactions and double spending:
19:26 <jonatack> git grep "double spend\|double-spend\|conflicted" -- 'src/' -- :^'*/qt/*'
19:27 <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
19:27 <jonatack> and to try to see the Mempool policy on double-spending, try: git grep -B4 -A3 "transaction that double-spends"
19:28 <fjahr> Ok, I have never thought of that as an unconfirmed block, thanks!
19:28 <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).
19:28 <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
19:28 <pinheadmz> jnewbery: "disconnected" ?
19:28 <jonatack> lightlike: thanks, where exactly?
19:29 <jnewbery> in your example, the transaction can be included later because it's still valid and its inputs haven't been spent
19:29 <fjahr> I would have said "re-orged out block" or something I guess
19:29 <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
19:30 <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
19:30 <lightlike> jonatack: line 2499, https://github.com/bitcoin/bitcoin/pull/17477/files#diff-24efdb00bfbe56b140fb006b562cc70bL2513 if that link works.
19:30 <jnewbery> pinheadmz: no, because the tx isn't in the mempool. The signal is fired when a tx is removed from the mempool
19:30 <jnewbery> pinheadmz: oh right. Sorry, I misunderstood. Yes, the signal gets fired
19:31 <jnewbery> I think the reason given is REORG rather than CONFLICTED in this case
19:31 <pinheadmz> jnewbery: ah yea
19:32 <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)
19:32 <jnewbery> pinheadmz: here: https://github.com/bitcoin/bitcoin/blob/631df3ee87ec93e1fc748715671cdb5cff7308e6/src/validation.cpp#L371
19:33 <jnewbery> pinheadmz: I think we make some attempt to put those transactions back into the mempool after the re-org, but I can't remember the details
19:34 <jnewbery> (described here: https://github.com/bitcoin/bitcoin/blob/631df3ee87ec93e1fc748715671cdb5cff7308e6/src/txmempool.h#L766)
19:35 <pinheadmz> jnewbery: very cool ty
19:35 <jonatack> txmempool.h is well-commented, kudos to the contributors who did that
19:36 <michaelfolkson> git blame
19:38 <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?
19:38 <jonatack> lightlike: the comment beginning with "// We always keep one extra block at the end of our list because"?
19:41 <jnewbery> ok, maybe simpler question: what does the wallet mean by a conflicted tx? Why does a wallet care about tracking these?
19:41 <lightlike> jonatack: yes, blocks are no longer "added after all the conflicted transactions have been filled in." after this PR.
19:41 <pinheadmz> the wallet should notify the user if they are being double spent against
19:41 <michaelfolkson> A conflict with a transaction in newly connected block versus another in the mempool?
19:41 <raj_> conflicted tx: two tx trying to spend same inputs.
19:42 <jonatack> jnewbery: in AddToWalletIfInvolvingMe?
19:42 <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?
19:43 <raj_> i would guess B1 would be the conflicted tx here.
19:43 <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`
19:44 <amiti> but in the mempool,  you're not really storing a history of txns, so conflicts are amongst the current set
19:44 <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
19:44 <jnewbery> amiti: 'abandoned' is a slightly different thing. It just means that the wallet isn't going to continue trying to resubmit the tx
19:45 <raj_> oh okay. so conflicted tx can be a full chain of such txs.
19:45 <jonatack> lightlike: i see your point that it should be removed in 17477
19:46 <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
19:47 <jnewbery> So the wallet wants to know if a transaction that it is tracking can never be confirmed. That's why it has a concept of conflicted
19:47 <jnewbery> what about MemPoolRemovalReason::CONFLICT?
19:48 <jonatack> jnewbery: ah, in AvailableCoins
19:48 <jonatack>         // It's possible for these to be conflicted via ancestors which we may never be able to detect
19:48 <jnewbery> jonatack: yes!
19:49 <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
19:49 <raj_> `CONFLICT,    //!< Removed for conflict with in-block transaction`, so that means if only a tx conflicts with another that is already in a block?
19:49 <jnewbery> (in practice the mempool descendant limit is 25, but that's still a long chain)
19:50 <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
19:51 <jonatack> raj_: yes: git grep -A7 "enum class MemPoolRemovalReason"
19:51 <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
19:52 <jnewbery> it's a little bit subtle
19:53 <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?
19:53 <jonatack> jnewbery: sounds like more code docs on this that would show up in trivial git grepping might be sweet
19:54 <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
19:55 <jonatack> all: note that this is touched on in the notes https://bitcoincore.reviews/17477 :)
19:56 <raj_> any idea how big such tx chains usally be in practise?
19:56 <jonatack> 3 minutes!
19:56 <jonatack> jnewbery: great distinction to make, ty
19:57 <jonatack> any last questions or comments?
19:57 <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?
19:58 <jonatack> lightlike: not in my case
19:58 <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
19:58 <jonatack> lightlike: running the whole suite or single files?
19:58 <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
19:58 <fjahr> think it through yet.
19:58 <lightlike> jonatack: the whole suite, but via test_bitcoin (not make check)
19:59 <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
19:59 <fjahr> * that I could come up with concerning that window question
20:00 <jonatack> fjahr: I think it would be very interesting to go through the current test coverage on these scenarii and add any missing
20:01 <lightlike> jonatack: src/test/test_bitcoin - ran it like 10 times and got varying strange output in 50% of the runs
20:01 <jonatack> tests that cover conflicted transactions and double spending: git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*'
20:01 <jonatack> let's wrap up
20:01 <jonatack> feel free to continue after!
20:01 <jonatack> #action review PR 17477 on GitHub
20:01 <fjahr> jonatack: yeah, but race conditions like this will always be somewhat flakey :/
20:01 <jonatack> and the follow-up PR
20:02 <raj_> thanks. Very nice pr.
20:02 <andrewtoth> thanks!
20:02 <lightlike> thanks jonatack and jnewbery!
20:02 <jonatack> fjahr: sure. maybe worth trying to make it work and reliable though.
20:02 <jonatack> thanks all for coming, thank you jnewbery for your insights
20:02 <jonatack> #endmeeting
20:02 <fjahr> thanks!
20:03 <amiti> fjahr: do you know, what would happen if we tried to RBF but txn was already in a block
20:03 <jonatack> if anyone would like to host a meeting, feel free to come forth
20:04 <jonatack> for instance, michaelfolkson +1
20:04 <michaelfolkson> One day ;)
20:04 <jonatack> TBH, I learn waay more when hosting
20:05 <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?
20:06 <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
20:10 <amiti> fjahr: yeah, totally. I'm still trying to think through how the eviction and conflict and RBF could play together to create the scenario
20:11 <amiti> michaelfolkson: I don't follow your question
20:14 <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
20:14 <jonatack> to think through the cases
20:20 <michaelfolkson> amiti: It's ok. I don't think I understood your question :)