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: Add DHT and RequestResponse notifications #461

Merged
merged 54 commits into from
Jan 11, 2024

Conversation

bgins
Copy link
Contributor

@bgins bgins commented Dec 4, 2023

Description

This PR implements the following changes:

  • Add DHT notifications
    • Put receipt and workflow info to DHT notifications
    • Got receipt and workflow info from DHT notifications
    • Receipt and workflow info quorum success notifications
    • Receipt and workflow info quorum failure notifications
  • Add request-response notifications
    • Sent workflow info to peer
    • Received workflow info from peer
  • Update notifications to accept Ipld data, not just strings
  • Refactor swarm FoundEvent into DecodedRecord and FoundEvent (to include extra information on FoundEvent without overloading decoding mechanism)
    • Add better error for timed out records (records with the code "Timeout")
  • Fix IndexedResources decoding and add/update unit tests to check it
  • Fix flaky test_libp2p_receipt_gossip_serial integration test
  • Remove no connected peer checks (we dial up a peer if they are not connected but stored in the DHT)
  • Rename check_lines_for test utility to check_for_line_with
  • Remove unused handler_timeout_fns
  • Move defaults.toml from homestar-runtime/fixtures to homestar-runtime/config
  • Update ReceivedReceiptPubsub notification "peerId" field to "publisher" to better reflect the role of the peer (breaking change for clients).
  • Remove receipt persistence on receiving workflow info (to be replaced with another mechanism)
  • Break retrieve_from_query into retrieve_from_dht and retrieve_from_provider
  • Update retrieve workflow info from provider mechanism to trigger on retrieve_from_dht error or timeout
  • Update DHT behavior to only add addresses manually with the kad::BucketInserts::Manual configuration
  • Handle swarm RequestResponse ResponseSent event with a debug log (instead of leaving it uncaught)
  • Increase threads allocated per test

Link to issue

Closes #131
Closes #475

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Refactor (non-breaking change that updates existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

The breaking change is the update to the ReceivedReceiptPubsub notification.

Test plan (required)

We've added tests to check:

  • Put receipt and workflow info to DHT
  • Got workflow info from DHT notifications
  • Receipt and workflow info quorum success
  • Receipt and workflow info quorum failure
  • Provider sent workflow info to peer
  • Peer received workflow info from a provider
  • Peer received workflow info indirectly through a recursive request (Test written, but ignored until we can isolate nodes)

We also have the starts for a test of got receipt from DHT that will be used with future updates to receipt retrieval (in the test_libp2p_dht_records_serial test, but commented out)

@bgins bgins added the networking Features, functionality involving networking label Dec 4, 2023
@bgins bgins self-assigned this Dec 4, 2023
@bgins bgins changed the title Bgins/add dht req resp notifications Add DHT and RequestResponse notifications Dec 4, 2023
@bgins bgins changed the title Add DHT and RequestResponse notifications feat: Add DHT and RequestResponse notifications Dec 4, 2023
@bgins bgins force-pushed the bgins/add-dht-req-resp-notifications branch 2 times, most recently from 16e3184 to 56035fe Compare December 11, 2023 18:17
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 407 lines in your changes are missing coverage. Please review.

Comparison is base (b1cf4da) 70.26% compared to head (fce81d0) 68.14%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #461      +/-   ##
==========================================
- Coverage   70.26%   68.14%   -2.13%     
==========================================
  Files          80       81       +1     
  Lines        9128     9476     +348     
==========================================
+ Hits         6414     6457      +43     
- Misses       2714     3019     +305     
Files Coverage Δ
homestar-runtime/src/event_handler.rs 95.77% <100.00%> (+0.06%) ⬆️
homestar-runtime/src/network/swarm.rs 52.91% <100.00%> (-0.56%) ⬇️
homestar-runtime/src/settings.rs 98.23% <100.00%> (ø)
homestar-runtime/src/workflow.rs 90.71% <100.00%> (-1.29%) ⬇️
homestar-runtime/src/workflow/settings.rs 100.00% <ø> (ø)
homestar-runtime/src/settings/libp2p_config.rs 95.23% <50.00%> (-4.77%) ⬇️
homestar-runtime/src/worker.rs 70.48% <85.00%> (+0.89%) ⬆️
homestar-runtime/src/runner.rs 61.47% <0.00%> (-0.42%) ⬇️
homestar-runtime/src/event_handler/notification.rs 66.47% <64.70%> (-2.35%) ⬇️
...ar-runtime/src/event_handler/swarm_event/record.rs 80.89% <80.89%> (ø)
... and 4 more

... and 1 file with indirect coverage changes

@bgins bgins force-pushed the bgins/add-dht-req-resp-notifications branch 2 times, most recently from 569b44a to dd4661f Compare December 13, 2023 05:42
@bgins bgins added the dx Developer experience applications and improvements label Dec 15, 2023
@bgins bgins force-pushed the bgins/add-dht-req-resp-notifications branch from 8aff5e3 to 775c3ef Compare January 8, 2024 22:59
@bgins bgins marked this pull request as ready for review January 8, 2024 23:01
@bgins bgins requested a review from a team as a code owner January 8, 2024 23:01
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.

Great work. I finally got through it. A mix-match of comments/minor-fixes. Also, want to chat on one thing @bgins.

.config/nextest.toml Outdated Show resolved Hide resolved
homestar-runtime/tests/network/dht.rs Outdated Show resolved Hide resolved
homestar-runtime/src/event_handler/event.rs Outdated Show resolved Hide resolved
homestar-runtime/src/workflow/info.rs Outdated Show resolved Hide resolved
homestar-runtime/src/workflow/info.rs Show resolved Hide resolved
homestar-runtime/src/workflow/info.rs Outdated Show resolved Hide resolved
homestar-runtime/src/workflow/info.rs Outdated Show resolved Hide resolved
homestar-runtime/src/workflow/info.rs Outdated Show resolved Hide resolved
@zeeshanlakhani
Copy link
Contributor

As discussed with @bgins, there are a few changes I'll make in #496 as part of the rebase.

@bgins bgins merged commit 5c876c5 into main Jan 11, 2024
27 checks passed
@bgins bgins deleted the bgins/add-dht-req-resp-notifications branch January 11, 2024 21:22
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
dx Developer experience applications and improvements networking Features, functionality involving networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DHT and workflow info provider notifications Networking: Multi-node libp2p testing ⚙️
3 participants