Sanitize fee options #22044

1. 
   amadeuszpawlik commented at 2:46 pm on May 24, 2021: contributor

   rpc: Sanitize fee option inputs. refs #21893

   Checks -blockmintxfee, -incrementalrelayfee, -dustrelayfee and -minrelaytxfee values. Fees are parsed as int64_t, and as a fee is being multiplied by the package size, too large a value might lead to an overflow.

   Max value set to 1 BTC as @MarcoFalke states in #21893:

   Assuming a maximum transaction size of at most 4MvB, this would give an upper bound for the fee rate of ~46116 BTC/kvB. Though, any fee rate larger than 1 BTC/kvB is probably nonsense and should be rejected early on startup.

2. 
   amadeuszpawlik commented at 2:46 pm on May 24, 2021: contributor

   Before undrafting: Are there any other fees that should be sanitized? Is this the right approach?
3. DrahtBot added the label TX fees and policy on May 24, 2021
4. in src/init.cpp:972 in a0cedcf0ac outdated

   967@@ -968,6 +968,9 @@ bool AppInitParameterInteraction(const ArgsManager& args)
   968         CAmount n = 0;
   969         if (!ParseMoney(args.GetArg("-blockmintxfee", ""), n))
   970             return InitError(AmountErrMsg("blockmintxfee", args.GetArg("-blockmintxfee", "")));
   971+        // High fee check
   972+        if(n > MAX_BLOCK_MIN_TX_FEE_PER_KVB)
   

   ---

   ---

   

   promag commented at 9:05 pm on May 24, 2021:

   Please use correct formatting, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c.
5. 
   promag commented at 9:05 pm on May 24, 2021: member

   Concept ACK.
6. amadeuszpawlik force-pushed on May 25, 2021
7. 
   maflcko commented at 5:27 am on May 25, 2021: member

   I’d say all fee rates should be sanitized with this
8. amadeuszpawlik force-pushed on May 25, 2021
9. amadeuszpawlik force-pushed on May 25, 2021
10. amadeuszpawlik force-pushed on May 25, 2021
11. amadeuszpawlik renamed this:
    High fee check for -blockmintxfee option
    High fee checks for fee options
    on May 25, 2021
12. 
    amadeuszpawlik commented at 1:36 pm on May 25, 2021: contributor

    I’d say all fee rates should be sanitized with this

    -minrelaytxfee is already checked for high fee in CWallet::Create(), does it make sense to do a rough (<1BTC) check in AppInitParameterInteraction(...) too, as to reject it as early as possible?

13. 
    maflcko commented at 3:44 pm on May 25, 2021: member

    The error should be printed even without the wallet
14. amadeuszpawlik force-pushed on May 26, 2021
15. amadeuszpawlik marked this as ready for review on May 26, 2021
16. amadeuszpawlik renamed this:
    High fee checks for fee options
    Sanitize fee options
    on May 26, 2021
17. 
    DrahtBot commented at 5:38 pm on June 11, 2021: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25388 (refactor: move policy constants to policy by fanquake)

    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.

18. 
    timgurto commented at 2:53 pm on July 12, 2021: none

    Approach NACK. A 1 BTC/kvB limit may make sense given the current market price of BTC, but in the context of the codebase it’s arbitrary and thus doesn’t belong.

    Given that the purpose of this change is to avoid overflows, it might be more appropriate to sanitize the potential fee once it’s ready to be calculated (e.g., check that fee rate < int max / size). In that way it’d based on the technical constraint itself, rather than on what a reasonable fee looks like in 2021.

19. 
    maflcko commented at 5:01 pm on July 12, 2021: member

    Given that the purpose of this change is to avoid overflows, it might be more appropriate to sanitize the potential fee once it’s ready to be calculated (e.g., check that fee rate < int max / size). In that way it’d based on the technical constraint itself, rather than on what a reasonable fee looks like in 2021.

    When you sanitize it once it’s calculated, it will become a run-time error, instead of InitError, as it is now. This forces the user to shutdown the node and adjust the fee when the node is already fully running. Also, the error might be intermittent, because it depends on the tx size.

    Also, we already have upper limits for the maximum tx fee for local transaction (maxtxfee), with a default of 0.1 BTC. So I don’t see why it is so bad to add another constant.

    If 1 BTC/kvB is still too controversial, 46116 BTC/kvB can be picked as I explained in #21893.

20. DrahtBot added the label Needs rebase on Aug 24, 2021
21. amadeuszpawlik force-pushed on Sep 6, 2021
22. DrahtBot removed the label Needs rebase on Sep 6, 2021
23. amadeuszpawlik force-pushed on Sep 6, 2021
24. High fee check for fee options

    Check if -blockmintxfee, -incrementalrelayfee, -dustrelayfee and
    -minrelaytxfee values are set higher than 1BTC/KvB, in which case
    reject such high values.
    
    This protects against potential overflow when fees are multiplied by the
    package size, as fees are parsed as int64_t.

    cdee1873da
25. amadeuszpawlik force-pushed on Sep 6, 2021
26. 
    DrahtBot commented at 7:55 pm on June 20, 2022: contributor

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

27. DrahtBot added the label Needs rebase on Jun 20, 2022
28. 
    achow101 commented at 6:30 pm on October 12, 2022: member

    Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
29. achow101 closed this on Oct 12, 2022

    ---

30. bitcoin locked this on Oct 12, 2023

Contributors
amadeuszpawlik promag maflcko DrahtBot timgurto achow101

Labels
TX fees and policy Needs rebase