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
fuzz: txorphan tests fixups #29974
fuzz: txorphan tests fixups #29974
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. |
4414b36
to
6998f02
Compare
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 6998f02
ec29dc4
to
0ab279e
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
Adds the following fixups in txorphan fuzz tests: - Don't bond the output count of the created orphans based on the number of available coins - Allow duplicate inputs, when applicable, but don't store duplicate outpoints Rationale --------- The way the test is currently written, duplicate inputs are allowed based on a random flag (`duplicate_input`). If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection, and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not. This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop. Additionally, both the input and output count of the transaction and bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.
0ab279e
to
58594c7
Compare
Updated the approach to get rid of |
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 58594c7
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 58594c7
ran fuzz for a while, no issues
@maflcko would you mind taking a look at this (given you reviewed the original PR for this fuzz test)? |
IIRC this was done to start with a few outputs in the beginning, and increase the number of outputs as the size of the fuzz input increases. The hope is that the runtime is linear with the size of the fuzz input, and that minimizing a potential crash results in a transaction with less outputs. If you allow 256 in the first iteration, the minimizer may spit out a transaction with that many outputs. However, I haven't benchmarked this. Should be fine either way. utACK 58594c7 |
Motivated by #28970 (comment)
Adds the following fixups in txorphan fuzz tests:
Most significantly, this gets rid of the
duplicate_input
flag altogether, making the test easier to reason about. Notice how, under normal conditions, duplicate inputs would be caught byMemPoolAccept::PreChecks
, however, no validations checks are run on the test before adding data to the orphanage (neither were they before this patch)Rationale
The way the test is currently written, duplicate inputs are allowed based on a random flag (
duplicate_input
). If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not. This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop.Additionally, both the input and output count of the transaction are bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.