Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unified writer interface for Data Availability providers [NIT-1249] #2157

Merged
merged 18 commits into from
May 8, 2024

Conversation

ganeshvanahalli
Copy link
Contributor

This PR Introduces a generic writer interface for DA providers to implement

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Feb 23, 2024
@ganeshvanahalli ganeshvanahalli marked this pull request as ready for review February 23, 2024 23:37
}
log.Warn("Falling back to storing data on chain", "err", err)
} else if err != nil {
sequencerMsg, err = b.dapWriter.Store(ctx, sequencerMsg, uint64(time.Now().Add(config.DASRetentionPeriod).Unix()), []byte{}, config.DisableDapFallbackStoreDataOnChain)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unbalanced that daprovider.Writer is used only to write to the DAS and daprovider.Reader is used to read both DAS and blobs. I'm not sure how you'd structure it to be symmetric, though, since on one hand posting a batch to the DAS is a separate operation to posting the batch itself to L1, and with blobs both get posted together with the L1 tx.

Additionally for blob batch posting there is some unique logic around deciding whether or not to post as a 4844 tx or not #2158 which is different to the logic around whether to post a DAS batch or not (falls back on error only), so that also would make it hard to unify.

This is just an observation, we should seek more feedback about this from others on the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I could not find a way to wrap blob posting to follow a common writer interface that other DAs implement.

@ganeshvanahalli ganeshvanahalli changed the title Unified writer interface for Data Availability providers Unified writer interface for Data Availability providers [NIT-1249] Mar 18, 2024
arbstate/daprovider/reader.go Outdated Show resolved Hide resolved
arbstate/daprovider/reader.go Outdated Show resolved Hide resolved
@Tristan-Wilson Tristan-Wilson self-requested a review April 26, 2024 23:15
das/bigcache_storage_service.go Outdated Show resolved Hide resolved
@Tristan-Wilson
Copy link
Member

Tristan-Wilson commented Apr 27, 2024

Might be interesting to look at Celestia's DA impl (celestiaorg@e8c888a) and see if it looks like it would be easy to adapt to this new abstraction. I think the original motivation for this work was to make it easier to plug in new DA impls.

Celestia took the Anytrust code as an example and added another DA system (eg blobs, Anytrust, and now Celestia). The batch poster writes the blob directly to Celestia, and then what gets stored on L1 is an object that contains enough data to retrieve and validate the blob directly from Celestia.

@Tristan-Wilson
Copy link
Member

I read through the Avail DA implementation availproject#8 and it looks like it should be easy to adapt to this interface since they conform to the pattern established in the prior unified reader interface PR #2155

They have already implemented RecoverPayloadFromAvailBatch which will be easily adaptable to the RecoverPayloadFromBatch in the new proposed daprovider.Reader , and likewise with Store in daprovider.Writer

@Tristan-Wilson
Copy link
Member

Likewise Celestia has implemented RecoverPayloadFromCelestiaBatch https://github.com/celestiaorg/nitro/blob/release/arbstate/inbox.go#L391 and a Celestia DA provider https://github.com/celestiaorg/nitro/blob/e8c888a8cf861fad01c33c6adb4f5755398eea54/das/celestia/celestia.go#L51

@Tristan-Wilson
Copy link
Member

Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Tristan-Wilson Tristan-Wilson merged commit 309de61 into master May 8, 2024
9 checks passed
@Tristan-Wilson Tristan-Wilson deleted the unified-daprovider-writer-interface branch May 8, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants