-
Notifications
You must be signed in to change notification settings - Fork 35.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Force port 0 in I2P #22112
Force port 0 in I2P #22112
Conversation
TODO: see if something has to be done about this assert: Line 91 in 8462cd5
|
I think that's just a way to test that the returned address object isn't empty. Should probably replace it with another test. |
Concept ACK, earlier I was convinced it would be better to get rid of ports completely for I2P, but at the time I thought it didn't exist as a concept there. I was unaware that the concept does exists for newer versions of the protocol. This is more forward compatible with future versions of |
src/net.cpp
Outdated
@@ -2019,8 +2019,9 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) | |||
// from advertising themselves as a service on another host and | |||
// port, causing a DoS attack as nodes around the network attempt | |||
// to connect to it fruitlessly. | |||
if (addr.GetPort() != Params().GetDefaultPort() && nTries < 50) | |||
if (!addr.IsI2P() && addr.GetPort() != Params().GetDefaultPort() && nTries < 50) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is exactly what we want. As (with SAM 3.1) we're unable to connect to anything except port 0 (from the perspective of later versions), shouldn't this equally bias away from port 0, as we bias away from the default port for other networks?
In other words: maybe this can be done a bit cleaner by having a network-dependent default port, which is 0 for I2P and the normal port for everything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the limitation of only being able to connect to ports that equal 0 is purely internal to the I2P code - it depends on the SAM version being used and higher level code shouldn't care about that or be aware of it. So I planted the refuse-to-connect-to-non-zero-ports check in i2p.cpp.
This allows some day to remove that by only modifying i2p.cpp
- easier, compared to if this knowledge is spilled in other modules.
A network-dependent default port seems like an unnecessary generalization to me because it is only relevant for I2P. If there was at least one other network that used !=8333 default port, then it would be more appealing :)
Off-scope: I think this entire if
should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be good to update the comment preceding this line to explain the exception for I2P.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From testing and offline discussion with @vasild, it seems a network-dependent GetDefaultPort()
is indeed needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, network-dependent default port seems to be the most elegant solution to resolve addresses without ports properly - e.g. 1.2.3.4
should be treated as 1.2.3.4:8333
and foo.b32.i2p
as foo.b32.i2p:0
.
Now this line looks better?
if (addr.GetPort() != Params().GetDefaultPort(addr.GetNetwork()) && nTries < 50) {
Me too! |
That seems to be ok as it is - addresses being used in the bench are generated, not taken from real-world addrman. The code that generates them takes care to avoid port 0: Lines 35 to 37 in a9435e3
So it might as well generate I2P addresses with !=0 ports. Currently it generates only IPv6 addresses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost-ACK 4401bb5 code review, tested rebased to master, with the following comments and modulo test coverage
- I2P local address port is now 0 in getnetworkinfo and -netinfo
- outbound connections are no longer made to any of the 13 I2P addresses in my addrman (all with port 8333), which is a bit of collateral annoyance for these nodes
- all of these 13 I2P addresses are still in the addrman, e.g. they are returned by -addrinfo and getnodeaddresses 0 i2p, confirming the pull description
- inbound I2P connections succeed (5 inbound I2P peers after 3 minutes, 7 after 15 minutes) and in getpeerinfo the addr and addrbind fields both show "b32.i2p:0"
This is a smaller and easier change than #21514 and more forward-compatible.
It might be a good idea to add tests, e.g. for the points mentioned in #21514 (comment) (and anything else you find salient).
src/net.cpp
Outdated
@@ -2019,8 +2019,9 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) | |||
// from advertising themselves as a service on another host and | |||
// port, causing a DoS attack as nodes around the network attempt | |||
// to connect to it fruitlessly. | |||
if (addr.GetPort() != Params().GetDefaultPort() && nTries < 50) | |||
if (!addr.IsI2P() && addr.GetPort() != Params().GetDefaultPort() && nTries < 50) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be good to update the comment preceding this line to explain the exception for I2P.
4401bb5
to
a9cacd2
Compare
Yes, that's really unfortunate and annoying. I wonder if it can be avoided... |
ISTM with this change we would need a way to manually or automatically convert the existing I2P entries in the addrman from port 8333 to 0. |
Add the test to the test runner? |
a9cacd2
to
d625589
Compare
Yes 🙉 |
d625589
to
28f9895
Compare
|
edit: snap! |
ACK 28f9895 modulo #22112 (comment) |
IRC discussion with @vasild and I regarding whether the remedies here and in #21514 are less desirable than leaving the "double connections" issue unresolved: https://www.erisian.com.au/bitcoin-core-dev/log-2021-06-23.html#l-167 |
|
0c6db5b
to
b336f12
Compare
|
Coverage report for modified lines by this PR and not covered by tests. There is just one line which was also not covered before this PR (this PR changed The new method added in the last commit is fully covered (I don't know what that red minus is on line 731, but it looks bogus). The last commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semi-ACK b336f12 modulo the issues noted below in #22112 (comment).
A couple of suggestions and questions below, along with some proposed edits in jonatack@539a281
Nice work on the new unit test!
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
* When accepting an I2P connection, assume the peer has port 0 instead of the default 8333 (for mainnet). It is not being sent to us, so we must assume something. * When deriving our own I2P listen CService use port 0 instead of the default 8333 (for mainnet). So that we later advertise it to peers with port 0. In the I2P protocol SAM 3.1 and older (we use 3.1) ports are not used, so they are irrelevant. However in SAM 3.2 and newer ports are used and from the point of view of SAM 3.2, a peer using SAM 3.1 seems to have specified port=0.
Change `CChainParams::GetDefaultPort()` to return 0 if the network is I2P.
When connecting to an I2P host we don't specify destination port and it is being forced to 0 by the SAM 3.1 proxy, so if we connect to the same host on two different ports, that would be actually two connections to the same service (listening on port 0). Fixes bitcoin#21389
This is a temporary change to convert I2P addresses that have propagated with port 8333 to ones with port 0. It would cause a problem some day if indeed some bitcoin software is listening on port 8333 only and rejects connections to port 0 and we are still using SAM 3.1 which only supports port 0. In this case we would replace 8333 with 0 and try to connect to such nodes. This commit should be included in 22.0 and be reverted before 23.0 is released.
Co-authored-by: Jon Atack <jon@atack.com>
0b0ee03
to
4101ec9
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 4101ec9 per git range-diff efff9c3 0b0ee03 4101ec9
, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
All the I2P peers returned by getaddednodeinfo
have port 0.
However, when bootstrapping with no peers.dat and some addnode peers, all but one of the peers returned by getnodeaddresses 0 i2p
have port 8333, presumably due to the first addnode peer connected (with port 0) relaying addresses with that port number. (No ports were specified in the addnode config.)
@@ -442,7 +442,7 @@ void SetupServerArgs(ArgsManager& argsman) | |||
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>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) are 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) 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ ./src/bitcoind -h | grep -A4 "\-port="
-port=<port>
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. Not relevant for I2P
(see doc/i2p.md).
maybe s/I2P/I2P connections/ and s/Not/This option is not/
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>", 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. This option is not relevant for I2P connections (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); |
Shut down the node, deleted the new peers.dat, recopied my backed-up peers.dat (
|
Repeated the bootstrapping steps in #22112 (review) but with no
All addresses returned by |
After a few minutes, one new Still seeing the double connection issue (all report port 0); and in some cases it is fairly long-lasting (see screenshots). Presumably, this also occurs due to those peers not running this branch. |
It is expected to have I2P addresses with port 8333 appearing in addrman - if some of our peers relays such one to us. After a restart the port will be changed to 0. It is important to remove this behavior (last commit from this PR) before real nodes appear that advertise themselves with port 8333 and only accept connections to that port (must be using SAM>=3.2 and deliberately close the connection if TO_PORT is not 8333). It is also expected that a node running master$ bitcoin-cli addnode pr.b32.i2p:8333 onetry
master$ bitcoin-cli addnode pr.b32.i2p:0 onetry
master$ bitcoin-cli getpeerinfo |jq -r 'map(select(.addr |contains("pr.b32.i2p"))) |map({id: .id, addr: .addr, addrbind: .addrbind, inbound: .inbound})'
[
{
"id": 0,
"addr": "pr.b32.i2p:8333",
"addrbind": "master.b32.i2p:8333",
"inbound": false
},
{
"id": 1,
"addr": "pr.b32.i2p:0",
"addrbind": "master.b32.i2p:8333",
"inbound": false
}
]
pr$ bitcoin-cli getpeerinfo |jq -r 'map(select(.addr |contains("master.b32.i2p"))) |map({id: .id, addr: .addr, addrbind: .addrbind, inbound: .inbound})'
[
{
"id": 23708,
"addr": "master.b32.i2p:0",
"addrbind": "pr.b32.i2p:0",
"inbound": true
},
{
"id": 23717,
"addr": "master.b32.i2p:0",
"addrbind": "pr.b32.i2p:0",
"inbound": true
}
]
|
Yes, I came to the same conclusions. FWIW tested the previous push and this one for 3 days each. |
That's pretty easy to forget. Let's open an issue for this once this is merged. |
I will take care about it. Maybe even a draft-PR that lingers for e.g. 6 months. |
Yeah, I'd remind you ;) Might be good to have this in rc1 to sanity test it with some other nodes running it. |
Code review ACK 4101ec9 |
65332b1 [addrman] Remove RemoveInvalid() (John Newbery) Pull request description: PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs: - #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed) - #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets) Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants. Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`. ACKs for top commit: sipa: utACK 65332b1. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any. fanquake: ACK 65332b1 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code. Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
Summary: > net: change assumed I2P port to 0 > > * When accepting an I2P connection, assume the peer has port 0 instead > of the default 8333 (for mainnet). It is not being sent to us, so we > must assume something. > * When deriving our own I2P listen CService use port 0 instead of the > default 8333 (for mainnet). So that we later advertise it to peers > with port 0. > > In the I2P protocol SAM 3.1 and older (we use 3.1) ports are not used, > so they are irrelevant. However in SAM 3.2 and newer ports are used and > from the point of view of SAM 3.2, a peer using SAM 3.1 seems to have > specified port=0. > net: distinguish default port per network > > Change `CChainParams::GetDefaultPort()` to return 0 if the network is > I2P. > net: do not connect to I2P hosts on port!=0 > > When connecting to an I2P host we don't specify destination port and it > is being forced to 0 by the SAM 3.1 proxy, so if we connect to the same > host on two different ports, that would be actually two connections to > the same service (listening on port 0). > > Fixes bitcoin/bitcoin#21389 > test: ensure I2P ports are handled as expected > doc: mention that we enforce port=0 in I2P > > Co-authored-by: Jon Atack <jon@atack.com> This is a backport of [[bitcoin/bitcoin#22112 | core#22112]] and [[bitcoin/bitcoin#22497 | core#22497]] (reverts all changes to `addrman` and `addrman_tests.cpp`) Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11083
This is an alternative to #21514, inspired by #21514 (comment). They are mutually exclusive. Just one of them should be merged.
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (#21514 (comment), #21514 (comment)).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.Edit: last commit amends that behavior to change all ports to 0 in addrman. There are no peers using SAM 3.2 yet and we need to be able to connect to existent peers that are already saved in addrman with port 8333.-addnode
or-connect
- a user who specifiesfoo.b32.i2p:1234
(non zero port) may wonder why "nothing is happening".Fixes #21389