Seems a lot of new code added for a simplification. Also, releasing the locks in between makes this non-atomic: Missing coins are counted as -1 satoshis (instead of their true amount), this can easily lead to not firing an error at all and accepting the tx later on, because the coins became available again.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16129 (refactor: Remove unused includes by practicalswift)
• #15921 (Tidy up ValidationState interface by jnewbery)
• #15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)
• #15169 (Parallelize CheckInputs() in AcceptToMemoryPool() by sdaftuar)
• #15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
• #14920 (Build: enable -Wdocumentation via isystem by Empact)

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.

Thanks for looking at this @MarcoFalke . This is still in RFC mode and I'm looking for concept feedback.

Seems a lot of new code added for a simplification.

Yes, it's additional LOC. It removes code from validation and simplifies the interface to the mempool, in exchange for adding checking to client code (RPC, wallet). I think that counts as an improvement because all other things being equal, we'd prefer less and simpler code in validation. It also makes sense for me that the client should be responsible for this checking.

Also, releasing the locks in between makes this non-atomic: Missing coins are counted as -1 satoshis (instead of their true amount), this can easily lead to not firing an error at all and accepting the tx later on, because the coins became available again.

Yikes! Good catch.

It removes code from validation and simplifies the interface to the mempool

The fee is already calculated in the mempool, so using it for other things is "free". I agree that the current interface to pass in the absurd fee is a bit odd, but what about returning the fee from testmempoolaccept? that way you wouldn't need to duplicate any utxo validation logic, but still could avoid passing in the absurd fee into the mempool and the client can use the result of TMPA to check if the fee was too high.

It would be nice to get this "local user specific behaviour" out of ATMP, I agree!

And even beyond that... fine with simplifying this, but having a lot of duplicated code isn't great. It's also important that it's reasonably precise. I think it would be okay to reject a txn with inputs not found, but we shouldn't bypass the check in that case. I think if the original behaviour had looked like this, the change would have been rejected as not worth it.

We have seen this functionality protect users from loss (including people coming into irc furious that it wouldn't send because it was protecting them... :) ) and it seems likely that there is a lot more that we don't hear about. So I wouldn't suggest removing the functionality.

It's not entirely trivial because we do have to resolve inputs against the mempool not just the chain or wallet... which ATMP inherently does for us.

+0.5 to Marco's suggestion, if something like that works, it sounds like it might be good.

Rebased. I still haven't addressed the feedback from Marco here: #15810 (comment)

I agree it's weird to have special logic for the absurd fee check in the mempool code, so concept ACK getting rid of that, but @MarcoFalke's suggestion seems simpler.

I'm not actively working on this, so I'm going to close the PR for now. In case anyone else wants to pick it up, I've rebased it on master.

A couple of things need addressing:

• @MarcoFalke has a suggestion for simplifying this by using testmempoolaccept ([WIP] mempool: Remove nAbsurdFee fee from AcceptToMemoryPool #15810 (comment)).
• I've had to change a lot of the tests in mempool_accept.py to fail due to too-high feerate to make the test pass, but really they should be changed to set a higher feerate in testmempoolaccept so the transaction fails for the intended reason.

I’ve started working on this, rationale still = (1) mempool behavior should not be user-specific and (2) enforcing a user’s maxfeerate should be the responsibility of the client/wallet. I’d like to ask for a little bit of approach advice.

but what about returning the fee from testmempoolaccept?

I’m guessing this motivates #19093; it’ll be really simple to have a check in testmempoolaccept without passing nAbsurdFee to ATMP this way. But just to clarify, @MarcoFalke do you mean that sendrawtransaction wouldn’t need to enforce maxfeerate anymore?

For sendrawtransaction and the wallet, my current thinking is we want something like John’s GetTransactionFee here to calculate fee as cheaply as possible (maybe implemented with less duplicate code)... does this approach make sense?

@gzhao408 - I think the idea is that you call AcceptToMemoryPool() with test_accept set to true to determine the feerate, and then if it's below the acceptable level, call AcceptToMemoryPool() again with test_accept set to false to actually submit the transaction to the mempool.

Ohhh okay 👍 thanks @jnewbery! Is that what "using the fee for other things is 'free'" means? It should be easy to do this on top of #19093 then.

Yes, I think Marco means that no additional logic is required to be written in AcceptToMemoryPool. We already work out the feerate, so it's no additional work to just return it to the caller.