Split transition information from `BatchInfo` to `BatchTransition` #637

prajwolrg · 2025-01-29T06:35:42Z

Description

Currently, the entire BatchInfo (including block heights and L1/L2 states) is proven, even though the block heights are already committed in the state. This leads to redundant data and unnatural edge cases—such as inclusive block ranges requiring small hacks to handle transitions.

This PR addresses those issues by splitting the structure into:

BatchTransition – contains only the essential proof data.
BatchInfo – handles auxiliary bookkeeping.
This separation removes duplication and simplifies handling of batch transitions.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests
Dependency Update

Notes to Reviewers

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

codecov · 2025-01-29T06:55:22Z

Codecov Report

Attention: Patch coverage is 15.35433% with 215 lines in your changes missing coverage. Please review.

Project coverage is 54.67%. Comparing base (a0d413d) to head (84dfcc8).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/sequencer/src/checkpoint/worker.rs	0.00%	76 Missing ⚠️
crates/state/src/batch.rs	27.14%	51 Missing ⚠️
bin/prover-client/src/operators/checkpoint.rs	0.00%	19 Missing ⚠️
...rates/consensus-logic/src/csm/client_transition.rs	0.00%	16 Missing ⚠️
crates/proof-impl/checkpoint/src/lib.rs	40.00%	12 Missing ⚠️
crates/db/src/types.rs	0.00%	11 Missing ⚠️
crates/sequencer/src/duty/types.rs	0.00%	8 Missing ⚠️
bin/strata-client/src/rpc_server.rs	0.00%	5 Missing ⚠️
crates/consensus-logic/src/csm/worker.rs	0.00%	4 Missing ⚠️
crates/state/src/client_state.rs	0.00%	4 Missing ⚠️
... and 5 more

@@            Coverage Diff             @@
##             main     #637      +/-   ##
==========================================
- Coverage   54.84%   54.67%   -0.17%     
==========================================
  Files         313      313              
  Lines       33526    33593      +67     
==========================================
- Hits        18386    18367      -19     
- Misses      15140    15226      +86

Files with missing lines	Coverage Δ
crates/primitives/src/l1/block.rs	`38.63% <ø> (ø)`
crates/proof-impl/cl-agg/src/lib.rs	`55.55% <100.00%> (-4.45%)`	⬇️
crates/proof-impl/cl-stf/src/lib.rs	`95.19% <100.00%> (+0.32%)`	⬆️
crates/proof-impl/l1-batch/src/logic.rs	`97.50% <100.00%> (ø)`
crates/state/src/l1/header_verification.rs	`65.60% <ø> (-14.63%)`	⬇️
crates/sequencer/src/duty/worker.rs	`0.00% <0.00%> (ø)`
bin/strata-sequencer-client/src/duty_executor.rs	`0.00% <0.00%> (ø)`
crates/consensus-logic/src/l1_handler.rs	`0.00% <0.00%> (ø)`
crates/rpc/types/src/types.rs	`0.00% <0.00%> (ø)`
crates/state/src/operation.rs	`49.60% <0.00%> (ø)`
... and 10 more

... and 11 files with indirect coverage changes

github-actions · 2025-01-29T07:19:00Z

Commit: 0960f83

SP1 Performance Test Results

program	cycles	success
BTC_BLOCKSPACE	30,370,824	✅
EL_BLOCK	97,975	✅
CL_BLOCK	90,286	✅
L1_BATCH	30,420,570	✅
L2_BATCH	3,566	✅
CHECKPOINT	25,699	✅

delbonis

Consider using the L1BlockCommitment/L2BlockCommitment types that are in the new chainstate manager PR.

crates/state/src/batch.rs

prajwolrg · 2025-01-30T06:52:36Z

Consider using the L1BlockCommitment/L2BlockCommitment types that are in the new chainstate manager PR.

Yes, makes sense. Will continue on this then once that is merged.

delbonis · 2025-02-02T20:55:26Z

@prajwolrg That PR got merged btw.

prajwolrg · 2025-02-04T14:49:48Z

Consider using the L1BlockCommitment/L2BlockCommitment types that are in the new chainstate manager PR.

Yes, makes sense. Will continue on this then once that is merged.

Right now L1BlockCommitment includes the height and the block hash, but in the BatchTransition we are verifying commitment of HeaderVerificationState which includes some additional info on top of height and block hash.

How do you suggest I proceed here? Do I include HeaderVerificationState commitment to L1BlockCommitment? Or leave it out as it is?

prajwolrg · 2025-02-04T18:21:31Z

Consider using the L1BlockCommitment/L2BlockCommitment types that are in the new chainstate manager PR.

Yes, makes sense. Will continue on this then once that is merged.

Right now L1BlockCommitment includes the height and the block hash, but in the BatchTransition we are verifying commitment of HeaderVerificationState which includes some additional info on top of height and block hash.

How do you suggest I proceed here? Do I include HeaderVerificationState commitment to L1BlockCommitment? Or leave it out as it is?

For BatchInfo, L1BlockCommitment and L2BlockCommitment has been used. BatchTransition has been left as is.

delbonis

Okay so I was somewhat confused by the type/field names being vague here. The documentation should have clarified it but the names themselves led me in the wrong direction initially.

crates/state/src/batch.rs

delbonis · 2025-02-04T18:53:30Z

crates/state/src/batch.rs

+pub struct BatchTransition {
    /// The inclusive hash range of `HeaderVerificationState` for L1 blocks.
-    /// This represents the transition of L1 state from the starting state to the
-    /// ending state. The hash is computed via
-    /// [`super::l1::HeaderVerificationState::compute_hash`].
+    ///
+    /// Represents a transition from the starting L1 state to the ending L1 state.
+    /// The hash is computed via [`super::l1::HeaderVerificationState::compute_hash`].
    pub l1_transition: (Buf32, Buf32),

    /// The inclusive hash range of `Chainstate` for L2 blocks.
-    /// This represents the transition of L2 state from the starting state to the
-    /// ending state. The state root is computed via
-    /// [`super::chain_state::Chainstate::compute_state_root`].
+    ///
+    /// Represents a transition from the starting L2 state to the ending L2 state.
+    /// The state root is computed via [`super::chain_state::Chainstate::compute_state_root`].
    pub l2_transition: (Buf32, Buf32),


Oh I was misunderstanding this the whole time.

For now I don't think we need to explicitly state these L2 state roots here since they're transitively committed to by the blkids in the block commitments in the other struct. Or do these represent something else.

In the future we might commit to the "outer post-state" here (right now that is what's put in the epoch terminal block), but we can revisit that later.

Right now there's a slight inconsistency between how states are recorded in BatchInfo versus BatchTransition. For example, in BatchInfo, if you have an l1_range of (1, 5), the next range will be (6, 10). In this context, the block IDs (and the corresponding state root it commits) are recorded after the block is applied. However, in BatchTransition, the state roots are captured before the block is applied, leading to a slight mismatch.

Then would it make sense to change the framing to be based on "prev checkpoint tip" and "new checkpoint tip". And make it half open in the reverse direction?

However, in BatchTransition, the state roots are captured before the block is applied, leading to a slight mismatch.

This seems like a weird special case. What is the reason for this?

For L2 it could have been based entirely on L2BlockId since it already commits the state_root. But since L1BlockId doesn't commit to the L1State, it's done this way for the consistency between L1 and L2.

crates/state/src/batch.rs

bewakes

I would go for some concise name for CheckpointBaseStateCommitment, something like BaseStateCommitment which should be clear enough given that the struct has docstring. But the rest is good.

delbonis

I think this can be merged right?

delbonis · 2025-02-06T17:28:31Z

Merging to unblock stuff.

prajwolrg force-pushed the STR-844-batch-logic-cleanups branch from 08a8a65 to 0c2e551 Compare January 29, 2025 06:44

prajwolrg marked this pull request as ready for review January 29, 2025 08:42

prajwolrg requested review from a team as code owners January 29, 2025 08:42

prajwolrg changed the title ~~Str 844 batch logic cleanups~~ Split verifiable transition information BatchInfo to BatchTransition Jan 29, 2025

prajwolrg changed the title ~~Split verifiable transition information BatchInfo to BatchTransition~~ Split transition information from BatchInfo to BatchTransition Jan 29, 2025

bewakes previously approved these changes Jan 29, 2025

View reviewed changes

prajwolrg dismissed bewakes’s stale review via a853e43 January 29, 2025 14:07

prajwolrg force-pushed the STR-844-batch-logic-cleanups branch from f24b345 to a853e43 Compare January 29, 2025 14:07

prajwolrg requested a review from bewakes January 29, 2025 15:00

delbonis requested changes Jan 29, 2025

View reviewed changes

crates/state/src/batch.rs Show resolved Hide resolved

crates/state/src/batch.rs Outdated Show resolved Hide resolved

crates/state/src/batch.rs Outdated Show resolved Hide resolved

prajwolrg requested a review from a team as a code owner January 30, 2025 06:56

prajwolrg requested a review from delbonis February 1, 2025 09:57

prajwolrg force-pushed the STR-844-batch-logic-cleanups branch from c82518a to 8060379 Compare February 4, 2025 13:53

prajwolrg marked this pull request as draft February 4, 2025 14:01

prajwolrg force-pushed the STR-844-batch-logic-cleanups branch from 0068f21 to b40b8cf Compare February 4, 2025 14:14

prajwolrg marked this pull request as ready for review February 4, 2025 18:13

prajwolrg requested review from a team as code owners February 4, 2025 18:13

delbonis requested changes Feb 4, 2025

View reviewed changes

prajwolrg requested a review from delbonis February 5, 2025 03:21

prajwolrg requested a review from a team as a code owner February 5, 2025 06:16

MdTeach self-requested a review February 5, 2025 06:19

MdTeach previously approved these changes Feb 5, 2025

View reviewed changes

bewakes previously approved these changes Feb 5, 2025

View reviewed changes

delbonis previously approved these changes Feb 5, 2025

View reviewed changes

prajwolrg added 8 commits February 6, 2025 14:11

refactor: checkpoint proof to verify only the batch transition

504b33b

fixes after rebase

31b2b3f

refactor: rename BootstrapState -> CheckpointBaseState

455aa33

refactor: clean CheckpointEntry

b71580c

refactor: use L1BlockCommitment and L2BlockCommitment

72fb21a

fixes: use BlockCommitments

df4f634

refactor: rename to CheckpointBaseStateCommitment

90e7fd7

fixes

5ba172c

prajwolrg dismissed stale reviews from delbonis, bewakes, and MdTeach via 5ba172c February 6, 2025 08:27

prajwolrg force-pushed the STR-844-batch-logic-cleanups branch from 84dfcc8 to 5ba172c Compare February 6, 2025 08:27

refactor: rename CheckpointBaseStateCommitment to BaseStateCommitment

ce222f8

prajwolrg force-pushed the STR-844-batch-logic-cleanups branch from 28f63bc to ce222f8 Compare February 6, 2025 08:52

MdTeach requested review from delbonis, MdTeach and bewakes February 6, 2025 13:40

MdTeach approved these changes Feb 6, 2025

View reviewed changes

delbonis approved these changes Feb 6, 2025

View reviewed changes

delbonis merged commit 004be7c into main Feb 6, 2025
16 of 17 checks passed

delbonis deleted the STR-844-batch-logic-cleanups branch February 6, 2025 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split transition information from `BatchInfo` to `BatchTransition` #637

Split transition information from `BatchInfo` to `BatchTransition` #637

prajwolrg commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

delbonis left a comment

prajwolrg commented Jan 30, 2025

delbonis commented Feb 2, 2025 •

edited

Loading

prajwolrg commented Feb 4, 2025

prajwolrg commented Feb 4, 2025 •

edited

Loading

delbonis left a comment

delbonis Feb 4, 2025

prajwolrg Feb 5, 2025

delbonis Feb 5, 2025

prajwolrg Feb 6, 2025

bewakes left a comment

delbonis left a comment

delbonis commented Feb 6, 2025

Split transition information from BatchInfo to BatchTransition #637

Split transition information from BatchInfo to BatchTransition #637

Conversation

prajwolrg commented Jan 29, 2025 • edited Loading

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 29, 2025 • edited Loading

delbonis left a comment

Choose a reason for hiding this comment

prajwolrg commented Jan 30, 2025

delbonis commented Feb 2, 2025 • edited Loading

prajwolrg commented Feb 4, 2025

prajwolrg commented Feb 4, 2025 • edited Loading

delbonis left a comment

Choose a reason for hiding this comment

delbonis Feb 4, 2025

Choose a reason for hiding this comment

prajwolrg Feb 5, 2025

Choose a reason for hiding this comment

delbonis Feb 5, 2025

Choose a reason for hiding this comment

prajwolrg Feb 6, 2025

Choose a reason for hiding this comment

bewakes left a comment

Choose a reason for hiding this comment

delbonis left a comment

Choose a reason for hiding this comment

delbonis commented Feb 6, 2025

Split transition information from `BatchInfo` to `BatchTransition` #637

Split transition information from `BatchInfo` to `BatchTransition` #637

prajwolrg commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

delbonis commented Feb 2, 2025 •

edited

Loading

prajwolrg commented Feb 4, 2025 •

edited

Loading