STR-881 inscription changes #590

bewakes · 2025-01-06T11:59:27Z

This PR primarily introduces the following changes:

Changes the OP_IF..OP_ENDIF envelope structure. It still retains version and size of the payload.
Support of multiple L1 payloads bundled as envelopes in a single reveal transaction.
Corresponding db changes. Previously, it had 1-1 mapping between payload intent and L1 transaction, now it is many-to-one.
Add a bundler task in btcio such that creates BundledPayloadEntry and stores in db, which is ultimately published to bitcoin. Currently, it just does 1-1 mapping like it was before, but the bundling logic can be advanced in future.
Renames sequencer db to writer db, because actually it contains data related to payloads and bundles.

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

github-actions · 2025-01-09T12:12:27Z

Commit: 107387d

SP1 Performance Test Results

program	cycles	success
BTC_BLOCKSPACE	30,357,715	✅
EL_BLOCK	98,789	✅
CL_BLOCK	57,174	✅
L1_BATCH	30,387,642	✅
L2_BATCH	5,473	✅
CHECKPOINT	15,895	✅

codecov · 2025-01-17T11:45:13Z

Codecov Report

Attention: Patch coverage is 70.23061% with 142 lines in your changes missing coverage. Please review.

Project coverage is 56.48%. Comparing base (38abd51) to head (e78e71d).

Files with missing lines	Patch %	Lines
crates/btcio/src/writer/task.rs	35.52%	49 Missing ⚠️
crates/btcio/src/writer/bundler.rs	0.00%	44 Missing ⚠️
crates/db/src/types.rs	22.72%	17 Missing ⚠️
crates/rocksdb-store/src/writer/db.rs	90.41%	14 Missing ⚠️
bin/strata-client/src/main.rs	0.00%	5 Missing ⚠️
crates/l1tx/src/envelope/parser.rs	92.85%	4 Missing ⚠️
crates/l1tx/src/filter.rs	95.52%	3 Missing ⚠️
crates/rocksdb-store/src/lib.rs	0.00%	3 Missing ⚠️
crates/btcio/src/reader/query.rs	0.00%	2 Missing ⚠️
crates/consensus-logic/src/duty/worker.rs	0.00%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #590      +/-   ##
==========================================
- Coverage   56.56%   56.48%   -0.08%     
==========================================
  Files         313      313              
  Lines       32634    32720      +86     
==========================================
+ Hits        18459    18482      +23     
- Misses      14175    14238      +63

Files with missing lines	Coverage Δ
crates/btcio/src/test_utils.rs	`70.91% <100.00%> (ø)`
crates/btcio/src/writer/builder.rs	`98.04% <100.00%> (ø)`
crates/btcio/src/writer/signer.rs	`100.00% <100.00%> (ø)`
crates/btcio/src/writer/test_utils.rs	`100.00% <100.00%> (ø)`
crates/db/src/traits.rs	`0.00% <ø> (ø)`
crates/l1tx/src/envelope/builder.rs	`100.00% <100.00%> (+20.00%)`	⬆️
crates/l1tx/src/utils.rs	`85.48% <100.00%> (-9.60%)`	⬇️
crates/primitives/src/l1/payload.rs	`24.65% <ø> (+8.21%)`	⬆️
crates/proof-impl/btc-blockspace/src/filter.rs	`50.00% <100.00%> (ø)`
crates/consensus-logic/src/duty/worker.rs	`0.00% <0.00%> (ø)`
... and 9 more

... and 6 files with indirect coverage changes

delbonis

This is a good start but what I would like to see is a new RPC that lets us test this new functionality properly and more explicit functional tests for it. I don't think enough is sufficiently covered.

This also doesn't get to replicating the visitor-based stuff in the old PR, but that wasn't quite ready yet and we're not going to be in a position with tn1 to be able to take advantage of it yet, so I'm not sure if it's worth trying to work on here. What do you think?

delbonis · 2025-01-17T17:16:03Z

crates/consensus-logic/src/duty/worker.rs

@@ -410,7 +410,7 @@ fn perform_duty<D: Database, E: ExecEngineCtl>(
            Ok(())
        }
        Duty::CommitBatch(data) => {
-            info!(data = ?data, "commit batch");
+            info!(data = ?data, "commit batch duty");


?data

But really we should not print the contents here, that's unnecessarily verbose, especially for an info! log.

True. Just logged the idx.

delbonis · 2025-01-17T17:22:57Z

crates/l1tx/src/envelope/builder.rs

+    trace!(batchdata_size = %payload.data().len(), "Inserting batch data");
+    for chunk in payload.data().chunks(520) {
        trace!(size=%chunk.len(), "inserting chunk");


This is way into the internals. We just have extensive-enough unit tests it so that we never have to worry about if it's correct or not.

Logs are most useful to have closer to the boundaries of components or for high level components doing significant operations the user might want to know about, where we might have to worry about unexpected interactions we want to go look at later.

Agreed. Removed the trace here.

delbonis · 2025-01-17T17:25:42Z

crates/l1tx/src/envelope/parser.rs

+}
+
+fn parse_payload_type(bytes: &[u8], params: &RollupParams) -> Option<L1PayloadType> {
+    let str = from_utf8(bytes).ok()?;


Instead of trying to parse the bytes as UTF-8 first, interpret the strings we're comparing against as bytes and compare that like tag_bytes == params.checkpoint_tag.as_bytes().

Also, don't like var names that shadow type names, would have called this tag_str to also be more explicit.

That's a good point. Changed to checking bytes. Conversion to string was really unnecessary.

bewakes · 2025-01-20T07:16:06Z

but what I would like to see is a new RPC that lets us test this new functionality properly and more explicit functional tests for it. I don't think enough is sufficiently covered.

I see what you are trying to say here. What we don't have exactly is testing of creation of envelopes and accepting them by the client. However, the unit tests do cover parsing multiple envelopes and getting ProtocolTxOps out of a transaction. There are functional tests that do cover parsing a checkpoint(the only protocol op that rollup posts to da now). So I think it is sufficient for what functionalities we have now. We could add tests here by introducing new RPCs, but since we also don't actually have multiple envelopes being created, maybe it makes more sense to have those func tests then?

This also doesn't get to replicating the visitor-based stuff in the old PR

Yes, Initially I was thinking I would do it here, but since this is getting bigger and another ticket also touches on this, it might be better to do it there(or split that one also if necessary).
Also, regarding the visitor stuff, I am not sure if I accurately follow how it is going to help. Here's the summary of what I understand and how I am thinking of implementing(in next PR) on a very high level, the problem we're trying to solve being "ability to store da data in separate db for rollup nodes and just keeping the da commitment in chain/protocol operation".

enum RawProtocolOp {
  Da(vec<u8>), // This has all of the DA data
  Checkpoint(CheckpointData),
  Deposit(..),
  DepositInfo(..),
}

// Actual operations, which prover cares and is in the chain
enum ProtocolOp {
  Da(vec<u8>), // This only has commitment, others will probably stay the same
  Checkpoint(CheckpointData),
  Deposit(..),
  DepositInfo(..),
}

// This takes a fn impl so any extra logic like storing raw data to db can be done 
// there and pass a no op otherwise.
fn process_raw_op(
  d: RawProtocolOp,
  f: impl FnOnce(RawProtocolOp) -> Result<()>
) -> Result<L1ProtocolOp> {
   let txop = to_tx_op(&d);
   f(d)?;
   Ok(txop)
}

Which will be used in prover and full node as:

fn extract_tx_ops_prover(tx: Transasction, params: Params, ..) -> Vec<L1ProtocolOp> {
   let raw_ops = extract_raw_protocol_ops(tx, params);

   raw_ops.map(|r| process_raw_op(r, |_| Ok(()))).collect()
}

fn extract_tx_ops_client(tx: Transasction, params: Params, db: Manager) -> Vec<L1ProtocolOp> {
   let raw_ops = extract_raw_protocol_ops(tx, params);

   raw_ops.map(|r| process_raw_op(r, |d| db.insert(d)).collect()
}

storopoli

minor nits and comments

storopoli · 2025-01-20T12:32:04Z

crates/btcio/src/writer/builder.rs

@@ -52,7 +52,7 @@ pub enum EnvelopeError {
 // dependencies on `tx-parser`, we include {btcio, feature="strata_test_utils"} , so cyclic
 // dependency doesn't happen
 pub async fn build_envelope_txs<W: WriterRpc>(
-    payload: &L1Payload,
+    payloads: &[L1Payload],


One thing that we can do is to make this accept both a single but also multiple LaPayloads:

pub async fn build_envelope_txs<'a, I: IntoIterator<Item = &'a L1Payload>, W: WriterRpc>( payload: I, //...

storopoli · 2025-01-20T12:33:34Z

crates/btcio/src/writer/bundler.rs

+/// Periodically bundles unbundled intents into payload entries.
+pub(crate) async fn bundler_task(ops: Arc<EnvelopeDataOps>) -> anyhow::Result<()> {
+    let mut last_idx = 0;
+    loop {


Ugh, this loop does not have a break.
I know that it is supposed to loop forever until the process is terminated, but still "code smells" for me.

storopoli · 2025-01-20T12:35:06Z

crates/db/src/traits.rs

-/// [`PayloadEntry`]
-pub trait SequencerDatabase {
-    type L1PayloadDB: L1PayloadDatabase;
+/// A trait encapsulating provider and store traits to create/update [`BundledPayloadEntry`] in the


Suggested change

/// A trait encapsulating provider and store traits to create/update [`BundledPayloadEntry`] in the

/// Encapsulates provider and store traits to create/update [`BundledPayloadEntry`] in the

It is a docstring to a "trait" hence it is implicit.

storopoli · 2025-01-20T12:38:50Z

crates/l1tx/src/envelope/parser.rs

-        }
-        _ => Err(EnvelopeParseError::InvalidNameTag),
-    }?;
+fn parse_l1_payload(


This function and the parse_payload_type below are awesome candidates for fuzzing.
Even if you just run locally (and don't need to setup CI, infra etc) it can pay huge dividends. Caught a bunch of simple and trivial bugs in as least as 5 min running in bitcoin-bosd thanks to @AaronFeickert (no action needed).

storopoli · 2025-01-20T12:40:33Z

crates/l1tx/src/filter.rs

-    // completely valid control block
-    fn create_envelope_tx(rollup_name: String) -> Transaction {
+    // completely valid control block. Includes `n_envelopes` envelopes in the tapscript.
+    fn create_checkpoint_envelope_tx(params: Arc<Params>, n_envelopes: u32) -> Transaction {


Since this is a library crate, I have to ask if it is really necessary to Arc<Params> here instead of &Params and then Arc'ing + derefing in any binary crate that consumes it?

bewakes force-pushed the STR-807-inscription-changes branch 3 times, most recently from b5e3480 to 286799a Compare January 7, 2025 12:15

john-light changed the title ~~Str 807 inscription changes~~ STR-881 inscription changes Jan 8, 2025

delbonis mentioned this pull request Jan 8, 2025

Refactor/btcio config and other renamings #595

Merged

13 tasks

bewakes force-pushed the STR-807-inscription-changes branch from 286799a to 9e770e7 Compare January 9, 2025 11:38

bewakes changed the base branch from main to refactor/btcio-config January 9, 2025 11:39

bewakes force-pushed the refactor/btcio-config branch 5 times, most recently from 54e228e to 7584ba8 Compare January 13, 2025 08:49

bewakes force-pushed the STR-807-inscription-changes branch from 9e770e7 to 16a2890 Compare January 13, 2025 12:23

bewakes force-pushed the refactor/btcio-config branch 2 times, most recently from 07fc70b to cc85da7 Compare January 14, 2025 09:29

bewakes force-pushed the STR-807-inscription-changes branch from 16a2890 to 0528eff Compare January 14, 2025 11:18

bewakes force-pushed the refactor/btcio-config branch from cc85da7 to 2eda45d Compare January 15, 2025 08:34

Base automatically changed from refactor/btcio-config to main January 15, 2025 09:31

bewakes force-pushed the STR-807-inscription-changes branch from 0528eff to 727ef1a Compare January 15, 2025 10:58

bewakes force-pushed the STR-807-inscription-changes branch 2 times, most recently from 1fbb9a5 to 037c18f Compare January 17, 2025 13:06

bewakes marked this pull request as ready for review January 17, 2025 13:11

bewakes requested review from a team as code owners January 17, 2025 13:11

bewakes requested a review from voidash January 17, 2025 13:11

delbonis requested changes Jan 17, 2025

View reviewed changes

bewakes force-pushed the STR-807-inscription-changes branch from 037c18f to 869ffec Compare January 20, 2025 08:05

Bibek Pandey added 14 commits January 20, 2025 14:13

Some cleanups

b015b62

l1tx: Use tags to parse and build l1 payloads

6dfb350

db: Add IntentEntry and corrensponding db operations, and tests

eb4a63f

Support multiple payloads as envelopes

934a2b9

DB changes for payload entries

2396c1e

Remove sequencerdb

1f0a5e0

btcio: Add payload bundler

26dd767

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

f7a01db

Fix bug around intent not being processed

88a90e3

Make unit tests compile

7f7893f

Get tests to pass

3940b67

Rename sequencer to writer in db modules

afc473c

Update tests

6f67c08

Actually parse multiple envelopes

7a80b35

bewakes force-pushed the STR-807-inscription-changes branch from 869ffec to da8be4c Compare January 20, 2025 08:28

Some renamings

e78e71d

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

storopoli requested changes Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STR-881 inscription changes #590

STR-881 inscription changes #590

bewakes commented Jan 6, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading

delbonis left a comment

delbonis Jan 17, 2025

bewakes Jan 20, 2025

delbonis Jan 17, 2025

bewakes Jan 20, 2025

delbonis Jan 17, 2025

bewakes Jan 20, 2025 •

edited

Loading

bewakes commented Jan 20, 2025 •

edited

Loading

storopoli left a comment

storopoli Jan 20, 2025

storopoli Jan 20, 2025

storopoli Jan 20, 2025

storopoli Jan 20, 2025

storopoli Jan 20, 2025

	/// A trait encapsulating provider and store traits to create/update [`BundledPayloadEntry`] in the
	/// Encapsulates provider and store traits to create/update [`BundledPayloadEntry`] in the

STR-881 inscription changes #590

Are you sure you want to change the base?

STR-881 inscription changes #590

Conversation

bewakes commented Jan 6, 2025 • edited Loading

Type of Change

Notes to Reviewers

Checklist

Related Issues

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

codecov bot commented Jan 17, 2025 • edited Loading

Codecov Report

delbonis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bewakes Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

bewakes commented Jan 20, 2025 • edited Loading

storopoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bewakes commented Jan 6, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading

bewakes Jan 20, 2025 •

edited

Loading

bewakes commented Jan 20, 2025 •

edited

Loading