wallet: ensure sat/vB feerates are in range (mantissa of 3)

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

Improve/close gaps in existing test coverage before making the change
Enable passing decimals to ParseFixedPoint() when calling AmountFromValue()
Limit explicit fee rates in sat/vB passed in by users to 3 decimals, and raise otherwise
Add regression test coverage

Closes #20534.

in src/wallet/rpcwallet.cpp:219 in 8acc7f9c62 outdated

215@@ -216,7 +216,8 @@ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const Un
216         if (!estimate_mode.isNull() && estimate_mode.get_str() != "unset") {
217             throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate");
218         }
219-        cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN);
220+        // Pass a mantissa of 3 as sat/vB values < 0.001 cannot be represented.

jonatack commented at 4:02 pm on April 27, 2021:

0        // Pass a mantissa of 3 as sat/vB fee rates cannot represent values having more than 3 significant digits.

MarcoFalke commented at 4:42 pm on April 27, 2021: member

Concept ACK

DrahtBot added the label RPC/REST/ZMQ on Apr 27, 2021

DrahtBot added the label Wallet on Apr 27, 2021

DrahtBot commented at 5:45 pm on April 27, 2021: 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:

#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
#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.

jonatack force-pushed on Apr 28, 2021

DrahtBot added the label Needs rebase on May 5, 2021

jonatack force-pushed on May 5, 2021

jonatack commented at 1:46 pm on May 5, 2021: member

rebased

DrahtBot removed the label Needs rebase on May 5, 2021

test: improve zero-value explicit fee rate coverage ea6f76b66e

jonatack force-pushed on May 6, 2021

jonatack commented at 7:08 am on May 6, 2021: member

Rebased for CI fix on master.

meshcollider commented at 7:05 pm on May 7, 2021: contributor

Concept ACK

in test/functional/rpc_fundrawtransaction.py:771 in 91b2524c15 outdated

769@@ -770,6 +770,9 @@ def test_option_feerate(self):
770                 node.fundrawtransaction, rawtx, {param: {"foo": "bar"}, "add_inputs": True})
771             assert_raises_rpc_error(-3, "Invalid amount",

fjahr commented at 1:40 pm on May 8, 2021:

in 91b2524c15810d6867d46f1aee6f172612a21fc7:

I think this one can be removed now since it is included below?

jonatack commented at 10:51 am on May 9, 2021:

Thanks! Done.

in test/functional/rpc_psbt.py:215 in 91b2524c15 outdated

213@@ -214,6 +214,10 @@ def run_test(self):
214                 self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: {"foo": "bar"}, "add_inputs": True})
215             assert_raises_rpc_error(-3, "Invalid amount",

fjahr commented at 1:41 pm on May 8, 2021:

in 91b2524:

Can also be removed I think.

jonatack commented at 10:51 am on May 9, 2021:

Indeed, good eye. Done.

fjahr commented at 2:24 pm on May 8, 2021: member

Code review ACK 9c7b474cfd71cc5311db4e98c0efd1736a1096fb

modulo I think those two tests mentioned below can be removed now.

test: explicit fee rates with invalid amounts c5fd4344f7

test: type error and out of range fee rates where missing b503327597

test: ParseFixedPoint with 3 decimals for sat/vB fee rates 8ce3ef57a3

rpc: enable passing decimals to AmountFromValue, add doxygen 0742c7840f

rpc: for sat/vB fee rates, limit ParseFixedPoint decimals to 3 06a90fa038

test: fee rate values that cannot be represented as sat/vB 847288df07

jonatack force-pushed on May 9, 2021

jonatack commented at 10:53 am on May 9, 2021: member

Thank you @fjahr, updated with your feedback: git diff 9c7b474 847288d

fjahr commented at 8:52 pm on May 9, 2021: member

Code review ACK 9c7b474cfd71cc5311db4e98c0efd1736a1096fb

The only change was the removal of two now redundant/duplicate tests.

in test/functional/rpc_fundrawtransaction.py:736 in ea6f76b66e outdated

736         assert_fee_amount(result4['fee'], count_bytes(result4['hex']), 10 * result_fee_rate)
737-        assert_fee_amount(result5['fee'], count_bytes(result5['hex']), 0)
738-        assert_fee_amount(result6['fee'], count_bytes(result6['hex']), 0)
739+
740+        # Test that funding non-standard "zero-fee" transactions is valid.
741+        for param, zero_value in product(["fee_rate", "feeRate"], [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]):

MarcoFalke commented at 6:51 am on May 10, 2021:

There should be no difference in python between 0., and .0, or any other combination that appends zeros to any side of either of them.

in src/rpc/util.h:87 in 0742c7840f outdated

83+ *
84+ * @param[in] value     UniValue number or string to parse.
85+ * @param[in] decimals  Number of significant digits (default: 8).
86+ * @returns a CAmount if the various checks pass.
87+ */
88+extern CAmount AmountFromValue(const UniValue& value, int decimals = 8);

MarcoFalke commented at 6:59 am on May 10, 2021:

In C++ all functions are extern, so this can be removed without any downside

MarcoFalke commented at 7:13 am on May 10, 2021:

Fixed unrelated cleanup in #21902

MarcoFalke approved

MarcoFalke commented at 7:02 am on May 10, 2021: member

review ACK 847288df07b45ca535c849e518b22818ab492896 🔷

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 847288df07b45ca535c849e518b22818ab492896 🔷
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjgOAv/U64jIK5O7lauqHgzSxVR8ARVhocyv95mL70s0DRDSUXyWeNjsQAbqq4d
 8d/Frd1URcN+ui/wiGAvzW7JjgPxdFBfUpcUZCh6uVwzMYIi5JfzDpVVC1Fa/S9PG
 9Wcvi14s/N7wWMEYRE85WMncceEyCAb5NP6/Pnr8zY3/jdMT+NGZv+OvIkNgpxpFt
10CP9iMkX5GFuHOJnFc53PNAvvuGnoDE/jGWDk7X48Q8EScNgwKnk16K7vNDbarKb2
11szmXO2H+ql7moWoTBK0xDr/X2GEdZw+49nr1aP11pxBAIbgk0T/PweW6HQlnKaQ4
12Ys/mVrSS18laMZkK2LvOgHQynSHmwwUyekcCzaxNWkRP2Si17bIwWlCCswPL4L1Z
13qgWDQomJ2eJoYUBJMFwxTe0ZqVlRNEQA3ah5dv9qmImo9Qezf28E5fHeZ/OmHiEo
14dBECvDU5QZlwFsUZ94kRY7t0/8E+5yGDESsmYV3120BkOYLcGx+OARlT0YZcyRaz
157SjbaQL7
16=gqY6
17-----END PGP SIGNATURE-----

Timestamp of file with hash 8b19ebaa4f993d9f7f5a3cd958924c1f48d2c00e94f5d00a4b4b7012058dc816 -

MarcoFalke merged this on May 10, 2021

MarcoFalke closed this on May 10, 2021

jonatack deleted the branch on May 10, 2021

sidhujag referenced this in commit 282259912e on May 10, 2021

gwillen referenced this in commit 9c4837db05 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

wallet: ensure sat/vB feerates are in range (mantissa of 3) #21786

Conflicts