Skip to content
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

Fix timeout bug in Genesis “Time limited leashing attack” test #1350

Merged
merged 7 commits into from
Dec 21, 2024

Conversation

Niols
Copy link
Contributor

@Niols Niols commented Dec 19, 2024

See #1179 for context.

Note 1: I am currently running extensive tests to check that this fix looks good indeed. I wouldn't want to reintroduce an other extremely rare bug! I will mark this PR as ready once I am happy with the result, hopefully tomorrow 🤞

Note 2: There is a big more than just the fix, because I thought that some of my debugging work could be kept for posterity. Feel free to tell me to discard some bits and pieces, the commits are very independent for a reason.

@Niols Niols added the Genesis PRs related to Genesis testing and implementation label Dec 19, 2024
Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

- Always disable `mustReplyTimeout`; explain why
- Always disable `idleTimeout`; explain why
- Keep the others by default in all the tests

This should fix the bug discussed in #1179
@Niols Niols force-pushed the blockfetch/milestone-1-with-fix branch from 9c8a5d7 to 4e56180 Compare December 19, 2024 11:41
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for hunting this down so quickly.

I think the "extra" stuff seems useful, so I agree it's worthwhile to retain.

I made one suggestion for enriching one comment.

Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>
@Niols
Copy link
Contributor Author

Niols commented Dec 21, 2024

Ran 2M tests, all green; previously, this would have had a very high probability to trigger the bug, so I have very strong confidence that this was the right fix. Will merge this.

@Niols Niols marked this pull request as ready for review December 21, 2024 08:37
@Niols Niols merged commit a36fd87 into blockfetch/milestone-1 Dec 21, 2024
8 checks passed
@Niols Niols deleted the blockfetch/milestone-1-with-fix branch December 21, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Genesis PRs related to Genesis testing and implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants