test: Fee estimation functional test cleanups #23075

darosior · 2021-09-23T14:42:28Z

Some cleanups that i noticed would be desirable while working on #23074 and #22539, which are intentionally not based on it. Mainly simplifications and a slight speedup.

Use a single tx to create the 2**9 coins instead of creating 2**8 2-outputs transactions
Use a single P2SH script
Avoid the use of non-standard transactions
Misc style nits (happy to take more)

DrahtBot · 2021-09-23T22:13:29Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13990 (Allow fee estimation to work with lower fees by ajtowns)

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.

theStack · 2021-10-04T22:33:50Z

Concept ACK

After #22539 is merged, I'd suggest to also replace bare asserts with assert_equal()/assert_greater_than() helpers in the newly introduced subtest sanity_check_rbf_estimates (plus other fixups proposed in #22539).

darosior · 2021-10-15T10:56:48Z

Rebased and addressed @theStack 's feedback from both here and #22539. But i forgot #22539 (comment), will address but i think it can start to be reviewed in the meantime.

glozow

Concept ACK

darosior · 2021-11-15T00:09:19Z

Rebased. Gave a look to replacing the use of the P2SH anyonecanspend by Miniwallet and it's actually a pretty invasive refactoring of the test.

DariusParvin · 2021-12-01T22:22:45Z

Concept ACK. In a future PR, I think the initial_split function could be moved to the MiniWallet since other tests could benefit from it too.

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior · 2021-12-09T12:27:36Z

Rebased.

glozow

overall looks good, nice cleanup. I think there's a commit diff reorg needed - feature_fee_estimation.py doesn't pass in 9d6a88e because it doesn't fully remove instances of SCRIPT_SIG

test/functional/feature_fee_estimation.py

glozow · 2021-12-09T13:39:33Z

Also seems like you tried to co-author @theStack in 7ccd775, but it needs to be on the 1st line maybe?

fanquake · 2021-12-09T13:41:22Z

Also seems like you tried to co-author

The issue is that Co-Atuhored-By: is spelt wrong.

Using 2 different scripts is unnecessary complication Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

This simplifies the code, and slightly speeds up the test. Running `./test/functional/test_runner.py -j15 $(printf 'feature_fee_estimation %.0s' {1..15})` on master 3 times gives: - Before: ALL | ✓ Passed | 788 s (accumulated) ALL | ✓ Passed | 818 s (accumulated) ALL | ✓ Passed | 873 s (accumulated) - After: ALL | ✓ Passed | 763 s (accumulated) ALL | ✓ Passed | 798 s (accumulated) ALL | ✓ Passed | 731 s (accumulated) Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

We don't need dust outputs anymore. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

Followups to bitcoin#22539 Co-Authored-By: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

…ation test

darosior · 2021-12-09T14:36:03Z

Thanks you both! Done.

pg156

Thank you for the pr. As a newcomer, I find it very educational to review.

I ACK all commits up to 60ae116 (except 1fc0315, where I have a question more for my own learning than actually questioning the PR). I built and ran the test successfully. I agree after the changes, the behavior is kept the same and the code is shorter and easier to reason.

pg156 · 2021-12-30T14:44:02Z

test/functional/feature_fee_estimation.py

+        self.confutxo = [
+            {"txid": txid, "vout": i, "amount": splitted_amount}
+            for i in range(utxo_count)
+        ]
        while len(node.getrawmempool()) > 0:
            self.generate(node, 1, sync_fun=self.no_op)


Is the node "state" here expected to exactly the same before and after changes in 1fc0315? If so, how to test?

E.g. this is what I tried to compare:

before

node.getbalance() returns Decimal('1200.00000000')
len(node.listunspent()) returns 24

after

node.getbalance() returns Decimal('1150.00000000')
len(node.listunspent()) returns 23

Is the node "state" here expected to exactly the same before and after changes in 1fc0315?

It's not. At least, it was not my intention.

pg156 · 2021-12-30T15:15:31Z

test/functional/feature_fee_estimation.py

+
+    # vbytes == bytes as we are using legacy transactions
+    fee = tx.get_vsize() * feerate
+    tx.vout[0].nValue -= fee



@darosior said "since we use legacy txs in this test it's indeed much cleaner" in #22539 (comment). Does this mean for non-legacy txs, we can no longer use the same logic fee = tx.get_vsize() * feerate to calculate fee?

#22539 (comment) was refering to using the length of the serialized transaction. This works only for legacy transactions as there is no witness data discount (hence 1 byte == 1 vbyte).

glozow

utACK 60ae116

Light code re-review, commits all pass now. Thanks for addressing comments, sorry for the delay!

glozow · 2022-01-04T10:24:59Z

test/functional/feature_fee_estimation.py

        # Force fSendTrickle to true (via whitelist.noban)
        self.extra_args = [
-            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1"],
-            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=68000"],
-            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=32000"],


yus 🚀 big fan of removing -acceptnonstdtxn

maflcko

LGTM

maflcko · 2022-01-06T12:31:29Z

test/functional/feature_fee_estimation.py

-P2SH_2 = script_to_p2sh_script(REDEEM_SCRIPT_2)
+SCRIPT = CScript([OP_1, OP_DROP])
+P2SH = script_to_p2sh_script(SCRIPT)
+REDEEM_SCRIPT = CScript([OP_TRUE, SCRIPT])


I think the previous wording was correct. Redeem script refers to the thing that is hashed (serialized), see also BIP 16:

serialized script - also referred to as the redeemScript

laanwj added the Tests label Sep 23, 2021

This was referenced Sep 23, 2021

Package-aware fee estimation #23074

Closed

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

Re-include RBF replacement txs in fee estimation #22539

Merged

DrahtBot added the Needs rebase label Oct 7, 2021

darosior force-pushed the fee_est_test_cleanup branch from c31068e to 35bc69b Compare October 15, 2021 10:55

DrahtBot removed the Needs rebase label Oct 15, 2021

DrahtBot mentioned this pull request Oct 18, 2021

test: Implicitly sync after generate*, unless opted out #23300

Merged

glozow reviewed Oct 26, 2021

View reviewed changes

DrahtBot added the Needs rebase label Nov 9, 2021

darosior force-pushed the fee_est_test_cleanup branch from 35bc69b to 4eb027e Compare November 15, 2021 00:02

DrahtBot removed the Needs rebase label Nov 15, 2021

DrahtBot mentioned this pull request Nov 15, 2021

test: scripted-diff cleanups after generate* changes #23474

Merged

DrahtBot added the Needs rebase label Nov 16, 2021

qa: remove a redundant condition in fee estimation test

19dd91a

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior force-pushed the fee_est_test_cleanup branch from 4eb027e to 0db0907 Compare December 9, 2021 12:27

DrahtBot removed the Needs rebase label Dec 9, 2021

glozow reviewed Dec 9, 2021

View reviewed changes

test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved

test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved

darosior added 4 commits December 9, 2021 14:59

qa: use a single p2sh script in fee estimation test

cc204b8

Using 2 different scripts is unnecessary complication Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

qa: pass scriptsig directly to txins constructor in fee estimation test

eae52dd

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

qa: don't mine non standard txs in fee estimation test

15f5fd6

We don't need dust outputs anymore. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior and others added 2 commits December 9, 2021 15:11

qa: fee estimation with RBF test cleanups

e502139

Followups to bitcoin#22539 Co-Authored-By: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

qa: replace assert with test framework assertion helpers in fee estim…

60ae116

…ation test

darosior force-pushed the fee_est_test_cleanup branch from 0db0907 to 60ae116 Compare December 9, 2021 14:31

DrahtBot mentioned this pull request Dec 24, 2021

Allow fee estimation to work with lower fees #13990

Closed

pg156 reviewed Dec 30, 2021

View reviewed changes

glozow reviewed Jan 4, 2022

View reviewed changes

maflcko changed the title ~~refactoring: Fee estimation functional test cleanups~~ test: Fee estimation functional test cleanups Jan 6, 2022

maflcko merged commit 799fd7a into bitcoin:master Jan 6, 2022

maflcko reviewed Jan 6, 2022

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 6, 2022

Merge bitcoin#23075: test: Fee estimation functional test cleanups

c30e11f

bitcoin locked and limited conversation to collaborators Jan 6, 2023

test: Fee estimation functional test cleanups #23075

test: Fee estimation functional test cleanups #23075

Uh oh!

Conversation

darosior commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Oct 4, 2021

Uh oh!

darosior commented Oct 15, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

darosior commented Nov 15, 2021

Uh oh!

DariusParvin commented Dec 1, 2021

Uh oh!

darosior commented Dec 9, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow commented Dec 9, 2021

Uh oh!

fanquake commented Dec 9, 2021

Uh oh!

darosior commented Dec 9, 2021

Uh oh!

pg156 left a comment

Choose a reason for hiding this comment

Uh oh!

pg156 Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

darosior Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

pg156 Dec 30, 2021

Choose a reason for hiding this comment

Uh oh!

darosior Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darosior commented Sep 23, 2021 •

edited

Loading

DrahtBot commented Sep 23, 2021 •

edited

Loading