Fix versionbits warning test #12264
Merged
Fix versionbits warning test #12264
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
maflcko
reviewed
Jan 24, 2018
Member
There was a problem hiding this comment.
Should say more than 50, not less than
maflcko
reviewed
Jan 24, 2018
Member
There was a problem hiding this comment.
nit: 60 seconds seems overkill, what about 5 or 10?
Contributor
Author
There was a problem hiding this comment.
no harm in having it longer
Member
There was a problem hiding this comment.
I sometimes make tests fail on purpose, so having to wait a minute (or going into the test each time and having to modify it) is somewhat inconvenient.
Member
There was a problem hiding this comment.
Also, should the test fail for some reason, the earlier you know the better. As a rule of thumb, I estimate the time it would take to fulfill the predicate, double that for travis, then double again just to be safe.
Contributor
Author
There was a problem hiding this comment.
You can change the timeout on your failure-triggering case 🙂
In my opinion it's better to have this too long than too short, to avoid spurious Travis failures.
Member
There was a problem hiding this comment.
I also prefer waiting on the long side, if it doesn't slow down the test in the passing case. Travis can be really slow when the servers are contended.
maflcko
reviewed
Jan 24, 2018
Member
There was a problem hiding this comment.
Generating that block will not help to get us out of ibd any sooner. Am I mistaken?
Contributor
Author
There was a problem hiding this comment.
generating a block guarantees that we'll get out of IBD. I've updated the comment.
Member
|
Concept ACK |
jnewbery
force-pushed
the
fix_versionbits_warning_test
branch
from
9100159 to
f36607e
Compare
Member
|
Seems to need rebase (already?). |
Member
|
utACK f36607eb8c Needs rebase |
Makes following changes to fix and tidy up p2p-versionbits-warning.py: - add node alias in the run() method - call versionbits_in_alert_file() in a wait_until loop. - don't clear out the alert.txt file - explicitly comment why the node needs to be stop-started - Verify that the node is out of IBD after stop-start (nodes in IBD do not generate alert messages) - no need to subclass P2PInterface
jnewbery
force-pushed
the
fix_versionbits_warning_test
branch
from
f36607e to
1e2e09e
Compare
Contributor
Author
|
rebased (test file name change) |
maflcko
pushed a commit
that referenced
this pull request
Jan 25, 2018
1e2e09e Fix intermittent failure in p2p-versionbits-warning.py (John Newbery) 3bbd843 Improve comments/logging in p2p-versionbits-warning.py (John Newbery) ef2beb2 Fix flake8 warnings in p2p-versionbits-warning.py (John Newbery) Pull request description: fixes #12259 (and tidies up the test) The problem was that the node was still in IBD at the point the last block was generated. UpdateTip() will not generate a warning if the node is still in IBD: https://github.com/bitcoin/bitcoin/blob/cc5870a4057f0322509dde5877fb08258bf4ec50/src/validation.cpp#L2151 The 'proper' fix would be to remove the overenthusiastic latching in DoWarning: https://github.com/bitcoin/bitcoin/blob/cc5870a4057f0322509dde5877fb08258bf4ec50/src/validation.cpp#L2135 so that more than one warning message can be output to `alertnotify`. Really we should suppress multiple messages of the same type, but allow messages to be output if they're for different warnings. That would mean the test wouldn't need to stop-start the node. Tree-SHA512: 5c9aa5af7eba3c1350ea28482d57d3d79e3166c6224ceddb5d5a631090081d890d7403015e41f413c22990959a488cf1231f88bb825c54a609b24f89c450a1f6
codablock
added a commit
to codablock/dash
that referenced
this pull request
Jan 2, 2020
UdjinM6
pushed a commit
to dashpay/dash
that referenced
this pull request
Jan 2, 2020
* Wait a little bit longer in wallet-encryption.py The timed wallet locking is happening by an asynchronous task internally, which means that it might need a little bit longer then the specified timeout. * Add workaround for p2p-versionbits-warnings.py until we backport bitcoin#12264
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
Jan 10, 2020
* Wait a little bit longer in wallet-encryption.py The timed wallet locking is happening by an asynchronous task internally, which means that it might need a little bit longer then the specified timeout. * Add workaround for p2p-versionbits-warnings.py until we backport bitcoin#12264
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 18, 2020
1e2e09e Fix intermittent failure in p2p-versionbits-warning.py (John Newbery) 3bbd843 Improve comments/logging in p2p-versionbits-warning.py (John Newbery) ef2beb2 Fix flake8 warnings in p2p-versionbits-warning.py (John Newbery) Pull request description: fixes bitcoin#12259 (and tidies up the test) The problem was that the node was still in IBD at the point the last block was generated. UpdateTip() will not generate a warning if the node is still in IBD: https://github.com/bitcoin/bitcoin/blob/cc5870a4057f0322509dde5877fb08258bf4ec50/src/validation.cpp#L2151 The 'proper' fix would be to remove the overenthusiastic latching in DoWarning: https://github.com/bitcoin/bitcoin/blob/cc5870a4057f0322509dde5877fb08258bf4ec50/src/validation.cpp#L2135 so that more than one warning message can be output to `alertnotify`. Really we should suppress multiple messages of the same type, but allow messages to be output if they're for different warnings. That would mean the test wouldn't need to stop-start the node. Tree-SHA512: 5c9aa5af7eba3c1350ea28482d57d3d79e3166c6224ceddb5d5a631090081d890d7403015e41f413c22990959a488cf1231f88bb825c54a609b24f89c450a1f6
FornaxA
pushed a commit
to ioncoincore/ion
that referenced
this pull request
Jul 6, 2020
* Wait a little bit longer in wallet-encryption.py The timed wallet locking is happening by an asynchronous task internally, which means that it might need a little bit longer then the specified timeout. * Add workaround for p2p-versionbits-warnings.py until we backport bitcoin#12264 Signed-off-by: cevap <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
fixes #12259 (and tidies up the test)
The problem was that the node was still in IBD at the point the last block was generated. UpdateTip() will not generate a warning if the node is still in IBD:
bitcoin/src/validation.cpp
Line 2151 in cc5870a
The 'proper' fix would be to remove the overenthusiastic latching in DoWarning:
bitcoin/src/validation.cpp
Line 2135 in cc5870a
so that more than one warning message can be output to
alertnotify. Really we should suppress multiple messages of the same type, but allow messages to be output if they're for different warnings. That would mean the test wouldn't need to stop-start the node.