Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 12 commits into from

Conversation

jonatack
Copy link
Contributor

@jonatack jonatack commented Nov 14, 2020

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
Copy link
Contributor Author

Thanks to @xekyo for suggesting this in #20305 (comment).

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 15, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link

ghost commented Nov 15, 2020

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

@jonatack
Copy link
Contributor Author

jonatack commented Nov 15, 2020

@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!)

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@jonatack
Copy link
Contributor Author

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

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/functional/rpc_getblockstats.py Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
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.
per PR 20403 review feedback by MarcoFalke
@S3RK
Copy link
Contributor

S3RK commented Feb 11, 2021

I start reviewing this one.

Copy link
Contributor

@S3RK S3RK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

switch (mode) {
case FeeEstimateMode::SAT_VB:
return strprintf("%d.%03d %s/vB", nSatoshisPerK / 1000, nSatoshisPerK % 1000, CURRENCY_ATOM);
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

BOOST_CHECK_EQUAL(CFeeRate(3141).ToString(FeeEstimateMode::SAT_VB, /* with_units */ false), "3.141");
BOOST_CHECK_EQUAL(CFeeRate(10002).ToString(FeeEstimateMode::SAT_VB, /* with_units */ false), "10.002");
BOOST_CHECK_EQUAL(CFeeRate(3141).ToString(FeeEstimateMode::BTC_KVB, /* with_units */ false), "0.00003141");
BOOST_CHECK_EQUAL(CFeeRate(10002).ToString(FeeEstimateMode::BTC_KVB, /* with_units */ false), "0.00010002");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we pass an absurdly big number?

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

BOOST_AUTO_TEST_CASE(rpc_parse_fee_rate_values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

default: return strprintf("%d.%08d %s/kvB", nSatoshisPerK / COIN, nSatoshisPerK % COIN, CURRENCY_UNIT);
if (with_units) {
switch (mode) {
case FeeEstimateMode::SAT_VB:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

LOCK(wallet.cs_wallet);

const CFeeRate amount{CFeeRate::FromSatB(AmountFromValue(request.params[0]))};
const CFeeRate relay_min_feerate{wallet.chain().relayMinFee().GetFeePerK()};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need GetFeePerK() call here and below?

BOOST_CHECK(CFeeRate::FromSatB(CAmount(100000)) == CFeeRate(1));
BOOST_CHECK(CFeeRate::FromSatB(CAmount(100001)) == CFeeRate(1));
BOOST_CHECK(CFeeRate::FromSatB(CAmount(2690000)) == CFeeRate(26));
BOOST_CHECK(CFeeRate::FromSatB(CAmount(123456789)) == CFeeRate(1234));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we pass an absurdly big number?

@jonatack
Copy link
Contributor Author

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

Copy link
Contributor

@S3RK S3RK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -717,7 +719,7 @@ def test_option_feerate(self):

result = node.fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee)
btc_kvb_to_sat_vb = 100000 # (1e5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already defined earlier

fee_per_byte = Decimal('0.001') / 1000
self.nodes[2].settxfee(fee_per_byte * 1000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fee_per_byte = Decimal('0.001') / 1000
self.nodes[2].settxfee(fee_per_byte * 1000)
fee_sat_per_byte = 100
self.nodes[2].setfeerate(fee_sat_per_byte)

@@ -717,7 +719,7 @@ def test_option_feerate(self):

result = node.fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}), # empty subtraction list
self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by settxfee)
self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by setfeerate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are couple more comments to fix below

test_response(requested=fee_rate, expected=fee_rate, msg="Fee rate for transactions with this wallet successfully set to 25.000 sat/vB")
bumped_tx = rbf_node.bumpfee(rbfid)
actual_feerate = bumped_tx["fee"] * COIN / rbf_node.getrawtransaction(bumped_tx["txid"], True)["vsize"]
assert_greater_than(Decimal("0.01"), abs(fee_rate - actual_feerate))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
assert_greater_than(Decimal("0.01"), abs(fee_rate - actual_feerate))
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)

def test_response(*, wallet="RBF wallet", requested=0, expected=0, error=None, msg):
assert_equal(rbf_node.setfeerate(requested), {"wallet_name": wallet, "fee_rate": expected, ("error" if error else "result"): msg})

# Test setfeerate with too high/low values returns expected errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert_greater_than(Decimal("0.01"), abs(fee_rate - actual_feerate))
test_response(msg="Fee rate for transactions with this wallet successfully unset. By default, automatic fee selection will be used.")

# Test setfeerate with a different -maxtxfee
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -27,6 +28,11 @@ UniValue ValueFromAmount(const CAmount& amount)
strprintf("%s%d.%08d", sign ? "-" : "", quotient, remainder));
}

UniValue ValueFromFeeRate(const CFeeRate& fee_rate, FeeEstimateMode mode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@@ -216,7 +216,7 @@ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const Un
if (!estimate_mode.isNull() && estimate_mode.get_str() != "unset") {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate");
}
cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN);
cc.m_feerate = CFeeRate::FromSatB(AmountFromValue(fee_rate));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def test_response(*, requested=0, expected=0, error=None, msg):
assert_equal(node.setfeerate(requested), {"wallet_name": "test_wallet", "fee_rate": expected, ("error" if error else "result"): msg})

# Test setfeerate with 10.0001 (CFeeRate rounding), "10.001" and "4" sat/vB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 3, 2021

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

@@ -216,7 +216,7 @@ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const Un
if (!estimate_mode.isNull() && estimate_mode.get_str() != "unset") {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate");
}
cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN);
cc.m_feerate = CFeeRate::FromSatB(AmountFromValue(fee_rate));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original discussion for reference: #20305 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • c4b288d policy: add CFeeRate::FromSatB/FromBtcKb constructors (1/12)
  • c72083c wallet: update rpcwallet to CFeeRate named constructors (2/12)
  • 5ed3ca1 policy: allow CFeeRate::ToString to not append fee rate units (3/12)
  • aee3571 core_io: Add ValueFromFeeRate helper (4/12)
  • 0479268 test: add ValueFromFeeRate/CFeeRate unit tests (5/12)
  • 8e863e3 wallet: introduce setfeerate, an improved settxfee in sat/vB (6/12)
  • 529bfc1 test: add setfeerate functional coverage in wallet_create_tx.py (7/12)
  • c907f15 test: add setfeerate functional coverage in wallet_bumpfee.py (8/12)
  • d87f0f3 test: update functional tests from settxfee to setfeerate (9/12)
  • c594a00 wallet: update settxfee help (10/12)
  • 3725a3f wallet, test: make settxfee a hidden rpc (11/12)
  • 1002e2d wallet, test: upgradewallet follow-ups (12/12)

@jonatack
Copy link
Contributor Author

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 jonatack closed this Sep 29, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
Was: test: update functional tests from settxfee to setfeerate

Github-Pull: bitcoin#20391
Rebased-From: d87f0f3 (partial)
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021
Was: test: update functional tests from settxfee to setfeerate

Github-Pull: bitcoin#20391
Rebased-From: d87f0f3 (partial)
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
Was: test: update functional tests from settxfee to setfeerate

Github-Pull: bitcoin#20391
Rebased-From: d87f0f3 (partial)
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants