From 45ad64981e7ea590a21858194d5fddc8b8a0c327 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 8 Jul 2024 10:47:52 +0100 Subject: [PATCH] BFT-474: Implement the new PersistentBatchStore methods for the inmemory version --- node/Cargo.lock | 1 + .../libs/roles/src/attester/messages/batch.rs | 8 +++ node/libs/storage/Cargo.toml | 1 + node/libs/storage/src/batch_store.rs | 46 ++++++++++++++--- node/libs/storage/src/testonly/in_memory.rs | 51 +++++++++++++++---- 5 files changed, 92 insertions(+), 15 deletions(-) diff --git a/node/Cargo.lock b/node/Cargo.lock index f85572f28..67a30dee8 100644 --- a/node/Cargo.lock +++ b/node/Cargo.lock @@ -4139,6 +4139,7 @@ dependencies = [ "tracing", "vise", "zksync_concurrency", + "zksync_consensus_crypto", "zksync_consensus_roles", "zksync_protobuf", "zksync_protobuf_build", diff --git a/node/libs/roles/src/attester/messages/batch.rs b/node/libs/roles/src/attester/messages/batch.rs index 67d7e56cc..f395a9011 100644 --- a/node/libs/roles/src/attester/messages/batch.rs +++ b/node/libs/roles/src/attester/messages/batch.rs @@ -8,6 +8,11 @@ use zksync_consensus_crypto::{keccak256::Keccak256, ByteFmt, Text, TextFmt}; use zksync_consensus_utils::enum_util::Variant; /// A batch of L2 blocks used for the peers to fetch and keep in sync. +/// +/// The use case for this is for peers to be able to catch up with L1 +/// batches they missed during periods of being offline. These will +/// come with a proof of having been included on L1, rather than an +/// attester quorum certificate. #[derive(Clone, Debug, PartialEq, Eq, Default)] pub struct SyncBatch { /// The number of the batch. @@ -15,6 +20,9 @@ pub struct SyncBatch { /// The payloads of the blocks the batch contains. pub payloads: Vec, /// 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. pub proof: Vec, } diff --git a/node/libs/storage/Cargo.toml b/node/libs/storage/Cargo.toml index e1cca9e22..279688246 100644 --- a/node/libs/storage/Cargo.toml +++ b/node/libs/storage/Cargo.toml @@ -9,6 +9,7 @@ license.workspace = true [dependencies] zksync_concurrency.workspace = true zksync_consensus_roles.workspace = true +zksync_consensus_crypto.workspace = true zksync_protobuf.workspace = true anyhow.workspace = true diff --git a/node/libs/storage/src/batch_store.rs b/node/libs/storage/src/batch_store.rs index 3be65b970..128182eb1 100644 --- a/node/libs/storage/src/batch_store.rs +++ b/node/libs/storage/src/batch_store.rs @@ -56,18 +56,42 @@ pub trait PersistentBatchStore: 'static + fmt::Debug + Send + Sync { /// Returns `None` if no batches have been created yet. async fn last_batch(&self, ctx: &ctx::Ctx) -> ctx::Result>; + /// Get the numbers of L1 batches which are missing the corresponding L1 batch quorum certificates + /// and potentially need to be signed by attesters. + /// + /// A replica might never have a complete history of L1 batch QCs; once the L1 batch is included on L1, + /// the replicas might use the [attester::SyncBatch] route to obtain them, in which case they will not + /// have a QC and no reason to get them either. The store will have sufficient information to decide + /// where it's still necessary to gossip votes; for example the main node will want to have a QC on + /// every batch while it's the one submitting them to L1, while replicas can ask the L1 what is considered + /// final. + async fn unsigned_batch_numbers( + &self, + ctx: &ctx::Ctx, + ) -> ctx::Result>; + /// Get the L1 batch QC from storage with the highest number. /// /// Returns `None` if we don't have a QC for any of the batches yet. async fn last_batch_qc(&self, ctx: &ctx::Ctx) -> ctx::Result>; - /// Returns the batch with the given number. + /// Returns the [attester::SyncBatch] with the given number, which is used by peers + /// to catch up with L1 batches that they might have missed if they went offline. async fn get_batch( &self, ctx: &ctx::Ctx, number: attester::BatchNumber, ) -> ctx::Result>; + /// Returns the [attester::Batch] with the given number, which is the `message` that + /// appears in [attester::BatchQC], and represents the content that needs to be signed + /// by the attesters. + async fn get_batch_to_sign( + &self, + ctx: &ctx::Ctx, + number: attester::BatchNumber, + ) -> ctx::Result>; + /// Returns the QC of the batch with the given number. async fn get_batch_qc( &self, @@ -240,18 +264,28 @@ impl BatchStore { /// before the earliest in these numbers that isn't signed, but it would be futile to sign them any more. pub async fn unsigned_batch_numbers( &self, - _ctx: &ctx::Ctx, + ctx: &ctx::Ctx, ) -> ctx::Result> { - todo!("retrieve unsigned numbers from persistent store") + let unsigned = self + .persistent + .unsigned_batch_numbers(ctx) + .await + .context("persistent.get_batch_to_sign()")?; + Ok(unsigned) } /// Retrieve a batch to be signed. pub async fn batch_to_sign( &self, - _ctx: &ctx::Ctx, - _number: attester::BatchNumber, + ctx: &ctx::Ctx, + number: attester::BatchNumber, ) -> ctx::Result> { - todo!("retrieve the batch commitment from persistent store") + let batch = self + .persistent + .get_batch_to_sign(ctx, number) + .await + .context("persistent.get_batch_to_sign()")?; + Ok(batch) } /// Append batch to a queue to be persisted eventually. diff --git a/node/libs/storage/src/testonly/in_memory.rs b/node/libs/storage/src/testonly/in_memory.rs index 18f4205ff..61c9acd4b 100644 --- a/node/libs/storage/src/testonly/in_memory.rs +++ b/node/libs/storage/src/testonly/in_memory.rs @@ -22,8 +22,7 @@ struct BlockStoreInner { struct BatchStoreInner { persisted: sync::watch::Sender, batches: Mutex>, - // This field was only added to be able to implement the PersistentBatchStore trait correctly. - qcs: Mutex>, + certs: Mutex>, } /// In-memory block store. @@ -74,7 +73,7 @@ impl BatchStore { Self(Arc::new(BatchStoreInner { persisted: sync::watch::channel(BatchStoreState { first, last: None }).0, batches: Mutex::default(), - qcs: Mutex::default(), + certs: Mutex::default(), })) } } @@ -137,9 +136,43 @@ impl PersistentBatchStore for BatchStore { } async fn last_batch_qc(&self, _ctx: &ctx::Ctx) -> ctx::Result> { - let qcs = self.0.qcs.lock().unwrap(); - let last_batch_number = qcs.keys().max().unwrap(); - Ok(qcs.get(last_batch_number).cloned()) + let certs = self.0.certs.lock().unwrap(); + let last_batch_number = certs.keys().max().unwrap(); + Ok(certs.get(last_batch_number).cloned()) + } + + async fn unsigned_batch_numbers( + &self, + _ctx: &ctx::Ctx, + ) -> ctx::Result> { + let batches = self.0.batches.lock().unwrap(); + let certs = self.0.certs.lock().unwrap(); + + Ok(batches + .iter() + .map(|b| b.number) + .filter(|n| !certs.contains_key(n)) + .collect()) + } + + async fn get_batch_to_sign( + &self, + ctx: &ctx::Ctx, + number: attester::BatchNumber, + ) -> ctx::Result> { + // Here we just produce some deterministic mock hash. The real hash is available in the database. + // and contains a commitment to the data submitted to L1. It is *not* over SyncBatch. + let Some(batch) = self.get_batch(ctx, number).await? else { + return Ok(None); + }; + + let bz = zksync_protobuf::canonical(&batch); + let hash = zksync_consensus_crypto::keccak256::Keccak256::new(&bz); + + Ok(Some(attester::Batch { + number, + hash: attester::BatchHash(hash), + })) } async fn get_batch_qc( @@ -147,12 +180,12 @@ impl PersistentBatchStore for BatchStore { _ctx: &ctx::Ctx, number: attester::BatchNumber, ) -> ctx::Result> { - let qcs = self.0.qcs.lock().unwrap(); - Ok(qcs.get(&number).cloned()) + let certs = self.0.certs.lock().unwrap(); + Ok(certs.get(&number).cloned()) } async fn store_qc(&self, _ctx: &ctx::Ctx, qc: attester::BatchQC) -> ctx::Result<()> { - self.0.qcs.lock().unwrap().insert(qc.message.number, qc); + self.0.certs.lock().unwrap().insert(qc.message.number, qc); Ok(()) }