bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee_rate #27969

1. 
   ismaelsadeeq commented at 10:06 pm on June 25, 2023: member

   Fixes #26973

   When using the bumpfee RPC and manually specifying fee_rate, there should be no requirement that the new fee must be at least the sum of the original fee and incrementalFee (maximum of relayIncrementalFee and WALLET_INCREMENTAL_RELAY_FEE).

   This restriction should only apply when user did not specify fee_rate.

   because the GUI doesn’t let the user specify the new fee rate yet (https://github.com/bitcoin-core/gui/issues/647), it would be very annoying to have to bump 20 times to increment by 20 sat/vbyte.

   The restriction should instead be the new fee must be at least the sum of the original fee and incrementalFee (relayIncrementalFee)

2. 
   DrahtBot commented at 10:06 pm on June 25, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, murchandamus

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot renamed this:
   bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee_rate
   bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee_rate
   on Jun 25, 2023
4. ismaelsadeeq force-pushed on Jun 25, 2023
5. DrahtBot added the label CI failed on Jun 25, 2023
6. DrahtBot removed the label CI failed on Jun 25, 2023
7. in test/functional/wallet_bumpfee.py:845 in a2c495c761 outdated

   767+    tx = rbf_node.send(outputs=[{dest_address: 1}], fee_rate=2)
   768+
   769+    # Ensure you can not fee bump with a feerate below or equal to the original fee rate
   770+    assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, tx["txid"], {"fee_rate": 1})
   771+    assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, tx["txid"], {"fee_rate": 2})
   772+
   

   ---

   ---

   

   maflcko commented at 6:01 am on June 26, 2023:

   missing test for larger than original fee rate, but not enough to be larger by incrementalrelayfee?

   ---

   

   ismaelsadeeq commented at 9:17 am on June 27, 2023:

   Added the tests, Thanks
8. ismaelsadeeq force-pushed on Jun 26, 2023
9. bitcoin deleted a comment on Jun 27, 2023
10. glozow added the label Wallet on Jun 30, 2023
11. achow101 requested review from achow101 on Sep 20, 2023
12. achow101 requested review from josibake on Sep 20, 2023
13. 
    achow101 commented at 3:34 pm on September 26, 2023: member

    When using the bumpfee RPC and manually specifying fee_rate, there should be no requirement that the new fee must be at least the sum of the original fee and incrementalFee (maximum of relayIncrementalFee and WALLET_INCREMENTAL_RELAY_FEE).

    Why? It’s not immediately obvious to me that this check is unnecessary. Can you provide more detailed rationale for making it less restrictive?

    ---

    f526546f045f378d8af96219e77e65612cbb3b9c “test: remove assumption from wallet_bumpfee.py” is basically reverted by fb730948aed21ed71ea3ea2f044b1c70de0e17fd “test: use min_fee_rate instead of min_fee_rate_with_walletminrelayfee”. These test changes could just be included in 83ce45104fb4c3ab9d83aa2302f82d2ca6def477 “bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate” directly.

14. ismaelsadeeq force-pushed on Sep 27, 2023
15. 
    ismaelsadeeq commented at 10:29 am on September 27, 2023: member

    Why? It’s not immediately obvious to me that this check is unnecessary. Can you provide more detailed rationale for making it less restrictive?

    This change to remove the restriction of the minimum fee bump of 5s/vb was to fix issue #26973. The 5s/vb minimum fee bump requirement, introduced in PR #9615, is primarily for GUI users who couldn’t specify their desired fee rates. For these users, bumping 10 times to move from 1sat/vb to 11 sat/vb may be undesirable.

    However, in the bumpfee RPC, where users set precise fee rates, enforcing a 5s/vb minimum not necessary, since the users will provide the exact fee rate they wish to fee bump, either 1s/vb, 2s/vb 4s/vb or 20s/vb.

16. 
    ismaelsadeeq commented at 10:30 am on September 27, 2023: member

    f526546 “test: remove assumption from wallet_bumpfee.py” is basically reverted by fb73094 “test: use min_fee_rate instead of min_fee_rate_with_walletminrelayfee”. These test changes could just be included in 83ce451 “bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate” directly.

    Thanks I have updated to 559d18fc707e1bfdded8b8ffbde917aaf6306fb8 with your suggestion.

17. DrahtBot added the label CI failed on Jan 13, 2024
18. bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate

    This commit update CheckFeeRate's incrementalRelayFee to use relayIncrementalFee
    not max of (walletIncrementalRelayfee and relayIncrementalFee).
    
    The restriction is not needed since user provided the fee rate.

    436e88f433
19. test: bumpfee with user specified fee_rate ignores walletIncrementalRelayFee f58beabe75
20. ismaelsadeeq force-pushed on Jan 25, 2024
21. DrahtBot removed the label CI failed on Jan 26, 2024
22. ismaelsadeeq requested review from maflcko on Feb 28, 2024
23. achow101 requested review from murchandamus on Apr 9, 2024
24. 
    achow101 commented at 5:06 pm on April 15, 2024: member

    ACK f58beabe754363cb7d5b24032fd392654b9514ac
25. achow101 removed review request from achow101 on Apr 15, 2024
26. maflcko requested review from Sjors on Apr 18, 2024
27. 
    murchandamus commented at 1:54 pm on June 14, 2024: contributor

    ACK f58beabe754363cb7d5b24032fd392654b9514ac
28. achow101 merged this on Jun 14, 2024
29. achow101 closed this on Jun 14, 2024

    ---

30. ismaelsadeeq deleted the branch on Jun 14, 2024
31. Theschorpioen approved

Contributors
ismaelsadeeq DrahtBot maflcko achow101 murchandamus Theschorpioen

Review Requested
Sjors maflcko josibake

Labels
Wallet