8000 p2p: `Switch` dial excludes peers from same machine (IP) · Issue #4494 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

p2p: Switch dial excludes peers from same machine (IP) #4494

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
Open
zivkovicmilos opened this issue Nov 16, 2024 · 4 comments
Open

p2p: Switch dial excludes peers from same machine (IP) #4494

zivkovicmilos opened this issue Nov 16, 2024 · 4 comments
Labels
needs-information Waiting for additional information or feedback

Comments

@zivkovicmilos
Copy link
zivkovicmilos commented Nov 16, 2024 •
edited
Loading

Bug Report / Question

Description

The current implementation of the p2p Switch keeps track of peers using their verified IDs.
However, if a peer connection originates / is directed towards a machine whose IP is already stored in the peer store (from a different peer), regardless of the port, the potential peer dial is ignored:

cometbft/p2p/switch.go

Lines 571 to 575 in c6fa225

func (sw *Switch) IsDialingOrExistingAddress(addr *na.NetAddr) bool {
return sw.dialing.Has(string(addr.ID)) ||
sw.peers.Has(addr.ID) ||
(!sw.config.AllowDuplicateIP && sw.peers.HasIP(addr.IP))
}

Example:

Peer A has peers:

  • cosmos123@127.0.0.1:3000
  • cosmos456@187.155.80.202:2525

If a new peer, originating either from 127.0.0.1 or 187.155.80.202, but from a different port (ex. 3001 / 2526) is considered for a dial on Peer A, they are ignored because a peer with the same IP is already present -- even though they can be 2 completely separate peers (clients) running on the same machine (but on different ports).

Edit:
My bigger question here is -- why does AllowDuplicateIP exist, why not do this by default? What is the benefit of excluding peers from the same machine?
I can only assume it's some kind of security measure to block specific endpoints from initiating connections, but IPs are easily spoofed.

cc @melekes

How to reproduce it

Craft a unit test that creates 3 Switch instances, with Transports running on different ports (same IP of course), and have Switch 0 connect to Switch 1 and Switch 2. (Switch 0 is the only dialer in this scenario, turn off discovery)

If Switch 0 connects to Switch 1 first, the dial to Switch 2 should be ignored, because Switch 1 and Switch 2 share the same IP, but run on different ports -- this shouldn't be the case in reality, since different client processes can originate from the same machine.
@zivkovicmilos zivkovicmilos added bug Something isn't working needs-triage This issue/PR has not yet been triaged by the team. labels Nov 16, 2024
@cason
Copy link
Contributor
cason commented Nov 21, 2024

I don't think the reported scenario can be observed when p2p.allow_duplicate_ip = true is present in the configuration file.

Actually, we always use this configuration when running unit tests:

cometbft/p2p/switch_test.go

Lines 32 to 36 in fc4e719

func init() {
cfg = config.DefaultP2PConfig()
cfg.PexReactor = true
cfg.AllowDuplicateIP = true
}

Exactly because all nodes end up having the same IP (e.g., 127.0.0.1).

@cason
Copy link
Contributor
cason commented Nov 21, 2024

My bigger question here is -- why does AllowDuplicateIP exist, why not do this by default? What is the benefit of excluding peers from the same machine?

This flag was introduced, with a different name, by this commit: 5796e87. It works like this from 2018.

This comment is also useful: tendermint/tendermint#1634 (comment).

@cason
Copy link
Contributor
cason commented Nov 21, 2024

The reason for the default being false is backwards compatibility.

@cason cason added needs-information Waiting for additional information or feedback and removed needs-information Waiting for additional information or feedback needs-triage This issue/PR has not yet been triaged by the team. bug Something isn't working labels Nov 21, 2024
@faddat
Copy link
Contributor
faddat commented Nov 27, 2024

Hmmm so interesting!

By the way I have hit this also

Yes, while testing

I think that the best solution is to get rid of the flag and makes a default behavior not do anything special if it's the same IP address because really what we should be looking for anyway is the node ID and not the IP address so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs-information Waiting for additional information or feedback
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@cason @faddat @zivkovicmilos
0