Re-delegate absurd fee checking from mempool to clients (rpc/rest/zmq, tests, validation, wallet)

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

Host: gzhao408  -  PR author: gzhao408

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

Notes

  • AcceptToMemoryPool (ATMP) processes a transaction for acceptance to the mempool. In addition to enforcing consensus rules, it also applies the node’s policy. PR #19339 deals with acceptance logic related to user preference (which falls into neither category) and thus doesn’t belong in validation.

  • Policy represents a node’s extra standards for validation, typically only used in situations where it may be appropriate to apply rules in addition to consensus. It includes things like fees (e.g. -minrelaytxfee) and extra verification flags (e.g. SCRIPT_VERIFY_DISCOURAGE_UPGRADEABLE*).

    • This contrasts with consensus rules, which are universally applied.
    • This also contrasts with user- or client-specific rules. These may vary by client or even by call (e.g. arguments passed into RPCs).
  • Pre-19339, ATMP accepts an argument, const CAmount& nAbsurdFee, and enforces it as a maximum fee amount; this protects users from paying “absurdly high” fees resulting from unintentional errors. ATMP returns a TxValidationResult::TX_NOT_STANDARD error for absurd fees, which seems to suggest that rejecting absurd fees is part of policy.

  • However, nAbsurdFee is only used for transactions originating from the node’s clients (RPC and wallet). When a node receives a transaction from its peers, it uses an empty value for nAbsurdFee when calling ATMP (you can see the values of nAbsurdFee removed in the Remove absurdfee from accepttomempool commit). Furthermore, there is no way for a node operator to set the maximum fee, e.g. through a config setting or command line argument. The wallet, on the other hand, allows the user to set a maximum fee using -maxtxfee.

  • PR #19339 redelegates fee checking from the mempool to the clients in order to make mempool logic less user-specific. The main observable behavior change is in the error message, which is updated to include RPC.

    • This only changes RPCs testmempoolaccept and sendrawtransaction. There are no changes to the wallet (reviewers may want to verify that this is not an issue).

    • Clients often use BroadcastTransaction() to interface with ATMP; it also takes max_tx_fee to pass in as nAbsurdFee. This PR adds fee checking to BroadcastTransaction() by adding a dry run of ATMP before the real submission.

  • To be careful to preserve the fee protection offered to the user, the PR includes a few intermediate stages. The author recommends reviewing each commit individually and confirming that the behavior is identical at each stage.

    1. Catch mempool rejections based on AbsurdFee and return “max-fee-exceeded” instead of “absurdly-high-fee” for high fees.

    2. In BroadcastTransaction, first dry-run ATMP and check the fee that is returned. If the fee is below max_tx_fee, send the transaction for real.

    3. Ignore AbsurdFee in ATMP.

    4. Remove AbsurdFee from ATMP.

Questions

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

  2. Does this PR change any behavior or is it a pure refactor?

  3. There was some discussion as to whether AbsurdFee is policy or a client preference. What do you think, and how did you determine this?

  4. Compare the implementations of the testmempoolaccept and sendrawtransaction RPCs. What are the differences? (hint: test_accept)

  5. What information do you need to check a transaction’s fee? At what point would the node have this information on hand?

  6. How does the wallet handle the user’s maximum fees? (hint: m_default_max_tx_fee) Do any changes to the wallet need to be made?

  7. Let’s consider the cost of dry-running ATMP beforehand: what is cached and what do we have to do all over again? Is this a performance concern? Why or why not?

  8. Bonus: should there be a maximum fee policy, e.g. -maxrelaytxfee for nodes? Why or why not would a node/miner want to avoid relaying transactions with very high fees?

Meeting Log

  119:00 <gzhao408> #startmeeting
  219:00 <nehan> hi!
  319:00 <pinheadmz> 🍹
  419:00 <amiti> hi !
  519:00 <gzhao408> Welcome to PR Review Club everyone!
  619:00 <pinheadmz> hi
  719:00 <troygiorshev> hi
  819:00 <DariusP> hi!
  919:00 <emzy> hi
 1019:00 <felixweis> hi
 1119:00 <jnewbery> hi
 1219:00 <evanlinjin> hi
 1319:00 <dhruvmehta> hi
 1419:00 <michaelfolkson> Haha hi!
 1519:00 <lightlike> hi
 1619:00 <jb55> hiya
 1719:00 <ariard> hi
 1819:00 <robot-dreams> hi
 1919:00 <shrouded> hi
 2019:01 <ajonas> hi
 2119:01 <gzhao408> Today we’re looking at #19339: re-delegate absurd fee checking from mempool to clients. Have y’all had the chance to review the PR? (y/n)
 2219:01 <pinheadmz> y
 2319:01 <amiti> y
 2419:01 <robot-dreams> y
 2519:01 <dhruvmehta> y
 2619:01 <jonatack> hi (half here)
 2719:01 <evanlinjin> n
 2819:01 <jnewbery> y
 2919:01 <willcl_ark> hi
 3019:01 <nehan> y
 3119:01 <jonatack> reviewed: a little
 3219:01 <lightlike> n
 3319:01 <troygiorshev> n
 3419:01 <jb55> n
 3519:01 <DariusP> n
 3619:02 <emzy> y/n
 3719:02 <michaelfolkson> y/n too
 3819:02 <willcl_ark> yes I took a look at it but not done any custom testing of it
 3919:02 <gzhao408> SO happy! :D Would anyone like to summarize what the PR is doing (and why)?
 4019:03 <behradkhodayar> hi, looked at it but not tested
 4119:03 <dhruvmehta> Summary: This PR moves the responsibility to avoid absurdly high fees from ATMP to upstream places like the RPC layer and wallets. It does so by re-using ATMP logic in dry-run mode (while caching expensive results). This reflects existing incentive structures as nodes should protect their owner-users from high fees, but not minimize fees being
 4219:03 <dhruvmehta> accepted from other owners.
 4319:04 <gzhao408> dhruvmehta: yep, it re-delegates the responsibility! could anyone tell us why it should be in clients instead of in ATMP?
 4419:04 <michaelfolkson> And we are worried about us a user creating a transaction with an absurdly high fee rather than accepting a transaction with an absurdly high fee from another peer into our mempool
 4519:04 <pinheadmz> because its a user-error not a mempool error
 4619:04 <pinheadmz> form the perspective of our mempool we shouldnt really care if someone wants to throw away money like that
 4719:05 <pinheadmz> but we should save the user from teh wallet perspecive from shooting themself in the foot
 4819:05 <evanlinjin> Sorry for the stupid question, but what is ATMP?
 4919:05 <evanlinjin> Oh memory pool?
 5019:05 <willcl_ark> (and miners would actively _want_ those transactions anyway)
 5119:05 <pinheadmz> accept to memory pool!
 5219:05 <gzhao408> pinheadmz: exactly! and we'll get to the part about "should" later :)
 5319:05 <gzhao408> AcceptToMemoryPool for reference: https://github.com/bitcoin/bitcoin/blob/c831e105/src/validation.cpp#L1084
 5419:05 <evanlinjin> ty
 5519:06 <jnewbery> evanlinjin ATMP = AcceptToMemoryPool() is the function that we call to process a transaction and add it to the mempool if it's acceptable
 5619:06 <gzhao408> <michaelfolkson> has a really good point as well, we definitely want to protect users from really high fees
 5719:07 <gzhao408> (just in the clients, not in ATMP 😛)
 5819:07 <gzhao408> Alright let's dive in: does this PR change any behavior or is it a pure refactor?
 5919:08 <robot-dreams> It changes user-facing error messages returned from the `testmempoolaccept` and `sendrawtransaction` RPCs
 6019:08 <pinheadmz> mainly a refactor (moving functionality around) but in this case it does remove something from the mempool acceptance
 6119:08 <dhruvmehta> It changes the behavior for ATMP which will no longer reject transactions with too high a fee. ATMP will instead compute the fee after policy checks and has the test_accept option for a dry run.
 6219:08 <willcl_ark> well it doesn't change behaviour from the user perspective, but your node might respond differently to incoming transactions
 6319:09 <nehan> i think it's just a refactor (aside from error codes) based on where 0 was passed in to ATMP everywhere
 6419:09 <robot-dreams> For wallet and RPC transactions, it also moves the absurd fee check later in the validation process, as dhruvmehta mentioned
 6519:09 <michaelfolkson> How might your node respond differently to incoming transactions willcl_ark?
 6619:09 <gzhao408> <robot-dreams>, <pinheadmz>, <dhruvmehta> correct!
 6719:10 <nehan> gzhao408: sorry, are you saying it *is* a behavior change?
 6819:10 <gzhao408> Here's a thought: ATMP is used for validating/mempoolaccepting both transactions received from peers and from clients (RPC/wallet). Before and after this PR, is it possible for ATMP to return a different result for the same transaction received from a peer as opposed to a client?
 6919:11 <michaelfolkson> I would say no
 7019:11 <robot-dreams> Interesting question! I think yes, if you and the peer have different settings for maxfee
 7119:11 <gzhao408> nehan: well I didn't think there was a real "correct" answer to this question to be honest :) you're correct as well (i just typed kind of late)
 7219:12 <jonatack> For the record, if anyone wants an interesting definition of policy with regards to bitcoin, gzhao408 gave me a way of thinking about it that I like. https://github.com/bitcoin-core-review-club/website/pull/242#discussion_r471059359
 7319:12 <emzy> I would also say no. Because 0 was passed into ATMP everywhere.
 7419:12 <gzhao408> <willcl_ark> You're correct as well that the behavior doesn't change from the perspective of the user, but I'd also like to hear more about what you meant by "your node might respond differently to incoming transactions"
 7519:12 <gzhao408> emzy: bingo! this leads us to our next question: There was some discussion as to whether AbsurdFee is policy or a client preference. What do you think, and how did you determine this?
 7619:13 <pinheadmz> absurdity is an opinion :-)
 7719:13 <pinheadmz> policy / mempool rules are generally DoS mitigations that are added to consensus rules
 7819:13 <pinheadmz> although luke-jr had a good point about this too
 7919:13 <gzhao408> (again, no real "correct" answers here - would love to hear a list of arguments for both policy and client preference)
 8019:14 <pinheadmz> that there are many settings that could be considered opinoins by the node operator
 8119:14 <emzy> I would say it's only a client preference.
 8219:15 <amiti> if I were a miner and somebody sent me a txn via p2p that had an insanely high fee, I would definitely want to accept it. as a user I don't want to create those kinds of transactions. that makes it clear to me that its a client preference
 8319:15 <willcl_ark> gzhao408: as I was reading it, before, we would reject transactions received from peers who were above our _own_ max_fee, whereas this now means this does not happen
 8419:15 <gzhao408> pinheadmz: is there an easy way for the node operator to set the max fee? :) similar to how they can set the min fee?
 8519:16 <jnewbery> willcl_ark: no. absurdfee is only used for locally submitted txs
 8619:16 <pinheadmz> bitcoind command line arg:
 8719:16 <pinheadmz> -maxtxfee=<amt>
 8819:16 <pinheadmz> Maximum total fees (in BTC) to use in a single wallet transaction;
 8919:16 <jnewbery> if you look at the calls to ATMP from net_processing, you'll see that they don't specify an absurdfee
 9019:17 <michaelfolkson> Right I think it has to be client preference. Although you may want to treat your own transactions favorably in your mempool policy over another's peer transaction
 9119:17 <sipa> gzhao408: philosophical point mostly, but i wouldn't call the absurdfee rule a policy - it's not something bitcoin core enforces on the network
 9219:17 <willcl_ark> jnewbery: thanks, I tried to trace that backwards...
 9319:17 <gzhao408> pinheadmz: is that used for wallet transactions or for the node's validation? :)
 9419:18 <willcl_ark> jnewbery: ah I see, they use the bypass_limits :) thanks
 9519:18 <jnewbery> (you can also see that -maxtxfee isn't used by the node, since it's defined in wallet/init.cpp)
 9619:18 <ariard> michaelfolkson: yes but it's quite limited if your transactions don't propagate, unless you have preferential peering with all the same settings
 9719:19 <sipa> michaelfolkson: you don't want to treat your own transactions differently from other's transactions in the mempool; the mempool is what you use to predict what will be mined
 9819:19 <pinheadmz> gzhao408 jnewbery ah, my bad i thought that was the value checked in ATMP currently
 9919:19 <sipa> michaelfolkson: and using a different policy for your own transactions means a possibly observable privacy leak too
10019:20 <michaelfolkson> sipa ariard: But you want it to propagate even if it is very low fee. Don't you rebroadcast from your own mempool....?
10119:20 <amiti> michaelfolkson: the wallet initiates rebroadcast, not mempool
10219:20 <sipa> michaelfolkson: if it's a very low fee, it won't propagate at all; even if you make your own mempool ignore that fact
10319:21 <michaelfolkson> Gotcha I thought that might be wrong, thanks amiti
10419:21 <gzhao408> Love the discussion! This PR's opinion is that it's client preference and thus belongs in clients :) and as sipa said, ATMP should give consistent results regardless of which who it comes from
10519:21 <gzhao408> "which who" haha, which client
10619:22 <gzhao408> Ok let's keep on chugging: This PR changes testmempoolaccept and sendrawtransaction - compare the implementations of the two RPCs. What are the differences? (hint: test_accept)
10719:22 <jnewbery> michaelfolkson: if the transaction fee is too low to get in your own mempool (eg if prevailing fees are high and your mempool is dropping low fee txs), then you won't rebroadcast it
10819:23 <jnewbery> it's impossible to broadcast something that isn't in your mempool
10919:23 <robot-dreams> unlike testmempoolaccept, sendrawtransaction calls into `BroadcastTransaction`, which (i) calls ATMP a second time with `test_accept = true` and (ii) does some extra bookkeeping
11019:23 <dhruvmehta> test_accept=1 is a dry run. It runs policy checks and signature checks which are cached though so a second "actual" run is significantly cheaper.
11119:23 <gzhao408> for reference testmempoolaccept and sendrawtransaction can be found here: https://github.com/bitcoin/bitcoin/blob/c831e105/src/rpc/rawtransaction.cpp#L858 and https://github.com/bitcoin/bitcoin/blob/c831e105/src/rpc/rawtransaction.cpp#L800
11219:24 <robot-dreams> Also, for some reason testmempoolaccept does an extra check `request.params[0].get_array().size() == 1` (maybe in the future, it's intended to test multiple transactions in a single RPC?)
11319:25 <sipa> robot-dreams: ah the age old dream of package relay :)
11419:25 <gzhao408> 👌 thanks robot-dreams and dhruvmehta for the explanations!
11519:25 <jnewbery> robot-dreams: that's exactly the reason. If we implement package relay/package mempool acceptance, then testmempoolaccept can be changed to support that without changing the interface
11619:25 <dhruvmehta> gzhao408 I will try and run your branch later and add timing, but do you happen to know how much cheaper a repeat ATMP call is after a dry run?
11719:26 <gzhao408> excellent observation robot-dreams, yes - I believe that is the plan, but it gets a little tricky with transactions that depend on each other (would love to talk about this more after pr review club)
11819:27 <gzhao408> dhruvmehta: we have a question coming up later about this :)
11919:27 <jnewbery> for anyone is wondering what package relay is: https://bitcoinops.org/en/topics/package-relay/
12019:28 <ariard> the golden dream of safe multi-party transactions
12119:29 <jonatack> we did a review club about package relay as well: https://bitcoincore.reviews/16401
12219:29 <robot-dreams> Thanks for the link jnewbery! I didn't realize the implications of package relay on fee bumping.
12319:29 <michaelfolkson> robot-dreams has got us dreaming :)
12419:29 <robot-dreams> haha :)
12519:29 <pinheadmz> ok i gotta embarass myself for a sec here... before this PR cant tell where nAbsurdFee comes from
12619:29 <pinheadmz> im chasing function calls around the codebase :-P
12719:29 <ariard> robot-dreams: in theory you can bump feerate of parent transactions whatever network topology
12819:30 <ariard> I've a PoC branch here : https://github.com/bitcoin/bitcoin/pull/19621
12919:30 <gzhao408> pinheadmz: it only comes from the clients :P
13019:30 <ariard> but actually there is different way to implement it and maybe not the best approach
13119:30 <jnewbery> pinheadmz: no embarassment necessary. It's a bit convoluted
13219:31 <pinheadmz> gzhao408 jnewbery im trying to find a call to ATMP with an actual value there instead of 0
13319:31 <gzhao408> I dream about Package Relay too, but let's go onto the next question! What information do you need to check a transaction’s fee? At what point would the node have this information on hand? (there are many ways. hint: look at where in ATMP the fees are calculated)
13419:31 <jnewbery> Everything that uses absurdfee goes through BroadcastTransaction() in node/transaction.cpp
13519:31 <gzhao408> pinheadmz: also look at calls to BroadcastTransaction maybe
13619:31 <pinheadmz> gzhao408 aha ty i see it now
13719:31 <robot-dreams> To check a transaction's fee, you'd need to load the actual UTXOs being spent, because the raw transaction only has `txid`, `vout`
13819:32 <jnewbery> The argument there is called max_tx_fee
13919:32 <jnewbery> https://github.com/bitcoin/bitcoin/blob/93ab136a33e46080c8aa02d59fb7c2a8d03a3387/src/node/transaction.cpp#L16
14019:32 <pinheadmz> yep on it now
14119:32 <michaelfolkson> Very cool ariard
14219:32 <gzhao408> thanks jnewbery for being super helpful 😊
14319:32 <pinheadmz> so wait, then i was wrong earlier - a tx relayed by a peer is not subject to absurd fee bc the value there checked against 0 ?
14419:32 <sipa> pinheadmz: indeed
14519:32 <willcl_ark> pinheadmz: correct
14619:32 <gzhao408> pinheadmz: yes
14719:32 <pinheadmz> everyone at once!
14819:32 <pinheadmz> thanks, i am now on the same page
14919:32 <jnewbery> imhelping.gif
15019:33 <willcl_ark> john set me straight on that earlier ;)
15119:33 <sipa> gzhao408: in a more PSBT-centric world the fee could be calculated from PSBT information, rather than needing the UTXOs being spent... doing it at broadcast time is arguably too late anyway
15219:33 <robot-dreams> sipa: do you mean it should arguably be done at signing time instead?
15319:34 <amiti> I was initially confused about that too. I think that's why this PR makes sense because essentially, it was already a client-only check. the code just didn't make that apparent.
15419:34 <sipa> robot-dreams: well, it is - that's why decodepsbt/analyzepsbt show fee information, as you wouldn't want to sign something that has a crazy fee
15519:34 <jnewbery> sipa: in that case, no harm in *also* doing it at broadcast time as belt-and-suspenders
15619:34 <gzhao408> robot-dreams: is off to a great start - fee is inputs minus outputs. are input amounts included in a transaction/ are they guaranteed to exist? what if the transaction is malformed?
15719:34 <sipa> jnewbery: of course
15819:35 <sipa> but it's only for "bundled" workflows where signing/broadcast happen at the same time that it's sufficient to only do it at ATMP (or around that time)
15919:35 <sipa> amiti: indeed
16019:35 <michaelfolkson> input amounts aren't included in a transaction
16119:36 <michaelfolkson> Are the amounts guaranteed to exist... hmm not sure what that means. Do you mean are the amounts guaranteed to cover the outputs?
16219:37 <ariard> michaelfolkson: are you sure they aren't included in segwit transactions ;) ?
16319:37 <gzhao408> michaelfolkson yep, and sorry i meant "are the inputs guaranteed to exist"
16419:37 <michaelfolkson> ariard: Oof
16519:37 <nehan> we have to look the inputs up in our utxo set. hopefully we have them.
16619:38 <gzhao408> nehan: yeah! (the point I'm trying to make is, we have to do a tiny bit of sanitization and "validation" in order to check the fee)
16719:38 <sipa> nehan: if we don't, they won't be accepted into the mempool anyway :)
16819:38 <sipa> gzhao408: right the property you want is that (a) either the fee calculation is correct or (b) the transaction will be invalid anyway
16919:39 <nehan> sipa: yes
17019:39 <gzhao408> sipa: yaaaas
17119:39 <ariard> due to hashPrevouts
17219:40 <jonatack> FWIW, in master it is now possible to set an explicit feerate in sentoaddress, sendmany, bumpfee, fundrawtransaction, and walletcreatefundedpsbt
17319:40 <dhruvmehta> Even if we have the utxos, merely subtracting sum{output values} from sum{input values} is not enough. We need to check that policy checks and signature checks will pass. That happens in ATMP. If the tx won't be accepted for other reasons, fee is zero :) and testmempoolaccept needs to raise that other rejection reason.
17419:40 <sipa> that's the security model underlying tx signing in general... in offline signing workflows there is no way you can know the input amounts/fees/utxos can exist, but you generally have enough information to decide what the fee will be *or* the tx will be invalid (but see the recent input amounts BIP143 issue)
17519:40 <jonatack> since https://github.com/bitcoin/bitcoin/pull/11413 was merged last month
17619:40 <nehan> dhruvmehta: i would argue to calculate the fee you shouldn't actually *need* to do all those things
17719:40 <ariard> sipa: unless merkleproof and headers verification by your signer, but never seen this on any hardware wallet
17819:41 <sipa> ariard: yes, but the point is that that isn't (or at least shouldn't be) necessary
17919:41 <jonatack> s/last month/June 25/
18019:41 <sipa> if you're given incorrect UTXOs, your signature should be invalid
18119:41 <dhruvmehta> nehan yeah i can see that. it depends on whether we think fee = "fee, if the rest of the tx is valid", of fee = "fee, for the tx as-is"
18219:41 <nehan> dhruvmehta: the second call to ATMP will check for all that once you've done the fee check. so as sipa says you just need to be sure that either you've calculated the fee correctly or it would be rejected anyway
18319:43 <nehan> ariard: i don't understand what you're talking about, could you say more?
18419:43 <gzhao408> nehan beat me to it :) dhruvmehta you're right that we need this information for the end result, although it's not strictly necessary to get the fee (which is why it's gathered in PreChecks)
18519:43 <ariard> sipa: but how do you that amounts are correct even if you sign valid utxos ?
18619:43 <ariard> I mean you need an oracle
18719:43 <gzhao408> Alright, next question? :) This PR _doesn’t_ change the wallet. How does the wallet handle the user’s maximum fees? (hint: m_default_max_tx_fee) Do any changes to the wallet need to be made?
18819:44 <sipa> ariard: you don't know they're correct, but if they're incorrect, your signature will be invalid
18919:44 <dhruvmehta> nehan ah, i think i understand now. thanks.
19019:44 <ariard> right, that's my point providing a merkle proof on all spend transaction and doing headers verification would let know the signer about amounts value ?
19119:44 <sipa> ariard: or at least should be - again, see the recent segwit amount signing issue
19219:44 <ariard> under a PoW security model
19319:45 <sipa> ariard: sure, but what's the point?
19419:45 <nehan> dhruvmehta: yeah to be clear in the code it does call ATMP twice so it does do all those checks.
19519:45 <jnewbery> ariard: getting a little bit in the weeds. perhaps defer this until after the meeting?
19619:45 <ariard> sipa: we likely derive from the main thread, but actually hardware wallet flow is to ask user input ?
19719:46 <sipa> ariard: let's discuss after meeting
19819:46 <robot-dreams> gzhao408: I think the wallet checks max fee in two places: `CreateTransactionInternal` and `SubmitMemoryPoolAndRelay`; the latter eventually calls into ATMP
19919:46 <jnewbery> ariard sipa: thanks :)
20019:46 <dhruvmehta> nehan yeah the caching makes the overhead small if most transactions are actually valid. but i can see that if we don't want to presume anything about how many transactions are valid, doing light-weight checks separately prior is valuable.
20119:46 <gzhao408> robot-dreams: awesome! yes! and as pinheadmz pointed out earlier, you can set the -maxtxfee in the wallet
20219:48 <gzhao408> so `SubmitMemoryPoolAndRelay` - what does it call to submit to the mempool? ATMP directly or...?
20319:49 <amiti> goes through broadcast transaction which calls ATMP
20419:50 <gzhao408> amiti: we have a winner! so BroadcastTransaction will give it a belt-and-suspenders check :)
20519:50 <gzhao408> alright i want to get to the juiciest question now
20619:50 <gzhao408> Let’s consider the cost of dry-running ATMP beforehand and then for realsies afterward: what is cached and what do we have to do all over again? Is this a performance concern? Why or why not?
20719:51 <dhruvmehta> The git branch with extra logging makes it clear that caching results in fewer calls. I am unsure about the overhead in terms of time though.
20819:52 <gzhao408> yes! c a c h i n g! btw, <dhruvmehta> is referring to this branch I made that adds some logging to ATMP so you can see what’s being cached: https://github.com/gzhao408/bitcoin/commit/483cc24a2923514a35f83682822aa29265c61555
20919:53 <nehan> it just pains me that ATMP is being used as a fee calculator
21019:53 <nehan> (though i still like this PR)
21119:53 <jeremyrubin> nehan: I think that's somewhat inavoidable, no?
21219:54 <emzy> There are also a limit on how many TX you can broadcast anyway.
21319:54 <jnewbery> nehan: why? The logic is already there!
21419:54 <gzhao408> <nehan> indeed :( but we need to run ATMP anyway in all of these cases
21519:54 <jeremyrubin> the only way to know if you have enough fee to go in the mempool is to go in the mempool.
21619:54 <pinheadmz> yes gzhao408 did a great job of providing proof-of-efficiency
21719:54 <nehan> jeremyrubin: this is not checking to see if you have enough fee to go in the mempool. that check has been moved outside. this is literally just calculating the fee to check it outside the call to ATMP.
21819:54 <sipa> i'm not sure that's correct; you needed the inputs to be able to sign the transaction anyway
21919:54 <pinheadmz> but i sorta feel with nehan i wish there were another way to comput fee that was outside ATMP
22019:54 <jnewbery> jeremyrubin: this isn't about enough fee to get in the mempool. It's about not having _too much_ fee
22119:55 <jnewbery> nehan: you beat me to it!
22219:55 <nehan> jnewbery: no i was slightly confused, your comment was super helpful. jeremyrubin is still wrong though ;)
22319:55 <jeremyrubin> i meant more generally because of CPFP rules you need to at least calc ancestors
22419:56 <jeremyrubin> I think?
22519:56 <jeremyrubin> Could be wrong on how cpfp plays in. Anyways, not on absurd fee stuff, which is the focus here
22619:56 <nehan> jeremyrubin: i'm not arguing ATMP shouldn't calculate fees and check them sometimes. I'm arguing you shouldn't *need* to use the full hammer of ATMP if all you want to do is calculate fees.
22719:56 <jnewbery> nehan: I tried to do this before without using ATMP to check the fee and it wasn't as good as this PR: https://github.com/bitcoin/bitcoin/pull/15810
22819:56 <nehan> jnewbery: i saw it!
22919:57 <michaelfolkson> 5 minutes to go so will attempt to start concurrent conversations. Re Luke's comment that all policy should be split from consensus changes not just one policy. Thoughts?
23019:57 <nehan> tbf i don't know how to fix this :)
23119:57 <gzhao408> nehan: i tried so hard man
23219:57 <jeremyrubin> michaelfolkson: cory is working on that a bit
23319:57 <nehan> gzhao408: i think what you did makes sense and is an improvement!
23419:57 <michaelfolkson> jeremyrubin: As in Cory is working on the rest in a separate PR?
23519:57 <gzhao408> to jeremyrubin's point, the failure cases are (1) doesn't even make it into mempool (2) fee too high. so we do kinda need both
23619:58 <jnewbery> michaelfolkson: this isn't policy so I think that's a separate conversation
23719:58 <jeremyrubin> it's something else. p2p policy, local safety policy, consensus
23819:58 <jeremyrubin> I think it's still "policy"
23919:58 <michaelfolkson> Oh yes of course. Ok that answers that haha
24019:58 <jnewbery> I don't think 'local safety policy' is helpful terminology!
24119:59 <jnewbery> policy already means something else
24219:59 <jeremyrubin> not sure what else to call it?
24319:59 <ariard> it's wallet policy
24419:59 <jnewbery> argh!
24519:59 <sipa> sanity check
24619:59 <gzhao408> yeah, we've only got 1 minute left!
24719:59 <jonatack> psa for anyone who hasn't seen it yet, more fun with fees: we have a new fee config option: -maxapsfee
24819:59 <shrouded> user preference
24919:59 <jonatack> https://github.com/bitcoin/bitcoin/pull/14582
25019:59 <gzhao408> any more questions/suggestions about the ATMPx2? I will leave the "should maxrelayfee be policy" as an exercise to the reader
25120:00 <gzhao408> wat iz policy??
25220:00 <evanlinjin> I'm getting confused by the definition of policy
25320:00 <michaelfolkson> It compiles to Miniscript haha
25420:00 <gzhao408> 11AM already! that's a wrap :) Thanks for coming everyone!
25520:00 <dhruvmehta> gzhao408 I think that the name for ATMP is probably a loaded thing, but calling it `CheckAndAcceptToMemPool` could perhaps help?
25620:00 ⚡ michaelfolkson getting my coat
25720:00 <nehan> gzhao408: thank you!
25820:01 <robot-dreams> gzhao408: thanks for hosting! (and I'm confused about policy too)
25920:01 <amiti> thanks gzhao408! that was fun :)
26020:01 <willcl_ark> thanks gzhao408! very interesting
26120:01 <evanlinjin> thank you!
26220:01 <troygiorshev> thanks gzhao408!!
26320:01 <jonatack> gzhao408: you know, since policy isn't defined anywhere (that we know of) in the codebase, maybe open a PR to add a definition?
26420:01 <shrouded> +1
26520:01 <dhruvmehta> gzhao408 thank you for the PR and for walking us through it so well.
26620:01 <troygiorshev> +1
26720:01 <emzy> Thank you gzhao408 and all!
26820:01 <shrouded> thanks gzhao408
26920:01 <DariusP> thanks gzhao408 !!! That was helpful and a lot of fun
27020:01 <gzhao408> #endmeeting
27120:01 <sipa> thanks gzhao408!
27220:01 <pinheadmz> thanks gzhao408 !
27320:01 <figs> Thanks
27420:01 <jnewbery> thanks gzhao! Great meeting!
27520:01 <pinheadmz> you rock, great job amd good hosting
27620:02 <ariard> policy/mechanism in the unix terminology I guess
27720:02 <jonatack> thanks gzhao408, great job!
27820:02 <ariard> you have an imperative mechanism (consensus rules) on which application are programming policy (tx-relay policy)
27920:02 <gzhao408> Feeling the love! 😊 thanks everyone for participating and being so nice to me
28020:02 <felixweis> thanks!
28120:03 <wumpus> from what I've noticed "policy" is generally defined as "P2P network transaction policy"
28220:03 <ariard> *policies
28320:03 <wumpus> that's, at least, always how I interpreted it
28420:03 <nehan> ariard: sipa: if you continue your conversation from before, it would be awesome if you could quickly summarize the point you were discussing
28520:03 <sipa> i don't understand ariard's point :)
28620:04 <ariard> wumpus: you have script interpreter flags, is this strictly part of tx-relay policy ?
28720:04 <sipa> ariard: yes
28820:04 <sipa> (it's even enabled on testnet!)
28920:04 <wumpus> ariard: everything that is not consensus: yes