Skip to content

Conversation

@g11tech
Copy link
Contributor

@g11tech g11tech commented May 16, 2025

PR - #4284 simplified DataColumnSidecarsByRoot request.

this PR further proposes to even pull out columns from the identifiers because a client generally would request same missing columns for a set of roots and hence don't need to keep repeating the same columns again and again for reach block root.

this further shortens the size of request making p2p traffic lighter and should be an easy update to the api usage in the clients.
it may also help the responders to quickly analyze the columns that they need to respond to the request without iterating the entire request which my offer a small space for optimization as well.

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think this change makes sense.

PS: please run make lint to fix the failing CI check.

@jtraglia jtraglia added the fulu label May 16, 2025
@ppopth
Copy link
Member

ppopth commented May 19, 2025

This PR makes sense to me. Having DataColumnsByRootIdentifier is quite redundant.

@tersec
Copy link
Contributor

tersec commented May 21, 2025

this further shortens the size of request making p2p traffic lighter

it may also help the responders to quickly analyze the columns that they need to respond to the request without iterating the entire request which my offer a small space for optimization as well.

can you quantify these? If it's an optimization, it should be quantitative.

@pawanjay176
Copy link
Contributor

In Lighthouse, we only request a single root in all our byroot request. So this change isn't really useful for us imo.
Not sure if we are thinking of asking for multiple roots in the same request.
cc @dapplion

@jtraglia
Copy link
Member

jtraglia commented May 27, 2025

Hey @g11tech, it appears that this change doesn't have a lot of support.

@g11tech
Copy link
Contributor Author

g11tech commented May 28, 2025

closing the PR as the current/future usage patterns don't warrant this optimization

@g11tech g11tech closed this May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants