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

Separate out the DA client into a separate binary #2533

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ganeshvanahalli
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli commented Jul 30, 2024

This PR introduces daclient library and --da-provider config option that can be used to connect a node to any external DA service that implements the unified reader and writer interfaces while adhering to the response formats, ...Result structs, available in /daprovider/daclient/daclient.go.

We also separate out the current data availability implementation (das) into a new binary daprovider that can be used to start a das-server which can in turn be used as a daprovider option above. To maintain backwards compatibility a nitro node does the above step by default when --data-availability config is provided.
(note: da-provider and data-availability cannot be enabled together`)

Thus enabling da providers to efficiently connect to nitro without having to alter any of the source code.

Below is the diagram showing multiple components and their working involved in this PR-
Screenshot 2024-08-07 at 3 04 44 PM

Resolves NIT-1158

@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 Jul 30, 2024
@ganeshvanahalli ganeshvanahalli marked this pull request as ready for review July 30, 2024 13:34
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

Looks good! Just minor comments.

cmd/daprovider/dasserver/dasserver.go Outdated Show resolved Hide resolved

// RecordPreimagesTo takes in preimages map and returns a function that can be used
// In recording (hash,preimage) key value pairs into preimages map, when fetching payload through RecoverPayloadFromBatch
func RecordPreimagesTo(preimages map[arbutil.PreimageType]map[common.Hash][]byte) PreimageRecorder {
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this long type map[arbutil.PreimageType]map[common.Hash] in multiple places. Maybe create a type alias for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a good idea and have aliased this as PreimagesMap for now. I don't foresee any issue with this but flagging this to @Tristan-Wilson to double check

@Tristan-Wilson
Copy link
Member

I think I need some help understanding this PR, can you make some diagrams showing the relations between the components and what some typical configurations would look like?

@ganeshvanahalli
Copy link
Contributor Author

I think I need some help understanding this PR, can you make some diagrams showing the relations between the components and what some typical configurations would look like?

I've added the diagram showing the relations between components to the title comment.

@@ -0,0 +1,216 @@
package main
Copy link
Contributor

Choose a reason for hiding this comment

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

cmd/ should have only small things that are only relevant to the binaries.
so this file makes sense here, but the "das" and "dasserver" subdirs should be moved somewhere else.

I think I do like the fact you removed it from "arbstate".. maybe under "/daprovider" or somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I also moved dasserver inside das directory since it made more sense to be placed in there

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.

You'll need to add --rpc-server-body-limit which is present in daserver to daprovider to allow it to handle batch bodies larger than 5MB.

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.

In this PR the batch poster is trusting the response of the daprovider, authenticated by JWT. Can we require that #2371 is enabled if daprovider is used for extra assurance?

Comment on lines +563 to 567
dasServer, closeFn, err := dasserver.NewServer(ctx, &serverConfig, dataSigner, l1client, l1Reader, deployInfo.SequencerInbox)
if err != nil {
return nil, err
}

Copy link
Member

Choose a reason for hiding this comment

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

If this is being initialized in the same process what's the point of communicating over TCP at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the reasoning here is we would like to demonstrate that the current design and implementation works.

Redirecting to @PlasmaPower for confirmation.

daprovider/daclient/daclient.go Show resolved Hide resolved
daprovider/das/dasserver/dasserver.go Show resolved Hide resolved
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.

4 participants