test: Remove unnecessary mining from importdescriptors test #25476
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Credit to @w0xlt for noticing something was off there |
|
Code review ACK e3d8d72
I think the PR title undersells it. The mining is not only unnecessary, but can actively cause failures. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 27, 2022
…iptors test e3d8d72 test: Remove unnecessary block mining from importdescriptors test (Fabian Jahr) Pull request description: This removes generation of 6 blocks and replaces is with a `sync_all` in the `importdescriptors` test. The generated blocks themself don't seem to serve any purpose in the test. Instead they could make the test flaky (although I did not find open issues pointing to this happening in practice in the CI). Right before the blocks being generated a transaction is created (L454) and later in the test this tx is assumed to be still in the mempool. If the nodes were to sync their mempools before the blocks are generated, the test fails. It currently only seems to work because one node sends the tx while the other generates the blocks and the mempools are not synced fast enough. The `sync_all` is still needed to let nodes catch up at that point. Otherwise races happen further below which the generate call seems to have prevented so far. ACKs for top commit: laanwj: Code review ACK e3d8d72 Tree-SHA512: 14f3dc2938d779d1ad43e09a7e046523fc3c92f41df012833f279a2e88e74c2fcab301fe4f3fcc038bd8460ea1360725a8d1eb5b59acd1039495bacb484fd790
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes generation of 6 blocks and replaces is with a
sync_allin theimportdescriptorstest.The generated blocks themself don't seem to serve any purpose in the test. Instead they could make the test flaky (although I did not find open issues pointing to this happening in practice in the CI). Right before the blocks being generated a transaction is created (L454) and later in the test this tx is assumed to be still in the mempool. If the nodes were to sync their mempools before the blocks are generated, the test fails. It currently only seems to work because one node sends the tx while the other generates the blocks and the mempools are not synced fast enough.
The
sync_allis still needed to let nodes catch up at that point. Otherwise races happen further below which the generate call seems to have prevented so far.