validation: re-delegate absurd fee checking from mempool to clients #19339

glozow · 2020-06-20T19:34:27Z

Picked up from #15810. Add separate fee-checking logic for clients that need to enforce max fee rates, then remove the absurdFee logic from ATMP.

ATMP's nAbsurdFee argument is used to enforce user-specific behavior (it is not policy since it isn't applied consistently: it is only ever used in RPC and wallet, and set to 0 everywhere else internally). It should be removed from AcceptToMemoryPool because (1) validation results/mempool behavior should not be user-specific and (2) enforcing a max fee rate should be the responsibility of the client instead of the mempool.

Note: this PR does not intend to remove protection from high fees, just re-delegate the responsibility to clients.

luke-jr · 2020-06-20T19:39:06Z

Absurd fees aren't even close to the only policy being enforced in ATMP... It doesn't make sense to split just this one out.

glozow · 2020-06-20T19:42:21Z

Absurd fees aren't even close to the only policy being enforced in ATMP... It doesn't make sense to split just this one out.

@luke-jr Sorry 😅 I'm a beginner so I don't know very much. Do you mean that there are other things that should be taken out too, or that it's not worth doing so at all?

luke-jr · 2020-06-20T19:45:27Z

It might be worth doing, but I'm not sure if the costs outweigh the benefits, and if/when done, it should be splitting all policy from all consensus checks, not just one policy from the rest.

luke-jr · 2020-06-20T19:50:33Z

Check out 0.20...luke-jr:sendraw_force-0.20+knots#diff-24efdb00bfbe56b140fb006b562cc70bR578

This changes all the policy checks to conditionals, while leaving the non-policy checks as strictly enforced. So a split would be on the same criteria. Note that reordering stuff may not always be a good idea, however...

DrahtBot · 2020-06-20T19:54:24Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: don't add AlreadyHave transactions to recentRejects #19753 (p2p: don't add AlreadyHave transactions to recentRejects by troygiorshev)
Allow updating mempool-txn with cheaper witnesses #19645 (Allow updating mempool-txn with cheaper witnesses by ariard)
External signer support - Wallet Box edition #16546 (External signer support - Wallet Box edition by Sjors)

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.

glozow · 2020-06-20T19:57:38Z

@luke-jr okay, I see what you're saying. Agree that refactoring consensus/policy should be as a whole + may or may not be worth it. I could be completely mistaken, but I thought that this was more... a user's preference rather than node's fee policy, since it's pretty much only used in RPC and wallet. It seems to be set to 0 in other places?

JeremyRubin · 2020-06-20T20:59:57Z

I think you're correct in the abstract on this one @gzhao408, but concretely this behavior is probably desirable as it's a belt-and-suspenders check of preventing things getting into the mempool that are almost certainly a mistake.

Even though mining/accepting these things should be done, and our node shouldn't really do 'deep transaction inspection', the case of absurd fees is pretty clearly accidental, so we add this redundant check (which can be disabled afaict via allowhighfees) even though clients should ultimately be responsible for this and not rely on absurd fees being rejected.

I think if you wanted to move the logic somewhere else (e.g., to just RPC layer) you could do that too, but the most convenient place for that logic is in ATMP as we load the required context to check fees there already.

Client side absurd fee problems should generally improve post-taproot & with the segwit fixes as signers should be more aware of how much fees their txn is spending as they can be made aware of all the inputs values they are signing for.

luke-jr · 2020-06-20T21:21:10Z

@gzhao408 You may be right - it's a good point that we don't enforce it on other stuff. Maybe we should really be checking it before even calling ATMP.

@JeremyRubin This is needed for GUI transactions too, at least.

JeremyRubin · 2020-06-20T21:31:44Z

One approach might be to introduce a redundant layer of logic to all the places we actually want to perform the absurdfee check, and then in a follow up PR (or commit if you want to do it in one), remove the now redundant ATMP check. I agree that ATMP is later than ideal for checking this property.

achow101 · 2020-06-20T22:08:37Z

I agree with removing the absurd fee check from ATMP, but we should still have it somewhere instead of removing it completely. Since it only is used for transactions sent by the node, the ideal place to move this to would be BroadcastTransaction in src/node/transaction.cpp. The RPC and wallet (GUI goes through the wallet) both use BroadcastTransaction.

jnewbery · 2020-06-22T23:08:49Z

Concept ACK! This isn't policy, it's wallet/RPC behaviour (and as such, belongs in the wallet/RPC).

NicolasDorier · 2020-06-23T00:49:04Z

Concept ACK

ariard · 2020-07-24T15:11:36Z

Concept ACK,

This check should be part of a consistent wallet tx-sanitization policy, i.e a set of checks protecting users funds against harmful actions before authorizing spend. Some signers already do this like Coincard. We could envision Core implementing the same kind of logic behind ScriptPubKeyMan::SignTransaction, with user settings. Now do this policy should apply to transaction flowing through our node but not originating from our wallet (sendrawtransaction) ? That's harder to decide on because in some cases an absurd fee may have a game-theory rational (a LN justice transaction burning everything in fees before CSV delay expiration).

Overall, absurd fee protection is already enforced on its own in CreateTransaction (grep m_default_max_tx_fee) but blindly removing from mempool checks would be a behavior change as it won't be applied anymore to sendrawtransaction/GUI transactions. So going forward with this PR check should be moved inside BroadcastTransaction to conserve behavior IMO.

With regards to untangling policy/consensus checks I think that should be a distinct conversation, order of them matter for anti-DoS reasons.

glozow · 2020-07-25T20:48:30Z

This builds on #19093. If that one is merged first then I’ll rebase this; if this one gets more review attention, @rajarshimaitra is listed as the author of the base commit here so hopefully that’s ok.

Per y’all’s wonderful advice, I moved the absurd fee checking into BroadcastTransaction and as an extra step in testmempoolaccept. Calling this a TransactionError::MAX_FEE_EXCEEDED seemed more appropriate, since that’s what the wallet uses. I still don’t think it’s policy since it’s empty for transactions not originating from the node’s own wallet. Whether it should be policy would a separate and interesting discussion 👀 .

There’s a couple intermediate stages to (1) test_accept and check fee before submitting (7dc55a5) and (2) keep absurd fee but ignore it in validation (40703b1) to show that the behavior is the same.

jnewbery · 2020-07-27T10:58:31Z

Approach ACK. This looks great. Thanks Gloria!

maflcko

Concept ACK 8784930, left some questions 😈

Show signature and timestamp

Signature:

Timestamp of file with hash 41bb2c8bffc4df4e4705ffe7775dabf9aa5e03d062a86e3f08624699d44592ad -

src/node/transaction.cpp

test/functional/rpc_rawtransaction.py

maflcko · 2020-10-05T08:51:27Z

test/functional/rpc_rawtransaction.py

@@ -456,9 +456,9 @@ def run_test(self):
        # Thus, testmempoolaccept should reject
        testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']], 0.00001000)[0]
        assert_equal(testres['allowed'], False)
-        assert_equal(testres['reject-reason'], 'absurdly-high-fee')
+        assert_equal(testres['reject-reason'], 'max-fee-exceeded')


question in commit f907f4b:

Any reason to change the reject-reason?

I wanted to use the same reject reason that wallet gives when a fee is too high, which is TransactionError::MAX_FEE_EXCEEDED. Also a little sanity check for all the absurdly-high-fee tests.

Not sure if I understand this. MAX_FEE_EXCEEDED returns the string "Fee exceeds maximum configured by user", not "max-fee-exceeded".

As there is no reject code, RPC users might match on the reject reason, which seems brittle, but if there is no convincing argument to change, I'd prefer to leave it as is.

edit: probably not too much of a deal

You're right, yeah. I guess this is an arbitrary change to make the error messages more similar 😅

src/rpc/rawtransaction.cpp

src/validation.cpp

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.

jnewbery · 2020-10-05T12:49:15Z

utACK b048b27

maflcko · 2020-10-05T13:10:16Z

re-ACK b048b27 , only change is squashing one commit 🏦

Show signature and timestamp

Signature:

Timestamp of file with hash ef381a3560ba63d06ec8d836ebc9b3379cdaab5ad0d2d7c5ddbfb38d235c2c69 -

LarryRuane · 2020-10-05T18:46:39Z

re-ACK b048b27

instagibbs · 2020-10-07T01:55:16Z

concept ack, this will require release notes since people tend to use sending failure codes/text in fine-grained ways and it may break downstream things

instagibbs

utACK b048b27

please don't address my nit unless you have to rebase or something.

instagibbs · 2020-10-07T02:04:18Z

src/node/transaction.cpp

-            } else {
-                return TransactionError::MEMPOOL_ERROR;
+        CAmount fee{0};
+        if (max_tx_fee) {


Suggested change

if (max_tx_fee) {

if (max_tx_fee > 0) {

fanquake · 2020-10-07T03:00:53Z

please don't address my nit unless you have to rebase or something.

@glozow can follow up with some release notes and address any remaining nits. Either in a new PR, or possibly as part of #20025.

maflcko · 2020-10-07T07:15:22Z

Instead of mentioning that absurdly-high-fee changed into max-fee-exceeded, I'd rather just change it back and prevent the release notes from bloating.

Though, the reason (my reason) testmempoolaccept doesn't return an error code because we can't really guarantee stability of failure reasons across versions of Bitcoin Core. Similarly the string can't be expected to be stable. So either we assume that and don't mention the string change in the release notes, or we mention in the RPC help doc that the strings shouldn't be assumed to be stable. In both cases, the release notes don't mention the change.

instagibbs · 2020-10-07T12:15:20Z

Might be worth opening an issue on the topic because apparently the codes aren't enough for downstream users. Recent breaks include LND, probably more.

…

On Wed, Oct 7, 2020, 3:15 AM MarcoFalke ***@***.***> wrote: Instead of mentioning that absurdly-high-fee changed into max-fee-exceeded, I'd rather just change it back and prevent the release notes from bloating. Though, the reason (my reason) testmempoolaccept doesn't return an error code because we can't really guarantee stability of failure reasons across versions of Bitcoin Core. Similarly the string can't be expected to be stable. So either we assume that and don't mention the string change in the release notes, or we mention in the RPC help doc that the strings shouldn't be assumed to be stable. In both cases, the release notes don't mention the change. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#19339 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMAFU67TYMCCYJAG63SDSTSJQIRVANCNFSM4ODQNOOQ> .

maflcko · 2020-10-07T12:17:37Z

Mind sharing a link on further information how LND was broken by this?

instagibbs · 2020-10-07T12:22:37Z

Not this but text changes in general for mempool rpc messages. I'll dig through my notes sometime when I have the time

…

On Wed, Oct 7, 2020, 8:17 AM MarcoFalke ***@***.***> wrote: Mind sharing a link on further information how LND was broken by this? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#19339 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMAFU3CRQUFMVDXYTTEOBDSJRL7JANCNFSM4ODQNOOQ> .

glozow · 2020-10-08T21:09:20Z

fwiw I think max-fee-exceeded is a more accurate error message - we're checking the maxfeerate passed in by the user, absurd or not. I'm not sure how to determine whether something should be in release notes or not :/ I'll just open a PR and we can talk there

88197b0 [doc] release notes for max fee checking (gzhao408) c201d73 style and nits for fee-checking in BroadcastTransaction (gzhao408) Pull request description: Pretty trivial... addresses some tiny comments from #19339. Also fixes a docs typo from #19940 and adds a release note about the error message change for testmempoolaccept. ACKs for top commit: achow101: ACK 88197b0 MarcoFalke: cr re-ACK 88197b0 Tree-SHA512: fff16d731426b9b4db5222df02633983402f4c7241551eec98bb1554145dbdc132f40ed8ca4abd5edcebe1f4d1e879fb6d11bd91730604f6552c10cdf65706a1

88197b0 [doc] release notes for max fee checking (gzhao408) c201d73 style and nits for fee-checking in BroadcastTransaction (gzhao408) Pull request description: Pretty trivial... addresses some tiny comments from bitcoin#19339. Also fixes a docs typo from bitcoin#19940 and adds a release note about the error message change for testmempoolaccept. ACKs for top commit: achow101: ACK 88197b0 MarcoFalke: cr re-ACK 88197b0 Tree-SHA512: fff16d731426b9b4db5222df02633983402f4c7241551eec98bb1554145dbdc132f40ed8ca4abd5edcebe1f4d1e879fb6d11bd91730604f6552c10cdf65706a1

Summary: 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. This is a backport of [[bitcoin/bitcoin#19339 | core#19339]] [1/3] and [[bitcoin/bitcoin#20109 | core#20109]] bitcoin/bitcoin@8f1290c Depends on D10463 and D10474 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10464

Summary: This is a backport of [[bitcoin/bitcoin#19339 | core#19339]] [2/3] bitcoin/bitcoin@932564b Depends on D10464 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10465

Summary: Mempool behavior should not be user-specific. Checking that txfee is acceptable should be the responsibility of the wallet or client, not the mempool. This is a backport of [[bitcoin/bitcoin#19339 | core#19339]] [3/3] bitcoin/bitcoin@b048b27 Depends on D10465 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10466

DrahtBot added GUI P2P RPC/REST/ZMQ Tests Validation Wallet labels Jun 20, 2020

glozow changed the title ~~[WIP] mempool: Remove absurd fee logic from ATMP~~ [WIP] mempool: re-delegate absurd fee checking from mempool to clients Jul 1, 2020

glozow force-pushed the mempool-remove-absurdfee branch from 717faed to 5fe1b1e Compare July 25, 2020 20:46

glozow force-pushed the mempool-remove-absurdfee branch from 5fe1b1e to 47980f4 Compare July 25, 2020 20:53

rajarshimaitra mentioned this pull request Jul 26, 2020

RPC: Return transaction fee from testmempoolaccept #19093

Closed

glozow marked this pull request as ready for review July 26, 2020 13:35

glozow changed the title ~~[WIP] mempool: re-delegate absurd fee checking from mempool to clients~~ validation: re-delegate absurd fee checking from mempool to clients Jul 26, 2020

This was referenced Jul 26, 2020

Tidy up ProcessOrphanTx #19498

Merged

External signer support - Wallet Box edition #16546

Merged

DrahtBot mentioned this pull request Jul 29, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

maflcko reviewed Oct 5, 2020

View reviewed changes

glozow and others added 3 commits October 5, 2020 04:54

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

8f1290c

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.

[validation] Remove absurdfee from accepttomempool

b048b27

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

glozow force-pushed the mempool-remove-absurdfee branch from 8784930 to b048b27 Compare October 5, 2020 11:58

fanquake added the Needs release note label Oct 7, 2020

instagibbs approved these changes Oct 7, 2020

View reviewed changes

fanquake merged commit db88db4 into bitcoin:master Oct 7, 2020

fanquake mentioned this pull request Oct 7, 2020

Followup with potential downstream breakage after #19339 #20099

Closed

glozow mentioned this pull request Oct 8, 2020

Release notes and followups from 19339 #20109

Merged

maflcko removed the Needs release note label Oct 9, 2020

ariard mentioned this pull request Feb 2, 2021

refactor: return MempoolAcceptResult from ATMP #21062

Merged

glozow mentioned this pull request Feb 3, 2021

Modify testmempoolaccept to handle client's max_raw_tx_fee check failure #21074

Closed

maflcko mentioned this pull request Jun 22, 2021

test: Add functional test for replacement relay fee check #22310

Merged

bitcoin locked and limited conversation to collaborators Aug 14, 2022

validation: re-delegate absurd fee checking from mempool to clients #19339

validation: re-delegate absurd fee checking from mempool to clients #19339

Uh oh!

Conversation

glozow commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Jun 20, 2020

Uh oh!

glozow commented Jun 20, 2020

Uh oh!

luke-jr commented Jun 20, 2020

Uh oh!

luke-jr commented Jun 20, 2020

Uh oh!

DrahtBot commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

glozow commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Jun 20, 2020

Uh oh!

luke-jr commented Jun 20, 2020

Uh oh!

JeremyRubin commented Jun 20, 2020

Uh oh!

achow101 commented Jun 20, 2020

Uh oh!

jnewbery commented Jun 22, 2020

Uh oh!

NicolasDorier commented Jun 23, 2020

Uh oh!

ariard commented Jul 24, 2020

Uh oh!

glozow commented Jul 25, 2020

Uh oh!

jnewbery commented Jul 27, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

glozow Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Oct 5, 2020

Uh oh!

maflcko commented Oct 5, 2020

Uh oh!

LarryRuane commented Oct 5, 2020

Uh oh!

instagibbs commented Oct 7, 2020

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

fanquake commented Oct 7, 2020

Uh oh!

maflcko commented Oct 7, 2020

Uh oh!

instagibbs commented Oct 7, 2020 via email

Uh oh!

glozow commented Jun 20, 2020 •

edited

Loading

DrahtBot commented Jun 20, 2020 •

edited

Loading

glozow commented Jun 20, 2020 •

edited

Loading

maflcko Oct 5, 2020 •

edited

Loading