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

Broadcast validators signature and collect QC (BFT-414) #76

Merged
merged 88 commits into from
May 21, 2024

Conversation

IAvecilla
Copy link
Contributor

What ❔

Implement a mechanism for validators to broadcast their signatures to other nodes in the network and collect them in a new certificate.

Why ❔

This is essential for signing L1 batches and sending them to L1 for verification. Validators need to broadcast their signatures and gather them in a certificate in case the majority signs the new batch.

@IAvecilla IAvecilla changed the title Broadcast validators signature and collect QC [BFT-414] Broadcast validators signature and collect QC (BFT-414) Mar 14, 2024
@lferrigno lferrigno marked this pull request as ready for review March 22, 2024 21:13
@lferrigno lferrigno requested a review from pompon0 as a code owner March 22, 2024 21:13
@IAvecilla IAvecilla marked this pull request as draft March 22, 2024 22:45
@brunoffranca
Copy link
Member

@IAvecilla I think it will be better if we create another validator key just for batch signing. So, this way we can use different signatures schemes for block signing and for batch signing.

pompon0 and others added 12 commits April 10, 2024 17:45
Statekeeper is storing blocks to postgres in parallel to processing next
blocks. Our simplified API so far required persisting one block to
finish before processing the next block. This effectively disabled the
parallelization in statekeeper and therefore the EN with consensus
enabled was significantly slower than EN without consensus. This PR
updates the api to allow persisting multiple blocks in parallel. Long
term this change most likely won't be that useful, because statekeeper
of a validator node will have to work synchronously with consensus (but
that requires improving statekeeper anyway).

I've also implemented caching inmemory the most recent blocks (even if
they got persisted) to address the inefficiency found during the
loadtest.
@IAvecilla IAvecilla requested a review from pompon0 May 16, 2024 16:20
@pompon0
Copy link
Collaborator

pompon0 commented May 17, 2024

remove -Dwarnings like here https://github.com/matter-labs/era-consensus/pull/114/files to fix the presubmit

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.

I am sending some comments to ask some conceptual questions and to help me understand the code. Sorry to add more to the long comment history, I hope you don't mind.

As a general note:

  • I see some TODOs left in the code, like this one. It would be helpful to state whether this PR completely implements what it says in the description, or there are things deferred to future PRs (perhaps with their ticket number).
  • Perhaps this PR could have been delivered in two parts: the attestations model first, and then hooking them into the flow.

node/actors/network/src/gossip/mod.rs Show resolved Hide resolved
node/actors/network/src/gossip/mod.rs Show resolved Hide resolved
node/actors/network/src/lib.rs Show resolved Hide resolved
@IAvecilla
Copy link
Contributor Author

@aakoshh Most of your comments are valid and correct. However, the current logic won't work as intended because we don't have the syncing batches logic in place yet (which is in progress). This means that much of this code will change in the future once we can test it properly. This was just an initial implementation to see how the signature propagation would look.

I think @pompon0 mentioned something similar while I was writing this. :)

@IAvecilla IAvecilla merged commit 3f5b4f6 into main May 21, 2024
5 checks passed
@IAvecilla IAvecilla deleted the signature_broadcast branch May 21, 2024 13:17
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