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

[HOTFIX] Poll forever for latest data #2473

Merged
merged 8 commits into from
Jan 29, 2024
Merged

[HOTFIX] Poll forever for latest data #2473

merged 8 commits into from
Jan 29, 2024

Conversation

rob-maron
Copy link
Collaborator

This PR:

  • Changes terminology from view sync proposal -> view sync cert everywhere to be more consistent

For view sync certificates and quorum proposals:

  • Refactors so that we always poll for the latest data
  • Starts each "poll for latest" task at the beginning of consensus
  • Adds an additional wait to specific polls

This PR does not:

Fix the relay number check.

Key places to review:

web_server_network.rs

@rob-maron rob-maron requested a review from shenkeyao January 28, 2024 18:57
@rob-maron rob-maron changed the title [HOTFIX[ Poll forever for latest data [HOTFIX] Poll forever for latest data Jan 28, 2024
@@ -325,7 +332,7 @@ impl<TYPES: NodeType> Inner<TYPES> {

return Ok(());
Copy link
Collaborator

@bfish713 bfish713 Jan 28, 2024

Choose a reason for hiding this comment

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

If we don't return here we could get away without the outer loop I mentioned in my other comment. I also think we want to use an LruCache for deduplication here just like in the LatestViewSyncCertificate case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we also consider actually having the cache a layer up? This way it can be shared for every task instead of just per task. The deduplication in this case was just so we don't pull multiple of the same latest proposal

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand what you mean by per task. If you mean the task we spawn to poll the webserver then yeah we should probably do a shared cache between the view number based and latest polls. On the other hand we could probably just get rid of the view based polling for proposals after this. I say we keep it as is for now

Copy link
Collaborator Author

@rob-maron rob-maron Jan 29, 2024

Choose a reason for hiding this comment

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

Yeah, the cache is intra-task and not inter-task. I'll leave it for now

@jbearer
Copy link
Member

jbearer commented Jan 28, 2024

@rob-maron can you do this for release/0.5.5 also?

@rob-maron rob-maron requested a review from bfish713 January 28, 2024 20:59
bfish713
bfish713 previously approved these changes Jan 29, 2024
Copy link
Collaborator

@bfish713 bfish713 left a comment

Choose a reason for hiding this comment

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

Fixes look good to me, lets get this into the release branch as well

@@ -33,21 +33,21 @@ struct WebServerState<KEY> {
/// view number -> (secret, proposal)
proposals: HashMap<u64, (String, Vec<u8>)>,
/// for view sync: view number -> (relay, certificate)
view_sync_proposals: HashMap<u64, Vec<(u64, Vec<u8>)>>,
view_sync_certificates: HashMap<u64, Vec<(u64, Vec<u8>)>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Since you are in I think it makes sense to use BTreeMaps for these so we don't have to keep a variable for oldest and newest proposal/certificate

@rob-maron rob-maron merged commit c953c1e into main Jan 29, 2024
12 of 13 checks passed
@rob-maron rob-maron deleted the rm/forever-poll branch January 29, 2024 03:09
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.

3 participants