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

pull jonatack wants to merge 7 commits into bitcoin:master from jonatack:ensure-sat-vb-feerates-are-in-range changing 9 files +128 −40
  1. jonatack commented at 3:55 PM on April 27, 2021: contributor
    • 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.

  2. jonatack cross-referenced this on Apr 27, 2021 from issue wallet: check for non-representable CFeeRates by jonatack
  3. jonatack cross-referenced this on Apr 27, 2021 from issue sat/b values aren't validated to be in-range by MarcoFalke
  4. 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.
  5. MarcoFalke commented at 4:42 PM on April 27, 2021: member

    Concept ACK

  6. DrahtBot added the label RPC/REST/ZMQ on Apr 27, 2021
  7. DrahtBot added the label Wallet on Apr 27, 2021
  8. DrahtBot cross-referenced this on Apr 27, 2021 from issue test: fix off-by-ones in rpc_fundrawtransaction assertions by jonatack
  9. DrahtBot commented at 5:45 PM on April 27, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  10. DrahtBot cross-referenced this on Apr 27, 2021 from issue tests: Run both descriptor and legacy tests within a single test invocation by achow101
  11. jonatack force-pushed on Apr 28, 2021
  12. DrahtBot cross-referenced this on Apr 30, 2021 from issue Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101
  13. DrahtBot added the label Needs rebase on May 5, 2021
  14. jonatack force-pushed on May 5, 2021
  15. jonatack commented at 1:46 PM on May 5, 2021: contributor

    rebased

  16. DrahtBot removed the label Needs rebase on May 5, 2021
  17. test: improve zero-value explicit fee rate coverage ea6f76b66e
  18. jonatack force-pushed on May 6, 2021
  19. jonatack commented at 7:08 AM on May 6, 2021: contributor

    Rebased for CI fix on master.

  20. meshcollider commented at 7:05 PM on May 7, 2021: contributor

    Concept ACK

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

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

  23. fjahr commented at 2:24 PM on May 8, 2021: contributor

    Code review ACK 9c7b474cfd71cc5311db4e98c0efd1736a1096fb

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

  24. test: explicit fee rates with invalid amounts c5fd4344f7
  25. test: type error and out of range fee rates where missing b503327597
  26. test: ParseFixedPoint with 3 decimals for sat/vB fee rates 8ce3ef57a3
  27. rpc: enable passing decimals to AmountFromValue, add doxygen 0742c7840f
  28. rpc: for sat/vB fee rates, limit ParseFixedPoint decimals to 3 06a90fa038
  29. test: fee rate values that cannot be represented as sat/vB 847288df07
  30. jonatack force-pushed on May 9, 2021
  31. jonatack commented at 10:53 AM on May 9, 2021: contributor

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

  32. fjahr commented at 8:52 PM on May 9, 2021: contributor

    Code review ACK 9c7b474cfd71cc5311db4e98c0efd1736a1096fb

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

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

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

  35. MarcoFalke approved
  36. MarcoFalke commented at 7:02 AM on May 10, 2021: member

    review ACK 847288df07b45ca535c849e518b22818ab492896 🔷

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

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 847288df07b45ca535c849e518b22818ab492896 🔷
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjgOAv/U64jIK5O7lauqHgzSxVR8ARVhocyv95mL70s0DRDSUXyWeNjsQAbqq4d
d/Frd1URcN+ui/wiGAvzW7JjgPxdFBfUpcUZCh6uVwzMYIi5JfzDpVVC1Fa/S9PG
Wcvi14s/N7wWMEYRE85WMncceEyCAb5NP6/Pnr8zY3/jdMT+NGZv+OvIkNgpxpFt
CP9iMkX5GFuHOJnFc53PNAvvuGnoDE/jGWDk7X48Q8EScNgwKnk16K7vNDbarKb2
szmXO2H+ql7moWoTBK0xDr/X2GEdZw+49nr1aP11pxBAIbgk0T/PweW6HQlnKaQ4
Ys/mVrSS18laMZkK2LvOgHQynSHmwwUyekcCzaxNWkRP2Si17bIwWlCCswPL4L1Z
qgWDQomJ2eJoYUBJMFwxTe0ZqVlRNEQA3ah5dv9qmImo9Qezf28E5fHeZ/OmHiEo
dBECvDU5QZlwFsUZ94kRY7t0/8E+5yGDESsmYV3120BkOYLcGx+OARlT0YZcyRaz
7SjbaQL7
=gqY6
-----END PGP SIGNATURE-----

    Timestamp of file with hash 8b19ebaa4f993d9f7f5a3cd958924c1f48d2c00e94f5d00a4b4b7012058dc816 -

    </details>

  37. MarcoFalke merged this on May 10, 2021
  38. MarcoFalke closed this on May 10, 2021
  39. jonatack deleted the branch on May 10, 2021
  40. sidhujag referenced this in commit 282259912e on May 10, 2021
  41. jonatack cross-referenced this on Jul 13, 2021 from issue wallet: introduce setfeerate (an improved settxfee, in sat/vB) by jonatack
  42. jonatack cross-referenced this on Sep 29, 2021 from issue Cleanup CFeeRate constructor (sat/vB vs BTC/kvB) by MarcoFalke
  43. gwillen referenced this in commit 9c4837db05 on Jun 1, 2022
  44. adamjonas cross-referenced this on Aug 2, 2022 from issue Improve documentation of estimatesmartfee by stevenroose
  45. bitcoin locked this on Aug 18, 2022
Contributors
jonatackMarcoFalkeDrahtBotmeshcolliderfjahr
Labels
WalletRPC/REST/ZMQ
Linked (view graph)
#18217 Improve documentation of estimatesmartfee#20391 wallet: introduce setfeerate (an improved settxfee, in sat/vB)#20534 sat/b values aren't validated to be in-range#20546 wallet: check for non-representable CFeeRates#21632 MemorySanitizer: use-of-uninitialized-value in sqlite3BtreeOpen#21902 refactor: Remove useless extern keyword#23129 Cleanup CFeeRate constructor (sat/vB vs BTC/kvB)
github-metadata-mirror

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: 2026-05-19 06:53 UTC