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
[WIP] EIP-7594: PeerDAS protocol #3574
[WIP] EIP-7594: PeerDAS protocol #3574
Changes from 3 commits
d6a37ec
93dddd1
504b4f9
696d443
2cc7c87
665e6fa
9553d54
a72ece8
65be5b0
55db861
4477cc6
56e6a98
edeef07
b2a4657
7aab577
170dae5
547460c
d23452d
87e9702
428c166
c47d5f3
91dbbb3
e7c0d5f
8150f76
bb33f90
1acb1ff
cebf78a
8728561
5535e6a
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.
Low priority, feel free to merge without 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.
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 usually use this syntax and general prefer:
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.
With discoveryV5 , in order to discover new peers we perform a random lookup of nodes by node ID and then try to perform a handshake and connect with them. This should result in the node id of your connected peers being random(therefore custody distributions are equally split). Is there a reason you do not believe this will happen in practice that we will have to enforce it ?
If we do have to enforce it, then the total peers you are connected to will have to be defined as
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 cannot assume that the node ids are random. Joining the discv5 DHT is permissionless, so the attacker can flood the DHT with their own distribution of node ids. In addition, we don't always assume that we will always do the random lookup even though we are doing right now.
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.
It would be nice if there were a defined type for this, such as
ExtendedBlob
:ExtendedBlob
ByteVector[2 * BYTES_PER_BLOB]
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.
How often will
ExtendedBlob
be used? I guess the new type will be rarely used in the code. I'm not sure. I may be wrong.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 imagine
ExtendedBlob
would be used as an intermediate type before converting the data into columns. So it would be used when publishing and reconstructing. You're right though, it might not be very useful. I don't have a strong opinion about having this type or not.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.
Along with poor/no response of requested samples, if a peer isn't subscribed to the required computed column subnets they should be disconnected and banned
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 it would be way better implementation wise if the spec defines a particular time within the slot when we should do the sampling.
Otherwise, there's no distinction between the scenario where a peer who is supposed to custody column
X
which would complicate peer scoring considerably implementation wise.
If the trailing fork choice filter described below is viable security wise, then maybe we only sample at the 4 second mark where validators have to attest to slot N
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 since it's for the past slot, so it should defintely be completed by before you import the block, one could try doing it in the last 3 seconds of previous block?
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, timing is TBD.
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.
Imo it would make sense to start sampling as soon as possible, e.g. as soon as you receive either the block or a sidecar (which has all of the commitments). I am not sure how this affect peer scoring. Perhaps a node could downscore a peer only if they fail to provide a sample after some deadline, and if the node has good certainty that this sample should be available? E.g. for samples for slot n, you could wait until you receive attestations from slot n, and only downscore peers if the block is canonical
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 more information on how the trailing slot fork choice works during network partitions or longer forks ? During any event where you might temporarily have your peers with split views(hard fork, consensus bug), the peers that you are connected to for the particular column subnets will neither have the block or the data. So when sampling for
n-1
from these peers, they will return nothing and you would mark the data as unavailable.How does sampling work when the network is undergoing stress and possible churn ? ( constant peer disconnections and new connections to find peers with the desired column subnets) . Is there a mode where we can fall back to so that a block and its data which was correctly broadcasted and propagated is marked as available rather than unavailable during such a situation.
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.
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 the example is correct for DAS in the same slot? (tightest DA filter)