Wallet: Add `maxfeerate` wallet startup option #29278

ismaelsadeeq · 2024-01-18T22:23:49Z

This PR fixes #29220

The PR adds a wallet -maxfeerate startup option, as the upper limit of wallet transactions fee rate.
This fixes the ambiguity of using maxtxfee value to check the upper limit of transactions fee rate.
Wallet will not create a transaction with fee rate above maxfeerate value. And you can not set wallet fee rate with settxfee RPC above maxfeerate value.
This PR adds a functional test that ensure the behavior is enforced.

DrahtBot · 2024-01-18T22:23:52Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29278.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	josibake, murchandamus

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32636 (Split CWallet::Create() into CreateNew and LoadExisting by davidgumberg)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

“for a wallet transactions” → “for wallet transactions” [“a” makes the noun plural ‘transactions’ ungrammatical]

^{drahtbot_id_4_m}

glozow

I mentioned adding a maxfeerate option in #29220, but I was more imagining it as a wallet-only option. I don't think that wallet configurations should bleed into how the node RPCs function. Instead, the interaction there should be for wallet to pass it as a param to BroadcastTransaction when the it submits a tx to the node.

I'm also not sure about maxburnamount being a node-wide config (#29217 (comment))

src/init.cpp

src/rpc/mempool.cpp

src/wallet/wallet.h

ismaelsadeeq · 2024-01-19T16:12:17Z

Thanks @glozow for your review.
Will put this PR in draft while addressing Approach feedback

luke-jr · 2024-01-23T04:28:54Z

I agree this feels more like it should be a wallet option.

DrahtBot · 2024-01-25T21:53:17Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20878506397}

ismaelsadeeq · 2024-01-25T22:19:38Z

Forced pushed from 2576efc to 0c9e0c5 Compare the diff

All review comments are addressed
Removed maxburnamount option
Made maxfeerate wallet option
Added a check in CreateTransactionInternal that makes sure we don't make transactions above this -maxfeerate.
Fixed ambiguity between -maxtxfee and maxfeerate
Prevent setting wallet feerate above maxfeerate from settxfee RPC
Added a functional test case
Updated PR OP

src/wallet/init.cpp

test/functional/wallet_send.py

src/wallet/wallet.h

josibake

It seems odd to me that we would ever have maxtxfee and maxfeerate set together as they seem to cover entirely different scenarios.

For maxtxfee, a user is expressing "I don't or care what my transaction size is but I know I never want to spend more than X in total fees."

For maxfeerate as user is expressing "I know that my transactions will vary a lot in size, so I don't know what the total fees will be, but I do know that I don't want to pay more than X sats/per vbyte."

I am struggling to think of a scenario where it makes sense to combine them. On the other hand, if I set a maxtxfee of Y and a maxfeerate of X, there input sets that are valid for Y but not valid for X and also inputs that are valid for X but not for Y.

What do you think about only letting the user set one or the other? Or is there a use case for caring about the total fees you pay AND caring about the sats per vbyte you pay that I'm not understanding?

src/wallet/rpc/spend.cpp

src/wallet/wallet.cpp

achow101 · 2024-01-29T17:59:49Z

Or is there a use case for caring about the total fees you pay AND caring about the sats per vbyte you pay that I'm not understanding?

I think it makes sense to have both. I could have a maxtxfee of the upper bound of what I'm willing to pay ever, and also set maxfeerate to something reasonable just to make sure I don't typo a feerate and accidentally pay a feerate orders of magnitude higher than intended. At maxfeerate, a small transaction can still pay a smaller fee than maxtxfee, but a large transaction could exceed maxtxfee.

josibake · 2024-01-29T18:37:46Z

I think it makes sense to have both. I could have a maxtxfee of the upper bound of what I'm willing to pay ever, and also set maxfeerate to something reasonable just to make sure I don't typo a feerate and accidentally pay a feerate orders of magnitude higher than intended. At maxfeerate, a small transaction can still pay a smaller fee than maxtxfee, but a large transaction could exceed maxtxfee.

But in this example, if you typo'd a feerate orders of magnitude higher, your maxtxfee should get hit. If you accidentally typo a higher fee rate than intended but it's below the maxtxfee you're willing to pay, that doesn't seem like a big problem to me.

Seems less footgunny to only let the user set one or the other (and then as a belt and suspenders still check the defaults for both, regardless of what is set?). But I might be overthinking this.. just seems really gross to have to now consider both of these in coin selection, especially if a user sets wonky values.

ismaelsadeeq · 2024-01-29T21:12:27Z

Thank you for your comments @josibake @glozow @achow101 , will address review comments shortly.

if you typo'd a feerate orders of magnitude higher, your maxtxfee should get hit.

I don't think this is true; it depends on what your maxtxfee is. Going by the default value of 0.10 BTC.

This is not likely to occur.

I did a manual test on master.

When I set maxtxfee to 0.10 BTC on master, if you mistakenly type 10,000 as the fee rate, the max fee exceed error will not be hit.

However, in some cases where you set a low maxtxfeeit may be hit or not depending on the transaction size.
It's uncertain.

abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -maxtxfee=0.10  -daemon                                                                                          
Bitcoin Core starting

abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test                                                                          
{
  "name": "abubakar-test"
}

abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest -named sendtoaddress address="bcrt1qt3fvqww6reduq287nhl59cu05hmyrj7achar44" fee_rate=10000 amount=1
22a92fdfa68581ba1d3da4aeedaeb20c7570b8c8eff9c29699c846aa1dacd73d

On this branch, if I set my -maxtxfee as 0.10 absolute fee and -maxfeerate as (0.010 which is 1000s/vb), and make a typo in a sendtoaddress call with a fee_rate of 10,000 s/vb instead of 100, the maxfeerate check will be hit.

 ./src/bitcoind -regtest -maxfeerate=0.01 -maxtxfee=0.10 -daemon                                                                         
Bitcoin Core starting
abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test                                                                         
{
  "name": "abubakar-test"
}

abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest -named sendtoaddress address="bcrt1qt3fvqww6reduq287nhl59cu05hmyrj7achar44" fee_rate=10000 amount=1
error code: -6
error message:
Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)

IIUC -maxtxfee should generally be high as the maximum absolute fee you think you can ever pay, and -maxfeerate as a cautionary check against an absurd fee rate for a transaction based on current fee estimates.

Maybe I should differentiate between the two error messages and make it more verbose, so that users can know which one is hit and adjust if its not intentional.

doc/release-notes-29278.md

src/qt/walletmodel.cpp

ismaelsadeeq · 2025-05-08T19:34:21Z

Rebased to fix merge conflict.

DrahtBot · 2025-08-12T09:16:53Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/47890704383}
_{LLM reason (✨ experimental): The CI failure is caused by a lint error due to missing a trailing newline in a markdown file.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

-BEGIN VERIFY SCRIPT- git grep -l "m_default_max_tx_fee" src | xargs sed -i "s/m_default_max_tx_fee/m_max_tx_fee/g" -END VERIFY SCRIPT- - The value is not always the default but can be configured during startup so `m_max_tx_fee` is the right name not `m_default_max_tx_fee`.

- Also update `-maxapsfee` option from `max_fee` to `max_aps_fee`. This fixes the ambiguity in the variable name.

- The commits adds a new wallet startup option `-maxfeerate` - The value will be used as the upper limit of wallet transaction fee rate. - The commit updates all instances where `-maxtxfee` value is utilized to check wallet transactions fee rate upper limit to now use `-maxfeerate` value. - `settxfee` RPC now only check if the fee rate user set is less `minrelaytxfee`, less than wallet min fee, or whether it exceed `maxfeerate`. We should not check whether `settxfee` RPC respects `maxtxfee` limit in in the functional test because its not enforced. We now only check if `settxfee` respects `maxfeerate` limit.

- `BroadcastTransaction` now accepts a fee rate limit as an additional input parameter. It ensures that the transaction with fee rate exceeding the specified limit are not added to mempool and not broadcasted to peers. - This will allow differentiating between the error messages when either `-maxtxfee` or `maxfeerate` is hit. - `TestSimpleSpend` creates transaction with base fee of `DEFAULT_TRANSACTION_MAXFEE`, since we now also check fee rate limit in broadcastTransaction, creating transaction with the base fee of `DEFAULT_TRANSACTION_MAXFEE` will prevent the transaction from being broadcasted because its fee rate will be above `DEFAULT_MAX_TRANSACTION_FEERATE`, hence update the maximum during broadcast to be the expected fee rate of the transaction.

- This distinguishes maxfeerate and maxtxfee error messages.

- This commit prevents the wallet from creating transactions above `maxfeerate`, and tests the new functionality.

… less than `minrelaytxfee` - Wallet prevents creating transactions with fee rates lower than `-minrelaytxfee`. Also prevents creating a transaction with a base fee above `-maxtxfee`. - Warn user if a 1kvb transaction, with a base fee set to `-maxtxfee`, has a fee rate less than `-minrelaytxfee`. It is likely that some transactions with fee rates greater than or equal to the `-minrelaytxfee` will also likely exceed `-maxtxfee`, the wallet won't be able to create transactions.

DrahtBot added the RPC/REST/ZMQ label Jan 18, 2024

ismaelsadeeq changed the title ~~RPC: Wallet: Add maxfeerate and maxburnamount startup option and use maxfeerate to check max wallet tx feerate~~ RPC: Wallet: Add maxfeerate and maxburnamount startup option Jan 18, 2024

glozow reviewed Jan 19, 2024

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

ismaelsadeeq mentioned this pull request Jan 19, 2024

Enable maxfeerate and maxburnamount as startup config options. #29217

Closed

ismaelsadeeq marked this pull request as draft January 19, 2024 16:12

ismaelsadeeq force-pushed the 01-2024-maxfeerate-fix branch 2 times, most recently from b9f2ddc to 0c9e0c5 Compare January 25, 2024 21:53

DrahtBot added the CI failed label Jan 25, 2024

ismaelsadeeq changed the title ~~RPC: Wallet: Add maxfeerate and maxburnamount startup option~~ Wallet: Add maxfeerate wallet startup option Jan 25, 2024

DrahtBot changed the title ~~Wallet: Add maxfeerate wallet startup option~~ Wallet: Add maxfeerate wallet startup option Jan 25, 2024

ismaelsadeeq marked this pull request as ready for review January 25, 2024 22:19

DrahtBot removed the CI failed label Jan 25, 2024

luke-jr reviewed Jan 27, 2024

View reviewed changes

src/wallet/init.cpp Outdated Show resolved Hide resolved

glozow reviewed Jan 29, 2024

View reviewed changes

test/functional/wallet_send.py Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

josibake reviewed Jan 29, 2024

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

yancyribbens reviewed Feb 22, 2025

View reviewed changes

doc/release-notes-29278.md Outdated Show resolved Hide resolved

yancyribbens reviewed Feb 22, 2025

View reviewed changes

src/qt/walletmodel.cpp Outdated Show resolved Hide resolved

DrahtBot added CI failed and removed CI failed labels Mar 13, 2025

ismaelsadeeq force-pushed the 01-2024-maxfeerate-fix branch from 6dec2d4 to 258a7fa Compare March 18, 2025 10:54

DrahtBot mentioned this pull request Mar 26, 2025

wallet, rpc: remove settxfee and paytxfee #32138

Closed

DrahtBot added the Needs rebase label Mar 27, 2025

ismaelsadeeq force-pushed the 01-2024-maxfeerate-fix branch from 258a7fa to 31a1609 Compare May 8, 2025 19:09

DrahtBot removed the Needs rebase label May 8, 2025

ismaelsadeeq mentioned this pull request May 9, 2025

RFC: Should node Wallet Startup Options Apply to Individual Wallets? #32462

Closed

ismaelsadeeq force-pushed the 01-2024-maxfeerate-fix branch from 31a1609 to c2db218 Compare May 13, 2025 17:13

DrahtBot mentioned this pull request May 29, 2025

Split CWallet::Create() into CreateNew and LoadExisting #32636

Open

DrahtBot mentioned this pull request Jun 12, 2025

docs: adds correct updated documentation links #32699

Open

ismaelsadeeq force-pushed the 01-2024-maxfeerate-fix branch 2 times, most recently from 908710d to 29db50d Compare August 12, 2025 09:16

DrahtBot added the CI failed label Aug 12, 2025

ismaelsadeeq force-pushed the 01-2024-maxfeerate-fix branch from 29db50d to b592d27 Compare August 12, 2025 09:21

ismaelsadeeq added 9 commits August 13, 2025 12:49

doc: add missing verb to make sentence readable

b8e0107

[wallet]: update max_fee to max_tx_fee

825a341

- Also update `-maxapsfee` option from `max_fee` to `max_aps_fee`. This fixes the ambiguity in the variable name.

[util]: add a new transaction error type

958dd68

- This distinguishes maxfeerate and maxtxfee error messages.

[wallet]: enforce -maxfeerate on wallet transactions

7a34511

- This commit prevents the wallet from creating transactions above `maxfeerate`, and tests the new functionality.

[doc]: add release notes

4d6de3c

ismaelsadeeq force-pushed the 01-2024-maxfeerate-fix branch from b592d27 to 4d6de3c Compare August 13, 2025 10:49

DrahtBot removed the CI failed label Aug 13, 2025

Wallet: Add maxfeerate wallet startup option #29278

Are you sure you want to change the base?

Wallet: Add maxfeerate wallet startup option #29278

Conversation

ismaelsadeeq commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ismaelsadeeq commented Jan 19, 2024

Uh oh!

luke-jr commented Jan 23, 2024

Uh oh!

DrahtBot commented Jan 25, 2024

Uh oh!

ismaelsadeeq commented Jan 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 commented Jan 29, 2024

Uh oh!

josibake commented Jan 29, 2024

Uh oh!

ismaelsadeeq commented Jan 29, 2024

Uh oh!

Uh oh!

Uh oh!

ismaelsadeeq commented May 8, 2025

Uh oh!

DrahtBot commented Aug 12, 2025

Uh oh!

Uh oh!

Wallet: Add `maxfeerate` wallet startup option #29278

Wallet: Add `maxfeerate` wallet startup option #29278

ismaelsadeeq commented Jan 18, 2024 •

edited

Loading

DrahtBot commented Jan 18, 2024 •

edited

Loading