nVersion=3 and Package RBF (tx fees and policy)

https://github.com/bitcoin/bitcoin/pull/25038

Host: glozow  -  PR author: glozow

The PR branch HEAD was c27864f at the time of this review club meeting.

Notes

See notes from the first review club.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

Concept

  1. If we were running a business that accepted unconfirmed transactions as payments, what changes should we make so that our wallet correctly tracks v3 transactions?

  2. Who might be interested in using v3 transactions? Who probably wouldn’t be interested?

  3. If there are some wallets/users/applications that use v3 while others don’t, is there a privacy issue (i.e. people can fingerprint which wallet likely created a transaction by looking at its version)?

  4. After the addition of v3, all non-v3 transactions are still subject to the existing limitations and pinning attacks. Wouldn’t it be safer to enforce these rules for all transactions on the network?

  5. V3 limits each transaction to 1 parent or 1 child (2 generations only), and perhaps that seems restrictive. Let’s consider an alternative set of rules that allows each transaction to have 2 parents or 2 children (a transaction may have up to 2 children or up to 2 parents. They cannot have both an unconfirmed parent and an unconfirmed child, so 2 generations only) - what would be the maximum cluster size?

  6. Why not limit the size of all v3 transactions to 1,000vB or something similarly small (Hint: what property of LN channels depends on standard transaction weight)?

Implementation

  1. Why does ApplyV3Rules check whether the transaction conflicts with any of its ancestors’ children? What happens if you remove this check?

  2. V3 ancestor/descendant limits are stricter than the default mempool limits. Do we need to call both ApplyV3Rules (which enforces v3 ancestor/descendant limits) and CalculateMemPoolAncestors() (which enforces ancestor/descendant limits)? Why or why not?

  3. V3 rules are added to the filtering function passed to removeForReorg() in Chainstate::MaybeUpdateMempoolForReorg. Why do we need to check v3 rules in a reorg?

  4. Is CTxMemPool::removeForReorg() the appropriate place for this check? Can you think of an alternative place to add this check?

  5. How does CheckMinerScores determine whether one transaction or package is more incentive compatible to mine than another? Is this function 100% accurate for v3 transactions? What about for non-v3 transactions?

  6. Why is it necessary to evict all transactions below minimum relay feerate in TrimToSize()? (Hint: what does this test check)?

  7. Why must PackageMempoolChecks never be called with only 1 transaction? (Hint: an explanation can be found in this commit).

Rabbit Holes

  1. V3 restrictions suck! Why can’t we just replace RBF Rules 3 and 4 with a better rule based on feerate? For example, “replacement tx must have a feerate 2x higher than each of the transactions it’s replacing.”

  2. Why not allow packages with multiple parents and 1 child in v3?

  3. Theoretically, if the mempool is not full, the maximum number of transactions that may be removed in a replacement is 100 (due to Rule 5). After this commit, what is the new limit?

Also feel free to bring your own questions.

Meeting Log

  117:00 <glozow> #startmeeting
  217:00 <emzy> hi
  317:00 <svav> Hi
  417:00 <LarryRuane> hi
  517:00 <theStack> hi
  617:01 <codo> hi
  717:01 <pakaro> hi
  817:01 <michaelfolkson2> hi
  917:01 <pakaro> michaelfolkson2 what did you do with michaelfolkson!?
 1017:01 <glozow> Welcome to PR review club everyone! feel free to say hi so we know you're here
 1117:01 <ranemirus> hi
 1217:01 <glozow> Today is part 2 of v3 policy review, notes here: https://bitcoincore.reviews/25038
 1317:02 <michaelfolkson> pakaro: Just an alter ego
 1417:02 <instagibbs> hi
 1517:02 <glozow> We have some questions prepared at https://bitcoincore.reviews/25038-2 but feel free to jump in whenever you'd like
 1617:02 <glozow> has everyone had a chance to look at the PR and/or the notes?
 1717:03 <theStack> y
 1817:03 <LarryRuane> y
 1917:03 <instagibbs> y
 2017:03 <abubakar_> hi
 2117:03 <michaelfolkson> y
 2217:04 <svav> y
 2317:04 <glozow> theStack: LarryRuane: instagibbs: michaelfolkson: abubakar_: svav: amazing! Would you like to share what your review approach was?
 2417:05 <LarryRuane> there are many commits, because there is some RBF stuff in the PR too, so first step for me is try to identify which commits are relevant for this review club
 2517:06 <abubakar_> concept review, and just an overview of some commits.
 2617:07 <theStack> my review approach was mainly driven by the review club notes, i.e. the implementation questions
 2717:07 <LarryRuane> (I know you're going to separate the commits out into separate PRs)
 2817:07 <glozow> LarryRuane: I think I put this in the notes as well but fyi the v3 commits are 44fcc4d…3db3e45 (https://github.com/bitcoin-core-review-club/bitcoin/commit/44fcc4d3912e21b055c377549c8882c67dddf278)
 2917:07 <LarryRuane> of the 24 commits, seems like the relevant ones are sort of in the middle ... oh thank you!
 3017:09 <glozow> first question for today: If we were running a business that accepted unconfirmed transactions as payments, what changes should we make so that our wallet correctly tracks v3 transactions?
 3117:10 <LarryRuane> seems too simple to say, but just inspect the nVersion number to see if it's 3 (or greater)?
 3217:10 <instagibbs> from an unconfirmed transaction perspective, your node needs to be updated to see them in the mempool
 3317:10 <abubakar_> Update our wallet bitcoin core node to the version that has v3 commits, and our mempool will start tracking v3 transaction which we can add to our wallet.
 3417:10 <michaelfolkson> Treat them like non v3 transactions that are signaling RBF?
 3517:10 <instagibbs> for confirmed, nothing needsto be done :)
 3617:11 <Murch> v3 transactions are non-standard, so upgrade our node? ^^
 3717:12 <michaelfolkson> Accepting unconfirmed isn't recommended (I guess it is worth stating the obvious) and especially if they are signaling RBF or are v3 that are also signaling RBF inherently
 3817:12 <glozow> LarryRuane: instagibbs: abubakar_: michaelfolkson: Murch: all good answers yep. upgrading so you understand v3 policy. understanding that v3 signals replaceability is pretty important
 3917:12 <glozow> Who might be interested in using v3 transactions? Who probably wouldn’t be interested?
 4017:13 <abubakar_> Lightning network users might be interested
 4117:13 <theStack> assuming that "who" is not only refering to individuals, i'd say any L2 protocol that currently suffers from pinning attacks
 4217:13 <LarryRuane> kind of obvious again, but lightning wallets (and related infrastructure)?
 4317:13 <instagibbs> batched payment processors
 4417:13 <abubakar_> eople who need more than one transaction cluster can’t think of any lol.
 4517:14 <abubakar_> might not be interested
 4617:14 <Murch> Since they’re explicitly designed to limit the pinning attack surface, they would mostly be used by multiparty transactions, e.g. lightning channel commitment transactions.
 4717:14 <instagibbs> since clients give you addresses, you can't force them to use specific kinds of scripts, and you may want to rbf your batch payment
 4817:14 <Murch> Merchants would probably prefer to treat them like other RBF transactions and wait for confirmation.
 4917:15 <instagibbs> it's much less interesting for things like donation transactions, or other low priority txns
 5017:16 <glozow> yep. and I think if you only spend confirmed utxos, there's no reason not to use v3
 5117:17 <instagibbs> it doesn't hurt in those cases either to be fair :)
 5217:17 <michaelfolkson> So the expectation would be all transactions use v3 eventually? Unless you strongly don't want to signal RBF?
 5317:18 <instagibbs> michaelfolkson great question, what topology is useful that isnt supported?
 5417:18 <glozow> I guess... unless you have some compelling reason to chain n>2 unconfirmed transactions together, v3 is always better
 5517:18 <glozow> I think somebody pinged me about chained coinjoins
 5617:19 <instagibbs> batched CPFP
 5717:19 <michaelfolkson> instagibbs: You mean package RBF (yet)?
 5817:19 <instagibbs> many parents, one child. All parents are f.e. channel commitment txns, child is fee-bringing child
 5917:20 <michaelfolkson> Ah that isn't supported by anything including v3
 6017:20 <glozow> yep, no batched CPFP. so if you sent a bunch of transactions and want to batch them together, you can't do a cpfp. but hopefully if you made them all v3, you can just batch them together in an rbf
 6117:20 <glozow> and if you're receiving a bunch of payments across multiple transactions, you gotta bump them 1 by 1
 6217:21 <glozow> If there are some wallets/users/applications that use v3 while others don’t, is there a privacy issue (i.e. people can fingerprint which wallet likely created a transaction by looking at its version)? (There is no "correct" answer to this question)
 6317:22 <abubakar_> I think you can just tell this transaction is from a wallet/user that uses v3 transactions, but you can not say which user or wallet application.
 6417:22 <instagibbs> michaelfolkson non-v3 can do it(poorly, due to pinning vectors)
 6517:22 <pakaro> ln-settlement transactions are currently fingerprintable already as [2/2multisig]
 6617:22 <pakaro> but it does give more info to chainanalysis
 6717:23 <glozow> pakaro: yeah I think some cases are worse than others. We don't really need to worry about "unilateral LN channel closes are identifiable" because they already are
 6817:24 <michaelfolkson> abubakar_: This depends on how many wallet applications support it. If only one does you can tell. If lots do you can't. The bigger the crowd (of wallets) the harder it is to know which wallet
 6917:24 <instagibbs> batched payment processors are also quite fingerprintable due to size/utxo clustering
 7017:24 <theStack> wondering if some wallets in the future will use either v2 or v3 randomly (in cases where both work, obviously), to increase confusion
 7117:26 <LarryRuane> theStack: +1 that's a good idea
 7217:26 <glozow> yeah i mean the argument "when very few wallets use it, it's easy to tell which wallet created a tx" is pretty weak, because then we should never update anything
 7317:26 <instagibbs> or mimic the nversion of the depositing txn, hard to say imo
 7417:27 <michaelfolkson> glozow: +1
 7517:27 <pakaro> +1 thestack neat idea
 7617:28 <glozow> that is interesting!
 7717:28 <pakaro> thestack -> is parameter randomization used similarly in transactions today?
 7817:29 <glozow> I'm going to move on to implementation questions since we're halfway. Why does `ApplyV3Rules` check whether the transaction conflicts with any of its ancestors’ children?
 7917:29 <glozow> code here: https://github.com/bitcoin-core-review-club/bitcoin/commit/44fcc4d3912e21b055c377549c8882c67dddf278#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R69
 8017:32 <glozow> more specific link: https://github.com/bitcoin-core-review-club/bitcoin/commit/44fcc4d3912e21b055c377549c8882c67dddf278#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R93-R105
 8117:33 <theStack> pakaro: good question. some wallet fingerprinting possibilities and mitigations are listed here: https://en.bitcoin.it/wiki/Privacy#Wallet_fingerprinting (but i have no idea if this is really up-to-date...)
 8217:33 <theStack> ad Q1: this seems to be for the case that there is already a child of the parent in the mempool. we allow that if the child is replaced, IIUC
 8317:34 <codo> My guess: it checks if it conflicts, because it should conflict. To replace it.
 8417:35 <glozow> theStack: exactly. what happens if we don't have this check?
 8517:36 <glozow> if we don't have this check, you can't replace a v3 transaction that has an ancestor
 8617:36 <glozow> next question. V3 ancestor/descendant limits are stricter than the default mempool limits. Do we need to call both ApplyV3Rules (which enforces v3 ancestor/descendant limits) and CalculateMemPoolAncestors() (which enforces ancestor/descendant limits)? Why or why not?
 8717:37 <michaelfolkson> If it is a V3 transaction we don't need both because V3 is stricter?
 8817:38 <LarryRuane> michaelfolkson: that seems right to me
 8917:38 <glozow> hint: ancestor/descendant limits are configurable
 9017:38 <theStack> the default mempool limits can be changed (`-limit{ancestor,descendant}{size,count}`), so they could be even stricter than v3?
 9117:38 <LarryRuane> OH .. so need to check both!
 9217:38 <michaelfolkson> Ha ok
 9317:39 <LarryRuane> wow i wouldn't have thought of that, good catch, theStack:
 9417:39 <glozow> theStack: exactly. see this test case: https://github.com/bitcoin-core-review-club/bitcoin/commit/2b17bbb6723b66262948994b375403e22efe5942#diff-15a1888c9151fc1d182c23e34b71d691f70df448bceb9eb78c8296f18854b6a3R186-R213
 9517:40 <glozow> V3 rules are added to the filtering function passed to removeForReorg() in Chainstate::MaybeUpdateMempoolForReorg. Why do we need to check v3 rules in a reorg?
 9617:40 <glozow> talking about this code: https://github.com/bitcoin-core-review-club/bitcoin/commit/a74218d1571de5880ba7e0c168571f560d12d166#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R353-R358
 9717:40 <michaelfolkson> There was never the equivalent of ApplyV2Rules? It was always treated as if it could be configurable?
 9817:41 <LarryRuane> glozow: because a v3 tx may have been legal before the reorg, but now, it's no longer legal because it has more ancestors
 9917:41 <glozow> michaelfolkson: what do you mean by "equivalent?" like a filtering function in removeforreorg?
10017:42 <LarryRuane> (since reorg means throw some "mined" tx back into the mempool)
10117:42 <theStack> v3 rules are not consensus rules, i.e. after disconnecting a block we can't just unconditonally put txs in the mempool (v2 could spend v3 and vice-versa)
10217:43 <glozow> LarryRuane: theStack: yep exactly! test cases here https://github.com/bitcoin-core-review-club/bitcoin/commit/2b17bbb6723b66262948994b375403e22efe5942#diff-15a1888c9151fc1d182c23e34b71d691f70df448bceb9eb78c8296f18854b6a3R163-R182
10317:43 <pakaro> theStack as an extension, after any re-org, does that mean _all_ policy checks must be redone
10417:43 <glozow> are there any other ways we could implement this check?
10517:44 <glozow> pakaro: yes, almost all policy rules are applied to transactions entering the mempool from disconnected blocks.
10617:45 <glozow> e.g. if you had a mempool tx that spends from 50 transactions in a block, and then that block gets disconnected, you don't get to keep all of them.
10717:46 <michaelfolkson> glozow: Sorry I was stuck on the last question. I think I get it now
10817:46 <glozow> ah ok, great
10917:49 <glozow> moving on to next question but if anybody comes up with a better way to check v3 rules in a reorg, please let me know because I personally find it a tad ugly
11017:49 <glozow> we already did Q5 last week
11117:49 <glozow> Why is it necessary to evict all transactions below minimum relay feerate in TrimToSize()? i.e. this commit: https://github.com/bitcoin-core-review-club/bitcoin/commit/898847e1907eda3d87d19ea43099b71d9eded5f4
11217:49 <glozow> What happens if we remove this check?
11317:51 <michaelfolkson> A transaction that gets re-orged out of a mined block sits in our mempool with too low feerate?
11417:52 <glozow> michaelfolkson: sure, that is potentially a concern. but what's wrong with a low feerate transaction from a block? is it a DoS vector?
11517:54 <theStack> hm i guess it won't propagate if it's not part of a package, i.e. there is no point in keeping it
11617:55 <instagibbs> theStack if you've already validated it, maybe it's fine to keep it around in case it gets mined?
11717:55 <michaelfolkson> glozow: I guess, a pretty sophisticated DoS vector. But if a low fee rate is a DoS vector then this is too
11817:55 <theStack> instagibbs: yeah good point
11917:56 <glozow> I think in some ways it's a pretty poor way to try to DoS people. you need to mine a block and then let it get reorged.
12017:56 <pakaro> +1 glozow
12117:56 <michaelfolkson> glozow: Mine a block full of transactions with low fee rates and then broadcast a competing mined block shortly afterwards
12217:56 <michaelfolkson> Agreed it ain't great
12317:57 <instagibbs> orphaning your own block seems even worse
12417:57 <glozow> welllll you need 2 blocks in the competing chain
12517:58 <glozow> I don't have all the answers tho. There's a discussion about this in https://github.com/bitcoin/bitcoin/pull/27018 if anybody's interested.
12617:59 <glozow> looks like we're out of time
12717:59 <glozow> thanks everyone for coming, I can't stay for extra questions this time unfortunately
12817:59 <instagibbs> glozow was thinking I'd like to do an ephemeral anchors sequel to v3 discussions if people are interested? mental delta should be quite small
12918:00 <instagibbs> can schedule me in for a slot
13018:00 <michaelfolkson> Seems like if you're unsure you keep the check in, conservatism FTW
13118:00 <glozow> instagibbs: that'd be doooope
13218:00 <michaelfolkson> instagibbs: +1
13318:00 <theStack> thanks for hosting glozow!
13418:00 <glozow> #endmeeting