#10823 Allow all mempool txs to be replaced after a configurable timeout (default 6h) (mempool, policy)

https://github.com/bitcoin/bitcoin/10823

Host: jnewbery

Meeting Log

13:00 <@jnewbery> Hi all! Welcome to episode two of the Bitcoin Core PR Review club.
13:00 <@jnewbery> This week, we're talking about PR10823 - "Allow all mempool txs to be replaced after a configurable timeout (default 6h) #10823".
13:00 < merehap> Hi!
13:00 < schmidty> Heyo
13:00 < fanquake> Hi
13:00 < jonatack> Hi
13:00 < dmkathayat> Hi
13:00 < ccdle12> hello
13:01 < amiti> hi!
13:01 < rafeeki> v excited
13:01 < ariard> hi
13:01 < emzy> Hi
13:01 <@jnewbery> I think greenaddress does a really good job of describing the change in the PR post. I'll highlight a few points about the PR and then lets go to questions.
13:01 <@jnewbery> 1. "This PR affects policy/relay only." - all transaction replacement policies are policy-only. They're not consensus. That's to say that The BIP125 replacement conditions are only applied when a node decides which transactions to accept to the mempool and relay on to peers.
13:02 < sosthene> Hi
13:03 <@jnewbery> for example, the nSequence signaling for opt-in to BIP125 replacement has no consensus meaning.
13:03 <@jnewbery> Individual nodes are free to choose their own policy rules, ie which txs to accept and relay. However, it's very useful for wallets to know what other nodes in the network are going to do with transactions, so something like BIP125 is useful so wallets know how to make transactions replaceable.
13:04 <@jnewbery> This PR would change the default policy in Bitcoin Core to accept/relay replacement transactions when the original transaction is older than some configurable time
13:04 <@jnewbery> the replacement tx would still need to meet the other BIP125 conditions
13:05 <@jnewbery> 2. There are two changes in this PR - the first is to the mempool policy, and the second is to the wallet, allowing it to bump txs that don't signal BIP125 if they're older than the configurable time
13:05 <@jnewbery> That's all I wanted to say about the PR
13:06 <@jnewbery> any thoughts/questions? Don't be shy!
13:07 < michaelfolkson> Ok. So I'll start with a basic question. Why is this an improvement on existing RBF?
13:07 < ariard> "A number of miners will mine transactions regardless of opt-in flags" does this mean miners are overriding policy rules with custom forks of core ?
13:08 <@jnewbery> The motivation is that a user might send a transactions without BIP125 signalling and it become stuck for a long time. They want to bump the fee on the transaction but under current policy have no way to do that
13:08 < harding> ariard: yes.  Miners with slighly modified versions of Bitcoin Core are believed to be pretty common.
13:08 <@jnewbery> ariard: I believe so (although it's not possible to know for sure what miners are running)
13:09 < rafeeki> so the issue with current RBF is wallets might not use BIP125 signalling by default, unbeknownst to the user?
13:09 <@jnewbery> rafeeki: I think that's possible, yes
13:10 <@jnewbery> and by the time they realise they need to bump the fee, it's too late
13:10 < ariard> okay so it may demean user experience with RBF who are trusting than the bumped tx will be enforced, maybe it's an issue for wallet developpers
13:10 < michaelfolkson> The discussion on Twitter seemed to suggest it was controversial. Can you outline the reasons against making this change?
13:11 < jonatack> Do you agree with the last comment by gmax that the change has become sort of pointless, or is it still a definite concept ack like last November?
13:11 < michaelfolkson> Other than demeaning user experience
13:11 < dmkathayat> What are zeroconf transactions and how are they useful?
13:11 < harding> michaelfolkson: do you have a handy link to the discussion?  (I didn't see that.)
13:11 <@jnewbery> michaelfolkson: merchants or other users accepting zero-conf transactions might feel that this reduces their security
13:11 < Lightlike> do most wallets signal BIP125 (even if they don't implement RBF themselves) or is it opt-in such that basically those who implement it signal it?
13:11 < michaelfolkson> Not really discussion but initial responses. I'll link to a couple here.
13:12 < jonatack> gmax march 11: "Subsequently to my earlier comments I now think this is kinda pointless: Testing without RBF set gave me 100% confirmation or replacement rate for very low fee transactions within 20 blocks without the low fee txn rising to being minable by feerate presumably due to restarting nodes, and miners that replace anyways. Is this just motivated by either speculation on actual behaviour without measuring it
13:12 < michaelfolkson> https://twitter.com/ziggamon/status/1125842285151817728
13:12 <@jnewbery> petertodd would argue that those transactions don't have any security anyway: https://github.com/bitcoin/bitcoin/pull/10823#issuecomment-472242603
13:12 < michaelfolkson> https://twitter.com/BobMcElrath/status/1125851888337997824
13:13 <@jnewbery> dmkathayat: a zero-conf transaction is any transaction that has not yet been confirmed in a block. They're not yet covered by any proof-of-work, so there's a risk that a conflicting transaction gets confirmed instead of it
13:13 <@jnewbery> Lightlike: currently about 5% of transactions signal RBF: https://dashboard.bitcoinops.org/d/ZsCio4Dmz/rbf-signalling?orgId=1
13:14 < emzy> How would you best test the pr? With testnet3 or with a network of regtest nodes?
13:14 <@jnewbery> emzy: generally, regtest is the way to locally test PRs
13:14 <@jnewbery> I think testnet is more useful for service providers doing integration testing, etc
13:14 < emzy> But you need more then one node, right?
13:15 < harding> michaelfolkson: thanks.
13:15 <@jnewbery> emzy: not really. The changes here are confined to what txs the mempool will accept, so it's possible to test with a single instance
13:15 < ajonas> It seems that the author is alone in advocating for the 6hr timeout versus most of the reviewers agreeing on 72 hours. What is the threshold for merging something that is mildly controversial like that?
13:16 < ariard> emzy: if needed, functional tests framework let you easily spoof nodes
13:16 <@jnewbery> If you do need more than one node, our test framework spins up a network of nodes-under-test. Take a look at setup_network() in test_framework.py
13:16 <@jnewbery> ariard: to be clear, it's running actual full nodes, so I don't think spoof is the right word
13:16 < ccdle12> is anyone aware of the extent of zero conf crime/fraud?
13:16 < emzy> Will do, tnx ajonas. jnewbery
13:17 < harding> emzy: to expand on jnewbery's remark, the sendrawtransaction RPC (and the testmempoolaccept RPC) will do the same checks on transactions you pass them as the node would on transactions received over the netwrok, so you can generate replacements and test them with those RPCs.
13:18 < harding> (I would personally probably spin up at least two nodes, though, as I wouldn't feel comfortable saying a particularl relay behavior works without actually testing relay.)
13:18 <@jnewbery> jonatack: I'm still a concept ACK, because I think it makes expectations clearer. If a transactions has been in the mempool for 72 hours (or even just 6 hours) and it hasn't been confirmed, a malicious actor will be able to get it double spent
13:18 < emzy> harding: that was my thinking, too
13:18 < sosthene> Why RBF usage isn't more widespread by now? Is there some trade-off using it?
13:19 < michaelfolkson> My understanding with zero conf transactions was that the introduction of RBF was a step away from relying on them regardless of this PR. Am I missing some subtlety here?
13:19 < ajonas> ccdle12: there is some coverage here on how ATM operators are approaching it -> https://coinatmradar.com/blog/support-zero-confirmation-transactions-at-bitcoin-atm/
13:20 < harding> sosthene: good question!  The ultimate answer is that you need to ask the authors of the wallets that haven't enabled it.  :-)  However, my guess is that few wallets have implemented fee bumping, which does require some work and has some gotchas, and so they haven't bother to even try signaling BIP125.
13:20 < ariard> sosthene: UX isn't that easy https://bitcoinops.org/en/rbf-in-the-wild/
13:20 <@jnewbery> sosthene: the consideration is on how the receiving party will treat that transaction. Merchants who otherwise accept zero-conf txs may not accept those txs if they have RBF signalled.
13:20 <@jnewbery> of course, once the tx is confirmed, there's no difference
13:20 < harding> sosthene: see schmidty's report here for some of the gotchas: https://bitcoinops.org/en/rbf-in-the-wild/
13:21 < harding> (Oh, just saw ariard posted the link.  Sorry for the duplicate.)
13:21 < sosthene> Ok thanks will have a look at this!
13:22 <@jnewbery> it's worth mentioning that this change makes much more sense in a world with always full blocks. If the mempool ever empties out then even txs with minimal feerates will be included
13:23 <@jnewbery> You can see here: https://jochen-hoenicke.de/queue/#0,2w that the mempool is still clearing out fairly regularly
13:24 < michaelfolkson> The fact that only 5% of transactions signal RBF implies that I was wrong to assume that zero conf transactions had been ditched as something to be relied upon. Whatever the advice was from some core devs
13:24 < harding> If I may, I think that line of enquiry raises a useful question when looking at PRs: how useful would this actually be?  If wallets aren't signaling BIP125 now, it probably means they don't have the tools to make use of RBF, which means that---even if transactions are technically replacable in the mempool---they won't be replaceable through the wallet that created them.  How many users
13:24 < harding> are going to export their private keys from one wallet (dangerous), import them into another, and then RBF?
13:24 < MaxHillebrand> harding: for example, Wasabi has not yet implemented RBF, mainly because of privacy concerns [RBF signaling decreases anonset and usage reveals change output], as well as development resources and UX clutter...
13:24 < fanquake> ajonas if there's no/minimal consensus the PR will likely just stagnate and discussion becomes sporadic/dies out (as it has done here). Any "threshold" for merging is usually hard to define.
13:25 <@jnewbery> harding: that's a good way of thinking about it
13:25 < rafeeki> harding: good point. Are there other non-RBF ways of dealing with "stuck" txs?
13:25 <@jnewbery> rafeeki: the other technique is child-pays-for-parent
13:25 < harding> rafeeki: child-pays-for-parent which can always be done by the receiver and can often be done by the spender (whenever there's a change output).
13:26 <@jnewbery> michaelfolkson: I'd say zero-conf transactions should never be something to be 'relied on'. Until a transaction is confirmed, it can always be double-spent
13:26 < MaxHillebrand> and one benefit is that CPFP does not chang the tx id of the parent tx.
13:27 < michaelfolkson> jfnewbery: But many are ignoring this advice e.g. Bitrefill
13:28 < harding> MaxHillebrand: True.  But can you now name one disadvantage of CPFP versus RBF?
13:28 < schmidty> harding: I think that by having such a feature in Core, if it is demanded by users, will 1. incentivize users to use Core as a wallet and thus 2. incentivize other wallets/services to implement to keep up
13:28 < harding> schmidty: what feature?
13:28 < harding> schmidty: Bitcoin Core already defaults to sending BIP125 from the GUI (and, IMO, it should default from the CLI/RPC too).
13:29 <@jnewbery> It's obviously up to the merchant whether and under what circumstances they accept zero-conf. Bitrefill have obviously decided that they can accept zero-conf up to a certain amount if the tx meets some requirements.
13:29 < b10cm> harding RPF is cheaper for the user than CPFP (using a change output)
13:29 < MaxHillebrand> harding: well, if you don't necessarily need to spend again, and you only generate a child 1 input - 1 output tx to bump overall fee / byte, then you add "unnecessary" tx and bytes to the chain. [well, at least the utxo set says the same in this case].
13:29 < schmidty> This "safety net" feature. Which I suppose would only apply if someone mistakenly does not set bip125.
13:29 < Lightlike> does CPFP require signaling too in the parent tx, or is it always possible after the fact?
13:29 <@jnewbery> (My issue is with the words 'relying on')
13:30 < harding> b10cm, MaxHillebrand: correct.  :-)
13:30 <@jnewbery> harding: I think the default in Bitcoin Core is to *not* signal BIP125
13:30 < MaxHillebrand> And so far, Bitrefill has not been double spend against...
13:30 < harding> Lightlike: CPFP is always possible after the fact, although there are some limits to the maximum number of children in mempool (policy limits) for anti-DoS reasons.
13:30 < michaelfolkson> <harding>: RBF is initiated by the sender but CPFP is initiated by the intended recipient. That's the key consideration in comparing them right?
13:31 <@jnewbery> oh, from the GUI. Maybe the default is different there
13:31 < harding> jnewbery: yep.
13:31 < MaxHillebrand> harding: where actually is that limit? I am thinking that we pumped against the limit of 20 child coin join transactions with Wasabi...?
13:32 < harding> MaxHillebrand: bitcoind -help-debug | grep -A3 -- -limit
13:33 < amiti> michaelfolkson: CPFP can be initiated by the sender using a change output
13:33 <@jnewbery> MaxHillebrand: the limit is in https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/validation.h#L57
13:34 < MaxHillebrand> harding: thank you!!   -limitancestorcount=<n>        Do not accept transactions if number of in-mempool ancestors is <n> or        more (default: 25)    -limitancestorsize=<n>        Do not accept transactions whose size with all in-mempool ancestors        exceeds <n> kilobytes (default: 101)    -limitdescendantcount=<n>        Do not accept transactions if any ancestor would have <n> or more        in-mempoo
13:34 < jonatack> with all those concept acks, seems the pr would have had more success if the author had followed-up more quickly on it to maintain momentum
13:34 <@jnewbery> Note that those ancestor/descendant limits are for your mempool policy
13:35 <@jnewbery> If you want to successfully CPFP a transaction, you need to have an idea of other nodes' mempool policies to know whether your tx will propagate
13:35 < michaelfolkson> <amiti>: Ah yes, thanks. Assuming the change is material amount and not close to dust limit
13:35 < MaxHillebrand> jnewberry: that is a good point. So miners could still include chains of >25 chilren in a block?
13:35 < b10cm> Yes, if they up the default
13:36 < harding> MaxHillebrand: yes.
13:36 < harding> MaxHillebrand: more importantly, I think jnewbery is trying to communicate that increasing the limit on your node doesn't mean your transactions will propagate to other nodes that haven't increased their limits.
13:36 < schmidty> What is the approach, if any, for gauging interest in a feature/PR from non developer communities (businesses, users, etc)?
13:36 <@jnewbery> jonatack: good observation. Yes, success in getting PRs merged is usually down to keeping momentum up and encourging other contributors to review the PR
13:37 < merehap> jonatack: Since it's just a policy change, they probably just implemented in their fork of Bitcoin core, and lost interest in driving the consensus process after a year or so.
13:37 <@jnewbery> MaxHillebrand: absolutely. RBF/CPFP are all *policy* settings. They have no effect on consensus
13:38 < MaxHillebrand> thanks harding & jnewberry for clarifying
13:38 < instagibbs> harding, back to your thought experiment about the type of users that would even need the replace after N hours, indeed, I think it's partially to work around the politics of making Core's wallet walletrbf=1 by default...
13:38 <@jnewbery> merehap: I don't think that's the case here. 'just' policy downplays the importance of it. It's important for wallet makers to have an idea of what the default policy is on the network
13:39 < fanquake> schmidty A major part (more recently) of it would probably be whatever bitcoin optech is doing at the moment.
13:39 < merehap> jnewbery: Fair enough.
13:39 < jonatack> merehap: good point. i suppose anyone motivated could ping the author and propose to take it over, but seems rare in practice. i've only seen jnewbery do that IIRC.
13:39 < harding> schmidty: there's no real process for that, except individual devs going out and trying to contact people.  The normal process in open source projects is that we try to get companies to hire devs involved in the project so that they can communicate to their bosses about any changes that might affect the business.
13:39 <@jnewbery> Put another way: It makes ~zero difference to tx propagation if I change my node's policy and everyone else's remains the same
13:40 < MaxHillebrand> schmidty: I estimate the "issue" of stuck transactions with low fees are existent and painfull for both merchants and users. I remember buying a cold card back when fees were so high, I did not signal RBF, and waited over one month till confirmation... [Was actually great for me, since price wen't down, so I got my hww for cheaper with this free short future :D]
13:40 < harding> schmidty: But that hasn't happened as much as I think some of us would've liked, so (as fanquake says) Optech is trying to bridge that gap to a certain extent.
13:41 < harding> instagibbs: interesting!
13:41 < instagibbs> without trying to derail the discussion further: Any other wallets other than Core that support fee bumping but don't have it on by default?
13:42 < MaxHillebrand> instagibbs: electrum
13:42 < fanquake> harding,schmidty: there's definitely space on the repos for more businesses/CTOs etc to come and join development discussion, hopefully that is the case in future.
13:43 <@jnewbery> For those who want to read up more on fee-bumping (RBF and CPFP), we're working on a chapter in the Optech scaling book here: https://github.com/bitcoinops/scaling-book/blob/master/1.fee_bumping/fee_bumping.md and here: https://github.com/bitcoinops/scaling-book/pull/5
13:43 <@jnewbery> Did anyone have any questions about the code changes in the PR?
13:43 < Lightlike> what do you think is the likely outcome for this pr? i saw that it was milestoned for 0.19, does that mean it will probably be accepted (possibly with >6h)? Or ist still completely unclear what will happen?
13:43 < instagibbs> MaxHillebrand, ah good to know, so that's a big one
13:44 <@jnewbery> It touches a few parts of the codebase, so I thought there might be some interesting discussion about what exactly is being changed
13:44 < instagibbs> Lightlike, anyone can ask for a milestone pretty much
13:44 < schmidty> instagibbs: electrum and if I recall from my testing Samourai too
13:44 <@jnewbery> Lighlike: it depends on how the other contributors review it. Having a milestone isn't any guarantee that it'll be merged
13:45 < michaelfolkson> <@jnewbery>: Perhaps you could explain in greater detail why it touches different parts of the codebase? :)
13:45 < merehap> jnewbery: But wouldn't greenwallet's full node be relaying their customers' SPV requests such that it does matter what their node's policy is (assuming they expose the new functionality in their SPV wallets)? Assuming other nodes, like miners will already sometimes relay the replacement transaction. I could be misunderstanding of course.
13:46 < jonatack> good links and kudos on the optech book content BTW, excellent
13:46 <@jnewbery> michaelfolkson: sure. Like I said at the top, this PR actually makes two changes. One is on mempool acceptance policy, and the other is on wallet feebumping functionality.
13:46 < michaelfolkson> Looking through it, it makes sense that it does but I wouldn't have guessed initially
13:46 <@jnewbery> Those two components have been better separated since this PR was opened, so I think the code could be changed a bit to reflect that
13:47 <@jnewbery> Like here for example: https://github.com/bitcoin/bitcoin/pull/10823/files#diff-c865a8939105e6350a50af02766291b7R520
13:47 < MaxHillebrand> michaelfolkson, absolutely agree!! optech is a great achievement already, thanks harding & jnewbery :)
13:47 <@jnewbery> That's supposedly a wallet command-line argument, but it's set in init.cpp, which is part of the node
13:47 < harding> merehap: I think greenwallet already signals BIP125, so implenting this on the node they use to relay transactions for their wallet users wouldn't benefit those users.
13:48 <@jnewbery> preferably, command-line options should just be for wallet behaviour or node behaviour, not shared like it is here
13:48 <@jnewbery> (like I said, this PR was opened before that split was well defined, so that's no criticism of the author)
13:49 <@jnewbery> This line https://github.com/bitcoin/bitcoin/pull/10823/files#diff-a6585739217ca647d3f10c40cccff7f1R202 in interfaces/chain.cpp is where the wallet accesses the node functionality and state
13:49 < merehap> harding: Makes sense. Seems like there could be a use case for greenwallet having optional RBF then allowing non-RBF replacement after 72 hours or something.
13:50 < merehap> As far as the actual code, what kinds of test cases are missing here? Do those two test cases actually cover everything?
13:51 < harding> merehap: ah, indeed that could be nice, but it would require they peer with other nodes willing to relay those replacements (and eventually someone peer with miners willing to mine the replacements).
13:51 <@jnewbery> merehap: I think the testcases are decent. What would you like to see added?
13:52 < ccdle12> IsRBFOptIn gets the transaction and checks if timeout is enabled and if RBF is expired?
13:52 < merehap> harding: And there isn't a critical mass of miners who currently have tweaks that support that?
13:52 <@jnewbery> (that was a really good question, btw!)
13:53 < merehap> jnewbery: I'm not sure I have anything in particular just from my 30 minutes of understanding the problem. But greenwallet did ask reviewers if there was any test cases that they are missing but I didn't see a reply there.
13:54 <@jnewbery> Anyone else have any suggestions for what might be missing from the tests?
13:54 < merehap> Their comment was "More feedback welcome. I think it would be good to have more tests (ideas?)". Will spend a few more minutes to see if anything comes to me.
13:55 < instagibbs> thinking of tests for open PRs that need them is a great way to contribute as well
13:55 <@jnewbery> The testcase here: https://github.com/bitcoin/bitcoin/pull/10823/files#diff-99f59f30717393a95407e30e3451b129R150 (test_nonrbf_bumpfee_fails_then_succeeds()) is testing that the wallet rejects a bumpfee on the transaction and then accepts the bumpfee
13:56 <@jnewbery> So the replacement transaction being rejected by the mempool isn't tested (the wallet rejects the feebump before constructing the replacement transaction)
13:57 <@jnewbery> Another test could be submitting the replacement transaction over P2P and checking that it's rejected, then moving time forward and checking that it's accepted
13:58 <@jnewbery> ok, it's about time to wrap up. Does anyone have feedback about the format of this meeting, or any requests for PRs to review next time?
13:59 < jonatack> updating the tests or the code would be a great exercise to dig in with the info that's been shared here
13:59 < jonatack> great stuff
13:59 <@jnewbery> fanquake has written up some review notes for another PR here: https://gist.github.com/fanquake/85e7506fdb70acf477d6b81bebe3aba1 . Take a look at that if you're looking for another PR to review.
13:59 < harding> Something that may not be obvious to readers of that test is that setmocktime is a "hidden" RPC that's not displayed in the help overview (you can still request help on it specifically using: bitcoin-cli help setmocktime).  I think this will give you a list of all hidden RPCs that you can use in tests: git grep '{ "hidden'
13:59 < ccdle12> this was awesome, only feedback is to please keep doing this every week :)
13:59 < michaelfolkson> Yeah agreed, I love it
14:00 <@jnewbery> ok, thanks all. See you next week!
14:00 < emzy> tnx to all!
14:00 < harding> Thanks!
14:00 < Lightlike> thank you!
14:01 < jonatack> Thanks!
14:01 < merehap> Thanks!
14:01 < b10c> Thanks!
14:01 <@jnewbery> info is here: https://gist.github.com/jnewbery/6e2797a6f484de59aefc849a6b184008 . The current plan is to review #14856 next week, but I might update that so keep an eye here and on the gist.