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

DEMO only: reproduce node many to many msg fail #206

Closed
wants to merge 13 commits into from

Conversation

oetyng
Copy link
Contributor

@oetyng oetyng commented Apr 28, 2023

For demonstrating this issue. Not for merge (at least not now).

A client sends a ClientPing to close group nodes of a given name.
Those nodes send a NodePing to close group nodes of the hash of the original name.
A node that receives a NodePing, will return NodePong(Ok(())) if it considers itself to be among the close group nodes of the given name.

The nodes receiving a ClientPing, will return a NodePong(Ok(())) if a majority of the nodes they queried, returned a NodePong(Ok(())).

The test will succeed if the client receives a majority of NodePong(Ok(())).

@reviewpad
Copy link

reviewpad bot commented Apr 28, 2023

AI-Generated Summary: This pull request introduces a new feature for handling ping-pong messages between nodes and clients. It adds two new variants to the Cmd enum - NodePing and ClientPing, as well as a new variant to the DataAddress enum - PingPong. Additionally, new functionalities are added to handle these new message types and to perform many-to-many node pinging. Finally, an end-to-end test node_many_to_many_ping_pong_succeeds has been added to ensure the correct behavior of the new functionality. The changes affect 5 files with a total of 111 insertions.

@reviewpad reviewpad bot requested a review from bochaco April 28, 2023 12:25
@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review labels Apr 28, 2023
@reviewpad
Copy link

reviewpad bot commented Apr 28, 2023

Reviewpad Report

⚠️ Warnings

  • Please link an issue to the pull request

@oetyng oetyng added bug Something isn't working tests labels Apr 28, 2023
@reviewpad
Copy link

reviewpad bot commented Apr 28, 2023

AI-Generated Summary: This pull request includes two patches:

  1. The first patch introduces a test to reproduce a many-to-many message failure in nodes. It makes changes in the following files: 'safenode/src/domain/storage/address/mod.rs', 'safenode/src/node/api.rs', 'safenode/src/protocol/messages/cmd.rs', 'safenode/src/protocol/messages/response.rs', and 'safenode/src/tests_e2e/mod.rs', adding a total of 111 insertions.

  2. The second patch adds the 'node_many_to_many' test to the continuous integration configuration. It updates the '.github/workflows/merge.yml' file by adding 6 new lines.

@reviewpad
Copy link

reviewpad bot commented Apr 28, 2023

AI-Generated Summary: This pull request consists of 3 patches:

  1. Adding a new test to reproduce the many-to-many message failure, as well as introducing new PingPong and related commands in various routers.
  2. Adding the node_many_to_many test to the Continuous Integration workflow.
  3. Introducing a small delay in the test to allow the client to find enough nodes before sending the pingpong message.

@oetyng oetyng force-pushed the verify-many-to-may-node-msg-issues branch from 1df5398 to f5fca09 Compare April 28, 2023 13:11
@reviewpad
Copy link

reviewpad bot commented Apr 28, 2023

AI-Generated Summary: This pull request contains changes related to improving node communication with the introduction of many-to-many ping pong messages. The changes span across 5 different files, with a total of 111 insertions. The primary changes include adding PingPong enum variant to DataAddress, handling new Cmd variants (NodePing and ClientPing) in the node api, and updating the Cmd enum with those new variants. Additionally, an end-to-end test named 'node_many_to_many_ping_pong_succeeds' has been added to verify the functionality. The test is also included in the CI configuration for running on pull requests with Ubuntu. Finally, a small delay has been inserted to allow the client to find enough nodes during testing.

@reviewpad
Copy link

reviewpad bot commented Apr 28, 2023

AI-Generated Summary: This pull request consists of 4 patches that aim to reproduce and fix a node many-to-many message failure by introducing new ping-pong messages, as well as updating the CI and adjusting the test process.

  1. The first patch adds a test node_many_to_many_ping_pong_succeeds to reproduce the issue, and implements relevant changes in safenode/src/domain/storage/address/mod.rs, safenode/src/node/api.rs, safenode/src/protocol/messages/cmd.rs, safenode/src/protocol/messages/response.rs, and safenode/src/tests_e2e/mod.rs.

  2. The second patch updates the CI in .github/workflows/merge.yml to include the new node_many_to_many test.

  3. The third patch adds a small delay in the test process to allow the client to find enough nodes in safenode/src/tests_e2e/mod.rs.

  4. The fourth patch increases the sleep time in the test from 15 seconds to 30 seconds to account for the slow CI environment in safenode/src/tests_e2e/mod.rs.

@reviewpad
Copy link

reviewpad bot commented Apr 29, 2023

AI-Generated Summary: This pull request contains a variety of updates related to handling ping-pong messages, improving the efficiency of communication with local and queried closest nodes, and enhancing concurrent access safety using async/await, Arc and RwLock. Major changes include adding new variants to the DataAddress, CmdResponse, Cmd, and SwarmCmd enums, refactoring functions to use async/await, adopting Arc and RwLock for safe concurrent access on structs like SpendQ and SpendStorage, and altering methods and constants in the network module to improve communication with closest peers, both locally and queried. The timeouts for request and connection keep-alive have been increased for better stability. New tests have been added to ensure the proper functioning of the updated components.

@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Apr 29, 2023
@oetyng oetyng force-pushed the verify-many-to-may-node-msg-issues branch from ebb668b to d435275 Compare April 29, 2023 15:14
@reviewpad
Copy link

reviewpad bot commented Apr 29, 2023

AI-Generated Summary: This pull request includes changes across several files related to network structure, node and client connectivity, asynchronous functions, and testing. Key modifications involve updates to the Network struct, the addition of new methods for querying and getting closest peers, the enhancement of async compatibility in the Transfers struct and the spends.rs file, and new end-to-end tests. Additionally, the GitHub Action workflow is updated with a new "Run many-to-many issue repro" test, and various enums and method calls have been altered in multiple files. Overall, these changes aim to improve the code's structure, functionality, and performance.

@oetyng oetyng marked this pull request as draft April 29, 2023 21:04
@oetyng oetyng changed the title test: reproduce node many to many msg fail DEMO only: reproduce node many to many msg fail Apr 29, 2023
@oetyng oetyng closed this May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working DoNotMerge large Pull request is large tests waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant