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.

epociask · 2024-12-10T21:16:58Z

api/clients/eigenda_client_v2.go

+		indices[i], indices[j] = indices[j], indices[i]
+	})
+
+	// TODO (litt3): consider creating a utility which can deprioritize relays that fail to respond (or respond maliciously)


good to think about but probably pretty low priority until we have relays that we don't run ourselves

Is this sort of TODO comment acceptable to leave in production code?

epociask · 2024-12-10T21:19:21Z

api/clients/eigenda_client_v2.go

+		// An honest relay should never send a blob which cannot be decoded
+		decodedData, err := c.codec.DecodeBlob(data)
+		if err != nil {
+			c.log.Warn("error decoding blob", "blobKey", blobKey, "relayKey", relayKey, "error", err)


knit - only warning in iteration logic that doesn't reference relay in core msg

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

epociask · 2024-12-10T21:26:54Z

api/clients/eigenda_client_v2.go

+		return decodedData, nil
+	}
+
+	return nil, fmt.Errorf("unable to retrieve blob from any relay")


knit - including the relay count in the error msg could be useful context when debugging

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

epociask · 2024-12-10T21:33:55Z

api/clients/eigenda_client_v2.go

+		// TODO: does this need a timeout?
+		data, err := c.relayClient.GetBlob(ctx, relayKey, blobKey)


yes - grpc clients by default don't have a default timeout iiuc:

https://medium.com/geekculture/timeout-context-in-go-e88af0abd08d

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

ian-shim · 2024-12-10T23:18:35Z

api/clients/codecs/mock/BlobCodec.go

@@ -0,0 +1,68 @@
+package mock


nit: filenames should be snakecase i.e. blob_codec.go

Fixed a48afb1

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!

ian-shim · 2024-12-10T23:27:04Z

api/clients/eigenda_client_v2.go

+	var indices []int
+	// create a randomized array of indices, so that it isn't always the first relay in the list which gets hit
+	for i := 0; i < relayKeyCount; i++ {
+		indices = append(indices, i)
+	}


nit:

indices := make([]int, relayKeyCount) for i := 0; i < relayKeyCount; i++ { indices[i] = i }

I simplified even further, as suggested in this comment

a48afb1

ian-shim · 2024-12-10T23:27:49Z

api/clients/eigenda_client_v2.go

+	for _, val := range indices {
+		relayKey := blobCertificate.RelayKeys[val]
+
+		// TODO: does this need a timeout?


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.

jianoaix · 2024-12-11T18:40:26Z

api/clients/codecs/mock/BlobCodec.go

+func (_m *BlobCodec) DecodeBlob(encodedData []byte) ([]byte, error) {
+	ret := _m.Called(encodedData)
+
+	if len(ret) == 0 {


should it expect leng(ret) == 2 since you also access Get(1)?

I believe the reason to only check ==0 here is to yield a more informative panic in the specific case where no return value is specified. The call to Get(1) later contains a length check under the hood, and would just yield a different message.

(for context, this mock was autogenerated)

jianoaix · 2024-12-11T18:42:33Z

api/clients/codecs/mock/BlobCodec.go

+	if rf, ok := ret.Get(0).(func([]byte) []byte); ok {
+		r0 = rf(encodedData)
+	} else {
+		if ret.Get(0) != nil {


nit: control flow may be simplified as else if ret.Get(0) != nil { ... }

The file was auto generated by mockery. Are you ok with leaving the auto generated code as is?

Let me know if you'd prefer I make these changes, and I will apply your suggestions.

jianoaix · 2024-12-11T18:43:54Z

api/clients/codecs/mock/BlobCodec.go

+		r0 = rf(encodedData)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).([]byte)


nit: may check the casting is done correctly r0, ok = ret.Get(0).([]byte)

jianoaix · 2024-12-11T18:45:37Z

api/clients/config_v2.go

+// EigenDAClientConfigV2 contains configuration values for EigenDAClientV2
+type EigenDAClientConfigV2 struct {
+	// The blob encoding version to use when writing blobs from the high level interface.
+	PutBlobEncodingVersion codecs.BlobEncodingVersion


What does "Put" in this naming mean?

It was referring to PUT, as contrasted with GET.

But I think you're right, and it's not necessary. I will simplify the name to BlobEncodingVersion

Changed in a48afb1

jianoaix · 2024-12-11T18:51:11Z

api/clients/config_v2.go

+	// This makes it possible to open points on the KZG commitment to prove that the field elements correspond to
+	// the commitment. With this mode disabled, you will need to supply the entire blob to perform a verification
+	// that any part of the data matches the KZG commitment.
+	DisablePointVerificationMode bool


Would it better to define a enum for point and coeff modes for this param? That may be more readable, since these two modes are important information.

Enum defined a48afb1

jianoaix · 2024-12-11T18:54:01Z

api/clients/eigenda_client_v2.go

+	relayKeyCount := len(blobCertificate.RelayKeys)
+
+	if relayKeyCount == 0 {
+		return nil, fmt.Errorf("relay key count is zero")


nit: may just errors.New("...") since there is not formatting in this error

Done a48afb1

jianoaix · 2024-12-11T18:59:42Z

api/clients/eigenda_client_v2.go

+		return nil, fmt.Errorf("relay key count is zero")
+	}
+
+	var indices []int


You can just simplify it: indices := rand.Perm(relayKeyCount)

Done a48afb1

jianoaix · 2024-12-11T19:01:48Z

api/clients/eigenda_client_v2.go

+
+		// An honest relay should never send an empty blob
+		if len(data) == 0 {
+			c.log.Warn("blob received from relay had length 0", "blobKey", blobKey, "relayKey", relayKey, "error", err)


The "err" is already made sure nil, no need to log it

Fixed a48afb1

jianoaix · 2024-12-11T19:05:46Z

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 {


Can you comment thread safety of this struct?

Added comment stating the struct is not threadsafe a48afb1

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

Respond to more PR comments

a48afb1

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

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

		// TODO: does this need a timeout?
		data, err := c.relayClient.GetBlob(ctx, relayKey, blobKey)

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

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

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

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

litt3 Dec 11, 2024 •

edited

Loading