net: open p2p connections to nodes that listen on non-default ports #23542

vasild · 2021-11-18T10:14:52Z

By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.

Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).

For further justification see the OP of:
#23306

ghost · 2021-11-18T10:31:24Z

Concept ACK

src/netbase.cpp

0xB10C · 2021-11-18T10:57:12Z

Concept ACK

vasild · 2021-11-18T11:01:33Z

38a7cf975c...a47716aa5a: warn if configured to listen on a "bad" port.

theStack · 2021-11-18T11:53:03Z

Concept ACK

naumenkogs · 2021-11-18T12:00:19Z

Concept ACK

src/test/netbase_tests.cpp

src/init.cpp

mzumsande

Concept ACK

src/netbase.cpp

src/netbase.h

vasild · 2021-11-19T10:28:33Z

a47716aa5a...cb3887ee36: avoid connections to bad ports only over clearnet (#23542 (comment)), minor other changes.

vasild · 2021-11-19T10:44:53Z

cb3887ee36...634f2838ee: fix arguments to AddArg()

rebroad · 2021-11-21T18:20:28Z

tACK

vasild · 2021-11-29T11:13:01Z

634f2838ee..0671bc5313: include the port when deciding a relay destination (append 3 commits)

DrahtBot · 2021-11-29T19:26:44Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24213 (refactor: use Span in random.*; make GetRand a template, remove GetRandInt by PastaPastaPasta)
#20196 (net: fix GetListenPort() to derive the proper port by vasild)
#18642 (Use std::chrono for the time to rotate destination of addr messages + tests by naumenkogs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

kristapsk · 2021-11-29T19:48:36Z

Concept ACK

jnewbery · 2021-12-02T14:10:12Z

Concept ACK

jonatack

Code review ACK 0671bc5 rebased to master, debug build clean, unit tests green, briefly ran a signet node.

Some non-blocking review thoughts.

src/net.cpp

src/test/netbase_tests.cpp

src/net_processing.cpp

ghost · 2021-12-09T15:21:36Z

src/netbase.h

+ * Determine if a port is "bad" from the perspective of attempting to connect
+ * to a node on that port.
+ * See
+ * https://fetch.spec.whatwg.org/#port-blocking
+ * https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc
+ * https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp
+ * https://github.com/bitcoin/bitcoin/pull/23306#issuecomment-947516736


Still not sure about few things and comment above was marked as resolved even though few other reviewers have similar confusion about ports:

What is a BAD port for Bitcoin Core?

Why browsers (bad ports) are referenced here because Bitcoin Core is not a browser and I was unable to find anything relevant for other P2P networks. Example: i2p

Make AddrMan support multiple ports per IP #23306 (comment) this doesn't look like a big issue or maybe there is some misunderstanding

What was the reason to allow p2p connections to nodes that are not using default ports? Why would someone use non-default port? Two reasons that I could think of: Privacy and Security. If someone wants to use a port that looks like other service but is Bitcoin Core, isn't that something we should encourage/allow?

... comment above was marked as resolved even though ...

Which comment? #23542 (comment) is still open.

What is a BAD port for Bitcoin Core?

Since this PR, if it gets merged, a bad port (for making automatic outgoing connections to) is one for which IsBadPort() returns true.

Why browsers (bad ports) are referenced here because Bitcoin Core is not a browser ...

#23542 (comment)

What was the reason to allow p2p connections to nodes that are not using default ports?

From this PR OP: "because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time)"

Why would someone use non-default port?

See above, and also the reasons you state below:

Two reasons that I could think of: Privacy and Security. If someone wants to use a port that looks like other service but is Bitcoin Core, isn't that something we should encourage/allow?

Yes. This is what the current PR aims to achieve.

Replied here: #23542 (comment)

vasild · 2021-12-10T13:25:26Z

0671bc5313...36bae788e1: address #23542 (comment)

jonatack · 2021-12-10T14:00:04Z

ACK 36bae78

vasild · 2022-02-11T14:23:25Z

a13278afd0...36ee76d1af: pet bogus lint warning src/init.cpp: Expected 0 argument(s) after format string but found 1 argument(s): strprintf(bad_port_warning, port_arg)

ghost

ACK 36ee76d

mzumsande · 2022-02-11T16:24:11Z

One possible issue is that DNS seeds return IPs without port information (try e.g. dig seed.bitcoin.sipa.be), and bitcoind just adds the standard port when creating an address entry from the seeder data.

That means that in the unlikely event that suddenly everyone switched to a non-default port after this is merged, the DNS seeds would become dysfunctional. I think that the seeder code wouldn't mark peers with non-default ports as good and return them though, so at least it wouldn't lead to conflicting entries with the standard port being advertised in addr relay.

But it seems to me that we shouldn't be too offensive in advertising the use of non-standard ports before a solution for the DNS seeds exists.

vasild · 2022-02-11T16:40:10Z

@mzumsande, if a DNS seed (e.g. seed.bitcoin.sipa.be) is listening on a non-default port, then it will be dysfunctional even without this PR.

mzumsande · 2022-02-11T16:51:33Z

@vasild: This is is not about the port the DNS seed itself listens on. The DNS seeder software has a crawler that connects to listening nodes of the network to check whether they are reachable, and returns a list of IPs (but not ports!) of reachable nodes to help new nodes find peers initially. It can't include results from nodes listening on non-default ports in these responses.

vasild · 2022-02-11T16:57:57Z

uff, of course the seeder itself does not run a bitcoin node 🤦, sorry for the confusion (or it could, but that is not relevant)

stickies-v

tACK 36ee76d
Left some very minor comments but nothing blocking - feel free to ignore.

stickies-v · 2022-02-13T22:55:40Z

src/net_processing.cpp

+    const uint64_t hash_addr{CServiceHash(0, 0)(addr)};
+    const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY)
+                                .Write(hash_addr)
+                                .Write((GetTime() + hash_addr) / (24 * 60 * 60))};


Since you're touching this line, maybe now is a good time to phase out the deprecated GetTime()?

DEPRECATED Use either GetTimeSeconds (not mockable) or GetTime<T> (mockable)

I really like doing such changes in a separate commit. It looks like that a GetTime() -> GetTime<T>() replacement "cannot possibly break anything". My experience is that once in 100 times, something gets affected by such "innocent" changes. And the last thing I want when investigating with e.g. git bisect is to stumble on one commit that combines a logical change + mechanical "innocent" replacement that requires extra effort to decouple (and revert).

I will append a commit to use GetTime<T>() if retouching.

I agree. Best as a separate pull (or commit, but it's unrelated to the change here).

src/net_processing.cpp

src/init.cpp

jonatack

ACK 36ee76d

Some minor grammar issues from my previous review subsist wrt the doc.

jonatack · 2022-02-14T10:14:00Z

doc/p2p-bad-ports.md

@@ -0,0 +1,114 @@
+When Bitcoin Core automatically opens outgoing P2P connections it chooses


Suggested change

When Bitcoin Core automatically opens outgoing P2P connections it chooses

When Bitcoin Core automatically opens outgoing P2P connections, it chooses

jonatack · 2022-02-14T10:15:28Z

doc/p2p-bad-ports.md

@@ -0,0 +1,114 @@
+When Bitcoin Core automatically opens outgoing P2P connections it chooses
+a peer (address and port) from its list of potential peers. This list is
+populated with unchecked data, gossiped over the P2P network by other peers.


Suggested change

populated with unchecked data, gossiped over the P2P network by other peers.

populated with unchecked data gossiped over the P2P network by other peers.

(or s/data,/data that is/ or s/data,/data, which is/)

jonatack · 2022-02-14T10:15:50Z

doc/p2p-bad-ports.md

+e.g. port 80 (http).
+
+Below is a list of "bad" ports which Bitcoin Core avoids when choosing a peer to
+connect to. If a node is listening on such a port, it will likely receive less


"fewer" for countable quantities

Suggested change

connect to. If a node is listening on such a port, it will likely receive less

connect to. If a node is listening on such a port, it will likely receive fewer

"fewer" for countable quantities

Tucked these fixups into #24468.

glozow

utACK 36ee76d
light code review, I'm not very familiar with this area of code

src/netbase.cpp

Sjors · 2022-02-17T10:27:47Z

This is is not about the port the DNS seed itself listens on. The DNS seeder software has a crawler that connects to listening nodes of the network to check whether they are reachable, and returns a list of IPs (but not ports!) of reachable nodes to help new nodes find peers initially. It can't include results from nodes listening on non-default ports in these responses.

For now it seems fine if DNS seeds only return nodes that listen on port 8333. As soon as your node finds a single peer, it will get nodes at other ports. We could later have port.someseed.com return nodes for port, or something like that.

This would be relevant for nodes that, during their bootstrapping phase, are not able to connect to anything on port 8333. In that case the first place to start might 80.someseed.com.

If blocking 8333 becomes a very common problem (as opposed to just annoying firewalls that only allow port 80 and 443), I would not be surprised if DNS seed domains are blocked by most ISP's too in that dystopia.

laanwj · 2022-03-02T08:32:14Z

Concept and light code review ACK 36ee76d

mzumsande · 2022-03-02T14:15:57Z

Should there be a release note for this?

…n non-d…

…ated tor/i2p documentation a1db99a init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack) Pull request description: including review feedback from bitcoin/bitcoin#22834 (comment) and bitcoin/bitcoin#24205 (comment) concerning `src/init.cpp`, `doc/tor.md` and `doc/i2p.md` - s/outgoing/automatic outbound/ - s/Incoming/Inbound and manual/ (are not affected by this option.) - s/only through network/only to network/ - s/this option. This option/this option. It/ - s/network types/networks/ and pick up a few nits in `doc/p2p-bad-ports.md` from bitcoin/bitcoin#23542 (review). ACKs for top commit: laanwj: ACK a1db99a w0xlt: ACK a1db99a theStack: ACK a1db99a Tree-SHA512: dd727904b9b3dadb16053e2b0350e6c0814ef68fb0cca7d34880b883123cfe3aa03b15813b40a863f6367d596d17ee4517eab55281cfe35cd00767b8a39593ca

vasild · 2022-03-07T15:36:17Z

Added release notes to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/23.0-Release-Notes-draft#p2p-and-network-changes, feel free to edit.

…/i2p documentation a1db99a init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack) Pull request description: including review feedback from bitcoin#22834 (comment) and bitcoin#24205 (comment) concerning `src/init.cpp`, `doc/tor.md` and `doc/i2p.md` - s/outgoing/automatic outbound/ - s/Incoming/Inbound and manual/ (are not affected by this option.) - s/only through network/only to network/ - s/this option. This option/this option. It/ - s/network types/networks/ and pick up a few nits in `doc/p2p-bad-ports.md` from bitcoin#23542 (review). ACKs for top commit: laanwj: ACK a1db99a w0xlt: ACK a1db99a theStack: ACK a1db99a Tree-SHA512: dd727904b9b3dadb16053e2b0350e6c0814ef68fb0cca7d34880b883123cfe3aa03b15813b40a863f6367d596d17ee4517eab55281cfe35cd00767b8a39593ca

fanquake · 2022-09-15T15:18:42Z

This was part of 23.0 and got a release note. Removing "Needs release note".

Summary: ``` By default, for mainnet, the p2p listening port is 8333. Bitcoin Core has a strong preference for only connecting to nodes that listen on that port. Remove that preference because connections over clearnet that involve port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p traffic before the connection is even established (at TCP SYN time). ``` Backport of [[bitcoin/bitcoin#23542 | core#23542]]. Depends on D12342 and D12343. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D12344

vasild mentioned this pull request Nov 18, 2021

Make AddrMan support multiple ports per IP #23306

Merged

ghost reviewed Nov 18, 2021

View reviewed changes

src/netbase.cpp Show resolved Hide resolved

DrahtBot added the P2P label Nov 18, 2021

vasild force-pushed the connect_to_any_port branch from 38a7cf9 to a47716a Compare November 18, 2021 11:00

benthecarman reviewed Nov 18, 2021

View reviewed changes

src/test/netbase_tests.cpp Show resolved Hide resolved

jb55 reviewed Nov 18, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

mzumsande reviewed Nov 18, 2021

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.h Outdated Show resolved Hide resolved

vasild force-pushed the connect_to_any_port branch from a47716a to cb3887e Compare November 19, 2021 10:27

vasild force-pushed the connect_to_any_port branch from cb3887e to 634f283 Compare November 19, 2021 10:44

DrahtBot mentioned this pull request Nov 30, 2021

Use std::chrono for the time to rotate destination of addr messages + tests #18642

Merged

jonatack reviewed Dec 9, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/test/netbase_tests.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

ghost reviewed Dec 9, 2021

View reviewed changes

vasild force-pushed the connect_to_any_port branch from 0671bc5 to 36bae78 Compare December 10, 2021 13:25

mzumsande mentioned this pull request Dec 13, 2021

test: Refactor addr_relay.py addr generation, increase mocktime #23720

Merged

DrahtBot mentioned this pull request Jan 5, 2022

Remove pointless and confusing shift in RelayAddress #23970

Merged

DrahtBot added the Needs rebase label Jan 6, 2022

net: remove unused CNetAddr::GetHash()

36ee76d

vasild force-pushed the connect_to_any_port branch from a13278a to 36ee76d Compare February 11, 2022 14:22

ghost approved these changes Feb 11, 2022

View reviewed changes

stickies-v approved these changes Feb 14, 2022

View reviewed changes

jonatack reviewed Feb 14, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 14, 2022

net: fix GetListenPort() to derive the proper port #20196

Merged

glozow reviewed Feb 15, 2022

View reviewed changes

src/netbase.cpp Show resolved Hide resolved

laanwj merged commit ba11eb3 into bitcoin:master Mar 2, 2022

vasild deleted the connect_to_any_port branch March 2, 2022 08:39

fanquake added the Needs release note label Mar 2, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 2, 2022

Merge bitcoin#23542: net: open p2p connections to nodes that listen o…

76d2b65

…n non-d…

jonatack mentioned this pull request Mar 3, 2022

init, doc: improve -onlynet help and related tor/i2p documentation #24468

Merged

jonatack mentioned this pull request Mar 9, 2022

23.x backports #24512

Merged

fanquake removed the Needs release note label Sep 15, 2022

ghost mentioned this pull request Sep 29, 2022

contrib: add vasild to trusted keys #25871

Closed

delta1 added a commit to delta1/elements that referenced this pull request Jul 3, 2023

Merge ba11eb3 into merged_master (Bitcoin PR bitcoin/bitcoin#23542)

1dc6ad6

bitcoin locked and limited conversation to collaborators Sep 15, 2023

		@@ -0,0 +1,114 @@
		When Bitcoin Core automatically opens outgoing P2P connections it chooses

	populated with unchecked data, gossiped over the P2P network by other peers.
	populated with unchecked data gossiped over the P2P network by other peers.

	connect to. If a node is listening on such a port, it will likely receive less
	connect to. If a node is listening on such a port, it will likely receive fewer

net: open p2p connections to nodes that listen on non-default ports #23542

net: open p2p connections to nodes that listen on non-default ports #23542

Conversation

vasild commented Nov 18, 2021 • edited by laanwj Loading

ghost commented Nov 18, 2021

0xB10C commented Nov 18, 2021

vasild commented Nov 18, 2021

theStack commented Nov 18, 2021

naumenkogs commented Nov 18, 2021

mzumsande left a comment

Choose a reason for hiding this comment

vasild commented Nov 19, 2021

vasild commented Nov 19, 2021

rebroad commented Nov 21, 2021 • edited Loading

vasild commented Nov 29, 2021

DrahtBot commented Nov 29, 2021 • edited Loading

Conflicts

kristapsk commented Nov 29, 2021

jnewbery commented Dec 2, 2021

jonatack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild commented Dec 10, 2021

jonatack commented Dec 10, 2021

vasild commented Feb 11, 2022

ghost left a comment

Choose a reason for hiding this comment

mzumsande commented Feb 11, 2022 • edited Loading

vasild commented Feb 11, 2022

mzumsande commented Feb 11, 2022

vasild commented Feb 11, 2022 • edited Loading

stickies-v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

jonatack left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

Sjors commented Feb 17, 2022 • edited Loading

laanwj commented Mar 2, 2022

mzumsande commented Mar 2, 2022

vasild commented Mar 7, 2022

fanquake commented Sep 15, 2022

vasild commented Nov 18, 2021 •

edited by laanwj

Loading

rebroad commented Nov 21, 2021 •

edited

Loading

DrahtBot commented Nov 29, 2021 •

edited

Loading

mzumsande commented Feb 11, 2022 •

edited

Loading

vasild commented Feb 11, 2022 •

edited

Loading

jonatack Feb 15, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Feb 14, 2022 •

edited

Loading

Sjors commented Feb 17, 2022 •

edited

Loading