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 configurable exclude dialer addresses for universe #6543

Merged

Conversation

hansieodendaal
Copy link
Contributor

Description

Added user-configurable communication node addresses that should never be dialled. In Tari Universe the base node and wallet is configured in a listen-only TCP mode with a fake public address and when their addresses are propagated throughout the network, other peers should not try to dial them.

Motivation and Context

The previous implementation was hard-coded and did not work properly.

How Has This Been Tested?

Added unit tests.
System-level testing.

What process can a PR reviewer use to test or verify this change?

Code review.

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@hansieodendaal hansieodendaal marked this pull request as draft September 9, 2024 13:23
Copy link

github-actions bot commented Sep 9, 2024

Test Results (CI)

    3 files    129 suites   35m 2s ⏱️
1 310 tests 1 310 ✅ 0 💤 0 ❌
3 928 runs  3 928 ✅ 0 💤 0 ❌

Results for commit 21717d7.

♻️ 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 Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

Test Results (Integration tests)

 2 files  11 suites   39m 34s ⏱️
36 tests 34 ✅ 0 💤 2 ❌
38 runs  36 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 21717d7.

♻️ This comment has been updated with latest results.

@hansieodendaal hansieodendaal force-pushed the ho_dialler_address_exclude branch 2 times, most recently from d700560 to 583d331 Compare September 10, 2024 05:58
@hansieodendaal hansieodendaal marked this pull request as ready for review September 10, 2024 05:59
@hansieodendaal hansieodendaal changed the title feat: configurable exclude dialer addresses for universe feat: add configurable exclude dialer addresses for universe Sep 10, 2024
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

This looks good so far, I have two issues with the implementation:
1: We should not exclude 172.2.3.4 this is a valid address, we should use 0.0.0.0 which is the special case address and exclude that.
2: We should not ban the node as it will cause issues for that node. We should just exclude it from being dialed. But we should allow that node to dial us.

base_layer/contacts/tests/contacts_service.rs Outdated Show resolved Hide resolved
comms/core/src/connectivity/manager.rs Outdated Show resolved Hide resolved
Addded user configurable communication node addresses that should never be dialled.
@hansieodendaal hansieodendaal force-pushed the ho_dialler_address_exclude branch from 8772813 to 3f38620 Compare September 10, 2024 13:53
@hansieodendaal
Copy link
Contributor Author

Currently, setting public_addresses = ["/ip4/0.0.0.0/tcp/xxxx",] for base node and public_addresses = ["/ip4/0.0.0.0/tcp/yyyy",] for wallet does not work, as the remote peer (i.e. seed node in this case) closes the channel once a connection has been made.

To allow a peer to advertize a public address of "/ip4/0.0.0.0/tcp/xxxx" and accept that it should keep open their inbound connections, but not try to connect to them, is for a next PR.

Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

I think we need to make an distinction between peers and address here.
A peer might have an invalid, excluded address. But if the peer has other valid addresses, it should not be excluded and still be include. We should just ignore the invalid address.

comms/dht/src/connectivity/mod.rs Outdated Show resolved Hide resolved
comms/dht/src/connectivity/mod.rs Outdated Show resolved Hide resolved
SWvheerden
SWvheerden previously approved these changes Sep 10, 2024
@SWvheerden SWvheerden merged commit e113a0e into tari-project:development Sep 10, 2024
16 of 17 checks passed
@hansieodendaal hansieodendaal deleted the ho_dialler_address_exclude branch September 11, 2024 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants