From db76db7329f6357c5226cd08611fe0f669c002af Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 22 Sep 2021 11:32:25 +0200 Subject: Fix (inverse) meaning of -persistmempool Github-Pull: #23061 Rebased-From: faff17bbde6dcb1482a6210bc48b3192603a446f --- test/functional/mempool_persist.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'test/functional') diff --git a/test/functional/mempool_persist.py b/test/functional/mempool_persist.py index 752b925b92..1ae95fced3 100755 --- a/test/functional/mempool_persist.py +++ b/test/functional/mempool_persist.py @@ -141,7 +141,7 @@ class MempoolPersistTest(BitcoinTestFramework): self.log.debug("Stop nodes, make node1 use mempool.dat from node0. Verify it has 6 transactions") os.rename(mempooldat0, mempooldat1) self.stop_nodes() - self.start_node(1, extra_args=[]) + self.start_node(1, extra_args=["-persistmempool"]) assert self.nodes[1].getmempoolinfo()["loaded"] assert_equal(len(self.nodes[1].getrawmempool()), 6) -- cgit v1.2.3 From bd7e08e36bf2e1238ddf8cc01433f8db82f848c9 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 10 Sep 2021 20:24:44 -0400 Subject: fees: Always round up fee calculated from a feerate When calculating the fee for a given tx size from a fee rate, we should always round up to the next satoshi. Otherwise, if we round down (via truncation), the calculated fee may result in a fee with a feerate slightly less than targeted. This is particularly important for coin selection as a slightly lower feerate than expected can result in a variety of issues. Github-Pull: #22949 Rebased-From: 0fbaef9676a1dcb84bcf95afd8d994831ab327b6 --- test/functional/wallet_keypool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'test/functional') diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index 28bfc9116f..9286387f96 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -179,7 +179,7 @@ class KeyPoolTest(BitcoinTestFramework): 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")) -- cgit v1.2.3 From f66bc42957ad2e86982c8c487f821683d3009b43 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 10 Sep 2021 20:27:41 -0400 Subject: tests: Test for assertion when feerate is rounded down When calculating a txs absolute fee, if the fee is rounded down to the nearest satoshi, it is possible for the coin selection algorithms to undercalculate the fee needed. This can lead to an assertion error in some situations. One such scenario is added to rpc_fundrawtransaction.py. Github-Pull: #22949 Rebased-From: ce2cc44afd51f3df4ee7f14ea05b8da229183923 --- test/functional/rpc_fundrawtransaction.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) (limited to 'test/functional') diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index fcc310394a..bbd3dbb10c 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -100,6 +100,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.test_transaction_too_large() self.test_include_unsafe() self.test_22670() + self.test_feerate_rounding() def test_change_position(self): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" @@ -1026,6 +1027,33 @@ class RawTransactionsTest(BitcoinTestFramework): 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.nodes[0].generate(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() -- cgit v1.2.3 From c768bfa08af034c744402d4294cc323d653b97b8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 8 Oct 2021 13:57:48 -0400 Subject: tests: Calculate fees more similarly to CFeeRate::GetFee Because of floating point precision issues, not all of the rounding done is always correct. To fix this, the fee calculation for assert_fee_amount is changed to better reflect how CFeeRate::GetFee does it. First the feerate is converted to an int representing sat/kvb. Then this is multiplied by the transaction size, divivided by 1000, and rounded up to the nearest sat. The result is then converted back to BTC (divided by 1e8) and then rounded down to the nearest sat to avoid precision errors. Github-Pull: #22949 Rebased-From: 80dc829be7f8c3914074b85bb4c125baba18cb2c --- test/functional/test_framework/util.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) (limited to 'test/functional') diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 35dbfbba8d..72fd83e3f7 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -35,13 +35,14 @@ def assert_approx(v, vexp, vspan=0.00001): raise AssertionError("%s > [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan))) -def assert_fee_amount(fee, tx_size, fee_per_kB): - """Assert the fee was in range""" - target_fee = round(tx_size * fee_per_kB / 1000, 8) +def assert_fee_amount(fee, tx_size, feerate_BTC_kvB): + """Assert the fee is in range.""" + 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) * fee_per_kB / 1000: + 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))) @@ -222,6 +223,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 + 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 + + def satoshi_round(amount): return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN) -- cgit v1.2.3 From 269553fe73b17f8acda3071a48836c66092d31d0 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 2 Feb 2022 17:52:05 +0100 Subject: test: Call ceildiv helper with integer It returns an incorrect result when called with a Decimal, for which the "//" operator works differently. Also drop unnecessary call to satoshi_round. Github-Pull: #24239 Rebased-From: d1fab9d5d27a2db2546db0f610e0f6929ec4864e --- test/functional/test_framework/util.py | 11 +++++++++-- test/functional/wallet_send.py | 9 +++++---- 2 files changed, 14 insertions(+), 6 deletions(-) (limited to 'test/functional') diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 72fd83e3f7..8a41ec4370 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -37,6 +37,7 @@ def assert_approx(v, vexp, vspan=0.00001): def assert_fee_amount(fee, tx_size, feerate_BTC_kvB): """Assert the fee is in range.""" + assert isinstance(tx_size, int) 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))) @@ -224,7 +225,13 @@ def str_to_b64str(string): def ceildiv(a, b): - """Divide 2 ints and round up to next int rather than round down""" + """ + Divide 2 ints and round up to next int rather than round down + Implementation requires python integers, which have a // operator that does floor division. + Other types like decimal.Decimal whose // operator truncates towards 0 will not work. + """ + assert isinstance(a, int) + assert isinstance(b, int) return -(-a // b) @@ -232,7 +239,7 @@ 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 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 + return target_fee_sat / Decimal(1e8) # Return result in BTC def satoshi_round(amount): diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index d24d1693af..d469af600e 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -15,6 +15,7 @@ from test_framework.util import ( assert_fee_amount, assert_greater_than, assert_raises_rpc_error, + count_bytes, ) class WalletSendTest(BitcoinTestFramework): @@ -318,20 +319,20 @@ class WalletSendTest(BitcoinTestFramework): res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=7, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] - assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00007")) + assert_fee_amount(fee, count_bytes(res["hex"]), Decimal("0.00007")) # "unset" and None are treated the same for estimate_mode res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=2, estimate_mode="unset", add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] - assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002")) + assert_fee_amount(fee, count_bytes(res["hex"]), Decimal("0.00002")) res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=4.531, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] - assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00004531")) + assert_fee_amount(fee, count_bytes(res["hex"]), Decimal("0.00004531")) res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=3, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] - assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00003")) + assert_fee_amount(fee, count_bytes(res["hex"]), Decimal("0.00003")) # Test that passing fee_rate as both an argument and an option raises. self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=1, fee_rate=1, add_to_wallet=False, -- cgit v1.2.3