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: ackHandler order responses under edge cases #637

Merged
merged 5 commits into from
Feb 14, 2025
Merged

Conversation

AlfredoG87
Copy link
Contributor

@AlfredoG87 AlfredoG87 commented Feb 11, 2025

Reviewer Notes

This PR adds test that helped replicate edge cases when acks were sent out of order.
either due to the workaround to determine the first valid block (where to start sending acks) or
due to random delays occur in either persistence or verification threads.

Also this PR includes the fixes so those tests always pass instead of always failing or sometimes failing.

Fixes #638

Before:
image

After: Under the same conditions, same result in multiple passes.
image

Related Issue(s)

@AlfredoG87 AlfredoG87 self-assigned this Feb 11, 2025
@AlfredoG87 AlfredoG87 added this to the 0.4.0 milestone Feb 11, 2025
@AlfredoG87 AlfredoG87 added Block Node Issues/PR related to the Block Node. Bug A error that causes the feature to behave differently than what was expected based on design docs labels Feb 11, 2025
@AlfredoG87 AlfredoG87 marked this pull request as ready for review February 11, 2025 22:58
@AlfredoG87 AlfredoG87 requested a review from a team as a code owner February 11, 2025 22:58
@AlfredoG87 AlfredoG87 modified the milestones: 0.4.0, 0.6.0 Feb 13, 2025
@AlfredoG87 AlfredoG87 requested a review from ata-nas February 13, 2025 13:50
Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

It looks like the discussions were resolved. Looks good Fredy

Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG
Rebase to resolve conflict and todo comment

…ner and in the UTs.

With these fixes we aim at fix them

Signed-off-by: Alfredo Gutierrez <[email protected]>
… with random delays and with no delays.

Signed-off-by: Alfredo Gutierrez <[email protected]>
…xt block as the last thing in the if block

Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87 AlfredoG87 merged commit 60cb247 into main Feb 14, 2025
19 checks passed
@AlfredoG87 AlfredoG87 deleted the fix-ack-logic branch February 14, 2025 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Node Issues/PR related to the Block Node. Bug A error that causes the feature to behave differently than what was expected based on design docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Under certain edge conditions Acks can be sent out of order.
4 participants