doc: warn that incoming conns are unlikely when not using default ports #20668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
diff: --- /tmp/1.log 2020-12-17 12:05:19.661809857 +0100
+++ /tmp/2.log 2020-12-17 12:04:46.502043685 +0100
@@ -1,4 +1,4 @@
-Bitcoin Core version v21.99.0-f0913f2f950c7a3e0a14d32216bd6ce4e19d85df
+Bitcoin Core version v21.99.0-6d74b0e0ac6e9bc63a893ca3cfe33362d445f595
Copyright (C) 2009-2020 The Bitcoin Core developers
Please contribute if you find Bitcoin Core useful. Visit
@@ -238,8 +238,9 @@
Relay non-P2SH multisig (default: 1)
-port=<port>
- Listen for connections on <port> (default: 8333, testnet: 18333 signet:
- 38333, regtest: 18444)
+ Listen for connections on <port>. Nodes not using the default ports
+ (default: 8333, testnet: 18333, signet: 38333, regtest: 18444)
+ are unlikely to get incoming connections.
-proxy=<ip:port>
Connect through SOCKS5 proxy, set -noproxy to disable (default:re-ACK 010eed3 |
jonatack
reviewed
Dec 16, 2020
src/init.cpp
Outdated
| @@ -450,7 +450,7 @@ void SetupServerArgs(NodeContext& node) | |||
| argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u signet: %u, regtest: %u)", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) will be unlikely to get incoming connections.", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); | |||
There was a problem hiding this comment.
thanks for fixing up the grammar while here
tried to make this part more easy/clear, not sure it's better ("not using"..."not see" symmetry)
Suggested change
| argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) will be unlikely to get incoming connections.", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); | |
| argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) will likely not see incoming connections.", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); |
|
ACK b2bbc08 Nice find |
src/init.cpp
Outdated
| @@ -450,7 +450,7 @@ void SetupServerArgs(NodeContext& node) | |||
| argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u signet: %u, regtest: %u)", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); | |||
| argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) will be unlikely to get incoming connections.", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); | |||
There was a problem hiding this comment.
Nit: "are" instead of "will be"?
adamjonas
force-pushed
the
121520-non-standard-port-doc
branch
from
b2bbc08
to
010eed3
Compare
|
Updated to go with @practicalswift's readability recommendation. Thanks for the quick review and suggestions. |
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Dec 17, 2020
Github-Pull: bitcoin#20668 Rebased-From: 010eed3
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 17, 2020
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Oct 25, 2021
92617b7 Make AddrMan support multiple ports per IP (Pieter Wuille) Pull request description: For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that. The folklore justification (eventually actually added as a comment to the codebase in bitcoin#20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in bitcoin#5150 (comment) e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups). And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there. One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket _already_ uses the port number, the only change required is making all addrman lookup be (IP,port) (aka `CService`) based, rather than IP (aka `CNetAddr`) based. This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually. ACKs for top commit: jnewbery: Code review ACK 92617b7 naumenkogs: ACK 92617b7 ajtowns: ACK 92617b7 vasild: ACK 92617b7 Tree-SHA512: 9eef06ce97a8b54a3f05fb8acf6941f253a9a5e0be8ce383dd05c44bb567cea243b74ee5667178e7497f6df2db93adab97ac66edbc37c883fd8ec840ee69a33f
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5150.
This was mostly copied from #5285 by sulks, who has since quit GitHub.
The issue has remained open for 6 years, but the extra explanation still seems useful.