Wallet: Add `maxfeerate` wallet startup option by ismaelsadeeq · Pull Request #29278 · bitcoin/bitcoin

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.
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
ACK	polespinasa
Stale ACK	josibake, murchandamus, achow101, furszy

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35713 (Remove boost as a unit test runner by rustaceanrob)
#35501 (wallet: store all witness variants of a transaction by achow101)
#35429 (wallet: avoid global access in external signer SPKM by w0xlt)
#35105 (Refactor: Updated TransactionError to use util::Expected and removed TransactionError:OK by kevkevinpal)
#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#34400 (wallet: parallel fast rescan (approx 8x speed up with 8 threads) by Eunovo)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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 places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (maxtxfee)", self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, ...) in test/functional/rpc_psbt.py
assert_raises_rpc_error(-4, msg, self.nodes[1].walletcreatefundedpsbt, inputs, outputs_array, 0, ...) in test/functional/rpc_psbt.py
assert_raises_rpc_error(-4, msg, self.nodes[1].walletcreatefundedpsbt, inputs, outputs_array, 0, ...) in test/functional/rpc_psbt.py

^{2026-07-03 16:37:49}

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))

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

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?

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.

ismaelsadeeq · 2026-05-29T11:20:26Z

Rebased to fix merge conflict.

C.I failure in https://github.com/bitcoin/bitcoin/actions/runs/26634202425/job/78492906366?pr=29278 is unrelated.

polespinasa

Approach ACK

Left some comments.
The PR is already well worked and reviewed so I mostly have nits before acking it.

polespinasa · 2026-06-25T18:01:41Z

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.

Note that after #32138 this line of the PR description is outdated.

-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" git grep -l "getDefaultMaxTxFee" src | xargs sed -i "s/getDefaultMaxTxFee/getMaxTxFee/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`.

ismaelsadeeq · 2026-07-01T11:25:52Z

Forced pushed from 440508a to ecd1e5d 629fcf81e6c7..ecd1e5da

Wallet: Add maxfeerate wallet startup option #29278 (comment), This is correct, I added a test for that.
Wallet: Add maxfeerate wallet startup option #29278 (comment), Wallet: Add maxfeerate wallet startup option #29278 (comment) Indeed, fixed.
Wallet: Add maxfeerate wallet startup option #29278 (comment), Fixed
Wallet: Add maxfeerate wallet startup option #29278 (comment), Fixed
Wallet: Add maxfeerate wallet startup option #29278 (comment), Fixed
Wallet: Add maxfeerate wallet startup option #29278 (comment) Fixed here and other places.
Wallet: Add maxfeerate wallet startup option #29278 (comment), Coming back now, I also noticed this is hard to read and not dry, and not using adjusted fee rate. I abstracted it and fixed those issues.
Wallet: Add maxfeerate wallet startup option #29278 (comment) Fixed.
Wallet: Add maxfeerate wallet startup option #29278 (comment) Fixed
Wallet: Add maxfeerate wallet startup option #29278 (comment) Fixed.

Thanks for review @polespinasa

DrahtBot · 2026-07-01T12:21:42Z

🚧 At least one of the CI tasks failed.
_{Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/28513745459/job/84520144485}
_{LLM reason (✨ experimental): CI failed due to a C++ compile error: TransactionError::MAX_FEE_RATE_EXCEEDED is referenced in src/node/transaction.cpp but no such enum member exists in node::TransactionError.}

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.

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

polespinasa · 2026-07-03T15:17:17Z

+            return Untranslated("Fee exceeds maximum configured by user (maxtxfee)");
+        case TransactionError::MAX_FEE_RATE_EXCEEDED:
+            return Untranslated("Fee rate exceeds maximum configured by user (maxfeerate)");


in 2baa98f [util]: add a new transaction error type
Missing - before maxtxfee and maxfeerate

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

`BroadcastTransaction` now accepts a fee rate limit as an additional input parameter. It rejects transactions whose fee exceeds the maximum fee amount permitted by that fee rate for the transaction vsize. Compare against `maxfeerate.GetFee(vsize)` instead of reconstructing a fee rate from the accepted fee. This preserves the existing rounded fee cap semantics used by `testmempoolaccept` and avoids rejecting transactions whose rounded fee is still within the configured limit. This allows callers to distinguish between failures caused by the absolute fee limit (`-maxtxfee`) and the fee rate limit (`maxfeerate` or `-maxfeerate`). `TestSimpleSpend` creates transactions paying `DEFAULT_TRANSACTION_MAXFEE`. Since `BroadcastTransaction` now also checks the fee rate limit, these small high-fee transactions would exceed `DEFAULT_MAX_TRANSACTION_FEERATE`. Update the test broadcast limit to be slightly above the transaction fee rate. In `wallet_fundrawtransaction.py`'s `test_22670` subtest, restart node 0 with `-maxfeerate` above `-minrelaytxfee` so high-feerate transactions can be broadcast.

- This distinguishes maxfeerate and maxtxfee error messages.

This commit prevents the wallet from submitting or broadcasting transactions above `-maxfeerate`, and tests the new functionality. Compare wallet transaction fees against `maxfeerate.GetFee(vsize)` instead of reconstructing a fee rate from the rounded fee. This allows transactions whose requested fee rate is at the configured limit, even when the resulting fee is rounded up to whole satoshis. `rpc_psbt.py` functional test nodes are modified to start with a custom `-maxfeerate=1`, because the test requires creating and broadcasting a transaction with a high fee rate. A node in some subtests in `wallet_fundrawtransaction.py` is restarted with `-maxfeerate=1` because those subtests require the node's wallet to be able to create and broadcast transactions with a high fee rate.

- Wallets cannot create transactions with fee rates below `-minrelaytxfee`, and they also reject transactions whose total fee exceeds `-maxtxfee`. - Warn when a 1 kvB transaction paying exactly `-maxtxfee` would still have a fee rate below `-minrelaytxfee`. In this configuration, some transactions that meet the minimum relay fee rate may exceed `-maxtxfee`, preventing the wallet from creating them. - Keep this warning independent from the existing high `-maxtxfee` warning, so users see both warnings when both conditions apply.

ismaelsadeeq · 2026-07-03T16:39:51Z

Forced pushed from 6f05410 to d46adaa 6f05410...d46adaa

Wallet: Add maxfeerate wallet startup option #29278 (comment) Fixed
Wallet: Add maxfeerate wallet startup option #29278 (comment) Taken, thanks
Wallet: Add maxfeerate wallet startup option #29278 (comment) Yeah, this is a user-facing update, hence added
Wallet: Add maxfeerate wallet startup option #29278 (comment) I am not sure the hyphen is necessary see maxburnamount. I prefer it without. Hence, not taken.

polespinasa

:)

ACK d46adaa

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

Comment thread src/init.cpp Outdated

Comment thread src/rpc/mempool.cpp

Comment thread src/wallet/wallet.h Outdated

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

Comment thread src/wallet/init.cpp Outdated

glozow reviewed Jan 29, 2024

View reviewed changes

Comment thread test/functional/wallet_send.py

Comment thread src/wallet/wallet.h Outdated

josibake reviewed Jan 29, 2024

View reviewed changes

Comment thread src/wallet/rpc/spend.cpp Outdated

Comment thread src/wallet/wallet.cpp

DrahtBot mentioned this pull request Apr 18, 2026

Refactor: Updated TransactionError to use util::Expected and removed TransactionError:OK #35105

Draft

DrahtBot mentioned this pull request May 13, 2026

psbt, test: remove address type restrictions in test #35279

Merged

This was referenced Jun 12, 2026

RFC: consensus: Make CAmount a class #35511

Open

wallet: store all witness variants of a transaction #35501

Open

wallet: move rescan logic into ChainScanner and wallet/scan #34681

Open

DrahtBot mentioned this pull request Jun 23, 2026

Remove boost as a unit test runner #35587

Closed

polespinasa reviewed Jun 25, 2026

View reviewed changes

ismaelsadeeq added 2 commits July 1, 2026 11:39

doc: add missing verb to make sentence readable

0d68721

[wallet]: update max_fee to max_tx_fee

5f1c955

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

polespinasa reviewed Jul 3, 2026

View reviewed changes

Comment thread src/wallet/init.cpp Outdated

Comment thread src/wallet/wallet.cpp Outdated

Comment thread doc/release-notes-29278.md

polespinasa reviewed Jul 3, 2026

View reviewed changes

ismaelsadeeq added 6 commits July 3, 2026 16:39

[util]: add a new transaction error type

66257be

- This distinguishes maxfeerate and maxtxfee error messages.

[doc]: add release notes

d46adaa

polespinasa reviewed Jul 3, 2026

View reviewed changes

This was referenced Jul 13, 2026

Remove boost as a unit test runner #35713

Draft

wallet: Replace mapWallet and wtxOrdered with a boost::multi_index #35716

Draft

wallet: parallel fast rescan (approx 8x speed up with 8 threads) #34400

Draft

Uh oh!

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!

ismaelsadeeq commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polespinasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polespinasa commented Jun 25, 2026

Uh oh!

ismaelsadeeq commented Jul 1, 2026

Uh oh!

DrahtBot commented Jul 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polespinasa Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq commented Jul 3, 2026

Uh oh!

polespinasa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

ismaelsadeeq commented Jan 18, 2024 •

edited

Loading

DrahtBot commented Jan 18, 2024 •

edited

Loading

ismaelsadeeq commented May 29, 2026 •

edited

Loading