-
Notifications
You must be signed in to change notification settings - Fork 39
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
Support for dynamic attester committee #175
Conversation
Couple of notes:
|
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.
Big change!
It got me confused that all the persistence side and the querying of the batch store for the next thing to attest for is gone, and the PR description doesn't mention it at all. I suspect the reason is that you want to only call wait_for_qc()
from zksync-era
and push all the code over to that side which retrieves the data from the DB and saves the result.
It's true that some of the methods were (apart from maintaining metrics) merely passing the data through, and it looks like we just want to take care of the gossiping of the data now, and forget the interface described here, but one could also argue that so far the interface on the zksync-era
side was relatively simple: implement the persistent store, start the runners, and you're done, now there will have to be more interaction about which method to call to update, which method to poll for changes, etc. Is there some general guideline in you mind about where to implement stuff when we have a choice? I guess we can discuss more on the offsite too.
Co-authored-by: Akosh Farkash <[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.
Some preliminary comments - diving deeper to the actual logic now
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.
Nice one 👍
The new algorithm (matter-labs/era-consensus#175) supports dynamic attestation committee. This pr does NOT implement committee rotation, it will be done once the consensus registry contract is merged. ENs no longer store the certificates. I've also implemented a test of the attestation logic.
It will support collecting votes for a single batch for now. I've extended the rpc to support both pushing and pulling the votes (with dynamic committee we cannot really collect future votes). I've reworked the API to concentrate the api surface in one place.
I've also moved the rpc registry from rust code to protobuf enum to make enforcing compatibility easier.
I'll prepare a corresponding PR in zksync-era before merging to make sure that the integration is smooth e2e.