fix(disperser-client): RetrieveBlob grpc max size regression bug #849

samlaf · 2024-10-30T20:58:22Z

A previous commit 6894828 (#826) caused regression by forgetting to set grpc max msg size in retrieveBlob call

Caught by integration test from eigenda-proxy: https://github.com/Layr-Labs/eigenda-proxy/actions/runs/11584563970/job/32251942842?pr=196

Added unit-test here to catch any future regression on this feature. Also made that connection size a parameter of the disperser-client (keeps the default 100MiB that was previously set).

Why are these changes needed?

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

api/clients/disperser_client.go

bxue-l2 · 2024-10-30T21:11:17Z

api/clients/disperser_client.go

@@ -38,7 +46,7 @@ func NewConfig(hostname, port string, timeout time.Duration, useSecureGrpcFlag b
 	}
 }

-type DisperserClient interface {
+type IDisperserClient interface {


I don't think we use this convention, this looks solidity

what do you think @ian-shim

I know, I don't like it either, but we already use this for eigenda_client, so I just followed the convention there.
Was forced to make the disperserClient struct public for the tests, so had to find another name for the interface. More golangy might be DisperserClienter. Honestly I think defining the interface here is code smell and should eventually all be refactored. Interfaces should be defined case-by-case at the call sites.

Ok, saw it. we used IEigenDAClient.

Looks fine to me

Was forced to make the disperserClient struct public for the tests

Don't see any place that uses it. We couldn't use the constructor in the test?

Ah, you're right. Originally was trying to put the tests in the same package so I can build the struct manually injecting the connection to test something. But then realized I can just add the config parameter.

So hmmm... could revert to the old names. But somehow I feel like our clients should follow the same convention.

Reverted: dc98581
Also added TODO to change eigenda-client to use same naming convention as disperser-client: 57d716f

api/clients/disperser_client.go

pschork · 2024-10-30T21:14:01Z

api/clients/disperser_client.go

-		BlobIndex:       blobIndex,
-	})
+	if c.config.MaxRetrieveBlobSizeBytes == 0 {
+		// max blob size on mainnet is currently 16MiB but we set this to 100MiB for backward compatibility


@samlaf are you planning to update this comment every time the disperser's MAX_BLOB_SIZE config is changed? I would rephrase this to set to 100MiB for forward compatibility. check official documentation for current max blob size on mainnet

Good call: a07336c

pschork · 2024-10-30T21:14:53Z

api/clients/disperser_client.go

+	// but all other methods use the default 4MiB max message size, whereas RetrieveBlob
+	// potentially needs a larger size.
+	//
+	// If not set, the default value is 100MiB, to be backward compatible.


forward compatible

It's actually backward compatible. The code before my previous PR (that caused the regression) had a default 100MiB.

Ah just saw your previous comment, I see what you mean now! Good point

A previous commit 6894828 (#826) caused regression by forgetting to set grpc max msg size in retrieveBlob call Caught by integration test from eigenda-proxy Added unit-test here to catch any future regression on this feature

…t value to constructor

bxue-l2 · 2024-10-31T00:12:15Z

lgtm, but address Ian's comment

…lient(struct)/DisperserClient(interface) to prevetn breaking change

…ention

ian-shim

lgtm, left one more comment!

ian-shim · 2024-10-31T00:34:37Z

api/clients/disperser_client.go

@@ -91,6 +103,14 @@ var _ DisperserClient = &disperserClient{}
 //	// Subsequent calls will use the existing connection
 //	status2, requestId2, err := client.DisperseBlob(ctx, otherData, otherQuorums)
 func NewDisperserClient(config *Config, signer core.BlobRequestSigner) *disperserClient {
+	if config == nil {
+		config = &Config{}


should it return error if config is nil?

yep good call: f2100c8

Just merged Layr-Labs/eigenda#849 which will fix our ci bug

* fix(e2e tests): missing new eigenda-client required config fields - ethrpc and svcmanageraddr * Revert "ci: give holesky-test workflow access to secrets via pull_request_target (#153)" This reverts commit 15b10fd. The commit was doing things very wrong. I hadn't understood how pull_request_target works. Was causing the workflow to run against main branch head commit instead of PR commit. We will need to find another solution to the problem of letting external contributors run this workflow. * ide: update vscode settings.json with env vars to run holesky testnet e2e tests * docs: shorten svcManagerAddr holesky testnet comment * tests: add panic when both INTEGRATION and TESTNET env vars are set This forces test runner to be fully aware of which test suite he is running (otherwise it implicitly runs the TESTNET suite) * style: make handleGetShared returned error print hex encoded commitment Previously it was printing as byte array, which is unreadable and clutters logs * docs: better comments in metrics middleware * deps: upgrade eigenda dep to regression fix commit TODO: will need to update this after that PR is merged * deps: update eigenda dependency to master head Just merged Layr-Labs/eigenda#849 which will fix our ci bug

samlaf changed the title ~~fix(disperser-client): RetrieveBlob max size regression bug~~ fix(disperser-client): RetrieveBlob grpc max size regression bug Oct 30, 2024

samlaf requested review from ian-shim, bxue-l2 and epociask October 30, 2024 21:02

bxue-l2 reviewed Oct 30, 2024

View reviewed changes

pschork reviewed Oct 30, 2024

View reviewed changes

samlaf added 2 commits October 30, 2024 23:54

fix(disperser-client): RetrieveBlob max size regression bug

6245212

A previous commit 6894828 (#826) caused regression by forgetting to set grpc max msg size in retrieveBlob call Caught by integration test from eigenda-proxy Added unit-test here to catch any future regression on this feature

docs: clearer MaxRetrieveBlobSizeBytes docstring

76bed5f

samlaf force-pushed the samlaf/fix--disperser-client-retrieve-blob-max-size-regression branch from a07336c to 76bed5f Compare October 30, 2024 23:54

style(disperser_client): move setting MaxRetrieveBlobSizeBytes defaul…

04499a5

…t value to constructor

samlaf requested a review from pschork October 31, 2024 00:03

bxue-l2 approved these changes Oct 31, 2024

View reviewed changes

samlaf added 2 commits October 31, 2024 00:30

revert(disperser-client): move back to previous convention disperserC…

dc98581

…lient(struct)/DisperserClient(interface) to prevetn breaking change

docs: add todo to rename eigenDAClient to follow disperserClient conv…

57d716f

…ention

ian-shim approved these changes Oct 31, 2024

View reviewed changes

fix(disperser-client): make constructor return err when config is wrong

f2100c8

samlaf merged commit e2ead56 into master Oct 31, 2024
9 checks passed

samlaf deleted the samlaf/fix--disperser-client-retrieve-blob-max-size-regression branch October 31, 2024 14:52

samlaf added a commit to Layr-Labs/eigenda-proxy that referenced this pull request Oct 31, 2024

deps: update eigenda dependency to master head

adf580f

Just merged Layr-Labs/eigenda#849 which will fix our ci bug

samlaf mentioned this pull request Oct 31, 2024

fix(e2e tests): missing new eigenda-client required config fields Layr-Labs/eigenda-proxy#196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(disperser-client): RetrieveBlob grpc max size regression bug #849

fix(disperser-client): RetrieveBlob grpc max size regression bug #849

samlaf commented Oct 30, 2024 •

edited

Loading

bxue-l2 Oct 30, 2024

samlaf Oct 30, 2024

bxue-l2 Oct 30, 2024

ian-shim Oct 30, 2024

samlaf Oct 31, 2024

samlaf Oct 31, 2024

samlaf Oct 31, 2024

pschork Oct 30, 2024

samlaf Oct 30, 2024

pschork Oct 30, 2024

samlaf Oct 30, 2024

samlaf Oct 30, 2024

samlaf Oct 30, 2024

bxue-l2 commented Oct 31, 2024

ian-shim left a comment

ian-shim Oct 31, 2024

samlaf Oct 31, 2024

fix(disperser-client): RetrieveBlob grpc max size regression bug #849

fix(disperser-client): RetrieveBlob grpc max size regression bug #849

Conversation

samlaf commented Oct 30, 2024 • edited 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

bxue-l2 commented Oct 31, 2024

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samlaf commented Oct 30, 2024 •

edited

Loading