maxfeerate wallet startup option
#29278
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29278.
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.
Reviewers, this pull request conflicts with the following ones:
CWallet::Create() into CreateNew and LoadExisting by davidgumberg)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.
Possible typos and grammar issues:
drahtbot_id_5_m
624@@ -624,7 +625,10 @@ void SetupServerArgs(ArgsManager& argsman)
625 SetupChainParamsBaseOptions(argsman);
626
627 argsman.AddArg("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (test networks only; default: %u)", DEFAULT_ACCEPT_NON_STD_TXN), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY);
628- argsman.AddArg("-incrementalrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define cost of relay, used for mempool limiting and replacement policy. (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_INCREMENTAL_RELAY_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY);
629+ argsman.AddArg("-maxfeerate=<amt>", strprintf("Reject transactions whose fee rate is higher than the specified value, expressed in %s/kvB. Set to 0 to accept any fee rate. (default: %s %s/kvB)", CURRENCY_UNIT, FormatMoney(node::DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()), CURRENCY_UNIT), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
630+ argsman.AddArg("-maxburnamount=<amt>", strprintf("Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in %s, if burning funds through unspendable outputs is desired, increase this value. This check is based on heuristics and does not guarantee spendability of outputs (default: 0%s).", CURRENCY_UNIT, CURRENCY_UNIT), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
631+ argsman.AddArg("-incrementalrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define cost of relay, used for mempool limiting and replacement policy. (default: %s) If burning funds through unspendable outputs is desired, increase this value. This check is based on heuristics and does not guarantee spendability of outputs.",
maxburnamount helptext in the incrementalrelayfee helptext here?
48- "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
49- "/kvB.\nSet to 0 to accept any fee rate."},
50- {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
51- "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
52- "If burning funds through unspendable outputs is desired, increase this value.\n"
53- "This check is based on heuristics and does not guarantee spendability of outputs.\n"},
728@@ -729,6 +729,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
729 /** Absolute maximum transaction fee (in satoshis) used by default for the wallet */
730 CAmount m_default_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE};
731
732+ /** Maximum transaction fee rate (in satoshis) used by default for the wallet */
733+ CAmount m_default_max_tx_fee_rate{node::DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()};
CFeeRate? Or describe this as satoshis per KvB. “fee rate (in satoshis)” doesn’t really make sense to me.
I also don’t see this value being used anywhere other than settxfee?
This approach maintains distinct role for maxfeerate as a wallet and mempool acceptance/relay sanity check
Shouldn’t there be a check in CreateTransactionInternal that makes sure we don’t make transactions above this feerate, and an arg passed to BroadcastTransaction? I also don’t see a test case for it.
CFeeRate is the right format since we are representing fee rate.
Shouldn’t there be a check in CreateTransactionInternal that makes sure we don’t make transactions above this feerate, and an arg passed to BroadcastTransaction?
Agreed
I Added a check in CreateTransactionInternal in 0c9e0c5b2218c154b62275794b878af7e1457647.
No need to pass an arg to BroadcastTransaction, if the transaction fee rate is above maxfeerate it will not reach BroadcastTransaction.
I also don’t see a test case for it.
I added a test in 0c9e0c5b2218c154b62275794b878af7e1457647.
Interestingly, in https://github.com/bitcoin/bitcoin/blob/411ba32af21a56efa0a570b6aa8bf8f035410230/src/rpc/mempool.cpp#L89
sendrawtransaction takes a fee_rate argument, which is used to calculate a max_tx_fee based on the size of the transaction, and this max_tx_fee is passed to BroadcastTransaction. This means a user could typo an insane maxfeerate when calling sendrawtransaction that bypasses the default max tx fee.
Perhaps it does make more sense to have BroadcastTransaction take two arguments, one for max_tx_fee and another for max_tx_fee_rate, and check them independently. Probably out of scope for this PR, since this is focused on the wallet, but wanted to mention it as a potential follow-up.
Thanks for highlighting this issue @josibake , I agree with your BroadcastTransaction should enforce check on both.
This will make sendrawtransaction returns a specific error message.
For this reason
MAX_FEE_RATE_EXCEEDED error string is now Fee exceeds maximum configured by user.
because if I add the option maxtxfee it will be misleading in sendrawtransaction output
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 (https://github.com/bitcoin/bitcoin/issues/29217#issuecomment-1900585661)
🚧 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.
Forced pushed from https://github.com/bitcoin/bitcoin/commit/2576efcf689401dcbea85856c74efc64af0c2cb2 to https://github.com/bitcoin/bitcoin/commit/0c9e0c5b2218c154b62275794b878af7e1457647 Compare the diff
maxburnamount optionmaxfeerate wallet optionCreateTransactionInternal that makes sure we don’t make transactions above this -maxfeerate.-maxtxfee and maxfeeratemaxfeerate from settxfee RPC61@@ -62,6 +62,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
62 argsman.AddArg("-maxapsfee=<n>", strprintf("Spend up to this amount in additional (absolute) fees (in %s) if it allows the use of partial spend avoidance (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
63 argsman.AddArg("-maxtxfee=<amt>", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)",
64 CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
65+ argsman.AddArg("-maxfeerate=<amt>", strprintf("Maximum fee rate (in %s) of a single wallet transaction (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEERATE.GetFeePerK())), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
I dont understand why you said it is not a wallet option.
The OptionsCategory of this startup option is WALLET.
And its only used in the wallet.
Can you please expand on your comment. Thanks
183@@ -184,6 +184,33 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
184
185 return res
186
187+ def test_maxfeerate(self):
-maxfeerate?
Maybe add tests where you (successfully) configure the node’s wallet using -maxfeerate?
I dont understand what you mean by successfully configure nodes wallet.
The test you are commenting on configure node[0] with the -maxfeerate option.
Could you clarify your comment, thanks.
Similarly, it would be good to test that both maxfee and maxfeerate requirements must be met.
Their is a test for maxtxfee already, and I added one for maxfeerate.
https://github.com/bitcoin/bitcoin/blob/11b436a66af3ceaebb0f907878715f331516a0bc/test/functional/wallet_bumpfee.py#L557
135@@ -136,6 +136,8 @@ static const bool DEFAULT_DISABLE_WALLET = false;
136 static const bool DEFAULT_WALLETCROSSCHAIN = false;
137 //! -maxtxfee default
138 constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN / 10};
139+//! maxfeerate default
140+const CFeeRate DEFAULT_TRANSACTION_MAXFEERATE(CFeeRate((COIN / 10), 1000));
438 } else if (tx_fee_rate < pwallet->m_min_fee) {
439 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than wallet min fee (%s)", pwallet->m_min_fee.ToString()));
440- } else if (tx_fee_rate > max_tx_fee_rate) {
441- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than wallet max tx fee (%s)", max_tx_fee_rate.ToString()));
442+ } else if (tx_fee_rate > pwallet->m_max_tx_fee_rate) {
443+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than max tx fee rate (%s)", pwallet->m_max_tx_fee_rate.ToString()));
relayMinFee and the wallet set policy m_min_fee by adding the world “wallet” to the RPC error.
3057@@ -3058,13 +3058,21 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3058 warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee"));
3059 }
3060
3061- if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) {
max_fee that results in fee rates less than the relayMinFee.
relayMinFee is relative to the fee rate of the transaction. I believe I did the right thing by removing it and enforcing this check on -maxfeerate instead.
I don’t think we should use the base fee value as a quantifier for the fee rate, as transaction sizes differ. This is indicated in the issue description #29220.
I think the check needs to be enforced on both, since we are letting a user set one or the other (or both). Removing the check entirely for one of the options feels like a regression.
It’s a good point that using the base fee value as a quantifier is not ideal, but previously this code was checking that the user did not set a maxtxfee that was so low that the minrelay fee could never be met. Now you’ve removed that check, which means I can set a very low maxtxfee and not get a warning.
Removing the check entirely for one of the options feels like a regression.
I don’t think its a regression because this is the right behavior to check if the maximum fee rate we set is < minimum relay fee rate.
but previously this code was checking that the user did not set a
maxtxfeethat was so low that the minrelay fee could never be met. Now you’ve removed that check, which means I can set a very low maxtxfee and not get a warning.
With the assumption of 1k vb size, transactions can have more or less virtual size though.
I think we prevent creating transactions with fee rate below relayMinFee , so I think this okay?
Now you’ve removed that check, which means I can set a very low maxtxfee and not get a warning.
Perhaps just retain a version of the old check by enforcing that maxtxfee is set to at least CAmount FLOOR_MAXTXFEE{1000} (a conversion from the default min relay feerate)?
Summarizing my thoughts from some offline discussion with @ismaelsadeeq :
relayMinFee when creating a transaction, but it would be good to confirm this (even better, to have a functional test!)relayMinFee and maxtxfee, I think we need to have a sanity check at startup to make sure they are not setting insane values that could result in them creating stuck transactionsBASELINE_TX_SIZE = <1000, or maybe a minimum tx size?>. That way, it is used consistently throughout the codebase, has a comment explaining what it used for and what it shouldn’t be used for, etcmaxtxfee to calculate a fake feerate (such as settxfee). We should change those to use maxtxfeerateIn general, I agree we should always use maxtxfeerate when checking things about feerates. The only exception is during startup where we need to compare maxtxfee to other user set values relating to feerates.
Perhaps just retain a version of the old check by enforcing that maxtxfee is set to at least CAmount FLOOR_MAXTXFEE{1000} (a conversion from the default min relay feerate)?
This doesn’t cover the case where a user sets a maxtxfee and a custom relayMinFee. Admittedly, the old check wasn’t perfect in that the user could still create > 1000 vbyte transactions that might still cause a problem. I think the best would be have a sanity check at startup (either the old one, or a new one with a different default for baseline tx size) and ALSO check that we are respecting the relayMinFee during transaction construction
I’m fairly certain we don’t check relayMinFee when creating a transaction, but it would be good to confirm this (even better, to have a functional test!)
I’ve tested this, and it turns out that the wallet indeed prevents creating transactions below the minrelaytxfee
https://github.com/bitcoin/bitcoin/blob/a11585692e72cac468fb1496ea2c30e4c07f73e5/src/wallet/spend.cpp#L1055
Here is a test that confirms we can’t create transactions with a fee rate below the minimum relay fee, and also the node won’t start if maxtxfee is very low so that a 1000 VB transaction with maxtxfee base fee fee rate is belowminrelaytxfee.
See https://github.com/ismaelsadeeq/bitcoin/commit/aa075b8ef0b43f03d2fbef41c04a71403e93d904
In cases where users set a high minrelaytxfee and low maxtxfee, they can’t even create transactions.
I agree with @josibake suggestion and added a commit that performs the same check in wallet initialization as before.
node won’t start if maxtxfee is very low so that a 1000 vb transaction with maxtxfee base fee fee rate is below the minrelaytxfee.
In cases where the transaction size exceeds 1000 vb, the wallet check should safeguard against creating stuck transactions.
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?
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.
I think it makes sense to have both. I could have a
maxtxfeeof the upper bound of what I’m willing to pay ever, and also setmaxfeerateto something reasonable just to make sure I don’t typo a feerate and accidentally pay a feerate orders of magnitude higher than intended. Atmaxfeerate, a small transaction can still pay a smaller fee thanmaxtxfee, but a large transaction could exceedmaxtxfee.
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.
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.
0abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoind -regtest -maxtxfee=0.10 -daemon
1Bitcoin Core starting
2
3abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
4{
5 "name": "abubakar-test"
6}
7
8abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest -named sendtoaddress address="bcrt1qt3fvqww6reduq287nhl59cu05hmyrj7achar44" fee_rate=10000 amount=1
922a92fdfa68581ba1d3da4aeedaeb20c7570b8c8eff9c29699c846aa1dacd73d
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.
0 ./src/bitcoind -regtest -maxfeerate=0.01 -maxtxfee=0.10 -daemon
1Bitcoin Core starting
2abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest loadwallet abubakar-test
3{
4 "name": "abubakar-test"
5}
6
7abubakarismail@Abubakars-MacBook-Pro bitcoin % ./src/bitcoin-cli -regtest -named sendtoaddress address="bcrt1qt3fvqww6reduq287nhl59cu05hmyrj7achar44" fee_rate=10000 amount=1
8error code: -6
9error message:
10Fee 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.
just seems really gross to have to now consider both of these in coin selection, especially if a user sets wonky values.
I dont really understand how this can affect coinselection, can you expand please? Thanks
I dont really understand how this can affect coinselection, can you expand please? Thanks
Sorry, I was completely overthinking it! I was incorrectly thinking that we took max_tx_fee and tx_fee_rate as inputs to coin selection, but what we do instead:
fee_rate (this fee rate would have already been sanity checked against max_fee_rate)max_tx_fee after the transaction is done (and now max_tx_fee_rate)
@ismaelsadeeq thanks for walking through the scenario! Reading over what you posted, if maxtxfee is left as the default, then the user can set whatever they want for maxfeerate without any issues. Depending on the types of transactions they make, they are essentially setting a new, lower maxtxfee.
If the user is setting both, they could create weird scenarios where the maxtxfee is too low compared to the maxtxfeerate and vice versa. But thinking it through and with your examples, I think most of my concern boils down to “the user set a maxtxfee which is too low for the current fee environment.”
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.
I think differentiating errors makes sense! Then it doesn’t really matter if the user sets bad values for max_fee_rate or max_tx_fee, so long as we tell them which bad value is getting hit and prompt them to change it.
1293@@ -1294,6 +1294,11 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1294 return util::Error{TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED)};
1295 }
1296
1297+ CFeeRate tx_fee_rate = CFeeRate(current_fee, nBytes);
1298+ if (tx_fee_rate > wallet.m_max_tx_fee_rate) {
1299+ return util::Error{TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED)};
perhaps introduce a new error type:
0 return util::Error{TransactionErrorString(TransactionError::MAX_FEE_RATE_EXCEEDED)};
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
I am a bit confused. You marked this as resolved and said you added this, but I don’t see it in the current version. Did you fix it in your working branch but haven’t pushed yet?
31@@ -32,7 +32,9 @@ bilingual_str TransactionErrorString(const TransactionError err)
32 case TransactionError::SIGHASH_MISMATCH:
33 return Untranslated("Specified sighash value does not match value stored in PSBT");
34 case TransactionError::MAX_FEE_EXCEEDED:
35- return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)");
36+ return Untranslated("Fee exceeds maximum configured by user.");
Better to keep the option name in the error message:
0 return Untranslated("Fee exceeds maximum configured by user (maxtxfee)");
Yes but I think should come in a follow-up after cleaning up your suggestion here #29278 (review)
I have a local branch that does this on top of this branch.
In d4dceeafc2292cd050cb10e308d73bc8ef84201a (wallet: test: add new transaction error type):
It isn’t clear to me why we would first ship a less descriptive new error message in this PR and then immediately fix it in a follow-up. It seems better to ship a descriptive error message right here.
The reason is that the maximum fee rate check for sendrawtransaction is performed in BroadcastTransaction which returned MAX_FEE_EXCEEDED error type to sendrawtransaction.
If I change the MAX_FEE_EXCEEDED error message to “Fee exceeds maximum configured by user (maxtxfee)” without changing how it’s checked, this would be misleading for sendrawtransaction callers and I wanted this to be a minimal wallet change that adds the maxfeerate option to the wallet.
However I agree with your comment, I have now added a commit that also checks the fee rate limit in BroadcastTransaction, and changed how sendrawtransaction is implemented, and updated the MAX_FEE_EXCEEDED transaction error type message.
Concept ACK
My scenario for using both maxtxfee and maxfeerate would be slightly different. For example looking at the mempool in January, I might generally be okay with paying up to 50 sat/vB, but not a higher feerate, because I expect that it will generally suffice to get a confirmation within 24h, but at such a high feerate, I would not want to make a transaction with more than 10 inputs. So, I’d delimit the feerate itself on the one hand, and then also limit the maxtxfee to 35'000 sats which is enough for a transaction with 10 P2TR keypath inputs and two P2TR outputs at 50 sat/vB. Through this general limit on the cost of a single transaction I could essentially set an upper bound on the total input weight of my transactions which would increase with lower feerates.
438 } else if (tx_fee_rate < pwallet->m_min_fee) {
439 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than wallet min fee (%s)", pwallet->m_min_fee.ToString()));
440- } else if (tx_fee_rate > max_tx_fee_rate) {
441- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than wallet max tx fee (%s)", max_tx_fee_rate.ToString()));
442+ } else if (tx_fee_rate > pwallet->m_max_tx_fee_rate) {
443+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than wallet max tx fee rate (%s)", pwallet->m_max_tx_fee_rate.ToString()));
We are comparing two feerates, should this perhaps be:
0 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("fee rate cannot be more than wallet max tx fee rate (%s)", pwallet->m_max_tx_fee_rate.ToString()));
3057@@ -3058,13 +3058,21 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3058 warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee"));
3059 }
3060
3061- if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) {
3062+ walletInstance->m_default_max_tx_fee = max_fee.value();
In 7664ec9bd744ab5ae403a87cd9d2aa3de747d278 (wallet: add maxfeerate wallet startup option):
walletInstance->m_default_max_tx_fee indicates to me that the value is the default if the user does not customize the value, so it is confusing to me that the user value is assigned to a variable with “default” in its name.
0 walletInstance->m_max_tx_fee = max_fee.value();
135@@ -136,6 +136,8 @@ static const bool DEFAULT_DISABLE_WALLET = false;
136 static const bool DEFAULT_WALLETCROSSCHAIN = false;
137 //! -maxtxfee default
138 constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN / 10};
139+//! maxfeerate default
140+const CFeeRate DEFAULT_TRANSACTION_MAXFEERATE(CFeeRate(COIN / 10));
In 7664ec9bd744ab5ae403a87cd9d2aa3de747d278 (wallet: add maxfeerate wallet startup option):
If I did not miscalculate this feerate translates to 10'000 sat/vB. A single input two output P2TR transaction would incur a cost of over 15 m₿ at that feerate. I could see how this might be reached if we CPFP a big transaction with a small transaction, but for a standalone transaction this limit fees too high for a failsafe.
Yes its 10'000 sat/vb, I agree its high indeed Should I reduce it to maybe 1000 s/vb instead?
This is configurable option can be reduced by users to threshold they want.
2000 sat/vB, but that might be insufficient for a child transaction bumping a much larger parent. Also see #29661.
In “[wallet]: add maxfeerate wallet startup option” (e4205cf978d24fe77ce555900e4ae77da12b46ec):
Given that we have max_tx_fee_rate, -maxfeerate, and max_fee_rate, should this now perhaps be DEFAULT_MAX_TRANSACTION_FEERATE instead of DEFAULT_TRANSACTION_MAXFEERATE? I.e. I’m surprised in this constant the “MAX” appears between tx and feerate now, when it is appearing before “tx” elsewhere?
but that might be insufficient for a child transaction bumping a much larger parent.
Users can then restart with a much higher maxfeerate whenever they want to feebump a transaction and the limit is hit? but I reckon restarting may be undesirable.
Anyways whether it’s 1000 or 2000 sat/vB a sane maximum transaction fee rate is a function of Block space demand which is unpredictable, what is insane fee rate maybe sane. Thats why users can pass a lower limit or higher limit based on that factor?
Should I leave as is and continue discussion about this in #29661?
79@@ -80,6 +80,13 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
80 return feebumper::Result::WALLET_ERROR;
81 }
82
83+ // check that new fee rate does not exceed maxfeerate
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
Given that we might use a small transaction to bump a big transaction, we might actually want to have a very large feerate on the child to achieve a reasonable mining score for the parent-child package. Would it perhaps make sense to generally lower the default maxfeerate but then compare it with the resulting mining score instead of the new transaction’s individual feerate?
compare it with the resulting mining score instead of the new transaction’s individual feerate?
🤔 Not sure how to implement the change you suggested honestly.
The current behavour is checking if target fee rate of user wants to bump the transaction to is greater than maxfeerate which I believe is the new mining score of the bumped transaction no?
I might be wrong on my assumption though, would you clarify your comment on how to check the resulting mining score of a bumped transaction please? thanks.
What @murchandamus is suggesting makes a lot of sense for CPFP’ing, but I imagine will not be trivial to implement. What do you think about deferring this to a follow-up, @murchandamus ? That way, this PR can stay focused on only adding the option and with the current default for maxtxfeerate I don’t think we will run into issues CPFP’ing with high feerate children.
Definitely agree though, comparing to the mining score would allow the user to set a lower limit to protect against high feerates on individual transactions, while also allowing high feerate children (potentially above the max limit) to be used to bump large parents.
188+ self.log.info("test -maxfeerate enforcement on wallet transactions.")
189+ # Default maxfeerate is 10,000 sats/vb
190+ # All transactions with a fee rate below 10,000 will be created.
191+
192+ # All transaction with feerate <= 10,000 sats/vb can be created.
193+ self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), amount=1, fee_rate=1000)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
If you first build a transaction that succeeds and then try to build a transaction that fails, this could also be caused by the success of the first transaction. Perhaps it would be better to turn around the order:
maxfeerate, since that fails the wallet is still in the same state.maxfeerate and show that it succeeds.Perhaps use 9900 sat/vB instead of 1000 sat/vB to box the maxfeerate a bit tighter.
203+ assert_raises_rpc_error(-6, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)",
204+ self.nodes[0].sendtoaddress, address=self.nodes[0].getnewaddress(), amount=1, fee_rate=11)
205+
206+ # Fee rates <= 10 sats/vb will be accepted by the wallet.
207+ self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), amount=1, fee_rate=5)
208+ self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), amount=1, fee_rate=6)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
One transaction but closer to the limit should suffice:
0 self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), amount=1, fee_rate=9)
320@@ -321,7 +321,7 @@ def run_test(self):
321
322 self.log.info("Test invalid fee rate settings")
323 for param, value in {("fee_rate", 100000), ("feeRate", 1)}:
324- assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)",
325+ assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user.",
In d4dceeafc2292cd050cb10e308d73bc8ef84201a (wallet: test: add new transaction error type):
This seems strictly less informative. I would second @josibake in the suggestion to keep mentioning the maxtxfee here. Maybe I’m missing something, could you elaborate why you would push that back to a follow-up?
36@@ -37,6 +37,7 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const
37 "-keypool=<n>",
38 "-maxapsfee=<n>",
39 "-maxtxfee=<amt>",
40+ "-maxfeerate=<amt>",
🚧 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.
90
91 std::string err_string;
92 AssertLockNotHeld(cs_main);
93 NodeContext& node = EnsureAnyNodeContext(request.context);
94- const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee, /*relay=*/true, /*wait_callback=*/true);
95+ const TransactionError err = BroadcastTransaction(node, tx, err_string, /*max_raw_fee*/ 0, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true);
in “node: BroadcastTransaction should also check tx fee rate limit” (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
0 const TransactionError err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true);
820@@ -820,7 +821,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
821 auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
822 m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
823 auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
824- BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
825+ BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, /*max_tx_feerate*/ CFeeRate(0), false, error));
in “node: BroadcastTransaction should also check tx fee rate limit” (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Don’t we have a DEFAULT_TRANSACTION_MAXFEERATE? Wouldn’t it be better to use that here instead of 0?
862@@ -862,7 +863,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
863 block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
864 m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
865 mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
866- BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
867+ BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, /*max_tx_feerate*/ CFeeRate(0), false, error));
in “node: BroadcastTransaction should also check tx fee rate limit” (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Same comment as above regarding defaults
1981@@ -1982,7 +1982,7 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string
1982 // If broadcast fails for any reason, trying to set wtx.m_state here would be incorrect.
1983 // If transaction was previously in the mempool, it should be updated when
1984 // TransactionRemovedFromMempool fires.
1985- bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string);
1986+ bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, /*max_tx_feerate*/ CFeeRate(0), relay, err_string);
in “node: BroadcastTransaction should also check tx fee rate limit” (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Same comment regarding defaults vs 0
1981@@ -1982,7 +1982,7 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string
1982 // If broadcast fails for any reason, trying to set wtx.m_state here would be incorrect.
1983 // If transaction was previously in the mempool, it should be updated when
1984 // TransactionRemovedFromMempool fires.
1985- bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, /*max_tx_feerate*/ CFeeRate(0), relay, err_string);
1986+ bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, m_max_tx_fee_rate, relay, err_string);
in “wallet: add maxfeerate wallet startup option” (https://github.com/bitcoin/bitcoin/pull/29278/commits/4165dc41289fd337e279f3986dfc1a84345c900e):
Instead of touching this line twice, what do you think about reordering the commits so that this commit is first and the changes to BroadcastTransaction come after it? This way, you avoid needing to first set /*max_tx_feerate=*/CFeeRate(0) only to remove it in this commit.
544@@ -545,9 +545,9 @@ def test_settxfee(self, rbf_node, dest_address):
545 assert_greater_than(Decimal("0.00001000"), abs(requested_feerate - actual_feerate))
546 rbf_node.settxfee(Decimal("0.00000000")) # unset paytxfee
547
548- # check that settxfee respects -maxtxfee
549- self.restart_node(1, ['-maxtxfee=0.000025'] + self.extra_args[1])
550- assert_raises_rpc_error(-8, "txfee cannot be more than wallet max tx fee", rbf_node.settxfee, Decimal('0.00003'))
in “wallet: add maxfeerate wallet startup option” (https://github.com/bitcoin/bitcoin/pull/29278/commits/4165dc41289fd337e279f3986dfc1a84345c900e):
Why are you removing the -maxtxfee test? Why not just add a new test case for -maxfeerate?
in “wallet: add maxfeerate wallet startup option” https://github.com/bitcoin/bitcoin/commit/4165dc41289fd337e279f3986dfc1a84345c900e
After adding maxfeerate option we now only check if the fee rate user set is less minrelaytxfee, less than wallet min fee, or whether it exceed maxfeerate.
So their is no need to check for maxtxfee here, because its not enforced here.
Will add this info in commit message, thanks
1484@@ -1485,6 +1485,9 @@ RPCHelpMan sendall()
1485 if (fee_from_size > pwallet->m_default_max_tx_fee) {
1486 throw JSONRPCError(RPC_WALLET_ERROR, TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED).original);
1487 }
1488+ if (CFeeRate(fee_from_size, tx_size.vsize) > pwallet->m_max_tx_fee_rate) {
1489+ throw JSONRPCError(RPC_WALLET_ERROR, TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED).original);
in “wallet: test: enforce -maxfeerate on wallet transactions” (https://github.com/bitcoin/bitcoin/pull/29278/commits/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):
This should be MAX_FEE_RATE_EXCEEDED, right?
129@@ -130,7 +130,7 @@ def test_invalid_parameters(self, rbf_node, peer_node, dest_address):
130 assert_raises_rpc_error(-8, "Insufficient total fee 0.00000141", rbf_node.bumpfee, rbfid, fee_rate=INSUFFICIENT)
131
132 self.log.info("Test invalid fee rate settings")
133- assert_raises_rpc_error(-4, "Specified or calculated fee 0.141 is too high (cannot be higher than -maxtxfee 0.10",
134+ assert_raises_rpc_error(-4, "New fee rate 1.00 is too high (cannot be higher than -maxfeerate 0.10)",
in “wallet: test: enforce -maxfeerate on wallet transactions” (https://github.com/bitcoin/bitcoin/commit/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):
What’s the reason for changing the tests instead of adding a new test case for maxfeerate? Seems like we should be testing both rather than changing the old maxtxfee tests to now check maxtxfeerate.
Because after https://github.com/bitcoin/bitcoin/commit/eadc3a4500d6b7be263a2981602c20f6e5eb13a4 we now check for maxfeerate threshold first before checking maxtxfee threshold.
And the specified fee rate is above default maxferate, as such it will be hit first.
But I agree, I will add another test where specified bump fee rate is below the maxfeerate but we hit maxtxfee due to size or maybe just restart with maxtxfee set to a lower value ?
230@@ -231,7 +231,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
231
232 // Reject absurdly high fee. (This can never happen because the
233 // wallet never creates transactions with fee greater than
234- // m_default_max_tx_fee. This merely a belt-and-suspenders check).
in “wallet: rename m_default_max_tx_fee to m_max_tx_fee” (https://github.com/bitcoin/bitcoin/pull/29278/commits/a1497af6fc11009839ef86de1bf2da5cf99ad489):
Agree that this is way less confusing as a name, but probably still good to have a commit message explaining your reasoning. I’d also suggest moving this commit to the beginning before any of the tests are changed and also add the refactor label to the commit message.
in “[refactor]: rename m_default_max_tx_fee to m_max_tx_fee” (https://github.com/bitcoin/bitcoin/pull/29278/commits/f1ff2b8f40edffc2b61240a976cff9a9fd51a110):
nit: this commit could be a scripted-diff
3061+ // Also the wallet prevents creating transaction with base fee above maxtxfee.
3062+ // Ensure that a 1kvb transaction, with a base fee set to maxtxfee, maintains a fee rate of at least minrelaytxfee.
3063+ // Otherwise transactions with fee rate greater than or equal to the minrelaytxfee will likely exceed maxtxfee.
3064+ // In such cases, the wallet won't be able to create transactions. Therefore, shut down early.
3065+ } else if (chain && CFeeRate(max_fee.value(), 1000) < chain->relayMinFee()) {
3066+ error = strprintf(_("Invalid amount for %s=<amount>: '%s' is too low. It won't allow creating transactions exceeding the minrelay fee of %s. Consider adjusting %s or %s."),
in “wallet: ensure 1kvb tx with maxtxfee base fee has feerate atleast minrelaytxfee” (https://github.com/bitcoin/bitcoin/pull/29278/commits/9791ff064cf28df332c3c343c22eca6aa5874f39):
Just a suggestion, but I think this reads a little bit better:
0 error = strprintf(_("Invalid amount for %s=<amount>: '%s' conflicts with the minrelay fee of %s. Consider adjusting %s or %s."),
This seems better to me since the reason for the error could be that the minrelayfee is set too high.
78 return HandleATMPError(result.m_state, err_string);
79- } else if (result.m_base_fees.value() > max_tx_fee) {
80+ } else if (max_tx_fee > 0 && result.m_base_fees.value() > max_tx_fee) {
81+ return TransactionError::MAX_FEE_EXCEEDED;
82+ } else if (max_tx_feerate > CFeeRate(0) && CFeeRate(result.m_base_fees.value(), result.m_vsize.value()) > max_tx_feerate) {
83 return TransactionError::MAX_FEE_EXCEEDED;
in “[node]: update BroadcastTransaction to check fee rate limit” (https://github.com/bitcoin/bitcoin/pull/29278/commits/eedbeccbdb274841fc2cf2c53931e58fbea50dfc):
This should be MAX_FEERATE_EXCEEDED
The MAX_FEERATE_EXCEEDED error type was not introduced yet at this point.
This is correct because at https://github.com/bitcoin/bitcoin/commit/eedbeccbdb274841fc2cf2c53931e58fbea50dfc, the error message indicates that the failure might be from -maxtxfee or maxfeerate.
And this is fixed in next commit 0ad2f2c32460fef9b8cd59e3920fcfd6602b02a4 after we introduced the MAX_FEERATE_EXCEEDED error type.
The options are;
BroadcastTransaction to support fee rate checks. This approach however, would result in misleading error messages in sendrawtransaction.BroadcastTransaction to support checking fee rate then introduce the new error type which in my opinion, is the better approach.
The current approach, update BroadcastTransaction to support checking fee rate then introduce the new error type which in my opinion, is the better approach.
sounds good!
38@@ -39,11 +39,12 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
39 * @param[in] tx the transaction to broadcast
40 * @param[out] err_string reference to std::string to fill with error string if available
41 * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee)
42+ * @param[in] max_tx_feerate reject txs with fee rate higher than this (if CFeeRate(0), accept any fee)
in “[node]: update BroadcastTransaction to check fee rate limit” (https://github.com/bitcoin/bitcoin/pull/29278/commits/eedbeccbdb274841fc2cf2c53931e58fbea50dfc):
If a max_tx_fee is set, and a max_tx_feerate is not set, BroadcastTransaction might hit the max_tx_fee limit, so its not quite correct to say that “if CFeeRate(0), accept any fee.” What about something like the following:
0* [@param](/bitcoin-bitcoin/contributor/param/)[in] max_tx_fee reject txs with fees higher than this (if 0, the fee is not checked)
1* [@param](/bitcoin-bitcoin/contributor/param/)[in] max_tx_feerate reject txs with fee rate higher than this (if CFeeRate(0), the feerate is not checked)
90
91 std::string err_string;
92 AssertLockNotHeld(cs_main);
93 NodeContext& node = EnsureAnyNodeContext(request.context);
94- const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee, /*relay=*/true, /*wait_callback=*/true);
95+ const TransactionError err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee*/0, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true);
in “[node]: update BroadcastTransaction to check fee rate limit” (https://github.com/bitcoin/bitcoin/commit/eedbeccbdb274841fc2cf2c53931e58fbea50dfc):
nit:
0 const TransactionError err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true);
Looking good! I think moving https://github.com/bitcoin/bitcoin/pull/29278/commits/d83ecdde0dfdbf46806be1310ad63716e33edc96 to be the 3rd commit (right after introducing the new option) makes more sense as there are still at least a few spots where you return a MAX_TX_FEE error incorrectly, and then fix it up in a later commit.
Also, small typo in https://github.com/bitcoin/bitcoin/pull/29278/commits/eedbeccbdb274841fc2cf2c53931e58fbea50dfc:
s/differenciating/differentiating/
438@@ -439,8 +439,8 @@ RPCHelpMan settxfee()
439 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", pwallet->chain().relayMinFee().ToString()));
440 } else if (tx_fee_rate < pwallet->m_min_fee) {
441 throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than wallet min fee (%s)", pwallet->m_min_fee.ToString()));
442- } else if (tx_fee_rate > max_tx_fee_rate) {
443- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than wallet max tx fee (%s)", max_tx_fee_rate.ToString()));
444+ } else if (tx_fee_rate > pwallet->m_max_tx_fee_rate) {
in “[wallet]: add maxfeerate wallet startup option” (https://github.com/bitcoin/bitcoin/pull/29278/commits/6a2c7722698d22c95220daf7727f9219ed843042):
The old max_tx_fee_rate on L435 should be deleted:
211@@ -212,6 +212,7 @@ class Chain
212 //! Return false if the transaction could not be added due to the fee or for another reason.
213 virtual bool broadcastTransaction(const CTransactionRef& tx,
214 const CAmount& max_tx_fee,
215+ const CFeeRate& max_tx_feerate,
in “[node]: update BroadcastTransaction to check fee rate limit” (https://github.com/bitcoin/bitcoin/pull/29278/commits/368d140db549387fb7bf162c96dd9e9064045103):
nit: in other places, you’ve been calling this max_tx_fee_rate.
0@@ -0,0 +1,7 @@
1+wallet startup option
2+========================
3+
4+- A new wallet startip option `-maxfeerate` is added.
5+- This option sets the upper limit for wallet transaction fee rate.
6+- The wallet will now refrain from creating transactions with a feerate exceeding the `maxfeerate`.
in “[doc]: add release notes” (https://github.com/bitcoin/bitcoin/pull/29278/commits/bf6fc126d1e0b37e490d38124945fa4e8f7475f4):
nit: inconsistent usage of “fee rate” vs “feerate.” considering “fee rate” is what you use in the error messages, that should probably be the one we stick with?
feerate vs fee_rate and “feerate” vs “fee rate” in the PR. I’m not sure which is more correct, considering I’ve seen both used in the wild, but “fee rate” / fee_rate feels more correct?
226@@ -227,7 +227,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
227
Nit:
Typo in the commit message of “scripted-diff: rename m_default_max_tx_fee to m_max_tx_fee” (e602fd275f3632fafa9416281cc375ed622e0876):
0-[…]but con be configured
1+[…]but can be configured
3119@@ -3120,13 +3120,22 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3120 warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee"));
3121 }
3122
3123- if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) {
3124+ walletInstance->m_max_tx_fee = max_fee.value();
In “Wallet: Add maxfeerate wallet startup option” (e4205cf978d24fe77ce555900e4ae77da12b46ec):
Nit: If you are touching a bunch of those lines anyway, is there a chance that you could update max_fee to max_tx_fee? It’s kinda weird that it’s sometimes with and sometimes without _tx_, it would be nice if the name were consistent to make it easier to search for and make it less confusing.
I added a commit that does that https://github.com/bitcoin/bitcoin/pull/29278/commits/ef8403b79585fd1dfd6e8b0a68923985268be16f
Thanks.
3136+ "-maxfeerate", args.GetArg("-maxfeerate", ""), chain->relayMinFee().ToString());
3137 return nullptr;
3138 }
3139
3140- walletInstance->m_max_tx_fee = max_fee.value();
3141+ walletInstance->m_max_tx_fee_rate = CFeeRate(max_fee_rate.value());
In “Wallet: Add maxfeerate wallet startup option” (e4205cf978d24fe77ce555900e4ae77da12b46ec):
Nit: Regarding max_tx_fee_rate vs -maxfeerate vs max_fee_rate vs DEFAULT_TRANSACTION_MAXFEERATE—if you are touching a bunch of those lines anyway, is there a chance that you could update max_fee_rate to max_tx_fee_rate? It’s kinda weird that it’s sometimes with and sometimes without _tx_, it would be nice if the name were consistent to make it easier to search for and make it less confusing.
544@@ -545,9 +545,9 @@ def test_settxfee(self, rbf_node, dest_address):
545 assert_greater_than(Decimal("0.00001000"), abs(requested_feerate - actual_feerate))
546 rbf_node.settxfee(Decimal("0.00000000")) # unset paytxfee
547
548- # check that settxfee respects -maxtxfee
549- self.restart_node(1, ['-maxtxfee=0.000025'] + self.extra_args[1])
550- assert_raises_rpc_error(-8, "txfee cannot be more than wallet max tx fee", rbf_node.settxfee, Decimal('0.00003'))
551+ # check that settxfee respects -maxfeerate
settxfee of course sets a feerate… *sigh*
settxfeerate in a followup?
65@@ -66,6 +66,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
66 argsman.AddArg("-maxapsfee=<n>", strprintf("Spend up to this amount in additional (absolute) fees (in %s) if it allows the use of partial spend avoidance (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
67 argsman.AddArg("-maxtxfee=<amt>", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)",
68 CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
69+ argsman.AddArg("-maxfeerate=<amt>", strprintf("Maximum fee rate (in %s) of a single wallet transaction (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEERATE.GetFeePerK())), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
In “[wallet]: add maxfeerate wallet startup option” (e4205cf978d24fe77ce555900e4ae77da12b46ec):
The help text for this option reads “Maximum fee rate (in BTC) of a single wallet transaction (default: BTC)”.
“BTC” is an amount of bitcoin, not a feerate. Also, the magnitude is missing on the default. Should “(default: BTC)” perhaps read “0.1 BTC/kvB”? Either way the error description should definitely specify the unit in which the feerate is expected here: BTC/kvB, sat/vB, or sat/kvB?
BTC/kvB) of a single wallet transaction (default: 0.10 BTC/kvB)” thank you
79@@ -80,6 +80,13 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
80 return feebumper::Result::WALLET_ERROR;
81 }
82
83+ // check that new fee rate does not exceed maxfeerate
84+ if (newFeerate > wallet.m_max_tx_fee_rate) {
85+ errors.push_back(strprintf(Untranslated("New fee rate %s is too high (cannot be higher than -maxfeerate %s)"),
86+ FormatMoney(newFeerate.GetFeePerK()), FormatMoney(wallet.m_max_tx_fee_rate.GetFeePerK())));
In “[wallet]: enforce -maxfeerate on wallet transactions” (7b858d5ee58cd6276b99072e37da90a16954ac1e):
This error message should provide a unit for the feerates. If it’s BTC/kvB:
0 errors.push_back(strprintf(Untranslated("New fee rate %s BTC/kvB is too high (cannot be higher than -maxfeerate %s BTC/kvB)"),
1 FormatMoney(newFeerate.GetFeePerK()), FormatMoney(wallet.m_max_tx_fee_rate.GetFeePerK())));
565@@ -563,6 +566,9 @@ def test_maxtxfee_fails(self, rbf_node, dest_address):
566 rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
567 rbfid = spend_one_input(rbf_node, dest_address)
568 assert_raises_rpc_error(-4, "Unable to create transaction. Fee exceeds maximum configured by user (maxtxfee)", rbf_node.bumpfee, rbfid)
569+
570+ # When user passed fee rate causes base fee to be above maxtxfee we fail early
571+ assert_raises_rpc_error(-4, "Specified or calculated fee 0.0000282 is too high (cannot be higher than -maxtxfee 0.000025)", rbf_node.bumpfee, rbfid, fee_rate=20)
In “[wallet]: enforce -maxfeerate on wallet transactions” (7b858d5ee58cd6276b99072e37da90a16954ac1e):
Probably out of the scope of this PR, but I wish this error message provided a unit on those numbers.
31@@ -32,7 +32,9 @@ bilingual_str TransactionErrorString(const TransactionError err)
32 case TransactionError::SIGHASH_MISMATCH:
33 return Untranslated("Specified sighash value does not match value stored in PSBT");
34 case TransactionError::MAX_FEE_EXCEEDED:
35- return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)");
36+ return Untranslated("Fee exceeds maximum configured by user (maxtxfee)");
37+ case TransactionError::MAX_FEE_RATE_EXCEEDED:
38+ return Untranslated("Fee rate exceeds maximum configured by user (maxfeerate)");
In “[util]: add a new transaction error type” (1b350aaf819f9fd1037a2e548c013e7c8c22fef4):
Please include the transaction’s feerate and the configured maximum in this error message (including the unit).
This will require changing the TransactionError utility function to accept this data, most of the error types don’t require the data. I thought about using default parameter and passing the parameter MAX_FEE_RATE_EXCEEDED and MAX_FEE_EXCEEDED error type, but it ended up not being a clean refactor as I want https://github.com/bitcoin/bitcoin/commit/2a20d35b28f73f0ff8e7a1de8b2d3833e7957a09
I will leave this as and fix it in a followup along with the comment above ?
183@@ -184,6 +184,30 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
184
185 return res
186
187+ def test_maxfeerate(self):
188+ self.log.info("test -maxfeerate enforcement on wallet transactions.")
189+ # Default maxfeerate is 10,000 sats/vb
190+ # Wallet will reject all transactions with fee rate above 10,000 sats/vb.
In “[wallet]: enforce -maxfeerate on wallet transactions” (7b858d5ee58cd6276b99072e37da90a16954ac1e):
Sorry, I’ll have to die in this hill:
b = bit B = byte ⇒ vbyte = vB
0 # Wallet will reject any transactions with a fee rate above 10,000 sat/vB.
3117@@ -3118,6 +3118,16 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3118 return nullptr;
3119 } else if (max_fee.value() > HIGH_MAX_TX_FEE) {
3120 warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee"));
3121+
3122+ // Wallet prevents creating transactions with fee rates lower than minrelaytxfee.
3123+ // Also the wallet prevents creating transaction with base fee above maxtxfee.
3124+ // Ensure that a 1kvb transaction, with a base fee set to maxtxfee, maintains a fee rate of at least minrelaytxfee.
In “[wallet]: ensure 1kvb tx with -maxtxfee base fee has fee rate atleast minrelaytxfee” (e5b1b43bfdb82fc1b5bc19af04ede21d0de0ff38):
1000 vB is significantly heavier than a minimal useful standard transaction. E.g. a tx with one P2TR input and one P2WPKH output is 99 vB and would have a feerate of 10.1 s/vB at a fee of 1000 sats. I wouldn’t be surprised if someone at some point had a legitimate reason to set a max_fee lower than 1000 sats. Should this therefore perhaps rather just be a warning like above with a max_fee above HIGH_MAX_TX_FEE instead of an error?
3123+ // Also the wallet prevents creating transaction with base fee above maxtxfee.
3124+ // Ensure that a 1kvb transaction, with a base fee set to maxtxfee, maintains a fee rate of at least minrelaytxfee.
3125+ // Otherwise transactions with fee rate greater than or equal to the minrelaytxfee will likely exceed maxtxfee.
3126+ // In such cases, the wallet won't be able to create transactions. Therefore, shut down early.
3127+ } else if (chain && CFeeRate(max_fee.value(), 1000) < chain->relayMinFee()) {
3128+ error = strprintf(_("Invalid amount for %s=<amount>: '%s' conflicts with the minrelay fee of %s. Consider adjusting %s or %s."),
In “[wallet]: ensure 1kvb tx with -maxtxfee base fee has fee rate atleast minrelaytxfee” (e5b1b43bfdb82fc1b5bc19af04ede21d0de0ff38):
max_fee amount was rejected, how about: “Please set a higher maxtxfee or lower -minrelaytxfee before retrying.”
Thanks for your review @josibake @murchandamus
Force pushed from https://github.com/bitcoin/bitcoin/commit/879c048543b1f0239b4650dd0eea2f5215877916 to https://github.com/bitcoin/bitcoin/commit/50d2a010fcd57b58537b711846340c68f6614720 to address recent review comments by @murchandamus compare differences
Changes
max_fee to max_tx_fee to fix ambiguitymax_fee_rate to max_tx_feerate to have consistent variable nameDEFAULT_TRANSACTION_MAXFEERATE to DEFAULT_MAX_TRANSACTION_FEERATE for consistency in namingmaxfeerate option description to “Maximum fee rate (in BTC/kvB) of a single wallet transaction (default: 0.10 BTC/kvB)”.maxfeerate limit is hitsats/vb to a more accurate description sat/vB
https://cirrus-ci.com/task/4669206682140672?logs=ci#L3887
0 test 2024-06-30T16:34:08.989000Z TestFramework (ERROR): Assertion failed
1 Traceback (most recent call last):
2 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
3 self.run_test()
4 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_send.py", line 606, in run_test
5 self.test_weight_limits()
6 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_send.py", line 620, in test_weight_limits
7 self.generate(self.nodes[0], 1)
8 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 707, in generate
9 sync_fun() if sync_fun else self.sync_all()
10 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 787, in sync_all
11 self.sync_blocks(nodes)
12 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 756, in sync_blocks
13 assert (all([len(x.getpeerinfo()) for x in rpc_connections]))
14 AssertionError
thanks @maflcko, will fix the C.I
https://cirrus-ci.com/task/5644803810000896?logs=ci#L2878
0 test 2024-11-12T10:38:10.183000Z TestFramework (ERROR): JSONRPC error
1 Traceback (most recent call last):
2 File "/ci_container_base/test/functional/test_framework/test_framework.py", line 132, in main
3 self.run_test()
4 File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 150, in run_test
5 self.test_22670()
6 File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 1434, in test_22670
7 do_fund_send(lower_bound)
8 File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 1419, in do_fund_send
9 funded_tx = tester.fundrawtransaction(create_tx)
10 File "/ci_container_base/test/functional/test_framework/coverage.py", line 50, in __call__
11 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
12 File "/ci_container_base/test/functional/test_framework/authproxy.py", line 146, in __call__
13 raise JSONRPCException(response['error'], status)
14 test_framework.authproxy.JSONRPCException: Insufficient funds (-4)
It should happen on 10% of the runs. Not sure if you have to rebase.
Let me know if it doesn’t reproduce.
@maflcko Interestingly I got the error after 5 runs.
02024-11-15T19:20:59.167000Z TestFramework (INFO): Test issue 22670 ApproximateBestSubset bug
12024-11-15T19:21:00.082000Z TestFramework (ERROR): JSONRPC error
2Traceback (most recent call last):
3 File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
4 self.run_test()
5 File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/build/test/functional/wallet_fundrawtransaction.py", line 150, in run_test
6 self.test_22670()
7 File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/build/test/functional/wallet_fundrawtransaction.py", line 1434, in test_22670
8 do_fund_send(lower_bound)
9 File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/build/test/functional/wallet_fundrawtransaction.py", line 1419, in do_fund_send
10 funded_tx = tester.fundrawtransaction(create_tx)
11 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12 File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
13 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
14 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15 File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
16 raise JSONRPCException(response['error'], status)
17test_framework.authproxy.JSONRPCException: Insufficient funds (-4)
182024-11-15T19:21:00.144000Z TestFramework (INFO): Stopping nodes
192024-11-15T19:21:00.422000Z TestFramework (WARNING): Not cleaning up dir /var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_per45s2i
202024-11-15T19:21:00.422000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_per45s2i/test_framework.log
212024-11-15T19:21:00.422000Z TestFramework (ERROR):
222024-11-15T19:21:00.422000Z TestFramework (ERROR): Hint: Call /Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/combine_logs.py '/var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_per45s2i' to consolidate all logs
232024-11-15T19:21:00.422000Z TestFramework (ERROR):
242024-11-15T19:21:00.422000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
252024-11-15T19:21:00.422000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
262024-11-15T19:21:00.422000Z TestFramework (ERROR):
63@@ -64,6 +64,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
64 argsman.AddArg("-maxapsfee=<n>", strprintf("Spend up to this amount in additional (absolute) fees (in %s) if it allows the use of partial spend avoidance (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
65 argsman.AddArg("-maxtxfee=<amt>", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)",
66 CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
67+ argsman.AddArg("-maxfeerate=<amt>", strprintf("Maximum fee rate (in %s/kvB) of a single wallet transaction (default: %s %s/kvB)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_TRANSACTION_FEERATE.GetFeePerK()), CURRENCY_UNIT), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
In bb8b8dcfc2ec4088f7bead133f373ea13a5300be “[wallet]: add maxfeerate wallet startup option”:
“of a single wallet transaction” makes sense with an absolute fee, but the feerate scales with the weight so it doesn’t matter whether it applies to a single transaction or multiple. Perhaps just go with: “for wallet transactions”:
0 argsman.AddArg("-maxfeerate=<amt>", strprintf("Maximum fee rate (in %s/kvB) for wallet transactions (default: %s %s/kvB)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_TRANSACTION_FEERATE.GetFeePerK()), CURRENCY_UNIT), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
399@@ -400,7 +400,7 @@ def sendrawtransaction_tests(self):
400
401 def sendrawtransaction_testmempoolaccept_tests(self):
402 self.log.info("Test sendrawtransaction/testmempoolaccept with maxfeerate")
403- fee_exceeds_max = "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)"
404+ fee_exceeds_max = "Fee rate exceeds maximum configured by user (maxfeerate)"
In “[util]: add a new transaction error type” (0aa4f2180e6c2dbf77c214e706cf04c054e491dd):
Given that we now use this value for a feerate and it only appears in this test, let’s name it as such:
0 fee_rate_exceeds_max = "Fee rate exceeds maximum configured by user (maxfeerate)"
570@@ -571,6 +571,20 @@ def test_maxtxfee_fails(self, rbf_node, dest_address):
571
572 # When user passed fee rate causes base fee to be above maxtxfee we fail early
573 assert_raises_rpc_error(-4, "Specified or calculated fee 0.0000282 is too high (cannot be higher than -maxtxfee 0.000025)", rbf_node.bumpfee, rbfid, fee_rate=20)
574+
575+ self.log.info("Test that a low -maxtxfee that will likely prevents txs fee rate to reach -minrelaytxfee should warn users")
In “[wallet]: warn user if 1kvb tx with -maxtxfee base fee has fee rate less than minrelaytxfee” (638a9a3ec0b63e31f92e90dc5822baec925fe4e2):
Nit: The grammar in this test title is a bit wonky
49@@ -49,7 +50,7 @@ BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)
50 static CMutableTransaction TestSimpleSpend(const CTransaction& from, uint32_t index, const CKey& key, const CScript& pubkey)
51 {
52 CMutableTransaction mtx;
53- mtx.vout.emplace_back(from.vout[index].nValue - DEFAULT_TRANSACTION_MAXFEE, pubkey);
54+ mtx.vout.emplace_back(from.vout[index].nValue - DEFAULT_TRANSACTION_MINFEE, pubkey);
In “[node]: update BroadcastTransaction to check fee rate limit” (b480007a4b9341153c4d895176776e0ef93cd200):
It seems to me that this test was testing whether we can create transactions with the MAXFEE. Turning it into MINFEE reduces the utility of this test. If the new MAXFEERATE interferes with this test, I think it would be better to keep the transaction paying MAXFEE, but to increase the MAXFEERATE for this one test, to continue testing that MAXFEE is permitted in itself.
MAXFEERATE and pass the tx fee rate as the max.
ACK e5d1d25912390954b1a064c5c1f9c5d2c1726430
I think the MAXFEE ↦ MINFEE test issue should still be amended, the rest of my comments are not urgent
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36082584858
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.
Thanks for review @murchandamus
Forced pushed from e5d1d25912390954b1a064c5c1f9c5d2c1726430 to 46f0bc5b485130d3e7d14e6855d81d387950cb39 see diff
Changes
maxfeerate startup optionsendrawtransaction_testmempoolaccept_tests fee_exceeds_max variable to fee_rate_exceeds_max.TestSimpleSend helper transaction fee to use MAXFEERATE
ACK https://github.com/bitcoin/bitcoin/pull/29278/commits/46f0bc5b485130d3e7d14e6855d81d387950cb39 per range-diff:
0git range-diff 3714692644f45808a6480525abc36870aeee1de4..e5d1d25912390954b1a064c5c1f9c5d2c1726430 5acf12bafeb126f2190b3f401f95199e0eea90c9..46f0bc5b485130d3e7d14e6855d81d387950cb39
858@@ -858,7 +859,8 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
859 auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
860 m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
861 auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
862- BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
863+ auto tx_vsize = GetVirtualTransactionSize(CTransaction{mempool_tx});
864+ BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, CFeeRate(tx_vsize, DEFAULT_TRANSACTION_MAXFEE), false, error));
CFeeRate construction seem wrong to me - you’ve swapped the size and fee?
901@@ -900,7 +902,8 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
902 block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
903 m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
904 mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
905- BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
906+ auto tx_vsize = GetVirtualTransactionSize(CTransaction{mempool_tx});
907+ BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, CFeeRate(tx_vsize, DEFAULT_TRANSACTION_MAXFEE), false, error));
0@@ -0,0 +1,7 @@
1+wallet startup option
2+========================
3+
4+- A new wallet startip option `-maxfeerate` is added.
0- A new wallet startup option `-maxfeerate` is added.
227@@ -228,7 +228,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
228
229 // Reject absurdly high fee. (This can never happen because the
230 // wallet never creates transactions with fee greater than
231- // m_default_max_tx_fee. This merely a belt-and-suspenders check).
232+ // m_max_tx_fee. This merely a belt-and-suspenders check).
0 // m_max_tx_fee. This is merely a belt-and-suspenders check).
🚧 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.
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.
3007@@ -3008,13 +3008,22 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3008 warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee"));
3009 }
3010
3011- if (chain && CFeeRate{max_tx_fee.value(), 1000} < chain->relayMinFee()) {
3012+ walletInstance->m_max_tx_fee = max_tx_fee.value();
3013+ }
3014+
3015+ if (const auto arg{args.GetArg("-maxfeerate")}) {
In 0d9f0888542081b1792705594b570ae748a9f839 “[wallet]: add maxfeerate wallet startup option”
-maxtxfee adds a warning if the max fee is too high. I think it would make sense to have a similar warning for -maxfeerate.
3010+ // Wallet prevents creating transactions with fee rates lower than minrelaytxfee.
3011+ // Also the wallet prevents creating transaction with base fee above maxtxfee.
3012+ // Warn when a 1kvb transaction, with a base fee set to maxtxfee, has a fee rate less than minrelaytxfee.
3013+ // It is likely that some transactions with fee rates greater than or equal to the minrelaytxfee will exceed maxtxfee.
3014+ // In such cases, the wallet won't be able to create transactions. Therefore, warn the user.
3015+ } else if (chain && CFeeRate(max_tx_fee.value(), 1000) < chain->relayMinFee()) {
In 99b45844cab7896e206dd0b8e24355586bc1a84d “[wallet]: warn user if 1kvb tx with -maxtxfee base fee has fee rate less than minrelaytxfee”
This check does not make sense to me. This converts the max tx fee back into a feerate just for this check, even though -maxfeerate has taken over that check. I don’t think we need to have this check here.
This just serves as a floor value for -maxtxfee.
Since this is a configurable knob, there is a “far-fetched edge case where " a user could start the node with a value so low that it would prevent creating transactions.
Because while creating transaction we ensure the fee is not above -maxtxfee.
A more accurate check would use the actual transaction size, but since size varies per transaction, 1000 is chosen arbitrarily. The idea is to calculate the fee rate when the fee is max_tx_fee and the size is 1000.
If that fee rate is less than minrelaytxfee, we should at least warn the user. cc @josibake
60@@ -61,6 +61,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
61 argsman.AddArg("-maxapsfee=<n>", strprintf("Spend up to this amount in additional (absolute) fees (in %s) if it allows the use of partial spend avoidance (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
62 argsman.AddArg("-maxtxfee=<amt>", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)",
63 CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
64+ argsman.AddArg("-maxfeerate=<amt>", strprintf("Maximum fee rate (in %s/kvB) for a wallet transactions (default: %s %s/kvB)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_TRANSACTION_FEERATE.GetFeePerK()), CURRENCY_UNIT), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
“for a wallet transactions” → “for wallet transactions” [“a” makes the noun plural ‘transactions’ ungrammatical]
🚧 At least one of the CI tasks failed.
Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/49336793803
LLM reason (✨ experimental): CTest timeout on the mptest test causing the CI failure.
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.
503@@ -504,7 +504,7 @@ class WalletImpl : public Wallet
504 return spk_man != nullptr;
505 }
506 OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; }
507- CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
508+ CAmount getDefaultMaxTxFee() override { return m_wallet->m_max_tx_fee; }
getDefaultMaxTxFee for getMaxTxFee too.
3007@@ -3008,13 +3008,25 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3008 warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee"));
3009 }
3010
3011- if (chain && CFeeRate{max_tx_fee.value(), 1000} < chain->relayMinFee()) {
3012+ walletInstance->m_max_tx_fee = max_tx_fee.value();
In 677be6566b3b998a1d7bf66693c5d9748cf3190c:
It seems to me that we had certain relation between max_fee and maxfeerate before that we no longer have. What do you think about parsing maxfeerate first so that you could still check any relation between these two configurations? e.g. that max_tx_fee isn’t ridiculously low/high compared to maxfeerate.
Hmm, these are different options that can be set for various use cases see comment #29278 (comment) and #29278 (comment)
Not exactly sure which relation do you want a check for because there is already check if bounds for each individual option. Perhaps suggest code ?
I was likely thinking of cases where the max tx fee is set below the relay fee rate for any standard tx size but still above the dust limit (iff this is ever possible..).
I assume this already fails on the feerate checks you added, but it might make sense to disallow such a max tx fee in the first place.
It seems we could compute the feerate for the smallest possible standard tx the wallet could create and reject a max tx fee that would fall below the relay feerate.
That being said, I might be overthinking this for no reason. Feel free to close this convo at any time.
3016+ std::optional<CAmount> max_tx_fee_rate = ParseMoney(*arg);
3017+ if (!max_tx_fee_rate) {
3018+ error = AmountErrMsg("maxfeerate", args.GetArg("-maxfeerate", ""));
3019+ return nullptr;
3020+ }
3021+ if (chain && CFeeRate(max_tx_fee_rate.value()) < chain->relayMinFee()) {
In https://github.com/bitcoin/bitcoin/commit/677be6566b3b998a1d7bf66693c5d9748cf3190c:
nano nit: to not construct CFeeRate every time you need it, could extract
0CFeeRate max_fee_rate(*max_tx_fee_rate);
1// rest of the code
547@@ -548,9 +548,9 @@ def test_settxfee(self, rbf_node, dest_address):
548 assert_greater_than(Decimal("0.00001000"), abs(requested_feerate - actual_feerate))
549 rbf_node.settxfee(Decimal("0.00000000")) # unset paytxfee
550
551- # check that settxfee respects -maxtxfee
552- self.restart_node(1, ['-maxtxfee=0.000025'] + self.extra_args[1])
In https://github.com/bitcoin/bitcoin/commit/677be6566b3b998a1d7bf66693c5d9748cf3190c:
this change smells we are now allowing something that was previously not allowed. Like setting a fee through settxfee that is higher than maxtxfee.
Indeed but master is even more smelly because settxfee is not setting tx fee but fee rate :) See discussion #31278 This effectively corrects that by changing the test to check against maxfeerate.
Are you suggesting that I add a test case that checks that we dont create transaction when the tx fee as a result of the fee rate provided by settxfee is more than maxtxfee?
Note: This wallet RPC is likely going to be removed soon.
Are you suggesting that I add a test case that checks that we dont create transaction when the tx fee as a result of the fee rate provided by settxfee is more than maxtxfee?
I’d be surprised if we don’t have a test for that already.
Note: This wallet RPC is likely going to be removed soon.
“soon” is very relative here.
I’d be surprised if we don’t have a test for that already.
Yes we do :)
-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`.
- 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.
- 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.
🚧 At least one of the CI tasks failed.
Task Linux->Windows cross, no tests: https://github.com/bitcoin/bitcoin/actions/runs/19305407337/job/55213729762
LLM reason (✨ experimental): Compiler error: an unused-but-set-variable warning is treated as an error in wallet_tests.cpp, causing the build to fail.
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.