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

[Stability] Poll for latest view sync cert #2246

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Conversation

rob-maron
Copy link
Collaborator

@rob-maron rob-maron commented Dec 15, 2023

Closes #2245

This PR:

Adds polling for the latest view sync cert.

Some creative liberties to note:

  • Since we don't want to miss any certs (and we can have multiple proposals/certs per view), the "latest view sync (proposal/cert)" function on the webserver just returns all certs for the latest view. We could do server side filtering but if we expect these to be sufficiently small and for view sync to be sufficiently infrequent, then I think this is good until our larger CDN changes.

Everything else is about the same as the "PollForRecentProposal" things from @bfish713.

This PR does not:

Change or update the view sync replicas or tasks, only the networking.

Key places to review:

Everything in view_sync.rs. To me it is, but I want to be sure the logic to start/stop the poll is sufficient.

I have run a bunch of manual view sync tests to make sure that we start/stop this polling when necessary, and also that unique certs it pulls are processed properly. I'm not sure if there are any more advanced tests I should run, please let me know!

@rob-maron rob-maron marked this pull request as ready for review December 16, 2023 14:25
elliedavidson
elliedavidson previously approved these changes Dec 18, 2023
Copy link
Member

@elliedavidson elliedavidson left a comment

Choose a reason for hiding this comment

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

In the "viewchange" event handler in view_sync.rs I think we need to add a "ConsensusIntentEvent::CancelPollForLatestViewSyncProposal". It is possible that a node will go into view sync, but then cancel view sync because they've seen a proposal with a higher view. In this case I don't think we'll properly cancel polling for the latest view sync certificate.

But, since it looks like we GC view sync certs properly, I don't think it will cause a memory leak - probably just more network traffic. So I'm going to approve this PR for now.

@rob-maron rob-maron merged commit 251a3e6 into main Dec 18, 2023
7 of 8 checks passed
@rob-maron rob-maron deleted the rm/poll-recent-vs-cert branch December 18, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Stability] - Add polling for latest view sync cert
2 participants