test: update fee rate assertion helper in the functional test framework

jonatack commented at 2:11 pm on September 29, 2021: member

Follow-up to 42e1b5d9797b65 (#12486).

update call to round() with our utility function satoshi_round() to avoid intermittent test failures
rename fee_per_kB to feerate_BTC_kvB for precision
store division result in feerate_BTC_vB

Possibly resolves #19418.

jonatack force-pushed on Sep 29, 2021

in test/functional/rpc_fundrawtransaction.py:783 in 4774f842fc outdated

782-        assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate)
783-        assert_fee_amount(result4['fee'], count_bytes(result4['hex']), 10 * result_fee_rate)
784+        assert_feerate_amount(result1['fee'], count_bytes(result1['hex']), 2 * result_fee_rate)
785+        assert_feerate_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate)
786+        assert_feerate_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate)
787+        assert_feerate_amount(result4['fee'], count_bytes(result4['hex']), 10 * result_fee_rate)

MarcoFalke commented at 2:18 pm on September 29, 2021:

the first arg is fee (absolute fee), not a feerate

jonatack commented at 2:29 pm on September 29, 2021:

oops. thanks, fixed, change is much smaller now

MarcoFalke dismissed

MarcoFalke commented at 2:18 pm on September 29, 2021: member

Can you explain the changes?

jonatack force-pushed on Sep 29, 2021

jonatack commented at 2:33 pm on September 29, 2021: member

Thanks for the quick look. Fixed, simplified, and updated the description to explain the changes.

DrahtBot added the label Tests on Sep 29, 2021

DrahtBot commented at 5:03 pm on September 29, 2021: 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:

#23141 (test: Fix intermitent failure in wallet_basic.py by meshcollider)
#22364 (wallet: Make a tr() descriptor by default 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.

in test/functional/test_framework/util.py:37 in f4f0ec5e2b outdated

33@@ -34,13 +34,13 @@ def assert_approx(v, vexp, vspan=0.00001):
34         raise AssertionError("%s > [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
35 
36 
37-def assert_fee_amount(fee, tx_size, fee_per_kB):
38-    """Assert the fee was in range"""
39-    target_fee = round(tx_size * fee_per_kB / 1000, 8)
40+def assert_fee_amount(fee, tx_size, feerate_per_kvB):

rajarshimaitra commented at 1:39 pm on September 30, 2021:

Wondering if feerate_per_kvB is better than fee_per_kvB. Technically _per_kvB indicates a rate, so feerate_per_kvB may sound like rate of a rate.

No hard opinion in either way.

jonatack commented at 2:44 pm on September 30, 2021:

Good point, updated to the more precise feerate_BTC_kvB. Let me know if that’s better.

rajarshimaitra commented at 6:55 pm on September 30, 2021:

Sorry I am probably bikeshedding here, this one seems even more confusing. To me the original fee_per_kvB seems reasonable.

jonatack commented at 8:45 pm on September 30, 2021:

Yes, if you run something like git grep -ni "btc_kvb\|sat_vb" from repo root, you’ll see that we have been working to upgrade and standardize how we describe fee rates

from fee to fee rate or feerate
from fee per (denominator without specifying the numerator) to fee rate in numerator/denominator
from fee rate units like BTC or satoshis or kB or B, to BTC/kvB and sat/vB

rajarshimaitra commented at 1:40 pm on September 30, 2021: contributor

Concept ACK.

Just one totally ignore-able nit.

test: update assert_fee_amount() in test_framework/util.py

- update call to round() with satoshi_round() to avoid intermittent test failures
- rename fee_per_kB to feerate_BTC_kvB for precision
- store division result in feerate_BTC_vB

b658d7d5c5

jonatack force-pushed on Sep 30, 2021

meshcollider commented at 9:35 pm on September 30, 2021: contributor

utACK b658d7d5c5339739dc19bf961d84186469a818d5

MarcoFalke commented at 8:54 am on October 1, 2021: member

I think the test is correct and this is a bug that should be fixed: #22949 (comment)

MarcoFalke merged this on Oct 1, 2021

MarcoFalke closed this on Oct 1, 2021

jonatack deleted the branch on Oct 1, 2021

jonatack commented at 11:56 am on October 1, 2021: member

Thanks everyone.

Sanity check if anyone passing by is curious:

 0--- a/test/functional/test_framework/util.py
 1+++ b/test/functional/test_framework/util.py
 2@@ -38,6 +38,12 @@ def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
 3     """Assert the fee is in range."""
 4     feerate_BTC_vB = feerate_BTC_kvB / 1000
 5     target_fee = satoshi_round(tx_size * feerate_BTC_vB)
 6+    target_fee_old = round(tx_size * feerate_BTC_vB, 8)
 7+
 8+    import pprint
 9+    pprint.pprint(f"{target_fee_old} round()")
10+    pprint.pprint(f"{target_fee} satoshi_round()")
11+
12     if fee < target_fee:
13         raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)" % (str(fee), str(target_fee)))
14     # allow the wallet's estimation to be at most 2 bytes off

 0$ test/functional/wallet_basic.py 
 12021-10-01T11:52:21.033000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_juu29c68
 22021-10-01T11:52:22.323000Z TestFramework (INFO): Mining blocks...
 32021-10-01T11:52:24.085000Z TestFramework (INFO): Test gettxout
 42021-10-01T11:52:24.209000Z TestFramework (INFO): Test gettxout (second part)
 5'0.00014100 round()'
 6'0.00014100 satoshi_round()'
 7'0.00011000 round()'
 8'0.00011000 satoshi_round()'
 92021-10-01T11:52:32.374000Z TestFramework (INFO): Test sendmany
10'0.00014100 round()'
11'0.00014100 satoshi_round()'
12'0.00014100 round()'
13'0.00014100 satoshi_round()'
142021-10-01T11:52:33.550000Z TestFramework (INFO): Test sendmany with fee_rate param (explicit fee rate in sat/vB)
15'0.00000282 round()'
16'0.00000282 satoshi_round()'
17'0.00000282 round()'
18'0.00000282 satoshi_round()'
192021-10-01T11:52:35.802000Z TestFramework (INFO): Test sendmany raises 'fee rate too low' if fee_rate of 0.99999999 is passed
202021-10-01T11:52:35.822000Z TestFramework (INFO): Test sendmany raises if an invalid fee_rate is passed
212021-10-01T11:52:35.926000Z TestFramework (INFO): Test sendmany raises if an invalid conf_target or estimate_mode is passed
222021-10-01T11:52:44.981000Z TestFramework (INFO): Test -walletbroadcast
232021-10-01T11:52:55.891000Z TestFramework (INFO): Test sendtoaddress with fee_rate param (explicit fee rate in sat/vB)
24'0.00000282 round()'
25'0.00000282 satoshi_round()'
26'0.00000173 round()'
27'0.00000173 satoshi_round()'
282021-10-01T11:53:03.407000Z TestFramework (INFO): Test sendtoaddress raises 'fee rate too low' if fee_rate of 0.99999999 is passed
292021-10-01T11:53:03.432000Z TestFramework (INFO): Test sendtoaddress raises if an invalid fee_rate is passed
302021-10-01T11:53:03.539000Z TestFramework (INFO): Test sendtoaddress raises if an invalid conf_target or estimate_mode is passed
312021-10-01T11:53:05.333000Z TestFramework (INFO): Test -reindex
322021-10-01T11:53:11.957000Z TestFramework (INFO): Testing gettransaction response with different arguments...
332021-10-01T11:53:11.976000Z TestFramework (INFO): Test send* RPCs with verbose=True
342021-10-01T11:53:12.210000Z TestFramework (INFO): Test send* RPCs with verbose=False
352021-10-01T11:53:12.462000Z TestFramework (INFO): Stopping nodes
362021-10-01T11:53:12.670000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_juu29c68 on exit
372021-10-01T11:53:12.670000Z TestFramework (INFO): Tests successful

DrahtBot locked this on Oct 30, 2022

test: update fee rate assertion helper in the functional test framework #23136

Conflicts