Implement v2 client GET functionality #972

litt3 · 2024-12-09T21:56:43Z

Why are these changes needed?

This PR creates a new EigenDAClientV2, and implements GET functionality for the new client. Additional client functionality will be implemented in upcoming PRs.

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

Signed-off-by: litt3 <[email protected]>

samlaf

First pass on the client code. Logic LGTM. Added a bunch of nit comments to make code cleaner.
Have not looked at tests yet. Can you try to clean up the linter errors, makes it hard to read on github.

api/clients/eigenda_client_v2.go

samlaf · 2024-12-09T23:13:20Z

api/clients/eigenda_client_v2.go

+// EigenDAClientV2 provides the ability to get blobs from the relay subsystem, and to send new blobs to the disperser.
+type EigenDAClientV2 interface {
+	// GetBlob iteratively attempts to retrieve a given blob with key blobKey from the relays listed in the blobCertificate.
+	GetBlob(ctx context.Context, blobKey corev2.BlobKey, blobCertificate corev2.BlobCertificate) ([]byte, error)


If we only need the BlobCertificate for the relay keys, I think we should just take the relayKeys slice directly?

I think we might need more data from the certificate for verification (though I'm not sure yet what is required). If it turns out we don't need the cert for anything else, I'll make the suggested change

makes sense! I'll keep this convo unresolved as a reminder for the next review when you commit the POST code

api/clients/eigenda_client_v2.go

Signed-off-by: litt3 <[email protected]>

samlaf · 2024-12-10T19:49:07Z

api/clients/config_v2.go

+package clients
+
+import (
+	"github.com/Layr-Labs/eigenda/api/clients/codecs"
+)
+
+// EigenDAClientConfigV2 contains configuration values for EigenDAClientV2
+type EigenDAClientConfigV2 struct {


Starting this thread to discuss package structure. I personally don't like that all our clients are in the same package. I think I would prefer if each client was in its own separate package, because that allows to have cleaner names, and make sure to prevent clashes. For eg:

import "...clients/v2/disperser" import edaclient ".../v2/eigenda" func main() { dispClient := disperser.New(disperser.Config{...}) edaclient.New(edaclient.Config{...}, dispClient) }

is way cleaner than having long names all under clients package.

I'm fine if people prefer to keep all the clients in same namespace, but at least I think we should separate the v1 and v2 namespace. Thoughts? @ian-shim @epociask @bxue-l2 @jianoaix

Makes sense to me. We are getting a lot "v2" in the names around.

api/clients/eigenda_client_v2.go

epociask · 2024-12-10T21:23:45Z

api/clients/eigenda_client_v2.go

+// EigenDAClientV2 provides the ability to get blobs from the relay subsystem, and to send new blobs to the disperser.
+type EigenDAClientV2 struct {
+	log logging.Logger
+	// doesn't need to be cryptographically secure, as it's only used to distribute load across relays


out of scope for this PR but curious if we'd ever wanna let users define their own retrieval policies when communicating with relays

Certainly something to consider, at the latest if/when users have to pay relays for retrieval

api/clients/eigenda_client_v2.go

epociask · 2024-12-10T21:28:05Z

api/clients/eigenda_client_v2.go

+func createCodec(config *EigenDAClientConfigV2) (codecs.BlobCodec, error) {
+	lowLevelCodec, err := codecs.BlobEncodingVersionToCodec(config.PutBlobEncodingVersion)
+	if err != nil {
+		return nil, fmt.Errorf("create low level codec: %w", err)


knit:

Suggested change

return nil, fmt.Errorf("create low level codec: %w", err)

return nil, fmt.Errorf("could not create low level codec: %w", err)

knit^2: disagree here, been trying to enforce https://github.com/uber-go/guide/blob/master/style.md#error-wrapping

api/clients/eigenda_client_v2.go

epociask · 2024-12-10T21:35:00Z

api/clients/eigenda_client_v2.go

+		// if GetBlob returned an error, try calling a different relay
+		if err != nil {
+			c.log.Warn("blob couldn't be retrieved from relay", "blobKey", blobKey, "relayKey", relayKey, "error", err)
+			continue
+		}


what about the circumstance where the error is transient and the # of relay keys == 1?

Are you suggesting that we have an additional timeout, during which the client repeatedly retries all relays?

I could implement this if it's the way we want to go- but I don't see how the case relay keys == 1 is unique?

epociask · 2024-12-10T21:39:29Z

api/clients/eigenda_client_v2.go

+			continue
+		}
+
+		// An honest relay should never send a blob which cannot be decoded


To expand these invariants: an honest relay should never send a blob which doesn't respect its polynomial commitments. The thing is though this check would get caught upstream (i.e, within proxy directly) and probably cause the request to fail. The proxy client would trigger a retry which would probably route to another relay.

this isn't a big problem rn and we can just document it somewhere for circle back sometime in the future.

Is there any reason not to check this invariant here, included in this PR? Seems like it wouldn't be hard to add

api/clients/codecs/mock/BlobCodec.go

ian-shim · 2024-12-10T23:23:37Z

api/clients/eigenda_client_v2.go

+
+// GetBlob iteratively attempts to retrieve a given blob with key blobKey from the relays listed in the blobCertificate.
+//
+// The relays are attempted in random order.


The disperser implementation itself shuffles the relay keys, so attempting each relay in order is actually fine. But it's good that we're not assuming that relay keys aren't ordered in any particular way.

Aka do we need to hit all the relayKeys to get all the blobs, or any ONE should do?

Any one should do!

api/clients/eigenda_client_v2.go

ian-shim · 2024-12-10T23:30:08Z

api/clients/eigenda_client_v2.go

+}
+
+// GetCodec returns the codec the client uses for encoding and decoding blobs
+func (c *EigenDAClientV2) GetCodec() codecs.BlobCodec {


why do we need this getter?

I carried it forward from v1 client - ~~will remove as it may not be necessary for v2~~ (nevermind, see comment from Sam below)

I think we'll still need it. We use it in proxy to get the codec and IFFT blobs when computing commitments.

api/clients/codecs/mock/BlobCodec.go

api/clients/config_v2.go

api/clients/eigenda_client_v2.go

Signed-off-by: litt3 <[email protected]>

jianoaix · 2024-12-12T17:51:16Z

api/clients/v2/config.go

+type VerificationMode uint
+
+const (
+	// TODO: write good docs here for IFFT and NoIFFT (I need to update my understanding to be able to write this)


It looks the "PointVerificationMode" field in config already documents what these two mean.

Actually, I am not entirely sure if we want to call out "how" (i.e. IFFT and then FFT) at the interface level. What about BlobFormat or something (point v.s. coeffs) that describes the blobs, not how they are processed underneath? cc @samlaf

It looks the "PointVerificationMode" field in config already documents what these two mean.

There's a description there, but I'm personally a bit confused by what it says, so I want to clarify to be able to write a good description here.

What about BlobFormat or something

Makes sense, I'll defer to you and Sam for the proper name here

Signed-off-by: litt3 <[email protected]>

Write GET tests

4625e96

Signed-off-by: litt3 <[email protected]>

litt3 requested review from cody-littley, epociask and samlaf December 9, 2024 22:46

litt3 self-assigned this Dec 9, 2024

samlaf reviewed Dec 9, 2024

View reviewed changes

Merge branch 'master' into client-v2-get

f07e820

litt3 mentioned this pull request Dec 10, 2024

Write TestRandom util #976

Merged

5 tasks

litt3 added 2 commits December 10, 2024 14:13

Respond to PR comments

885c131

Signed-off-by: litt3 <[email protected]>

Create new V2 client config

6848663

Signed-off-by: litt3 <[email protected]>

samlaf reviewed Dec 10, 2024

View reviewed changes

epociask reviewed Dec 10, 2024

View reviewed changes

ian-shim reviewed Dec 10, 2024

View reviewed changes

jianoaix reviewed Dec 11, 2024

View reviewed changes

litt3 added 6 commits December 11, 2024 17:07

Respond to more PR comments

a48afb1

Signed-off-by: litt3 <[email protected]>

Fix failing unit test

225f2a3

Signed-off-by: litt3 <[email protected]>

Merge branch 'master' into client-v2-get

d265f6a

Adopt new package structure

e9d91c5

Signed-off-by: litt3 <[email protected]>

Use new test random util

dd3c262

Signed-off-by: litt3 <[email protected]>

Implement relay call timeout

88df865

Signed-off-by: litt3 <[email protected]>

jianoaix reviewed Dec 12, 2024

View reviewed changes

litt3 added 9 commits December 12, 2024 12:57

Use correct error join method

505a1f0

Signed-off-by: litt3 <[email protected]>

Merge branch 'master' into client-v2-get

2b87633

Make updates required by upstream changes

cf1cd80

Signed-off-by: litt3 <[email protected]>

Update how FFT and IFFT are referred to

53893d8

Signed-off-by: litt3 <[email protected]>

Implement GetPayload

0373dd7

Signed-off-by: litt3 <[email protected]>

Remove GetBlob, leaving only GetPayload

826a026

Signed-off-by: litt3 <[email protected]>

Remove unnecessary codec mock

975b6e5

Signed-off-by: litt3 <[email protected]>

Use more reasonable line breaks for logs

0666d24

Signed-off-by: litt3 <[email protected]>

Test malicious cert

0a49aa5

Signed-off-by: litt3 <[email protected]>

litt3 added 4 commits January 13, 2025 17:16

Merge branch 'master' into client-v2-get

1193ce7

Merge branch 'master' into client-v2-get

496e277

Finish test coverage

2d392ff

Signed-off-by: litt3 <[email protected]>

Fix commitment length check

db51291

Signed-off-by: litt3 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement v2 client GET functionality #972

Implement v2 client GET functionality #972

litt3 commented Dec 9, 2024

samlaf left a comment

samlaf Dec 9, 2024

litt3 Dec 10, 2024

samlaf Dec 10, 2024

samlaf Dec 10, 2024

jianoaix Dec 11, 2024

epociask Dec 10, 2024

litt3 Jan 14, 2025

epociask Dec 10, 2024

samlaf Dec 10, 2024

epociask Dec 10, 2024

litt3 Dec 11, 2024

epociask Dec 10, 2024

litt3 Dec 11, 2024

ian-shim Dec 10, 2024

ian-shim Dec 10, 2024

litt3 Dec 11, 2024 •

edited

Loading

samlaf Dec 11, 2024

jianoaix Dec 12, 2024

litt3 Dec 12, 2024

	return nil, fmt.Errorf("create low level codec: %w", err)
	return nil, fmt.Errorf("could not create low level codec: %w", err)

Implement v2 client GET functionality #972

Are you sure you want to change the base?

Implement v2 client GET functionality #972

Conversation

litt3 commented Dec 9, 2024

Why are these changes needed?

Checks

samlaf left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litt3 Dec 11, 2024 • 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

litt3 Dec 11, 2024 •

edited

Loading