From e79137f28f010e703dd18b6a123739158c7d5151 Mon Sep 17 00:00:00 2001
From: Akosh Farkash <aakoshh@gmail.com>
Date: Tue, 9 Jul 2024 10:21:43 +0100
Subject: [PATCH] BFT-474: Comments about incomplete things around batches.
 Ignore queued.

---
 node/actors/network/src/gossip/runner.rs |  5 +++++
 node/libs/storage/src/batch_store.rs     | 15 +++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/node/actors/network/src/gossip/runner.rs b/node/actors/network/src/gossip/runner.rs
index 60765c46f..7cd7a3515 100644
--- a/node/actors/network/src/gossip/runner.rs
+++ b/node/actors/network/src/gossip/runner.rs
@@ -113,6 +113,9 @@ impl rpc::Handler<rpc::push_block_store_state::Rpc> for &PushBlockStoreStateServ
 #[async_trait]
 impl rpc::Handler<rpc::push_batch_store_state::Rpc> for &PushBatchStoreStateServer {
     fn max_req_size(&self) -> usize {
+        // XXX: The request will actually contain a `SyncBatch` which has all the blocks in the batch,
+        // so a constant 10kB cannot be the right limit. There is a `max_block_size` in config which
+        // should come into play, with some other limit on the batch size.
         10 * kB
     }
     async fn handle(
@@ -353,6 +356,8 @@ impl Network {
                             let ctx_with_timeout =
                                 self.cfg.rpc.get_batch_timeout.map(|t| ctx.with_timeout(t));
                             let ctx = ctx_with_timeout.as_ref().unwrap_or(ctx);
+                            // XXX: `max_block_size` isn't the right limit here as the response
+                            // will contain all blocks of a batch.
                             let batch = call
                                 .call(ctx, &req, self.cfg.max_block_size.saturating_add(kB))
                                 .await?
diff --git a/node/libs/storage/src/batch_store.rs b/node/libs/storage/src/batch_store.rs
index a03f361ed..dfb185ee2 100644
--- a/node/libs/storage/src/batch_store.rs
+++ b/node/libs/storage/src/batch_store.rs
@@ -258,16 +258,20 @@ impl BatchStore {
         Ok(batch)
     }
 
-    /// Retrieve the maximum batch number that we know about.
+    /// Retrieve the maximum persisted batch number.
     pub async fn latest_batch_number(
         &self,
         ctx: &ctx::Ctx,
     ) -> ctx::Result<Option<attester::BatchNumber>> {
         {
             let inner = self.inner.borrow();
-            if let Some(ref batch) = inner.queued.last {
-                return Ok(Some(batch.number));
-            }
+            // For now we ignore the cache 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));
+            // }
             if let Some(ref batch) = inner.persisted.last {
                 return Ok(Some(batch.number));
             }
@@ -332,6 +336,9 @@ impl BatchStore {
         batch: attester::SyncBatch,
         _genesis: validator::Genesis,
     ) -> ctx::Result<()> {
+        // XXX: Once we can validate `SyncBatch::proof` we should do it before adding the
+        // batch to the cache, otherwise a malicious peer could serve us data that prevents
+        // other inputs from entering the queue. It will also cause it to be gossiped at the moment.
         sync::wait_for(ctx, &mut self.inner.subscribe(), |inner| {
             inner.queued.next() >= batch.number
         })