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

feat: block endless peer stream #5951

Merged
merged 2 commits into from Nov 14, 2023
Merged

feat: block endless peer stream #5951

merged 2 commits into from Nov 14, 2023

Conversation

SWvheerden
Copy link
Collaborator

Description

Block endless peer stream from remote peer.
Block duplicate peers.

Motivation and Context

See #5811

Fixes #5811

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 13, 2023 •
edited

Test Results (CI)

1 258 tests   1 258 ✔️  10m 45s ⏱️
     39 suites         0 💤
       1 files           0 ❌

Results for commit b1f6ba1.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Nov 13, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 13, 2023 •
edited

Test Results (Integration tests)

31 tests  +31   31 ✔️ +31   14m 42s ⏱️ + 14m 42s
11 suites +11     0 💤 ±  0 
  2 files   +  2     0 ❌ ±  0 

Results for commit b1f6ba1. ± Comparison against base commit 4cbdfec.

♻️ This comment has been updated with latest results.

AaronFeickert
AaronFeickert reviewed Nov 13, 2023
View reviewed changes
Copy link
Collaborator

@AaronFeickert AaronFeickert left a comment

Choose a reason for hiding this comment

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

Is it straightforward to include unit tests exercising the two new failure modes?

@SWvheerden
Copy link
Collaborator Author

I thought about it, but it would mean we need to a full malicious server client.

sdbondi
sdbondi previously approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

utACK

comms/dht/src/network_discovery/discovering.rs Outdated Show resolved Hide resolved
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 14, 2023
@sdbondi
Copy link
Member

sdbondi commented Nov 14, 2023

There is a mock DHT RPC implementation that you could set to return duplicates or too many peers and then pass that into the discovery module. Will likely be a fair amount of work.

DHT Mock service
https://github.com/tari-project/tari/blob/development/comms/dht/src/rpc/mock.rs
Examples of making calls to a mocked service
https://github.com/tari-project/tari/blob/development/comms/core/src/protocol/rpc/test/smoke.rs
https://github.com/tari-project/tari/blob/development/comms/core/src/protocol/rpc/client/tests.rs

@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 14, 2023
@SWvheerden SWvheerden merged commit 16b325d into tari-project:development Nov 14, 2023
14 checks passed
@SWvheerden SWvheerden deleted the sw_block_duplicate_peers branch November 14, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronFeickert AaronFeickert AaronFeickert left review comments

@sdbondi sdbondi sdbondi left review comments

Assignees
No one assigned
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Peer can hold the peer-stream open indefinitely
4 participants
@SWvheerden @sdbondi @AaronFeickert @ghpbot-tari-project