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

feat(kad): improve automatic bootstrap #5474

Merged

Conversation

stormshield-frb
Copy link
Contributor

@stormshield-frb stormshield-frb commented Jun 18, 2024

Description

As discussed in the last maintainer call, some improvements are probably necessary for the automatic bootstrap feature (introduced by #4838). Indeed, like @drHuangMHT mentioned in #5341 and like @guillaumemichel has agreed, triggering a bootstrap every time an update happens inside the routing table consumes a lot more resources.

The idea behind the automatic bootstrap feature it that, when a peer is starting, if a routing table update happens we probably don't want to wait for the periodic bootstrap to trigger and we want to trigger it right now. However, like @guillaumemichel said, this is something we want to do at startup or when a network connectivity problem happens, we don't want to do that all the time.

This PR is a proposal to trigger automatically a bootstrap on routing table update but only when we have less that K_VALUE peers in it (meaning that we are starting up or something went wrong and the fact that a new peer is inserted is probably a sign that the network connectivity issue is resolved).

I have also added a new triggering condition like mentioned in the maintainer call. When discovering a new listen address and if we have no connected peers, we trigger a bootstrap. This condition is based on our own experience at Stormshield : some peers were starting before the network interfaces were up, doing so, the automatic and periodic bootstrap failed, but when the network interfaces were finally up, we were waiting X minutes for the periodic bootstrap to actually trigger a bootstrap and join the p2p network.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@stormshield-frb stormshield-frb changed the title feat/improve-automatic-bootstrap feat(kad) : improve-automatic-bootstrap Jun 18, 2024
@stormshield-frb stormshield-frb changed the title feat(kad) : improve-automatic-bootstrap feat(kad): improve-automatic-bootstrap Jun 18, 2024
@stormshield-frb stormshield-frb force-pushed the feat/improve-automatic-bootstrap branch from 3229558 to ee83002 Compare June 18, 2024 16:18
@stormshield-frb stormshield-frb changed the title feat(kad): improve-automatic-bootstrap feat(kad): improve automatic bootstrap Jun 18, 2024
Copy link
Contributor

mergify bot commented Jun 18, 2024

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

@stormshield-frb stormshield-frb force-pushed the feat/improve-automatic-bootstrap branch from ee83002 to 7268d7e Compare June 19, 2024 08:51
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks François, and Darius and Gui for the review

@jxs
Copy link
Member

jxs commented Jul 3, 2024

@stormshield-frb stormshield-frb force-pushed the feat/improve-automatic-bootstrap branch from 1982c05 to 0ae16d1 Compare July 3, 2024 15:52
@stormshield-frb
Copy link
Contributor Author

@stormshield-frb can you check what's happening with https://github.com/libp2p/rust-libp2p/actions/runs/9777683040/job/26994305156?pr=5474#step:6:46 ?

Should be corrected @jxs. The use crate::K_VALUE was removed in a recent commit from master that's why it failed.

@stormshield-frb
Copy link
Contributor Author

It seems there is a problem with the runner no ? Almost every job fails, complaining about a unknown command tomlq

@jxs
Copy link
Member

jxs commented Jul 4, 2024

It seems there is a problem with the runner no ? Almost every job fails, complaining about a unknown command tomlq

yeah submitted #5481 to address it, thanks François

@jxs jxs added the send-it label Jul 4, 2024
@mergify mergify bot merged commit 36c2f94 into libp2p:master Jul 4, 2024
72 checks passed
@stormshield-frb stormshield-frb deleted the feat/improve-automatic-bootstrap branch August 28, 2024 09:40
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
As discussed in the last maintainer call, some improvements are probably necessary for the automatic bootstrap feature (introduced by libp2p#4838). Indeed, like @drHuangMHT mentioned in libp2p#5341 and like @guillaumemichel has agreed, triggering a bootstrap every time an update happens inside the routing table consumes a lot more resources.

The idea behind the automatic bootstrap feature it that, when a peer is starting, if a routing table update happens we probably don't want to wait for the periodic bootstrap to trigger and we want to trigger it right now. However, like @guillaumemichel said, this is something we want to do at startup or when a network connectivity problem happens, we don't want to do that all the time.

This PR is a proposal to trigger automatically a bootstrap on routing table update but only when we have less that `K_VALUE` peers in it (meaning that we are starting up or something went wrong and the fact that a new peer is inserted is probably a sign that the network connectivity issue is resolved).

I have also added a new triggering condition like mentioned in the maintainer call. When discovering a new listen address and if we have no connected peers, we trigger a bootstrap. This condition is based on our own experience at Stormshield : some peers were starting before the network interfaces were up, doing so, the automatic and periodic bootstrap failed, but when the network interfaces were finally up, we were waiting X minutes for the periodic bootstrap to actually trigger a bootstrap and join the p2p network.

Pull-Request: libp2p#5474.
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.

4 participants