-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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.
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
test: remove unneeded -maxorphantx=1000
settings
#30133
test: remove unneeded -maxorphantx=1000
settings
#30133
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
It's unclear what the motivation for increasing the orphan pool is, and it seems that this not needed at all. None of these tests involve orphan transactions explicitly, and if they would occur occasionally, there is no good reason to prefer a value of 1000 over the default of 100 (see DEFAULT_MAX_ORPHAN_TRANSACTIONS).
75246ee
to
8950053
Compare
utACK 8950053 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 8950053
I did wonder whether maxorphantx
was zeroed or otherwise not the default 100
for functests which would perhaps explain it being set here, but I can't see anything to indicate that being the case.
[ | ||
"-maxorphantx=1000", | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
self.extra_args = [
[],
...
leaving
self.extra_args = [
[
],
...
is a bit weird, practically any linter/autoformatter would complain 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this as a problem per se. test/functional/feature_rbf.py already had the same pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, it's a stylistic nit so just an opinion, can be disregarded
Tested ACK 8950053 The changes don't break the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 8950053 From skimming the tests, it appears that none of these need a larger -maxorphantx
.
It looks like the extra -maxorphantx=1000
s have existed since each of the tests were introduced. My best guess is this was common when package limits were higher and/or tx relay and orphanage worked a bit differently, and then not cleaned up. For example, descendant packages of 1000 were allowed when mempool_packages.py was first added (along with package tracking): https://github.com/bitcoin/bitcoin/pull/6654/files#diff-4cee285ce55bc2b79f96cb2746cec9815129dd061a797d86ff4b84eda8ea0cdaR18.
feature_rbf.py: https://github.com/bitcoin/bitcoin/pull/6871/files#diff-181f65de910230cbedfa2da002451abab45b0c0baf7dd0fe4da56747a7ca07c8R75.
mempool_package_onemore.py: https://github.com/bitcoin/bitcoin/pull/15681/files#diff-e6c355d5d6411731bcc975121cae3a145896c8a0df5c210d32a6ba5fab4a104eR21
It's unclear what the motivation for increasing the orphan pool is here, and it seems that this not needed at all. None of these tests involve orphan transactions explicitly, and if they would occur occasionally, there is no good reason to prefer a value of 1000 over the default of 100 (see DEFAULT_MAX_ORPHAN_TRANSACTIONS).