p2p: add more bad ports

jlopp commented at 5:16 pm on June 29, 2025: contributor

Add a few more ports used by extremely well adopted services that require authentication and really ought not be used by bitcoin nodes for p2p traffic.

DrahtBot commented at 5:16 pm on June 29, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32826.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, l0rinc, glozow
Stale ACK	kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

jlopp renamed this:
~~add more bad p2p ports~~
p2p: add more bad ports
on Jun 29, 2025

DrahtBot added the label P2P on Jun 29, 2025

in src/test/netbase_tests.cpp:607 in 23a7807ec6 outdated

602@@ -603,14 +603,14 @@ BOOST_AUTO_TEST_CASE(isbadport)
603     BOOST_CHECK(!IsBadPort(443));
604     BOOST_CHECK(!IsBadPort(8333));
605 
606-    // Check all ports, there must be 80 bad ports in total.
607+    // Check all ports, there must be 85 bad ports in total.

l0rinc commented at 6:39 pm on June 29, 2025:

instead of updating the comment to match the code (which is the ultimate source of truth anyway), seems like a good opportunity to remove the duplication of the code below - which is already trivial, no need for a dead comment that doesn’t add any new info.

in src/netbase.cpp:924 in 23a7807ec6 outdated

920@@ -921,10 +921,14 @@ bool IsBadPort(uint16_t port)
921     case 1720:  // h323hostcall
922     case 1723:  // pptp
923     case 2049:  // nfs
924+    case 3306:  // MySQL

l0rinc commented at 6:41 pm on June 29, 2025:

👍 https://dev.mysql.com/doc/mysql-port-reference/en/mysql-port-reference-tables.html#GUID-65C1FF7E-5357-4E58-8D68-A0C3D24C0832__MYSQL-CLIENT-SERVER-PORTS

in src/netbase.cpp:925 in 23a7807ec6 outdated

920@@ -921,10 +921,14 @@ bool IsBadPort(uint16_t port)
921     case 1720:  // h323hostcall
922     case 1723:  // pptp
923     case 2049:  // nfs
924+    case 3306:  // MySQL
925+    case 3389:  // RDP / Windows Remote Desktop

l0rinc commented at 6:42 pm on June 29, 2025:

👍 https://www.datacentreplus.co.uk/support/knowledge-base/changing-the-rdp-port-on-a-windows-server

in src/netbase.cpp:930 in 23a7807ec6 outdated

925+    case 3389:  // RDP / Windows Remote Desktop
926     case 3659:  // apple-sasl / PasswordServer
927     case 4045:  // lockd
928     case 5060:  // sip
929     case 5061:  // sips
930+    case 5432:  // PostgreSQL

l0rinc commented at 6:43 pm on June 29, 2025:

👍 https://www.postgresql.org/docs/current/app-postgres.html

in src/netbase.cpp:931 in 23a7807ec6 outdated

926     case 3659:  // apple-sasl / PasswordServer
927     case 4045:  // lockd
928     case 5060:  // sip
929     case 5061:  // sips
930+    case 5432:  // PostgreSQL
931+    case 5900:  // VNC

l0rinc commented at 6:45 pm on June 29, 2025:

👍 https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/6/html/deployment_guide/s1-connecting-to-vnc-server#sec-Configuring_the_Firewall_for_VNC

in src/netbase.cpp:941 in 23a7807ec6 outdated

937@@ -934,6 +938,7 @@ bool IsBadPort(uint16_t port)
938     case 6669:  // Alternate IRC
939     case 6697:  // IRC + TLS
940     case 10080: // Amanda
941+    case 27017: // MongoDB

l0rinc commented at 6:45 pm on June 29, 2025:

👍 https://www.mongodb.com/docs/manual/reference/default-mongodb-port

in src/test/netbase_tests.cpp:613 in 23a7807ec6 outdated

610         if (IsBadPort(port)) {
611             ++total_bad_ports;
612         }
613     }
614-    BOOST_CHECK_EQUAL(total_bad_ports, 80);
615+    BOOST_CHECK_EQUAL(total_bad_ports, 85);

l0rinc commented at 6:59 pm on June 29, 2025:

I was wondering if we could do a more functional approach here, since we already have a few count_if instances - not sure it’s better or not, will let you decide, please resolve if you don’t think it’s a good alternative (assuming integer promotion for max + 1 here):

0    using namespace std::ranges;
1    auto ports{views::iota(1, std::numeric_limits<uint16_t>::max() + 1)};
2    BOOST_CHECK_EQUAL(count_if(ports, IsBadPort), 85);

jlopp commented at 7:32 pm on June 29, 2025:

implemented

l0rinc approved

l0rinc commented at 7:05 pm on June 29, 2025: contributor

ACK 23a7807ec6d35d3ef5b65e4abc3b3841e84b702e

Verified that these are indeed the default ports and that the docs match the code. Have a miner test suggestion, LGTM otherwise.

in src/test/netbase_tests.cpp:611 in 8716fd2f4a outdated

614-    BOOST_CHECK_EQUAL(total_bad_ports, 80);
615+    // Check all ports, there must be 85 bad ports in total.
616+    using namespace std::ranges;
617+    std::list<int> ports(std::numeric_limits<uint16_t>::max());
618+    std::iota(ports.begin(), ports.end(), 1);
619+    BOOST_CHECK_EQUAL(count_if(ports, IsBadPort), 85);

l0rinc commented at 7:35 pm on June 29, 2025:

might as well inline it now that it’s only used once

0    BOOST_CHECK_EQUAL(std::ranges::count_if(ports, IsBadPort), 85);

jlopp commented at 8:05 pm on June 29, 2025:

done

in src/test/netbase_tests.cpp:608 in 8716fd2f4a outdated

612-        }
613-    }
614-    BOOST_CHECK_EQUAL(total_bad_ports, 80);
615+    // Check all ports, there must be 85 bad ports in total.
616+    using namespace std::ranges;
617+    std::list<int> ports(std::numeric_limits<uint16_t>::max());

l0rinc commented at 7:41 pm on June 29, 2025:

👍, looks like this contains the end as well, no +1 needed. Checked with BOOST_CHECK_EQUAL(std::ranges::count_if(ports, [&](const int i) { return i == std::numeric_limits<uint16_t>::max(); }), 1); to make sure.

Note though that this is a substantial memory allocation, the previous suggestion was lazy. For the record here’s the complete lazy solution I suggested (with fixed imports):

 0diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
 1index e3bf482873..0404a45c79 100644
 2--- a/src/test/netbase_tests.cpp
 3+++ b/src/test/netbase_tests.cpp
 4@@ -2,6 +2,12 @@
 5 // Distributed under the MIT software license, see the accompanying
 6 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 7 
 8+#include <algorithm>
 9+#include <limits>
10+#include <numeric>
11+#include <ranges>
12+#include <string>
13+
14 #include <net_permissions.h>
15 #include <netaddress.h>
16 #include <netbase.h>
17@@ -13,12 +19,10 @@
18 #include <util/strencodings.h>
19 #include <util/translation.h>
20 
21-#include <string>
22-#include <numeric>
23-
24 #include <boost/test/unit_test.hpp>
25 
26 using namespace std::literals;
27+using namespace std::ranges;
28 
29 BOOST_FIXTURE_TEST_SUITE(netbase_tests, BasicTestingSetup)
30 
31@@ -604,10 +608,7 @@ BOOST_AUTO_TEST_CASE(isbadport)
32     BOOST_CHECK(!IsBadPort(443));
33     BOOST_CHECK(!IsBadPort(8333));
34 
35-    // Check all ports, there must be 85 bad ports in total.
36-    using namespace std::ranges;
37-    std::list<int> ports(std::numeric_limits<uint16_t>::max());
38-    std::iota(ports.begin(), ports.end(), 1);
39+    constexpr auto ports{views::iota(1, std::numeric_limits<uint16_t>::max() + 1)};
40     BOOST_CHECK_EQUAL(count_if(ports, IsBadPort), 85);
41 }

l0rinc commented at 7:55 pm on June 29, 2025: contributor

ACK 8716fd2f4a00b07426e54a1491af421d6345f708

kevkevinpal commented at 7:58 pm on June 29, 2025: contributor

ACK 8716fd2

It makes sense to me to add these ports since they’re popular and used. I double-checked the links @l0rinc provided and also did a quick search myself to make sure they were correct

maflcko commented at 6:25 am on June 30, 2025: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

in doc/p2p-bad-ports.md:91 in 8f36e7e5cf outdated

86@@ -87,10 +87,14 @@ incoming connections.
87     1720:  h323hostcall
88     1723:  pptp
89     2049:  nfs
90+    3306:  MySQL
91+    3389:  RDP / Windows Remote Desktop

Sjors commented at 8:43 am on June 30, 2025:

Trying to connect to (or open( that port sounds like a great way to be flagged as malware / attacker.

jlopp force-pushed on Jun 30, 2025

add more bad p2p ports 6967e8e8ab

jlopp force-pushed on Jun 30, 2025

Sjors commented at 10:41 am on June 30, 2025: member

utACK 6967e8e8abbc35ac98e8e3745a8bbed56e77526f

I checked that the ports make sense.

DrahtBot requested review from l0rinc on Jun 30, 2025

l0rinc approved

l0rinc commented at 11:09 am on June 30, 2025: contributor

ACK 6967e8e8abbc35ac98e8e3745a8bbed56e77526f

Changes since my last review are comment updates and namespace inlines.

My only remaining concern is the unnecessary heavy memory usage of the test - but it’s not a blocker.

bitcoin deleted a comment on Jun 30, 2025

glozow commented at 5:21 pm on June 30, 2025: member

ACK 6967e8e8abbc35ac98e8e3745a8bbed56e77526f

glozow merged this on Jun 30, 2025

glozow closed this on Jun 30, 2025

jlopp deleted the branch on Jun 30, 2025

fanquake commented at 3:39 pm on July 4, 2025: member

Backported to 29.x in #32863.

fanquake referenced this in commit 58b1a65ab0 on Jul 4, 2025

p2p: add more bad ports #32826

Code Coverage & Benchmarks

Reviews