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
Concept ACK
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
Not sure about this list of bad ports because more ports can be added to this list if we need it in Bitcoin Core. I had followed the comments in other PR: #23306 (comment)
@prayank23: You just mean additional ports can always be added at a later date? I agree the comment seems reasonable
If there is a clear definition of what Bitcoin Core is considering a bad port maybe they can be added right now
There is/was not a definition before, but I guess that will be settled up with this PR.
Concept ACK
38a7cf975c...a47716aa5a: warn if configured to listen on a "bad" port.
Concept ACK
Concept ACK
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));
any reason these 2 shouldn't be bad ports?
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"),
I'm a bit confused as to why bitcoin cares about what a brower's fetch api considers a "bad port", it's not clear what this even means? Maybe I'm missing something.
looks like I was missing this context: #23306 (comment)
Just to get a starting list of bad ports I think. Can adjust from there if certain ports should be added or removed from that.
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
I'll still point out that the link provided doesn't explain why a particular port is "bad". Following the link to get an explaination leads to: "A port is a bad port if it is listed in the first column of the following table". ok...??? 😕 I found this pretty confusing from a user perspective trying to understand this warning.
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
I understand that from reading the comment on github, but I was just pointing out from a user perspective there is no insight from the link in the warning in this commit. Perhaps it could be reworded: "See https://fetch.spec.whatwg.org/#port-blocking for a list of bad ports". 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.
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
nit: hostname (looks like a OCR fail that made it to the browsers' lists)
Good catch! Fixed.
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
I think i2p uses port 0, so we'd need to make an exception for that
TCP port 0 does not exist. It also doesn't appear in the Mozilla list; it's just a terminator to indicate "end of list" there. @benthecarman Right; this list should only apply to IPv4/IPv6.
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).
Concept ACK
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()
tACK
634f2838ee..0671bc5313: include the port when deciding a relay destination (append 3 commits)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
Concept ACK
Concept ACK
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
if (nTries < 50 && (addr.IsIPv4() || addr.IsIPv6()) && IsBadPort(addr.GetPort())) {
Done. Although I think that in practice this is irrelevant (a few CPU cycles saved in a code that is not executed in a hot loop). Also, in most of the cases nTries < 50 will be true, so the other conditions will be executed anyway.
True; my initial thought was to suggest only the IP 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
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.
Huh, just rewatched the video and it seems != (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);
b3a1a3ba A comment documenting why (0, 0) is passed to CServiceHash may be nice (or perhaps a dedicated/named ctor).
The comment above says // 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.
Replied here: #23542 (review)
0671bc5313...36bae788e1: address #23542 (review)
ACK 36bae788e1c34261b108d57fc2ffff0954a33848
ACK 8f40ea6e4569ceea3b55e9dee94664f6c3c4bd9f per git range-diff 711ce2e 36bae78 8f40ea6 change since last review is rebase only
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. Without an option to only consider 8333 as valid, you would need to use external tools to achieve the same, or shut down the software.
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:
I had shared this pull request and related things in an email sent to bitcoin dev mailing list: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019837.html
<details> <summary>off topic</summary> <p>
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.
</p> </details>
@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.
<details> <summary>offtopic</summary>
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.
</details>
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)
<details> <summary>off topic</summary> <p>
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.
</p> </details>
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);
I will revert this change because the original text will still be true even after this PR is released. It will be true for some time and will start becoming less true once this PR becomes widely used. The text should be removed then.
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:
1: tcpmux
7: echo
9: discard
11: systat
13: daytime
15: netstat
17: qotd
19: chargen
20: ftp data
21: ftp access
22: ssh
23: telnet
25: smtp
37: time
42: name
43: nicname
53: domain
69: tftp
77: priv-rjs
79: finger
87: ttylink
95: supdup
101: hostname
102: iso-tsap
103: gppitnp
104: acr-nema
109: pop2
110: pop3
111: sunrpc
113: auth
115: sftp
117: uucp-path
119: nntp
123: NTP
135: loc-srv /epmap
137: netbios
139: netbios
143: imap2
161: snmp
179: BGP
389: ldap
427: SLP (Also used by Apple Filing Protocol)
465: smtp+ssl
512: print / exec
513: login
514: shell
515: printer
526: tempo
530: courier
531: chat
532: netnews
540: uucp
548: AFP (Apple Filing Protocol)
554: rtsp
556: remotefs
563: nntp+ssl
587: smtp (rfc6409)
601: syslog-conn (rfc3195)
636: ldap+ssl
989: ftps-data
990: ftps
993: ldap+ssl
995: pop3+ssl
1719: h323gatestat
1720: h323hostcall
1723: pptp
2049: nfs
3659: apple-sasl / PasswordServer
4045: lockd
5060: sip
5061: sips
6000: X11
6566: sane-port
6665: Alternate IRC [Apple addition]
6666: Alternate IRC [Apple addition]
6667: Standard IRC [Apple addition]
6668: Alternate IRC [Apple addition]
6669: Alternate IRC [Apple addition]
6697: IRC + TLS
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.
Done.
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:
For further information see:
[pull/23306](/bitcoin-bitcoin/23306/#issuecomment-947516736)
[pull/23542](https://github.com/bitcoin/bitcoin/pull/23542)
[fetch.spec.whatwg.org](https://fetch.spec.whatwg.org/#port-blocking)
[chromium.googlesource.com](https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc)
[hg.mozilla.org](https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsIOService.cpp)
Trailing whitespace often gets removed by editors. This change would also trigger test/lint/lint-whitespace.sh. Maybe use bullet points or a blank line between items?
My editor highlights extra whitespace but sees only a line break here and test/lint/lint-whitespace.sh seems happy.
Added empty lines.
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]
Guess this was copied from /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".
Removed [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 "
Maybe add "because it is typically used by SERVICE".
I think that would be an overkill. It would require getting the service name from the port somehow, like in your suggestion below. But then 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.
Agree that that would be overkill. The ports are well-known and if not it's only a web search away.
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)
See #23542 (review). Marking this as resolved as the discussion can continue there.
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";
See #23542 (review). Marking this as resolved as the discussion can continue there.
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.
<details><summary>suggestion for your consideration</summary><p>
When Bitcoin Core automatically opens outgoing P2P connections, it chooses a
peer from the potential peers in `peers.dat` gossiped over the P2P network by
other peers.
A malicious actor can gossip an address:port where no Bitcoin node is listening,
or one where a service is listening that is not related to the Bitcoin
network. These may receive connection attempts from Bitcoin nodes.
"Bad" ports are ones used by services which are usually not open to the public
and require authentication. A connection attempt to such a service by a Bitcoin
Core node may be considered a malicious action by a paranoid administrator. An
example for such a port is 22 (ssh). On the other hand, connection attempts to
public services that usually do not require authentication are unlikely to be
considered malicious, 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 fewer incoming connections.
</p></details>
Picked some of that, thanks!
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:
// Don't forget to update doc/p2p-bad-ports.md if you change this list.
Question. Given that this code is nicely commented with ports' corresponding services, and there is a maintenance cost to keeping the lists the same, why does the list need to be duplicated in two places?
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.
Sure, makes sense
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
* [@returns](/bitcoin-bitcoin/contributor/returns/) whether the port is bad
Done.
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?
- 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);
+ 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.
@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.
@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.
uff, of course the seeder itself does not run a bitcoin node 🤦, sorry for the confusion (or it could, but that is not relevant)
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()?
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).
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)};
This doesn't seem like security-critical code, but maybe worth noting in the commit message that in addition to hashing the port we now use SipHash instead of SHA256? Since we apply rate limiting to relaying ADDR messages I don't think this is performance critical either, so I suppose any performance changes should be negligible.
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?
...
2022-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.
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.
2022-02-14T00:27:34Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
...
Is this in debug.log or terminal running bitcoind? If in terminal I think it's expected and happens for a few other logs as well.
Yes, the seeing the duplicated warning in the terminal output (not in the debug log file).
Got it - this can be marked as resolved then.
tACK 36ee76d1a Left some very minor comments but nothing blocking - feel free to ignore.
0 | @@ -0,0 +1,114 @@ 1 | +When Bitcoin Core automatically opens outgoing P2P connections it chooses
When 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.
populated 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
connect 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.
utACK 36ee76d1afbb278500fc8aa01606ec933b52c17d light code review, I'm not very familiar with this area of code
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.
Concept and light code review ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
Should there be a release note for this?
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.
This was part of 23.0 and got a release note. Removing "Needs release note".
Milestone
23.0