-
Notifications
You must be signed in to change notification settings - Fork 30
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
[DEPENDENCY_TASK] Fix CDN Crash Tests #3390
Conversation
@@ -686,6 +686,7 @@ pub(crate) async fn fetch_proposal<TYPES: NodeType>( | |||
quorum_membership: Arc<TYPES::Membership>, | |||
consensus: Arc<RwLock<Consensus<TYPES>>>, | |||
) -> Result<Leaf<TYPES>> { | |||
tracing::info!("Fetchign proposal for view {:?}", view); |
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.
nit: maybe we should consider logs like these being debug
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.
My thinking was that this should be a relatively rare occurrence. We should only get this if we miss a proposal and are catching back up but I'm not 100%
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.
I’m getting that Friday feeling!!! So glad these are done…
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.
Looks good to me, just some minor comments
@@ -253,6 +254,8 @@ struct DelayedRequester<TYPES: NodeType, I: NodeImplementation<TYPES>> { | |||
pub network: Arc<I::Network>, | |||
/// Shared state to check if the data go populated | |||
state: Arc<RwLock<Consensus<TYPES>>>, |
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.
Nit: not really related to the PR, but could this be called consensus
rather than state
for consistency with the other tasks?
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.
Yeah it should be I want to follow up on this pr with one that handles the VID stuff a little smarter
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.
Sounds good! reapproved
Co-authored-by: ss-es <[email protected]>
Co-authored-by: ss-es <[email protected]>
Co-authored-by: ss-es <[email protected]>
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.
Looks great!
Closes #3368
This PR:
Solves the final tests failures for the dependency task refactor. The failure was caused because for some code paths we can calculate the VID Shares, store them in the vid share map but not emit a
VidShareRecv
event. This only happens when running with the combined network. There are 2 places we could calculate the VID shares from a DAProposal. The first is in the DA task if the primary is down. The second is if we get a request from another peer for a VID share we'll try to calculate it if we don't have it but do have a DA Proposal. This led to some nodes not voting when they should be able to because the dependency task was waiting for a VIDShareRecv event that wouldn't come.Typically what I was seeing in CI was that 6/10 nodes would vote and 4 would not. I think the reasoning for this is that the VID calculations were triggered by the requests for VID. What happens was the first nodes to request VID get back a share but the later nodes to request it are already calculating the VID due to the earlier requesters and don't try to request until the calculation finishes and when it does the request task cancels itself.
The sequence that could cause the bug, from the perspective of one were:
In this example the VID calculation could also be triggered if primary network was known down by the node.
The fix for this is to just broadcast that we have the share if we end up cancelling the VID Task because we have the share. This means there is no way for the vote dependency to not get the event if we got the share or calculated it.
This PR does not:
Key places to review: