-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat!: add multiaddr with range checks for use with universe #6557
Conversation
Test Results (CI) 3 files 129 suites 35m 5s ⏱️ Results for commit 0fbfdf4. ♻️ This comment has been updated with latest results. |
49b10b3
to
3fb29c6
Compare
3fb29c6
to
46ca15b
Compare
Test Results (Integration tests) 2 files 11 suites 19m 37s ⏱️ For more details on these failures, see this check. Results for commit 0fbfdf4. ♻️ This comment has been updated with latest results. |
d721377
to
4cdc43a
Compare
4cdc43a
to
6a796f3
Compare
There was a problem hiding this 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
6a796f3
to
9d5874d
Compare
Added MultiaddrRange, which implements range checks for Multiaddr, when using IP4 with TCP. This enables specifying a range of IP4 with TCP addresses.
9d5874d
to
0fbfdf4
Compare
I found specifying as below intuitive and flexible.
|
There was a problem hiding this 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.
Description
Added
MultiaddrRange
, which implements range checks forMultiaddr
, 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:
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?
From the seed node to the universe wallet
From the seed node to the universe base node
From the universe base node to the universe wallet
From the universe base node to the seed node
What process can a PR reviewer use to test or verify this change?
Breaking Changes
BREAKING CHANGE: The wallet FFI interface changed (in
fn comms_config_create
)