-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
Execute sequenced certs serially in execution driver #5788
Conversation
@@ -1622,11 +1626,24 @@ impl AuthorityState { | |||
&self, |
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: rename this function to add_pending_certificates_from_gossip? Not sure where else this is called.
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.
it is called in authority_server as well - if we can't execute a cert received via handle_certificate, we enqueue it there for causal completion.
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.
Btw, why did we have to enqueue the failed certificate in authority server? Just to avoid lagging behind?
@@ -112,31 +113,67 @@ where | |||
true | |||
} | |||
}) | |||
.collect(); | |||
.partition(|(_, (is_sequenced, _))| *is_sequenced); | |||
|
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.
Could we add a debug logging here to print the number on each side of the partition?
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 idea
.into_iter() | ||
.filter(|(idx, digest)| { | ||
.filter(|(idx, (_, digest))| { |
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 think here we need to favor digests that are sequenced, if there is duplicate.
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.
correct, we first serially execute all sequenced digests.
66b0562
to
387c421
Compare
387c421
to
c5b6c51
Compare
* Execute sequenced certs serially in execution driver * Just use a bool to indicate whether sequenced * PR comments * Prioritize sequenced over unsequenced when there are duplicates
* Execute sequenced certs serially in execution driver * Just use a bool to indicate whether sequenced * PR comments * Prioritize sequenced over unsequenced when there are duplicates
Temporary work around (until the scheduler is finished) for degenerate behavior when there are many pending certs on the same object.