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

Rename BlockSync -> ChainExchange and tweak parameters #852

Merged
merged 5 commits into from
Nov 16, 2020

Conversation

austinabell
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • I was tweaking parameters to test syncing and decided to just do this now while I'm testing other things
  • Deduplicates RequestResponse codecs using generics

Reference issue to close (if applicable)

Other information and links

@austinabell austinabell changed the title Switch BlockSync -> ChainExchange and tweak parameters Rename BlockSync -> ChainExchange and tweak parameters Nov 14, 2020
yamux_config.set_max_buffer_size(1 << 20);
yamux_config.set_receive_window(1 << 20);
yamux_config.set_max_buffer_size(16 * 1024 * 1024);
yamux_config.set_receive_window(16 * 1024 * 1024);
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain a little how these parameters are decided on? You said in the PR description the window size is now large enough, but 1 << 20 is larger than 16 * 1024 * 1024...

Copy link
Contributor Author

@austinabell austinabell Nov 16, 2020

Choose a reason for hiding this comment

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

nah it isn't (16_777_216 > 1_048_576) I updated the window size to be equal to the default go one, just to make sure no issues. 1<<20 was an arbitrary number I didn't think we would hit lol.

2^4*2^10*2^10 > 2^20 (16x larger)

Copy link
Member

Choose a reason for hiding this comment

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

oh shoot, youre right.... throws out my math degree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks smaller though, right?

Copy link
Member

@ec2 ec2 left a comment

Choose a reason for hiding this comment

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

Everything LGTM, just wanna understand the Yamux param changes a little more... The generic codecs is pretty nice <3 very cool

@austinabell austinabell merged commit 0c1febb into main Nov 16, 2020
@austinabell austinabell deleted the austin/bstweak branch November 16, 2020 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants