Skip to content
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

Jump to bottom

test: use MiniWallet for simple doublespend sub-test in feature_rbf.py #22330

Merged
merged 3 commits into from Jul 30, 2021
Merged

test: use MiniWallet for simple doublespend sub-test in feature_rbf.py #22330

merged 3 commits into from Jul 30, 2021

Conversation

theStack
Copy link
Contributor

This PR's goal is to prepare the functional test feature_rbf.py for more MiniWallet usage. It first gets rid of unused initialization code (I guess that was needed at times when the nodes were still in IBD at the start of tests?), then makes the MiniWallet instance introduced in #22210 available for all sub-tests, and finally, uses that instance in the first sub-test test_simple_doublespend.

Note that the same idea of replacing the make_utxo calls with MiniWallet can be also applied to other sub-tests too; this just serves as a first proof-of-concept.

@DrahtBot DrahtBot added the Tests label Jun 23, 2021
@brunoerg
Copy link
Contributor

Concept ACK

@mjdietzx
Copy link
Contributor

ACK c582546

@maflcko
Copy link
Member

maflcko commented Jul 15, 2021

Needs rebase

@theStack theStack force-pushed the 202106-test-feature_rbf_use_miniwallet_for_doublespend branch from c582546 to 9f2a53d Compare July 15, 2021 20:19
@theStack
Copy link
Contributor Author

Needs rebase

Done. Took the new sub-test test_replacement_relay_fee() into account for the second commit.

ShubhamPalriwala
ShubhamPalriwala suggested changes Jul 21, 2021
View reviewed changes
test/functional/feature_rbf.py Outdated Show resolved Hide resolved
sriramdvt
sriramdvt approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@sriramdvt sriramdvt left a comment

Choose a reason for hiding this comment

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

tACK 9f2a53d.
nit: I agree with https://github.com/bitcoin/bitcoin/pull/22330/files#r674195638 that it is not necessary to scan 3 blocks

@theStack theStack force-pushed the 202106-test-feature_rbf_use_miniwallet_for_doublespend branch from 9f2a53d to 48e5b1f Compare July 24, 2021 16:11
@theStack
Copy link
Contributor Author

@ShubhamPalriwala @sriramdvt: Thanks for your reviews! You are right, in total we only need two blocks to scan -- in the second commit one UTXO in MiniWallet to start with is enough (both sub-tests test_no_inherited_signaling and test_replacement_relay_fee consume one UTXO, but also create one, due to calls to send_self_transfer), in the latest commit, we have to increase it to two, however (the sub-test test_simple_doublespend only consumes a UTXO, but doesn't create a new one).

@practicalswift
Copy link
Contributor

Concept ACK

josibake
josibake approved these changes Jul 26, 2021
View reviewed changes
Copy link
Member

@josibake josibake left a comment

Choose a reason for hiding this comment

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

tACK 48e5b1f

I ran the tests of master and then off the PR to confirm everything still passes as expected. Left some optional feedback in the comments.

test/functional/feature_rbf.py Outdated Show resolved Hide resolved
test/functional/feature_rbf.py
# Ensure nodes are synced
self.sync_all()
self.wallet = MiniWallet(self.nodes[0])
self.wallet.scan_blocks(start=76, num=2)
Copy link
Member

Choose a reason for hiding this comment

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

Initially, I was confused by these parameters and noted that the test would fail if I picked a start less than 70, but would run for starts greater than 76. I dug through the comments and found the explanation on a previous PR, but considering this test is meant to be a proof of concept to motivate re-writing all the tests to use miniwallet, it might be worth adding a small comment explaining why 76 and 2 are chosen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, extended the second commit with an explanation about why we start at block 76, with reference to the test framework's method where the chain is pre-mined.

hg333
hg333 approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@hg333 hg333 left a comment

Choose a reason for hiding this comment

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

tACK 48e5b1f

@ShubhamPalriwala
Copy link
Contributor

tested and works fine on Ubuntu 21.04 48e5b1f

However, I agree with @josibake here:

Initially, I was confused by these parameters and noted that the test would fail if I picked a start less than 70, but would run for starts greater than 76. I dug through the comments and found the explanation on a previous PR, but considering this test is meant to be a proof of concept to motivate re-writing all the tests to use miniwallet, it might be worth adding a small comment explaining why 76 and 2 are chosen.

Adding a comment explaining the 2 blocks would help to understand the code even further

@theStack
test: feature_rbf.py: make MiniWallet instance available for all sub-…
a3f6397 
…tests

also document on why we start scanning blocks at height 76
@theStack theStack force-pushed the 202106-test-feature_rbf_use_miniwallet_for_doublespend branch from 48e5b1f to c1c0768 Compare July 27, 2021 21:18
@theStack
Copy link
Contributor Author

Force-pushed with changes suggested by josibake (#22330 (comment), #22330 (comment)). Thanks for the valuable reviews so far :)

@DrahtBot DrahtBot mentioned this pull request Jul 28, 2021
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 28, 2021 •
edited

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

Conflicts

No conflicts as of last run.

@josibake
Copy link
Member

ACK c1c0768

thanks for adding the comment! it will make it much easier for others to followup and refactor the remaining tests to use miniwallet

maflcko
maflcko reviewed Jul 28, 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.

ACK c1c0768 🌴

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK c1c0768b619ab463a00052d0a584fba191eb02a9 🌴
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi2Fwv/ZxETYQ3y9g77WwtNwVdqsHBrnHUaGAdjmEL1sF9HoCIwN1Ag92X0cVZV
0thS0WL81c/c7jWDG7H4XGAMfA254XvIWsA8vOjrx48YmbFxUZU0BNbtonT7iY04
arLwF4NsAg++ZmZni+Ofqyt90moqnlNLdtUvU8IQjm3fqZ2mu+SMM4WVxuD9rP8J
cxoAq7R5B+QuLKpL+S6fvbZHHrtn0aAIv8C58kDMJDvQasrsI1Pn9gzmPmu0Kg5T
/zbLextjgQcQwMU3IBOCYkiENU1akjxJ9OMF5F6rsdILJ52hbqpAHJP3QxmgUXUB
83smxjrBzaLf3sBBvSbiUplfDpGdjhPIoGcs2rV1wPPJAHtaLmTk4LHMwsTI46S8
ulDmutPrXKOdzXTTg0MhvRksve+5sXOq+4HnEke4qbGp0ij2qabYwFJ0rOdZ7gM7
Vpbkufy2U4fSrOO+amRCR0/JTznFIOyymij/UWhfC0G9p/XRSGvKVFIr0XA4qoJS
IvWqeNPN
=LPRU
-----END PGP SIGNATURE-----

Timestamp of file with hash 86af1901202bc55a0ba833a2df907819403554b0cc025a2c2c40822d8fff1ade -

test/functional/feature_rbf.py Outdated Show resolved Hide resolved
test/functional/feature_rbf.py Outdated Show resolved Hide resolved
@theStack theStack force-pushed the 202106-test-feature_rbf_use_miniwallet_for_doublespend branch from c1c0768 to aa02c64 Compare July 28, 2021 16:16
@theStack
Copy link
Contributor Author

Force-pushed with simplification suggestions by MarcoFalke (#22330 (comment), #22330 (comment)) in the last commit.

@maflcko
Copy link
Member

maflcko commented Jul 30, 2021

re-ACK aa02c64 🌯

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK aa02c64540cd77199c2834549786b9bc91fd4bc9 🌯
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjpsAwAqmy0sSSxypNFh60782o9jMhgNWj8IOLk5Ba1qVoM4aYgm1jCnfsomCIU
hIYihGM/NajSzN9tq0nzpHILs3Tb3TY/GTwMeuh8+Uab8R4oUCYC6X+S/SxG13Oz
w5IaM3XbewO6MAdSFJq/nu0poCW8IfO4/fqxuVAZzDXAhgGlIldsu9+F9WPM03G1
y4K29E+Udff1dDowwtzu+EcndG0dXfpvctJyWXPhslOI2fLeyLK/6Ya6v0Sp3D5t
18444yWuSDmtRP/AjN0z1zOqzxN5D8m8JQP9+fuaG+/BvO4+F8v/abLGVaiZyV5h
4bt8x22GejTonIr/XmEM0icBLxrGAFOzdLVb50g35zV8yijtrbHKnyKn0oLJv6l3
FyoqUCLm4tczdXBUyFjzNGRpB3SlDGogn1ocJXNXrsehSSCqWY1cBE8ecfes9TsX
NJvmdyzQFjvoaRVuugWTHn0Gr2zizUMPrJHtVLx4RGgGvTzTnMwIzmuyNoFEtwBb
wzDGQOTL
=58KE
-----END PGP SIGNATURE-----

Timestamp of file with hash 2623a0532a45417a171e97e69a61a4be006bec6bb4efa264a467e8da9aa7a356 -

@maflcko maflcko merged commit da1c0c6 into bitcoin:master Jul 30, 2021
@theStack theStack deleted the 202106-test-feature_rbf_use_miniwallet_for_doublespend branch July 31, 2021 20:05
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 1, 2021
Merge bitcoin#22330: test: use MiniWallet for simple doublespend sub-…
4bc4415 
…test in…
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko left review comments

@ShubhamPalriwala ShubhamPalriwala ShubhamPalriwala requested changes

@josibake josibake josibake approved these changes

@sriramdvt sriramdvt sriramdvt approved these changes

@hg333 hg333 hg333 approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@theStack @brunoerg @mjdietzx @maflcko @practicalswift @ShubhamPalriwala @DrahtBot @josibake @sriramdvt @hg333