Update L1Tx ref types to include multiple proto ops per tx #618

bewakes · 2025-01-20T10:06:45Z

Description

This PR does the following:

Add RawProtocolOperation type, which is supposed to contain extra info(like da blob) which is not required for the rollup protocol along with the data that is required(like da commitment, deposit info, etc).
Add a mechanism in filter_protocol_op_tx_refs wherein a function of type (&RawProtocolOperation) -> Result<()> can be passed such that prover can pass a no-op while full node can pass logic to store extra info to database if necessary.
Adds unit test that tests multiple protocol ops(checkpoint and deposit in particular) can be parsed from a single transaction. Previously, only one operation could be parsed from a transaction.

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

STR-935

github-actions · 2025-01-20T10:42:29Z

Commit: a8d4e6e

SP1 Performance Test Results

program	cycles	success
BTC_BLOCKSPACE	30,357,513	✅
EL_BLOCK	98,789	✅
CL_BLOCK	57,214	✅
L1_BATCH	30,387,795	✅
L2_BATCH	5,473	✅
CHECKPOINT	15,895	✅

codecov · 2025-01-21T15:25:19Z

Codecov Report

Attention: Patch coverage is 74.72527% with 46 lines in your changes missing coverage. Please review.

Project coverage is 56.28%. Comparing base (3eb992c) to head (fcf9d7c).

Files with missing lines	Patch %	Lines
crates/consensus-logic/src/l1_handler.rs	0.00%	9 Missing ⚠️
crates/l1tx/src/filter/mod.rs	92.07%	8 Missing ⚠️
crates/consensus-logic/src/duty/block_assembly.rs	0.00%	6 Missing ⚠️
crates/l1tx/src/messages.rs	40.00%	6 Missing ⚠️
crates/l1tx/src/filter/visitor.rs	64.28%	5 Missing ⚠️
crates/btcio/src/reader/ops_visitor.rs	0.00%	4 Missing ⚠️
crates/btcio/src/reader/query.rs	0.00%	4 Missing ⚠️
crates/state/src/l1/utils.rs	0.00%	2 Missing ⚠️
bin/strata-client/src/extractor.rs	94.73%	1 Missing ⚠️
crates/proof-impl/btc-blockspace/src/filter.rs	80.00%	1 Missing ⚠️

@@                       Coverage Diff                       @@
##           STR-807-inscription-changes     #618      +/-   ##
===============================================================
+ Coverage                        56.15%   56.28%   +0.12%     
===============================================================
  Files                              316      318       +2     
  Lines                            32852    32929      +77     
===============================================================
+ Hits                             18449    18535      +86     
+ Misses                           14403    14394       -9

Files with missing lines	Coverage Δ
crates/chaintsn/src/transition.rs	`82.86% <100.00%> (ø)`
crates/l1tx/src/filter/types.rs	`95.65% <ø> (ø)`
crates/rocksdb-store/src/l1/db.rs	`97.29% <100.00%> (ø)`
crates/state/src/l1/tx.rs	`64.70% <100.00%> (-2.95%)`	⬇️
crates/state/src/state_op.rs	`43.97% <100.00%> (ø)`
crates/state/src/tx.rs	`0.00% <ø> (ø)`
bin/strata-client/src/extractor.rs	`95.02% <94.73%> (+0.45%)`	⬆️
crates/proof-impl/btc-blockspace/src/filter.rs	`53.84% <80.00%> (+3.84%)`	⬆️
crates/state/src/l1/utils.rs	`58.33% <0.00%> (ø)`
crates/btcio/src/reader/ops_visitor.rs	`0.00% <0.00%> (ø)`
... and 6 more

... and 3 files with indirect coverage changes

delbonis

Need to rework the data structures a little bit to make them more accurately reflect the design we have now.

Also, see the visitor-oriented approach. The goal with that was to decouple the "figuring out what to do with protocol ops we get" from the "actually finding the protocol ops" parts so that handling can be different depending on if it's in the proof or in the reader.

delbonis · 2025-01-21T18:24:06Z

crates/l1tx/src/filter.rs

+pub fn filter_protocol_op_tx_refs<F>(
    block: &Block,
    params: &RollupParams,
    filter_config: &TxFilterConfig,
-) -> Vec<ProtocolOpTxRef> {
+    process_raw: &mut F,
+) -> Vec<ProtocolOpTxRef>
+where
+    F: FnMut(&RawProtocolOperation) -> anyhow::Result<()>,


You can write this with &mut impl FnMut(...) -> ...

delbonis · 2025-01-21T18:25:14Z

crates/state/src/tx.rs

+/// Similar to [`ProtocolOperation`] except that this also contains blob data which is not relevant
+/// to chain.


I don't see DA blobs here.

Also it is relevant, we need to see the blob commitments.

I'm a bit confused about what the purpose of this type is, being distinct from the other one.

Also it is relevant, we need to see the blob commitments.

Yes, the commitments, but not the actual data/blob which is what I meant in the docstring.

'm a bit confused about what the purpose of this type is, being distinct from the other one.

So the idea here is that RawProtocolOp would contain a variant Da(Vec<u8>) whereas ProtocolOp would have the counterpart DaCommitment(Buf32). The other variants would be the same. Those(Da/DaCommithemt) are not here right now because we don't actually have DA at the moment.

Just do the visitor thing. The approach with the handler function is just an roundabout and inelegant way of doing the same general idea. Do not copy out the full contents of DA into a type if it's not necessary. That's what the visitor based approach (that I guided in the original PR) enables.

crates/state/src/state_op.rs

crates/l1tx/src/messages.rs

delbonis

(Discussed in core sync, leaving review here to continue work.)

delbonis · 2025-01-23T15:19:57Z

crates/l1tx/src/filter/visitor.rs

+    }
+
+    // Do stuffs with DA.
+    fn visit_da(&self, d: &[u8]) -> ProtocolOperation {


chunks: impl Iterator<Item = &[u8]>

delbonis · 2025-01-23T15:21:50Z

crates/l1tx/src/filter/visitor.rs

+pub trait OpsVisitor {
+    // Do stuffs with `SignedBatchCheckpoint`.
+    fn visit_checkpoint(&self, chkpt: SignedBatchCheckpoint) -> ProtocolOperation {
+        ProtocolOperation::Checkpoint(chkpt)
+    }
+
+    // Do stuffs with `DepositInfo`.
+    fn visit_deposit(&self, d: DepositInfo) -> ProtocolOperation {
+        ProtocolOperation::Deposit(d)
+    }
+
+    // Do stuffs with `DepositRequest`.
+    fn visit_deposit_request(&self, d: DepositRequestInfo) -> ProtocolOperation {
+        ProtocolOperation::DepositRequest(d)
+    }
+
+    // Do stuffs with DA.
+    fn visit_da(&self, d: &[u8]) -> ProtocolOperation {


Needs to take &mut self.

bewakes force-pushed the STR-807-inscription-changes branch from da8be4c to e78e71d Compare January 20, 2025 10:12

bewakes force-pushed the STR-807-inscription-changes branch 4 times, most recently from 0a66e86 to d8f04ee Compare January 21, 2025 11:26

bewakes force-pushed the STR-402-rework-protocol-op-types branch 2 times, most recently from 6c885d6 to 505735d Compare January 21, 2025 12:18

bewakes marked this pull request as ready for review January 21, 2025 13:03

bewakes requested review from a team as code owners January 21, 2025 13:03

delbonis requested changes Jan 21, 2025

View reviewed changes

bewakes mentioned this pull request Jan 22, 2025

STR-950 envelope changes #590

Open

13 tasks

Bibek Pandey added 11 commits January 23, 2025 15:24

Some cleanups

0a31959

l1tx: Use tags to parse and build l1 payloads

1a5e2cb

db: Add IntentEntry and corrensponding db operations, and tests

914407e

Support multiple payloads as envelopes

48c46cc

DB changes for payload entries

fd52a83

Remove sequencerdb

72b6c76

btcio: Add payload bundler

8e7e52e

db: rename envelope_db to writer, Add/edit more methods for writer db

f279162

Fix bug around intent not being processed

e5eefd2

Make unit tests compile

b039bd9

Get tests to pass

6710b75

Bibek Pandey added 7 commits January 23, 2025 15:24

Rename sequencer to writer in db modules

f48b853

Update tests

5efdfe9

Actually parse multiple envelopes

a603ce4

Some renamings

0030b4d

Enhance bundler task

88f35be

Review fixes, fix config unit test

aa142c7

l1tx: Do not include payload size in envelope

3eb992c

bewakes force-pushed the STR-807-inscription-changes branch from d8f04ee to 3eb992c Compare January 23, 2025 10:01

bewakes requested a review from a team as a code owner January 23, 2025 10:01

Bibek Pandey added 6 commits January 23, 2025 15:54

Update L1Tx ref types to include multiple proto ops per tx

28b3c5c

Add RawProtocolOperation, update filtering to support extra processing

94eb6d5

Add unit test for parsing multiple protocol ops in a tx

607e8a4

l1tx: Fix filter unit tests

50c4ec2

Cleanup: remove generic F in proto ops filtering, some renaming

5b4e0e3

l1tx/filter: Add visitor trait

501dbc9

bewakes force-pushed the STR-402-rework-protocol-op-types branch from 9991efb to 501dbc9 Compare January 23, 2025 10:10

Rename ProtocolOpsTxRef -> ProtocolTxEntry

fcf9d7c

bewakes requested a review from delbonis January 23, 2025 10:28

delbonis requested changes Jan 23, 2025

View reviewed changes

bewakes force-pushed the STR-807-inscription-changes branch from 3eb992c to d7dc0a9 Compare January 24, 2025 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update L1Tx ref types to include multiple proto ops per tx #618

Update L1Tx ref types to include multiple proto ops per tx #618

bewakes commented Jan 20, 2025 •

edited by john-light

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

delbonis left a comment

delbonis Jan 21, 2025

delbonis Jan 21, 2025

bewakes Jan 22, 2025 •

edited

Loading

bewakes Jan 22, 2025 •

edited

Loading

delbonis Jan 22, 2025 •

edited

Loading

bewakes Jan 23, 2025

delbonis left a comment

delbonis Jan 23, 2025

delbonis Jan 23, 2025

		/// Similar to [`ProtocolOperation`] except that this also contains blob data which is not relevant
		/// to chain.

Update L1Tx ref types to include multiple proto ops per tx #618

Are you sure you want to change the base?

Update L1Tx ref types to include multiple proto ops per tx #618

Conversation

bewakes commented Jan 20, 2025 • edited by john-light Loading

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

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

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

delbonis left a comment

Choose a reason for hiding this comment

delbonis Jan 21, 2025

Choose a reason for hiding this comment

delbonis Jan 21, 2025

Choose a reason for hiding this comment

bewakes Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

bewakes Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

delbonis Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

bewakes Jan 23, 2025

Choose a reason for hiding this comment

delbonis left a comment

Choose a reason for hiding this comment

delbonis Jan 23, 2025

Choose a reason for hiding this comment

delbonis Jan 23, 2025

Choose a reason for hiding this comment

bewakes commented Jan 20, 2025 •

edited by john-light

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

bewakes Jan 22, 2025 •

edited

Loading

bewakes Jan 22, 2025 •

edited

Loading

delbonis Jan 22, 2025 •

edited

Loading