-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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: fix intermittent failure in rpc_setban.py --v2transport
, run it in CI
#29372
test: fix intermittent failure in rpc_setban.py --v2transport
, run it in CI
#29372
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. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
When initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later connect_nodes call. Also add the test with --v2transport to the test runner.
8538c04
to
cc87ee4
Compare
rpc_setban.py --v2transport
, run it in CIrpc_setban.py --v2transport
, run it in CI
tested ACK cc87ee4 |
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.
tested ACK cc87ee4. nice find!
I don't think this is something to stall a merge, but I could not make the test fail in master. I tried with different loads in my machine from other work. How are you calling it?
|
You have to call it with v2, see PR title: |
I'm sorry for not being clear in my comment. My point is that running Yet, I don't see this as a problem to merge.
|
Concept ACK cc87ee4. |
ACK cc87ee4 |
This test failed for me on master locally:
The reason is that when initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later
connect_nodes
call.Also add the test with
--v2transport
to the test runner because banning with v2 seems like a useful thing to have test coverage for.