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
Implement v2 client GET functionality #972
Changes from 1 commit
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
ec190ca
d27c463
840ca9a
16f0c74
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.
curious why this choice. How is the relayKey list populated? Shouldnt we let the person populating be able to dictate some preference on the relays by iterating through them in normal order?
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.
My logic (which may be based on incorrect assumptions) is that, since the relayKey list is part of the cert, the order would the same for every client.
It seems we wouldn't want all clients to attempt the first relay in the list, rather we should distribute load across all available 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.
Ah I see. I think this makes sense. Although now I just realized I'm confused myself. @ian-shim does this make sense? Are the relayKeys meant to be for data replication or sharding? Aka do we need to hit all the relayKeys to get all the blobs, or any ONE should do?
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 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.
Any one should do!
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.
@ian-shim The randomization is done while creating the cert, and all clients will be given the same cert from the disperser, right?
So, for a given blob that can be served by relays
[X, Y, Z]
, if all clients were to attempt the relays in order, then they all would try to get the blob from relayX
, and relays[Y, Z]
wouldn't receive any load.Is this the correct understanding, and if so, are you saying this behavior would be ok?
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.
That's also how I understand it. Agree that your shuffling is better because if we assume that all validator nodes are perfectly in sync (which is rare though, and this not being true would validate ian's approach), then they would all be querying the same relay (first one for that blob), then all querying another one (for the next blob), etc.
Agree that your approach is better, but if the nodes are often out of sync (even by a few blocks only), then that would probably be sufficient to distribute the load across the relays (assuming there's only very few 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.
The primary purpose of having different relay IDs is sharding. Not all relays have all blobs, allowing us to grow the capacity of the relay cohort by adding more relays and assigning fewer blobs to each. But data replication is also something we can achieve by leveraging the relay IDs. By assigning each blob to multiple relays (but not all relays), we can ensure that the failure of individual relays does not result in data unavailability.
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 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.
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 there any reason not to check this invariant here, included in this PR? Seems like it wouldn't be hard to add
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.
Commitments are being checked in the most recent iteration.