Skip to content
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

Merged
merged 34 commits into from
Aug 16, 2024
Merged

Conversation

pompon0
Copy link
Collaborator

@pompon0 pompon0 commented Aug 2, 2024

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.

@brunoffranca
Copy link
Member

Couple of notes:

  1. If possible, I would prefer to have the attester code into its own actor. Otherwise it will be harder to understand. I also don't want the network actor to keep growing in size until it basically encapsulates all the code.
  2. We can have yet another trait (Client) to be implemented on era. But we already have 3 (PayloadManager, PersistentBatchStore, PersistentBlockStore) and I want us to merge all these traits into one. The consensus repo really should have a single interface to interact with era. This will be better discussed on the offsite, but just keep in mind that you might not want to create more traits like this.

@pompon0 pompon0 changed the title Gprusak dynamic attesters Support for dynamic attester committee Aug 12, 2024
@pompon0 pompon0 requested a review from aakoshh August 12, 2024 11:21
@pompon0 pompon0 marked this pull request as ready for review August 12, 2024 11:22
Copy link
Contributor

@aakoshh aakoshh left a 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.

node/actors/executor/src/attestation.rs Outdated Show resolved Hide resolved
node/actors/executor/src/tests.rs Outdated Show resolved Hide resolved
node/actors/executor/src/attestation.rs Outdated Show resolved Hide resolved
node/actors/network/src/gossip/attestation/mod.rs Outdated Show resolved Hide resolved
node/actors/network/src/gossip/attestation/mod.rs Outdated Show resolved Hide resolved
node/libs/storage/src/batch_store/mod.rs Show resolved Hide resolved
node/libs/storage/src/batch_store/mod.rs Show resolved Hide resolved
node/actors/network/src/lib.rs Show resolved Hide resolved
node/actors/executor/src/attestation.rs Outdated Show resolved Hide resolved
node/actors/executor/src/attestation.rs Outdated Show resolved Hide resolved
node/tools/src/config.rs Outdated Show resolved Hide resolved
Copy link

@RomanBrodetski RomanBrodetski left a 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

node/actors/network/src/gossip/attestation/mod.rs Outdated Show resolved Hide resolved
node/actors/network/src/gossip/attestation/mod.rs Outdated Show resolved Hide resolved
node/actors/network/src/gossip/attestation/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 👍

@pompon0 pompon0 merged commit b9c5fa7 into main Aug 16, 2024
5 checks passed
@pompon0 pompon0 deleted the gprusak-dynamic-attesters branch August 16, 2024 06:52
github-merge-queue bot pushed a commit to matter-labs/zksync-era that referenced this pull request Aug 16, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants