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

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

Host: jonatack  -  PR author: jnewbery

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

  117:03 <jonatack> We'll get started in a little under 2 hours for this week's Review Club episode
  218: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?
  318: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
  418:20 <pinheadmz> jonatack: honestly on macos theres a piece of junk called filemerge that is the default for git difftool
  518: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
  618:22 <jonatack> we shouldn't be reviewing on GitHub anyway, only using for commenting
  718:24 <jonatack> pinheadmz: interesting, i no longer use macOS but back then i was using things like opendiff and meld for macOS
  818:25 <jonatack> i have a note-to-self blog post coming on about how to choose PRs to review
  918:27 <jonatack> which PRs we review with our limited time, of the 300+ open ones on the stack, is an important choice
 1018:28 <jonatack> and the trivial/newest/easy ones were receiving more attention (at least from me) than they would warrant
 1118: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
 1218:30 <pinheadmz> Looking forward to review club for Taproot :-)
 1318:30 <jonatack> we see really important PRs sometimes sitting for months or even years without enough reviewers
 1418: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
 1518:33 <jonatack> i'm also actively trying to review PRs by people who also review PRs
 1618:33 <jonatack> and less by people who add PRs to the stack but don't review PRs or test issuens
 1718:34 <jonatack> heh, enough ranting, will make a blog post methinks
 1818: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
 1918: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.
 2018:53 <jnewbery> jonatack: those are good things to think about. I'm looking forward to reading that blog post!
 2118: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
 2218:56 <jonatack> jnewbery: +1
 2318: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/
 2418:59 <kanzure> 404
 2519:00 <jonatack> it's WIP and thanks to lisa neigut for helping with the styling
 2619:00 <jonatack> #startmeeting
 2719:00 <jnewbery> hi
 2819:00 <ajonas> hi
 2919:00 <pinheadmz> hi
 3019:00 <kanzure> hi
 3119:00 <lightlike> hi
 3219:00 <amiti> hi
 3319:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club
 3419:00 <raj_> hi
 3519:00 <andrewtoth> hi
 3619:00 <michaelfolkson> hi
 3719:01 <jonatack> topic Today we are looking at PR 17477, "Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals" (validation)
 3819:01 <fjahr> hi
 3919: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!
 4019:01 <jonatack> Do jump in at any point with thoughts and questions.
 4119:01 <jonatack> I am personally here above all to learn -- and look forward to everyone sharing their thoughts.
 4219:01 <jonatack> Don't be shy! This discussion is about your thoughts and input.
 4319:02 <gr0kchain> hi
 4419: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?
 4519:02 <raj_> y
 4619:02 <gr0kchain> n
 4719:02 <fjahr> y
 4819:02 <amiti> y
 4919:02 <lightlike> y
 5019:02 <andrewtoth> y
 5119:02 <jnewbery> y
 5219:03 <pinheadmz> y
 5319:03 <jonatack> Nice. Now y or n: did you have the chance to review the PR?
 5419:03 <gr0kchain> n
 5519:03 <raj_> y
 5619:03 <fjahr> y
 5719:04 <lightlike> y
 5819:04 <amiti> I started reviewing but am not done
 5919:04 <andrewtoth> n
 6019:04 <pinheadmz> yeah
 6119:04 <jnewbery> y
 6219:05 <jonatack> Great. raj_ fjahr lightlike pinheadmz: Concept ACK, approach ACK, ACK, or NACK?
 6319:05 <pinheadmz> "limited knowledge ACK" haha
 6419:05 <jonatack> (I think jnewbery is an ACK)
 6519:05 <pinheadmz> I dont know if this breaks anythign
 6619:06 <jonatack> amiti: (initial thoughts yay or nay?)
 6719:06 <raj_> Concept ACK. Ran ubit and functional test, standard tests passing. Havent tested manually.
 6819:06 <fjahr> still need to think about it a little more, 90% there to ACK
 6919:06 <jonatack> Would anyone like to describe what these mempool notifications do?
 7019: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.
 7119:07 <lightlike> concept ack for me, not 100% sure about the subtle differences to the new handling of conflicted txs
 7219:07 <fjahr> The wallet needs information about its txs so it needs to be informed when the mempool changes
 7319:08 <jonatack> (anyone want to describe why?)
 7419:08 <raj_> They notify tx entry/removal events in mempool to subscribing instances.
 7519: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.
 7619:09 <andrewtoth> I believe the description becomes part of the commit description, so any upstream projects will notify the tagged users. Is that right?
 7719: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)
 7819:09 <andrewtoth> *downstream
 7919:10 <jonatack> or at least go through the existing tests
 8019:10 <fjahr> andrewtoth: +1
 8119:10 <jonatack> existing test files: git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*'
 8219:10 <jonatack> andrewtoth: right, anyone tagged with get a bunch of notifs
 8319:10 <jonatack> will*
 8419:11 <jonatack> recently iiuc the merge script was updated to remove the @-tags
 8519:11 <pinheadmz> this PR removes code that is essentially dead since mempool notifiactions were intorduced
 8619:11 <jonatack> but it's a good practice to not tag in the PR description and in the commits
 8719:11 <pinheadmz> is there a rason why it wasnt pulled out by that PR?
 8819:12 <jonatack> pinheadmz: i'm guessing here but can imagine at least two good reasons:
 8919:12 <jnewbery> pinheadmz: which PR?
 9019:13 <jonatack> - smaller, more focused PRs are generally easier to review and have merged
 9119: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
 9219:13 <pinheadmz> jnewbery: 9371 for example
 9319:13 <jonatack> - anything involving validation needs a high degree of review and careful scrutiny
 9419:13 <pinheadmz> jonatack: makes sense
 9519:14 <jonatack> as maintaining consensus is the overarching mission of bitcoin core
 9619:14 <pinheadmz> it just means this unused vector was still getting written to memory for about 3 years of release :-) (IIUC)
 9719:14 <jnewbery> You're talking about the NotifyEntryAdded and NotifyEntryRemoved signals?
 9819: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
 9919:15 <pinheadmz> but i may be missing other uses for it that kept it around until now
10019:15 <jnewbery> NotifyEntryRemoved is used to add conflicted txs to the conflictedTxs vector in PerBlockConnectTrace
10119:16 <jonatack> kanzure: thank you, the link had an extra / ... it's https://jonatack.github.io/articles (and WIP)
10219:16 <jnewbery> that's changed by this PR17477
10319:17 <pinheadmz> jnewbery: oh i see! so that is the last reference to the conflicted Txs vector
10419:17 <jonatack> right, this code isn't dead per se
10519:17 <pinheadmz> ty.
10619: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
10719:18 <jonatack> I liked how jnewbery structured the commits with step-by-step sequentially logical changes that made it much easier to review
10819:19 <jonatack> The first commit is the most impactful one
10919: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?
11019:19 <jonatack> followed by almost mechanical changes that lead to the last commit removing the callbacks
11119:19 <raj_> jonatack: +1, really loved the organization of this commit. made things much easier.
11219:20 <jonatack> yes!
11319:20 <pinheadmz> yes +1 for long commit messages and including the module name in [brackets]
11419:20 <jonatack> imagine if it had been a single commit, and pinheadmz yes +1 on the messages
11519:21 <jonatack> the commit messages are so clear that i didn't see what else to add to describing them in the notes
11619:21 <jonatack> Anyone: What are conflicted transactions?
11719:22 <pinheadmz> you can tell when an author spends a lot of time reviewing :-) "what would make this easier for everyone else to review?"
11819: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?
11919: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
12019:22 <jonatack> pinheadmz: I agree, this is an underrated quality
12119:22 <pinheadmz> but more interesting cases inolve premature coinbase spends when a block is UN condifmred
12219:23 <pinheadmz> and relative timelocks
12319: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
12419:24 <michaelfolkson> Call site?
12519:24 <fjahr> pinheadmz: "when a block is UN condifmred" => I don't understand
12619: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
12719:24 <raj_> jonatack: nice trick, thnaks.
12819:25 <jonatack> Is a double spend and a conflicted transaction the same thing?
12919:25 <jnewbery> michaelfolkson: where the function is called from somewhere else
13019:25 <michaelfolkson> fjahr Unconfirmed
13119:26 <jonatack> For example, to try to see what code covers conflicted transactions and double spending:
13219:26 <jonatack> git grep "double spend\|double-spend\|conflicted" -- 'src/' -- :^'*/qt/*'
13319: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
13419:27 <jonatack> and to try to see the Mempool policy on double-spending, try: git grep -B4 -A3 "transaction that double-spends"
13519:28 <fjahr> Ok, I have never thought of that as an unconfirmed block, thanks!
13619: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).
13719: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
13819:28 <pinheadmz> jnewbery: "disconnected" ?
13919:28 <jonatack> lightlike: thanks, where exactly?
14019:29 <jnewbery> in your example, the transaction can be included later because it's still valid and its inputs haven't been spent
14119:29 <fjahr> I would have said "re-orged out block" or something I guess
14219: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
14319: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
14419:30 <lightlike> jonatack: line 2499, https://github.com/bitcoin/bitcoin/pull/17477/files#diff-24efdb00bfbe56b140fb006b562cc70bL2513 if that link works.
14519:30 <jnewbery> pinheadmz: no, because the tx isn't in the mempool. The signal is fired when a tx is removed from the mempool
14619:30 <jnewbery> pinheadmz: oh right. Sorry, I misunderstood. Yes, the signal gets fired
14719:31 <jnewbery> I think the reason given is REORG rather than CONFLICTED in this case
14819:31 <pinheadmz> jnewbery: ah yea
14919: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)
15019:32 <jnewbery> pinheadmz: here: https://github.com/bitcoin/bitcoin/blob/631df3ee87ec93e1fc748715671cdb5cff7308e6/src/validation.cpp#L371
15119: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
15219:34 <jnewbery> (described here: https://github.com/bitcoin/bitcoin/blob/631df3ee87ec93e1fc748715671cdb5cff7308e6/src/txmempool.h#L766)
15319:35 <pinheadmz> jnewbery: very cool ty
15419:35 <jonatack> txmempool.h is well-commented, kudos to the contributors who did that
15519:36 <michaelfolkson> git blame
15619: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?
15719:38 <jonatack> lightlike: the comment beginning with "// We always keep one extra block at the end of our list because"?
15819:41 <jnewbery> ok, maybe simpler question: what does the wallet mean by a conflicted tx? Why does a wallet care about tracking these?
15919:41 <lightlike> jonatack: yes, blocks are no longer "added after all the conflicted transactions have been filled in." after this PR.
16019:41 <pinheadmz> the wallet should notify the user if they are being double spent against
16119:41 <michaelfolkson> A conflict with a transaction in newly connected block versus another in the mempool?
16219:41 <raj_> conflicted tx: two tx trying to spend same inputs.
16319:42 <jonatack> jnewbery: in AddToWalletIfInvolvingMe?
16419: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?
16519:43 <raj_> i would guess B1 would be the conflicted tx here.
16619: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`
16719:44 <amiti> but in the mempool, you're not really storing a history of txns, so conflicts are amongst the current set
16819: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
16919: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
17019:45 <raj_> oh okay. so conflicted tx can be a full chain of such txs.
17119:45 <jonatack> lightlike: i see your point that it should be removed in 17477
17219: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
17319: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
17419:47 <jnewbery> what about MemPoolRemovalReason::CONFLICT?
17519:48 <jonatack> jnewbery: ah, in AvailableCoins
17619:48 <jonatack> // It's possible for these to be conflicted via ancestors which we may never be able to detect
17719:48 <jnewbery> jonatack: yes!
17819: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
17919: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?
18019:49 <jnewbery> (in practice the mempool descendant limit is 25, but that's still a long chain)
18119: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
18219:51 <jonatack> raj_: yes: git grep -A7 "enum class MemPoolRemovalReason"
18319: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
18419:52 <jnewbery> it's a little bit subtle
18519: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?
18619:53 <jonatack> jnewbery: sounds like more code docs on this that would show up in trivial git grepping might be sweet
18719: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
18819:55 <jonatack> all: note that this is touched on in the notes https://bitcoincore.reviews/17477 :)
18919:56 <raj_> any idea how big such tx chains usally be in practise?
19019:56 <jonatack> 3 minutes!
19119:56 <jonatack> jnewbery: great distinction to make, ty
19219:57 <jonatack> any last questions or comments?
19319: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?
19419:58 <jonatack> lightlike: not in my case
19519: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
19619:58 <jonatack> lightlike: running the whole suite or single files?
19719: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
19819:58 <fjahr> think it through yet.
19919:58 <lightlike> jonatack: the whole suite, but via test_bitcoin (not make check)
20019: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
20119:59 <fjahr> * that I could come up with concerning that window question
20220:00 <jonatack> fjahr: I think it would be very interesting to go through the current test coverage on these scenarii and add any missing
20320:01 <lightlike> jonatack: src/test/test_bitcoin - ran it like 10 times and got varying strange output in 50% of the runs
20420:01 <jonatack> tests that cover conflicted transactions and double spending: git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*'
20520:01 <jonatack> let's wrap up
20620:01 <jonatack> feel free to continue after!
20720:01 <jonatack> #action review PR 17477 on GitHub
20820:01 <fjahr> jonatack: yeah, but race conditions like this will always be somewhat flakey :/
20920:01 <jonatack> and the follow-up PR
21020:02 <raj_> thanks. Very nice pr.
21120:02 <andrewtoth> thanks!
21220:02 <lightlike> thanks jonatack and jnewbery!
21320:02 <jonatack> fjahr: sure. maybe worth trying to make it work and reliable though.
21420:02 <jonatack> thanks all for coming, thank you jnewbery for your insights
21520:02 <jonatack> #endmeeting
21620:02 <fjahr> thanks!
21720:03 <amiti> fjahr: do you know, what would happen if we tried to RBF but txn was already in a block
21820:03 <jonatack> if anyone would like to host a meeting, feel free to come forth
21920:04 <jonatack> for instance, michaelfolkson +1
22020:04 <michaelfolkson> One day ;)
22120:04 <jonatack> TBH, I learn waay more when hosting
22220: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?
22320: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
22420: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
22520:11 <amiti> michaelfolkson: I don't follow your question
22620: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
22720:14 <jonatack> to think through the cases
22820:20 <michaelfolkson> amiti: It's ok. I don't think I understood your question :)