validation/util: add GetTransactionFee

glozow commented at 4:09 pm on September 26, 2020: member

This is a followup to #19339, refining upon the 2xATMP approach. It adds a GetTransactionFee utility function which uses CheckTxInputs to get a transaction fee.

It also updates the Absurd Fee checking (see BroadcastTransaction in src/node/transaction.cpp):

Use GetTransactionFee to get the fee.
- If it fails, dry-run ATMP to return a proper validation result. (GetTransactionFee does not take responsibility for filling in TxValidationState although it may be modified).
- If it succeeds, check against max_tx_fee. If fee is too high, just return MAX_FEE_EXCEEDED.
If fee is ok, call ATMP to submit to mempool. Broadcast.

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 TxValidationResult here. If the results don’t match; i.e. GetTransactionFee fails but ATMP somehow succeeds (which should never happen), we don’t risk accidentally allowing an absurd fee through.

[rpc/node] check for high fee before ATMP in clients

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.

b1ea8a00f9

scripted-diff: update max-fee-exceeded error message to include RPC

-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-

4568fa37f7

[validation] ignore absurdfee arg in ATMP b635b5d51d

[validation] Remove absurdfee from accepttomempool

Mempool behavior should not be user-specific.
Checking that txfee is acceptable should be
the responsibility of the wallet or client, not
the mempool.

22ebb7fda0

[validation] add GetTransactionFee

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

6831972c09

glozow commented at 4:29 pm on September 26, 2020: member

@sdaftuar @jnewbery following up on our discussion about GetTransactionFee here!

DrahtBot added the label P2P on Sep 26, 2020

DrahtBot added the label RPC/REST/ZMQ on Sep 26, 2020

DrahtBot added the label Utils/log/libs on Sep 26, 2020

DrahtBot added the label Validation on Sep 26, 2020

DrahtBot commented at 9:07 pm on September 26, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19753 (p2p: don’t add AlreadyHave transactions to recentRejects by troygiorshev)
#19645 (Allow updating mempool-txn with cheaper witnesses by ariard)
#19498 (Tidy up ProcessOrphanTx by jnewbery)
#19438 (Introduce deploymentstatus by ajtowns)

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.

fanquake removed the label P2P on Sep 29, 2020

DrahtBot commented at 3:34 pm on September 30, 2020: member

🐙 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”.

DrahtBot added the label Needs rebase on Sep 30, 2020

in src/node/transaction.cpp:60 in 6831972c09

60+            Optional<CAmount> fee = GetTransactionFee(*node.mempool, state, *tx);
61+            if (fee == nullopt) {
62+                // Failed GetTransactionFee indicates a validation failure.
63+                // Use ATMP with test_accept to return the validation error.
64+                AcceptToMemoryPool(*node.mempool, state, tx,
65+                    nullptr /* plTxnReplaced */, false /* bypass_limits */, true /* test_accept */);

LarryRuane commented at 4:23 pm on September 30, 2020:

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.

glozow commented at 5:45 pm on October 1, 2020:

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 🤔

LarryRuane commented at 4:41 pm on September 30, 2020: contributor

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.

glozow commented at 5:43 pm on October 1, 2020: member

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.

We just introduced the fee return value in #19940 and the hope is that, for testmempoolaccept, we can return more fee information gathered from ATMP like modified fees as well. We get this information for free, so I think it’s cleaner to just use ATMP. I don’t think it’s possible to separate fees since it’s a big part of policy.

LarryRuane commented at 8:06 pm on October 4, 2020: contributor

ACK 6831972c09837c1a06034eaf63dfc4f3a6f0db6b (last commit only), provisional since this PR needs rebasing onto latest #19339.

laanwj added this to the milestone 22.0 on Oct 28, 2020

glozow closed this on Nov 19, 2020

DrahtBot locked this on Feb 15, 2022

validation/util: add GetTransactionFee #20025

Conflicts