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

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.

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.

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?

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.

Check out https://github.com/bitcoin/bitcoin/compare/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...

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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)
• #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.

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

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.

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

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.

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.

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

Concept ACK

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.

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 (https://github.com/bitcoin/bitcoin/pull/19339/commits/7dc55a5f4c2bc3ba3c301338ce75b25412365dd7) and (2) keep absurd fee but ignore it in validation (https://github.com/bitcoin/bitcoin/pull/19339/commits/40703b1d5185cd26626d9da1fb76c3babdcdd550) to show that the behavior is the same.

Approach ACK. This looks great. Thanks Gloria!

Rebased and refactored some of the commits to make it cleaner + easier to review in preparation for PR Review Club (Notes). Now's a perfect time to review if anyone has time :) thanks everybody!

Concept ACK! Makes a lot of sense. Builds on OSX 10.14, unit tests passed but I had one functional test tool_wallet not pass every time (not sure why, investigating...)

Concept ACK, Approach ACK, unit tests + all updated functional tests pass locally

I think the discussion of "what are the performance implications of calling ATMP twice" is very interesting. I looked up where BroadcastTransaction could be called, and here's what I found:

• sendrawtransaction RPC
• SubmitMemoryPoolAndRelay in the wallet
• PSBTOperationsDialog in the UI

In particular, I'm guessing BroadcastTransaction calls aren't too frequent (e.g. compared to relaying transactions in the P2P network), so the overhead of calling ATMP twice (especially since @gzhao408 showed that many of the verification results are cached) is OK.

Follow-ups for review club discussion:

What would be the tradeoffs / considerations involved with either of these alternative approaches?

1. Replace the nAbsurdFee parameter with some sort of generic clientSafetyChecks object that ATMP can call without knowing the client-specific details

2. Split up ATMP into two parts: (i) TestForMemoryPool performs all checks and returns a fee together with some context, (ii) AddToMemoryPool takes the context and finishes the process. This way, we can remove the test_accept parameter. The testmempoolaccept RPC could just call TestForMemoryPool, and then sendrawtransaction could do something like this:

context = TestForMemoryPool(...);
if (max_tx_fee && context.fee > max_tx_fee) {
	return TransactionError::MAX_FEE_EXCEEDED;
}

if (!AddToMemoryPool(*node.mempool, context, ...)) {
	// return error result
}

How will this change interact with taproot?

Given the "Client side absurd fee problems should generally improve post-taproot" comment above, will this code need to change again soon, or will "post-taproot" have multiple code paths, of which this is one of them?

Thanks for the review @robot-dreams :)

In particular, I'm guessing BroadcastTransaction calls aren't too frequent (e.g. compared to relaying transactions in the P2P network)

Nice observation; I'd like to highlight that BroadcastTransaction is used for transactions coming from clients as opposed to peers. Even if the slowdown were significant, it's very different from an inefficiency that may be exploited by random network peers.

Replace the nAbsurdFee parameter with some sort of generic clientSafetyChecks object that ATMP can call without knowing the client-specific details

The idea here is that we want to separate client-specific details from ATMP. In general, the validation result returned by ATMP shouldn't depend on which client submitted it (my opinion).

Split up ATMP into two parts: (i) TestForMemoryPool performs all checks and returns a fee together with some context, (ii) AddToMemoryPool takes the context and finishes the process.

I'm not quite sure I fully understand this idea, but it seems pretty similar to what happens right now in ATMP. Most of it can be seen in AcceptSingleTransaction which has 4 components: the 3 *Checks (validation and fill in the fee), and Finalize (try to add it to the mempool). A test-accept returns before Finalize; a "real" accept runs through everything (and uses what's cached if possible). Other than repeating checks (which is the safe thing to do), this seems equivalent to what you're suggesting.

In the last commit log, there's no need to state which commit hash the commit was cherry-picked from.

@gzhao408 I did some cleanup and pushed it to https://github.com/robot-dreams/bitcoin/commits/mempool-remove-absurdfee ; feel free to grab any of the updated commits if you'd find them helpful, but of course no pressure to do so!

• Rebase and fix merge conflict
• Change std::move(tx) to tx
• Change PR number in release notes
• Change release notes about testmempoolaccept from "if transaction is accepted" to "if transaction would be accepted"
• Fix typo in commit message (udpate -> update)
• Remove "cherry-picked from" from commit message

Concept ACK

Typo in c763343c0064fe88aeef15eecd6273ccf2746fcc commit message: "udpate"

I think this would be easier to review if the renaming commit was a scripted-diff and all the other commits (except the one from #19093) would be squashed. I found this easier to review looking at the final diff than going through the commits.

Rebased and addressed comments. Apologies, I'm wrestling with the scripted-diff linter :'(

I also edited the first commit so that I'm only picking the ATMP changes in #19093 (which is more RPC-focused) - these two PRs are now uncoupled.

Rebased and addressed comments. Apologies, I'm wrestling with the scripted-diff linter :'(

The issue is GNU vs BSD sed. I'm assuming you are on macOS (BSD sed), and you've written a command that is incompatible with GNU sed (used by Travis). The fix here is to drop the '' after -i, which is the part that's incompatible. Unfortunately portability when using -i isn't trivial.

When testing sed commands on macOS I'd suggest brew installing and using gsed.

The issue is GNU vs BSD sed. I'm assuming you are on macOS (BSD sed), and you've written a command that is incompatible with GNU sed (used by Travis). The fix here is to drop the '' after -i, which is the part that's incompatible. Unfortunately portability when using -i isn't trivial.

This issue has tripped lots of people up (including me). Perhaps commit-script-check.sh should detect if a scripted diff uses the BSD sed syntax and print a more helpful error message.

Concept ACK

Feel free to ignore these, but they came to mind while reviewing:

• with the current structure, reviewing commit-by-commit is less clear/more laborious than reviewing the overall diff; would suggest combining some of the steps

• consider harmonising the commit titles between e.g. [rpc] and validation:

• if a field (fee) is added to an RPC, you'll want to update the RPCHelpman, write a release note, and add fee test assertions.

Concept ACK, Approach ACK

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.

As was pointed out during the Bitcoin Core PR review club session and John earlier here absurd fee checking is not policy and so splitting policy from consensus checks is not the goal in this PR.

(Using @gzhao408 definition of policy here as "a node’s extra standards for validation in addition to consensus")

Rebased, addressed @jonatack's review comments, fixed the sed problem. I also squashed the commits that add the client checking so hopefully the commits are more clear. This is ready for another look!

I'm not sure what the appveyor failure is...? It doesn't seem related to this PR's changes 🤔

I'm not sure what the appveyor failure is...? It doesn't seem related to this PR's changes 🤔

Yes, appveyor is down ATM, #19839 was opened today as a fix.

utACK d0f10e25afbbbcfc2c46300cf7112ce4f303987d (but needs rebase)

Rebased

utACK 6bbda0e5d4

Verified git range-diff 4afff49265~..d0f10e25af 9ec325e2e9~..6bbda0e5d4

Code Review ACK 6bbda0e

utACK 6bbda0e5d4c63d4923ddc5ef6fcf4d8145a5e8c7 modulo #19339 (review)

The CI failures seem unrelated--the travis job logs no error afaict and cirrus timed out.

I agree with the goal of this PR, but invoking ATMP twice really feels like the wrong solution. Could we just add a function to validation.cpp that returns the fee of a transaction (similar to what @ariard suggested in #19339 (review))? That would be helpful more generally, for instance so that our wallet could someday tell us what the feerate is of incoming transactions.

invoking ATMP twice really feels like the wrong solution

Can you give a bit more concrete reason why this feels like the wrong solution to you? It seems like a very elegant way to utilize the existing code and avoid repetition. Note that this approach was suggested by @MarcoFalke in #15810 and got approach ACKs there from @sipa, @gmaxwell and me.

Note that this approach was suggested by @MarcoFalke in #15810 and got approach ACKs there from @sipa, @gmaxwell and me.

Thanks for the context of #15810. I agree that this PR's approach is better in some ways than the one there, by making the check in one place, at the time we try to call ATMP, rather than spread across multiple places in our rpc/wallet code.

However ATMP does so much more than just calculate the fee of a transaction, so if that's the only reason we're invoking it twice it seems like overkill. How much slower does this make the rpc call, particularly if a user is working with large transactions? Re-validating a transaction, even when its signature checks have been cached, can still be slow for large transactions (especially non-segwit transactions) due to needing to rehash the data for those signature checks again.

If all we need is to look up inputs in the utxo set or in the mempool that is much quicker; so that aspect of #15810 I think is preferable to what we have here. I think the best outcome would be to add a function to validation that does the very simple thing of returning the fee of a given transaction (if calculable from the utxo and mempool, otherwise return an error); and where we call ATMP twice in this patch we could instead call the fee calculator and, on success and passing the absurd fee check, invoke ATMP.

This would result in the cleanest outcome for ATMP (slight improvement over what is proposed here is to get rid of the optional fee_out argument), and would be most performant. The downside is the small amount of logic duplication in having a way to calculate fee in a single place outside of ATMP, but I think the performance benefit would outweigh that. (By contrast, the version of #15810 that I reviewed just now had this check reimplemented in 3 places through the rpc/wallet code, so I wonder if that aspect is what reviewers were objecting to?)

Thanks for your input @sdaftuar! I agree that the fee check should be minimal.

Some concerns I’ve heard about the 2x ATMP and will try to summarize here:

• Performance: I did some bookkeeping by adding some logs to see how much use we get out of the signature and script caches (comment). We get quite a bit of reuse although, as you said, the hashing is also expensive. I assumed that a small performance hit wouldn't be too egregious in a function only used by clients.
• Unnecessary ATMP validation logic when all we need is the fee: I definitely see this. Adding another function would probably have to do many of the sanitization steps as PreChecks, which isn’t too expensive but would also be a little redundant. I think that just calling PreChecks would be sufficient if we could do that, but perhaps we would want to uncache the coins as well (handled in ATMPWithTime afterward)? As I thought about the “minimum amount” we need, it started to look more and more similar to ATMP, even if it's just adding it in 1 place.

Some reasons for using 2x ATMP:

• Minimal new code
• In the case where a transaction has both, say, an invalid script and absurd fee, I think it would make more sense to return the more serious validation failure rather than max-fee-exceeded.
• If the minimal fee-checking function returns an error, should we try again with ATMP or trust its result and return failure? If the latter, unless it's comprehensive, I was worried I'd accidentally introduce a bug where BroadcastTransaction is often failing due to imperfect fee-checker rather than real invalidity. If I were to do it this way, I'd probably reuse PreChecks.

This would result in the cleanest outcome for ATMP (slight improvement over what is proposed here is to get rid of the optional fee_out argument)

For some context, #19093 added fee_out to return the fee to the user in testmempoolaccept. I built on top of that since it suited my needs as well.

I followed this approach under others' guidance (as @jnewbery mentioned), but also thought about the case of adding a separate check. Since you mentioned a separate fee-checking function could be helpful in other cases as well, I'm open/interested in implementing this. I just think that many of the things we do in ATMP would need to be repeated there as well. Would you agree that this is the case, and would it still be appropriate to add this?

EDIT: Just to be clear, I maintain my Code Review ACK. That said it would be great if we open an issue or else to discuss and track further improvements in this area of the code, there is room for it.

If the problem solves by this PR is "How to disentangle wallet sanity max fee from mempool policy checks ?", I'm not sure the current approach achieves it as types of checks are still blurred, i.e a policy failure will mask the wallet max fee.

In the case where a transaction has both, say, an invalid script and absurd fee, I think it would make more sense to return the more serious validation failure rather than max-fee-exceeded.

A failure is a failure, what heuristic do you follow to sort the severity of them ? Your transaction can still succeed all mempool rules, but still not broadcast due to its feerate inferior to all your peer's feefilter. Gathering checks by component which are responsible of them a) should ease review reasoning b) should enhance performance.

I was worried I'd accidentally introduce a bug where BroadcastTransaction is often failing due to imperfect fee-checker rather than real invalidity. If I were to do it this way, I'd probably reuse PreChecks.

Note that this approach isn't bug-free, see above if we refactor the main lock tacking. Code duplication is a concern. The fee computation job is partially done by Consensus::CheckTxInputs which can be reused. What we can do is refactor internally PreChecks to extract the outpoints gathering logic in its own method thus a new FeeCalculator just have to wrap both and should be consistent with ATMP.

Also, any wallet check shouldn't belong to BroadcastTransaction but on the other side of the interface in src/wallet/. IIRC the design goal of BroadcastTransaction is just to be a wrapper between nodes components.

The current PR is worthy on its own to at least separate logically wallet from mempool checks and the reason I gave my ACK. But more I think about it more I guess we would like to have a clean FeeCalculator method. It's more costly in reviewing time but we could reuse it for exacting feerate of Taproot witnesses (which may come with their own feerate accounting rules, hard to reimplement correctly by client) or build an automatic RBF logic in the wallet (and you need to cache feerate across attempts).

Thanks @ariard! I appreciate that your ACK is not being withdrawn.

A failure is a failure, what heuristic do you follow to sort the severity of them ?

We have a few defined categories that are relevant here, afaik:

• Validation/Consensus failures (e.g. an invalid script or signature, etc)
• Policy fails (e.g. minimum feerate for relay)
• Client preference (e.g. absurd fee)

I’ve [arbitrarily] sorted them in severity, based on the idea that consensus failure = nobody will accept, policy failure = this node won’t accept, client preference = this user won’t accept. So I meant that any validation failure would be more serious than a client preference not being met. That being said, I could be misguided to say that higher severity should take priority. But this PR doesn’t hinge on this idea.

If the problem solves by this PR is “How to disentangle wallet sanity max fee from mempool policy checks ?“, I’m not sure the current approach achieves it as types of checks are still blurred, i.e a policy failure will mask the wallet max fee.

Please let me know if I misunderstand your comment, but I don’t think the checks are “still blurred.” I would interpret blurring to be when code should only be responsible for checking mempool/policy also has a client check mixed in there, which was the case before this PR. Also confusing, it would return TX_NOT_STANDARD (should = policy error) for absurd fee. In terms of failure cases, let me enumerate them:

I’d say this is less blurred than before. Further, the wallet (before and after this PR) checks against max fee in CreateTransaction. Thus, afaik it catches absurd fees before ever trying to submit to mempool, and case 1 and 3 don’t actually happen. This PR is more relevant to the RPCs testmempoolaccept and sendrawtransaction, where ATMP was actually shouldering the fee checking responsibility.

I think we should discuss/implement a FeeCalculator method on a separate issue or PR if it isn’t a blocker here (although still waiting to hear from @sdaftuar on whether it is) :slightly_smiling_face:

I’ve [arbitrarily] sorted them in severity, based on the idea that consensus failure = nobody will accept, policy failure = this node won’t accept, client preference = this user won’t accept.

Sure, the set of parties interested to the rule violation is one axis of analysis. The one I would favor is ordering them by tightness, namely a set A is more tight than set B if transaction is accepted by B but not by A. So wallet's absurd fee is more strict than any consensus rules violation, even if local user is the only one interested. Concretely that gives you a heuristic to organize code in a way that if your transaction is rejected by set B (policy) it won't mask rejection by set A (client preference), as current code after the PR is doing by reusing ATMP.

I would interpret blurring to be when code should only be responsible for checking mempool/policy also has a client check mixed in there, which was the case before this PR.

Correct if I'm wrong but before/after this PR, if your transaction violates MIN_STANDARD_TX_NONWITNESS_SIZEand absurd fee, a user will only see reject message for the first violation and might still fail its client preferences after correcting the policy violation. Layering cleanly errors means a user is less confused about what to do in case of failure.

This PR is more relevant to the RPCs testmempoolaccept and sendrawtransaction, where ATMP was actually shouldering the fee checking responsibility.

Yep and encapsulating the code further to be reused across RPCs, wallets or even as a library sounds like future steps :)

I think this patch is basically what the fee calculator would need to look like:

diff --git a/src/validation.cpp b/src/validation.cpp
index 58af8744d90..87685b44f6b 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1059,6 +1059,30 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs
 
 } // anon namespace
 
+
+// Calculate the fee of a transaction, by looking up inputs from the mempool if
+// necessary. Return true if the fee was successfully calculated, or false if
+// an error with the transaction was discovered.
+// Success does not imply that the transaction is valid -- but if it is valid,
+// then the fee returned is correct.
+bool GetTransactionFee(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, CAmount *fee_out)
+{
+    LOCK2(cs_main, pool.cs);
+
+    CCoinsViewMemPool viewmempool(&::ChainstateActive().CoinsTip(), pool);
+    CCoinsViewCache view(&viewmempool);
+    view.GetBestBlock();
+
+    *fee_out = 0;
+
+    // Fees don't make sense for coinbase transactions
+    if (tx->IsCoinBase()) return false;
+    // Re-use the fee calculation in CheckTxInputs, which also validates that
+    // the fee calculation is correct (ie that we don't overflow)
+    if (!Consensus::CheckTxInputs(*tx, state, view, GetSpendHeight(view), *fee_out)) return false;
+    return true;
+}
+
 /** (try to) add transaction to memory pool with a specified acceptance time **/
 static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
                         int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
diff --git a/src/validation.h b/src/validation.h
index cac9473c7a5..9833b9d9e05 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -199,6 +199,8 @@ void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
 /** Prune block files up to a given height */
 void PruneBlockFilesManual(int nManualPruneHeight);
 
+bool GetTransactionFee(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, CAmount *fee_out);
+
 /** (try to) add transaction to memory pool
  * plTxnReplaced will be appended to with all transactions replaced from mempool **/
 bool AcceptToMemoryPool(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,

Well it should probably fill in the TxValidationState for a coinbase transaction rather than just return false, but aside from that I don't believe we need more than this. The semantics I suggest are: if the transaction is valid, then the fee calculated here is definitely correct; but if the transaction is invalid you don't know. I think this is sufficient for many purposes, including the proposal here.

I was giving this more thought and I can also see a philosophical argument for fees only being meaningful if a transaction is valid, but validity is a tricky idea, because mempool acceptance couples various policy and anti-DoS ideas in with consensus validity. So something like #19093 is reasonable to me for that reason, and building on that to do the absurd fee check for testmempoolaccept seems reasonable. However for other use cases in our wallet/rpc code, I think pulling out the fee calculation separately would be preferable to running ATMP twice. And it seems to me that this kind of function should be more generally useful (for example I could imagine wanting a way to calculate the fee of a rawtransaction, without having to run it through ATMP).

I think the way you'd use this is, if you're doing the maximum fee check, invoke this function and see if it succeeds; if not you give up on the transaction. If it succeeds and the fee is too high, again give up. Otherwise, invoke ATMP() and return the success or failure from that.

In the case where a transaction has both, say, an invalid script and absurd fee, I think it would make more sense to return the more serious validation failure rather than max-fee-exceeded.

I think this is hard to say; I suspect it's way more likely to get a max-fee-exceeded than an actual script or policy failure! It seems perfectly reasonable to me that a workflow for submitting a transaction is to first check the fees are sane, and then validate if that passes, so if our software does that too I think it's fine.

If the minimal fee-checking function returns an error, should we try again with ATMP or trust its result and return failure? If the latter, unless it's comprehensive, I was worried I'd accidentally introduce a bug where BroadcastTransaction is often failing due to imperfect fee-checker rather than real invalidity. If I were to do it this way, I'd probably reuse PreChecks.

With my proposed semantics, we wouldn't continue if there's an error -- the fee calculator I propose is only correct if there are no errors, otherwise all bets are off. I don't think using PreChecks() would be problematic, but I think it's easier and more concise to just use CheckTxInputs() as I suggest.

This PR updates several of the tests (both unit and functional), but, unless I'm missing it, it doesn't look like there's any verification of the modified behavior (asserting that transactions with high fees are forwarded with the PR but not without the PR). This would probably require a brand new test or tests. Have you considered doing that? I may be able to help with this, but not until next week at the earliest. What do you think?

Just pushed with a rebase and GetTransactionFee. Hope that you can take a look at it @sdaftuar; it's largely based on your comment. And @jnewbery @ariard @jonatack - diff of the first 5 commits is the same so I hope it's easier to re-review. Thank you so much :pray:

Thank you for your review @larryruane! Incorporated your only if(max_tx_fee) suggestion in https://github.com/bitcoin/bitcoin/pull/19339/commits/8c8b936dca571e4fd631c9a8a1306549072b449c

it doesn't look like there's any verification of the modified behavior (asserting that transactions with high fees are forwarded with the PR but not without the PR).

This behavior hasn’t been changed, high fee transactions are still not forwarded if from client and are forwarded if from peers :)

Thanks for your feedback @DrahtBot, addressed.

ACK 5c5fd76ad1ee68c3cd77436bad49041969cf1cb1 (three nonblocking suggestions)

I think the last commit is unnecessary, and I have a few questions/comments about the new code.

I felt that we had a lot of open questions regarding GetTransactionFee so I've opened a separate PR for review, #20025 (addressed @jnewbery's comments there as well).

This PR just re-delegates absurdfee checking to clients. The branch as it currently stands is pretty much the same as what was ACK'ed before, with a few small fixes.

For everyone who has reviewed this PR, I really appreciate your time so far! If you're still comfortable with these changes, please let me know. If this approach is unacceptable please let me know as well, I can always close this in favor of #20025 since it's based on this branch :) in general I'd just like to discuss GetTransactionFee there instead of here.

utACK 22ebb7fda03fd18361066b4ce92eeed009542976 This PR removes six instances of 0 /* nAbsurdFee */ which, just by itself, is a nice cleanup! (In addition to the other more important reasons.) Thanks for the PR!

I ran the unit tests but didn't do any careful manual testing.

A few more non-blocking mostly-formatting suggestions (most of these were from running contrib/devtools/clang-format-diff.py):

Rebased

utACK 8784930

<details> <summary>git range-diff 22ebb7f~4..22ebb7f 8784930~4..87849300</summary>

1:  b1ea8a00f ! 1:  f907f4b93 [rpc/node] check for high fee before ATMP in clients
    @@ src/node/transaction.cpp: TransactionError BroadcastTransaction(NodeContext& nod
      
     
      ## src/rpc/rawtransaction.cpp ##
    -@@ src/rpc/rawtransaction.cpp: static UniValue testmempoolaccept(const JSONRPCRequest& request)
    +@@ src/rpc/rawtransaction.cpp: static RPCHelpMan testmempoolaccept()
      
          TxValidationState state;
          bool test_accept_res;
2:  4568fa37f = 2:  b0ec04de0 scripted-diff: update max-fee-exceeded error message to include RPC
3:  b635b5d51 ! 3:  6a4a7c642 [validation] ignore absurdfee arg in ATMP
    @@ Commit message
         [validation] ignore absurdfee arg in ATMP
     
      ## src/rpc/rawtransaction.cpp ##
    -@@ src/rpc/rawtransaction.cpp: static UniValue testmempoolaccept(const JSONRPCRequest& request)
    +@@ src/rpc/rawtransaction.cpp: static RPCHelpMan testmempoolaccept()
                  nullptr /* plTxnReplaced */, false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true, &fee);
          }
      
4:  22ebb7fda ! 4:  8784930df [validation] Remove absurdfee from accepttomempool
    @@ src/bench/block_assemble.cpp: static void AssembleBlock(benchmark::Bench& bench)
          }
     
      ## src/net_processing.cpp ##
    -@@ src/net_processing.cpp: void PeerManager::ProcessOrphanTx(std::set<uint256>& orphan_work_set, std::list<
    -         TxValidationState orphan_state;
    +@@ src/net_processing.cpp: void PeerManager::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
    +         TxValidationState state;
    +         std::list<CTransactionRef> removed_txn;
      
    -         if (setMisbehaving.count(fromPeer)) continue;
    --        if (AcceptToMemoryPool(m_mempool, orphan_state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {
    -+        if (AcceptToMemoryPool(m_mempool, orphan_state, porphanTx, &removed_txn, false /* bypass_limits */)) {
    +-        if (AcceptToMemoryPool(m_mempool, state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {
    ++        if (AcceptToMemoryPool(m_mempool, state, porphanTx, &removed_txn, false /* bypass_limits */)) {
                  LogPrint(BCLog::MEMPOOL, "   accepted orphan tx %s\n", orphanHash.ToString());
                  RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), m_connman);
    -             for (unsigned int i = 0; i < orphanTx.vout.size(); i++) {
    +             for (unsigned int i = 0; i < porphanTx->vout.size(); i++) {
     @@ src/net_processing.cpp: void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
              // (older than our recency filter) if trying to DoS us, without any need
              // for witness malleation.
    @@ src/node/transaction.cpp: TransactionError BroadcastTransaction(NodeContext& nod
      
     
      ## src/rpc/rawtransaction.cpp ##
    -@@ src/rpc/rawtransaction.cpp: static UniValue testmempoolaccept(const JSONRPCRequest& request)
    +@@ src/rpc/rawtransaction.cpp: static RPCHelpMan testmempoolaccept()
          {
              LOCK(cs_main);
              test_accept_res = AcceptToMemoryPool(mempool, state, std::move(tx),

</details>

utACK 8784930dfaf4b981be6a3670c4298de2226a3989

I left a couple of style suggestions inline, which you should ignore unless you touch this branch again.

Concept ACK 8784930dfaf4b981be6a3670c4298de2226a3989, left some questions 😈

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Concept ACK 8784930dfaf4b981be6a3670c4298de2226a3989, left some questions 😈
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjoWAwAwUFIuJljEL0JfJpPE2ETBPrIL0soXgmLHNBRHXgwPZ10WH2y6TrXAR7t
Xo9NESbW4Qp17O/PJWn9GZ/Oxl6UjbE9KF6XMqptXaUC0Jt2eGbNdvoHkVyj2Mqc
+pp76rgrKd+AU0G6YObPkDy0e8RCG66PX8u2JfLmCYoQA+cRcblg4GEFqWlBTZGw
43jQ9uGLCC3ShF2tbFfFJVeTRF4ywnyUGnmWno/vtqlWxGRtYSBdf8WSseUMHJz7
OzsWnvPyOajSCIFEHI2rJ7rutEXJQ4RXQMwCwC4RMkLTL5rsm+E3QHXmHUXOoT3r
chFpzmP7XqiEaPDEJE4zhmM8+aL3JUzGa6bBoc4EZyWLIf1imwHVREhVW9BZYV4d
AydaxhQ96c0uK5sIcfUWGZb4h8cv6tgSl/glM9gmUDewAg3JczHmwDCZv22cvcy6
ONYwgiB/0S8BpWx4OsIGVQvAjfBXtfpmkjD+ya5VoPTSCqIW+mYPpVWkGyRDjInm
EVIDY2IH
=1moD
-----END PGP SIGNATURE-----

Timestamp of file with hash 41bb2c8bffc4df4e4705ffe7775dabf9aa5e03d062a86e3f08624699d44592ad -

</details>

utACK b048b275d9711f70847afaea5450f17a0f7e673a

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

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK b048b275d9 , only change is squashing one commit 🏦
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgq2gv8DSuQInycB5w6VMTMI7aBtPBoUDAX9yY9Z4Ruix1Im9Bf2fcJts6d9TxY
Abgr2NlSDVGkF4wNvOi3SnXkcYSto+Yt7vNat1HCORsRSzKiyshjKbXjJg/Rvi7K
S11vfpiA5YI87HoYgP/+QGUM912cLBt7DH78lnuskTVeN7B94PiyycU+WHgZw9Qm
8WvFmyft5Tn5iEhOJufn7kB1B4Zdc4CbwTRsT1FpDyxoS+JQ2rvcpJgsXRf4l4Yn
9wmZt637cYe4r5Nh4qyi56CgClbQvyyW1jgXZr3InFozkzPfXt9flbQJSKGGBDKy
c8sVErF1g2QYC5jjkvhJ6c5fpkCt9lvrudf5ol8Ouv/KVhGE9HBqtK/05GVjUefw
FfJaatnobEBXGE8JXVGvHWIazgjEd70MF1GSkZjxIyGHvvWU6IELJ2um0WP/1t4B
SDSUbDFECh4hhZQupjXmTKF5dxl2h3RgkAot+tQc4EgIt9mAg10+vL/jTWTaE1B6
l9ujyThH
=rq5V
-----END PGP SIGNATURE-----

Timestamp of file with hash ef381a3560ba63d06ec8d836ebc9b3379cdaab5ad0d2d7c5ddbfb38d235c2c69 -

</details>

re-ACK b048b275d9711f70847afaea5450f17a0f7e673a

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

utACK https://github.com/bitcoin/bitcoin/pull/19339/commits/b048b275d9711f70847afaea5450f17a0f7e673a

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

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.

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.

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 notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/19339#issuecomment-704744784, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU67TYMCCYJAG63SDSTSJQIRVANCNFSM4ODQNOOQ .

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

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 notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/19339#issuecomment-704895935, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU3CRQUFMVDXYTTEOBDSJRL7JANCNFSM4ODQNOOQ .

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