p2p: Ignore ports on I2P addresses #21514

vasild · 2021-03-23T16:27:18Z

This is an alternative to #22112. They are mutually exclusive. Just one of them should be merged.

Background

Listening for incoming connections in I2P does not involve a port - we just listen on an address. Similarly we just connect to an I2P address, without a port (this is in I2P SAM 3.1, what's used in Bitcoin Core).

Problem

It is possible to connect to the same node multiple times if different ports are used.

Solution

For I2P addresses:

Ignore ports when comparing CService objects
Do not print ports in CService::ToString()
Strip ports from user-input addresses - this helps to detect duplicates earlier (by CConnMan::FindNode(std::string)) and to print without the ports in the cases where we print the user input without converting it to CService object and using its ToString() method

Fixes #21389

DrahtBot · 2021-03-23T21:57:57Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Validate port-options #22087 by amadeuszpawlik
p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified #20018 by dhruv

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.

laanwj · 2021-03-25T10:00:29Z

Concept ACK

I think it would be a useful abstraction to introduce CService::HasPort or something like that. There are more protocols that do not have multi-port endpoints (UNIX sockets come to mind as an important one) and it would be slightly clearer than special-casing I2P.

vasild · 2021-03-30T10:44:11Z

5c3eaea36...f30e489c3: added an abstraction to check if a given network is using ports, making this not I2P specific.

"uses ports or not" is a per-network property (like IPv4 uses ports, I2P does not use ports), so I added a standalone function that takes an enum Network argument to check for that instead of adding a CNetAddr or CService method. It is not required to have a CNetAddr or CService object in order to check the uses-ports property of a given network.

jonatack · 2021-03-30T13:36:12Z

Thanks for working on this! Will review soon.

jonatack

ACK f30e489c3882cd58e382c203e5e4722fbe70f58e with some suggestions

src/netaddress.h

jonatack · 2021-03-31T14:20:03Z

src/test/netbase_tests.cpp

987d372 Verified that these new assertions fail without the change to ToStringIPPort() in the same commit.

While here, maybe sneak this minor fixup into the commit

-bool static TestParse(std::string src, std::string canon) +bool static TestParse(const std::string& src, const std::string& canon) { - CService addr(LookupNumeric(src, 65535)); + CService addr{LookupNumeric(src, 65535)};

jonatack · 2021-03-31T14:23:14Z

src/test/netbase_tests.cpp

8f4bd325 This I2P address string is created 4 times in this test file now; perhaps hoist to a constant.

I prefer to keep it local to each test. It is just a random address, does not need to be equal in all test cases.

src/test/netbase_tests.cpp

jonatack · 2021-03-31T14:46:30Z

test/functional/feature_config_args.py

6f19e34 suggested simplification

def test_addnode_and_connect_i2p(self): - self.log.info('Test that ports are removed from -addnode= and -connect= I2P addresses') - + self.log.info("Test ports are removed from I2P addresses passed to -addnode/-connect") i2p_addr = "ukeu3k5oycgaauneqgtnvselmt4yemvoilkln7jpvamvfx7dnkdq.b32.i2p" i2p_addr_with_port = i2p_addr + ":3333" - - self.stop_node(0) - with self.nodes[0].assert_debug_log(expected_msgs=[ - f"trying connection {i2p_addr} lastseen=", - ]): - self.start_node(0, extra_args=[f"-addnode={i2p_addr_with_port}"]) - - self.stop_node(0) - with self.nodes[0].assert_debug_log(expected_msgs=[ - f"trying connection {i2p_addr} lastseen=", - ]): - self.start_node(0, extra_args=[f"-connect={i2p_addr_with_port}"]) + msg = [f"trying connection {i2p_addr} lastseen="] + for option in {"addnode", "connect"}: + with self.nodes[0].assert_debug_log(expected_msgs=msg): + self.restart_node(0, extra_args=[f"-{option}={i2p_addr_with_port}"]) def run_test(self):

Taketh, thanks!

One minor downside of the new variant is that if we get exception on line 224, then it is not clear whether the problem is in -addnode or -connect.

Good point, loglevel=debug (logged by the CI in case of failure) would reliably indicate which one if we use an array/dict rather than a set:

- for option in {"addnode", "connect"}: + for option in ["addnode", "connect"]:

or by adding, for instance

for option in {"addnode", "connect"}: + self.log.debug(f"-{option}") with self.nodes[0].assert_debug_log(expected_msgs=[msg]):

then the log would show

2021-04-01T17:42:16.846000Z TestFramework (INFO): Test ports are removed from I2P addresses passed to -addnode/-connect 2021-04-01T17:42:16.846000Z TestFramework (DEBUG): -connect 2021-04-01T17:42:16.846000Z TestFramework.node0 (DEBUG): Stopping node 2021-04-01T17:42:16.999000Z TestFramework.node0 (DEBUG): Node stopped 2021-04-01T17:42:17.002000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up 2021-04-01T17:42:17.512000Z TestFramework.node0 (DEBUG): RPC successfully started 2021-04-01T17:42:17.514000Z TestFramework (DEBUG): -addnode 2021-04-01T17:42:17.514000Z TestFramework.node0 (DEBUG): Stopping node 2021-04-01T17:42:17.618000Z TestFramework.node0 (DEBUG): Node stopped 2021-04-01T17:42:17.628000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up 2021-04-01T17:42:18.140000Z TestFramework.node0 (DEBUG): RPC successfully started

...would reliably indicate which one if we use an array/dict rather than a set...

Hmm, I tried this, but did not see any difference in the output, what am I missing? (I sneaked a deliberate change of the expected message if the option is e.g. addnode in order to trigger a failure).

I wasn't very clear. I meant that it would have a fixed order so you would know which failure it is. Array + debug logging probably best.

vasild · 2021-04-01T16:57:21Z

f30e489c3...5be0a46b1: address suggestions

jonatack · 2021-04-01T17:54:21Z

re-ACK 5be0a46b1623536d98aec641af0d24401995e837

sipa · 2021-04-01T18:41:06Z

Would it make sense to make it illegal for I2P hostnames to have a port in the first place?

vasild · 2021-04-02T15:53:08Z

Would it make sense to make it illegal for I2P hostnames to have a port in the first place?

We receive I2P host names from the outside world from:

command line arguments
RPC
the P2P layer via gossip
(is there more to this?)

The first two receive string arguments, so we can parse them and reject/return an error if we see an I2P address followed by :port. I am -0.1 on this, see below.

On the P2P layer we inevitably receive a port in the addrv2 encoding. We may insist that the port is e.g. 0 and ignore the address otherwise. I am -1 on this, because:

I2P in general supports ports. It is just the proxy protocol we use that does not support them (SAM 3.1). Other proxy protocols or newer versions of SAM do support ports (we don't use newer SAM versions because i2pd supports maximum SAM 3.1).

So, I think it is best to ignore ports in I2P addresses and some day in the distant future if we decide to support them, then revert parts of this PR. But not impose restrictions on the P2P protocol.

sipa · 2021-04-02T16:25:05Z

Hmm can you explain a bit more what changes in later SAM protocols? The ability to create "portful" services, the ability to connect to them, both, ... what happens when an older-protocol node tries to connect to portful newer-protocol node or the other way around?

This may inform what to do, if we envision ever adopting a later version.

jonatack · 2021-04-02T16:36:01Z

If helpful,

https://github.com/PurpleI2P/i2pd/wiki/Roadmap has "Implement SAM v3.3" on the roadmap (but the last update to the roadmap was in 2016; the code repo appears to be active however)
https://i2pd.readthedocs.io/en/latest/user-guide/configuration/#sam-interface is the current i2pd SAM interface doc
https://github.com/PurpleI2P/i2pd/wiki/Differences-between-i2pd-and-Java-I2P-router lists a few differences between the implementations, nothing "portful" though

vasild · 2021-04-02T17:34:47Z

@sipa, short answer - it works either way. Long answer:

Hmm can you explain a bit more what changes in later SAM protocols? The ability to create "portful" services, the ability to connect to them, both,

Both. The SAM protocol is described here: https://geti2p.net/en/docs/api/samv3 (relevant: "Version 3.2 Changes" and "FROM_PORT" / "TO_PORT").

what happens when an older-protocol node tries to connect to portful newer-protocol node or the other way around?

SAM 3.2 Client -> SAM 3.2 Listener
- Listener: SESSION CREATE ... FROM_PORT=100 TO_PORT=200
- Listener: STREAM ACCEPT ...
- Client: SESSION CREATE ... (no ports needed here)
- Client: STREAM CONNECT ... FROM_PORT=500 TO_PORT=600 (deliberately different from the above)
- Listener receives: ...addr of client... FROM_PORT=500 TO_PORT=600
- Then the connection is established just fine (!). I guess the listener has to close the connection manually if he does not like {FROM,TO}_PORT. It is unclear to me what is the purpose of {FROM,TO}_PORT in the SESSION CREATE command.
SAM 3.2 Client -> SAM 3.1 Listener
- Listener: SESSION CREATE ... (without {FROM,TO}_PORT)
- Listener: STREAM ACCEPT ...
- Client: SESSION CREATE ... (no ports needed here)
- Client: STREAM CONNECT ... FROM_PORT=500 TO_PORT=600
- Listener receives: ...addr of client...
- Then the connection is established just fine.
SAM 3.1 Client -> SAM 3.2 Listener
- Listener: SESSION CREATE ... FROM_PORT=100 TO_PORT=200
- Listener: STREAM ACCEPT ...
- Client: SESSION CREATE ... (no ports needed here)
- Client: STREAM CONNECT ...
- Listener receives: ...addr of client... FROM_PORT=0 TO_PORT=0
- Then the connection is established just fine.

raw transcripts in case somebody wants to play

# Listener 3.2 socket1
HELLO VERSION MIN=3.2 MAX=3.2
SESSION CREATE STYLE=STREAM ID=newlistenersession DESTINATION=il9BGrm9DNCXjCQ7lBCkmOOcaMIEIjdypahLIdn6xlIktHB4e2Dve6Wpo5dueVpH7s~UcK9XS9aUplxWU8zK1kEuk3Ty~dmwhs-vO0hzEdb7a8Hp25hQL1otv0InignPkLRx3eV-3Ksn6PmYuGDJt7gUXIVR2Q-Qc8tALBlDxejDuWsu4O33jeG9uw-pvAnza4FOSbP2KYv5Zm86RVwV2nQs78tBHsNVHqNuD5pCkQGlTbBA-PMQNqvvwjgqAln9jiqVDCTkcz~lF1NnntCctnoVHxPrNdxde3kEXRc~MCBOuuJ8Dals89TlGqd~9-P7PZxDbqqH4K6olUoqSTH-YtSb8ZjUAz35~pn-hkeyEJQOjVTzzAO20W~Udh3bNQamLUma4Hml6IDmpBi4fLa7450LJKb3WYhobhptCgem91lWSCqYeA3bW~UuCMp2OUrZ~i8-OhVh1SMGhChS1Qu-ZsLh4ZF1SCJdugZHSBF8VVnrlv--vasP2JBhbXb3LczHBQAEAAcAAMacUI6416R9uJapm5lWWfxiyWLPjJCaiYT0p0S-KySbPqOtXjy5HBd63HbC9u4eeT34RTPEuR0zaiVoSFIOyOaf0zTSQD2r32gwysjSgmjyJX7t9VB-uTGzL24W2WGrD1uidTW2YdQ2mGmwJXeSy2pFySEfJP34BYs~0V5c~rDz2CgyBpaAVuDMO3kCNB0tVw6JIprpX-bxoTkrHOgB6IkhTdDMJMwTvBtUBeWAwZJtt7-uc8aGZf5In0NyaRwn~JaqjKjbxjA0A9e1sYyayOtJpZxnek2s0hRxdW64uJFIbywYmnbQuMn~ovfbLl17ZC4Dj0IlNubjIehal9TmgIaVYJ-svqKadbOare6wZ5ETG5kNjx19YQ~F-DfB-UmTQQ== FROM_PORT=100 TO_PORT=200

# Listener 3.2 socket2
HELLO VERSION MIN=3.2 MAX=3.2
STREAM ACCEPT ID=newlistenersession SILENT=false

# Listener 3.1 socket1
HELLO VERSION MIN=3.1 MAX=3.1
SESSION CREATE STYLE=STREAM ID=oldlistenersession DESTINATION=il9BGrm9DNCXjCQ7lBCkmOOcaMIEIjdypahLIdn6xlIktHB4e2Dve6Wpo5dueVpH7s~UcK9XS9aUplxWU8zK1kEuk3Ty~dmwhs-vO0hzEdb7a8Hp25hQL1otv0InignPkLRx3eV-3Ksn6PmYuGDJt7gUXIVR2Q-Qc8tALBlDxejDuWsu4O33jeG9uw-pvAnza4FOSbP2KYv5Zm86RVwV2nQs78tBHsNVHqNuD5pCkQGlTbBA-PMQNqvvwjgqAln9jiqVDCTkcz~lF1NnntCctnoVHxPrNdxde3kEXRc~MCBOuuJ8Dals89TlGqd~9-P7PZxDbqqH4K6olUoqSTH-YtSb8ZjUAz35~pn-hkeyEJQOjVTzzAO20W~Udh3bNQamLUma4Hml6IDmpBi4fLa7450LJKb3WYhobhptCgem91lWSCqYeA3bW~UuCMp2OUrZ~i8-OhVh1SMGhChS1Qu-ZsLh4ZF1SCJdugZHSBF8VVnrlv--vasP2JBhbXb3LczHBQAEAAcAAMacUI6416R9uJapm5lWWfxiyWLPjJCaiYT0p0S-KySbPqOtXjy5HBd63HbC9u4eeT34RTPEuR0zaiVoSFIOyOaf0zTSQD2r32gwysjSgmjyJX7t9VB-uTGzL24W2WGrD1uidTW2YdQ2mGmwJXeSy2pFySEfJP34BYs~0V5c~rDz2CgyBpaAVuDMO3kCNB0tVw6JIprpX-bxoTkrHOgB6IkhTdDMJMwTvBtUBeWAwZJtt7-uc8aGZf5In0NyaRwn~JaqjKjbxjA0A9e1sYyayOtJpZxnek2s0hRxdW64uJFIbywYmnbQuMn~ovfbLl17ZC4Dj0IlNubjIehal9TmgIaVYJ-svqKadbOare6wZ5ETG5kNjx19YQ~F-DfB-UmTQQ==

# Listener 3.1 socket2
HELLO VERSION MIN=3.1 MAX=3.1
STREAM ACCEPT ID=oldlistenersession SILENT=false

# Client 3.2 socket1
HELLO VERSION MIN=3.2 MAX=3.2
SESSION CREATE STYLE=STREAM ID=newclientsession DESTINATION=bTy5lwz1fc6nhH4hd88~8nlhT-Ls6E8qrGIEId1Of1JwILHya-gsXBXqPyO7X4Nd4zjPrOxWLmXrluxOxQWHr4ARrL2GhfgCj6hqySM~tG9BnnMqZiu4oil9i~XhJ7XU526FcG4o3jNL0vGxtQqle64PO9dezxtsORpsNnl-2l8soQFHxzMjw6nkZSwBIT2Uy44rB3jvQ0ufGNr8kKVKZgIS-7GpFuvPZucJSehztIZ2hK4Ffz-qquUrTaXTSBmaLQPEQpgxePXgasfb2mnbOx6L1qmjUoAUWaL4JQEfPYbfU5hr~BJ7NkxzvaMY1nb-FyJF5se-huNxRIovmbkz-35Ui6D0WZnV0WG2yCyVvRApLa90CHI~sy~El2wUzAk51TY7PJ29TdqMH7fLVTgPyCr1p2Yq7tJgqYxn2nIiUi90qiAHI5N7YqUAetjzWf48mgtgvlh7bD6vsb7CBPN311zYjK3CGOpHEDHvB4Q7S~tWKaVdKZteitopsGAiPCY3BQAEAAcAAA1nRAP3r9C4iPvXg-oGc9bVnOuLx2c2jein3cYgg0JJugOOU5uwYfpptLIHcK5Ejm6O6T0YWXAa9MgdGT9Qq7-urBsJIWng1h67rcIKXV5sj9AkuKXF43zBk-7LTsZujWnqU8LGoE49-2dMNJfQHH0HMUxyw23AuTZHQ4esK-ymCB5eD6EBnuWdpZAzo7Rbk-Spq5RVVoPrP-sR-FweFW3Lr-irPuGGE1B4bzODqesnpJeQlbwxCek40zdhqK5rBOW~rxCdxYk3-mrxnJxuOCnHs0eM0686ur3KPbJYhA2ibXTyDShnWCxTiLyYawfkK17IuP9V1R0magY3DlGTukgcsrRri1vQMpB2DJ3JLwcB9hUHeFcglFKZ7ZkSOCKTnw==

# Client 3.2 socket2
HELLO VERSION MIN=3.2 MAX=3.2
STREAM CONNECT ID=newclientsession DESTINATION=il9BGrm9DNCXjCQ7lBCkmOOcaMIEIjdypahLIdn6xlIktHB4e2Dve6Wpo5dueVpH7s~UcK9XS9aUplxWU8zK1kEuk3Ty~dmwhs-vO0hzEdb7a8Hp25hQL1otv0InignPkLRx3eV-3Ksn6PmYuGDJt7gUXIVR2Q-Qc8tALBlDxejDuWsu4O33jeG9uw-pvAnza4FOSbP2KYv5Zm86RVwV2nQs78tBHsNVHqNuD5pCkQGlTbBA-PMQNqvvwjgqAln9jiqVDCTkcz~lF1NnntCctnoVHxPrNdxde3kEXRc~MCBOuuJ8Dals89TlGqd~9-P7PZxDbqqH4K6olUoqSTH-YtSb8ZjUAz35~pn-hkeyEJQOjVTzzAO20W~Udh3bNQamLUma4Hml6IDmpBi4fLa7450LJKb3WYhobhptCgem91lWSCqYeA3bW~UuCMp2OUrZ~i8-OhVh1SMGhChS1Qu-ZsLh4ZF1SCJdugZHSBF8VVnrlv--vasP2JBhbXb3LczHBQAEAAcAAA== SILENT=false FROM_PORT=500 TO_PORT=600

# Client 3.1 socket1
HELLO VERSION MIN=3.1 MAX=3.1
SESSION CREATE STYLE=STREAM ID=oldclientsession DESTINATION=bTy5lwz1fc6nhH4hd88~8nlhT-Ls6E8qrGIEId1Of1JwILHya-gsXBXqPyO7X4Nd4zjPrOxWLmXrluxOxQWHr4ARrL2GhfgCj6hqySM~tG9BnnMqZiu4oil9i~XhJ7XU526FcG4o3jNL0vGxtQqle64PO9dezxtsORpsNnl-2l8soQFHxzMjw6nkZSwBIT2Uy44rB3jvQ0ufGNr8kKVKZgIS-7GpFuvPZucJSehztIZ2hK4Ffz-qquUrTaXTSBmaLQPEQpgxePXgasfb2mnbOx6L1qmjUoAUWaL4JQEfPYbfU5hr~BJ7NkxzvaMY1nb-FyJF5se-huNxRIovmbkz-35Ui6D0WZnV0WG2yCyVvRApLa90CHI~sy~El2wUzAk51TY7PJ29TdqMH7fLVTgPyCr1p2Yq7tJgqYxn2nIiUi90qiAHI5N7YqUAetjzWf48mgtgvlh7bD6vsb7CBPN311zYjK3CGOpHEDHvB4Q7S~tWKaVdKZteitopsGAiPCY3BQAEAAcAAA1nRAP3r9C4iPvXg-oGc9bVnOuLx2c2jein3cYgg0JJugOOU5uwYfpptLIHcK5Ejm6O6T0YWXAa9MgdGT9Qq7-urBsJIWng1h67rcIKXV5sj9AkuKXF43zBk-7LTsZujWnqU8LGoE49-2dMNJfQHH0HMUxyw23AuTZHQ4esK-ymCB5eD6EBnuWdpZAzo7Rbk-Spq5RVVoPrP-sR-FweFW3Lr-irPuGGE1B4bzODqesnpJeQlbwxCek40zdhqK5rBOW~rxCdxYk3-mrxnJxuOCnHs0eM0686ur3KPbJYhA2ibXTyDShnWCxTiLyYawfkK17IuP9V1R0magY3DlGTukgcsrRri1vQMpB2DJ3JLwcB9hUHeFcglFKZ7ZkSOCKTnw==

# Client 3.1 socket2
HELLO VERSION MIN=3.1 MAX=3.1
STREAM CONNECT ID=oldclientsession DESTINATION=il9BGrm9DNCXjCQ7lBCkmOOcaMIEIjdypahLIdn6xlIktHB4e2Dve6Wpo5dueVpH7s~UcK9XS9aUplxWU8zK1kEuk3Ty~dmwhs-vO0hzEdb7a8Hp25hQL1otv0InignPkLRx3eV-3Ksn6PmYuGDJt7gUXIVR2Q-Qc8tALBlDxejDuWsu4O33jeG9uw-pvAnza4FOSbP2KYv5Zm86RVwV2nQs78tBHsNVHqNuD5pCkQGlTbBA-PMQNqvvwjgqAln9jiqVDCTkcz~lF1NnntCctnoVHxPrNdxde3kEXRc~MCBOuuJ8Dals89TlGqd~9-P7PZxDbqqH4K6olUoqSTH-YtSb8ZjUAz35~pn-hkeyEJQOjVTzzAO20W~Udh3bNQamLUma4Hml6IDmpBi4fLa7450LJKb3WYhobhptCgem91lWSCqYeA3bW~UuCMp2OUrZ~i8-OhVh1SMGhChS1Qu-ZsLh4ZF1SCJdugZHSBF8VVnrlv--vasP2JBhbXb3LczHBQAEAAcAAA== SILENT=false

sipa · 2021-04-07T16:25:40Z

@vasild So... that kind of calls for treating the lack of ports right now as forcing port=0. I guess that's a bit annoying to implement, but if it's possible perhaps the most forward-compatible approach:

Make the default port in I2P connections 0 (rather than 8333/18333 etc).
Refuse to make outgoing connections that don't have port field equal to 0
Still print it in output/ToString conversions

That would mean that if/when ports are actually adopted, you won't get any surprises.

vasild · 2021-04-09T08:44:24Z

5be0a46b1...9ea6503d7: in the added test, use python's dict ([]) instead of set ({}) to make the order of execution deterministic (would help with diagnosis if the test fails)

vasild · 2021-04-09T09:48:36Z

@sipa, I made some further experiments. Since one may connect to I2P in ways other than using the SAM protocol, I tried using the I2P router's HTTP proxy to connect to a SAM 3.2 listener (who has done SESSION CREATE ... FROM_PORT=100 TO_PORT=200). All went as expected:

Entering http://foo.b32.i2p in the browser, the listener receives ... FROM_PORT=0 TO_PORT=80
Entering https://foo.b32.i2p in the browser, the listener receives ... FROM_PORT=0 TO_PORT=443
Entering http://foo.b32.i2p:1234 in the browser, the listener receives ... FROM_PORT=0 TO_PORT=1234

Hmm...

vasild · 2021-04-12T15:07:58Z

So, it looks like with I2P we listen on just address (not on address:port like in TCP). The listener receives the port the client is connecting to as a parameter along with the incoming connection (or port=0 if the client uses SAM 3.1 or older). This means that in I2P it is not possible to have two different programs listening on the same address but different port like in TCP.

naumenkogs · 2021-04-16T10:03:31Z

Concept ACK on improving abstractions.
Haven't verify the mentioned problem.

jonatack · 2021-05-05T15:50:26Z

re-ACK 9ea6503d753a3c84bc2ea3b3e61504e8bd46c40c modulo ensuring this change is compatible with the I2P seeds and non-default port check in net.cpp::CConnman::ThreadOpenConnections.

(When testing the I2P seed nodes for #21825, I first tried adding them with no port numbers. Testing with a fresh peers.dat and only those I2P seeds in chainparamsseeds and in my AddrMan, I realizing they weren't organically connecting because the port number was missing (e.g. a missing port is converted to 0). Adding port 8333 like for the other seeds enabled the I2P ones to connect immediately.)

jonatack · 2021-05-19T14:44:00Z

Would be good to have this fixed for v22.

From our perspective ports are ignored when connecting to the I2P network. I.e. `aaa.b32.i2p:1111` is the same as `aaa.b32.i2p:2222`, so compare them equal to avoid redundant connections to the same node if ports happen to differ for some reason. Fixes bitcoin#21389

Since ports are irrelevant in our usage of I2P, print just `aaa.b32.i2p` instead of `aaa.b32.i2p:8333` when converting `CService` to string.

This makes printing to the log proper (without ports for I2P addresses).

This makes printing to the log proper (without ports for I2P addresses) and also discovers duplicates earlier in `CConnman::AddNode()`.

For networks that do not use ports (e.g. I2P), remove the `:port` suffix (if any) when searching for an address by string.

This is a followup to `236618061a445d2cb11e722cfac5fdae5be26abb` which forgot to update the comment - we don't return the existent connection from `CConnman::ConnectNode()` anymore after that commit.

vasild · 2021-05-31T16:19:16Z

9ea6503d75...6d8d59284b: rebase due to conflicts

vasild · 2021-05-31T16:32:47Z

@vasild So... that kind of calls for treating the lack of ports right now as forcing port=0...

That makes sense (too). Implemented in #22112, lets see if it will fly better than this PR :)

4101ec9 doc: mention that we enforce port=0 in I2P (Vasil Dimov) e0a2b39 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov) 41cda9d test: ensure I2P ports are handled as expected (Vasil Dimov) 4f432bd net: do not connect to I2P hosts on port!=0 (Vasil Dimov) 1f096f0 net: distinguish default port per network (Vasil Dimov) aeac3bc net: change I2P seeds' ports to 0 (Vasil Dimov) 38f9002 net: change assumed I2P port to 0 (Vasil Dimov) Pull request description: _This is an alternative to bitcoin/bitcoin#21514, inspired by bitcoin/bitcoin#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 (bitcoin/bitcoin#21514 (comment), bitcoin/bitcoin#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. * Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening". Fixes #21389 ACKs for top commit: laanwj: Code review ACK 4101ec9 jonatack: 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. Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe

vasild · 2021-07-13T15:40:23Z

Closing in favor of #22112 (which was merged).

DrahtBot added P2P RPC/REST/ZMQ Utils/log/libs labels Mar 23, 2021

maflcko removed RPC/REST/ZMQ Utils/log/libs labels Mar 23, 2021

maflcko changed the title ~~Ignore ports on I2P addresses~~ p2p: Ignore ports on I2P addresses Mar 23, 2021

DrahtBot mentioned this pull request Mar 24, 2021

p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified #20018

Merged

vasild mentioned this pull request Mar 24, 2021

It is possible to open >1 connections to the same I2P node #21389

Closed

vasild force-pushed the ignore_port_in_i2p branch from 5c3eaea to f30e489 Compare March 30, 2021 10:37

jonatack reviewed Mar 31, 2021

View reviewed changes

vasild force-pushed the ignore_port_in_i2p branch from f30e489 to 5be0a46 Compare April 1, 2021 16:57

vasild force-pushed the ignore_port_in_i2p branch from 5be0a46 to 9ea6503 Compare April 9, 2021 08:40

DrahtBot mentioned this pull request May 24, 2021

rpc, test: addpeeraddress test coverage, code simplify/constness #22043

Merged

DrahtBot added the Needs rebase label May 25, 2021

vasild added 9 commits May 31, 2021 18:16

net: remove unused operator<(CService, CService)

2b8ab55

net: skip ports when generating string representations of some addresses

2edbe03

Since ports are irrelevant in our usage of I2P, print just `aaa.b32.i2p` instead of `aaa.b32.i2p:8333` when converting `CService` to string.

net: add a helper to remove ports from string addresses if irrelevant

5ab2683

net: possibly strip ports from addresses passed to -connect and -addnode

0335655

This makes printing to the log proper (without ports for I2P addresses).

rpc: possibly strip ports from arguments to addnode & addconnection RPCs

63b80cc

This makes printing to the log proper (without ports for I2P addresses) and also discovers duplicates earlier in `CConnman::AddNode()`.

net: CConnman::FindNode(): ignore ports for some addresses

75adc00

For networks that do not use ports (e.g. I2P), remove the `:port` suffix (if any) when searching for an address by string.

net: update outdated comment

b6b9819

This is a followup to `236618061a445d2cb11e722cfac5fdae5be26abb` which forgot to update the comment - we don't return the existent connection from `CConnman::ConnectNode()` anymore after that commit.

util: document SplitHostPort()

6d8d592

vasild force-pushed the ignore_port_in_i2p branch from 9ea6503 to 6d8d592 Compare May 31, 2021 16:19

vasild mentioned this pull request May 31, 2021

Force port 0 in I2P #22112

Merged

DrahtBot removed the Needs rebase label May 31, 2021

DrahtBot mentioned this pull request May 31, 2021

Validate port-options #22087

Merged

vasild closed this Jul 13, 2021

vasild deleted the ignore_port_in_i2p branch July 13, 2021 15:40

laanwj mentioned this pull request Jul 19, 2021

addrman: Remove addrman hotfixes #22496

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

p2p: Ignore ports on I2P addresses #21514

p2p: Ignore ports on I2P addresses #21514

Uh oh!

Conversation

vasild commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Problem

Solution

Uh oh!

DrahtBot commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Mar 25, 2021

Uh oh!

vasild commented Mar 30, 2021

Uh oh!

jonatack commented Mar 30, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Apr 1, 2021

Uh oh!

jonatack commented Apr 1, 2021

Uh oh!

sipa commented Apr 1, 2021

Uh oh!

vasild commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 2, 2021

Uh oh!

jonatack commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Apr 2, 2021

Uh oh!

sipa commented Apr 7, 2021

Uh oh!

vasild commented Apr 9, 2021

Uh oh!

vasild commented Apr 9, 2021

Uh oh!

vasild commented Apr 12, 2021

Uh oh!

naumenkogs commented Apr 16, 2021

Uh oh!

jonatack commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented May 19, 2021

Uh oh!

vasild commented May 31, 2021

Uh oh!

vasild commented May 31, 2021

Uh oh!

vasild commented Mar 23, 2021 •

edited

Loading

DrahtBot commented Mar 23, 2021 •

edited

Loading

jonatack Apr 7, 2021 •

edited

Loading

vasild commented Apr 2, 2021 •

edited

Loading

jonatack commented Apr 2, 2021 •

edited

Loading

jonatack commented May 5, 2021 •

edited

Loading