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: make p2p_handshake robust against timeoffset warnings #29704
test: make p2p_handshake robust against timeoffset warnings #29704
Conversation
The test requires that limited nodes are not peered with when the node's system time exceeds ~ 24h of the node's chaintip timestamp, as per PeerManagerImpl::GetDesirableServiceFlags. By patching this test to modify the timestamp of the chaintip as opposed to mocking the node's system time, we make it resilient to future commits where the node raises a warning if it detects its system time is too much out of sync with its outbound peers. See bitcoin#29623
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. |
lgtm ACK 032a597 A third alternative might be to restart the node after each test to drop the state, but that seems fragile. |
Please don't |
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 032a597
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.
crACK 032a597
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.
Code Review ACK 032a597
The new
p2p_handshake
test requires that limited nodes are not peered with when the node's system time exceeds ~ 24h of the node's chaintip timestamp, as perPeerManagerImpl::GetDesirableServiceFlags
.By patching this test to modify the timestamp of the chaintip as opposed to mocking the node's system time, we make it resilient to future commits where the node raises a warning if it detects its system time is too much out of sync with its outbound peers.
Resolves a silent merge conflict in #29623, that is changing the warning behaviour when significant time differences with outbound peers are detected, failing the test as it's currently in master.
Considerations/alternatives I've thought of:
self.setup_clean_chain = True
toself.set_test_params()
, to avoid creating a new tip with a (much) older date, but it doesn't seem to matter?setmocktime
altogether and instead usecreate_block
instead, but that seems like it'll be a lot more verbose and I don't think it's worth it?Big thanks to theStack for his time in discussing this with me offline.