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

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:

        // 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 🔷

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 8b19ebaa4f993d9f7f5a3cd958924c1f48d2c00e94f5d00a4b4b7012058dc816 -

</details>

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