Skip to content

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

Merged
maflcko merged 1 commit intobitcoin:masterfrom
jonatack:update-assert_fee_amount
Oct 1, 2021
Merged

test: update fee rate assertion helper in the functional test framework#23136
maflcko merged 1 commit intobitcoin:masterfrom
jonatack:update-assert_fee_amount

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Sep 29, 2021 •
edited
Loading

Follow-up to 42e1b5d (#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 jonatack force-pushed the update-assert_fee_amount branch from 0881046 to 4774f84 Compare September 29, 2021 14:16
maflcko
maflcko previously requested changes Sep 29, 2021
View reviewed changes
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.

Can you explain the changes?

@jonatack jonatack force-pushed the update-assert_fee_amount branch from 4774f84 to f4f0ec5 Compare September 29, 2021 14:28
@jonatack
Copy link
Member Author

jonatack commented Sep 29, 2021

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

@DrahtBot DrahtBot added the Tests label Sep 29, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 29, 2021 •
edited
Loading

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.

@DrahtBot DrahtBot mentioned this pull request Sep 30, 2021
Merged
This was referenced Sep 30, 2021
Closed
Closed
rajarshimaitra
rajarshimaitra reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Concept ACK.

Just one totally ignore-able nit.

@jonatack
test: update assert_fee_amount() in test_framework/util.py
b658d7d 
- 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
@jonatack jonatack force-pushed the update-assert_fee_amount branch from f4f0ec5 to b658d7d Compare September 30, 2021 14:42
meshcollider
meshcollider reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

utACK b658d7d

@maflcko
Copy link
Member

maflcko commented Oct 1, 2021

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

@maflcko maflcko merged commit 35a31d5 into bitcoin:master Oct 1, 2021
@jonatack jonatack deleted the update-assert_fee_amount branch October 1, 2021 10:09
@jonatack
Copy link
Member Author

jonatack commented Oct 1, 2021

Thanks everyone.

Sanity check if anyone passing by is curious:

--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -38,6 +38,12 @@ 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_old = round(tx_size * feerate_BTC_vB, 8)
+
+    import pprint
+    pprint.pprint(f"{target_fee_old} round()")
+    pprint.pprint(f"{target_fee} satoshi_round()")
+
     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
$ test/functional/wallet_basic.py 
2021-10-01T11:52:21.033000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_juu29c68
2021-10-01T11:52:22.323000Z TestFramework (INFO): Mining blocks...
2021-10-01T11:52:24.085000Z TestFramework (INFO): Test gettxout
2021-10-01T11:52:24.209000Z TestFramework (INFO): Test gettxout (second part)
'0.00014100 round()'
'0.00014100 satoshi_round()'
'0.00011000 round()'
'0.00011000 satoshi_round()'
2021-10-01T11:52:32.374000Z TestFramework (INFO): Test sendmany
'0.00014100 round()'
'0.00014100 satoshi_round()'
'0.00014100 round()'
'0.00014100 satoshi_round()'
2021-10-01T11:52:33.550000Z TestFramework (INFO): Test sendmany with fee_rate param (explicit fee rate in sat/vB)
'0.00000282 round()'
'0.00000282 satoshi_round()'
'0.00000282 round()'
'0.00000282 satoshi_round()'
2021-10-01T11:52:35.802000Z TestFramework (INFO): Test sendmany raises 'fee rate too low' if fee_rate of 0.99999999 is passed
2021-10-01T11:52:35.822000Z TestFramework (INFO): Test sendmany raises if an invalid fee_rate is passed
2021-10-01T11:52:35.926000Z TestFramework (INFO): Test sendmany raises if an invalid conf_target or estimate_mode is passed
2021-10-01T11:52:44.981000Z TestFramework (INFO): Test -walletbroadcast
2021-10-01T11:52:55.891000Z TestFramework (INFO): Test sendtoaddress with fee_rate param (explicit fee rate in sat/vB)
'0.00000282 round()'
'0.00000282 satoshi_round()'
'0.00000173 round()'
'0.00000173 satoshi_round()'
2021-10-01T11:53:03.407000Z TestFramework (INFO): Test sendtoaddress raises 'fee rate too low' if fee_rate of 0.99999999 is passed
2021-10-01T11:53:03.432000Z TestFramework (INFO): Test sendtoaddress raises if an invalid fee_rate is passed
2021-10-01T11:53:03.539000Z TestFramework (INFO): Test sendtoaddress raises if an invalid conf_target or estimate_mode is passed
2021-10-01T11:53:05.333000Z TestFramework (INFO): Test -reindex
2021-10-01T11:53:11.957000Z TestFramework (INFO): Testing gettransaction response with different arguments...
2021-10-01T11:53:11.976000Z TestFramework (INFO): Test send* RPCs with verbose=True
2021-10-01T11:53:12.210000Z TestFramework (INFO): Test send* RPCs with verbose=False
2021-10-01T11:53:12.462000Z TestFramework (INFO): Stopping nodes
2021-10-01T11:53:12.670000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_juu29c68 on exit
2021-10-01T11:53:12.670000Z TestFramework (INFO): Tests successful

@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.

Reviewers

3 more reviewers

@meshcollider meshcollider meshcollider left review comments

@rajarshimaitra rajarshimaitra rajarshimaitra left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Intermittent CI failure: "AssertionError: Fee of 0.00011000 BTC too low!" when running wallet_basic.py

5 participants

@jonatack @DrahtBot @maflcko @meshcollider @rajarshimaitra