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

Swarm Bootstrap Nodes #201

Merged
merged 11 commits into from
Aug 15, 2023
Merged

Swarm Bootstrap Nodes #201

merged 11 commits into from
Aug 15, 2023

Conversation

mriise
Copy link
Contributor

@mriise mriise commented Jul 17, 2023

Description

Adds bootstrap nodes in settings that will be connected to on node startup.

Link to issue

Please add a link to any relevant issues/tickets.

Type of change

  • New feature (non-breaking change that adds functionality)

Please delete options that are not relevant.

Test plan (required)

Parsing testing is added with the other settings test. Tests actually dialing nodes will be added in a different PR since this is a perquisite to gossip/receipt testing anyways.

@mriise mriise requested a review from a team as a code owner July 17, 2023 23:47
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #201 (c37d577) into main (63c23aa) will increase coverage by 0.04%.
The diff coverage is 40.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #201      +/-   ##
==========================================
+ Coverage   75.20%   75.25%   +0.04%     
==========================================
  Files          65       65              
  Lines        6107     6135      +28     
==========================================
+ Hits         4593     4617      +24     
- Misses       1514     1518       +4     
Files Changed Coverage Δ
homestar-runtime/src/event_handler/swarm_event.rs 27.01% <0.00%> (+2.17%) ⬆️
homestar-runtime/src/network/swarm.rs 58.09% <42.10%> (+2.41%) ⬆️
homestar-runtime/src/event_handler.rs 92.68% <100.00%> (+0.18%) ⬆️
homestar-runtime/src/settings.rs 96.29% <100.00%> (+0.04%) ⬆️

homestar-runtime/src/settings.rs Outdated Show resolved Hide resolved
homestar-runtime/src/network/swarm.rs Outdated Show resolved Hide resolved
@mriise
Copy link
Contributor Author

mriise commented Jul 20, 2023

tests will probably fail at the moment since i havent setup nix properly on my laptop yet. needed to get this change uploaded though.

@zeeshanlakhani
Copy link
Contributor

Ok, putting together some additional work here @mriise, to close this out:

  • In swarm_event.rs, need to match on the SwarmEvent::ConnectionEstablished and add kademlia.add_address to add the peer id and peer address. I had implemented this earlier but wanted to tackle this separately again, as here, so 👍🏽.
    • As part of this, we'll want the event handler to keep a local set (fnvhashset) of trusted peers that are actually "connected", vs those we configured.
    • On dial, in the startup here, add addresses to the set and to kademlia. On, SwarmEvent::OutgoingConnectionError and SwarmEvent::ConnectionClosed, remove from the set.
    • Currently, the set is just a "view" on trusted ones that are connected. Could be useful for query for now.
  • The explicit peer piece we discussed, as these are trusted, bootstrap nodes, since we configured them.

Copy link
Contributor

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

@mriise some minor cleanup mentions here and a suggestion for startup cleanup.

homestar-runtime/fixtures/settings.toml Outdated Show resolved Hide resolved
homestar-runtime/src/network/swarm.rs Show resolved Hide resolved
@zeeshanlakhani
Copy link
Contributor

@mriise I'm about to push a small reworking of when scheduling runs vs the ahead-of-time parsing. Let's try to get the last few bits lined up for this one and get it in. I know you've been pushing to the other branch as well :).

@mriise
Copy link
Contributor Author

mriise commented Aug 14, 2023

ci shouldn't be failing like this...

Copy link
Contributor

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

Good work @mriise!

@zeeshanlakhani zeeshanlakhani merged commit ba26eff into main Aug 15, 2023
22 checks passed
@zeeshanlakhani zeeshanlakhani deleted the gossip-bootstrap-cfg branch August 15, 2023 01:05
@release-plz-ipvm-wg release-plz-ipvm-wg bot mentioned this pull request Oct 9, 2023
This was referenced Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants