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

Blocksync: Implement dynamic request length #801

Closed
amerameen opened this issue Nov 2, 2020 · 2 comments · Fixed by #892
Closed

Blocksync: Implement dynamic request length #801

amerameen opened this issue Nov 2, 2020 · 2 comments · Fixed by #892
Assignees
Labels
Priority: 3 - Medium Nice-to-have, does not impede core functionality

Comments

@amerameen
Copy link
Contributor

amerameen commented Nov 2, 2020

Issue summary

Currently, for Blocksync, we're using a static request length (10) (the number of blocks requested at once).

It would be better to have dynamic requests (i.e. make longer requests when we can successfully, or make shorter requests when they fail)

For example: Lotus starts with 500 and if request fails, they shrink request size.

Acceptance Criteria
The Blocksync request lengths are dynamic and adjust (i.e. get bigger or smaller) based on what's feasible with current peers and network conditions.

@amerameen amerameen added Blockchain Priority: 4 - Low Limited impact and can be implemented at any time Status: Needs Triage Issue has unresolved discussions and/or needs to be assigned a priority and assignee labels Nov 2, 2020
@austinabell
Copy link
Contributor

austinabell commented Nov 2, 2020

To clarify, Lotus has a default window size of 500, and they send the request to 16 peers before changing window size (used to be 5, so this should also change on our side, assuming there is a good reason for this).

Ideally it would be good to benchmark this on our own client, and make sure that we have sufficient enough channel sizes through libp2p to handle receiving 500 tipset headers over the network (it is a lot)

@austinabell austinabell removed the Status: Needs Triage Issue has unresolved discussions and/or needs to be assigned a priority and assignee label Nov 2, 2020
@amerameen amerameen added Priority: 3 - Medium Nice-to-have, does not impede core functionality and removed Priority: 4 - Low Limited impact and can be implemented at any time labels Nov 2, 2020
@dutterbutter
Copy link
Contributor

After discussing with @austinabell rather than having a dynamic window size as described above, I will be implementing a sync configuration that will sit within the chainsyncer where users can update the window size via command line as well as the number worker tasks related to syncing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 3 - Medium Nice-to-have, does not impede core functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants