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

[P2P] chore: concurrent bootstrapping #694

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

bryanchriswhite
Copy link
Contributor

Description

Allows P2P bootsrapping to happen concurrently across known bootstrap nodes.

Issue

No issue

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Refactored P2P bootstrapping to its own go routine

Testing

  • make develop_test; if any code changes were made
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@bryanchriswhite bryanchriswhite added p2p P2P specific changes small labels Apr 19, 2023
@bryanchriswhite bryanchriswhite self-assigned this Apr 19, 2023
github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review April 19, 2023 12:39

The check succeeded, dismissing the review comment.

@bryanchriswhite bryanchriswhite force-pushed the chore/concurrent-p2p-bootstrap branch from 97f4223 to af6189a Compare April 19, 2023 12:53
@bryanchriswhite bryanchriswhite marked this pull request as ready for review April 19, 2023 13:21
p2p/bootstrap.go Outdated Show resolved Hide resolved
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [0.0.0.44] - 2023-04-19

- Refactored P2P bootstrapping to its own go routine
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@bryanchriswhite bryanchriswhite marked this pull request as draft April 19, 2023 18:17
@Olshansk Olshansk removed their request for review April 19, 2023 19:39
@reviewpad reviewpad bot added large and removed small labels Apr 24, 2023
@bryanchriswhite
Copy link
Contributor Author

bryanchriswhite commented Apr 24, 2023

@deblasis I'm seeing RPC health check failures during bootstrapping which were previously hidden (not being attempted). I'm not sure if it's related to the DNS updates but wanted to call it out here:

image

@deblasis
Copy link
Contributor

deblasis commented Apr 24, 2023

@deblasis I'm seeing RPC health check failures during bootstrapping which were previously hidden (not being attempted). I'm not sure if it's related to the DNS updates but wanted to call it out here:

image

@bryanchriswhite 👋
At a glance, I don't recognise the validator naming convention so you might be onto something. I don't have an eye on the updated codebase but in my MVP I naively used the validator name to build the logic necessary to look for bootstrap nodes. If I remember well I left a comment explaining my train of thought.

Let me know if this helps otherwise I'll pull the updated codebase and answer with more details.

Edit: these warnings might be normal and simply indicating that one of the two bootstrap nodes that I hardcoded is not reachable but it's by design and it was meant to make sure that the fallback to the next bootstrap node worked. The comment I mentioned above should explain this.

@bryanchriswhite
Copy link
Contributor Author

@deblasis hey! 👋 Good to hear from you and apologies for the autocomplete failure - I meant to tag @okdas. 🤦‍♂️

@Olshansk
Copy link
Member

Slightly unrelated.

@bryanchriswhite I know you're looking into kademlia and such, and just wanted to make sure you were aware they have some of their own native configs: https://docs.libp2p.io/concepts/discovery-routing/kaddht/#bootstrap-process. Found an example here.

Copy link
Member

@okdas okdas left a comment

Choose a reason for hiding this comment

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

@bryanchriswhite Thank you for catching this!

  1. To properly diagnose where the health check error comes from, I think we should add a status code and possibly response body (maybe in debug output?) to figure out what and how responds to the health check. Just want to double-check — this doesn't seem to be used currently? Not until we have peer discovery and possibly state sync?
  2. For concurrency limiter, we already have that package, but I actually like yours more because it allows to pass a context! :)

}
healthCheck, err := client.GetV1Health(context.TODO())
if err != nil || healthCheck == nil || healthCheck.StatusCode != http.StatusOK {
m.logger.Warn().Str("serviceURL", serviceURL).Msg("Error getting a green health check from bootstrap node")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
m.logger.Warn().Str("serviceURL", serviceURL).Msg("Error getting a green health check from bootstrap node")
m.logger.Warn().Str("serviceURL", serviceURL).Int("code", healthCheck.StatusCode).Msg("Error getting a green health check from bootstrap node")

@Olshansk Olshansk removed the large label May 17, 2023
@reviewpad reviewpad bot added the large Pull request is large label May 17, 2023
@bryanchriswhite bryanchriswhite linked an issue May 31, 2023 that may be closed by this pull request
11 tasks
@bryanchriswhite bryanchriswhite mentioned this pull request Jun 26, 2023
13 tasks
@bryanchriswhite bryanchriswhite linked an issue Jun 26, 2023 that may be closed by this pull request
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large p2p P2P specific changes waiting-for-review
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[P2P] Router bootstrapping [P2P] Background gossip using libp2p-pubsub
5 participants