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

feat: Batch vote publish loop (BFT-474) #148

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Jul 5, 2024

What ❔

  • Add BatchStore::unsigned_batch_numbers to query BatchNumbers that still need a QC
  • Add BatchStore::batch_to_sign to fetch the Batch with the correct BatchHash that needs to be signed
  • Add PersistentBatchStore::unsigned_batch_numbers to implement the above
  • Add PersistentBatchStore::batch_to_sign to implement the above
  • Add AttesterRunner to periodically query unsigned_batch_numbers, retrieve the data with batch_to_sign and publish the Signed<Batch> to the gossip network.
  • Run the AttesterRunner in Executor if the node has an Attester key.
  • Add a minimum batch number to sign to prevent repeated signing

The code will re-submit the node's signature after a restart to a number of batches to be determined by the database query. We can say we'll re-publish the top 10 unsigned batches, or we can say we'll only look for the ones which follow the last finalised batch observed the Main node. It is enough to publish a vote once because of the way BatchVotes works (gossip peers will pull the data they need when they connect). Because we'll always have the batches in order, we can take note of the highest number we have published a vote for and never publish it again (until a restart), to save ourselves the trouble of repeatedly retrieving and signing the payload. That said it is assumed that batch_to_sign is cheap as it just needs to look up a hash in the DB.

Follow up steps

The ticket will need a counterpart in zksync-era to implement the new PersistentBatchStore operations by querying Postgres. Need figuring out which column needs to go into the hash.

Why ❔

To publish the attestations over L1 batches to the gossip network, so that a QC can be formed in #145

@aakoshh aakoshh marked this pull request as ready for review July 8, 2024 09:50
@aakoshh aakoshh changed the title BFT-474: Batch vote publish loop feat: Batch vote publish loop (BFT-474) Jul 8, 2024
Base automatically changed from bft-477-save-batch-qc to main July 8, 2024 16:46
@brunoffranca brunoffranca requested a review from pompon0 as a code owner July 8, 2024 16:46
Copy link
Member

@brunoffranca brunoffranca left a comment

Choose a reason for hiding this comment

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

Approved!

node/actors/executor/src/attestation.rs Outdated Show resolved Hide resolved
@aakoshh aakoshh force-pushed the bft-474-batch-vote-publish-loop branch from e7693d7 to 49dc942 Compare July 8, 2024 18:17
@aakoshh aakoshh merged commit 55c6061 into main Jul 9, 2024
5 checks passed
@aakoshh aakoshh deleted the bft-474-batch-vote-publish-loop branch July 9, 2024 09:55
brunoffranca pushed a commit that referenced this pull request Jul 9, 2024
## What ❔

Followup for some design discussions in
#148

* Removes `BatchStore::unsigned_batch_numbers`
* Add `BatchStore::earliest_batch_number_to_sign`
* Add `BatchStore::latest_batch_number`
* Changes `AttesterRunner` to have a mutable range of
`earliest_batch_number` and `latest_batch_number` instead of querying
`unsigned_batch_numbers` and filtering it with a `min_batch_number`

## Why ❔

This might be more intuitive than the original version. 

Another motivation was that the current implementation of
`unsigned_batch_numbers` in
matter-labs/zksync-era#2399 has to do two
queries: 1) to figure out the latest batch number and 2) to find all
batches without certificates within a limited range of the latest batch.

Here we only query the latest batch number in each loop, and we forego
the check on whether a QC already exists, otherwise we'd be back at
doing two queries, or some ugly book keeping to only check it on the
first loop after a restart. It should be cheap enough to publish a
duplicate vote.
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.

3 participants