wallet: check for non-representable CFeeRates #20546

jonatack · 2020-12-02T13:19:30Z

As part of our migration to sat/vB feerates that began with #20305, this pull should resolve #20534 and improve/reduce the size of #20391 that adds a setfeerate RPC in sat/vB, as well as for a future estimatefeerate RPC in sat/vB. The code changes are less than 40 lines. Most of the diff is test coverage, as these changes touch a number of RPCs.

Changes:

add CFeeRate::FromSatB and CFeeRate::FromBtcKb named constructors
add a CFeeRate::IsZero() class member helper to replace == CFeeRate(0) conditionals and avoid object allocations/constructions
add a FeeRateFromValueInSatB() utility function that calls AmountFromValue() and then performs an additional check for non-representable CFeeRate values in the range between 0 and 0.001 sat/vB
add unit and functional tests and fill some current fee rate gaps in the functional test coverage

DrahtBot · 2020-12-02T15:58:35Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: createwallet examples for descriptor wallets #21302 (wallet: createwallet examples for descriptor wallets by S3RK)
tests: Run both descriptor and legacy tests within a single test invocation #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
refactor: split CWallet::Create #20773 (refactor: split CWallet::Create by S3RK)
Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2020-12-02T16:19:24Z

the error message and error code should be the same when an invalid BTC/kb amount is rejected

maflcko · 2020-12-02T16:22:42Z

Concept ACK

jonatack · 2020-12-02T16:42:43Z

the error message and error code should be the same when an invalid BTC/kb amount is rejected

The other "too-low" fee messages I am aware of are:

Amount out of range
Fee rate (%s) is lower than the minimum fee rate setting
txfee cannot be less than min relay tx fee
txfee cannot be less than wallet min fee
Invalid amount
Invalid amount for -paytxfee=: '%s' (must be at least %s)
Invalid amount for -maxtxfee=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)
Invalid amount for -fallbackfee=: '%s'
Invalid amount for -%s=: '%s

maflcko · 2020-12-02T16:45:58Z

It would be confusing if 0.00000001 and 0.000000001 produced different error messages

jonatack · 2020-12-02T20:52:28Z

fee_rate=0.000000001

error code: -3
error message:
Invalid amount

...which is not a great error message, as it indicates neither which argument is invalid, nor why. How about:

fee_rate=0.00000001

error code: -3
error message:
Invalid amount for fee_rate (must be at least 0.001 sat/vB)

This is close to these similar error messages:

Invalid amount for -paytxfee=: '%s' (must be at least %s)
Invalid amount for -maxtxfee=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)
Invalid amount for -fallbackfee=: '%s'
Invalid amount for -%s=: '%s

jonatack · 2020-12-03T17:00:04Z

Just found a bug in RPC send. It only allows fee rates to be passed as a number, not as a string. Fixing with a commit here since it fits with the changes. Edit: fixed in #20573 (merged).

maflcko · 2020-12-03T17:38:15Z

...which is not a great error message, as it indicates neither which argument is invalid, nor why.

Then the error message should be fixed. But that is an issue that can be solve completely separate. It shouldn't be a reason to make the behaviour inconsistent.

jonatack · 2020-12-04T12:28:37Z

...which is not a great error message, as it indicates neither which argument is invalid, nor why.

Then the error message should be fixed. But that is an issue that can be solve completely separate. It shouldn't be a reason to make the behaviour inconsistent.

Done. Fixing the rpc send bug and improving the AmountFromValue error messages in separate pulls.

test/functional/wallet_basic.py

fjahr

TBH, I don't understand how this can close #20534 if the behavior is kept consistent. After merge the behavior described there is still the same. An error is caught at the "right" place internally but if the user can't tell the difference this rather seems to be a refactor?

test/functional/rpc_fundrawtransaction.py

jonatack · 2020-12-05T12:10:59Z

The bug fix is that the response is consistent, though I agree it somewhat less user friendly. I have a follow-up that improves the error responses here and in AmountFromValue.

jonatack · 2020-12-05T13:27:57Z

I wonder if this check shouldn't be in AmountFromValue and apply to fundrawtransaction and walletcreatefundedpsbt as well.

jonatack · 2020-12-05T13:29:59Z

E.g. zero is ok for those but between zero and 0.001 sat/vB is invalid.

maflcko · 2020-12-05T13:49:11Z

the check should be in AmountFromValue. My issue is purely about parsing decimals, not about specific rpcs.

To clarify, I might be misunderstanding what you fix here, but approach NACK if this is a fix for the parsing issue I created.

jonatack · 2020-12-06T19:59:31Z

Pulled in the CFeeRate::FromSatB and CFeeRate::FromBtcKb named constructors from the setfeerate PR, added a CFeeRate::IsZero() class member helper, and used these to build a FeeRateFromValueInSatB() utility function. Added unit test coverage for each of these. This should handle the case of parsing decimals for fee rates constructed from sat/vB UniValue values.

test/functional/wallet_send.py

jonatack · 2020-12-07T14:58:23Z

rebased

fjahr

Concept ACK

Thanks for compressing this a bit, makes it easier to review for me at least. I had some thoughts but no major issues. Will follow up soon with another pass.

src/policy/feerate.h

src/test/amount_tests.cpp

and use it instead of constructing CFeeRates in conditionals

similar to AmountFromValue() with an additional check for non-representable CFeeRates in the exclusive range of 0 to 0.001 sat/vB as CFeeRate rounds these values down to CFeeRate(0)

jonatack · 2021-01-26T16:33:15Z

Thanks for reviewing @fjahr, updated with your feedback per git diff 1fc25a4 236f556

fjahr · 2021-01-26T22:36:20Z

Code review ACK 236f556

jarolrod

ACK 236f556

This PR fixes the behavior shown in #20534

Master: Not representable, should through error code: -3

./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009

error code: -6
error message:
Fee rate (0.000 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)

PR: throws error code: -3

./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009

error code: -3
error message:
Invalid amount

maflcko · 2021-01-27T07:43:40Z

(changed title because the only behaviour changes are in the wallet and this is not a refactor)

maflcko

Approach re-NACK

Concept ACK

NACK 236f556 😰

Show signature and timestamp

Signature:

Timestamp of file with hash d9e37613c2875ff346833a9cd2c564a076e87e0eb46dd8c7fa743c1e891d0796 -

src/test/amount_tests.cpp

maflcko · 2021-01-27T08:03:35Z

src/rpc/util.cpp

+    const CAmount amount{AmountFromValue(value)};
+    const CFeeRate fee_rate{CFeeRate::FromSatB(amount)};
+    if (fee_rate.IsZero() && amount != 0) {
+        // passed value is an unrepresentable fee rate between 0 and 0.001 sat/vB


8885b72: Whether a feerate is representable or not has nothing to do with 0

9.99999999999999999999999999999 isn't representable and also isn't zero

You need to fix this in AmountFromValue, as I pointed out earlier: #20546 (comment)

Can you explain how and why this needs to be in AmountFromValue? Do we pass in an is_sat_b_fee_rate bool to it, so it has context? Is that an improvement over a dedicated function? I feel like I'm failing to read your mind on this since a couple months now.

dropped the comment

AmountFromValue parses the string and fails if the amount is not representable, thus any check about representability should be in AmountFromValue.

btc/kb are representable when there are at most 8 digits. sat/b are representable when there are at most 3 digits. How you pass in the digits shouldn't matter. Can be done as normal argument or template argument (and function alias, if this makes the code easier to read).

jonatack · 2021-01-27T11:27:09Z

This PR fixes the behavior shown in #20534

Master: Not representable, should through error code: -3

./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009

error code: -6
error message:
Fee rate (0.000 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)

PR: throws error code: -3

./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009

error code: -3
error message:
Invalid amount

Thanks for testing! This shows that the error message here isn't as helpful as it could be, but I have a branch that improves the various error messages as a follow-up.

jonatack · 2021-01-27T11:55:44Z

NACK 236f556

Can you explain why?

maflcko · 2021-01-27T13:24:31Z

NACK 236f556

Can you explain why?

because it doesn't fix the bug. See:

$ ./src/bitcoin-cli -named sendtoaddress address=bcrt1qwkmhqum095zau5rf2velq55cq938vrd7uw8m4r amount=1.23 fee_rate=31.999999999999999999999
error code: -3
error message:
Invalid amount
$ ./src/bitcoin-cli -named sendtoaddress address=bcrt1qwkmhqum095zau5rf2velq55cq938vrd7uw8m4r amount=1.23 fee_rate=31.99999999
ac1ee7d06c8a276a148bf2378539c957bba643ba343f102779d618d5c95b9049

Both calls should fail because the amount is not representable

jonatack · 2021-01-27T13:46:06Z

Ah, thanks for explaining. I thought that comment was referring to the commit.

DrahtBot · 2021-04-05T14:50:09Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

jonatack · 2021-04-27T15:39:12Z

Can you explain why?

because it doesn't fix the bug. See:

$ ./src/bitcoin-cli -named sendtoaddress address=bcrt1qwkmhqum095zau5rf2velq55cq938vrd7uw8m4r amount=1.23 fee_rate=31.99999999
ac1ee7d06c8a276a148bf2378539c957bba643ba343f102779d618d5c95b9049

This patch fixes the issue described in #20534 "values smaller than 0.001 sat/B can't be represented by CFeeRate, but they are not rejected" but I agree this should also be fixed. The approach here won't suffice so am trying a different one.

jonatack · 2021-04-27T15:56:11Z

Closing for now in favor of #21786.

jonatack mentioned this pull request Dec 2, 2020

sat/b values aren't validated to be in-range #20534

Closed

fanquake added the Wallet label Dec 2, 2020

DrahtBot mentioned this pull request Dec 2, 2020

wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391

Closed

jonatack force-pushed the non-representable-CFeeRate-check branch from 307ecc5 to 03b30e9 Compare December 4, 2020 12:27

jonatack commented Dec 4, 2020

View reviewed changes

test/functional/wallet_basic.py Show resolved Hide resolved

jonatack mentioned this pull request Dec 4, 2020

wallet, bugfix: allow send with string fee_rate amounts #20573

Merged

fjahr reviewed Dec 5, 2020

View reviewed changes

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

jonatack force-pushed the non-representable-CFeeRate-check branch from 03b30e9 to d580e9c Compare December 6, 2020 19:50

jonatack changed the title ~~wallet: check for non-representable CFeeRates (0-0.0009 sat/vB)~~ policy, wallet, refactor: check for non-representable CFeeRates Dec 6, 2020

jonatack force-pushed the non-representable-CFeeRate-check branch from d580e9c to 7d6c6aa Compare December 7, 2020 00:53

DrahtBot mentioned this pull request Dec 7, 2020

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Merged

jonatack commented Dec 7, 2020

View reviewed changes

test/functional/wallet_send.py Outdated Show resolved Hide resolved

jonatack commented Dec 7, 2020

View reviewed changes

test/functional/wallet_send.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Dec 7, 2020

jonatack force-pushed the non-representable-CFeeRate-check branch from 7d6c6aa to 670c6ef Compare December 7, 2020 14:55

DrahtBot mentioned this pull request Jan 23, 2021

Remove names from translatable strings #20404

Closed

fjahr reviewed Jan 26, 2021

View reviewed changes

src/policy/feerate.h Show resolved Hide resolved

src/test/amount_tests.cpp Outdated Show resolved Hide resolved

src/test/amount_tests.cpp Outdated Show resolved Hide resolved

jonatack added 7 commits January 26, 2021 17:16

policy: create CFeeRate::FromSatB/FromBtcKb named ctors

6581118

policy: create CFeeRate::IsZero() class member helper

83fb610

and use it instead of constructing CFeeRates in conditionals

wallet: create FeeRateFromValueInSatB() util function

8885b72

similar to AmountFromValue() with an additional check for non-representable CFeeRates in the exclusive range of 0 to 0.001 sat/vB as CFeeRate rounds these values down to CFeeRate(0)

test: add CFeeRate zero values functional test coverage

17b75d6

test: add non-representable CFeeRate test coverage

4743c86

test: add ParseFixedPoint fee rate rejection coverage

8ba11be

test: add type error and out of range fee rate coverage

236f556

jonatack force-pushed the non-representable-CFeeRate-check branch from 1fc25a4 to 236f556 Compare January 26, 2021 16:31

jarolrod reviewed Jan 26, 2021

View reviewed changes

maflcko removed the Refactoring label Jan 27, 2021

maflcko changed the title ~~policy, wallet, refactor: check for non-representable CFeeRates~~ policy, wallet: check for non-representable CFeeRates Jan 27, 2021

maflcko changed the title ~~policy, wallet: check for non-representable CFeeRates~~ wallet: check for non-representable CFeeRates Jan 27, 2021

maflcko suggested changes Jan 27, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 3, 2021

wallet: createwallet examples for descriptor wallets #21302

Merged

jonatack mentioned this pull request Mar 24, 2021

Add feerate histogram to getmempoolinfo #21422

Closed

DrahtBot added the Needs rebase label Apr 5, 2021

jonatack closed this Apr 27, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

wallet: check for non-representable CFeeRates #20546

wallet: check for non-representable CFeeRates #20546

Uh oh!

Conversation

jonatack commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Dec 2, 2020

Uh oh!

maflcko commented Dec 2, 2020

Uh oh!

jonatack commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 2, 2020

Uh oh!

jonatack commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 3, 2020

Uh oh!

jonatack commented Dec 4, 2020

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Dec 5, 2020

Uh oh!

jonatack commented Dec 5, 2020

Uh oh!

jonatack commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Dec 6, 2020

Uh oh!

Uh oh!

Uh oh!

jonatack commented Dec 7, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jan 26, 2021

Uh oh!

fjahr commented Jan 26, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 27, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 27, 2021

jonatack commented Dec 2, 2020 •

edited

Loading

DrahtBot commented Dec 2, 2020 •

edited

Loading

jonatack commented Dec 2, 2020 •

edited

Loading

jonatack commented Dec 2, 2020 •

edited

Loading

jonatack commented Dec 3, 2020 •

edited

Loading

jonatack commented Dec 5, 2020 •

edited

Loading

maflcko commented Dec 5, 2020 •

edited

Loading

jonatack commented Jan 27, 2021 •

edited

Loading