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

Update internal data structures #732

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

Conversation

mooselumph
Copy link
Contributor

@mooselumph mooselumph commented Aug 29, 2024

Why are these changes needed?

Internal renaming of data structures in support of planned interface updates and unified blob lookup scheme.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

lgtm, there are still references calling blob certificates as blob headers, which could be confusing. Do you think there's any issues with updating the references as part of this refactor?

core/serialization.go Outdated Show resolved Hide resolved
api/clients/mock/node_client.go Outdated Show resolved Hide resolved
api/clients/node_client.go Outdated Show resolved Hide resolved
api/clients/node_client.go Outdated Show resolved Hide resolved
core/assignment.go Outdated Show resolved Hide resolved
core/data.go Outdated
// Signing the combination of the Nonce and the BlobCommitments prohibits the disperser from
// using the signature to charge the user for a different blob or for dispersing the same blob
// multiple times (Replay attack).
type BlobAuthHeader struct {
type BlobHeader struct {
// Commitments
encoding.BlobCommitments `json:"commitments"`
Copy link
Contributor

Choose a reason for hiding this comment

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

For the purpose of auth, is the kzg commitment itself sufficient (i.e. the 3 length related fields are not needed)?

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 length is needed for payment purposes, but the length proof can probably be ommitted. I've made some updates to the document: https://www.notion.so/eigen-labs/Unified-blob-addressal-in-EigenDA-ae426c610f9e4e61b2e46eec76f7ca4d?pvs=4#ed6c05cfb2e34e3e9289a998cb25dc4d

@@ -184,7 +184,7 @@ func HashBatchHeader(batchHeader binding.IEigenDAServiceManagerBatchHeader) ([32
}

// GetBlobHeaderHash returns the hash of the BlobHeader that is used to sign the Blob
Copy link
Contributor

Choose a reason for hiding this comment

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

Stale comment

@@ -326,15 +324,15 @@ type EncodedBundles map[QuorumID]*ChunksData

// BlobMessage is the message that is sent to DA nodes. It contains the blob header and the associated chunk bundles.
type BlobMessage struct {
BlobHeader *BlobHeader
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the concept of certificate internally? It looked nicer/more intuitive to have blob header + body (chunks/bundle).

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 it's pretty useful to maintain the distinction between the two since they have different purposes. Here it makes sense that it is the certificate because we are asking the DA node to sign the certificate.

That being said, I'm open to other naming suggestions!

@ian-shim
Copy link
Contributor

ian-shim commented Oct 3, 2024

Does the way we ended up building this into a breaking v2 release vs. just interface update in v1 change the approach in this PR?
For example, we don't need the concept of blob cert and nodes keep signing the batch header hash in v1.

@mooselumph
Copy link
Contributor Author

Does the way we ended up building this into a breaking v2 release vs. just interface update in v1 change the approach in this PR? For example, we don't need the concept of blob cert and nodes keep signing the batch header hash in v1.

As we build V2, I think it will be useful to transform the internal data structures to the V2 format as much as possible. Do you have an alternative approach?

Copy link

@hopeyen hopeyen left a comment

Choose a reason for hiding this comment

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

lgtm

header := &core.BlobHeader{
BlobCommitments: encoding.BlobCommitments{
Length: blobLength,
header := &core.BlobCertificate{
Copy link

Choose a reason for hiding this comment

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

maybe cert :=?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants