feat: integrate batch changes #41

greged93 · 2025-03-26T16:15:49Z

Integrate batch changes in L1Watcher, Database and derivation pipeline.

Resolves #2.

⚠️ This needs to be reviewed after #40 ⚠️

greged93 · 2025-03-27T17:46:40Z

crates/watcher/src/lib.rs

+        commit_logs_with_tx
+            .sort_by(|(_, data_a, _), (_, data_b, _)| data_a.batch_index.cmp(&data_b.batch_index));
+        let groups = commit_logs_with_tx.into_iter().chunk_by(|(_, _, hash)| *hash);
+        let groups: Vec<_> =
+            groups.into_iter().map(|(hash, group)| (hash, group.collect::<Vec<_>>())).collect();


idea here is to order by batch index and then group the commits by transaction hash. This allows us to only query the transaction once when receiving multiple commit events, which can happen after Euclid.

frisitano

Logically looks good. Added a few comments in line.

crates/codec/src/lib.rs

frisitano · 2025-04-01T11:33:46Z

crates/codec/src/lib.rs

+        return None
+    }
+
+    Some((version & LOW_BYTES_FLAG).saturating_to())


Why use saturating here? Can we use to().expect("masked to a single byte") or something like this?

saturating_to doesn't introduce any overhead compared to to(), but will update just to stay consistent and clearly translate that no saturation can happen.

frisitano · 2025-04-01T11:37:27Z

crates/database/db/src/models/batch_input.rs

 pub struct Model {
    #[sea_orm(primary_key)]
    index: i64,
-    version: u8,
-    codec_version: u8,
    hash: Vec<u8>,
    block_number: i64,
-    parent_batch_header: Vec<u8>,
-    #[sea_orm(column_type = "JsonBinary")]
-    chunks: Chunks,
-    skipped_l1_message_bitmap: Vec<u8>,
-    blob_hash: Vec<u8>,
+    calldata: Vec<u8>,
+    blob_hash: Option<Vec<u8>>,


I wonder if we want to keep both encoded and decoded information in the database. i.e. just add calldata and blob_hash in addition to what we already have. It may be insightful to keep the additional information. Lets open a PR to review the data model in the future.

we could, although I would try to limit the amount of duplicate data we are storing.

frisitano · 2025-04-01T11:39:32Z

crates/database/db/src/operations.rs

 #[async_trait::async_trait]
 pub trait DatabaseOperations: DatabaseConnectionProvider {
-    /// Insert a [`BatchInput`] into the database.
-    async fn insert_batch_input(&self, batch_input: BatchInput) -> Result<(), DatabaseError> {
-        tracing::trace!(target: "scroll::db", batch_hash = ?batch_input.batch_hash(), batch_index = batch_input.batch_index(), "Inserting batch input into database.");
+    /// Insert a [`BatchCommitData`] into the database.
+    async fn insert_batch(&self, batch_input: BatchCommitData) -> Result<(), DatabaseError> {
+        tracing::trace!(target: "scroll::db", batch_hash = ?batch_input.hash, batch_index = batch_input.index, "Inserting batch input into database.");
        let batch_input: models::batch_input::ActiveModel = batch_input.into();


Should we rename models::batch_input to models::batch_commit?

frisitano · 2025-04-01T11:51:45Z

crates/derivation-pipeline/src/lib.rs

+/// An instance of the trait can be used to provide L1 data.
+pub trait L1Provider: L1MessageProvider {
+    /// Returns corresponding blob data for the provided hash.
+    fn blob(&self, hash: B256) -> Option<Blob>;
 }


Is there a reason that blob is at the root level and L1MessageProvider methods are not? How about:

Suggested change

/// An instance of the trait can be used to provide L1 data.

pub trait L1Provider: L1MessageProvider {

/// Returns corresponding blob data for the provided hash.

fn blob(&self, hash: B256) -> Option<Blob>;

}

pub trait L1Provider: L1MessageProvider + L1BlobProvider {}

impl L1Provider for T where T: L1MessageProvider + L1BlobProvider {}

Right this makes more sense, let me update

frisitano

Looks good! Left some minor comments inline.

crates/codec/src/lib.rs

frisitano · 2025-04-02T03:08:11Z

crates/codec/src/lib.rs

+        return Err(DecodingError::MissingCodecVersion)
+    }
+
+    Ok((version & LOW_BYTES_FLAG).uint_try_to().expect("masked to a single byte"))


In the check above, we have already asserted that the upper 31 bytes are 0. So, is there any value in using the LOW_BYTES_FLAG?

nit: typically would use the terminology of bitmask instead of flag

yeah you're right, using the last mask doesn't make sense, will update

frisitano · 2025-04-02T03:24:36Z

crates/watcher/src/lib.rs

-                if let Some(batch_input) = batch_builder.try_build() {
-                    self.notify(L1Notification::BatchCommit(batch_input)).await;
-                }
+                let batch_index = decoded_log.batch_index.saturating_to();


should we use .to() or the more explicit version you used previously .uint_try_to().expect("Uint conversion error")

I actually kept the saturating to here, because there isn't a check on the value of the index, which caused a panic in testing where index is generated randomly. I would prefer to either keep the saturating or add a check on the value, what do you think?

Is it possible for us to bound the value for the batch_index in testing? I don't think our code logic should be driven by our ability to produce representative test data. I would propose that we be practical here and use saturating variant in this instance but add a TODO inline and maybe create an issue to track it.

Is it possible for us to bound the value for the batch_index in testing? I don't think our code logic should be driven by our ability to produce representative test data.

It would be easy and I agree with your statement about avoiding code to be driven by test data. Maybe the expect makes more sense since this would be an unrecoverable error, linked to a contract issue on the L1.

Agreed 👍

* test: move calldata to files * feat: batch header decoding * feat: improve codec interface * chore: manifests fixes * feat: revert some codec changes * feat: wip derivation pipeline * feat: batch header v7 * feat: add batch abstraction * feat: basic derivation pipeline * feat: implement batch data hash * feat: move PayloadData * feat: improve batch data hash computation * test: derivation * chore: cleaning * fix: lints * fix: lints * fix: skip wasm for derivation pipeline * fix: data hash computation for batch * fix: lint * fix: lint * fix: lints * fix: answer comments * fix: lints

This reverts commit dbb9cb0.

* test: move calldata to files * feat: batch header decoding * feat: improve codec interface * chore: manifests fixes * feat: revert some codec changes * feat: wip derivation pipeline * feat: batch header v7 * feat: add batch abstraction * feat: basic derivation pipeline * feat: implement batch data hash * feat: move PayloadData * feat: improve batch data hash computation * test: derivation * chore: cleaning * fix: lints * fix: lints * fix: skip wasm for derivation pipeline * fix: data hash computation for batch * fix: lint * fix: lint * fix: lints * fix: answer comments * fix: lints * feat: integrate batch changes (#41) * feat: wip * feat: changes to the data model * fix: codec issue * feat: modify derivation pipeline to fetch blob * test: move test data to file * fix: lints * fix: answer comments * test: fix migration * feat: derivation pipeline (#40) * test: move calldata to files * feat: batch header decoding * feat: improve codec interface * chore: manifests fixes * feat: revert some codec changes * feat: wip derivation pipeline * feat: batch header v7 * feat: add batch abstraction * feat: basic derivation pipeline * feat: implement batch data hash * feat: move PayloadData * feat: improve batch data hash computation * test: derivation * chore: cleaning * fix: lints * fix: lints * fix: skip wasm for derivation pipeline * fix: data hash computation for batch * fix: lint * fix: lint * fix: lints * fix: answer comments * fix: lints * fix: comments

greged93 force-pushed the feat/integrate-batch-changes branch from 4c04f84 to 106d5d1 Compare March 26, 2025 16:24

greged93 mentioned this pull request Mar 26, 2025

feat: derivation pipeline #40

Merged

greged93 changed the base branch from main to feat/derivation-pipeline March 27, 2025 17:15

greged93 commented Mar 27, 2025

View reviewed changes

greged93 requested a review from frisitano March 31, 2025 09:56

frisitano reviewed Apr 1, 2025

View reviewed changes

greged93 added 7 commits April 1, 2025 16:34

feat: wip

1cf831a

feat: changes to the data model

f1de162

fix: codec issue

86f3880

feat: modify derivation pipeline to fetch blob

a8f18d9

test: move test data to file

d5c7313

fix: lints

3a6da41

fix: answer comments

eeb7a63

greged93 force-pushed the feat/integrate-batch-changes branch from 32549df to eeb7a63 Compare April 1, 2025 15:05

test: fix migration

52f35cf

frisitano approved these changes Apr 2, 2025

View reviewed changes

greged93 and others added 3 commits April 2, 2025 08:51

Merge branch 'main' into feat/integrate-batch-changes

0e5fa79

fix: comments

72831db

greged93 merged commit dbb9cb0 into feat/derivation-pipeline Apr 3, 2025
12 checks passed

greged93 added a commit that referenced this pull request Apr 3, 2025

Revert "feat: integrate batch changes (#41)"

d53aedb

This reverts commit dbb9cb0.

feat: integrate batch changes #41

feat: integrate batch changes #41

Uh oh!

Conversation

greged93 commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frisitano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greged93 Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frisitano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

greged93 commented Mar 26, 2025 •

edited

Loading

greged93 Apr 1, 2025 •

edited

Loading