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
836+ case 6666: // Alternate IRC [Apple addition]
837+ case 6667: // Standard IRC [Apple addition]
838+ case 6668: // Alternate IRC [Apple addition]
839+ case 6669: // Alternate IRC [Apple addition]
840+ case 6697: // IRC + TLS
841+ case 10080: // Amanda
38a7cf975c...a47716aa5a: warn if configured to listen on a “bad” port.
581+ BOOST_CHECK(IsBadPort(1));
582+ BOOST_CHECK(IsBadPort(22));
583+ BOOST_CHECK(IsBadPort(6000));
584+
585+ BOOST_CHECK(!IsBadPort(80));
586+ BOOST_CHECK(!IsBadPort(443));
I guess, on one side a lot of connections and connection attempts are to be expected by operators of web servers which are often open to the public and everybody is expected to be able to connect to them, so it is not likely that a web server operator complains like “why are you trying to connect to my web server?”.
On the other side, there are lots of restrictive firewalls that only allow outbound traffic to the internet to ports 80 and 443. So, I think it is good that 80 and 443 are not “bad”. Ideally Bitcoin Core should listen on 443 and talk SSL, so it is indistinguishable from normal HTTPS traffic.
1769@@ -1770,6 +1770,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1770 if (index == std::string::npos) {
1771 if (Lookup(bind_arg, bind_addr, GetListenPort(), false)) {
1772 connOptions.vBinds.push_back(bind_addr);
1773+ if (IsBadPort(bind_addr.GetPort())) {
1774+ InitWarning(strprintf(
1775+ _("-bind request to listen on port %u. This port is considered \"bad\" and "
1776+ "thus it is unlikely that any Bitcoin Core peers connect to it. See "
1777+ "https://fetch.spec.whatwg.org/#port-blocking"),
Adding entries to the list may be undesirable, as it may mean that someone running a node on such a port silently stops getting connections.
Not a very serious concern, as this would happen spread over some amount of time anyway, but still a reason I think to err on the side of caution and start with a possibly overly expansive list here
For me, the following comment answers the question:
“why am I seeing a failed SSH-login to my server on port 22 from your IP-address w.x.y.z?”
(from #23306 (comment))
There is some similarity between browser logic and Bitcoin node logic in this aspect where a malicious actor may trick a honest node to try to open a connection to a “victim” target:
area51.gov:22 if that address gets gossiped over the bitcoin network
There is some similarity between browser logic and Bitcoin node logic in this aspect where a malicious actor may trick a honest node to try to open a connection to a “victim” target: a honest web-browser user may be tricked to click specially crafted (malicious) links like e.g. http://area51.gov:22/ a honest node may be tricked to try to open connection to area51.gov:22 if that address gets gossiped over the bitcoin network
If this is the reason for adding a list of bad ports then we can add lot of other port numbers. Right now there are only 80 ports in the list. Example: 3389
Also this goes against the main reason we are allowing p2p connections to non-default ports: #23542 (review)
Or perhaps (eventually? sweat_smile ) a link to docs within bitcoin itself that lists our bad ports with an explanation of why they are bad.
Done.
779+ case 69: // tftp
780+ case 77: // priv-rjs
781+ case 79: // finger
782+ case 87: // ttylink
783+ case 95: // supdup
784+ case 101: // hostriame
245@@ -246,4 +246,17 @@ void InterruptSocks5(bool interrupt);
246 */
247 bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* auth, const Sock& socket);
248
249+/**
250+ * Determine if a port is "bad" from the perspective of attempting to connect
251+ * to a node on that port.
252+ * See
253+ * https://fetch.spec.whatwg.org/#port-blocking
All 3 sources agree on which ports are bad (which is good, pun intended). Can ignore port 0, that is something like opening connection to port 70000 or to IP address 1.2.3.500.
Changed the logic to apply only to clearnet (IPv4 and IPv6).
a47716aa5a...cb3887ee36: avoid connections to bad ports only over clearnet (https://github.com/bitcoin/bitcoin/pull/23542#discussion_r752635951), minor other changes.
cb3887ee36...634f2838ee: fix arguments to AddArg()
634f2838ee..0671bc5313: include the port when deciding a relay destination (append 3 commits)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
2089- // from advertising themselves as a service on another host and
2090- // port, causing a DoS attack as nodes around the network attempt
2091- // to connect to it fruitlessly.
2092- if (addr.GetPort() != Params().GetDefaultPort(addr.GetNetwork()) && nTries < 50) {
2093+ // Do not connect to bad ports, unless 50 invalid addresses have been selected already.
2094+ if (IsBadPort(addr.GetPort()) && (addr.IsIPv4() || addr.IsIPv6()) && nTries < 50) {
634f283 maybe perform less expensive checks first
0 if (nTries < 50 && (addr.IsIPv4() || addr.IsIPv6()) && IsBadPort(addr.GetPort())) {
nTries < 50 will be true, so the other conditions will be executed anyway.
m_net check first.
586+ BOOST_CHECK(!IsBadPort(443));
587+ BOOST_CHECK(!IsBadPort(8333));
588+
589+ // Check all ports, there must be 80 bad ports in total.
590+ size_t total_bad_ports{0};
591+ for (uint16_t port = std::numeric_limits<uint16_t>::max(); port > 0; --port) {
634f2838ee137234093a1fc89162f395366a387f might be a good place to use std::count_if, or alternatively, possibly faster given the number of iterations with
0 for (uint16_t port = std::numeric_limits<uint16_t>::max(); port != 0; --port) {
std::count_if works with iterators, how could I use it to go over the numbers 1..64k? Are you saying != is faster than >? Even if “yes”, I would leave it as is coz I find the > more natural/readable.
Right, and std::ranges::count_if is C++20 (and test code doesn’t need to be elegant).
!= 0 might be a faster iterator comparator (e.g. https://www.youtube.com/watch?v=oelQ4uAw2WQ) but in this case of a “manual” loop it may also be inlined by compilers in optimized non-debug modes. A non-blocking thought in any case.
!= (jump if not equal instead of jump if not greater/less than) was also what a ranged for-loop generated (GCC 10.2 -O3). Ofc feel free to ignore.
1706@@ -1707,8 +1707,10 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
1707 // Relay to a limited number of other nodes
1708 // Use deterministic randomness to send to the same nodes for 24 hours
1709 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1710- uint64_t hashAddr = addr.GetHash();
1711- const CSipHasher hasher = m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1712+ const uint64_t hash_addr = CServiceHash(0, 0)(addr);
// Use deterministic randomness .... I think that is sufficient but would be happy to reword the existent comment if you have a suggestion.
Code review ACK 0671bc5313b0a5fd125ef40c7ed4590bdca6b347 rebased to master, debug build clean, unit tests green, briefly ran a signet node.
Some non-blocking review thoughts.
251+ * to a node on that port.
252+ * See
253+ * https://fetch.spec.whatwg.org/#port-blocking
254+ * https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc
255+ * https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp
256+ * 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:
… comment above was marked as resolved even though …
Which comment? #23542 (review) 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 …
- 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.
0671bc5313...36bae788e1: address #23542 (review)
git range-diff 711ce2e 36bae78 8f40ea6 change since last review is rebase only
Concept ACK on moving away from the 8333-only policy.
While this is purely client policy, I think it’s a pretty impactful for the network. It may be worth at least sending a mail to the bitcoin-dev list for discussion.
The explanation of the bad ports in the current PR is pretty minimal and unclear, I think. I’d like to see those addressed (exactly why do we care about not connecting to some ports? I know there are links one can follow, but I feel it should be clearer in the documentation itself.
As far as which ports are included in the bad ports list, I’d prefer to include too many over too little at this point. Effectively, right now every port except 8333 is a bad port, and we’re reducing the list to just the explicitly included ones. Reducing further is always possible later, but adding ones may be hard if people start relying on being able to certain ports.
Also, being able to run Bitcoin nodes on well-known ports (particularly 443, https) would be the most useful way to hide traffic, but I think that’s a bit premature, and we should keep it in the bad ports list for now. With the work underway on BIP324 we may get encrypted P2P traffic, and with that, it should be (in a next step) be much easier to masquerade traffic as actual HTTPS running on port 443. Avoiding that port for now would make such a transition a lot easier.
It might be worthwhile to add an option to just use the vanilla port for outgoing connections in the case that the node software is triggering abuse complaints on your own network by reaching out to things it can’t handshake with. @wpeckr I believe that’s exactly why certain ports are blacklisted (the whole “bad ports” discussion). I think it may be useful to permit providing a custom list for that.
--vanilla-p2p-port-only is much easier than providing a custom blacklist, parsing or loading that from the configuration. With a blacklist to achieve this I need to include 2**16-1 entries to my bitcoin.conf.
This should definitely go to the mailing list for discussion though, it seems to me like is a very significant change with unclear advantages; I don’t think it meaningfully improves censorship resistance enough to warrant it. Bitcoin’s traffic is always very obvious and it’s unlikely that’s ever going to be obscured, given it is almost entirely an open network where the participants advertise their sockets.
--vanilla-p2p-port-onlyis much easier than providing a custom blacklist, parsing or loading that from the configuration. With a blacklist to achieve this I need to include 2**16-1 entries to my bitcoin.conf.
Is there a particular scenario you’re thinking of where you care about specifically only making connections to 8333? If there are particular services/ports you worry about angering, I think those should just be included in the bad ports list. Note that there are also non-Bitcoin Core clients on the network already which connect out to everything (including moderately aggressive crawlers), so all IP:port combinations rumoured on the network do get some traffic already.
I don’t think it meaningfully improves censorship resistance enough to warrant it. Bitcoin’s traffic is always very obvious and it’s unlikely that’s ever going to be obscured, given it is almost entirely an open network where the participants advertise their sockets.
I think that’s too black-and-white. Clearly, it is very hard to protect against targetted censorship attack with unbounded resources, but costs matter. Indiscriminately blocking port 8333 is extremely simple and effective right now. Needing to do packet inspection to detect the Bitcoin P2P traffic is more complex and costly, and will stop working if BIP324 gets adopted. Needing to update censoring rules based on what gets rumoured on the P2P network is even more complex, and can easily be triggered to block legitimate services too. Disguising traffic as HTTPS or another popular service is nontrivial but doable, I believe, and even harder to block reliably. Lastly, of course powerful attackers can also just try sybiling the network, or eclipsing specific nodes, but ideas like optional authentication can make that more detectable as well.
Overall, I don’t want to claim that this is an easy problem, or that silver bullet solutions exist. But I do think that very meaningful improvements are possible, and all of them depend on at the very least not gratuitously revealing “Bitcoin node here!” by using port 8333.
Yet, I agree this deserves wider discussion.
Is there a particular scenario you’re thinking of where you care about specifically only making connections to 8333?
No, just if there is an issue caused by it, having a simple toggle to disable non-8333 ports seems trivial to implement, and may come in useful.
tACK https://github.com/bitcoin/bitcoin/pull/23542/commits/8f40ea6e4569ceea3b55e9dee94664f6c3c4bd9f
ℹ️ Anyone can run the PowerShell script. I had tested it on Fedora
nit:
If for example the difficulty is 26643185256535.46 We could parse the value into 4 and 5 digit values (removing the decimal) to get a list of values: 2664 6643 6431 4318 3185 1852 8525 5256 5653 6535 5354 3546 26643 etc…
Basing these values on difficulty - the proposed good list would be in lock step over the network. The node could open and listen on each of these possible ports (excluding any values hard coded on a bad list) every new day UTC Time. If a threshold of other listeners (10-100?) are also listening on that port - that port value is marked as good. If it is not - it is purged from the potential candidates and the next port is tried. If no good candidates are found - the entire list is incremented by 1 and tried again.
While network quality and availability may vary in different regions - we can use the difficulty. This gives flexibility to the “good list” based on local conditions. IMO a process like this is robust and dynamic. It also removes developers from making this decision.
@RandyMcMillan, this PR and all the discussion is about whether to connect to a peer who listens on port X. Not whether to listen on port X. On which port to listen ourselves is a separate topic.
Currently on which port to listen is statically configured by the user using the -bind= and -port arguments and is more or less constant - you do not want to change your listening port often or automatically because that may require adjusting your firewall to allow incoming connections to it and also, more importantly, your addr+port need some time to propagate to the network so that other peers are aware of it.
But please, do not dilute this PR with discussion about on which port to listen.
Concept ACK
It seems impossible to know the reason a node is using a particular port - not sure why this is a debate. In fact - this should be a high priority change. (see off topic)
To my previous comment - it would be useful to derive a list of alternate qualified ports for a user to select from, if circumstances dictated they could not use 8333. In a worst case scenario (or IBD) - all the user would need to know is the current difficulty - to have a predictable list of ports to use with a higher degree of certainty that multiple honest nodes would be using them (due to local network/political/whatever conditions). In a desperate situation the user would not want to eclipse themself unknowingly.
A rolling port list may augment obfuscation and censorship resistance.
No further off topic comments - my apologize.
446@@ -447,7 +447,7 @@ void SetupServerArgs(ArgsManager& argsman)
447 argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
448 argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
449 argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
450- argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
451+ argsman.AddArg("-port=<port>", "Listen for connections on <port>. Not relevant for I2P (see doc/i2p.md).", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
8f40ea6e45...794319bc77: add a formal definition of a “bad” port in doc/p2p-bad-ports.md together with an explanation why it is considered bad.
Invalidates ACKs from @jonatack, @prayank23
97+6666
98+6667
99+6668
100+6669
101+6697
102+10080
Added 4 spaces before each line to get code block and info about each port:
0 1: tcpmux
1 7: echo
2 9: discard
3 11: systat
4 13: daytime
5 15: netstat
6 17: qotd
7 19: chargen
8 20: ftp data
9 21: ftp access
10 22: ssh
11 23: telnet
12 25: smtp
13 37: time
14 42: name
15 43: nicname
16 53: domain
17 69: tftp
18 77: priv-rjs
19 79: finger
20 87: ttylink
21 95: supdup
22 101: hostname
23 102: iso-tsap
24 103: gppitnp
25 104: acr-nema
26 109: pop2
27 110: pop3
28 111: sunrpc
29 113: auth
30 115: sftp
31 117: uucp-path
32 119: nntp
33 123: NTP
34 135: loc-srv /epmap
35 137: netbios
36 139: netbios
37 143: imap2
38 161: snmp
39 179: BGP
40 389: ldap
41 427: SLP (Also used by Apple Filing Protocol)
42 465: smtp+ssl
43 512: print / exec
44 513: login
45 514: shell
46 515: printer
47 526: tempo
48 530: courier
49 531: chat
50 532: netnews
51 540: uucp
52 548: AFP (Apple Filing Protocol)
53 554: rtsp
54 556: remotefs
55 563: nntp+ssl
56 587: smtp (rfc6409)
57 601: syslog-conn (rfc3195)
58 636: ldap+ssl
59 989: ftps-data
60 990: ftps
61 993: ldap+ssl
62 995: pop3+ssl
63 1719: h323gatestat
64 1720: h323hostcall
65 1723: pptp
66 2049: nfs
67 3659: apple-sasl / PasswordServer
68 4045: lockd
69 5060: sip
70 5061: sips
71 6000: X11
72 6566: sane-port
73 6665: Alternate IRC [Apple addition]
74 6666: Alternate IRC [Apple addition]
75 6667: Standard IRC [Apple addition]
76 6668: Alternate IRC [Apple addition]
77 6669: Alternate IRC [Apple addition]
78 6697: IRC + TLS
79 10080: Amanda
Added 4 spaces before each line to get code block and info about each port:
Nice. @prayank23, consider using HTML details tags to hide long diffs in review comments, see https://github.com/bitcoin/bitcoin/pull/23542/files#r800765686 for an example.
104+For further information see:
105+[pull/23306](https://github.com/bitcoin/bitcoin/pull/23306#issuecomment-947516736)
106+[pull/23542](https://github.com/bitcoin/bitcoin/pull/23542)
107+[fetch.spec.whatwg.org](https://fetch.spec.whatwg.org/#port-blocking)
108+[chromium.googlesource.com](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc)
109+[hg.mozilla.org](https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp)
Added 2 spaces after each link for line break:
0For further information see:
1
2[pull/23306](/bitcoin-bitcoin/23306/#issuecomment-947516736)
3[pull/23542](https://github.com/bitcoin/bitcoin/pull/23542)
4[fetch.spec.whatwg.org](https://fetch.spec.whatwg.org/#port-blocking)
5[chromium.googlesource.com](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc)
6[hg.mozilla.org](https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp)
test/lint/lint-whitespace.sh. Maybe use bullet points or a blank line between items?
test/lint/lint-whitespace.sh seems happy.
830+ case 6566: // sane-port
831+ case 6665: // Alternate IRC [Apple addition]
832+ case 6666: // Alternate IRC [Apple addition]
833+ case 6667: // Standard IRC [Apple addition]
834+ case 6668: // Alternate IRC [Apple addition]
835+ case 6669: // Alternate IRC [Apple addition]
/etc/services from macOS? I think [Apple addition] in context of Bitcoin Core is redundant. For example, 6667 on Linux machines is just “Internet Relay Chat”.
[Apple addition].
1700@@ -1701,6 +1701,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1701 if (index == std::string::npos) {
1702 if (Lookup(bind_arg, bind_addr, GetListenPort(), false)) {
1703 connOptions.vBinds.push_back(bind_addr);
1704+ if (IsBadPort(bind_addr.GetPort())) {
1705+ InitWarning(strprintf(
1706+ _("-bind request to listen on port %u. This port is considered \"bad\" and "
IsBadPort() would not be a suitable name anymore, it should be renamed to something like GetBadPortName() which would imply that the passed value is bad. Maybe IsBadPortAndIfYesThenReturnItsName() :-( I would Keep It Simple, Stupid. The names are now in doc/p2p-bad-ports.md which is mentioned in this message.
748@@ -749,3 +749,93 @@ void InterruptSocks5(bool interrupt)
749 {
750 interruptSocks5Recv = interrupt;
751 }
752+
753+bool IsBadPort(uint16_t port)
std::optional<string> IsBadPort(uint16_t port)
753+bool IsBadPort(uint16_t port)
754+{
755+ /* Don't forget to update doc/p2p-bad-ports.md if you change this list. */
756+
757+ switch (port) {
758+ case 1: // tcpmux
case 1: return "tcpmux";
In 10a08ce771dc08750a0e8dfe1d17f864284fb19b: the warning only shows when using -bind, not when using -port.
Also, when the user has -printtoconsole=1 they’ll probably miss it, but we can deal with that later perhaps.
You could add a TODO near the help text for -port that “Nodes not using the default ports are unlikely to get incoming connections” will no longer be true once this has been live for a while.
16+authentication are unlikely to be considered a malicious action,
17+e.g. port 80 (http).
18+
19+Following is a list of "bad" ports which Bitcoin Core avoids when choosing a
20+peer to connect to. If a node is listening on such a port, it will likely get
21+less incoming connections.
This text could maybe be pithier and has a few grammar issues.
0When Bitcoin Core automatically opens outgoing P2P connections, it chooses a
1peer from the potential peers in `peers.dat` gossiped over the P2P network by
2other peers.
3
4A malicious actor can gossip an address:port where no Bitcoin node is listening,
5or one where a service is listening that is not related to the Bitcoin
6network. These may receive connection attempts from Bitcoin nodes.
7
8"Bad" ports are ones used by services which are usually not open to the public
9and require authentication. A connection attempt to such a service by a Bitcoin
10Core node may be considered a malicious action by a paranoid administrator. An
11example for such a port is 22 (ssh). On the other hand, connection attempts to
12public services that usually do not require authentication are unlikely to be
13considered malicious, e.g. port 80 (http).
14
15Below is a list of "bad" ports which Bitcoin Core avoids when choosing a
16peer to connect to. If a node is listening on such a port, it will likely
17receive fewer incoming connections.
748@@ -749,3 +749,93 @@ void InterruptSocks5(bool interrupt)
749 {
750 interruptSocks5Recv = interrupt;
751 }
752+
753+bool IsBadPort(uint16_t port)
754+{
755+ /* Don't forget to update doc/p2p-bad-ports.md if you change this list. */
Not sure if this should be a doxygen comment. You did update the doxygen in the declaration :+1:
0 // Don't forget to update doc/p2p-bad-ports.md if you change this list.
It is good for the user to be able to see the list of the ports and I think it would be too rough to refer the user to go to look into some .cpp file on line 753.
The other way around would be to generate the .cpp file from the user-facing p2p-bad-ports.md but I think that would be an overkill, given that this list is not supposed to be updated often, if ever.
I had the same consideration as glozow initially but agree that we shouldn’t point users to .cpp files, especially since the line number is variable over time.
I also agree generating .cpp from .md is overkill - and it wouldn’t even guarantee that future contributions don’t just update one without the other I think? I think adding a test that parses the .md file and compares all ports could be an alternative, but personally I think that’s overkill too. There’s a clear reference to update the doc in IsBadPort, I think after this initial review any changes should be quite trivial to review.
246@@ -247,4 +247,13 @@ void InterruptSocks5(bool interrupt);
247 */
248 bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* auth, const Sock& socket);
249
250+/**
251+ * Determine if a port is "bad" from the perspective of attempting to connect
252+ * to a node on that port.
253+ * @see doc/p2p-bad-ports.md
254+ * @param[in] port Port to check.
255+ * @return true if the port is bad
nit, returns both true and false
0 * [@returns](/bitcoin-bitcoin/contributor/returns/) whether the port is bad
Diff-review re-ACK 794319bc7787fb47f3de58f9526d33f9a7e90fea per git range-diff f7a3647 8f40ea6 794319b
A few minor comments follow.
Did you mean to drop this change in the latest push, and should doc/p2p-bad-ports.md be mentioned in the -port help?
0- argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
1+ argsman.AddArg("-port=<port>", "Listen for connections on <port>. Not relevant for I2P (see doc/i2p.md).", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
794319bc77...a13278afd0: rebase and address some suggestions
the warning only shows when using
-bind, not when using-port
Good catch, fixed!
You could add a
TODOnear the help text for-port…
Done.
Did you mean to drop this change in the latest push…
Yes, because that sentence will still be true after this PR is merged. It has to be removed later, not now. See the added TODO comment, suggested above.
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:
https://github.com/bitcoin/bitcoin/pull/23306
This new constructor will be useful if we just want to hash a `CService`
object without the two `GetRand()` calls (in `RelayAddress()` in a
subsequent commit).
In `PeerManagerImpl::RelayAddress()` we used just the hash of the
address that is being relayed to decide where to relay it to. Include
the port in that hash, so that e.g. `1.1.1.1:5555` and `1.1.1.1:6666`
get relayed to different peers. Those are two different, distinct
services after all.
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)
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.
seed.bitcoin.sipa.be) is listening on a non-default port, then it will be dysfunctional even without this PR.
1774@@ -1775,8 +1775,10 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
1775 // Relay to a limited number of other nodes
1776 // Use deterministic randomness to send to the same nodes for 24 hours
1777 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1778- const uint64_t hashAddr{addr.GetHash()};
1779- const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr).Write((GetTime() + hashAddr) / (24 * 60 * 60))};
1780+ const uint64_t hash_addr{CServiceHash(0, 0)(addr)};
1781+ const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY)
1782+ .Write(hash_addr)
1783+ .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()?
0DEPRECATED
1Use 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.
1774@@ -1775,8 +1775,10 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
1775 // Relay to a limited number of other nodes
1776 // Use deterministic randomness to send to the same nodes for 24 hours
1777 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1778- const uint64_t hashAddr{addr.GetHash()};
1779- const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr).Write((GetTime() + hashAddr) / (24 * 60 * 60))};
1780+ const uint64_t hash_addr{CServiceHash(0, 0)(addr)};
I think the SipHash vs SHA256 is a detail that need not be put in the commit message. It is really not relevant here. I changed it because we already have the CServiceHash which uses SipHash.
I played with bench_bitcoin and the SHA256_32b and SipHash_32b tests (the SipHash needs to be changed to bench.batch(32).unit("byte").run(... to print the same units - ns/byte). It looks like SipHash is about 7 times faster than the double-SHA256. But as you say, this code is not performance hot spot.
1742@@ -1730,6 +1743,15 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1743 // on any address - 0.0.0.0 (IPv4) and :: (IPv6).
1744 connOptions.bind_on_any = args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty();
1745
1746+ // Emit a warning if a bad port is given to -port= but only if -bind and -whitebind are not
1747+ // given, because if they are, then -port= is ignored.
1748+ if (connOptions.bind_on_any && args.IsArgSet("-port")) {
1749+ const uint16_t port_arg = args.GetIntArg("-port", 0);
1750+ if (IsBadPort(port_arg)) {
1751+ InitWarning(BadPortWarning("-port", port_arg));
I’m getting duplicated warnings in my console, is this on purpose?
0...
12022-02-14T00:27:34Z Warning: -port request to listen on port 587. This port is considered "bad" and thus it is unlikely that any Bitcoin Core peers connect to it. See doc/p2p-bad-ports.md for details and a full list.
2Warning: -port request to listen on port 587. This port is considered "bad" and thus it is unlikely that any Bitcoin Core peers connect to it. See doc/p2p-bad-ports.md for details and a full list.
32022-02-14T00:27:34Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
4...
0@@ -0,0 +1,114 @@
1+When Bitcoin Core automatically opens outgoing P2P connections it chooses
0When Bitcoin Core automatically opens outgoing P2P connections, it chooses
0@@ -0,0 +1,114 @@
1+When Bitcoin Core automatically opens outgoing P2P connections it chooses
2+a peer (address and port) from its list of potential peers. This list is
3+populated with unchecked data, gossiped over the P2P network by other peers.
0populated with unchecked data gossiped over the P2P network by other peers.
(or s/data,/data that is/ or s/data,/data, which is/)
15+other hand, connection attempts to public services that usually do not require
16+authentication are unlikely to be considered a malicious action,
17+e.g. port 80 (http).
18+
19+Below is a list of "bad" ports which Bitcoin Core avoids when choosing a peer to
20+connect to. If a node is listening on such a port, it will likely receive less
“fewer” for countable quantities
0connect to. If a node is listening on such a port, it will likely receive fewer
ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
Some minor grammar issues from my previous review subsist wrt the doc.
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.
Labels
P2P
Milestone
23.0