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.

Sign up for GitHub

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

Jump to bottom

refactor: Improve Kademlia DHT usage #378

Merged
merged 16 commits into from
Oct 17, 2023
Merged

refactor: Improve Kademlia DHT usage #378

merged 16 commits into from
Oct 17, 2023

Conversation

bgins
Copy link
Contributor

@bgins bgins commented Oct 12, 2023 •
edited
Loading

Description

This PR implements the following:

  • Add Kademlia server mode always on
  • Add Kademlia table add and remove debug logs
  • Add Kademlia routing table updated debug log
  • Remove peers that were discovered from DHT (Known peers remain in the DHT)
  • Fix check for matching Homestar protocol in Identify events
  • Add check_lines_for utility function to correlate log outputs on a single line
  • Test that known peers are added to DHT
  • Test that known peers are not removed from DHT on closed connection
  • Test that peers are added to DHT on identify event following mDNS discovery
  • Test that peers that were discovered through mDNS are removed from DHT on closed connection
  • Add utility function to extract a Peer ID from a Multiaddress

Link to issue

Please add a link to any relevant issues/tickets.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • Refactor (non-breaking change that updates existing functionality)

Test plan (required)

We are adding tests for existing functionality. In addition, we added tests to check:

  • Peers are added to the Kademlia table on connection when they been discovered through mDNS
  • Peers are removed from the Kademlia table on disconnection when they were discovered through mDNS
  • Peers are not removed from the Kademlia when they were configured in node_addresses
  • Unit tests to check the Peer ID extraction utility

@bgins
test: Add check_lines_for utility
c81caf7 
Checks process output for predicates in a line
@bgins
fix: Check for matching protocol version
6677820 
We had a logic error where we would early exit from this function on a
matching protocol version. We should exit when the protocol does not
match.
@codecov
Copy link

codecov bot commented Oct 12, 2023 •
edited
Loading

Codecov Report

Merging #378 (091f11e) into main (ea11c27) will increase coverage by 0.36%.
Report is 8 commits behind head on main.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
+ Coverage   72.14%   72.51%   +0.36%     
==========================================
  Files          69       70       +1     
  Lines        6889     6945      +56     
==========================================
+ Hits         4970     5036      +66     
+ Misses       1919     1909      -10
Files Coverage Δ
homestar-runtime/src/event_handler.rs 93.61% <100.00%> (+0.28%) ⬆️
homestar-runtime/src/libp2p/multiaddr.rs 100.00% <100.00%> (ø)
homestar-runtime/src/network/swarm.rs 63.41% <100.00%> (+5.18%) ⬆️
homestar-runtime/src/event_handler/swarm_event.rs 22.15% <24.32%> (+5.63%) ⬆️

@bgins
chore: Add kademlia table add and remove debug logs
6f68bf6
@bgins
refactor: Move homestar protocol check earlier
3a4104c 
We want to check before we add an identified node to external addresses.
@bgins bgins force-pushed the bgins/add-libp2p-integration-tests branch from bd66198 to 09a5b7e Compare October 16, 2023 19:50
@bgins bgins changed the title test: Add more network tests refactor: Improve Kademlia DHT usage Oct 16, 2023
@bgins bgins force-pushed the bgins/add-libp2p-integration-tests branch from 09a5b7e to 3a12403 Compare October 16, 2023 20:13
@bgins bgins force-pushed the bgins/add-libp2p-integration-tests branch from 64ffcfa to cf417a8 Compare October 16, 2023 23:01
@bgins bgins marked this pull request as ready for review October 17, 2023 17:36
@bgins bgins requested a review from a team as a code owner October 17, 2023 17:36
@bgins bgins self-assigned this Oct 17, 2023
zeeshanlakhani
zeeshanlakhani approved these changes Oct 17, 2023
View reviewed changes
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.

some minor nits, but great work otherwise @bgins

homestar-runtime/src/event_handler/swarm_event.rs Outdated Show resolved Hide resolved
homestar-runtime/src/libp2p/multiaddr.rs
@@ -0,0 +1,36 @@
use libp2p::{multiaddr::Protocol, Multiaddr, PeerId};
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

homestar-runtime/src/network/swarm.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler/swarm_event.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler/swarm_event.rs Show resolved Hide resolved
@bgins bgins merged commit 91aae7f into main Oct 17, 2023
27 checks passed
@bgins bgins deleted the bgins/add-libp2p-integration-tests branch October 17, 2023 21:56
@release-plz-ipvm-wg release-plz-ipvm-wg bot mentioned this pull request Jan 19, 2024
Closed
@release-plz-ipvm-wg release-plz-ipvm-wg bot mentioned this pull request Jan 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeeshanlakhani zeeshanlakhani zeeshanlakhani approved these changes

Assignees

@bgins bgins

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bgins @zeeshanlakhani