-
Notifications
You must be signed in to change notification settings - Fork 638
Add DA proof support to daprovider interface #3600
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
base: master
Are you sure you want to change the base?
Conversation
This is part of a series of changes for the Custom DA project. Summary: - Added a new daprovider.Validator interface with methods for generating proofs for Custom DA systems. - Added a reference implementation of a Custom DA provider. - Added a daprovider factory that supports anytrust and referenceda modes. In a follow-up PR the nitro node startup sequence will be modified to use this. Currently only the separate daprovider server uses this. - Replaced the AnyTrust-specific (aka das) provider server with a unified provider server that works with anytrust or referenceda modes. - Extended the DA Client with new RPC methods for generating proofs. Notes: The separate provider server executable is a thin RPC server wrapper around the anytrust and referenceda implementations. The idea is that people wanting to integrate their own DA system can use this as a guide for how to implement their own RPC service that lives outside the nitro codebase; we won't be including support for any additional DA implementations in provider server executable that we distribute. For legacy AnyTrust deployments we will most likely continue to have nitro spawn the daprovider server in-process to avoid needing to run an extra service, but by channeling everything through the JSON-RPC interface it reduces surface area of what we have to support. The Reference DA (referenceda) implementation is a minimal working example of how one could implement a daprovider, including support for validating the certificate against trusted signers. It uses an in-memory data storage backend. In various places there is commented out code related to the osp contract bindings that haven't yet been committed to a nitro-contracts branch that we want to use in nitro master. This PR shouldn't change any existing functionality or behavior, except the daprovider executable (which isn't used in production) has some new configuration options: ``` --mode must be "anytrust" or "referenceda" --provider-server.* server config eg address, port, etc --anytrust.* was previosly called das-server --referenceda.* referenceda specific options ``` As much as possible we will try to rename references to "das" to "anytrust". When we launched Anytrust, we only had one offchain data availability mode so we just called it "das" at the time. This PR doesn't include new test code, but testing was done with the end-to-end block validator and challenge system tests on the custom-da branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
daprovider/util.go
Outdated
|
||
// KnownHeaderBits is all header bits with known meaning to this nitro version | ||
const KnownHeaderBits byte = DASMessageHeaderFlag | TreeDASMessageHeaderFlag | L1AuthenticatedMessageHeaderFlag | ZeroheavyMessageHeaderFlag | BlobHashesHeaderFlag | BrotliMessageHeaderByte | ||
const KnownHeaderBits byte = DASMessageHeaderFlag | TreeDASMessageHeaderFlag | L1AuthenticatedMessageHeaderFlag | ZeroheavyMessageHeaderFlag | BlobHashesHeaderFlag | BrotliMessageHeaderByte | DACertificateMessageHeaderFlag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes behavior of replay.wasm.
It should either only be added later - or the current change should modify replay.wasm to specifically exclude DACertificateMessageHeaderFlag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I agree the best thing to do seems to be to take it out of the known headers for now.
writer: writer, | ||
validator: validator, | ||
} | ||
if err = rpcServer.RegisterName("daprovider", server); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have 3 separate interfaces (reader/writer/validator) and each has their own rpc functions - it might make sense to have 3 separate provider server objects - one for wrapping each interface - and to have here 3 separate calls to rpcServer.RegisterName - even if all 3 objects register to the same namespace.
Not a must.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't done this yet but it should be an easy enough change. Either tomorrow or in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm already dreading the merge conflicts of this PR back to custom-da especially around provider_server so I think I'll defer this change til after this is merged to master, and then master is merged back to custom-da.
daprovider/validator.go
Outdated
// The proof format depends on the implementation and must be compatible with the Solidity | ||
// IDACertificateValidator contract. | ||
// certHash is the keccak256 hash of the certificate. | ||
GenerateReadPreimageProof(ctx context.Context, preimageType arbutil.PreimageType, certHash common.Hash, offset uint64, certificate []byte) ([]byte, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For calls that will be wrapped by rpc calls - please return Promises. See for example validator/interface.go
promises make it more visible in code that a function call could take long, and they handle context well on both sides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done now for all methods except Store, which is being heavily reworked on the custom-da branch and we can do later.
We will add DACertificateMessageHeaderFlag back to KnownHeaderBits in a future PR; removing it for now avoids changing the replay binary unnecessarily when merging in this PR.
daprovider.Readers are now registered with their respective header bytes at initialization. For external DA providers where the RPC client is used this becomes single call at init to daprovider_getSupportedHeaderBytes. Where previously in the inbox and block validator components we had loops over each provider, checking each provider if they handle that type of message, we now just check if we have a provider registered for that message. This means less RPC calls and makes the interface that providers need to implement a bit simpler, with room for taking it out completely for CustomDA if we can detect from other config that the external provider is expected to be a CustomDA provider.
We moved the contents of dasserver to provider_server on the custom-da branch and I accidentally left dasserver in place on this branch when moving things across. I don't want to bring over the changes to the daprovider construction logic in node.go yet so I just made the das_migration.go file which we'll delete later.
In normal execution we only care about the payload, and in validation we only care about the preimages (we will reconstruct the payload using the preimages). This change separates these concerns and simplifies the signatures of these methods.
For DAS and ReferenceDA we almost always want to validate the certificate. The only time we don't is in the replay binary when we want to panic instead if it's invalid on the first read of the message, otherwise we want to ignore it and assume it's already valid. So this means that it doesn't make sense for validateSeqMsg to be part of the reader API at all, which is great because it means we can simplify the API. We can pass into the reader at construction time how to handle this because in replay it always constructs a new (fake, non-network calling) reader for each invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looking good. Just getting initial comments out.
Have not yer reviewed:
cmd/daprovider
daprovider/factore
daprovider/referenceda
daprovider/server/das_migrations.go
daprovider/server/provider_server
daprovider/reader.go
Outdated
batchBlockHash common.Hash, | ||
sequencerMsg []byte, | ||
) containers.PromiseInterface[PayloadResult] { | ||
promise := containers.NewPromise[PayloadResult](nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the canonical way (which we should make easier) is something like:
ctx, cancel: context.WithCancel(context.Background)
promise := NewPromise[..](cancel)
and then use that ctx in the go thread..
Not critical for now, but should open a different task to handle that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should create a thin wrapper fot those two lines, create a func that gets parent context and returns promise and promise-context (the promise-context being cancellable with promise's cancel)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're using Promise incorrectly in a lot of places in the codebase (passing in a nil cancel func). I made a new function NewPromiseWithContext
as you described and used in the places where this PR uses Promises
. We can do a separate change on master later to use it elsewhere.
dapReaders = append(dapReaders, daprovider.NewReaderForBlobReader(&simulatedBlobReader{kzgBlobs})) | ||
if err := dapReaders.SetupBlobReader(daprovider.NewReaderForBlobReader(&simulatedBlobReader{kzgBlobs})); err != nil { | ||
return false, fmt.Errorf("failed to register simulated blob reader: %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to create a new registry and not just use b.dapReaders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When batch correctness checking is enabled and we're using 4844 we need to use a simulated blob reader because the blobs have not been posted yet. We don't want to register the simulated reader with the main registry used by the batch poster, so we create a copy of it.
I had missed that I needed to exclude copying the normal blob reader to the new registry, I added that now. Registering another reader for the same header byte is a runtime error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a later PR: we can implement a dapReaders-wrapper that returns it's own fake-blobreader for blob header, and passes through any other function to the wrapped dasReader.
This will be simpler and easier to understand.
// Copyright 2024-2025, Offchain Labs, Inc. | ||
// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md | ||
|
||
package dapserver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better name the dir dapserver as well, to prevent having to rename on import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we defer this change til after this PR gets merged? Further renamings are annoying to merge back into the big custom-da PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
daprovider/daclient/daclient.go
Outdated
if err := c.CallContext(ctx, &isValidHeaderByteResult, "daprovider_isValidHeaderByte", headerByte); err != nil { | ||
log.Error("Error returned from daprovider_isValidHeaderByte rpc method, defaulting to result as false", "err", err) | ||
return false | ||
func (c *Client) GetSupportedHeaderBytes(ctx context.Context) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this one should also return a promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(instead of getting a ctx parameter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to use Promise
// Copyright 2024-2025, Offchain Labs, Inc. | ||
// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md | ||
|
||
package daprovider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the interface can live in the parent directory "daprovider", since it's just an interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you left a comment on the wrong file here? It's already in daprovider. In any case, I'd like to defer further moves/renamings til after this gets merged unless they're critical.
❌ 12 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
many refactor suggestions, but those can be done in a separate PR to keep merge overhead down.
Also - I'm not done going over referenceDA but comments there should be easy to fix later if needed
l1Client *ethclient.Client | ||
} | ||
|
||
func NewDAProviderFactory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this adds more complexity then clarity.
It's a little hard to chase around.
I think a "CreateReferenceDAFactory in referenceda dir, and "CreateAnyTrustFactory" in anytrust package (or actually naming both just "CreateFactory" and using package name to tell them apart) would be clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
especially since you already need the sub-packages with the configs inside them to create the factory.
} | ||
|
||
type AnyTrustFactory struct { | ||
config *das.DataAvailabilityConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this config (and some more code) should be moved from "das.DataAvailabilityConfig" to a separate anytrust package and become something like anytrust.Config?
// - *http.Server: The HTTP server instance | ||
// - func(): Cleanup function to stop the DAS lifecycle manager | ||
// - error: Any error that occurred during initialization | ||
func NewServerForDAS( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU: the name "anytrust" also belongs somewhere here and/or in the package?
"github.com/offchainlabs/nitro/util/signature" | ||
) | ||
|
||
// Certificate represents a ReferenceDA certificate with signature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth adding a small comment explaining what the referenceDA does
} | ||
|
||
// Validate certificate - always validate for ReferenceDA | ||
// TODO: Uncomment the following once we have merged customda contracts changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add the ReferenceDA contract in contract-local, which is used for testing?
I know we need changes in the contracts as well, but the referenceDA should probably only be here.
sequencerMsg []byte, | ||
) containers.PromiseInterface[daprovider.PreimagesResult] { | ||
promise, ctx := containers.NewPromiseWithContext[daprovider.PreimagesResult](context.Background()) | ||
go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I think we can also encapsulate further with something like
func LaunchPromisethread[T any](parent context.Context, threadfunc func(ctx context) (T, error)) *PromiseInterface[T]
not urgent, and we could keep both :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very much like stopwaiter.LaunchPromiseThread, but without the stopwaiter encapsulation
This is part of a series of changes for the Custom DA project.
Summary:
Notes: The separate provider server executable is a thin RPC server wrapper around the anytrust and referenceda implementations. The idea is that people wanting to integrate their own DA system can use this as a guide for how to implement their own RPC service that lives outside the nitro codebase; we won't be including support for any additional DA implementations in provider server executable that we distribute. For legacy AnyTrust deployments we will most likely continue to have nitro spawn the daprovider server in-process to avoid needing to run an extra service, but by channeling everything through the JSON-RPC interface it reduces surface area of what we have to support.
The Reference DA (referenceda) implementation is a minimal working example of how one could implement a daprovider, including support for validating the certificate against trusted signers. It uses an in-memory data storage backend.
In various places there is commented out code related to the osp contract bindings that haven't yet been committed to a nitro-contracts branch that we want to use in nitro master.
This PR shouldn't change any existing functionality or behavior, except the daprovider executable (which isn't used in production) has some new configuration options:
As much as possible we will try to rename references to "das" to "anytrust". When we launched Anytrust, we only had one offchain data availability mode so we just called it "das" at the time.
This PR doesn't include new test code, but testing was done with the end-to-end block validator and challenge system tests on the custom-da branch.