Add `maxfeerate` and `maxburnamount` args to submitpackage (rpc/rest/zmq)

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

Host: ismaelsadeeq  -  PR author: instagibbs

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

Notes

  • Package transactions submitted to the submitpackage RPC successfully are accepted to the node’s mempool, but broadcasted to peers individually because package relay is currently a work in progress. Successful submission does not mean the transactions will propagate.

  • maxburnamount is an optional parameter for the sendrawtransaction RPC that helps prevent users from accidentally burning funds. Transactions with a data carrier OP_RETURN output amount that exceeds maxburnamount (default value: 0) are not submitted or broadcasted.

    • Upon decoding a transaction in sendrawtransaction and testmempoolaccept RPCs, maxburnamount is checked immediately.
  • maxfeerate is an optional parameter for the sendrawtransaction and testmempoolaccept RPCs. Transactions exceeding this feerate won’t be added to mempool and broadcasted to peers, helping prevent unintentional overpayment.

    • testmempoolaccept performs maxfeerate check after package processing. For each successful transaction, it uses the base fee and transaction virtual size in the validation result to check that the feerate does not exceed maxfeerate.

    • In the sendrawtransaction RPC, maxfeerate check is performed by calculating the transaction size fee at maxfeerate fee rate (as max_raw_tx_fee) and passing it to BroadcastTransaction. BroadcastTransaction first does a test accept in order to get the fee and virtual size of the transaction. Transactions whose feerate does not exceed the max_raw_tx_fee are then submitted to the mempool and broadcasted if successful.

  • This PR adds optional parameters maxfeerate and maxburnamount to the submitpackage RPC, enabling similar checks on package transactions.

    • After decoding each package transaction in submitpackage, it checks maxburnamount for each output.

    • It introduces the max_sane_feerate parameter to the ProcessNewPackage function. submitpackage passes maxfeerate as max_sane_feerate.

    • ProcessNewPackage forwards max_sane_feerate to AcceptPackage.

    • Transactions with individual modified feerates exceeding max_sane_feerate are not accepted into the mempool.

Questions

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

  2. Why is it important to perform these checks on submitted packages?

  3. Are there other important checks apart from maxburnamount and maxfeerate that should be performed on packages before they are accepted to the mempool?

  4. The options maxburnamount and maxfeerate can prevent a transaction from entering the mempool and being relayed. Can we consider these options as policy rules? Why or why not?

  5. The commit message states: “This allows subpackage processing and is compatible with future package RBF work.” What makes this compatible with future RBF work?

  6. Why do we validate maxfeerate against the modified feerate instead of the base fee rate?

  7. We validate maxfeerate against the modified feerate of individual package transactions, not package feerate.
    • When can this be inaccurate?
    • Why not use package feerate instead?
    • Under what conditions might it become problematic?
  8. Why can’t maxfeerate be checked immediately after decoding like maxburnamount is?

  9. How does the maxfeerate check in testmempoolaccept RPC differ from submitpackage RPC? Why can’t they be the same?

  10. Can you think of a different approach from the one taken by this PR? What are the tradeoffs?

Meeting Log

Meeting 1

  117:00 <abubakarsadiq> #startmeeting
  217:00 <vmammal> hi
  317:00 <abubakarsadiq> hi
  417:00 <Guest42> hi
  517:00 <alfonsoromanz> hi
  617:00 <Guest4> hi
  717:00 <dberkelmans> hi
  817:00 <hernanmarino> hi
  917:00 <cguida> hi
 1017:00 <GregTonoski> hi
 1117:01 <abubakarsadiq> Hello everyone today we are looking at PR #28950 authored by instagibbs, notes and question are available here https://bitcoincore.reviews/28950
 1217:01 <Ayelen> hi
 1317:01 <instagibbs> hi
 1417:01 <vmammal> hey cguida!
 1517:01 <cguida> sup vmammal!
 1617:01 <abubakarsadiq> Anyone joining us for the first time today? even if you're just lurking, feel free to say hi! thanks for joining @instagibbs
 1717:01 <henmeh> hi
 1817:02 <effexzi> Hi every1
 1917:02 <abubakarsadiq> Have you read the notes, review or tested the PR (y/n)?
 2017:02 <abubakarsadiq> If you reviewed it Concept ACK, approach ACK, tested ACK, or NACK?
 2117:02 <monlovesmango> hey
 2217:03 <vmammal> ACK
 2317:03 <monlovesmango> y, no opinion on ACK/NACK
 2417:03 <larryruane_> hi
 2517:03 <hernanmarino> not me , just lurking today
 2617:04 <abubakarsadiq> Okay lets jump right in
 2717:04 <abubakarsadiq> 1. Why is it important to perform these checks on submitted packages?
 2817:04 <monlovesmango> to prevent users from unintentionally losing funds?
 2917:04 <larryruane_> is it just that if there's a reason to have a check on a single tx submission, then that check would also make sense for the txes within a package?
 3017:05 <larryruane_> (i know that's kind of obvious, i may be missing something!)
 3117:05 <abubakarsadiq> yes @monlovesmango @larryruane it will be helpful to users in ensuring the transactions in packages they are adding to their mempool and broadcasting to peers does not pay unreasonable fee rate
 3217:06 <abubakarsadiq> Also ensuring they dont burn money above some threshold.
 3317:06 <abubakarsadiq> 2.Are there other important checks apart from maxburnamount and maxfeerate that should be performed on packages before they are accepted to the mempool?
 3417:07 <larryruane_> you mean checks that already exist?
 3517:08 <vmammal> 2. yes, various policy rules, descendant limits, etc
 3617:08 <monlovesmango> can only really think of consensus and policy rules
 3717:08 <abubakarsadiq> yes like base fee check can also be helpful I think
 3817:08 <cguida> rbf rules
 3917:09 <abubakarsadiq> rbf rules, policy and consensus check are done during package processing
 4017:10 <monlovesmango> is package processing done prior to acceptance into mempool then?
 4117:10 <abubakarsadiq> maybe we can also can also check package transactions does not exceed the maximum standard transaction size and fail early. However I don't think users should configure this?
 4217:10 <larryruane_> validation.cpp: PreChecks() has a bunch of checks
 4317:11 <abubakarsadiq> Yes @larryruane
 4417:12 <larryruane_> you mean users should not configure the max standard transaction size?
 4517:13 <GregTonoski> Do I understand correctly that the RPC sendrawtransaction will not be unsuccessful ("rejected") by default if there is OP_RETURN?
 4617:13 <abubakarsadiq> No we could just check max standard transaction size early and avoid the work
 4717:14 <abubakarsadiq> 3. The options `maxburnamount` and `maxfeerate` can prevent a transaction from entering the mempool and being relayed. Can we consider these options as policy rules? Why or why not?
 4817:15 <cguida> GregTonoski: just submitpackage, not submitrawtransaction i think
 4917:15 <abubakarsadiq> @GregTonoski if the value in the `OP_RETURN` exceeds `maxburnamount` set by user, if not set default value of 0.
 5017:16 <larryruane_> I guess they are policy, but only local for this node. Definitely not consensus!
 5117:16 <monlovesmango> I think no, bc this only affects the tx that you are broadcasting and not transactions you are relaying..?
 5217:16 <larryruane_> monlovesmango: that's a good point!
 5317:16 <monlovesmango> but do agree that these are personal policy rules
 5417:16 <abubakarsadiq> @cguida do we have `submitrawtransaction` I think he is right it's `sendrawtransaction`
 5517:17 <abubakarsadiq> Thats what I think also, its policy
 5617:17 <cguida> oh snap, you are correct GregTonoski, my bad
 5717:18 <monlovesmango> but just to confirm, this doesn't change the policy for relaying tx right?
 5817:19 <abubakarsadiq> No it does not, it only affect broadcasted transactions from the RPC's.
 5917:19 <monlovesmango> cool thank you!
 6017:19 <abubakarsadiq> 4. The commit message states: “This allows subpackage processing and is compatible with future package RBF work.” What makes this compatible with future RBF work?
 6117:21 <monlovesmango> it enables checks on package transactions? (just guessing heeh)
 6217:21 <abubakarsadiq> My guess is its compatible with package RBF, maybe @instagibbs can chip in here :)
 6317:22 <instagibbs> right, basically moving forward we'll probably process packages in "chunks", but maybe post cluster mempool we cna abort even earlier?
 6417:23 <instagibbs> I dont remember the exactm eaning behind the commit message, I should probably update it :)
 6517:23 <abubakarsadiq> Is it because subpackage processing is where package RBF rules will be checked, and we check the individual modified fee rate against `maxfeerate` during subpackage evaluation/
 6617:24 <glozow> instagibbs: maybe you were distinguishing between sub(ancestor)packages and chunks
 6717:25 <monlovesmango> so is "This allows subpackage processing WHICH is compatible with future package RBF work." more apt for the description of the commit?
 6817:26 <abubakarsadiq> We can continue discussing on that.
 6917:26 <abubakarsadiq> 5. Why do we validate maxfeerate against the modified feerate instead of the base fee rate?
 7017:27 <vmammal> if present, modified feerate will always override the base feerate ?
 7117:27 <monlovesmango> bc modified feerate is higher than base feerate:
 7217:27 <monlovesmango> ?
 7317:27 <glozow> I don't see a good reason why tbh
 7417:27 <glozow> modified isn't necessarily higher than base, no. you can prioritise with a negative value
 7517:27 <abubakarsadiq> I not sure why also It's a bit unclear to me that `sendrawtransaction` and `testmempoolaccept` `maxfeerate` checks are validated against base fee while `submitpackage` is using a modified fee rate.
 7617:28 <abubakarsadiq> I mean base fee rate
 7717:28 <vmammal> glozow ah, true
 7817:29 <monlovesmango> where can I read about the difference between base fee rate and modified fee rate?
 7917:29 <larryruane_> glozow: I think I see what you mean; the maxfeerate is a local parameter so the user can adjust that param directly (user would know the modified feerate)
 8017:29 <glozow> presumably we want to check that the actual, real fees paid by the user are not too high
 8117:30 <instagibbs> I don't know, with non-trivial structures I think that will be difficult. modified feerate is what drives subpackage eval, and would drive linearization post-cluster mempool?
 8217:31 <glozow> yeah, but this is only checking individual right now anyway
 8317:32 <abubakarsadiq> Talking about individual
 8417:32 <abubakarsadiq> We validate maxfeerate against the modified feerate of individual package transactions, not package feerate.
 8517:32 <abubakarsadiq> When can this be inaccurate?
 8617:32 <glozow> I guess you're worried about a prioritisation that would change the linearization/chunking?
 8717:33 <vmammal> monlovesmango I think `getrawmempool` and `prioritisetransaction` rpcs have some info on base vs modified
 8817:33 <instagibbs> glozow it definitely will change it, but if you think modified is the wrong thing, then maybe the future approach would ahve been wrong too
 8917:34 <glozow> maybe we should omit the check if there’s prioritisation 🤷🏻‍♀️
 9017:34 <monlovesmango> thank you vmammal!
 9117:34 <instagibbs> messing with priority is kind of asking for pain if you aren't careful
 9217:35 <monlovesmango> abubakarsadiq: would it be inaccurate if we bump the fee to be higher than 'maxfeerate' and then subsequently bump the package fee to be lower than 'maxfeerate'?
 9317:35 <glozow> instagibbs: yeah. maybe add a bool arg to the RPC for checking even if prioritised?
 9417:35 <abubakarsadiq> even if prioritization affect linearization, I think the aim is to check the actual fee rate, should do just because thats what other nodes will see when its broadcasted?
 9517:36 <glozow> Yeah. But I don't think we'll have logic to have an alternate linearization for non-modified, which is why I'm suggesting to just skip it
 9617:36 <instagibbs> abubakarsadiq I don't know if that's practical. The mempool will be totally ordered via modified fee(like today)
 9717:37 <instagibbs> well, it's not totally ordered now
 9817:37 <abubakarsadiq> In advance :P
 9917:37 <glozow> could have a RPC param to "force check chunk feerate even though it's based on modified fees"
10017:39 <abubakarsadiq> @monlovesmango: I think its when the package child transaction is rejected because its modified fee rate exceeds `maxfeerate` individually, but does not if it's checked as a package.
10117:39 <glozow> monlovesmango: we've discussed modified fees in https://bitcoincore.reviews/24152, https://bitcoincore.reviews/24538, and https://bitcoincore.reviews/27501
10217:40 <monlovesmango> thanks abubakarsadiq and glozow!
10317:41 <glozow> basically imagine CPFPing something very large with a small transaction. child might be super high feerate, but the package feerate is not.
10417:42 <abubakarsadiq> exactly @glozow we might accept the child ancestors at a lower mining score.
10517:42 <monlovesmango> interesting
10617:42 <instagibbs> hmm, when the parent is in the mempool, this check will likely trip as it doesnt take the low fee parent into account
10717:42 <instagibbs> (with a high fee child)
10817:43 <abubakarsadiq> A follow-up question is why check `maxfeerate` against package feerate instead then?
10917:44 <abubakarsadiq> why not*
11017:47 <vmammal> i feel like maxfeerate check should occur on a package, if possible. it seems the answer given on the PR is that this check occurs "prior to any relay"
11117:47 <glozow> vmammal: is it possible?
11217:47 <vmammal> oh wait
11317:48 <vmammal> scoring a package relies on chainstate context ?
11417:48 <glozow> well, all of this needs chainstate
11517:48 <abubakarsadiq> I dont think it would be possible with the current approach because the transaction fee and size are determined during subpackage processing, we have to just check at that time, the subpackage will be added to the mempool (if it passed the check) before the next subpackage is going to be evaluated.
11617:50 <abubakarsadiq> By the time we know the modified fee and size of all the package transactions some might have been added to the mempool already
11717:51 <glozow> no, it's because subpackages aren't chunks, they aren't necessarily grouped as CPFPs
11817:52 <glozow> we know the aggregate package feerate, but it's not an accurate assessment. Let's say you have parents A and B, and child C. parent B spends parent A.
11917:52 <glozow> Let's say A pays 0 fees
12017:52 <glozow> and B bumps it
12117:53 <glozow> A+B is a CPFP, and C can be on its own
12217:54 <glozow> Ah, is this still a concern if it's a tree?
12317:55 <monlovesmango> would C still be considered part of the aggregate package in this scenario?
12417:55 <abubakarsadiq> A+B+C will be evaluated as a subpackage, because A will fail individually, B and C due to missing inputs
12517:56 <glozow> abubakarsadiq: correct
12617:56 <abubakarsadiq> So if we evaluate the `maxfeerate` against A+B+C package its incorrect, ah I see
12717:56 <glozow> However it just occurred to me that this topology isn't allowed through the RPC
12817:57 <monlovesmango> abubakarsadiq: can you expand on why it would be incorrect?
12917:58 <abubakarsadiq> because we dont accept 0 fee txs yet?
13018:00 <glozow> No - so imagine that A+B are large and together not above the maxfeerate, but C is. C should fail the maxfeerate check, but wouldn't because its fees are absorbed by A+B in the aggregation
13119:00 <abubakarsadiq> @monlovesmango because B CPFP A, and C is an individual txs. so should instead check against (A,B) and then check against (C) seperately.
13218:01 <abubakarsadiq> #endmeeting

Meeting 2

13317:00 <abubakarsadiq> #startmeeting
13417:00 <vmammal> hi
13517:00 <abubakarsadiq> hi
13617:01 <monlovesmango> hey
13717:01 <Guest60> hi
13817:01 <vmammal> im pretty sure i got every question wrong yesterday
13917:02 <abubakarsadiq> Welcome everyone, lets continue the discussion of PR #28950 by @instagibbs yesterday discussion are available already on https://bitcoincore.reviews/28950
14017:02 <abubakarsadiq> Hi vmammal which question is that?
14117:03 <abubakarsadiq> The next question is 7. Why can’t `maxfeerate` be checked immediately after decoding like `maxburnamount` is?
14217:04 <monlovesmango> is it bc we have to check for package validity first? ie valid ancestor/descendent relationships
14317:06 <vmammal> what do you mean by 'decoding'
14417:06 <abubakarsadiq> It's because the fee and size are not known yet I think
14517:06 <abubakarsadiq> After decoding the transaction
14617:07 <vmammal> abubakarsadiq Agree, we need the tx vsizes to compute feerates
14717:08 <vmammal> also you would need to get prevouts?
14817:08 <vmammal> for maxburnamount, you can just look at the value of the op_return
14917:10 <abubakarsadiq> since now we just check against individual transaction if it were possible we should just do the check after decoding all the package transactions
15017:10 <abubakarsadiq> but thats not possible because the fee and size are known during package processing after we load coins and subtract output values from input values
15117:11 <abubakarsadiq> @vmammal yes
15217:12 <monlovesmango> interesting thank you for expanding! that makes sense
15317:12 <abubakarsadiq> 8. How does the `maxfeerate` check in `testmempoolaccept` RPC differ from `submitpackage` RPC? Why can’t they be the same?
15417:13 <monlovesmango> `testmempoolaccept` uses base fee rate and `submitpackage` uses modified fee rate
15517:14 <monlovesmango> they cant be the same bc `testmempoolaccept` doesn't have access to modified fee rates...?
15617:15 <abubakarsadiq> Yes, the second thing is we do the maxfeerate check post package processing in `testmempoolaccept` any idea why?
15717:17 <monlovesmango> bc it isn't build to handle package eval yet? no clue
15817:18 <monlovesmango> but I would think only submitpackage is specialized to accurately estimate package fee rate
15917:18 <abubakarsadiq> @monlovesmango it does have access modified fee, I think.
16017:19 <abubakarsadiq> It’s done after the testaccept package processing because the txs are not added to mempool and broadcasted after the processing, we can safely check maxfeerate and return appropriate error messages.
16117:20 <abubakarsadiq> They cannot be the same because in `submitpackage`, the package transactions might have already been accepted into the mempool and broadcasted to peers, rendering the check redundant.
16217:21 <abubakarsadiq> If we were to do it after package processing in `submitpackage` it would just be redundant check.
16317:22 <abubakarsadiq> Last Question
16417:22 <abubakarsadiq> 9. Can you think of a different approach from the one taken by this PR? What are the tradeoffs?
16517:24 <monlovesmango> so why is maxfeerate check done post testaccept package processing?
16617:26 <vmammal> slightly off-topic, but if anyone's interested in cluster mempool, I recommend this https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393
16717:26 <abubakarsadiq> Do you mean why we check `maxfeerate` after the package has been processed? Thats because the package processing only `test_accept`, i.e see if we can accept this list of transactions, they are not actually accepted.
16817:27 <abubakarsadiq> Thats in `testmempoolaccept` RPC .
16917:28 <abubakarsadiq> thanks vmammal yeah I am :)
17017:29 <abubakarsadiq> There are some approaches listed by glozow here https://github.com/bitcoin/bitcoin/pull/28950#pullrequestreview-1790751210 we can discuss the tradeoffs
17117:29 <monlovesmango> yes thats what I meant. why can't `testmempoolaccept` use modified fee rate? if testing acceptance into mempool seems like you want to use modified fee rate?
17217:31 <monlovesmango> haha ok now glozow talking about chunk fee rate... need to read up on that
17317:34 <abubakarsadiq> Yeah linearizing uses modified fee not base fee, so thats why I guess
17417:35 <vmammal> i think there was a recent Optech podcast where sipa explains clusters pretty well
17517:36 <monlovesmango> you mean linearizing uses base fee, not modified fee?
17617:36 <monlovesmango> vmammal: reading the clusters thing now, very intriguing
17717:38 <abubakarsadiq> So listing the alternative approaches in the link
17817:38 <abubakarsadiq> 1. test package accept, then perform `maxfeerate` check before package processing
17917:38 <abubakarsadiq> 2. Adding a helper that loads coins and calculate a fees, and calculate vsize so that we can immediately perform the check before package processing.
18017:40 <abubakarsadiq> The tradeoffs of 2 is double work, we have to process the package twice
18117:41 <vmammal> abubakarsadiq which alternative do you prefer?
18217:41 <monlovesmango> whats the trade off for 1?
18317:42 <monlovesmango> I assume theres also some duplicative processing? since earlier it was said that you need to process the package to figure out fee rate
18417:43 <abubakarsadiq> I don't about that, but quoting from the PR discussion there is concern about not being extensible to chunk feerate
18517:43 <monlovesmango> what is chunk feerate?
18617:44 <abubakarsadiq> Sorry the tradeoff I was talking about is for 1 you are right @monlovesmango
18717:45 <monlovesmango> no worries!
18817:45 <monlovesmango> but then my question is what is the trade off for 2? haha
18917:45 <monlovesmango> sorry I really need to start digging into code more.
19017:45 <abubakarsadiq> Given a cluster of transactions, you linearize it to chunks (the chunk fee rate is the mining score of each chunk)
19117:46 <monlovesmango> gotcha
19217:47 <monlovesmango> is trade off for 2 similar to 1? I guess I don't understand enough to really differentiate between 1 and 2, since both perform maxfeerate check prior to package processing
19317:47 <abubakarsadiq> https://delvingbitcoin.org/t/cluster-mempool-definitions-theory/202 I find this also helpful
19417:48 <monlovesmango> OHH so this is related to cluster pool that vmammal mentioned
19517:48 <monlovesmango> thank you
19617:51 <vmammal> still in early stages from what i can tell
19717:51 <monlovesmango> and gloria already thinking ahead, impressive
19817:52 <vmammal> in `test/functional/rpc_packages.py`, under "relax restrictions.. parent gets through", I was wondering what happened to the child tx. But I think I answered my own question - the child IsUnspendable?
19917:54 <abubakarsadiq> So If i understand this correctly the tradeoff is that in the future when we have cluster mempool we will like to check `maxfeerate` against chunk fee rate not individual fee rate.
20017:54 <abubakarsadiq> The approach is suggesting we create a helper function that calculate the fee rate of all the package transactions and their sizes and we `maxfeerate` against individual txs fee rate early before package processing.
20117:54 <abubakarsadiq> But when we have cluster mempool, we would like to switch to checking `maxfeerate` against the chunk fee rate, which means this approach is not extensible to that, we have to update and check after the package is linearized and we got the chunk fee rate.
20217:56 <monlovesmango> ah ok that makes sense
20317:56 <monlovesmango> really appreciate the level of detail!
20417:59 <abubakarsadiq> #endmeeting