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 <102969658+litt3@users.noreply.github.com>

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

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

api/clients/config_v2.go

api/clients/eigenda_client_v2.go

ethenotethan · 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

ethenotethan · 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?

Agree, I would prefer we let the outer layer implement the retry behavior it wants. In this case this means either the proxy, or even the batcher.

ethenotethan · 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

Commitments are being checked in the most recent iteration.

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

api/clients/codecs/mock/BlobCodec.go

api/clients/config_v2.go

api/clients/eigenda_client_v2.go

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

api/clients/v2/config.go

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

api/clients/codecs/polynomial_form.go

api/clients/v2/config.go

api/clients/v2/verification/blob_verifier.go

api/clients/v2/verification/commitment_utils.go

api/clients/v2/eigenda_client.go

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

ethenotethan · 2025-01-21T15:46:57Z

api/clients/v2/eigenda_client.go

+	// doesn't need to be cryptographically secure, as it's only used to distribute load across relays
+	random       *rand.Rand


small dumb question - does the use of a non-deterministic key potentially impact retrieval latencies across a subnetwork of verifier nodes?

I don't think it's possible to provide any guarantee of highly similar latencies across a network of verifier nodes. Even if all nodes were to talk to a single relay, that relay could have high latency variability in responding to the different verifier nodes.

I think the best solution would be to implement a tool which prioritizes the best relay partner on a node-by-node basis, as mentioned in the TODO comment, so that every verifier node gets a response as quickly as possible

ethenotethan · 2025-01-21T15:49:14Z

api/clients/v2/eigenda_client.go

+		return nil, fmt.Errorf("new relay client: %w", err)
+	}
+
+	ethClient, err := geth.NewClient(ethConfig, gethcommon.Address{}, 0, log)


probably out of scope but does the use of geth for the eth package imply that the node being used has to be a go-ethereum one or do other execution client nodes (e.g, reth, besu) also work?

does the use of geth for the eth package imply that the node being used has to be a go-ethereum

I don't think that's the implication. The bindings method I'm using requires an EthClient input parameter, and the implementation happens to be in a package called geth. But I don't see why the target node would be required to be geth

@0x0aa0 can you weigh in here?

yeah no it doesn't, it's just a geth provided library. its just an implementation of the eth rpc methods which all clients implement.

ethenotethan · 2025-01-21T19:10:31Z

api/clients/v2/eigenda_client.go

+		}
+
+		payload, err := c.codec.DecodeBlob(blob)
+		if err != nil {


why is this error a terminal one but others warrant retrying? Is the idea that if a blob passes verification then the contents would always be the same and therefore the codec decoding would yield the same result irrespective of relay?

e.g couldn't only one relay lie about the length of the blob, causing the initial varuint decoding and length invariant to fail?

if a blob passes verification then the contents would always be the same and therefore the codec decoding would yield the same result irrespective of relay

Correct. If the blob contents verified against the cert we have, that means the relay delivered the blob as it was dispersed. If we asked another relay, either it would:

return the same blob bytes, and we end up at the same place

return different blob bytes. if these different bytes are decodable, that means they are necessarily different from the bytes we currently have, so the cert can't possibly verify

If a non-parseable blob verifies against the commitments, time to panic. Either it's a bug, or worse

api/clients/v2/eigenda_client.go

api/clients/codecs/polynomial_form.go

api/clients/v2/config.go

api/clients/v2/eigenda_client.go

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

ian-shim

lgtm

api/clients/v2/eigenda_client.go

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

api/clients/v2/config.go

api/clients/v2/eigenda_client.go

api/clients/v2/verification/eigenda_cert.go

api/clients/v2/mock/blob_verifier.go

samlaf · 2025-01-23T02:51:32Z

api/clients/v2/verification/blob_verifier.go

Sorry for catching this so late, but can we rename this file, the struct, the mock, everything literally, to cert_verifier instead? We can't change the contract name (although I will put a LOT of pressure to get this to change), but I don't think it means our entire codebase should be riddled with this mistake.

I agree with this name change.

I'm going to do a rename PR after this PR merges, since we know the contained EigenDAClient needs to become PayloadRetriever.

Since this blob_verifier is pre-existing, I'd prefer to postpone this suggestion until that rename PR.

api/clients/v2/verification/blob_verifier.go

samlaf · 2025-01-23T02:53:01Z

api/clients/v2/eigenda_client.go

+type EigenDAClient struct {
+	log logging.Logger
+	// doesn't need to be cryptographically secure, as it's only used to distribute load across relays
+	random       *rand.Rand


we need a mutex around rand

I don't think we do.

The only random method we're using is Perm, which calls Intn under the hood.

There exists a static rand method in math.Rand, which calls Intn on the global rand singleton, without any synchronization.

func Intn(n int) int { return globalRand().Intn(n) }

This indicates to me that it must be safe to call without synchronization.

Documentation says

Random numbers are generated by a Source, usually wrapped in a Rand. Both types should be used by a single goroutine at a time: sharing among multiple goroutines requires some kind of synchronization.

Think the whole package is not goroutine safe. I think (but can't find a link atm) that everything in golang is assumed to NOT be goroutine safe, unless explicitly stated in the documentation comment. For eg golang maps are not goroutine safe: there's https://pkg.go.dev/golang.org/x/sync/syncmap for that.

The package itself is comfortable using a global instance of Rand for some functionality, without explicit synchronization. I think it would be reasonable to copy the same usage patterns, without explicit synchronization

If you're not comfortable with this, though, I think I would lean toward sacrificing test determinism, and just use the static methods from rand instead of maintaining a local source of randomness. Having mutexes around random is very ugly.

Thoughts on this?

see comment above

samlaf · 2025-01-23T03:03:58Z

api/clients/v2/eigenda_client.go

+	blobCommitmentProto := contractEigenDABlobVerifier.BlobCommitmentBindingToProto(
+		&eigenDACert.BlobVerificationProof.BlobCertificate.BlobHeader.Commitment)
+	blobCommitment, err := encoding.BlobCommitmentsFromProtobuf(blobCommitmentProto)
+
+	if err != nil {
+		return nil, fmt.Errorf("blob commitments from protobuf: %w", err)
+	}


feel like this should be a single util function that does both steps.
Also can we rename blobCommitmentProto to something like blobCommitmentInCert

I added a utility function, and sidestepped the name blobCommitmentProto 7b66df6a

LMK what you think about the chosen name for the util function, it's awkward but nothing better came to mind

LGTM, thanks! I don't like though that we use commitmentS (with an s) in one place and not the other.

Can we make those the same (prob remove the s?)

This is a larger renaming discussion.

Considering the struct itself contains 2 commitments, in addition to other elements, if anything BlobCommitments, plural, is the better name.

api/clients/v2/eigenda_client.go

samlaf · 2025-01-23T03:32:55Z

api/clients/v2/eigenda_client.go

+	// here: it isn't necessary to verify the length proof itself, since this will have been done by DA nodes prior to
+	// signing for availability.


Is this really true? I feel like we should still check the proof?
Otherwise wouldn't the same argument also apply to the actual commitment above? and possibly some other checks we check in the CertVerification contract call.

Also still having trouble understanding why we check <= :( :(

Summoning the experts @bxue-l2 @anupsv

But here's my attempt at an explanation:
The reason why we need to verify the kzg commitment is because without doing that, we don't know for sure that the relay sent us the same bytes that the DA nodes received. Once we have verified the commitment, this is guaranteed, so we can begin to rely on our trust of the DA nodes. We assume that a given threshold of DA nodes must be honest, and the only way the length proof could fail verification is if greater than the assumed threshold is malicious.

Now, as to whether the length check is needed at all, let me repeat here my comment I recently made on the notion doc:

I’m also not 100% convinced it is necessary in the retrieval path. The only thing I can think that this is protecting against is a relay sending tons of extra padded 0s? But we could also protect against that by simply forbidding relays from sending trailing zeros, and check that

cody-littley · 2025-01-23T14:42:02Z

api/clients/v2/eigenda_client.go

+	// create a randomized array of indices, so that it isn't always the first relay in the list which gets hit
+	indices := c.random.Perm(relayKeyCount)
+
+	// TODO (litt3): consider creating a utility which can deprioritize relays that fail to respond (or respond maliciously)


Nit, utility should also prioritize relays with lower latencies (although perhaps it should still reach out to lower priority relays with small but non-zero probability).

Expanded TODO to mention prioritizing low latency relays

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

samlaf

LGTM!! Let's go

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Write GET tests

4625e96

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 requested review from cody-littley, ethenotethan 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 <102969658+litt3@users.noreply.github.com>

Create new V2 client config

6848663

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

samlaf reviewed Dec 10, 2024

View reviewed changes

api/clients/config_v2.go Outdated Show resolved Hide resolved

ethenotethan 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 <102969658+litt3@users.noreply.github.com>

Fix failing unit test

225f2a3

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into client-v2-get

d265f6a

Adopt new package structure

e9d91c5

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Use new test random util

dd3c262

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Implement relay call timeout

88df865

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

jianoaix reviewed Dec 12, 2024

View reviewed changes

api/clients/v2/config.go Outdated Show resolved Hide resolved

litt3 added 9 commits December 12, 2024 12:57

Use correct error join method

505a1f0

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into client-v2-get

2b87633

Make updates required by upstream changes

cf1cd80

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Update how FFT and IFFT are referred to

53893d8

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Implement GetPayload

0373dd7

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Remove GetBlob, leaving only GetPayload

826a026

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Remove unnecessary codec mock

975b6e5

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Use more reasonable line breaks for logs

0666d24

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Test malicious cert

0a49aa5

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into client-v2-get

b645370

litt3 requested a review from cody-littley January 21, 2025 14:39

samlaf reviewed Jan 21, 2025

View reviewed changes

Address some PR comments

03f8018

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

ethenotethan reviewed Jan 21, 2025

View reviewed changes

bxue-l2 reviewed Jan 21, 2025

View reviewed changes

api/clients/codecs/polynomial_form.go Show resolved Hide resolved

api/clients/v2/config.go Outdated Show resolved Hide resolved

api/clients/v2/eigenda_client.go Outdated Show resolved Hide resolved

Rename methods, and clean up

ef3944d

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 requested review from ethenotethan and samlaf January 21, 2025 22:17

ian-shim approved these changes Jan 21, 2025

View reviewed changes

api/clients/v2/eigenda_client.go Outdated Show resolved Hide resolved

litt3 added 5 commits January 22, 2025 11:11

Actually apply fix for poor doc

78cab0d

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Fix goroutine safety comment

e27d3ea

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into client-v2-get

f6126af

Fix test

28c3d02

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Rework polynomial encoding enum, and descriptions

036a222

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 requested a review from anupsv January 22, 2025 21:12

bxue-l2 approved these changes Jan 22, 2025

View reviewed changes

api/clients/v2/config.go Show resolved Hide resolved

samlaf reviewed Jan 22, 2025

View reviewed changes

api/clients/v2/eigenda_client.go Show resolved Hide resolved

samlaf reviewed Jan 23, 2025

View reviewed changes

cody-littley approved these changes Jan 23, 2025

View reviewed changes

litt3 added 2 commits January 23, 2025 11:02

Make PR fixes

7b66df6

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Move conversion utils

ad3dc97

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

samlaf mentioned this pull request Jan 23, 2025

Refactor clients package #1143

Open

litt3 added 4 commits January 23, 2025 13:07

Remove GetCodec

6930a47

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into client-v2-get

ec190ca

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into client-v2-get

d27c463

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Fix merge

840ca9a

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

samlaf approved these changes Jan 24, 2025

View reviewed changes

Add additional comment about random

16f0c74

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 merged commit 180bc96 into Layr-Labs:master Jan 24, 2025
8 checks passed

		// doesn't need to be cryptographically secure, as it's only used to distribute load across relays
		random *rand.Rand

		// here: it isn't necessary to verify the length proof itself, since this will have been done by DA nodes prior to
		// signing for availability.

Implement v2 client GET functionality #972

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

litt3 Jan 17, 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

litt3 Jan 21, 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

litt3 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litt3 Jan 23, 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

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

samlaf left a comment

Choose a reason for hiding this comment

litt3 Jan 17, 2025 •

edited

Loading

litt3 Jan 21, 2025 •

edited

Loading

litt3 Jan 21, 2025 •

edited

Loading

litt3 Jan 23, 2025 •

edited

Loading