feat: blob staleness check #226

samlaf · 2025-01-08T22:32:40Z

We add a check to discard blobs that have been included in the rollup's batcher inbox a long time after it was included in an EigenDA batch by the eigenda disperser.

It is now expected that GET requests to the proxy can include an optional l1_inclusion_block_number query argument, which will cause a blob to be rejected if it was included onchain > RollupBlobInclusionWindow L1 blocks after the blob's batch's RBN.

The RollupBlobInclusionWindow is a flag added to the proxy binary, which is set to 0 by default, which skips the check.

Fixes Issue

Fixes #

Changes proposed

Screenshots (Optional)

Note to reviewers

Commits are:

8eaef49: contains the main feature logic
a3b6ca4: test the query param parsing logic
0cf0e78: test the verification logic (also includes refactoring the CertVerifier to an interface to be able to test other verification that doesnt need an eth rpc connection)

epociask · 2025-01-09T07:20:50Z

common/store.go

+	// The eigenDA batch header contains a reference block number (RBN) which is used to pin the stake of the eigenda operators at that specific blocks.
+	// The rollup batch containing the eigenDA cert is only valid if it was included within a certain number of blocks after the RBN.
+	// validity condition is: RBN < l1_inclusion_block_number < RBN + some_delta
+	RollupL1InclusionBlockNum int64


shouldn't this be uint64?

I'm using -1 to mean that this query param was not passed, and hence to skip the test. Could using a pointer to uint64 instead and use nil to mean this? I don't like either tbh... but go is dumb.

Can't you use 0 to indicate "skip the test"?

RollupL1InclusionBlockNum must be strictly greater RBN, and the min value of RBN is 0, therefore RollupL1InclusionBlockNum == 0 isn't sensical, and is free realestate to have special meaning

Hmm that's a good point! 2 questions to answer here:

do we really want < or should I have been using <= ? Don't think it makes a lot of sense for RBN to be == l1_inclusion_block_number but I think its technically possible, especially on a devnet?

if we have ==, perhaps its still fine to use 0 because I don't think txs are allowed in the genesis block (although apparently bitcoin allows it? so I'm not sure.... would an op L2 ever have a batcher tx in genesis block? that doesn't make a lot of sense but what if...

but I think its technically possible, especially on a devnet

I don't see how this would be possible

The eigenDA cert directly contains the RBN, so you can't honestly construct the cert until the reference block exists. And if the RB already exists, how could the eigenDA cert be put on chain any earlier than RBN+1?

epociask · 2025-01-09T07:23:16Z

verify/cert.go

 // to ensure disperser returned fields haven't been tampered with
-type CertVerifier struct {
+type CertVerification struct {


knit - any reason to make this public if we're binding to an interface?

Not really, I can change. Don't think using an interface matters here though, it's more about whether we think people will want to import and use this struct elsewhere.

epociask · 2025-01-09T07:24:54Z

verify/verifier.go

+	// If batch.RBN + rollupBlobInclusionWindow < cert.L1InclusionBlock, the batch is considered stale and verification will fail.
+	// This check is optional and will be skipped when rollupBlobInclusionWindow is set to 0.
+	// Note: if there are more rollup related properties that we need to check in the future, then maybe create a RollupVerifier struct
+	rollupBlobInclusionWindow uint32


wouldn't this need to be globally set per the rollup? Otherwise couldn't you have multiple verifiers articulating alternative views due to different values?

Yes that's a good point. I wrote the code out but now's the time to discuss how we want to handle this feature. cc @bxue-l2 for discussion. I'll at least add a bit WARNING comment to the flag, saying that the feature is experimental and not really meant to be turned on? But then what's our plan for eventually turning it on?

yes, this require an upgrade process like what op currently has. not very good. Alternatively, we can do it inside the op-node derivation pipeline, but we have the burden to keep our fork up to date

we can put the staleness gap in the cert, signed by DA network

epociask · 2025-01-09T07:26:13Z

verify/verifier.go

 	// 1 - verify batch in the cert is confirmed onchain
 	err := v.cv.verifyBatchConfirmedOnChain(ctx, cert.Proof().GetBatchId(), cert.Proof().GetBatchMetadata())
 	if err != nil {
 		return fmt.Errorf("failed to verify batch: %w", err)
 	}

-	// 2 - verify merkle inclusion proof
+	// 2 - verify that the blob cert was submitted to the rollup's batch inbox within the allowed RollupBlobInclusionWindow window.
+	// This is to prevent timing attacks where a rollup batcher could try to game the fraud proof window by including an old DA blob that is about to expire on the DA layer


how will this verification look when one step proving the certificate? ie, will a challenger need to provide the inbox confirmation tx block #, if so then how do they do so in a way that isn't tamper resistant?

Right precisely the right question to ask. Discussed this at length with Bowen. I'll need to add comments detailing this.

The 2 options for securing this check is:

do it in derivation pipeline

do it in preimage oracle contract (as you mention, this would require a zk proof that the inclusion block # is correct)

We are opting for 1, to do this check in the derivation pipeline. It's a simple check to make in the rust hokulea client, so we'll add it there. Ideally we'd add the same check to the golang derivation pipeline, but the problem is that we haven't had much success lately getting our PRs upstreamed. Also if ever a problem happens and we need to change the logic in the golang binary then we depend on optimism's team to review and merge our PR, which feels less than ideal (this is different from the rust code b/c in rust we have our own Hokulea library which projects using eigenda can import directly, and which we have full control over).

So the idea is that the rust stack will be a requirement to do the fraud proofs. But we still want normal validators running op-node to be able to follow the chain properly, which is why we are adding this check here in the proxy.

cc @bxue-l2 for any more thoughts.

Currently for op, the check is done inside the client program, and derivation pipeline provide the correct block Info. See hokulea code here.

I think it worths some discussion, and tbh didn't know sam made a lot of progress. Nice Sam. I thought you are debugging the bug all the times. I am down to have a meeting about this.

epociask · 2025-01-09T07:27:26Z

mocks/manager.go

+//
+// Generated by this command:
+//
+//	mockgen -package mocks --destination ../mocks/manager.go . IManager
+//


There's also make go-gen-mocks

I did use make go-gen-mocks. Not sure why this repo was setup to use the deprecated go mock library instead of the new uber mock repo. Updated that dependency in go.mod.

Created this issue to make a reproducible dev env: #236

common/store.go

litt3 · 2025-01-09T15:31:38Z

verify/verifier.go

+		if !(blockNumEigenDA < blockNumRollup) {
+			return fmt.Errorf("eigenda batch reference block number (%d) needs to be < rollup inclusion block number (%d)", blockNumEigenDA, blockNumRollup)
+		}
+		if !(blockNumRollup < blockNumEigenDA+int64(v.rollupBlobInclusionWindow)) {


I believe you have an off-by-one error here

The comment in the config struct above states that the following constitutes a stale batch:

batch.RBN + RollupBlobInclusionWindow < cert.L1InclusionBlock

but it seems what's actually implemented is this:

batch.RBN + RollupBlobInclusionWindow <= cert.L1InclusionBlock

It might help to have consistent variable names between the two.

I think it was doing < everywhere? But I agree it should be <=, so changed. Also updates comments and made naming consistent: 72451c6

The discrepancy was in the config doc:

// If batch.RBN + RollupBlobInclusionWindow < cert.L1InclusionBlock, the batch is considered stale and verification will fail.

Since this doc is defining what constitutes staleness, the implied invariant would be the inverse

cert.L1InclusionBlock <= batch.RBN + RollupBlobInclusionWindow

Your most recent changeset has things correct, since you modified the invariant, but left the staleness definition in the Config struct unchanged.

verify/verifier.go

We add a check to discard blobs that have been included in the rollup's batcher inbox a long time after it was included in an EigenDA batch by the eigenda disperser.

…om GET queries is correctly parsed

Made the Verifier take an interface for the certVerifier instead of the struct, to allow me to test functionality that didn't depend on having a blockchain connection (like the certVerifier does). This way we can test the blob staleness check by using a noopCertVerifier to skip all the other verification that require an eth rpc.

samlaf · 2025-01-10T21:24:08Z

rebased on top of master to get the latest change (was one minor conflict where verifier's Verify function had change the argument name from expectedSomething to certCommitment)

Also fixed comments and naming to be more consistent

litt3 · 2025-01-13T15:03:58Z

verify/cert.go

+}
+
+// NoopCertVerifier is used in place of CertVerification when certificate verification is disabled
+type NoopCertVerifier struct{}


Any reason you left this public?

litt3 · 2025-01-13T15:48:09Z

verify/verifier.go

+		batchRBN := int64(cert.BlobVerificationProof.BatchMetadata.BatchHeader.ReferenceBlockNumber)
+		rollupInclusionBlock := args.RollupL1InclusionBlockNum
+		// We need batchRBN < rollupInclusionBlock <= batch.RBN + rollupBlobInclusionWindow
+		// TODO: should we be using <= <= instead?


IMO, no. See my comment above, I don't think RBN == inclusion block is sensical

samlaf marked this pull request as draft January 8, 2025 22:32

samlaf marked this pull request as ready for review January 9, 2025 02:11

samlaf requested review from epociask, litt3 and bxue-l2 January 9, 2025 02:11

epociask reviewed Jan 9, 2025

View reviewed changes

litt3 reviewed Jan 9, 2025

View reviewed changes

samlaf added 7 commits January 10, 2025 16:23

feat: blob staleness check

9f64716

We add a check to discard blobs that have been included in the rollup's batcher inbox a long time after it was included in an EigenDA batch by the eigenda disperser.

chore: fix lint issues

88d6fde

test: add test to check that l1_inclusion_block_number query param fr…

55b91bd

…om GET queries is correctly parsed

test: fix unit tests (was passing nil log pointer)

4d94a08

style: fix lint issues

a0ca9cb

style: make cert verifier structs/interfaces private

06237d8

samlaf force-pushed the feat--rollup-blob-inclusion-staleness-check branch from bc6d810 to 06237d8 Compare January 10, 2025 21:23

samlaf mentioned this pull request Jan 10, 2025

Create reproducible dev env #236

Open

samlaf added 3 commits January 10, 2025 16:31

style: fix typo blocks -> block

e633096

test: add missing cert verifier test for blob staleness

47ff9ba

style: make blob staleness check use <= instead of <

72451c6

Also fixed comments and naming to be more consistent

samlaf requested review from litt3 and epociask January 10, 2025 23:10

test: fix verifier test

496320e

litt3 reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: blob staleness check #226

feat: blob staleness check #226

samlaf commented Jan 8, 2025 •

edited

Loading

epociask Jan 9, 2025

samlaf Jan 9, 2025

litt3 Jan 9, 2025

samlaf Jan 10, 2025

litt3 Jan 13, 2025

epociask Jan 9, 2025

samlaf Jan 9, 2025

samlaf Jan 10, 2025

epociask Jan 9, 2025

samlaf Jan 9, 2025

bxue-l2 Jan 10, 2025 •

edited

Loading

bxue-l2 Jan 10, 2025

epociask Jan 9, 2025

samlaf Jan 9, 2025

bxue-l2 Jan 10, 2025

epociask Jan 9, 2025

samlaf Jan 10, 2025

litt3 Jan 9, 2025

samlaf Jan 10, 2025

litt3 Jan 13, 2025

samlaf commented Jan 10, 2025

litt3 Jan 13, 2025

litt3 Jan 13, 2025 •

edited

Loading

feat: blob staleness check #226

Are you sure you want to change the base?

feat: blob staleness check #226

Conversation

samlaf commented Jan 8, 2025 • edited Loading

Fixes Issue

Changes proposed

Screenshots (Optional)

Note to reviewers

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

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

bxue-l2 Jan 10, 2025 • edited Loading

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

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

samlaf commented Jan 10, 2025

Choose a reason for hiding this comment

litt3 Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

samlaf commented Jan 8, 2025 •

edited

Loading

bxue-l2 Jan 10, 2025 •

edited

Loading

litt3 Jan 13, 2025 •

edited

Loading