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

jonatack commented at 10:23 PM on November 14, 2020: member

Continuing the work on issue #19543, this proposes a setfeerate RPC in sat/vB and makes settxfee a hidden RPC per the plan described in #20484 (comment).

jonatack commented at 10:24 PM on November 14, 2020: member

Thanks to @Xekyo for suggesting this in #20305 (review).

DrahtBot added the label RPC/REST/ZMQ on Nov 14, 2020

DrahtBot added the label Wallet on Nov 14, 2020

DrahtBot commented at 12:07 AM on November 15, 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:

#20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)
#20546 (wallet: check for non-representable CFeeRates by jonatack)
#20406 (util: Avoid invalid integer negation in FormatMoney and ValueFromAmount by practicalswift)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
#16795 (rpc: have raw transaction decoding infer output descriptors by instagibbs)

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.

ghost commented at 3:28 AM on November 15, 2020: none

In the 'Successful responses' and 'Error responses' above shouldn't it be sat/vB instead of sat/B?

jonatack commented at 7:21 AM on November 15, 2020: member

@prayank23 thanks for looking. Those will be automatically updated from sat/B to sat/vB by #20305, which updates the ToString() helper in CFeeRate. I updated the PR descpription to clarify that (thanks!)

jonatack force-pushed on Nov 15, 2020

DrahtBot added the label Needs rebase on Nov 17, 2020

in src/wallet/rpcwallet.cpp:2410 in a96417d85a outdated

2405 | +                result = "Fee rate for transactions with this wallet successfully set to " + amount_str;
2406 | +            }
2407 | +
2408 | +            UniValue obj(UniValue::VOBJ);
2409 | +            obj.pushKV("wallet_name", wallet.GetName());
2410 | +            obj.pushKV("fee_rate", UniValue(UniValue::VNUM, wallet.m_pay_tx_fee.SatsToString()));

luke-jr commented at 12:48 AM on November 25, 2020:

IMO we should add a ValueFromFeeRate

luke-jr commented at 1:13 AM on November 25, 2020:

Maybe rebase this on top of 8798383475f4e1db44f734cdd94885e5527761fe

jonatack commented at 5:27 PM on November 27, 2020:

Maybe rebase this on top of 8798383

Pulled 8798383 in as commit 0e2c3ace86b8aa

jonatack force-pushed on Nov 27, 2020

DrahtBot removed the label Needs rebase on Nov 27, 2020

jonatack commented at 5:36 PM on November 27, 2020: member

Rebased and updated per the plan outlined in #20484 (comment).

in test/functional/rpc_getblockstats.py:110 in e915a2a2c8 outdated

 106 | @@ -107,7 +107,7 @@ def run_test(self):
 107 |          assert_equal(stats[self.max_stat_pos]['height'], self.start_height + self.max_stat_pos)
 108 |  
 109 |          for i in range(self.max_stat_pos+1):
 110 | -            self.log.info('Checking block %d\n' % (i))
 111 | +            self.log.info('Checking block %d' % (i))

MarcoFalke commented at 9:25 AM on November 28, 2020:

style-nit: might use f-string?

jonatack commented at 7:01 PM on November 28, 2020:

good idea; done

in src/wallet/rpcwallet.cpp:2389 in e915a2a2c8 outdated

2384 | +            if (!rpc_wallet) return NullUniValue;
2385 | +            CWallet& wallet = *rpc_wallet;
2386 | +
2387 | +            LOCK(wallet.cs_wallet);
2388 | +
2389 | +            CFeeRate amount{AmountFromValue(request.params[0]), COIN};

MarcoFalke commented at 9:28 AM on November 28, 2020:

The second argument of the cfeerate constructor is size_t (number of bytes), not CAmount (number of satoshis). Instead of adding more calls to the confusing constructor, I'd prefer to add a new constructor that takes in a univalue feerate as sat/b. I think this has also been provided as feedback when this code template was initially introduced.

jonatack commented at 7:03 PM on November 28, 2020:

Good idea, done in 9c479bfc293fec906 and proposed a refactoring in 392495bc4d9648fe0c28d that can be dropped

jonatack commented at 7:52 PM on November 28, 2020:

Hm, bringing the RPC utilities and UniValue into CFeeRate seems like a layer violation (and caused a circular dependency). Maybe a CFeeRate ctor that takes a CAmount + FeeEstimateMode instead of UniValue.

jonatack commented at 8:40 PM on November 29, 2020:

Updated the new CFeeRate ctor to take (CAmount fee_rate, FeeEstimateMode mode) and added unit tests.

MarcoFalke approved

MarcoFalke commented at 9:29 AM on November 28, 2020: member

Concept ACK on adding the feature for users to set -paytxfee via rpc at runtime in sat/b

jonatack force-pushed on Nov 28, 2020

jonatack force-pushed on Nov 29, 2020

jonatack force-pushed on Nov 30, 2020

jonatack commented at 10:22 AM on November 30, 2020: member

Added more unit test coverage per git diff 14c5e50 2176a3.

in src/policy/feerate.cpp:46 in d6643ef609 outdated

  41 | @@ -42,3 +42,7 @@ std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const
  42 |      default:                      return strprintf("%d.%08d %s/kvB", nSatoshisPerK / COIN, nSatoshisPerK % COIN, CURRENCY_UNIT);
  43 |      }
  44 |  }
  45 | +
  46 | +std::string CFeeRate::SatsToString() const {

MarcoFalke commented at 11:06 AM on November 30, 2020:

style nit d6643ef609bf442fe38dd8a1335dd7c580f1fdd7:

could clang-format?

jonatack commented at 11:35 PM on November 30, 2020:

no longer adding this SatsToString() function in favor of making the existing ToString() more flexible

in src/test/amount_tests.cpp:115 in 9d41bb8c7b outdated

 110 | +    BOOST_CHECK(CFeeRate(CAmount(123456789), FeeEstimateMode::SAT_VB) == CFeeRate(1234));
 111 | +    // ...with invalid unit values returns CFeeRate(0)
 112 | +    BOOST_CHECK(CFeeRate(CAmount{123}, FeeEstimateMode::UNSET) == CFeeRate{0});
 113 | +    BOOST_CHECK(CFeeRate(CAmount{123}, std::numeric_limits<size_t>::max()) == CFeeRate{0});
 114 | +    BOOST_CHECK(CFeeRate(CAmount{123}, 0) == CFeeRate{0});
 115 | +    BOOST_CHECK(CFeeRate(CAmount{123}, -1) == CFeeRate{0});

MarcoFalke commented at 11:15 AM on November 30, 2020:

style nit in 4cccf2d88377a9f813ab6d63889cbb5136c31aad

Might be good to make this impossible to compile instead of a runtime-just-return-0. Can be achieved via constexpr if (and some template magic) or by having a CFeeRate::FromSatB(amount) and FromBtcKb(amount) helper.

jonatack commented at 3:54 PM on November 30, 2020:

Oh oops hm, these lines 113-115 aren't testing the new constructor, but the CFeeRate(CAmount, size_t) one, so it was mistake to add them here. Will move them into the tests for that constructor and see if anything needs to be done there; normally the ctor should just set nSatoshisPerK to 0 in that case.

MarcoFalke commented at 3:58 PM on November 30, 2020:

(my comment should be on the BOOST_CHECK(CFeeRate(CAmount{123}, FeeEstimateMode::UNSET) == CFeeRate{0}) line)

jonatack commented at 10:37 PM on November 30, 2020:

Good idea, much better...switching to named constructors.

jonatack commented at 11:35 PM on November 30, 2020:

done, and a leaner implementation, thanks

in src/core_io.h:44 in 3c92185b95 outdated

  40 | @@ -40,6 +41,7 @@ int ParseSighashString(const UniValue& sighash);
  41 |  
  42 |  // core_write.cpp
  43 |  UniValue ValueFromAmount(const CAmount& amount);
  44 | +UniValue ValueFromFeeRate(const CFeeRate& fee_rate);

MarcoFalke commented at 11:45 AM on November 30, 2020:

in commit 3c92185b95764f5c3536637e5923316cd9c2d8b0:

Might be good if the method name included the unit (sat/b) to avoid calling this in a btc/kb context

jonatack commented at 11:35 PM on November 30, 2020:

Updated to allow passing in the fee mode to work with both sat/vB and BTC/kvB

MarcoFalke commented at 11:48 AM on November 30, 2020: member

left some comments

in src/policy/feerate.cpp:7 in 7289e559eb outdated

   6 | @@ -7,35 +7,31 @@
   7 |

MarcoFalke commented at 11:50 AM on November 30, 2020:

why are the refactors in 7289e559eb720d972e3e31c0ca81dd8091775b07 needed? They break asan, it seems.

jonatack commented at 3:28 PM on November 30, 2020:

The CI was green with the refactors yesterday; the change was the added unit tests. It may be better to propose the refactors in their own PR. I'll drop the refactoring commit to test what the CI says.

jonatack force-pushed on Nov 30, 2020

jonatack commented at 11:43 PM on November 30, 2020: member

Thanks for the review @MarcoFalke. Dropped the policy/feerate refactoring commit and the FeeModeToBytes commit, changed the new CFeeRate delegating constructor to named constructors, dropped SatsToString in favor of making the existing ToString more flexible, and made ValueFromFeeRate able to work with both sat/b and btc/kb.

in src/wallet/rpcwallet.cpp:2360 in c6e9bfd1bd outdated

2354 | +{
2355 | +    return RPCHelpMan{
2356 | +        "setfeerate",
2357 | +        "\nSet the transaction fee rate in " + CURRENCY_ATOM + "/vB for this wallet.\n"
2358 | +        "Overrides the global -paytxfee configuration option.\n"
2359 | +        "Can be deactivated by passing 0 as the fee rate, in which case automatic fee selection will be used by default.\n",

promag commented at 12:17 PM on December 3, 2020:

c6e9bfd1bd9d6a65673fbe1c278a7c7ada35273d

Worth noting that the new settings is not persisted?

jonatack commented at 7:01 PM on December 4, 2020:

Thanks, added to settxfee and setfeerate helps

in src/wallet/rpcwallet.cpp:2417 in c6e9bfd1bd outdated

2409 | +                result = "Fee rate for transactions with this wallet successfully set to " + amount_str;
2410 | +            }
2411 | +            CHECK_NONFATAL(result.empty() != error.empty());
2412 | +
2413 | +            UniValue obj{UniValue::VOBJ};
2414 | +            obj.pushKV("wallet_name", wallet.GetName());

promag commented at 12:19 PM on December 3, 2020:

c6e9bfd1bd9d6a65673fbe1c278a7c7ada35273d

Why return wallet name? What about returning previous effective value?

MarcoFalke commented at 12:45 PM on December 3, 2020:

why would the user care about the previous value? If they did, they could call getwalletinfo

promag commented at 12:52 PM on December 3, 2020:

Right, the same goes for the wallet name.

jonatack commented at 7:23 PM on December 4, 2020:

I'd prefer to keep wallet name.

jonasschnelli commented at 8:40 AM on December 7, 2020:

No strong opinion about the wallet name in the response. I think it does not hurt and in asynchronous environments it might simplify the client code.

MarcoFalke commented at 8:55 AM on December 7, 2020:

I think we should establish a guideline first on what input parameters to return. I don't think we've done this in the past and this could lead to user confusion: "What am I supposed to do with the return parameter that is equal to the input parameter? Assert that they are the same? Ignore?"

If this is something useful to return for every wallet RPC, then all wallet RPCs should be updated.

jonatack commented at 12:25 PM on December 7, 2020:

I see it as good for rpcs that make per-wallet changes, like here and upgradewallet as a recent example (see upgradewallet help). It would also be good in rpc/cli options that return info on a per-wallet basis, like getwalletinfo but also -rpcwallet= -getinfo, for example.

promag commented at 12:24 PM on December 3, 2020: member

Code review ACK 73a0f0276392e4432774807f2e4b8a3f4dfc69ab, needs to squash some commits. Also add release notes?

jonatack force-pushed on Dec 4, 2020

jonatack commented at 7:39 PM on December 4, 2020: member

Thanks @promag and @MarcoFalke for the review feedback. Updates in latest push:

Check for and raise "Invalid amount" if the passed value is non-representable by CFeeRate (lesson learned from #20546)
Fix the error messages if the current value is unset, to indicate a current value of "0 (unset)" instead of "0.000 sat/vB"
Make sure the tests check passing amounts with a string as well as a number, and update the coverage for the first two items
Update the settxfee and setfeerate helps with @promag's suggestion

jonatack commented at 7:40 PM on December 4, 2020: member

I'll re-push with a release note in f4195d2a94b

in src/wallet/rpcwallet.cpp:2401 in 43b0d996b2 outdated

2397 | +            const std::string amount_str{amount.ToString(FeeEstimateMode::SAT_VB)};
2398 | +            const std::string current_setting{strprintf("The current setting of %s for this wallet remains unchanged.", wallet.m_pay_tx_fee == zero ? "0 (unset)" : wallet.m_pay_tx_fee.ToString(FeeEstimateMode::SAT_VB))};
2399 | +            std::string result, error;
2400 | +
2401 | +            if (amount == zero) {
2402 | +                if (request.params[0].get_real() != 0) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");

MarcoFalke commented at 5:42 AM on December 5, 2020:

This should be thrown in the amountfromvalue function. Doing it here with the zero-not-zero hack smells a bit like a layer violation

jonatack commented at 12:12 PM on December 7, 2020:

After #20546 this will go away:

-            const CFeeRate amount{CFeeRate::FromSatB(AmountFromValue(request.params[0]))};
+            const CFeeRate amount{FeeRateFromValueInSatB(request.params[0])};
@@ -2399,7 +2399,6 @@ static RPCHelpMan setfeerate()
 
             if (amount == zero) {
-                if (request.params[0].get_real() != 0) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
                 wallet.m_pay_tx_fee = amount;

jonatack commented at 6:09 PM on January 3, 2021:

This PR has been gated since a month on the changes in #20546 to handle this properly, but that PR didn't go anywhere, so I'll drop this check for now.

jonatack commented at 11:24 PM on January 3, 2021:

Hm, realized I need to leave this check or else will have to drop a bunch of tests, which would be a shame and a regression. A lot of work went into this and it was waiting for #20546. I guess leaving it as-is for now.

in src/wallet/rpcwallet.cpp:2314 in 43b0d996b2 outdated

2310 | @@ -2311,10 +2311,12 @@ static RPCHelpMan listlockunspent()
2311 |  static RPCHelpMan settxfee()
2312 |  {
2313 |      return RPCHelpMan{"settxfee",
2314 | -                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n"
2315 | -                "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n",
2316 | +                "\n(Superseded by the setfeerate RPC in sat/vB, which it is recommended to use instead.)\n"

jonasschnelli commented at 8:38 AM on December 7, 2020:

Wouldn't deprecated instead of superseded be more consistent with previous RPC API "changes"?

jonatack commented at 12:14 PM on December 7, 2020:

Thanks, will do on next update.

jonatack commented at 10:29 AM on January 4, 2021:

done

jonasschnelli approved

jonasschnelli commented at 8:40 AM on December 7, 2020: contributor

Concept ACK (partially code reviewed).

DrahtBot added the label Needs rebase on Dec 10, 2020

jonatack force-pushed on Jan 4, 2021

jonatack commented at 10:37 AM on January 4, 2021: member

Code review ACK 73a0f02, needs to squash some commits. Also add release notes?

Rebased, updated settxfee help with @jonasschnelli's suggestion, added release notes per @promag's suggestion. Sorry for the delay in updating, this was waiting since a month on the improvements and fixes in #20546 but I've had to move forward without them. I think this is done. Please re-ACK if you can.

DrahtBot removed the label Needs rebase on Jan 4, 2021

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

DrahtBot added the label Needs rebase on Jan 28, 2021

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

policy: add CFeeRate::FromSatB/FromBtcKb constructors c4b288d5d9

wallet: update rpcwallet to CFeeRate named constructors c72083c2d5

policy: allow CFeeRate::ToString to not append fee rate units 5ed3ca1faf

core_io: Add ValueFromFeeRate helper aee3571392

test: add ValueFromFeeRate/CFeeRate unit tests 0479268c08

wallet: introduce setfeerate, an improved settxfee in sat/vB 8e863e3d3c

test: add setfeerate functional coverage in wallet_create_tx.py 529bfc16ff

test: add setfeerate functional coverage in wallet_bumpfee.py c907f158a6

test: update functional tests from settxfee to setfeerate

updates tests that call settxfee to call setfeerate instead;
settxfee functionality still has test coverage in
- wallet_bumpfee.py
- wallet_create_tx.py
- wallet_multiwallet.py

also fixes two fundrawtransaction tests that had off-by-one naming errors
and tightens up one of the two

d87f0f3a92

wallet: update settxfee help c594a00284

wallet, test: make settxfee a hidden rpc

Make settxfee a hidden rpc to avoid confusion for new users and guide them
to use the setfeerate rpc in sat/vB, while allowing existing scripts based
on settxfee in BTC/kvB to continue using it without breaking.

A hidden RPC means that `bitcoin-cli help settxfee` continues to display
the help documentation while removing it from the results of the general
`bitcoin-cli help`.

Updates the release note.

3725a3f180

wallet, test: upgradewallet follow-ups

per PR 20403 review feedback by MarcoFalke

1002e2d0d7

jonatack force-pushed on Jan 28, 2021

DrahtBot removed the label Needs rebase on Jan 28, 2021

S3RK commented at 8:16 PM on February 11, 2021: member

I start reviewing this one.

in src/policy/feerate.h:43 in 1002e2d0d7

  38 | @@ -39,6 +39,8 @@ class CFeeRate
  39 |          // We've previously had bugs creep in from silent double->int conversion...
  40 |          static_assert(std::is_integral<I>::value, "CFeeRate should be used without floats");
  41 |      }
  42 | +    static CFeeRate FromSatB(CAmount fee_rate);
  43 | +    static CFeeRate FromBtcKb(CAmount fee_rate);

S3RK commented at 8:51 PM on February 11, 2021:

On line 47 I believe it should be (nFeePaid * 1e3 / 1e8) == (nFeePaid / 1e5) and not (nFeePaid * 1e8 / 1e3) == (nFeePaid / 1e5)

in src/policy/feerate.cpp:44 in 1002e2d0d7

  43 | -    default:                      return strprintf("%d.%08d %s/kvB", nSatoshisPerK / COIN, nSatoshisPerK % COIN, CURRENCY_UNIT);
  44 | +    if (with_units) {
  45 | +        switch (mode) {
  46 | +        case FeeEstimateMode::SAT_VB:
  47 | +            return strprintf("%d.%03d %s/vB", nSatoshisPerK / 1000, nSatoshisPerK % 1000, CURRENCY_ATOM);
  48 | +        default:

S3RK commented at 8:58 PM on February 11, 2021:

Do we want to assert some other FeeEstimateMode isn't passed by mistake?

in src/test/amount_tests.cpp:145 in 1002e2d0d7

 145 | +    BOOST_CHECK_EQUAL(CFeeRate(1).ToString(FeeEstimateMode::SAT_VB, /* with_units */ false), "0.001");
 146 | +    BOOST_CHECK_EQUAL(CFeeRate(70).ToString(FeeEstimateMode::SAT_VB, /* with_units */ false), "0.070");
 147 | +    BOOST_CHECK_EQUAL(CFeeRate(3141).ToString(FeeEstimateMode::SAT_VB, /* with_units */ false), "3.141");
 148 | +    BOOST_CHECK_EQUAL(CFeeRate(10002).ToString(FeeEstimateMode::SAT_VB, /* with_units */ false), "10.002");
 149 | +    BOOST_CHECK_EQUAL(CFeeRate(3141).ToString(FeeEstimateMode::BTC_KVB, /* with_units */ false), "0.00003141");
 150 | +    BOOST_CHECK_EQUAL(CFeeRate(10002).ToString(FeeEstimateMode::BTC_KVB, /* with_units */ false), "0.00010002");

S3RK commented at 9:11 PM on February 11, 2021:

what happens if we pass an absurdly big number?

in src/test/rpc_tests.cpp:219 in 1002e2d0d7

 215 | @@ -216,6 +216,29 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values)
 216 |      BOOST_CHECK_THROW(AmountFromValue(ValueFromString("93e+9")), UniValue); //overflow error
 217 |  }
 218 |  
 219 | +BOOST_AUTO_TEST_CASE(rpc_parse_fee_rate_values)

S3RK commented at 8:23 AM on February 12, 2021:

Aren't CFeeRate various constructors and ToString() already covered by other unit tests? Unit tests have a non-zero maintenance cost and I'm missing the point of such duplication. The ValueFromFeeRate fn is pretty trivial and I believe the test should be trivial as well.

in src/policy/feerate.cpp:42 in 1002e2d0d7

  41 | -    switch (fee_estimate_mode) {
  42 | -    case FeeEstimateMode::SAT_VB: return strprintf("%d.%03d %s/vB", nSatoshisPerK / 1000, nSatoshisPerK % 1000, CURRENCY_ATOM);
  43 | -    default:                      return strprintf("%d.%08d %s/kvB", nSatoshisPerK / COIN, nSatoshisPerK % COIN, CURRENCY_UNIT);
  44 | +    if (with_units) {
  45 | +        switch (mode) {
  46 | +        case FeeEstimateMode::SAT_VB:

S3RK commented at 8:31 AM on February 12, 2021:

I'm confused why do we put fee estimate mode and fee units, which are different things, into the same enum. I understand this is a bit out of scope for this PR, but I'd appreciate if you could provide some historical context for educational purposes.

jonatack commented at 3:59 PM on February 12, 2021:

IIRC that was done in #11413, but good point. I'll look at fixing that.

jonatack commented at 4:02 PM on February 12, 2021:

Ah, just remembered that I wrote a patch in November 2020 that does that but I have not opened it yet to not have too many PRs open.

in src/wallet/rpcwallet.cpp:2392 in 1002e2d0d7

2387 | +            CWallet& wallet = *rpc_wallet;
2388 | +
2389 | +            LOCK(wallet.cs_wallet);
2390 | +
2391 | +            const CFeeRate amount{CFeeRate::FromSatB(AmountFromValue(request.params[0]))};
2392 | +            const CFeeRate relay_min_feerate{wallet.chain().relayMinFee().GetFeePerK()};

S3RK commented at 8:51 AM on February 12, 2021:

Do we need GetFeePerK() call here and below?

in src/test/amount_tests.cpp:110 in 1002e2d0d7

 105 | +    BOOST_CHECK(CFeeRate::FromSatB(CAmount(0)) == CFeeRate(0));
 106 | +    BOOST_CHECK(CFeeRate::FromSatB(CAmount(99999)) == CFeeRate(0));
 107 | +    BOOST_CHECK(CFeeRate::FromSatB(CAmount(100000)) == CFeeRate(1));
 108 | +    BOOST_CHECK(CFeeRate::FromSatB(CAmount(100001)) == CFeeRate(1));
 109 | +    BOOST_CHECK(CFeeRate::FromSatB(CAmount(2690000)) == CFeeRate(26));
 110 | +    BOOST_CHECK(CFeeRate::FromSatB(CAmount(123456789)) == CFeeRate(1234));

S3RK commented at 9:18 AM on February 12, 2021:

what happens if we pass an absurdly big number?

S3RK commented at 9:22 AM on February 12, 2021: member

I did a high level pass, didn't look at functional tests at all yet. Overall seems good, but I need to give it more thorough look. Please check my comments.

jonatack commented at 4:04 PM on February 12, 2021: member

Thank you @S3RK for your feedback, much appreciated. I'll reply soon.

in test/functional/rpc_fundrawtransaction.py:721 in 1002e2d0d7

 718 | @@ -717,7 +719,7 @@ def test_option_feerate(self):
 719 |  
 720 |          result = node.fundrawtransaction(rawtx)  # uses self.min_relay_tx_fee (set by settxfee)
 721 |          btc_kvb_to_sat_vb = 100000  # (1e5)

S3RK commented at 8:48 AM on February 17, 2021:

This is already defined earlier

in test/functional/wallet_basic.py:200 in 1002e2d0d7

 197 | @@ -197,7 +198,7 @@ def run_test(self):
 198 |          # Send 10 BTC normal
 199 |          address = self.nodes[0].getnewaddress("test")
 200 |          fee_per_byte = Decimal('0.001') / 1000
 201 | -        self.nodes[2].settxfee(fee_per_byte * 1000)

S3RK commented at 9:00 AM on February 17, 2021:

        fee_sat_per_byte = 100
        self.nodes[2].setfeerate(fee_sat_per_byte)

in test/functional/rpc_fundrawtransaction.py:720 in 1002e2d0d7

 718 | @@ -717,7 +719,7 @@ def test_option_feerate(self):
 719 |  
 720 |          result = node.fundrawtransaction(rawtx)  # uses self.min_relay_tx_fee (set by settxfee)

S3RK commented at 9:04 AM on February 17, 2021:

        result = node.fundrawtransaction(rawtx)  # uses self.min_relay_tx_fee (set by setfeerate)

in test/functional/rpc_fundrawtransaction.py:824 in 1002e2d0d7

 818 | @@ -817,9 +819,9 @@ def test_option_subtract_fee_from_outputs(self):
 819 |          rawtx = self.nodes[3].createrawtransaction(inputs, outputs)
 820 |  
 821 |          # Test subtract fee from outputs with feeRate (BTC/kvB)
 822 | -        result = [self.nodes[3].fundrawtransaction(rawtx),  # uses self.min_relay_tx_fee (set by settxfee)
 823 | +        result = [self.nodes[3].fundrawtransaction(rawtx),  # uses self.min_relay_tx_fee (set by setfeerate)
 824 |              self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}),  # empty subtraction list
 825 | -            self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}),  # uses self.min_relay_tx_fee (set by settxfee)
 826 | +            self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}),  # uses self.min_relay_tx_fee (set by setfeerate)

S3RK commented at 9:05 AM on February 17, 2021:

There are couple more comments to fix below

in test/functional/wallet_bumpfee.py:342 in 1002e2d0d7

 337 | +    rbfid = spend_one_input(rbf_node, dest_address)
 338 | +    fee_rate = 25
 339 | +    test_response(requested=fee_rate, expected=fee_rate, msg="Fee rate for transactions with this wallet successfully set to 25.000 sat/vB")
 340 | +    bumped_tx = rbf_node.bumpfee(rbfid)
 341 | +    actual_feerate = bumped_tx["fee"] * COIN / rbf_node.getrawtransaction(bumped_tx["txid"], True)["vsize"]
 342 | +    assert_greater_than(Decimal("0.01"), abs(fee_rate - actual_feerate))

S3RK commented at 8:20 AM on February 18, 2021:

Why this specific value of 0.01? In the test case with settxfee we have a different value of 0.00001000. Should we have a testcase-wise constant?

luke-jr commented at 1:33 AM on June 30, 2021:

Even 0.01 is too small for this. Signatures can be 71-73 bytes long, but even 72 bytes throws this off.

    bumped_txdetails = rbf_node.getrawtransaction(bumped_tx["txid"], True)
    allow_for_bytes_offset = len(bumped_txdetails['vout']) * 2  # potentially up to 2 bytes per output
    actual_fee = bumped_tx["fee"] * COIN
    assert_approx(actual_fee, fee_rate * bumped_txdetails['vsize'], fee_rate * allow_for_bytes_offset)

in test/functional/wallet_bumpfee.py:326 in 1002e2d0d7

 321 | +    self.log.info("Test setfeerate")
 322 | +
 323 | +    def test_response(*, wallet="RBF wallet", requested=0, expected=0, error=None, msg):
 324 | +        assert_equal(rbf_node.setfeerate(requested), {"wallet_name": wallet, "fee_rate": expected, ("error" if error else "result"): msg})
 325 | +
 326 | +    # Test setfeerate with too high/low values returns expected errors

S3RK commented at 8:22 AM on February 18, 2021:

This test has nothing to do with bumpfee RPC. I believe it's better to have it in a separate test. wallet_bumpfee.py is already huge

in test/functional/wallet_bumpfee.py:345 in 1002e2d0d7

 340 | +    bumped_tx = rbf_node.bumpfee(rbfid)
 341 | +    actual_feerate = bumped_tx["fee"] * COIN / rbf_node.getrawtransaction(bumped_tx["txid"], True)["vsize"]
 342 | +    assert_greater_than(Decimal("0.01"), abs(fee_rate - actual_feerate))
 343 | +    test_response(msg="Fee rate for transactions with this wallet successfully unset. By default, automatic fee selection will be used.")
 344 | +
 345 | +    # Test setfeerate with a different -maxtxfee

S3RK commented at 8:24 AM on February 18, 2021:

Same as above. This belongs to setfeerate test, not bumpfee test

in src/core_write.cpp:31 in aee3571392 outdated

  27 | @@ -27,6 +28,11 @@ UniValue ValueFromAmount(const CAmount& amount)
  28 |              strprintf("%s%d.%08d", sign ? "-" : "", quotient, remainder));
  29 |  }
  30 |  
  31 | +UniValue ValueFromFeeRate(const CFeeRate& fee_rate, FeeEstimateMode mode)

S3RK commented at 8:55 AM on February 18, 2021:

While I think it's a great idea to have this function, I'm a bit concerned that it'll be not used and forgotten. Do you have plans to do a follow up and put it to use by replacing ValuefromAmount in appropriate places ?

in src/wallet/rpcwallet.cpp:219 in c72083c2d5 outdated

 215 | @@ -216,7 +216,7 @@ 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 | +        cc.m_feerate = CFeeRate::FromSatB(AmountFromValue(fee_rate));

S3RK commented at 9:10 AM on February 18, 2021:

I love the idea of named constructors. Should we strive for consistency and use them in more places?

ryanofsky commented at 5:59 PM on July 13, 2021:

In commit "wallet: update rpcwallet to CFeeRate named constructors" (c72083c2d56237f8fbd3a82e3ff395f0cdb8fd57)

Wow wow. I see that this is doing the right thing, but these constructor names seem absurdly misleading! The AmountFromValue function converts a floating point number to a fixed-point representation of that number multiplied by 10e8. So even though the fee_rate univalue argument has units of sat/B, the AmountFromValue integer return value is no longer in the same units, because it has been multiplied by 100000000. So FromSatB should really be called CFeeRate::FromSatB_Fixed_Point_e8 or something like that because it is taking sat/B values with a very specific, atypical number representation.

An analogous thing is happening with FromBtcKb below. It doesn't really take a BTC/Kb argument. That's basically false advertising. What it really takes is a BTC/Kb*10e8 argument which is the same as a sat/Kb argument, so this constructor would be more accurately called CFeeRate::FromSatKb

I guess I'm having a negative reaction to these constructor names, but I do think this could be cleaned up pretty easily. I think it would be better if CFeeRate class had no involvement with univalue parsing or fixed point representations, and if it didn't (mis)use the CAmount type as a way to represent rates when CAmount is used almost everywhere else only to represent quantities.

My suggestion to be less confusing / misleading would be to drop the two new CFeeRate constructors, and just add standalone RPC util functions:

CFeeRate FeeRateFromSatB(const UniValue& value);
CFeeRate FeeRateFromBtcKb(const UniValue& value);

Then call these to simplify CFeeRate::FromSatB(AmountFromValue(feerate)) as FeeRateFromSatB(feerate) here and in setfeerate and to simplify CFeeRate::FromBtcKb(AmountFromValue(feerate)) as FeeRateFromBtcKb(feerate) in settxfee

MarcoFalke commented at 9:15 AM on September 28, 2021:

Wow wow. I see that this is doing the right thing, but these constructor names seem absurdly misleading!

The current code in master isn't exactly clear right now either:

    /** Constructor for a fee rate in satoshis per kvB (sat/kvB).
     *
     *  Passing a num_bytes value of COIN (1e8) returns a fee rate in satoshis per vB (sat/vB),
     *  e.g. (nFeePaid * 1e8 / 1e3) == (nFeePaid / 1e5),
     *  where 1e5 is the ratio to convert from BTC/kvB to sat/vB.
     */
    CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes);

I think it would be good to figure out how to clear this up before introducing further features that depend on this.

MarcoFalke commented at 9:17 AM on September 28, 2021:

Original discussion for reference: #20305 (review)

jonatack commented at 12:04 PM on September 29, 2021:

Wow wow. I see that this is doing the right thing, but these constructor names seem absurdly misleading!

The current code in master isn't exactly clear right now either:
    /** Constructor for a fee rate in satoshis per kvB (sat/kvB).
     *
     *  Passing a num_bytes value of COIN (1e8) returns a fee rate in satoshis per vB (sat/vB),
     *  e.g. (nFeePaid * 1e8 / 1e3) == (nFeePaid / 1e5),
     *  where 1e5 is the ratio to convert from BTC/kvB to sat/vB.
     */
    CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes);
I think it would be good to figure out how to clear this up before introducing further features that depend on this.

Ok, seems best to close this then. It's almost a year old and needs to be done differently. I think using sat/vB is still a popular request from users and plan to try again a bit later.

in test/functional/wallet_create_tx.py:90 in 529bfc16ff outdated

  85 | +        node = self.nodes[0]
  86 | +
  87 | +        def test_response(*, requested=0, expected=0, error=None, msg):
  88 | +            assert_equal(node.setfeerate(requested), {"wallet_name": "test_wallet", "fee_rate": expected, ("error" if error else "result"): msg})
  89 | +
  90 | +        # Test setfeerate with 10.0001 (CFeeRate rounding), "10.001" and "4" sat/vB

S3RK commented at 9:12 AM on February 18, 2021:

Again I believe those check would be better places in a separate setfeerate test

S3RK commented at 9:16 AM on February 18, 2021: member

I finished the review. A bunch of small questions/comments, but nothing serious. Happy to discuss and do further reviews.

DrahtBot added the label Needs rebase on Mar 3, 2021

DrahtBot commented at 7:25 PM on March 3, 2021: member

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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

Xekyo referenced this in commit 31054ef653 on Jun 25, 2021

ryanofsky commented at 6:02 PM on July 13, 2021: member

Started review (will update list below with progress). I got a little sidetracked on the cfeerate constructor names, but overall this seems like a very nice, well thought out PR

c4b288d5d9ac0035c1f2d935aa7d80d86d76a010 policy: add CFeeRate::FromSatB/FromBtcKb constructors (1/12)
c72083c2d56237f8fbd3a82e3ff395f0cdb8fd57 wallet: update rpcwallet to CFeeRate named constructors (2/12)
5ed3ca1fafcfe93c35737c26f6e826ed451081fa policy: allow CFeeRate::ToString to not append fee rate units (3/12)
aee3571392d095412ac6bda132ec6b1db6c42774 core_io: Add ValueFromFeeRate helper (4/12)
0479268c0830caea8cb6ff344531d85417b99d02 test: add ValueFromFeeRate/CFeeRate unit tests (5/12)
8e863e3d3ce457c9ca26a12e8fd6beac2f50aa7d wallet: introduce setfeerate, an improved settxfee in sat/vB (6/12)
529bfc16ffb35c5356e27f61d59395fae6707bcd test: add setfeerate functional coverage in wallet_create_tx.py (7/12)
c907f158a6bf3cad782d4441e02abcbda210265b test: add setfeerate functional coverage in wallet_bumpfee.py (8/12)
d87f0f3a923dca2ce8d99aa700b0beda8e42d3ec test: update functional tests from settxfee to setfeerate (9/12)
c594a002845e7003319252961fc35101fcb1496e wallet: update settxfee help (10/12)
3725a3f180fa8f6edeee2fdca3e09b6022c5eb21 wallet, test: make settxfee a hidden rpc (11/12)
1002e2d0d7f10221a3542391a429978cdcf0d426 wallet, test: upgradewallet follow-ups (12/12)

jonatack commented at 6:08 PM on July 13, 2021: member

Thanks! Huge apologies, since the merge of #21786 that resolved some of these things differently (at a lower level), the approach of the initial refactoring commits is obsolete and being changed. I need to push an update and have been intending to do so for some time. 🙏

jonatack closed this on Sep 29, 2021

luke-jr referenced this in commit a8ae170400 on Oct 10, 2021

luke-jr referenced this in commit d94518e507 on Oct 10, 2021

luke-jr referenced this in commit 031ebf4b20 on Oct 10, 2021

luke-jr referenced this in commit 1e6469d0ca on Oct 10, 2021

luke-jr referenced this in commit a77000fc19 on Dec 14, 2021

luke-jr referenced this in commit f807d1359b on May 21, 2022

luke-jr referenced this in commit f19c78c899 on May 21, 2022

luke-jr referenced this in commit a2293d9111 on May 21, 2022

luke-jr referenced this in commit fc4f061dd5 on May 21, 2022

DrahtBot locked this on Oct 30, 2022