[WIP] mempool: Remove nAbsurdFee fee from AcceptToMemoryPool #15810

1. 
   jnewbery commented at 8:54 pm on April 12, 2019: member

   nAbsurdFee is indeed absurd. It’s a parameter that the client passes in when calling AcceptToMemoryPool() which asks “If the transaction that I’m passing you has a fee greater than this, don’t accept it”. The client already has the transaction in hand, and is able to look up the inputs, so it can just as easily do the check itself before submitting the transaction to the mempool.

   Furthermore, nAbsurdFee is only used by the RPC and wallet clients. For transactions received from the network or the mempool.dat file (which are the majority of calls to AcceptToMemoryPool()), it is not used.

   Removing this cleans up the AcceptToMemoryPool() interface and clarifies responsibilities (don’t submit a transaction to the mempool if its fee is higher than you want to pay!)

   This builds on #15778 and #15784.

2. 
   MarcoFalke commented at 9:28 pm on April 12, 2019: member

   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.
3. fanquake added the label Validation on Apr 13, 2019
4. 
   DrahtBot commented at 5:09 pm on April 18, 2019: 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:

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

5. 
   jnewbery commented at 5:57 pm on April 18, 2019: member

   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.

6. 
   MarcoFalke commented at 6:31 pm on April 18, 2019: member

   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.

7. 
   gmaxwell commented at 9:40 pm on April 18, 2019: contributor

   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.

8. DrahtBot added the label Needs rebase on Apr 27, 2019
9. jnewbery force-pushed on May 16, 2019
10. 
    jnewbery commented at 4:16 pm on May 16, 2019: member

    Rebased. I still haven’t addressed the feedback from Marco here: #15810 (comment)
11. DrahtBot removed the label Needs rebase on May 16, 2019
12. 
    sipa commented at 6:30 pm on May 20, 2019: member

    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.
13. 
    DrahtBot commented at 3:22 pm on June 6, 2019: member
14. DrahtBot added the label Needs rebase on Jun 6, 2019
15. jnewbery renamed this:
    [WIP] Remove nAbsurdFee fee from AcceptToMemoryPool
    [WIP] mempool: Remove nAbsurdFee fee from AcceptToMemoryPool
    on Aug 25, 2019
16. [rpc] [wallet] Check fee before submitting to mempool d9d9ba1a28
17. [validation] Remove absurdfee from accepttomempool 2373200be5
18. jnewbery force-pushed on Oct 11, 2019
19. 
    jnewbery commented at 3:21 pm on October 11, 2019: member

    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 (https://github.com/bitcoin/bitcoin/pull/15810#issuecomment-484630260).
    • 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.
20. jnewbery closed this on Oct 11, 2019

    ---

21. ajtowns added the label Up for grabs on Oct 14, 2019
22. laanwj removed the label Needs rebase on Oct 24, 2019
23. 
    glozow commented at 3:30 pm on June 19, 2020: member

    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?

24. 
    jnewbery commented at 8:44 pm on June 19, 2020: member

    @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.
25. 
    glozow commented at 7:14 pm on June 20, 2020: member

    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.
26. 
    jnewbery commented at 9:13 pm on June 20, 2020: member

    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.
27. fanquake removed the label Up for grabs on Jun 21, 2020
28. fanquake referenced this in commit db88db4727 on Oct 7, 2020
29. DrahtBot locked this on Feb 15, 2022

Contributors
jnewbery MarcoFalke DrahtBot gmaxwell sipa glozow

Labels
Validation