Skip to content

Conversation

hopeyen
Copy link
Contributor

@hopeyen hopeyen commented May 6, 2025

Why are these changes needed?

https://linear.app/eigenlabs/issue/EGDA-1195/validator-node-checks-individual-dispersal-requests

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 :(

@hopeyen hopeyen force-pushed the hope/node-blob-authenticator branch 8 times, most recently from d23be41 to 9aa3353 Compare May 7, 2025 22:58
@hopeyen hopeyen force-pushed the hope/node-blob-authenticator branch 5 times, most recently from c362715 to a841632 Compare May 8, 2025 18:57
@hopeyen hopeyen force-pushed the hope/node-blob-authenticator branch from a841632 to fe701a1 Compare May 8, 2025 18:59
@hopeyen hopeyen marked this pull request as ready for review May 8, 2025 19:24
@hopeyen hopeyen requested review from jianoaix and anupsv May 8, 2025 19:24
jianoaix
jianoaix previously approved these changes May 9, 2025
Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

It seems this is mostly an enhancement of the validator request validation, is it for detecting noncompliant dispersers later?

// - no encoding prover GetCommitmentsForPaddedLength check
// - directly take blob lengths (no blob data yet)
// - doesn't check every 32 bytes is a valid field element
func (s *ServerV2) validateDispersalRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting we didn't validate the blob certificates from the request before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think we just trusted the disperser for those checks:p

return nil, fmt.Errorf("failed to authenticate blob request: %v", err)
}

//this is the length in SYMBOLS (32 byte field elements) of the blob. it must be a power of 2
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a space

// - Takes *corev2.BlobCertificate instead of DisperseBlobRequest
// - no encoding prover GetCommitmentsForPaddedLength check
// - directly take blob lengths (no blob data yet)
// - doesn't check every 32 bytes is a valid field element
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it useful to check these items at validator here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be, but not possible, because those checks are done to the blob directly while we shouldn't make validators validate request against the blob itself

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is it checked?
It's generally better to check at earlier stage if there's sufficient information so we can reject invalid requests fast and cheaply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't checked by the validators. I don't think it is possible because it requires the blob data itself:(

@@ -141,7 +147,14 @@ func (s *ServerV2) StoreChunks(ctx context.Context, in *pb.StoreChunksRequest) (
return nil, api.NewErrorInvalidArg(fmt.Sprintf("failed to verify request: %v", err))
}
}

if s.blobAuthenticator != nil {
for _, blob := range batch.BlobCertificates {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth checking the latency of request validation later, and this part could be parallelized if it takes a meaningful amount of time (the time spent here is contributing directly to the overall request latency)

@hopeyen
Copy link
Contributor Author

hopeyen commented May 9, 2025

It seems this is mostly an enhancement of the validator request validation, is it for detecting noncompliant dispersers later?

yes! in theory nodes could blacklist a disperser as soon as a blob fails here. I think we can later utilize these for slashing dispersers

@hopeyen hopeyen merged commit 93eea09 into master May 15, 2025
15 checks passed
@hopeyen hopeyen deleted the hope/node-blob-authenticator branch May 15, 2025 22:08
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.

3 participants