Rename v2 structs #1136

ian-shim · 2025-01-22T07:23:06Z

Why are these changes needed?

Changes:

Data -> Blob from disperser gRPC service
BlobVerificationInfo -> BlobInclusionInfo
Dispersal request signature is moved from BlobHeader to BlobCertificate
Relays -> RelayKeys (in BlobCertificate)
BlobCertificate hashing method (both offchain and onchain) includes Signature

Checks

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.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

0x0aa0 · 2025-01-22T16:38:17Z

contracts/src/interfaces/IEigenDAStructs.sol

Do we also need to rename BlobVerificationProofV2 -> BlobInclusionInfo here?

I thought BlobVerificationProofV2 in this context was fine. But curious if @samlaf has any takes

Am ok to approve this and then have a follow up PR to make further onchain updates

Yes please! Don't mind if its done here or in another PR but ideally all onchain names should match the onchain structs.

api/proto/common/v2/common_v2.proto

jianoaix · 2025-01-22T18:24:52Z

api/proto/disperser/v2/disperser_v2.proto

@@ -35,14 +35,16 @@ message DisperseBlobRequest {
  // significant bits. The integer must stay in the valid range to be interpreted as a field element on the bn254 curve.
  // The valid range is 0 <= x < 21888242871839275222246405745257275088548364400416034343698204186575808495617.
  // If any one of the 32 bytes elements is outside the range, the whole request is deemed as invalid, and rejected.
-  bytes data = 1;
+  bytes blob = 1;


This is encodedPayload not blob per the latest naming definition.

I think we settled on still calling this blob, correct me if im wrong @samlaf

Yes this is actually a blob now. @jianoaix can't be an encodedPayload because encodedPayload can either be interpreted as Eval or Coeffs, whereas a blob is always evaluated as coefficients by disperser:

We mentioned we were going to add an comment explanation that as a network optimization, the blob is allowed to be passed without suffix 0s and the disperser will pad to the next power of 2.

A blob has a power of 2 length, but this one doesn't have to

Reread my last sentence. We agreed that the disperser endpoints accepts "conceptual blobs". Documentation has to be updated to reflect this. But basically to save network bandwidth the disperser allows you to send a blob with the last 0s removed (as long as you don't remove more then half), and itll pad the last zeros to the next power of 2 to turn it into an actual blob.

What you're dispersing is still a blob conceptually.

I'm confused at this: isn't all the effort exactly to bring it from conceptual level to actual semantics level?

If we are happy at conceptual level, there isn't even a need for payload/encodedPayload etc, they are all blobs conceptually anyway. The "what is a blob" confusion is exactly due to that conceptual blob is too vague.

jianoaix · 2025-01-22T18:29:26Z

api/proto/common/v2/common_v2.proto

@@ -25,8 +25,6 @@ message BlobHeader {
  // reserved payments when the same blob is submitted multiple times within the same reservation period. On-demand
  // payments already have unique cumulative_payment values for intentionally unique dispersal requests.
  uint32 salt = 5;
-  // signature over keccak hash of the blob_header that can be verified by blob_header.account_id
-  bytes signature = 6;
 }

 // BlobCertificate contains a full description of a blob and how it is dispersed. Part of the certificate


This comment (a full description of a blob...) gives a good hint about a better naming for BlobCertificate, it's BlobDescriptor. The TBSBlobCertificate is more accurate than BlobCertificate, but BlobDescriptor is further better than TBSBlobCertificate as a name.

Agree this is not 100% accurate naming, but as we move signature from BlobHeader to BlobCertificate, this was more appropriate than before

I don't actually see it's more accurate, this "signature" is signed by the owner of this blob (*), not the validators. A TBSCertificate becomes only after sufficient validators have signed it.

(*) Can we could name it more specifically with a qualification so it's clear and not be confused with validators' signature?

Let's settle this one as a follow up and not let it block this PR.
Simple renaming can be done after audit starts, but want to get this structure change in before that

Agree with @ian-shim here. Too bad @jianoaix you should have joined the discussion. Let's continue on slack, but basically I do think that Certificate makes sense now. A BlobCertificate is signed by the blob sender. A DACertificate is signed by the Validators. Both are certificates though.

Go ahead for the separation of PRs.
Isn't the renaming "simple" as well?

I don't know what specific discussion you are talking about, but I don't think it's too late to have discussion.

api/proto/common/v2/common_v2.proto

disperser/apiserver/disperse_blob_v2.go

samlaf

Overall LGTM, thanks for making these changes!

2 last things I care about getting right that I don't see in this PR (perhaps they belong in a follow-up PR?) are:

renaming EigenDABlobVerifier.verifyBlobV2 function to verifyCertV2 (because its verifying a cert, not a blob!) cc @0x0aa0
blobKey vs blobHeaderHash. I think I would still prefer we get rid of blobKey and use the more precise and self-documenting blobHeaderHash everywhere.

samlaf · 2025-01-22T21:06:59Z

api/proto/common/v2/common_v2.proto

@@ -25,8 +25,6 @@ message BlobHeader {
  // reserved payments when the same blob is submitted multiple times within the same reservation period. On-demand
  // payments already have unique cumulative_payment values for intentionally unique dispersal requests.
  uint32 salt = 5;


Can we move the salt to position 3 above commitment?
Would make the hashing structure easier to draw out:

Unless the point is to have salt as the explicitly last field? But can we guarantee we'll never need to add a 6th field?

Why does salt have to always be the last field in the struct?
Is this suggestion just to make the diagram prettier?

doesn't have to be, was asking if that's what you were trying to achieve.
Yes.

Well not just the diagram. I feel everything that gets hashed together should be next to each other. Just easier to not have bugs by forgetting one field that was far, for eg when replicating the new hashing structure onchain.

api/proto/common/v2/common_v2.proto

samlaf · 2025-01-22T21:11:49Z

api/proto/disperser/v2/disperser_v2.proto

  // The header contains metadata about the blob.
  //
  // This header can be thought of as an "eigenDA tx", in that it plays a purpose similar to an eth_tx to disperse a
  // 4844 blob. Note that a call to DisperseBlob requires the blob and the blobHeader, which is similar to how
  // dispersing a blob to ethereum requires sending a tx whose data contains the hash of the kzg commit of the blob,
  // which is dispersed separately.
  common.v2.BlobHeader blob_header = 2;
+  // signature over keccak hash of the blob_header that can be verified by blob_header.payment_header.account_id


I'm fussy about this but I think it would be good to be super precise about the exact thing we are signing over. "keccak hash of blob_header" is not precise as it doesn't describe the serialization procedure (nor the fact that we use multi-level hashing).

Perhaps this is one reason to keep BlobKey around, and be super precise in its documentation that it refers to a very specific hashing/serialization combination of the BlobHeader? Then we can just use BlobKey everywhere and refer people to its documentation comment.

samlaf · 2025-01-22T21:14:51Z

core/v2/types.go

@@ -170,6 +164,10 @@ type RelayKey = uint32
 type BlobCertificate struct {
 	BlobHeader *BlobHeader

+	// Signature is an ECDSA signature signed by the blob request signer's account ID over the BlobHeader's blobKey,


Suggested change

// Signature is an ECDSA signature signed by the blob request signer's account ID over the BlobHeader's blobKey,

// Signature is an ECDSA signature signed by the blob request signer's account ID over the blobKey,

blobheader's blobkey sounds like blobkey is a field of blobheader

ian-shim force-pushed the restructs branch 2 times, most recently from 1bd911c to d1343c9 Compare January 22, 2025 16:11

ian-shim requested review from jianoaix, cody-littley, samlaf and hopeyen January 22, 2025 16:23

ian-shim marked this pull request as ready for review January 22, 2025 16:23

ian-shim requested a review from 0x0aa0 January 22, 2025 16:24

0x0aa0 reviewed Jan 22, 2025

View reviewed changes

jianoaix reviewed Jan 22, 2025

View reviewed changes

ian-shim force-pushed the restructs branch from d1343c9 to 1513d07 Compare January 22, 2025 18:39

hopeyen reviewed Jan 22, 2025

View reviewed changes

api/proto/common/v2/common_v2.proto Outdated Show resolved Hide resolved

disperser/apiserver/disperse_blob_v2.go Outdated Show resolved Hide resolved

ian-shim force-pushed the restructs branch 3 times, most recently from 9563b92 to 7cdfa8a Compare January 22, 2025 20:54

samlaf reviewed Jan 22, 2025

View reviewed changes

cody-littley approved these changes Jan 23, 2025

View reviewed changes

0x0aa0 self-requested a review January 23, 2025 15:42

0x0aa0 approved these changes Jan 23, 2025

View reviewed changes

ian-shim force-pushed the restructs branch from 7cdfa8a to 88aa2f7 Compare January 23, 2025 21:17

rename structs

5bd7e4a

ian-shim force-pushed the restructs branch from 88aa2f7 to 5bd7e4a Compare January 23, 2025 21:29

ian-shim merged commit f8c81b2 into Layr-Labs:master Jan 23, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename v2 structs #1136

Rename v2 structs #1136

ian-shim commented Jan 22, 2025 •

edited by samlaf

Loading

0x0aa0 Jan 22, 2025

ian-shim Jan 22, 2025

0x0aa0 Jan 22, 2025

samlaf Jan 22, 2025

jianoaix Jan 22, 2025

ian-shim Jan 22, 2025

samlaf Jan 22, 2025

jianoaix Jan 22, 2025

samlaf Jan 22, 2025

jianoaix Jan 22, 2025

jianoaix Jan 22, 2025

ian-shim Jan 22, 2025

jianoaix Jan 22, 2025

ian-shim Jan 22, 2025

samlaf Jan 22, 2025

jianoaix Jan 22, 2025 •

edited

Loading

samlaf left a comment

samlaf Jan 22, 2025

ian-shim Jan 22, 2025 •

edited

Loading

samlaf Jan 23, 2025

samlaf Jan 23, 2025

samlaf Jan 22, 2025

samlaf Jan 22, 2025

samlaf Jan 22, 2025

	// Signature is an ECDSA signature signed by the blob request signer's account ID over the BlobHeader's blobKey,
	// Signature is an ECDSA signature signed by the blob request signer's account ID over the blobKey,

Rename v2 structs #1136

Rename v2 structs #1136

Conversation

ian-shim commented Jan 22, 2025 • edited by samlaf Loading

Why are these changes needed?

Checks

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

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

jianoaix Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

samlaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim Jan 22, 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

ian-shim commented Jan 22, 2025 •

edited by samlaf

Loading

jianoaix Jan 22, 2025 •

edited

Loading

ian-shim Jan 22, 2025 •

edited

Loading