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.
Does this PR change any behavior or is it a pure refactor?
There was some discussion as to whether AbsurdFee is policy or a client
preference. What do you think, and how did you determine this?
Compare the implementations of the testmempoolaccept and
sendrawtransaction RPCs. What are the differences? (hint: test_accept)
What information do you need to check a transactionâs fee? At what point
would the node have this information on hand?
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?
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?
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?
<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)
<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
<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
<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.
<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?
<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)
<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"
<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?
<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
<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
<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
<ariard> michaelfolkson: yes but it's quite limited if your transactions don't propagate, unless you have preferential peering with all the same settings
<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
<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
<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)
<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
<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
<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.
<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?)
<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
<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?
<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)
<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)
<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
<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.
<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?
<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)
<jonatack> FWIW, in master it is now possible to set an explicit feerate in sentoaddress, sendmany, bumpfee, fundrawtransaction, and walletcreatefundedpsbt
<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.
<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)
<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
<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)
<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?
<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 ?
<robot-dreams> gzhao408: I think the wallet checks max fee in two places: `CreateTransactionInternal` and `SubmitMemoryPoolAndRelay`; the latter eventually calls into ATMP
<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.
<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?
<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.
<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.
<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.
<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?