The PR adds a wallet -maxfeerate startup option, as the upper limit of wallet transactions fee rate.
This fixes the ambiguity of using maxtxfee value to check the upper limit of transactions fee rate.
Wallet will not create a transaction with fee rate above maxfeerate value. And you can not set wallet fee rate with settxfee RPC above maxfeerate value.
This PR adds a functional test that ensure the behavior is enforced.
DrahtBot
commented at 10:23 pm on January 18, 2024:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#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.
DrahtBot added the label
RPC/REST/ZMQ
on Jan 18, 2024
ismaelsadeeq renamed this:
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
on Jan 18, 2024
in
src/init.cpp:630
in
2576efcf68outdated
624@@ -624,7 +625,10 @@ void SetupServerArgs(ArgsManager& argsman)
625 SetupChainParamsBaseOptions(argsman);
626627 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.",
I think you accidentally put the maxburnamount helptext in the incrementalrelayfee helptext here?
ismaelsadeeq
commented at 10:06 pm on January 25, 2024:
This is gone now.
in
src/rpc/mempool.cpp:56
in
2576efcf68outdated
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"},
Why delete these options (suddenly removing support for something that users might rely on)? Even if a config exists, I can imagine somebody wanting to change the maximum for a single RPC call without needing to restart their node.
ismaelsadeeq
commented at 10:06 pm on January 25, 2024:
Reverted thank you.
in
src/wallet/wallet.h:733
in
2576efcf68outdated
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};
731732+ /** 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()};
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.
ismaelsadeeq
commented at 10:07 pm on January 25, 2024:
Done, CFeeRate is the right format since we are representing fee rate.
ismaelsadeeq
commented at 10:11 pm on January 25, 2024:
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.
josibake
commented at 10:28 am on January 30, 2024:
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.
ismaelsadeeq
commented at 1:01 pm on January 31, 2024:
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
glozow
commented at 3:05 pm on January 19, 2024:
member
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.
ismaelsadeeq
commented at 4:12 pm on January 19, 2024:
member
Thanks @glozow for your review.
Will put this PR in draft while addressing Approach feedback
ismaelsadeeq marked this as a draft
on Jan 19, 2024
luke-jr
commented at 4:28 am on January 23, 2024:
member
I agree this feels more like it should be a wallet option.
ismaelsadeeq force-pushed
on Jan 25, 2024
ismaelsadeeq force-pushed
on Jan 25, 2024
DrahtBot added the label
CI failed
on Jan 25, 2024
DrahtBot
commented at 9:53 pm on January 25, 2024:
contributor
🚧 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.
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
ismaelsadeeq marked this as ready for review
on Jan 25, 2024
DrahtBot removed the label
CI failed
on Jan 25, 2024
in
src/wallet/init.cpp:69
in
0c9e0c5b22outdated
61@@ -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);
ismaelsadeeq
commented at 12:41 pm on January 31, 2024:
Yes, fixed here and other place I did the same.
in
src/wallet/rpc/spend.cpp:440
in
677dec10ccoutdated
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()));
josibake
commented at 10:15 am on January 29, 2024:
I think we should keep the word “wallet” here, since this is a client set option. If you look at the RPC call above, it differentiates between the node wide policy of relayMinFee and the wallet set policy m_min_fee by adding the world “wallet” to the RPC error.
in
src/wallet/wallet.cpp:3164
in
677dec10ccoutdated
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 }
30603061- if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) {
josibake
commented at 10:18 am on January 29, 2024:
I don’t think we should remove this check for setting a max_fee that results in fee rates less than the relayMinFee.
ismaelsadeeq
commented at 12:56 pm on January 31, 2024:
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.
josibake
commented at 1:18 pm on January 31, 2024:
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.
ismaelsadeeq
commented at 1:55 pm on January 31, 2024:
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 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.
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)?
josibake
commented at 6:14 pm on January 31, 2024:
Summarizing my thoughts from some offline discussion with @ismaelsadeeq :
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!)
If the user can set both 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 transactions
I think its okay to use a “fake transaction size” for a sanity check, but we should probably call it that i.e. BASELINE_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, etc
There are a few other places where we use maxtxfee to calculate a fake feerate (such as settxfee). We should change those to use maxtxfeerate
In 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.
josibake
commented at 6:27 pm on January 31, 2024:
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
ismaelsadeeq
commented at 8:02 am on February 5, 2024:
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!)
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.
josibake
commented at 10:40 am on January 29, 2024:
member
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
commented at 5:59 pm on January 29, 2024:
member
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
commented at 6:37 pm on January 29, 2024:
member
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
commented at 9:12 pm on January 29, 2024:
member
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.
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
commented at 9:14 pm on January 29, 2024:
member
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
josibake
commented at 9:26 am on January 30, 2024:
member
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:
Check the if the user provided a fee_rate (this fee rate would have already been sanity checked against max_fee_rate)
If not, get a fee rate by doing some checks against min fees and fee estimation
Check max_tx_feeafter the transaction is done (and now max_tx_fee_rate)
josibake
commented at 9:44 am on January 30, 2024:
member
@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.
in
src/wallet/spend.cpp:1299
in
0c9e0c5b22outdated
ismaelsadeeq
commented at 12:40 pm on January 31, 2024:
Added
murchandamus
commented at 6:09 pm on January 31, 2024:
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?
murchandamus
commented at 6:24 pm on January 31, 2024:
I realized now that you are adding the new Error type in the third commit. It would make more sense to me if the new Error type were introduced first, and then the test that uses it would get added rather than adding the test and then later introducing the new Error type.
ismaelsadeeq
commented at 8:04 am on February 5, 2024:
Thanks @murchandamus, I agree with your suggestion and reorder the commits.
ismaelsadeeq force-pushed
on Jan 31, 2024
in
src/util/error.cpp:35
in
d4dceeafc2outdated
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.");
josibake
commented at 1:22 pm on January 31, 2024:
Better to keep the option name in the error message:
0 return Untranslated("Fee exceeds maximum configured by user (maxtxfee)");
ismaelsadeeq
commented at 1:25 pm on January 31, 2024:
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.
murchandamus
commented at 6:20 pm on January 31, 2024:
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.
ismaelsadeeq
commented at 8:23 am on February 5, 2024:
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.
murchandamus
commented at 5:50 pm on January 31, 2024:
contributor
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.
in
src/wallet/rpc/spend.cpp:442
in
7664ec9bd7outdated
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()));
murchandamus
commented at 5:52 pm on January 31, 2024:
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()));
ismaelsadeeq
commented at 8:30 am on February 5, 2024:
Fixed, thanks
in
src/wallet/wallet.cpp:3061
in
7664ec9bd7outdated
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 }
30603061- if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) {
3062+ walletInstance->m_default_max_tx_fee = max_fee.value();
murchandamus
commented at 5:56 pm on January 31, 2024:
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();
ismaelsadeeq
commented at 8:31 am on February 5, 2024:
murchandamus
commented at 6:02 pm on January 31, 2024:
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.
ismaelsadeeq
commented at 8:39 am on February 5, 2024:
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.
murchandamus
commented at 3:41 pm on March 15, 2024:
Yeah, I think that currently a lower limit would be better. For a standalone transaction, we could probably go with 2000 sat/vB, but that might be insufficient for a child transaction bumping a much larger parent. Also see #29661.
murchandamus
commented at 6:27 pm on March 15, 2024:
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?
ismaelsadeeq
commented at 12:57 pm on March 18, 2024:
Fixed thank you
ismaelsadeeq
commented at 1:35 pm on March 18, 2024:
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 2000sat/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?
in
src/wallet/feebumper.cpp:84
in
a4b93767dfoutdated
79@@ -80,6 +80,13 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
80 return feebumper::Result::WALLET_ERROR;
81 }
8283+ // check that new fee rate does not exceed maxfeerate
murchandamus
commented at 6:07 pm on January 31, 2024:
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?
ismaelsadeeq
commented at 9:57 am on February 5, 2024:
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.
josibake
commented at 12:18 pm on February 6, 2024:
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.
murchandamus
commented at 3:42 pm on March 15, 2024:
Yeah, it will probably be easier to introduce that in the context of a cluster mempool where we would much more easily be able to get a read on the effective feerate at which a transaction would be up for block-inclusion.
in
test/functional/wallet_send.py:193
in
a4b93767dfoutdated
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)
murchandamus
commented at 6:14 pm on January 31, 2024:
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:
Try to send at a feerate above the maxfeerate, since that fails the wallet is still in the same state.
Try to send at a feerate just below the maxfeerate and show that it succeeds.
Perhaps use 9900 sat/vB instead of 1000 sat/vB to box the maxfeerate a bit tighter.
ismaelsadeeq
commented at 8:34 am on February 5, 2024:
I’ve implemented your suggestion 👍🏾
in
test/functional/wallet_send.py:208
in
a4b93767dfoutdated
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)
murchandamus
commented at 6:16 pm on January 31, 2024:
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
One transaction but closer to the limit should suffice:
ismaelsadeeq
commented at 8:34 am on February 5, 2024:
Fixed
in
test/functional/rpc_psbt.py:324
in
d4dceeafc2outdated
320@@ -321,7 +321,7 @@ def run_test(self):
321322 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.",
murchandamus
commented at 6:21 pm on January 31, 2024:
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?
ismaelsadeeq
commented at 8:21 am on February 26, 2024:
This is fixed, resolving.
murchandamus
commented at 6:21 pm on January 31, 2024:
contributor
Concept ACK
josibake
commented at 6:30 pm on January 31, 2024:
member
jonatack
commented at 5:19 pm on February 2, 2024:
Adding a new configuration option would need a release note.
ismaelsadeeq
commented at 8:35 am on February 5, 2024:
Added thanks
ismaelsadeeq force-pushed
on Feb 5, 2024
ismaelsadeeq force-pushed
on Feb 5, 2024
DrahtBot added the label
CI failed
on Feb 5, 2024
DrahtBot
commented at 7:38 am on February 5, 2024:
contributor
🚧 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.
in
src/wallet/wallet.cpp:1985
in
30bf6674b1outdated
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);
josibake
commented at 11:53 am on February 6, 2024:
in
src/wallet/wallet.cpp:1985
in
4165dc4128outdated
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);
josibake
commented at 12:01 pm on February 6, 2024:
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.
ismaelsadeeq
commented at 1:24 pm on February 6, 2024:
Yes 👍🏾 its better that way.
in
test/functional/wallet_bumpfee.py:552
in
4165dc4128outdated
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
547548- # 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'))
josibake
commented at 12:08 pm on February 6, 2024:
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
in
src/wallet/rpc/spend.cpp:1489
in
eadc3a4500outdated
ismaelsadeeq
commented at 1:31 pm on February 6, 2024:
Yes 👍🏾
ismaelsadeeq
commented at 1:29 pm on February 8, 2024:
Fixed
in
test/functional/wallet_bumpfee.py:135
in
eadc3a4500outdated
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)
131132 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)",
josibake
commented at 12:22 pm on February 6, 2024:
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.
ismaelsadeeq
commented at 1:40 pm on February 6, 2024:
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 ?
josibake
commented at 3:32 pm on February 6, 2024:
Either sounds fine! The main point is we shouldn’t be removing test cases for maxtxfee while adding maxtxfeerate.
ismaelsadeeq
commented at 1:30 pm on February 8, 2024:
Thanks updated the tests.
in
src/qt/walletmodel.cpp:231
in
a1497af6fcoutdated
230@@ -231,7 +231,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
231232 // 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).
josibake
commented at 12:25 pm on February 6, 2024:
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.
ismaelsadeeq
commented at 1:30 pm on February 8, 2024:
Done
josibake
commented at 4:21 pm on February 26, 2024:
ismaelsadeeq
commented at 10:42 am on February 27, 2024:
Thanks, changed to a scripted-diff
in
src/wallet/wallet.cpp:3066
in
9791ff064coutdated
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."),
josibake
commented at 12:35 pm on February 6, 2024:
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.
ismaelsadeeq
commented at 1:31 pm on February 8, 2024:
Fixed
josibake
commented at 12:37 pm on February 6, 2024:
member
Looking good, and thanks for adding more tests!
ismaelsadeeq force-pushed
on Feb 8, 2024
ismaelsadeeq
commented at 1:34 pm on February 8, 2024:
member
Thanks for your review @josibake
I fixed all review comments
Forced pushed from a5d10367cf832497af2ac72f8c2c42dd25398c63 to 1fae882f98205afb05e96432ce57212226bc0909Compare changes
in
src/node/transaction.cpp:81
in
eedbeccbdboutdated
And this is fixed in next commit 0ad2f2c32460fef9b8cd59e3920fcfd6602b02a4 after we introduced the MAX_FEERATE_EXCEEDED error type.
The options are;
Introduce the error type first, then update BroadcastTransaction to support fee rate checks. This approach however, would result in misleading error messages in sendrawtransaction.
Combine both changes in a single commit.
The current approach, update BroadcastTransaction to support checking fee rate then introduce the new error type which in my opinion, is the better approach.
josibake
commented at 11:10 am on February 13, 2024:
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!
in
src/node/transaction.h:42
in
eedbeccbdboutdated
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)
josibake
commented at 1:00 pm on February 12, 2024:
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)
ismaelsadeeq
commented at 9:19 am on February 13, 2024:
ismaelsadeeq
commented at 2:44 pm on February 12, 2024:
member
Thank you @josibake will address comments shortly!
ismaelsadeeq force-pushed
on Feb 13, 2024
josibake
commented at 11:12 am on February 13, 2024:
member
CI failure doesn’t make any sense given the lines that you changed in your latest force push. Could be a transient failure or a silent merge conflict? FWIW, I rebased locally on master and ran the functional tests without any failures.
glozow requested review from josibake
on Feb 21, 2024
in
src/wallet/rpc/spend.cpp:448
in
6a2c772269outdated
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) {
josibake
commented at 4:30 pm on February 26, 2024:
ismaelsadeeq
commented at 10:42 am on February 27, 2024:
Deleted 👍🏾
in
src/interfaces/chain.h:215
in
368d140db5outdated
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,
josibake
commented at 4:35 pm on February 26, 2024:
nit: in other places, you’ve been calling this max_tx_fee_rate.
ismaelsadeeq
commented at 10:42 am on February 27, 2024:
Fixed
in
doc/release-notes-29278.md:6
in
bf6fc126d1outdated
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`.
josibake
commented at 4:53 pm on February 26, 2024:
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?
ismaelsadeeq
commented at 10:45 am on February 27, 2024:
Fixed the inconsistencies added in this PR, I am sticking to fee rate as u suggested, there are still others remaining in the codebase fixing that it is beyond the scope of this PR.
josibake
commented at 4:57 pm on February 26, 2024:
member
Overall, looking good! Left some nits and I noticed inconsistent usage of 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_ratefeels more correct?
ismaelsadeeq force-pushed
on Feb 27, 2024
ismaelsadeeq force-pushed
on Feb 27, 2024
DrahtBot added the label
CI failed
on Feb 27, 2024
DrahtBot removed the label
CI failed
on Feb 27, 2024
josibake
commented at 12:57 pm on February 28, 2024:
member
murchandamus
commented at 3:51 pm on March 15, 2024:
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
ismaelsadeeq
commented at 10:27 am on March 18, 2024:
Fixed
in
src/wallet/wallet.cpp:3123
in
e4205cf978outdated
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 }
31223123- if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) {
3124+ walletInstance->m_max_tx_fee = max_fee.value();
murchandamus
commented at 5:51 pm on March 15, 2024:
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.
ismaelsadeeq
commented at 10:31 am on March 18, 2024:
murchandamus
commented at 5:52 pm on March 15, 2024:
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.
ismaelsadeeq
commented at 10:31 am on March 18, 2024:
Fixed
in
test/functional/wallet_bumpfee.py:553
in
e4205cf978outdated
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
547548- # 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
murchandamus
commented at 6:16 pm on March 15, 2024:
Note to self and others: settxfee of course sets a feerate… *sigh*
ismaelsadeeq
commented at 12:59 pm on March 18, 2024:
I think should be updated to settxfeerate in a followup?
in
src/wallet/init.cpp:69
in
e4205cf978outdated
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);
murchandamus
commented at 6:57 pm on March 15, 2024:
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?
ismaelsadeeq
commented at 12:56 pm on March 18, 2024:
Thats right reworded to “Maximum fee rate (in BTC/kvB) of a single wallet transaction (default: 0.10BTC/kvB)” thank you
in
src/wallet/feebumper.cpp:86
in
7b858d5ee5outdated
79@@ -80,6 +80,13 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
80 return feebumper::Result::WALLET_ERROR;
81 }
8283+ // 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())));
murchandamus
commented at 7:03 pm on March 15, 2024:
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())));
ismaelsadeeq
commented at 12:54 pm on March 18, 2024:
Fixed
in
test/functional/wallet_bumpfee.py:573
in
7b858d5ee5outdated
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)
murchandamus
commented at 7:16 pm on March 15, 2024:
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.
in
src/util/error.cpp:37
in
1b350aaf81outdated
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)");
murchandamus
commented at 7:19 pm on March 15, 2024:
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).
ismaelsadeeq
commented at 12:50 pm on March 18, 2024:
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 ?
in
test/functional/wallet_send.py:190
in
7b858d5ee5outdated
183@@ -184,6 +184,30 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
184185 return res
186187+ 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.
murchandamus
commented at 7:26 pm on March 15, 2024:
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.
ismaelsadeeq
commented at 12:51 pm on March 18, 2024:
I agree with your suggestion. Fixed
in
src/wallet/wallet.cpp:3124
in
e5b1b43bfdoutdated
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.
murchandamus
commented at 7:59 pm on March 15, 2024:
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?
ismaelsadeeq
commented at 12:52 pm on March 18, 2024:
Fixed, it’s now a warning instead a startup failure..
in
src/wallet/wallet.cpp:3128
in
e5b1b43bfdoutdated
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."),
murchandamus
commented at 8:04 pm on March 15, 2024:
In “[wallet]: ensure 1kvb tx with -maxtxfee base fee has fee rate atleast minrelaytxfee” (e5b1b43bfdb82fc1b5bc19af04ede21d0de0ff38):
“minrelay fee” is a feerate, not a fee: How about: “[…] conflicts with the minimum relay transaction feerate […]”
“Consider” is a bit funny given that the max_fee amount was rejected, how about: “Please set a higher maxtxfee or lower -minrelaytxfee before retrying.”
ismaelsadeeq
commented at 12:52 pm on March 18, 2024:
Reworded to your suggestion
ismaelsadeeq force-pushed
on Mar 18, 2024
ismaelsadeeq force-pushed
on Mar 18, 2024
DrahtBot added the label
CI failed
on Mar 18, 2024
DrahtBot removed the label
CI failed
on Mar 18, 2024
ismaelsadeeq
commented at 1:47 pm on March 18, 2024:
member
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
ismaelsadeeq
commented at 5:48 pm on July 2, 2024:
member
scripted-diff: rename `m_default_max_tx_fee` to `m_max_tx_fee`
-BEGIN VERIFY SCRIPT-
git grep -l "m_default_max_tx_fee" src | xargs sed -i "s/m_default_max_tx_fee/m_max_tx_fee/g"
-END VERIFY SCRIPT-
- The value is not always the default but can be configured
during startup so `m_max_tx_fee` is the right name not
`m_default_max_tx_fee`.
7b26847efe
[wallet]: update `max_fee` to `max_tx_fee`
- Also update `-maxapsfee` option from `max_fee` to `max_aps_fee`.
This fixes the ambiguity in the variable name.
2947a325f5
[wallet]: add `maxfeerate` wallet startup option
- 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.
bb8b8dcfc2
[node]: update `BroadcastTransaction` to check fee rate 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` should create transaction with base fee of
`DEFAULT_TRANSACTION_MINFEE`, 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`.
- Wallet unit test is updated to check the fee rate limit when broadcasting
transactions using `DEFAULT_MAX_TRANSACTION_FEERATE` as the threshold.
b480007a4b
[util]: add a new transaction error type
- This distinguishes maxfeerate and maxtxfee error messages.
0aa4f2180e
[wallet]: enforce `-maxfeerate` on wallet transactions
- This commit prevents the wallet from creating transactions
above `maxfeerate`, and tests the new functionality.
f9939af553
[wallet]: warn user if 1kvb tx with `-maxtxfee` base fee has fee rate less than `minrelaytxfee`
- Wallet prevents creating transactions with fee rates lower than `-minrelaytxfee`.
Also prevents creating a transaction with a base fee above `-maxtxfee`.
- Warn user if a 1kvb transaction, with a base fee set to `-maxtxfee`, has a fee rate less than
`-minrelaytxfee`. It is likely that some transactions with fee rates greater than or equal
to the `-minrelaytxfee` will also likely exceed `-maxtxfee`, the wallet won't be able to create
transactions.
638a9a3ec0
[doc]: add release notese5d1d25912
ismaelsadeeq force-pushed
on Jul 3, 2024
DrahtBot removed the label
CI failed
on Jul 3, 2024
DrahtBot added the label
CI failed
on Aug 11, 2024
DrahtBot removed the label
CI failed
on Aug 17, 2024
DrahtBot added the label
CI failed
on Oct 20, 2024
DrahtBot removed the label
CI failed
on Oct 24, 2024
DrahtBot added the label
CI failed
on Nov 12, 2024
DrahtBot
commented at 2:52 pm on November 13, 2024:
contributor
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)
ismaelsadeeq
commented at 6:49 pm on November 15, 2024:
member
@DrahtBot I think C.I failure here is unrelated?
I was not successful in my attempt to recreate it locally.
maflcko
commented at 7:05 pm on November 15, 2024:
member
The failure happens intermittently. How often did you try?
maflcko
commented at 7:07 pm on November 15, 2024:
member
It should happen on 10% of the runs. Not sure if you have to rebase.
Let me know if it doesn’t reproduce.
ismaelsadeeq
commented at 7:25 pm on November 15, 2024:
member
@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):
3File"/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
4 self.run_test()
5File"/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/build/test/functional/wallet_fundrawtransaction.py", line 150, in run_test
6 self.test_22670()
7File"/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/build/test/functional/wallet_fundrawtransaction.py", line 1434, in test_22670
8 do_fund_send(lower_bound)
9File"/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^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^12File"/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^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^15File"/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):
DrahtBot removed the label
CI failed
on Nov 16, 2024
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-01-08 06:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me