Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement v2 client GET functionality #972
base: master
Are you sure you want to change the base?
Implement v2 client GET functionality #972
Changes from 38 commits
4625e96
f07e820
885c131
6848663
a48afb1
225f2a3
d265f6a
e9d91c5
dd3c262
88df865
505a1f0
2b87633
cf1cd80
53893d8
0373dd7
826a026
975b6e5
0666d24
0a49aa5
1193ce7
496e277
2d392ff
db51291
4f3280c
aaa1342
9be51e6
cc6b9a1
ae926c7
f82d128
017a48c
b645370
03f8018
ef3944d
78cab0d
e27d3ea
f6126af
28c3d02
036a222
7b66df6
ad3dc97
6930a47
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small dumb question - does the use of a non-deterministic key potentially impact retrieval latencies across a subnetwork of verifier nodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a mutex around rand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do.
The only random method we're using is
Perm
, which callsIntn
under the hood.There exists a static rand method in
math.Rand
, which callsIntn
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation says
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 synchronizationIf 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 begeth
@0x0aa0 can you weigh in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel like this should be a single util function that does both steps.
Also can we rename
blobCommitmentProto
to something likeblobCommitmentInCert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a utility function, and sidestepped the name
blobCommitmentProto
7b66df6aLMK what you think about the chosen name for the util function, it's awkward but nothing better came to mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanded TODO to mention prioritizing low latency relays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g couldn't only one relay lie about the length of the blob, causing the initial varuint decoding and length invariant to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
If a non-parseable blob verifies against the commitments, time to panic. Either it's a bug, or worse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is not 100% accurate. the cert only talks about the blob. Here its the decoding of the blob to the payload that is failing. Our disperser client would only construct correctly encoded blobs, but it might have been constructed by some other library (lets say our rust library), and there might be some incompatibility between the two for eg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, "honestly generated certificate" includes the dispersing client verifying that the input bytes have a valid encoding for the intended use. By "should not be possible", I'm trying to convey that this should never happen in the normal course of business. If it does happen, it indicates when of the following:
Do you have any suggestions for rewording here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure because I haven't wrapped my head around this whole length proof verification stuff, and the kind of attacks possible. But your "a malicious dispersing client" is PRECISELY the kind of thing EigenDA is meant to protect against (otherwise... just use S3). So we absolutely need good safeguards and guarantees around these attacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's chat during our Integrations Food for Thought this afternoon. I'm fine in the meantime with merging this PR as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is. EigenDA is meant to guarantee that data is immutable and available. If the dispersing client wants to fill a blob with total gobbldygook that means nothing to anyone, DA isn't in a position to protect against that. All DA can do it guarantee that the gobbldygook is faithfully preserved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this method take a blob and a cert? That's what I would have intuited from the name. Is it really worth extracting all of these fields from the cert? Would we ever want to call it without actually having a cert in hand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with passing in the entire
EigenDACert
is thatverifyBlobAgainstCert
is called in a loop, and the data contained in the cert must be processed before it can be used (the binding type must be converted to the internal type). And we wouldn't want to do this conversion multiple times.One option would be to pass in an
encoding.BlobCommitments
type, in place of the separateG1Commitment
anduint
length. But since these 2 fields are only a subset of the data contained in theencoding.BlobCommitments
struct, my personal preference is to only pass in the things we need.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha that makes sense at least. Might be nice to have an internal type for Cert (contains all the datatypes that a cert has, but in the internal types instead of contract types). But I'm fine with merging without this. This PR has been alive for too long already haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a larger team discussion about when to create and use internal datatypes.
Should we always have internal types? Only when we need to define methods for autogenerated types? Only when necessary (i.e., use proto types wherever possible)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also still having trouble understanding why we check <= :( :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.