-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[dag][bugfix] fix bitmask for incomplete first round #9723
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
@@ -272,6 +272,10 @@ impl Dag { | |||
} | |||
|
|||
pub fn lowest_incomplete_round(&self) -> Option<Round> { | |||
if self.nodes_by_round.is_empty() { | |||
return Some(self.lowest_round()); | |||
} |
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: if the DAG is empty you return Some(lowest_round), but if the DAG is full you return None. The question is why empty DAG is not considered full. In the sense that all rounds either have all nodes or none?
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.
Good point, removed the option and now I return the actual round that is incomplete. For a full dag, its highest_round+1, and an empty dag, its lowest_round.
178b9f5
to
2bcdbd4
Compare
2bcdbd4
to
bc4d9ee
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
This PR fixes the bitmask fn in the dag_store to make it return the proper bitmask when starting from an empty DAG. Previously, it was only properly handling the case when the initial round of the DAG store is 0, which doesn't accommodate for when starting a DAG store without the whole prefix.
Test Plan
Fixed unit tests.