fix: Comments about SyncBatch vs L1 and wait for batch persisted (BFT…

…-474) (#154)

## What ❔

* Fixes the comments I made about `SyncBatch::proof` requiring L1
inclusion - that was based on a misunderstanding that the `proof` was a
Merkle proof starting from the L1 state root hash, rather than a field
inside the zksync system contract on L1. We only need an L1 client to
validate the proof, but we can produce it offline as soon as we have the
commitment locally.
* Changed the loop in `AttesterRunner` so that it waits for `persisted`
state of the `BatchStore` to indicate that the next batch number is
available. This will be when the `SyncBatch` can be constructed, so it's
when the commitment is ready too (or will be, not currently) and we're
ready to sign the payload.
* Removed `BatchStore::last_batch_number`; I wanted to use
`persisted.last`, which can be `None` in the beginning until
`SyncBatch::proof` is available, with a fallback to
`PersistentBatchStore::last_batch` which just looks at the latest
`l1_batches` record, but I wasn't sure whether a next batch can be
created before the commitment for the previous one is available, which
would cause a regression in the value returned. Instead we just use
`BatchStore::wait_for_persisted(0)` to tell us what the starting value
is after the first batch is ready.

## Why ❔

To remove misleading comments, and to reduce unneeded polling of the
database itself - now we wait until the memory indicates it's worth
hitting the DB.

node/actors/executor/src/attestation.rs

@@ -52,9 +52,11 @@ impl AttesterRunner {
         // Find the initial range of batches that we want to (re)sign after a (re)start.
         let last_batch_number = self
             .batch_store
-            .last_batch_number(ctx)
+            .wait_until_persisted(ctx, attester::BatchNumber(0))
             .await
-            .context("last_batch_number")?
+            .context("wait_until_persisted")?
+            .last
+            .map(|b| b.number)
             .unwrap_or_default();
 
         // Determine the batch to start signing from.
@@ -86,6 +88,14 @@ impl AttesterRunner {
                 .context("publish")?;
 
             batch_number = batch_number.next();
+
+            // We can avoid actively polling the database by waiting for the next persisted batch to appear
+            // in the memory (which itself relies on polling). This happens once we have the commitment,
+            // which for nodes that get the blocks through BFT should happen after execution. Nodes which
+            // rely on batch sync don't participate in attestations as they need the batch on L1 first.
+            self.batch_store
+                .wait_until_persisted(ctx, batch_number)
+                .await?;
         }
     }
 

node/libs/roles/src/attester/messages/batch.rs

@@ -22,7 +22,10 @@ pub struct SyncBatch {
     /// The proof of the batch.
     ///
     /// It is going to be a Merkle proof the the batch has been included
-    /// in the state hash of the L1.
+    /// in the state of the L1 system contract (not the L1 state root hash).
+    /// It can be produced as soon as we have the commitment available
+    /// locally, but validation requires a trusted L1 client to look up
+    /// the system contract state.
     pub proof: Vec<u8>,
 }
 

node/libs/storage/src/batch_store.rs

@@ -128,12 +128,8 @@ struct Inner {
     ///
     /// This reflects the state of the database. Its source is mainly the `PersistentBatchStore`:
     /// * the `BatchStoreRunner` subscribes to `PersistedBatchStore::persisted()` and copies its contents to here;
-    ///   it also uses the opportunity to clear items from the `cache` but notably doesn't update `queued` which
-    ///   which would cause the data to be gossiped
-    ///
-    /// Be careful that the `BatchStoreState` works with `SyncBatch` which requires a `proof` of inclusion on L1,
-    /// so this persistence is much delayed compared to the latest batch physically available in the database:
-    /// the batch also has to be signed by attesters, submitted to L1, and finalised there to appear here.
+    /// * it also uses the opportunity to clear items from the `cache`
+    /// * but notably doesn't update `queued`, which would cause the data to be gossiped
     persisted: BatchStoreState,
     cache: VecDeque<attester::SyncBatch>,
 }
@@ -199,6 +195,7 @@ impl BatchStoreRunner {
                 loop {
                     let persisted = sync::changed(ctx, &mut persisted).await?.clone();
                     self.0.inner.send_modify(|inner| {
+                        // XXX: In `BlockStoreRunner` update both the `queued` and the `persisted` here.
                         inner.persisted = persisted;
                         inner.truncate_cache();
                     });
@@ -275,42 +272,6 @@ impl BatchStore {
         Ok(batch)
     }
 
-    /// Retrieve the maximum persisted batch number.
-    pub async fn last_batch_number(
-        &self,
-        ctx: &ctx::Ctx,
-    ) -> ctx::Result<Option<attester::BatchNumber>> {
-        {
-            // let inner = self.inner.borrow();
-
-            // For now we ignore `queued` here because it's not clear how it's updated,
-            // validation is missing and it seems to depend entirely on gossip. Don't
-            // want it to somehow get us stuck and prevent voting. At least the persisted
-            // cache is maintained by two background processes copying the data from the DB.
-
-            // if let Some(ref batch) = inner.queued.last {
-            //     return Ok(Some(batch.number));
-            // }
-
-            // We also have to ignore `persisted` because `last` is an instance of `SyncBatch`
-            // which is conceptually only available once we have a proof that it's been included
-            // on L1, which requires a signature in the first place.
-
-            // if let Some(ref batch) = inner.persisted.last {
-            //     return Ok(Some(batch.number));
-            // }
-        }
-
-        // Get the last L1 batch that exists in the DB regardless of its status.
-        let batch = self
-            .persistent
-            .last_batch(ctx)
-            .await
-            .context("persistent.last_batch()")?;
-
-        Ok(batch)
-    }
-
     /// Retrieve the minimum batch number that doesn't have a QC yet and potentially need to be signed.
     ///
     /// There might be unsigned batches before this one in the database, however we don't consider it