Skip to content
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

Add DataColumnSidecarsByRange v1 Req/Resp #3750

Merged
merged 2 commits into from
May 7, 2024
Merged

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 6, 2024

Add DataColumnSidecarsByRange message for syncing as @jimmygchen suggested in #3574 (comment).

The description was copied and edited from Deneb BlobSidecarsByRange.

@djrtwo mentioned (#3574 (comment)) that considering the load-balancing, it's unclear if plural columns: List[ColumnIndex] or single column: ColumnIndex makes more sense. I now use columns: List[ColumnIndex] for flexibities.

@hwwhww hwwhww added the EIP-7594 PeerDAS label May 6, 2024
@jimmygchen
Copy link
Contributor

Load-balancing is an interesting idea, although it's unclear to me if we need it with the current setup.

When node is performing custody columns sync, they'll be downloading from peers in the same column subnet, so the ByRange requests they send could contain all the columns assigned to that subnet, so I think plural columns make sense. Given the response size wouldn't be much bigger than BlobsByRange today (potentially way smaller depending on custody_requirement), feels like it would be much simpler to download custody columns from the same peer so we don't have to worry about some peers being faster / slower.

When performing historic sampling, we'd likely be requesting the bulk of the samples from a different set of peers (different columns), so it probably wouldn't put too much additional load to the same peer that we're gathering custody from?

Perhaps load-balancing could be use as an optimisation technique to speed up syncing? and in either case, plurals would probably be more flexible.

@dapplion
Copy link
Collaborator

dapplion commented May 6, 2024

👍 on the addition

Want to note that this protocol is helpful to speed up forward sync and backfill sync for a custody peer to quickly fetch its columns. However, it's at odds with the server peer rate-limiting outgoing bandwidth shared between syncing and sampling peers. If by range requests will be rate-limited to the point that they become the bottleneck for syncing then there's not much benefit to just waiting to fetch the block first and then requesting by root.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't have the full context to say more at the moment, but the suggestion make sense

would have to go review the full peerDAS spec as-is now to say more on if this is the best way to handle this...

Copy link
Member

@ppopth ppopth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense and should be implemented.

@hwwhww
Copy link
Contributor Author

hwwhww commented May 7, 2024

I'm going to merge it for the interop milestone requirements. Thank y'all for reviewing it!

@hwwhww hwwhww merged commit 1a5671d into dev May 7, 2024
26 checks passed
@hwwhww hwwhww deleted the DataColumnSidecarsByRange branch May 7, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EIP-7594 PeerDAS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants