p2p: Ignore ports on I2P addresses

vasild commented at 4:27 pm on March 23, 2021: member

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 https://github.com/bitcoin/bitcoin/issues/21389

DrahtBot added the label P2P on Mar 23, 2021

DrahtBot added the label RPC/REST/ZMQ on Mar 23, 2021

DrahtBot added the label Utils/log/libs on Mar 23, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Mar 23, 2021

MarcoFalke removed the label Utils/log/libs on Mar 23, 2021

MarcoFalke renamed this:
~~Ignore ports on I2P addresses~~
p2p: Ignore ports on I2P addresses
on Mar 23, 2021

DrahtBot commented at 9:57 pm on March 23, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22087 by amadeuszpawlik
#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 commented at 10:00 am on March 25, 2021: member

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 force-pushed on Mar 30, 2021

vasild commented at 10:44 am on March 30, 2021: member

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 commented at 1:36 pm on March 30, 2021: member

Thanks for working on this! Will review soon.

in src/netaddress.h:571 in f30e489c38 outdated

567+
568+        /**
569+         * Avoid silently, unintentionally falling back to comparing CService objects as
570+         * CNetAddr objects.
571+         */
572+        bool operator<(const CService&) = delete;

jonatack commented at 1:58 pm on March 31, 2021:

44cf17a maybe add a unit test assertion

vasild commented at 2:20 pm on April 1, 2021:

Trying to order CService objects wouldn’t compile.

in src/test/netbase_tests.cpp:138 in f30e489c38 outdated

130@@ -131,6 +131,11 @@ BOOST_AUTO_TEST_CASE(netbase_lookupnumeric)
131     BOOST_CHECK(TestParse("[fd6b:88c0:8724:1:2:3:4:5]", "[::]:0"));
132     // and that a one-off resolves correctly
133     BOOST_CHECK(TestParse("[fd6c:88c0:8724:1:2:3:4:5]", "[fd6c:88c0:8724:1:2:3:4:5]:65535"));
134+
135+    // I2P skips the port.
136+    const std::string i2p_addr{"ukeu3k5oycgaauneqgtnvselmt4yemvoilkln7jpvamvfx7dnkdq.b32.i2p"};
137+    BOOST_CHECK(TestParse(i2p_addr, i2p_addr));
138+    BOOST_CHECK(TestParse(i2p_addr + ":8333", i2p_addr));

jonatack commented at 2:20 pm on March 31, 2021:

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

jonatack commented at 3:07 pm on March 31, 2021:

While here, maybe sneak this minor fixup into the commit

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

in src/test/netbase_tests.cpp:602 in f30e489c38 outdated

573+{
574+    BOOST_CHECK(LookupNumeric("1.1.1.1:555") == LookupNumeric("1.1.1.1:555"));
575+    BOOST_CHECK(LookupNumeric("1.1.1.1:555") != LookupNumeric("1.1.1.1:666"));
576+    BOOST_CHECK(LookupNumeric("1.1.1.1:555") != LookupNumeric("2.2.2.2:555"));
577+
578+    const std::string i2p_addr{"ukeu3k5oycgaauneqgtnvselmt4yemvoilkln7jpvamvfx7dnkdq.b32.i2p"};

jonatack commented at 2:23 pm on March 31, 2021:

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

vasild commented at 2:36 pm on April 1, 2021:

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.

in src/test/netbase_tests.cpp:622 in f30e489c38 outdated

591+             "[1:2::3:4]:5678",
592+             "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion",
593+             "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion:1111",
594+             "ukeu3k5oycgaauneqgtnvselmt4yemvoilkln7jpvamvfx7dnkdq.b32.i2p",
595+         }) {
596+        BOOST_CHECK_EQUAL(RemovePortIfIrrelevant(addr), addr);

jonatack commented at 2:33 pm on March 31, 2021:

8f4bd325 can also test passing RemovePortIfIrrelevant() a vector of addresses

vasild commented at 4:41 pm on April 1, 2021:

Done.

in test/functional/feature_config_args.py:232 in f30e489c38 outdated

227+
228+        self.stop_node(0)
229+        with self.nodes[0].assert_debug_log(expected_msgs=[
230+                f"trying connection {i2p_addr} lastseen=",
231+        ]):
232+            self.start_node(0, extra_args=[f"-connect={i2p_addr_with_port}"])

jonatack commented at 2:46 pm on March 31, 2021:

6f19e34 suggested simplification

 0     def test_addnode_and_connect_i2p(self):
 1-        self.log.info('Test that ports are removed from -addnode= and -connect= I2P addresses')
 2-
 3+        self.log.info("Test ports are removed from I2P addresses passed to -addnode/-connect")
 4         i2p_addr = "ukeu3k5oycgaauneqgtnvselmt4yemvoilkln7jpvamvfx7dnkdq.b32.i2p"
 5         i2p_addr_with_port = i2p_addr + ":3333"
 6-
 7-        self.stop_node(0)
 8-        with self.nodes[0].assert_debug_log(expected_msgs=[
 9-                f"trying connection {i2p_addr} lastseen=",
10-        ]):
11-            self.start_node(0, extra_args=[f"-addnode={i2p_addr_with_port}"])
12-
13-        self.stop_node(0)
14-        with self.nodes[0].assert_debug_log(expected_msgs=[
15-                f"trying connection {i2p_addr} lastseen=",
16-        ]):
17-            self.start_node(0, extra_args=[f"-connect={i2p_addr_with_port}"])
18+        msg = [f"trying connection {i2p_addr} lastseen="]
19+        for option in {"addnode", "connect"}:
20+            with self.nodes[0].assert_debug_log(expected_msgs=msg):
21+                self.restart_node(0, extra_args=[f"-{option}={i2p_addr_with_port}"])
22 
23     def run_test(self):

vasild commented at 4:56 pm on April 1, 2021:

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.

jonatack commented at 5:53 pm on April 1, 2021:

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:

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

or by adding, for instance

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

then the log would show

 02021-04-01T17:42:16.846000Z TestFramework (INFO): Test ports are removed from I2P addresses passed to -addnode/-connect
 12021-04-01T17:42:16.846000Z TestFramework (DEBUG): -connect
 22021-04-01T17:42:16.846000Z TestFramework.node0 (DEBUG): Stopping node
 32021-04-01T17:42:16.999000Z TestFramework.node0 (DEBUG): Node stopped
 42021-04-01T17:42:17.002000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
 52021-04-01T17:42:17.512000Z TestFramework.node0 (DEBUG): RPC successfully started
 62021-04-01T17:42:17.514000Z TestFramework (DEBUG): -addnode
 72021-04-01T17:42:17.514000Z TestFramework.node0 (DEBUG): Stopping node
 82021-04-01T17:42:17.618000Z TestFramework.node0 (DEBUG): Node stopped
 92021-04-01T17:42:17.628000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
102021-04-01T17:42:18.140000Z TestFramework.node0 (DEBUG): RPC successfully started

vasild commented at 4:22 pm on April 7, 2021:

…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).

jonatack commented at 5:01 pm on April 7, 2021:

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.

jonatack commented at 3:01 pm on March 31, 2021: member

ACK f30e489c3882cd58e382c203e5e4722fbe70f58e with some suggestions

vasild commented at 4:57 pm on April 1, 2021: member

f30e489c3...5be0a46b1: address suggestions

vasild force-pushed on Apr 1, 2021

jonatack commented at 5:54 pm on April 1, 2021: member

re-ACK 5be0a46b1623536d98aec641af0d24401995e837

sipa commented at 6:41 pm on April 1, 2021: member

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

vasild commented at 3:53 pm on April 2, 2021: member

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 commented at 4:25 pm on April 2, 2021: member

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 commented at 4:36 pm on April 2, 2021: member

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 commented at 5:34 pm on April 2, 2021: member

@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.

 0# Listener 3.2 socket1
 1HELLO VERSION MIN=3.2 MAX=3.2
 2SESSION 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
 3
 4# Listener 3.2 socket2
 5HELLO VERSION MIN=3.2 MAX=3.2
 6STREAM ACCEPT ID=newlistenersession SILENT=false
 7
 8# Listener 3.1 socket1
 9HELLO VERSION MIN=3.1 MAX=3.1
10SESSION 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==
11
12# Listener 3.1 socket2
13HELLO VERSION MIN=3.1 MAX=3.1
14STREAM ACCEPT ID=oldlistenersession SILENT=false
15
16# Client 3.2 socket1
17HELLO VERSION MIN=3.2 MAX=3.2
18SESSION 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==
19
20# Client 3.2 socket2
21HELLO VERSION MIN=3.2 MAX=3.2
22STREAM 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
23
24# Client 3.1 socket1
25HELLO VERSION MIN=3.1 MAX=3.1
26SESSION 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==
27
28# Client 3.1 socket2
29HELLO VERSION MIN=3.1 MAX=3.1
30STREAM 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 commented at 4:25 pm on April 7, 2021: member

@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 force-pushed on Apr 9, 2021

vasild commented at 8:44 am on April 9, 2021: member

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 commented at 9:48 am on April 9, 2021: member

@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 commented at 3:07 pm on April 12, 2021: member

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 commented at 10:03 am on April 16, 2021: member

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

jonatack commented at 3:50 pm on May 5, 2021: member

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 commented at 2:44 pm on May 19, 2021: member

Would be good to have this fixed for v22.

DrahtBot added the label Needs rebase on May 25, 2021

net: remove unused operator<(CService, CService) 2b8ab5584f

net: ignore ports when comparing some addresses (I2P)

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 https://github.com/bitcoin/bitcoin/issues/21389

71a294d77d

net: skip ports when generating string representations of some addresses

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.

2edbe032cf

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

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

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

03356556da

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

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

63b80cc5d2

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

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

75adc00b8b

net: update outdated comment

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.

b6b98195b2

util: document SplitHostPort() 6d8d59284b

vasild force-pushed on May 31, 2021

vasild commented at 4:19 pm on May 31, 2021: member

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

vasild commented at 4:32 pm on May 31, 2021: member

@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 :)

DrahtBot removed the label Needs rebase on May 31, 2021

laanwj referenced this in commit d8f1e1327f on Jul 13, 2021

vasild commented at 3:40 pm on July 13, 2021: member

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

vasild closed this on Jul 13, 2021

vasild deleted the branch on Jul 13, 2021

DrahtBot locked this on Aug 16, 2022

p2p: Ignore ports on I2P addresses #21514

Background

Problem

Solution

Conflicts