wallet: check for non-representable CFeeRates

jonatack commented at 1:19 pm on December 2, 2020: member

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

fanquake added the label Wallet on Dec 2, 2020

DrahtBot commented at 3:58 pm on December 2, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21302 (wallet: createwallet examples for descriptor wallets by S3RK)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
#20773 (refactor: split CWallet::Create by S3RK)
#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.

MarcoFalke commented at 4:19 pm on December 2, 2020: member

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

MarcoFalke commented at 4:22 pm on December 2, 2020: member

Concept ACK

jonatack commented at 4:42 pm on December 2, 2020: member

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

MarcoFalke commented at 4:45 pm on December 2, 2020: member

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

jonatack commented at 8:52 pm on December 2, 2020: member

fee_rate=0.000000001

0error code: -3
1error message:
2Invalid amount

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

fee_rate=0.00000001

0error code: -3
1error message:
2Invalid 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 commented at 5:00 pm on December 3, 2020: member

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

MarcoFalke commented at 5:38 pm on December 3, 2020: member

…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 force-pushed on Dec 4, 2020

jonatack commented at 12:28 pm on December 4, 2020: member

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

in test/functional/wallet_basic.py:279 in 03b30e978a outdated

260+        # Test zero and values non-representable by CFeeRate
261+        for invalid_value in ["0", 0, 0.00000001, 0.0009, 0.00099999]:
262+            assert_raises_rpc_error(-3, msg, self.nodes[2].sendmany, amounts={address: 10}, fee_rate=invalid_value)
263+        # Test values rejected by ParseFixedPoint() called in AmountFromValue()
264+        for invalid_value in ["", 0.000000001, 1.111111111, 11111111111]:
265+            assert_raises_rpc_error(-3, msg, self.nodes[2].sendmany, amounts={address: 1.0}, fee_rate=invalid_value)

jonatack commented at 12:33 pm on December 4, 2020:

Am testing CFeeRate errors separately from AmountFromValue errors, as a follow-up PR will improve the messages of both.

in test/functional/rpc_fundrawtransaction.py:768 in b70a0e1b3b outdated

764@@ -765,8 +765,9 @@ def test_option_feerate(self):
765                 node.fundrawtransaction, rawtx, {param: -1, "add_inputs": True})
766             assert_raises_rpc_error(-3, "Amount is not a number or string",
767                 node.fundrawtransaction, rawtx, {param: {"foo": "bar"}, "add_inputs": True})
768-            assert_raises_rpc_error(-3, "Invalid amount",
769-                node.fundrawtransaction, rawtx, {param: "", "add_inputs": True})
770+            # Test values rejected by ParseFixedPoint() called in AmountFromValue()

fjahr commented at 11:33 am on December 5, 2020:

In b70a0e1b3bdb3aff4f3b41dc7a0bd6e52d67e6af:

This is C++ code internals referenced in the functional test comments. IMO that doesn’t belong here since it’s basically unmaintainable and I don’t think we do this anywhere else. Unless there is a really strong reason I am not seeing, please remove it. IMO the test with the log message above is explanatory enough.

Same for all the other places this occurs in this commit.

jonatack commented at 12:16 pm on December 5, 2020:

We do refer to the implementation sometimes, perhaps most often when defining constants.

This is to document why these values are tested separately for the same message. I have a follow-up to improve these messages in different ways, e.g. not just “Invalid amount” but also provide context to the user about which field and in some cases the valid range or values. Could remove the documentation in the follow-up if it’s not useful, or provide more general info on what is being tested.

fjahr commented at 12:02 pm on December 5, 2020: member

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?

jonatack commented at 12:10 pm on December 5, 2020: member

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 commented at 1:27 pm on December 5, 2020: member

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

jonatack commented at 1:29 pm on December 5, 2020: member

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

MarcoFalke commented at 1:49 pm on December 5, 2020: member

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 force-pushed on Dec 6, 2020

jonatack renamed this:
~~wallet: check for non-representable CFeeRates (0-0.0009 sat/vB)~~
policy, wallet, refactor: check for non-representable CFeeRates
on Dec 6, 2020

jonatack commented at 7:59 pm on December 6, 2020: member

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.

jonatack force-pushed on Dec 7, 2020

in test/functional/wallet_send.py:314 in 7d6c6aaa64 outdated

318+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=0.99999999, expect_error=(-4, msg))
319+
320+        self.log.info("Explicit fee rate raises RPC error when invalid fee rates are passed")
321+        # Test fee_rate with zero values
322+        msg = "Fee rate (0.000 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)"
323+        for zero_value in [0, 0.000, 0.00000000]:

jonatack commented at 12:05 pm on December 7, 2020:

Note to self: update this line after #20573 is merged and send can take string fee rate values

0        for zero_value in [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]:

jonatack commented at 7:08 pm on December 10, 2020:

updated

in test/functional/wallet_send.py:323 in 7d6c6aaa64 outdated

327+        msg = "Invalid amount"
328+        for invalid_value in [0.00000001, 0.0009, 0.00099999]:
329+            self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=invalid_value, expect_error=(-3, msg))
330+            self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=invalid_value, expect_error=(-3, msg))
331+        # Test fee_rate values rejected by ParseFixedPoint
332+        for invalid_value in [0.000000001, 1.111111111, 11111111111]:

jonatack commented at 12:06 pm on December 7, 2020:

Note to self: update this line as well after #20573 is merged

0        for invalid_value in ["", 0.000000001, 1.111111111, 11111111111]:

jonatack commented at 7:09 pm on December 10, 2020:

updated

DrahtBot added the label Needs rebase on Dec 7, 2020

jonatack force-pushed on Dec 7, 2020

jonatack commented at 2:58 pm on December 7, 2020: member

rebased

DrahtBot removed the label Needs rebase on Dec 7, 2020

DrahtBot added the label Needs rebase on Dec 10, 2020

laanwj added this to the "Blockers" column in a project

jonatack force-pushed on Dec 10, 2020

jonatack commented at 8:11 pm on December 10, 2020: member

Updated now that #20573 has been merged.

DrahtBot removed the label Needs rebase on Dec 10, 2020

jonatack force-pushed on Dec 17, 2020

laanwj removed this from the "Blockers" column in a project

jonatack closed this on Jan 3, 2021

jonatack commented at 11:12 am on January 14, 2021: member

Re-opening for review club to hopefully get some eyes here.

jonatack reopened this on Jan 14, 2021

jonatack commented at 12:30 pm on January 15, 2021: member

Suggest the following labels be added, please: TX fees and policy, Refactoring, Review club

fanquake added the label Review club on Jan 15, 2021

fanquake added the label TX fees and policy on Jan 15, 2021

fanquake added the label Refactoring on Jan 15, 2021

jonatack commented at 4:41 pm on January 15, 2021: member

Thanks @fanquake

felipsoarez commented at 5:49 pm on January 15, 2021: none

utACK

in src/test/rpc_tests.cpp:222 in 54dcf5ab2e outdated

216@@ -214,6 +217,30 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values)
217     BOOST_CHECK_THROW(AmountFromValue(ValueFromString("1e+11")), UniValue); //overflow error
218     BOOST_CHECK_THROW(AmountFromValue(ValueFromString("1e11")), UniValue); //overflow error signless
219     BOOST_CHECK_THROW(AmountFromValue(ValueFromString("93e+9")), UniValue); //overflow error
220+
221+    // values rejected by ParseFixedPoint raise invalid amount
222+    BOOST_CHECK_THROW(AmountFromValue(ValueFromString("0.000000001")), UniValue);

fanquake commented at 1:02 am on January 20, 2021:

54dcf5ab2e25f6eacf8955ab18a0cd92ef165a4e: Is there anything special about 11111111111 that wont be caught by one of the existing AmountFromValue tests? Looks like it’s just a big number. Same for 1e-9, which is tested above.

Not sure about the value of comments like values rejected by ParseFixedPoint raise invalid amount. What’s useful about naming the function called internally by AmountFromValue and the runtime error? We don’t actually check for invalid amount, just that something throws. None of these tests will fail with:

 0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
 1index 1a83b9222..0199fd539 100644
 2--- a/src/rpc/util.cpp
 3+++ b/src/rpc/util.cpp
 4@@ -81,7 +81,7 @@ CAmount AmountFromValue(const UniValue& value)
 5         throw JSONRPCError(RPC_TYPE_ERROR, "Amount is not a number or string");
 6     CAmount amount;
 7     if (!ParseFixedPoint(value.getValStr(), 8, &amount))
 8-        throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
 9+        throw JSONRPCError(RPC_TYPE_ERROR, "just throw whatever!");
10     if (!MoneyRange(amount))
11         throw JSONRPCError(RPC_TYPE_ERROR, "Amount out of range");
12     return amount;

In general these seem like notes that will likely just become outdated in regards to the code.

jonatack commented at 8:50 pm on January 22, 2021:

Dropped these assertions and comments.

in src/policy/feerate.h:37 in d2b40010fa outdated

33@@ -34,6 +34,7 @@ class CFeeRate
34 public:
35     /** Fee rate of 0 satoshis per kB */
36     CFeeRate() : nSatoshisPerK(0) { }
37+    bool IsZero() const { return nSatoshisPerK == 0; }

fanquake commented at 1:12 am on January 20, 2021:

In d2b40010fab5a1ef7f6d946da076d4de6b442e09: This is simple enough that you can combine e4a3bceef3d9483961c1f0a2f70b6a4e9a225acd and ed414f6dd72c103b5ba9e17c6b6bd2bcc8548b5b into it. Otherwise we are starting to get very granular commits.

It’s also inconsistent with other changes in this PR, as in 85e0fe3f783ebaabbf9b27e0b452ba8073905db5 you add the FromSatB & FromBtcKb constructors and refactor to use them at the same time.

jonatack commented at 8:51 pm on January 22, 2021:

Combined the CFeeRate::IsZero() creation, unit tests, and codebase updates into a single commit.

jonatack commented at 8:53 pm on January 22, 2021:

Idem for the named ctors and for the FeeRateFromValueInSatB utility function: creation, unit tests, and codebase updates all in a single commit.

in test/functional/wallet_basic.py:276 in 3df75585cc outdated

272@@ -273,6 +273,9 @@ def run_test(self):
273         msg = "Invalid amount"
274         for invalid_value in [0.00000001, 0.00099999, "0.00000001", "0.00099999"]:
275             assert_raises_rpc_error(-3, msg, self.nodes[2].sendmany, amounts={address: 10}, fee_rate=invalid_value)
276+        # Test fee_rate values rejected by ParseFixedPoint

fanquake commented at 1:30 am on January 20, 2021:

3df75585cc2595ff3d1065e6fdaa4e9df8716594:

Am testing CFeeRate errors separately from AmountFromValue errors, as a follow-up PR will improve the messages of both.

I think it’d be better to add the additional tests when the different error messages are added. Otherwise at the moment we’ve got 24 additional “tests” which just pass values into wallet RPCs, and check that invalid value is thrown. Basically all following the same code path.

Same point as in a different commit, I don’t think details like ParseFixedPoint() belong in comments here.

jonatack commented at 11:33 am on January 20, 2021:

I think all the coverage is relevant? The documentation is helpful to me when coming back to this months later (like now) to regain context. When I’m done with the fee rate work, I’m happy to remove the documentation.

jonatack commented at 8:54 pm on January 22, 2021:

Changed the comments in this commit to not name the specific function: “Test fee_rate values that don’t pass fixed-point parsing checks”.

in test/functional/rpc_fundrawtransaction.py:772 in d7c2399f3e outdated

767@@ -768,6 +768,10 @@ def test_option_feerate(self):
768                 node.fundrawtransaction, rawtx, {param: {"foo": "bar"}, "add_inputs": True})
769             assert_raises_rpc_error(-3, "Invalid amount",
770                 node.fundrawtransaction, rawtx, {param: "", "add_inputs": True})
771+        # Test fee_rate values non-representable by CFeeRate

fanquake commented at 1:40 am on January 20, 2021:

d7c2399f3e23ce54d27f8ef26077e58324b991d8: I must be misunderstanding this, otherwise it looks like it just does the same thing twice. i.e:

0node.fundrawtransaction(fee_rate: 0.00000001) -> throw("Invalid amount")
1node.fundrawtransaction(fee_rate: 0.00099999) -> throw("Invalid amount")
2node.fundrawtransaction(fee_rate: 0.00000001) -> throw("Invalid amount")
3node.fundrawtransaction(fee_rate: 0.00099999) -> throw("Invalid amount")

Same for most of the other files in this commit.

jonatack commented at 11:07 am on January 20, 2021:

Fee rates should be accepted as an amount, e.g. a string or a number, as documented in the sendtoaddress, sendmany, fundrawtransaction, walletcreatefundedpsbt, send, and bumpfee helps. Adding this testing found the bug in #20573.

fanquake commented at 11:32 am on January 20, 2021:

If this is just to test that both amount and string fee rates are accepted, isn’t that also tested by either

0for param, zero_value in product(["fee_rate", "feeRate"], [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]):

or

0for invalid_value in ["", 0.000000001, 1.111111111, 11111111111]:

(both added above) which use a mix of strings and numbers? Or the test changes made in #20573, which added specific tests for “Test passing fee_rate as an integer” and “Test passing fee_rate as a string”?

jonatack commented at 11:47 am on January 20, 2021:

I think they are testing different cases/codepaths, as documented, reverifying (I’m grateful for the documentation in picking up the context again nearly two months later.)

jonatack commented at 1:37 pm on January 20, 2021:

I reverified that:

the “Test fee_rate values non-representable by CFeeRate” section verifies the exception raised in the new FeeRateFromValueInSatB() function (one can test this by changing the error message that function returns)
the “Test fee_rate values rejected by ParseFixedPoint” section verifies the rejection by ParseFixedPoint() in AmountFromValue()

I’d like these cases to be tested separately to make sure the new function is raising on the amounts it is intended to. The next step is for both exceptions to raise better, more specific error messages.

in src/test/rpc_tests.cpp:243 in ed414f6dd7 outdated

238+    BOOST_CHECK_THROW(FeeRateFromValueInSatB(ValueFromString("0.00099999")), UniValue);
239+    BOOST_CHECK_THROW(FeeRateFromValueInSatB(ValueFromString("0.00000001")), UniValue);
240+
241+    // values rejected by ParseFixedPoint raise invalid amount
242+    BOOST_CHECK_THROW(FeeRateFromValueInSatB(ValueFromString("0.000999999")), UniValue);
243+    BOOST_CHECK_THROW(FeeRateFromValueInSatB(ValueFromString("0.000000001")), UniValue);

fanquake commented at 2:16 am on January 20, 2021:

How far do we want to reach with these tests. This is basically “testing” that FeeRateFromValueInSatB() calls AmountFromValue() calls ParseFixedPoint() & throws. Which is the same as:

0BOOST_CHECK_THROW(AmountFromValue(ValueFromString("0.000000001")), UniValue);

above, with an additional function call to get to throw.

However, because nothing specific is being checked, you could imagine how FeeRateFromValueInSatB() may end up being refactored such that there is some other throwing call, i.e:

 0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
 1index 1a83b9222..02604b595 100644
 2--- a/src/rpc/util.cpp
 3+++ b/src/rpc/util.cpp
 4@@ -89,6 +89,7 @@ CAmount AmountFromValue(const UniValue& value)
 5 
 6 CFeeRate FeeRateFromValueInSatB(const UniValue& value)
 7 {
 8+    this_might_throw();
 9     const CAmount amount{AmountFromValue(value)};
10     const CFeeRate fee_rate{CFeeRate::FromSatB(amount)};

and the tests still pass, but only because this_might_throw() throws for some reason. Now what are we testing? This is also why I’m not a fan of the raise invalid amount type comments. That might be what we want the code to be testing, but that doesn’t mean that’s what it’s doing.

jonatack commented at 11:37 am on January 20, 2021:

Relooking at it in light of your thoughts here.

jonatack commented at 8:56 pm on January 22, 2021:

Dropped these unit tests and comments and combined the tests into the same commit as the function creation and the codebase updates.

fanquake commented at 2:53 am on January 20, 2021: member

It would seem like we could do with some sort of fee rate specific functional test, as we are getting to the point where basically all wallet RPC related functional tests are having (essentially) the same blocks copied and pasted between all of them. This seems like an awful lot of duplication, where code easily get out of “sync”, for not much additional coverage.

jonatack commented at 12:01 pm on January 20, 2021: member

It would seem like we could do with some sort of fee rate specific functional test, as we are getting to the point where basically all wallet RPC related functional tests are having (essentially) the same blocks copied and pasted between all of them. This seems like an awful lot of duplication, where code easily get out of “sync”, for not much additional coverage.

Well, only six (sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee) out of the sixty-some wallet/rpc functional tests, e.g. only for each of the send calls where it is important to have coverage.

Most of the work in #20220, #20305, #20426, and #20573 was writing tests, but the tests found a number of issues in different calls because coverage was previously partial or absent.

While grouping the coverage into a fee rate specific test file is out of scope of this patch, I’m not sure it would reduce much duplication. Functional tests are end-to-end and need to exercise the endpoints or we’ll end up back at square one with bugs. The coverage is unfortunately not copy-and-paste (I wish it were!), but having the test blocks look similar and cover evenly was a goal, because where the coverage was previously uneven, I came across issues, and it’s pretty easy to omit coverage somewhere…gaps in coverage here tend to come back and bite and can affect users. It’s also nice to have coverage to protect against future regressions and help changes be easier and safer.

theStack commented at 11:16 pm on January 20, 2021: member

Concept ACK! Unfortunately I missed the review club for this PR today, but went over the logs and it was a very interesting read.

Some thoughts:

agree with fanquake’s suggestion that commits https://github.com/bitcoin/bitcoin/pull/20546/commits/d2b40010fab5a1ef7f6d946da076d4de6b442e09 and https://github.com/bitcoin/bitcoin/pull/20546/commits/ed414f6dd72c103b5ba9e17c6b6bd2bcc8548b5b should be squashed; for newly introduced methods/functions that are really short, I barely see any benefit in splitting up the usage of it into another commit (other potential candidates are https://github.com/bitcoin/bitcoin/pull/20546/commits/ec62ce0997167b6110e029cc3b55afd97482fe4c + https://github.com/bitcoin/bitcoin/pull/20546/commits/ab4b12823dee3b8659ea2bc1fae43fb4efb6ecfb) generally for a merely 40 LOC change the number of commits seems quite large
nice idea to introduce named constructors, makes the code way more readable 👍
with those, do we still need the two-parameter ctor CFeeRate(const CAmount& nFeePaid, size_t nBytes) to be public? all instances I see are either in test code or can be replaced by the named constructors now, so I guess there is not much point in exposing it

jonatack commented at 9:00 pm on January 22, 2021: member

The first commit now applies the named ctor CFeeRate::FromBtcKb to two additional calls in wallet/wallet.cpp where it can replace the two-parameter (fee, 1000) ctor.

do we still need the two-parameter ctor CFeeRate(const CAmount& nFeePaid, size_t nBytes) to be public? all instances I see are either in test code or can be replaced by the named constructors now, so I guess there is not much point in exposing it

I looked at making it private but it is still used extensively in the codebase with the transaction size passed in as nBytes:

0src/miner.cpp::L241
1src/node/psbt.cpp::L140
2src/policy/fees.cpp::L549,579
3src/txmempool.cpp::L1037
4src/validation.cpp::L816,834
5src/wallet/feebumper.cpp::L86,120

(and in these tests)

0src/test/amount_tests.cpp
1src/test/mempool_tests.cpp
2src/test/amount_tests.cpp
3src/test/policyestimator_tests.cpp

jonatack force-pushed on Jan 22, 2021

jonatack commented at 9:10 pm on January 22, 2021: member

Squashed a bunch of commits from 12 down to 7. Hopefully this is more pleasing to reviewers while remaining easy to review.

in src/policy/feerate.h:77 in afba3e188f outdated

71@@ -70,4 +72,10 @@ class CFeeRate
72     SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.nSatoshisPerK); }
73 };
74 
75+/** Construct a CFeeRate from a CAmount in sat/vB */
76+inline CFeeRate CFeeRate::FromSatB(CAmount fee_rate) { return CFeeRate(fee_rate, COIN); }

fjahr commented at 11:57 pm on January 25, 2021:

in afba3e188fcb14236324a0d1355445a99b85d155:

Why not just have the body of the ctor in the class?

jonatack commented at 3:26 pm on January 26, 2021:

Here is a better explanation than I could provide: https://isocpp.org/wiki/faq/ctors#named-ctor-idiom by Stroustrup, Cline, Sutter and Alexandrescu. This is an implementation of that idiom.

fjahr commented at 10:35 pm on January 26, 2021:

I did understand the idiom, the point I was trying to make that this should be functionally equivalent but just a bit simpler:

 0@ src/policy/feerate.h:43 @ public:
 1         // We've previously had bugs creep in from silent double->int conversion...
 2         static_assert(std::is_integral<I>::value, "CFeeRate should be used without floats");
 3     }
 4-    static CFeeRate FromSatB(CAmount fee_rate);
 5-    static CFeeRate FromBtcKb(CAmount fee_rate);
 6+    /** Construct a CFeeRate from a CAmount in sat/vB */
 7+    static CFeeRate FromSatB(CAmount fee_rate) { return CFeeRate(fee_rate, COIN); };
 8+    /** Construct a CFeeRate from a CAmount in BTC/kvB */
 9+    static CFeeRate FromBtcKb(CAmount fee_rate) { return CFeeRate(fee_rate, 1000); };
10     /** Constructor for a fee rate in satoshis per kvB (sat/kvB). The size in bytes must not exceed (2^63 - 1).
11      *
12      *  Passing an nBytes value of COIN (1e8) returns a fee rate in satoshis per vB (sat/vB),
13@ src/policy/feerate.h:80 @ public:
14     SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.nSatoshisPerK); }
15 };
16
17-/** Construct a CFeeRate from a CAmount in sat/vB */
18-inline CFeeRate CFeeRate::FromSatB(CAmount fee_rate) { return CFeeRate(fee_rate, COIN); }
19-
20-/** Construct a CFeeRate from a CAmount in BTC/kvB */
21-inline CFeeRate CFeeRate::FromBtcKb(CAmount fee_rate) { return CFeeRate(fee_rate, 1000); }
22-
23 #endif //  BITCOIN_POLICY_FEERATE_H

It seems the inline definition outside of the body is mostly interesting if you want to do that multiple times in different translation units.

jonatack commented at 10:52 pm on January 26, 2021:

Oh! I’ll look into it.

MarcoFalke commented at 1:25 pm on January 27, 2021:

I also prefer what @fjahr suggested, assuming it compiles

jonatack commented at 3:31 pm on April 27, 2021:

Thanks @fjahr. Your suggestion works; will use it if named constructors are needed in the future. Currently trying a different approach to this issue.

in src/test/amount_tests.cpp:82 in b427ce1785 outdated

77 
78 BOOST_AUTO_TEST_CASE(CFeeRateConstructorTest)
79 {
80     // Test CFeeRate(CAmount fee_rate, size_t bytes) constructor
81     // full constructor
82+    BOOST_CHECK(CFeeRate(CAmount(0), 0).IsZero());

fjahr commented at 0:15 am on January 26, 2021:

in b427ce178592283b9211b9fbd19874c7349bfd7e:

nit: I would have stuck these IsZero() tests in one place below since they are testing the function and not the ctor.

jonatack commented at 3:40 pm on January 26, 2021:

done

in src/test/amount_tests.cpp:98 in afba3e188f outdated

89@@ -86,6 +90,31 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
90     CFeeRate(MAX_MONEY, std::numeric_limits<size_t>::max() >> 1).GetFeePerK();
91 }
92 
93+BOOST_AUTO_TEST_CASE(CFeeRateNamedConstructorsTest)
94+{
95+    // Test CFeerate(CAmount fee_rate, FeeEstimatemode mode) constructor
96+    // with BTC/kvB, returns same values as CFeeRate(amount) or CFeeRate(amount, 1000)
97+    BOOST_CHECK(CFeeRate::FromBtcKb(CAmount(-1)) == CFeeRate(-1));
98+    BOOST_CHECK(CFeeRate::FromBtcKb(CAmount(-1)) == CFeeRate(-1, 1000));

fjahr commented at 0:20 am on January 26, 2021:

in afba3e188fcb14236324a0d1355445a99b85d155:

Aren’t we already testing somewhere that CFeeRate(-1) == CFeeRate(-1, 1000)? Then I think we could save one of those and some other similar lines following.

jonatack commented at 4:31 pm on January 26, 2021:

Dropped that line and three other ones from this test.

fjahr commented at 0:37 am on January 26, 2021: member

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.

policy: create CFeeRate::FromSatB/FromBtcKb named ctors 6581118b5e

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

and use it instead of constructing CFeeRates in conditionals

83fb6101f0

wallet: create FeeRateFromValueInSatB() util function

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)

8885b724ea

test: add CFeeRate zero values functional test coverage 17b75d61f5

test: add non-representable CFeeRate test coverage 4743c867ef

test: add ParseFixedPoint fee rate rejection coverage 8ba11be9d7

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

jonatack force-pushed on Jan 26, 2021

jonatack commented at 4:33 pm on January 26, 2021: member

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

fjahr commented at 10:36 pm on January 26, 2021: member

Code review ACK 236f556708fb619167b72cca451d0048a9274646

jarolrod commented at 11:02 pm on January 26, 2021: member

ACK 236f556708fb619167b72cca451d0048a9274646

This PR fixes the behavior shown in #20534

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

0./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009
1
2error code: -6
3error message:
4Fee rate (0.000 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)

PR: throws error code: -3

0./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009
1
2error code: -3
3error message:
4Invalid amount

MarcoFalke removed the label Refactoring on Jan 27, 2021

MarcoFalke renamed this:
~~policy, wallet, refactor: check for non-representable CFeeRates~~
policy, wallet: check for non-representable CFeeRates
on Jan 27, 2021

MarcoFalke renamed this:
~~policy, wallet: check for non-representable CFeeRates~~
wallet: check for non-representable CFeeRates
on Jan 27, 2021

MarcoFalke commented at 7:43 am on January 27, 2021: member

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

in src/test/amount_tests.cpp:123 in 83fb6101f0 outdated

118+    BOOST_CHECK(!CFeeRate(-1000).IsZero());
119+    BOOST_CHECK(CFeeRate(CAmount(0), 0).IsZero());
120+    BOOST_CHECK(CFeeRate(CAmount(1), 1001).IsZero());
121+    BOOST_CHECK(CFeeRate::FromBtcKb(CAmount(0)).IsZero());
122+    BOOST_CHECK(!CFeeRate::FromBtcKb(CAmount(1)).IsZero());
123+    BOOST_CHECK(CFeeRate::FromSatB(CAmount(0.000)).IsZero());

MarcoFalke commented at 8:00 am on January 27, 2021:

in commit 83fb6101f0768e8f540e187caa2679c9f0b98c20: CAmount can’t represent floats. This would be a compile error, if you didn’t use a c-style-narrowing-cast

MarcoFalke commented at 8:07 am on January 27, 2021:

For reference:

0test/amount_tests.cpp:123:44: error: type 'double' cannot be narrowed to 'CAmount' (aka 'long') in initializer list [-Wc++11-narrowing]
1    BOOST_CHECK(CFeeRate::FromSatB(CAmount{0.000}).IsZero());
2                                           ^~~~~

jonatack commented at 12:08 pm on January 27, 2021:

changed to 0

in src/rpc/util.cpp:95 in 8885b724ea outdated

86@@ -86,6 +87,17 @@ CAmount AmountFromValue(const UniValue& value)
87     return amount;
88 }
89 
90+CFeeRate FeeRateFromValueInSatB(const UniValue& value)
91+{
92+    const CAmount amount{AmountFromValue(value)};
93+    const CFeeRate fee_rate{CFeeRate::FromSatB(amount)};
94+    if (fee_rate.IsZero() && amount != 0) {
95+        // passed value is an unrepresentable fee rate between 0 and 0.001 sat/vB

MarcoFalke commented at 8:03 am on January 27, 2021:

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

9.99999999999999999999999999999 isn’t representable and also isn’t zero

MarcoFalke commented at 8:05 am on January 27, 2021:

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

jonatack commented at 11:30 am on January 27, 2021:

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.

jonatack commented at 12:09 pm on January 27, 2021:

dropped the comment

MarcoFalke commented at 12:13 pm on January 27, 2021:

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

MarcoFalke changes_requested

MarcoFalke commented at 8:05 am on January 27, 2021: member

Approach re-NACK

Concept ACK

NACK 236f556708fb619167b72cca451d0048a9274646 😰

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3NACK 236f556708fb619167b72cca451d0048a9274646 😰
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgD5Av/Xab0IOByGeikbyUJLvFeyqNKCpgDZOwPp8IH3gMOw0RsQSOTba1Mw/mi
 8TYwJbgZs861puVMSvVGcdp3knH4GsE2SnhX/h3h3ozL1lEu0s4K0uksel7PtrwWH
 9Om4oCDT3kNNAcrGE/HBvCk723gx3JwT5zOWbBv6X//ESGrkUM9+yMbg3od4HBhmG
10o24Ju8mugPXPKY9eQ+IG2RzXWItZPVVJIQBOVgD5awIBy15rgK50oOnLbT/8Mw8/
11xbgaOZJzmt+3VczxsOTX+Qp+FMvX1fEJgO4Blsrk5bx2COy+tMst3lU16Vw9vcjh
12huXxo7VdqiFPZB/q0BRg1Ysxl2o8o6ldKhrFm/W6Vtr5v3iIYxg7PTVjZAsJY98a
136DFpasP/KkWRwJSbqT63GETinOE+hKlu8sZO2X8R0NRGsyF+d9Lkyjq0FJsn4GDu
14fcPe1KUYGa0nKQB7TvbZSBHxZ/eM84Qg1tCtX88rVLgXtuQCnFt92aW0l5PScw1I
154tC98cjZ
16=4KcO
17-----END PGP SIGNATURE-----

Timestamp of file with hash d9e37613c2875ff346833a9cd2c564a076e87e0eb46dd8c7fa743c1e891d0796 -

jonatack commented at 11:27 am on January 27, 2021: member

This PR fixes the behavior shown in #20534

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

0./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009
1
2error code: -6
3error message:
4Fee rate (0.000 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)

PR: throws error code: -3

0./src/bitcoin-cli -regtest -named sendtoaddress address=<addr> amount=1.23 fee_rate=0.0009
1
2error code: -3
3error message:
4Invalid 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 commented at 11:55 am on January 27, 2021: member

NACK 236f556

Can you explain why?

MarcoFalke commented at 1:24 pm on January 27, 2021: member

NACK 236f556

Can you explain why?

because it doesn’t fix the bug. See:

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

Both calls should fail because the amount is not representable

jonatack commented at 1:46 pm on January 27, 2021: member

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

DrahtBot commented at 2:50 pm on April 5, 2021: member

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

DrahtBot added the label Needs rebase on Apr 5, 2021

jonatack commented at 3:39 pm on April 27, 2021: member

Can you explain why?

because it doesn’t fix the bug. See:

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

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 commented at 3:56 pm on April 27, 2021: member

Closing for now in favor of #21786.

jonatack closed this on Apr 27, 2021

DrahtBot locked this on Aug 18, 2022

wallet: check for non-representable CFeeRates #20546

Conflicts