Skip to content

fee: Round up fee calculation to avoid a lower than expected feerate #22949

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 4, 2021
Merged

fee: Round up fee calculation to avoid a lower than expected feerate #22949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fbaef9
fees: Always round up fee calculated from a feerate
achow101 Sep 11, 2021
ce2cc44
tests: Test for assertion when feerate is rounded down
achow101 Sep 11, 2021
80dc829
tests: Calculate fees more similarly to CFeeRate::GetFee
achow101 Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/policy/feerate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

#include <tinyformat.h>

#include <cmath>

CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)
{
const int64_t nSize{num_bytes};
Expand All @@ -22,7 +24,9 @@ CAmount CFeeRate::GetFee(uint32_t num_bytes) const
{
const int64_t nSize{num_bytes};

CAmount nFee = nSatoshisPerK * nSize / 1000;
// Be explicit that we're converting from a double to int64_t (CAmount) here.
// We've previously had issues with the silent double->int64_t conversion.
CAmount nFee{static_cast<CAmount>(std::ceil(nSatoshisPerK * nSize / 1000.0))};

if (nFee == 0 && nSize != 0) {
if (nSatoshisPerK > 0) nFee = CAmount(1);
Expand Down
1 change: 1 addition & 0 deletions src/policy/feerate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class CFeeRate
CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes);
/**
* Return the fee in satoshis for the given size in bytes.
* If the calculated fee would have fractional satoshis, then the returned fee will always be rounded up to the nearest satoshi.
*/
CAmount GetFee(uint32_t num_bytes) const;
/**
Expand Down
10 changes: 5 additions & 5 deletions src/test/amount_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), CAmount(-9e3));

feeRate = CFeeRate(123);
// Truncates the result, if not integer
// Rounds up the result, if not integer
BOOST_CHECK_EQUAL(feeRate.GetFee(0), CAmount(0));
BOOST_CHECK_EQUAL(feeRate.GetFee(8), CAmount(1)); // Special case: returns 1 instead of 0
BOOST_CHECK_EQUAL(feeRate.GetFee(9), CAmount(1));
BOOST_CHECK_EQUAL(feeRate.GetFee(121), CAmount(14));
BOOST_CHECK_EQUAL(feeRate.GetFee(122), CAmount(15));
BOOST_CHECK_EQUAL(feeRate.GetFee(999), CAmount(122));
BOOST_CHECK_EQUAL(feeRate.GetFee(9), CAmount(2));
BOOST_CHECK_EQUAL(feeRate.GetFee(121), CAmount(15));
BOOST_CHECK_EQUAL(feeRate.GetFee(122), CAmount(16));
BOOST_CHECK_EQUAL(feeRate.GetFee(999), CAmount(123));
BOOST_CHECK_EQUAL(feeRate.GetFee(1e3), CAmount(123));
BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), CAmount(1107));

Expand Down
4 changes: 2 additions & 2 deletions src/test/transaction_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,10 +810,10 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
// nDustThreshold = 182 * 3702 / 1000
dustRelayFee = CFeeRate(3702);
// dust:
t.vout[0].nValue = 673 - 1;
t.vout[0].nValue = 674 - 1;
CheckIsNotStandard(t, "dust");
// not dust:
t.vout[0].nValue = 673;
t.vout[0].nValue = 674;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this changes behaviour between releases if the user changed the default dust rate.

Copy link
Member

Choose a reason for hiding this comment

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

Same for -blockmintxfee, and -incrementalRelayFee, sendrawtransaction/testmempoolaccept feerate RPC arg, checks in CheckFeeRate.

CheckIsStandard(t);
dustRelayFee = CFeeRate(DUST_RELAY_TX_FEE);

Expand Down
28 changes: 28 additions & 0 deletions test/functional/rpc_fundrawtransaction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ def run_test(self):
self.test_include_unsafe()
self.test_external_inputs()
self.test_22670()
self.test_feerate_rounding()

def test_change_position(self):
"""Ensure setting changePosition in fundraw with an exact match is handled properly."""
Expand Down Expand Up @@ -1129,6 +1130,33 @@ def do_fund_send(target):
do_fund_send(upper_bound)

self.restart_node(0)
self.connect_nodes(0, 1)
self.connect_nodes(0, 2)
self.connect_nodes(0, 3)

def test_feerate_rounding(self):
self.log.info("Test that rounding of GetFee does not result in an assertion")

self.nodes[1].createwallet("roundtest")
w = self.nodes[1].get_wallet_rpc("roundtest")

addr = w.getnewaddress(address_type="bech32")
self.nodes[0].sendtoaddress(addr, 1)
self.generate(self.nodes[0], 1)
self.sync_all()

# A P2WPKH input costs 68 vbytes; With a single P2WPKH output, the rest of the tx is 42 vbytes for a total of 110 vbytes.
# At a feerate of 1.85 sat/vb, the input will need a fee of 125.8 sats and the rest 77.7 sats
# The entire tx fee should be 203.5 sats.
# Coin selection rounds the fee individually instead of at the end (due to how CFeeRate::GetFee works).
# If rounding down (which is the incorrect behavior), then the calculated fee will be 125 + 77 = 202.
# If rounding up, then the calculated fee will be 126 + 78 = 204.
# In the former case, the calculated needed fee is higher than the actual fee being paid, so an assertion is reached
# To test this does not happen, we subtract 202 sats from the input value. If working correctly, this should
# fail with insufficient funds rather than bitcoind asserting.
rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(address_type="bech32"): 1 - 0.00000202}])
assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, {"fee_rate": 1.85})


if __name__ == '__main__':
RawTransactionsTest().main()
18 changes: 15 additions & 3 deletions test/functional/test_framework/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ def assert_approx(v, vexp, vspan=0.00001):

def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
"""Assert the fee is in range."""
feerate_BTC_vB = feerate_BTC_kvB / 1000
target_fee = satoshi_round(tx_size * feerate_BTC_vB)
target_fee = get_fee(tx_size, feerate_BTC_kvB)
if fee < target_fee:
raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)" % (str(fee), str(target_fee)))
# allow the wallet's estimation to be at most 2 bytes off
if fee > (tx_size + 2) * feerate_BTC_vB:
high_fee = get_fee(tx_size + 2, feerate_BTC_kvB)
if fee > high_fee:
raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))


Expand Down Expand Up @@ -218,6 +218,18 @@ def str_to_b64str(string):
return b64encode(string.encode('utf-8')).decode('ascii')


def ceildiv(a, b):
"""Divide 2 ints and round up to next int rather than round down"""
return -(-a // b)


def get_fee(tx_size, feerate_btc_kvb):
"""Calculate the fee in BTC given a feerate is BTC/kvB. Reflects CFeeRate::GetFee"""
feerate_sat_kvb = int(feerate_btc_kvb * Decimal(1e8)) # Fee in sat/kvb as an int to avoid float precision errors
Copy link
Member

@maflcko maflcko Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It seems a bug to silently accept sub-decimal feerates (Bitcoin Core doesn't accept them either). I think this can be fixed (and the whole conversion avoided) by simply changing all call sites to provide the value in feerate_sat_vB. I can create a pull for that, if this is too unrelated to this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a bug to silently accept sub-decimal feerates

This doesn't seem like bug conceptually. Fees are discrete values, so a fixed precision decimal representation makes sense for absolute fees. Feerates are continuous values (ratio of the discrete values, rational numbers) so any floating or fractional representation makes sense and while error feedback about being out of range would be useful, error feedback about being too precise would be strange.

Also, this is just a drive-by review comment so feel free to ignore, but the math here converting between '0.00000001' string literals and 1e8 floating point literals, and decimal objects, long integer objects plus use of a satoshi_round function that is rounding down when you are trying to round up seems overcomplicated even if it is correct. If I were trying to write this in the most readable way I would probably do all the math with python long integers and a ceildiv helper, and just return a Decimal object at the end.

def ceildiv(a, b):
    return -(-a // b)

A ceildiv function would probably also simplify the c++ code, though the implementation in c++ would be different (something like (a + b - 1) / b)

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like bug conceptually.

Sure, feerates in theory are rational numbers. However, this function is there to replicate the behavior of Bitcoin Core when it comes to feerates. And Bitcoin Core doesn't accept rational numbers as feerates. Only natural numbers, which represent a sat/vB ratio.

Copy link
Member Author

@achow101 achow101 Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

CFeeRates are BTC/kvb so we actually can get sub-decimal sat/vb feerates, to 4 decimal places of precision.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a ceildiv function as suggested.

I think any broader feerate changes to the tests should be done in another PR. The goal of the test changes here is to just make sure that the calculation is the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be fixed (and the whole conversion avoided) by simply changing all call sites to provide the value in feerate_sat_vB. I can create a pull for that, if this is too unrelated to this pull request.

I think this could be a nice simplification for another PR. This function is only used by assert_fee_amount which is should be convenient for developers and make tests readable. I don't know if it's make tests more readable to be able to write fees as feerate_btc_kvb and have this automatic conversion, or to write fees as feerate_sat_kvb and avoid the need for this conversion. I just don't think it's a bug for the conversion to accept fractional feerates since they do make sense conceptually and since this line isn't really trying to emulate anything in bitcoin core.

target_fee_sat = ceildiv(feerate_sat_kvb * tx_size, 1000) # Round calculated fee up to nearest sat
return satoshi_round(target_fee_sat / Decimal(1e8)) # Truncate BTC result to nearest sat
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 "tests: Calculate fees more similarly to CFeeRate::GetFee" (80dc829)

I think the satoshi_round call just complicates things here and should be dropped. Just return target_fee_sat / Decimal(1e8) should already return a decimal object with the right level of precision



def satoshi_round(amount):
return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)

Expand Down
2 changes: 1 addition & 1 deletion test/functional/wallet_keypool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ def run_test(self):
assert_equal("psbt" in res, True)

# create a transaction without change at the maximum fee rate, such that the output is still spendable:
res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.0008824})
res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.0008823})
assert_equal("psbt" in res, True)
assert_equal(res["fee"], Decimal("0.00009706"))

Expand Down