-
Notifications
You must be signed in to change notification settings - Fork 641
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
[3 / 5] Move crypto checks in the approval-distribution #4928
Conversation
78708d7
to
406d11c
Compare
88e15da
to
7c968e1
Compare
406d11c
to
2281330
Compare
7c968e1
to
5fe747c
Compare
The CI pipeline was cancelled due to failure one of the required jobs. |
5fe747c
to
e4f883e
Compare
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
2281330
to
8dbb088
Compare
e4f883e
to
14727c5
Compare
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
…l-1-5' into alexaggh/approval-voting-parallel-3-5
- rename check_and_import in import. - refactor un-needed variable. Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
…oting-parallel-3-5
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.
Excellent work @alexggh 🚀 . Left some comments and suggestions.
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
…oting-parallel-3-5
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
PR ran in versi for a few days without any issues! |
…oting-parallel-3-5
Anything preventing us from merging it ? |
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.
This optimization makes sense and great work 🚀
Left a few minor remarks
The only thing missing now is a security audit, @patriciobcs any thoughts when this PR will get its turn ? |
…oting-parallel-3-5
Will be done by 13th September. |
…ting Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
…oting-parallel-3-5
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Prerequisite
This is part of the work to further optimize the approval subsystems, if you want to understand the full context start with reading #4849 (comment),
Description
This PR contain changes, so that the crypto checks are performed by the approval-distribution subsystem instead of the approval-voting one. The benefit for these, is twofold:
Approval-distribution won't have to wait every single time for the approval-voting to finish its job, so the work gets to be pipelined between approval-distribution and approval-voting.
By running in parallel multiple instances of approval-distribution as described here [5 / 5] Introduce approval-voting-parallel #4849 (comment), this significant body of work gets to run in parallel.
Changes:
ApprovalDistributionMessage::NewBlocks
it needs to pass the core_index and candidate_hash of the candidates.RuntimeInfo
to be able to fetch the SessionInfo from the runtime.approval-voting
logic that checks VRF assignment intoapproval-distribution
approval-voting
logic that checks vote is correctly signed intoapproval-distribution
approval-distribution
andapproval-voting
tests to support the new logic.Benefits
Even without parallelisation the gains are significant, for example on my machine if we run approval subsystem bench for 500 validators and 100 cores and trigger all 89 tranches of assignments and approvals, the system won't fall behind anymore because of late processing of messages.
TODO: