-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: new behavior for network bootstrapping #1257
base: main
Are you sure you want to change the base?
Conversation
@matteojug I ended up doing quite a bit of refactoring, sorry :( So this probably needs a fresh look. |
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.
Just had some nits, and one question.
.map(|either, _| either.into_inner()) | ||
.boxed(); | ||
} | ||
let tcp_config = tcp::Config::default().nodelay(true); |
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.
What is the significance of the nodelay
here.
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 disables Nagle's Algorithm on the socket which is enabled by default on most OS's. I think this option might be set by default now, but I added it explicitly to be, well, explicit as to our intentions.
.map_err(|e| SignerSwarmError::LibP2P(Box::new(e)))?; | ||
} | ||
{ | ||
let mut swarm = self.swarm.lock().await; |
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 just locked, and unlock the swarm a moment ago for the local_peer_id
. Maybe we should move that log down here.
enable_quic_transport: false, | ||
enable_memory_transport: false, | ||
initial_bootstrap_delay: Duration::from_secs(0), |
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.
nit: this could be Duration::ZERO
.
Description
Closes: #1133
Changes
Testing Information
Existing tests pass and I've tested this quite a bit in
devenv
. I'll add some actual tests for the behavior, but PR'ing this now for preview.Checklist: