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 multiaddr with range checks for use with universe #6557

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Sep 11, 2024

Description

Added MultiaddrRange, which implements range checks for Multiaddr, when using IP4 with TCP. This enables specifying a range of IP4 with TCP addresses. As an exmple, any communication node can enable test addresses to connect to them (allow_test_addresses = true), but refrain from dialling any test addresses in return (excluded_dial_addresses = ["/ip4/127.*.*.*/tcp/*"]).

With application to universe:

  • TCP seed node settings:
    allow_test_addresses = true
    excluded_dial_addresses = [
          "/ip4/127.*.*.*/tcp/0:18188", 
          "/ip4/127.*.*.*/tcp/18190:65534",
          "/ip4/127.0.0.0/tcp/18189",
          "/ip4/127.1:255.1:255.2:255/tcp/18189"
    ] # Only '/ip4/127.0.0.1/tcp/0:18189' allowed
    
  • Universe base node settings:
    type = "tcp"
    public_addresses = ["/ip4/127.0.0.1/tcp/18189"]
    tcp.listener_address = "/ip4/0.0.0.0/tcp/18189"
    allow_test_addresses = true
    #excluded_dial_addresses = []
    
  • Universe wallet settings:
    dns_seeds = []
    custom_base_node = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx::/ip4/127.0.0.1/tcp/18189"
    type = "tcp"
    public_addresses = ["/ip4/127.0.0.1/tcp/18188"]
    tcp.listener_address = "/ip4/0.0.0.0/tcp/18188"
    allow_test_addresses = true
    #excluded_dial_addresses = []
    

Motivation and Context

Currently, Universe base nodes and wallets use /ip4/172.2.3.4/tcp/18189 and /ip4/172.2.3.4/tcp/18188 as their public addresses respectively, but any node trying to contact them is not able to. This results in many wasted resources. The Universe wallets also maintain connections with the seed nodes, which is not ideal.

How Has This Been Tested?

  • Added new unit tests
  • System-level testing using the suggested settings^ (simulated seed node, simulated universe base node, simulated universe wallet)

From the seed node to the universe wallet

>> add-peer 4602fb85883fec887e6b5e5a93cbc9547f19817685e78ff0a9e585826e322b44 /ip4/127.0.0.1/tcp/18188
Peer with node id '9a7764f742bf4e4bae6c5bd4f6' was added to the base node.
>> ☎️  Dialing peer...
☠️ ConnectionFailed: All peer addresses are excluded for peer 9a7764f742bf4e4bae6c5bd4f6

From the seed node to the universe base node

>> add-peer ee9ad9dce31a2d4a9225f4965e50df98ae4f85b58f94b34b9db9cc44f2aa2921 /ip4/127.0.0.1/tcp/18189
Peer with node id '998eb49cf4f2dd3b3d5a394c8e' was added to the base node.
☎️  Dialing peer...
⚡️ Peer connected in 0ms!
Connection: Id: 1, Node ID: 998eb49cf4f2dd3b, Direction: Inbound, Peer Address: /ip4/192.168.5.114/tcp/62398, Age: 1913s, #Substreams: 2, #Refs: 2

From the universe base node to the universe wallet

>> add-peer 4602fb85883fec887e6b5e5a93cbc9547f19817685e78ff0a9e585826e322b44 /ip4/127.0.0.1/tcp/18188
Peer with node id '9a7764f742bf4e4bae6c5bd4f6' was added to the base node.
☎️  Dialing peer...
⚡️ Peer connected in 0ms!
Connection: Id: 8, Node ID: 9a7764f742bf4e4b, Direction: Inbound, Peer Address: /ip4/127.0.0.1/tcp/62412, Age: 2054s, #Substreams: 6, #Refs: 2

From the universe base node to the seed node

>> add-peer 6677c4d401b98f403de671712a98ad8bf2976db27a1d411b08bedfd86751e048 /ip4/192.168.5.114/tcp/9991
Peer with node id '85d605836f02951c65651f99d0' was added to the base node.
☎️  Dialing peer...
>> ⚡️ Peer connected in 0ms!
Connection: Id: 0, Node ID: 85d605836f02951c, Direction: Outbound, Peer Address: /ip4/192.168.5.114/tcp/9991, Age: 2050s, #Substreams: 2, #Refs: 3

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

  • Code review
  • System-level testing

Breaking Changes

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

BREAKING CHANGE: The wallet FFI interface changed (in fn comms_config_create)

Copy link

github-actions bot commented Sep 11, 2024

Test Results (CI)

    3 files    129 suites   35m 5s ⏱️
1 322 tests 1 322 ✅ 0 💤 0 ❌
3 964 runs  3 964 ✅ 0 💤 0 ❌

Results for commit 0fbfdf4.

♻️ 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 11, 2024
@hansieodendaal hansieodendaal requested review from SWvheerden and stringhandler and removed request for SWvheerden September 11, 2024 14:45
@hansieodendaal hansieodendaal marked this pull request as draft September 11, 2024 15:59
Copy link

github-actions bot commented Sep 11, 2024

Test Results (Integration tests)

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

For more details on these failures, see this check.

Results for commit 0fbfdf4.

♻️ This comment has been updated with latest results.

@hansieodendaal hansieodendaal force-pushed the ho_multiaddr_range branch 2 times, most recently from d721377 to 4cdc43a Compare September 12, 2024 13:14
@hansieodendaal hansieodendaal marked this pull request as ready for review September 12, 2024 13:35
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, just one question about the implementation.
Concept of wild card and range is interesting, and would def work. I would have expected to block network segments,
as in if you want to block for example 127.0.0.1, you had 127.0.0.1/32
if you want to block 127.0.. you block 127.0.0.0/16
They way IP addresses work, I don't think you will ever find a need to block 127.0.*.1

base_layer/wallet_ffi/src/lib.rs Outdated Show resolved Hide resolved
comms/core/src/net_address/multiaddr_range.rs Outdated Show resolved Hide resolved
Added MultiaddrRange, which implements range checks for Multiaddr, when using IP4
with TCP. This enables specifying a range of IP4 with TCP addresses.
:
flexible toml parsing
@hansieodendaal
Copy link
Contributor Author

This looks good, just one question about the implementation. Concept of wild card and range is interesting, and would def work. I would have expected to block network segments, as in if you want to block for example 127.0.0.1, you had 127.0.0.1/32 if you want to block 127.0.. you block 127.0.0.0/16 They way IP addresses work, I don't think you will ever find a need to block 127.0.*.1

I found specifying as below intuitive and flexible.

let config_str = r#"something = [
            "/ip4/127.*.100:200.*/tcp/18000:19000",
            "/ip4/127.0.150.1/tcp/18500",
            "/ip4/127.0.0.1/udt/sctp/5678",
            "/ip4/127.*.*.*/tcp/*"
        ]"#;

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.

What you have done seems intuitive in terms of filtering in general, but its not for networking as that's not how IP addresses work. Thats the point I am making. Networks work in bits and you have your mask (/x at the end) to indicate how many bits are for the network address, and how many bits are for the individual devices on the network.

For example 127.0.0.1/24 means that the network is 127.0.0 and the 1 is the device id, so 127.0.0.1 and 127.0.0.2 are in the same network while 127.0.1.1 and 127.0.0.1 is not.
If the IP address is 127.0.0.1/22 then it shares a network with 127.0.0.3.254. They are the same network. the . in the IP address mean nothing, they are only there to make it easier to read.

If you take for example the IP we "spam" a lot 172.2.3.4, its part of 172.2.0.0/22 network, so we cant block that with a single command as that network runs from 172.2.0.1 - 172.2.3.254

But I will iterate this is fine for now, but we might need to revisit this in future as that's not how IP addresses work.

@SWvheerden SWvheerden merged commit aca61f3 into tari-project:development Sep 18, 2024
16 of 17 checks passed
@hansieodendaal hansieodendaal deleted the ho_multiaddr_range branch September 18, 2024 08:36
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Sep 19, 2024
@hansieodendaal hansieodendaal changed the title feat: add multiaddr with range checks for use with universe feat!: add multiaddr with range checks for use with universe Sep 20, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants