validation/util: add GetTransactionFee #20025
Conversation
Check absurd fee in BroadcastTransaction and RPC, return TransactionError::MAX_FEE_EXCEEDED instead of TxValidationResult::TX_NOT_STANDARD because this is client preference, not a node-wide policy.
-BEGIN VERIFY SCRIPT- sed -i 's/Fee exceeds maximum configured by \-\maxtxfee/Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)/g' src/util/error.cpp test/functional/rpc_fundrawtransaction.py test/functional/rpc_psbt.py test/functional/rpc_rawtransaction.py test/functional/wallet_bumpfee.py test/functional/wallet_create_tx.py -END VERIFY SCRIPT-
Mempool behavior should not be user-specific. Checking that txfee is acceptable should be the responsibility of the wallet or client, not the mempool.
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
There was a problem hiding this comment.
Could this PR also remove the fee return value from ATMP? I think all you'd need to do is call GetTransactionFee() from the testmempoolaccept rpc handler (before the existing call to ATMP(test_accept=true)). That would increase the lines-of-code count slightly, but is cleaner and creates even more separation between the fee stuff and ATMP.
| // Failed GetTransactionFee indicates a validation failure. | ||
| // Use ATMP with test_accept to return the validation error. | ||
| AcceptToMemoryPool(*node.mempool, state, tx, | ||
| nullptr /* plTxnReplaced */, false /* bypass_limits */, true /* test_accept */); |
There was a problem hiding this comment.
I may be missing something, but I think this can be simplified further by dropping the call to ATMP here. Suppose the transaction has two problems, one that GetTransaction() catches and another that's outside its scope. GetTransactionFee() will return a failure and presumably set state appropriately (I didn't verify this, but if it doesn't, that's a bug that should be fixed). I don't see what's wrong with just returning that error.
It's true that by calling ATMP here, state may get set to a different error, but it's not clear to me why that's a better error to return.
There was a problem hiding this comment.
ATMP should be the authority on validation; I don't want to trust anything else with filling in TxValidationResult. I think I could make it a little cleaner and have GetTransactionFee just not touch state, but CheckTxInputs needs it 🤔
We just introduced the |
This is a followup to #19339, refining upon the 2xATMP approach. It adds a
GetTransactionFeeutility function which usesCheckTxInputsto get a transaction fee.It also updates the Absurd Fee checking (see
BroadcastTransactionin src/node/transaction.cpp):GetTransactionFeeto get the fee.GetTransactionFeedoes not take responsibility for filling inTxValidationStatealthough it may be modified).max_tx_fee. If fee is too high, just returnMAX_FEE_EXCEEDED.ATMP is called at most 1 time. I think it's best to dry-run ATMP in the failure case because ATMP should be the authority on validation, i.e. be responsible for filling in
TxValidationResulthere. If the results don't match; i.e.GetTransactionFeefails but ATMP somehow succeeds (which should never happen), we don't risk accidentally allowing an absurd fee through.