Force port 0 in I2P #22112

vasild commented at 4:29 PM on May 31, 2021: member

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 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, #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.
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

vasild commented at 4:34 PM on May 31, 2021: member

TODO: see if something has to be done about this assert:

https://github.com/bitcoin/bitcoin/blob/8462cd56010a8beadf95d4f3cb1357ca9d88493b/src/bench/addrman.cpp#L91

sipa commented at 4:36 PM on May 31, 2021: member

I think that's just a way to test that the returned address object isn't empty. Should probably replace it with another test.

DrahtBot added the label P2P on May 31, 2021

DrahtBot added the label Scripts and tools on May 31, 2021

DrahtBot added the label Validation on May 31, 2021

laanwj commented at 3:51 PM on June 2, 2021: member

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 i2pd and new SAM protocol, so seems better to me.

in src/net.cpp:2023 in 4401bb56cf outdated

2018 | @@ -2019,8 +2019,9 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
2019 |              // from advertising themselves as a service on another host and
2020 |              // port, causing a DoS attack as nodes around the network attempt
2021 |              // to connect to it fruitlessly.
2022 | -            if (addr.GetPort() != Params().GetDefaultPort() && nTries < 50)
2023 | +            if (!addr.IsI2P() && addr.GetPort() != Params().GetDefaultPort() && nTries < 50) {

sipa commented at 6:22 PM on June 2, 2021:

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?

vasild commented at 10:56 AM on June 3, 2021:

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.

jonatack commented at 3:12 PM on June 22, 2021:

It may be good to update the comment preceding this line to explain the exception for I2P.

vasild commented at 9:23 AM on June 23, 2021:

Added a comment.

jonatack commented at 10:01 AM on July 1, 2021:

From testing and offline discussion with @vasild, it seems a network-dependent GetDefaultPort() is indeed needed.

vasild commented at 1:23 PM on July 1, 2021:

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) {

vasild commented at 10:58 AM on June 3, 2021: member

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

Me too!

vasild commented at 12:00 PM on June 3, 2021: member

TODO: see if something has to be done about this assert:

https://github.com/bitcoin/bitcoin/blob/8462cd56010a8beadf95d4f3cb1357ca9d88493b/src/bench/addrman.cpp#L91

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:

https://github.com/bitcoin/bitcoin/blob/a9435e34457e0bfebd22e574fe63428537948aeb/src/bench/addrman.cpp#L35-L37

So it might as well generate I2P addresses with !=0 ports. Currently it generates only IPv6 addresses.

jonatack commented at 3:12 PM on June 22, 2021: member

Almost-ACK 4401bb56cf3822950b221c3fa47fb0b65312d12f 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).

vasild force-pushed on Jun 23, 2021

vasild commented at 9:22 AM on June 23, 2021: member

4401bb56cf...a9cacd29a1: add a test and a comment

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

Yes, that's really unfortunate and annoying. I wonder if it can be avoided...

jonatack commented at 9:23 AM on June 23, 2021: member

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.

jonatack commented at 9:35 AM on June 23, 2021: member

4401bb56cf...a9cacd29a1: add a test and a comment

Add the test to the test runner?

vasild force-pushed on Jun 23, 2021

vasild commented at 9:48 AM on June 23, 2021: member

a9cacd29a1...d6255891da: add the newly added test to test_runner.py (forgotten in the previous push)

Add the test to the test runner?

Yes :hear_no_evil:

vasild force-pushed on Jun 23, 2021

vasild commented at 10:29 AM on June 23, 2021: member

d6255891da...28f98950ee: rename the functional test to match the naming convention

jonatack commented at 10:29 AM on June 23, 2021: member

test_runner.py#check_script_prefixes() is unhappy about the name of the test file

edit: snap!

jonatack commented at 10:32 AM on June 23, 2021: member

ACK 28f98950ee89dafcb75b131b810f5a933c2977b3 modulo #22112 (comment)

jonatack commented at 2:46 PM on June 23, 2021: member

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

vasild commented at 10:20 AM on June 29, 2021: member

28f98950ee..0c6db5b87f: append a new commit that changes the ports of existent I2P addrman entries to 0.

vasild force-pushed on Jun 29, 2021

vasild commented at 11:48 AM on June 29, 2021: member

0c6db5b87f...b336f12eda: no need to lock CAddrMan::cs in the ResetI2PPorts() method, it is already locked by the caller and in master the mutex is not recursive anymore.

vasild commented at 1:09 PM on June 29, 2021: member

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 Params().GetDefaultPort() to PORT_SAM31 in that line).

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 addrman: reset I2P ports to 0 when loading from disk is somewhat optional, feel free to ACK the last but one commit. From the list in: #21389 (comment) this full PR implements option 3. Without the last commit it implements option 2.

in src/addrman.cpp:744 in b336f12eda outdated

 739 | +            if (vvTried[bucket_target][i_target] == -1) {
 740 | +                vvTried[bucket_target][i_target] = id;
 741 | +                vvTried[bucket][i] = -1;
 742 | +                addr_info = addr_info_newport;
 743 | +            } else {
 744 | +                vvTried[bucket][i] = -1;

jonatack commented at 3:44 PM on June 30, 2021:

b336f12 it looks like this line can be pulled out of the conditional

             // Reposition from (bucket, i) to (bucket_target, i_target) or delete if the target
             // position is occupied.
+            vvTried[bucket][i] = -1;
 
             if (vvTried[bucket_target][i_target] == -1) {
                 vvTried[bucket_target][i_target] = id;
-                vvTried[bucket][i] = -1;
                 addr_info = addr_info_newport;
             } else {
-                vvTried[bucket][i] = -1;

vasild commented at 12:27 PM on July 1, 2021:

Did that, but after that changed the code to remove the existent entry (if any), so the above is not relevant any more.

in src/addrman.cpp:745 in b336f12eda outdated

 740 | +                vvTried[bucket_target][i_target] = id;
 741 | +                vvTried[bucket][i] = -1;
 742 | +                addr_info = addr_info_newport;
 743 | +            } else {
 744 | +                vvTried[bucket][i] = -1;
 745 | +                SwapRandom(addr_info.nRandomPos, vRandom.size() - 1);

jonatack commented at 3:49 PM on June 30, 2021:

b336f12 would asserts like those seen in CAddrMan::Delete() be a good idea?

@@ -675,6 +675,7 @@ void CAddrMan::ResetI2PPorts()
             if (id == -1) {
                 continue;
             }
+            assert(mapInfo.count(id) != 0);
             auto& addr_info = mapInfo[id];
@@ -716,6 +717,7 @@ void CAddrMan::ResetI2PPorts()
             if (id == -1) {
                 continue;
             }
+            assert(mapInfo.count(id) != 0);
             auto& addr_info = mapInfo[id];
@@ -737,13 +739,13 @@ void CAddrMan::ResetI2PPorts()
             if (vvTried[bucket_target][i_target] == -1) {
                 vvTried[bucket_target][i_target] = id;
                 addr_info = addr_info_newport;
             } else {
+                assert(addr_info.nRefCount == 0);
                 SwapRandom(addr_info.nRandomPos, vRandom.size() - 1);

vasild commented at 12:29 PM on July 1, 2021:

I think asserts are too excessive - if something goes wrong with this code I don't think it is critical enough to stop the node (that's during startup). Instead of the first two asserts I added a check & early quit. The 3rd suggested assert is not relevant now because I changed those lines to remove the existent entry (not the I2P one).

jonatack commented at 2:53 PM on July 1, 2021:

FWIW the asserts passed for me in mainnet testing.

jonatack commented at 3:54 PM on June 30, 2021: member

Semi-ACK b336f12eda9ce3aeefc6d573c4068a27beef8def modulo the issues noted below in #22112 (comment).

A couple of suggestions and questions below, along with some proposed edits in https://github.com/jonatack/bitcoin/commit/539a28180b06fa99f81fbd01223ac30e7e4276af

Nice work on the new unit test!

DrahtBot commented at 4:11 PM on June 30, 2021: member

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

Conflicts

No conflicts as of last run.

jonatack commented at 7:58 PM on June 30, 2021: member

Tested this rebased to master, then restarted the node after backing up peers.dat:

Loaded 65986 addresses from peers.dat
The number of I2P addresses reported by -addrinfo and getnodeaddresses 0 i2p dropped from 15 to 5 (all have port 0 as expected)
I2P: SAM session created: my address=zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p:0
No outbound I2P connections (set using addnode without port number in the conf file), 4 inbound ones after a minute

Restarted again, no change:

Loaded 66102 addresses from peers.dat
5 I2P addresses known (port 0 as expected)
I2P: SAM session created: my address=zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p:0
No outbound I2P connections (as above), 4 inbound ones after a few minutes
Tried addpeeraddress to re-add the I2P addresses, it returns "success": true but the address isn't added.

The outbound I2P connections via addnode sans port number needs to be resolved.

It's unfortunate to lose I2P addresses due to the bucket repositioning (10 out of 15, in my case), though I suppose it's a minor inconvenience affecting only a small number of existing nodes running I2P.

Port 0 is default in SAM 3.2 and up per https://geti2p.net/en/docs/api (and portful routing starts from SAM 3.3 IIUC) but port 0 looks odd when all the other networks default to another value (8333, etc). Users may think it's a bug if not documented, e.g. in doc/i2p.md and the release notes.

vasild force-pushed on Jul 1, 2021

jonatack commented at 12:56 PM on July 1, 2021: member

$ test/lint/lint-circular-dependencies.sh
A new circular dependency in the form of "chainparams -> i2p -> chainparams" appears to have been introduced.

vasild commented at 12:58 PM on July 1, 2021: member

b336f12eda...e4684144e9:

If a collision occurs due to re-bucketing of the I2P address with the changed port to 0, then remove the other entry, not the I2P one. Looks like I2P addresses are more precious than a random one from an addrman filled mostly with IPv4 addresses.
If addnode=foo.b32.i2p is specified (without port) then we would do something like Lookup(str, GetDefaultPort()) and would end up with CService with address foo.b32.i2p and port 8333 which is not what we want (we want port 0) (thanks, @jonatack for discovering this!). So, implement @sipa's suggestion of network-dependent default port.
Took some suggestions.

vasild force-pushed on Jul 1, 2021

vasild commented at 1:25 PM on July 1, 2021: member

e4684144e9...36cf2ca392: define the default SAM 3.1 port in netaddress.h, which is included in both i2p.h and chainparams.h to avoid circular dependency i2p.h -> chainparams.h -> i2p.h

laanwj added this to the milestone 22.0 on Jul 1, 2021

laanwj commented at 2:26 PM on July 1, 2021: member

Added 22.0 milestone (@jonatack mentioned that one of the solutions to the I2P port problem needs to make it in, and I think that makes sense. From what I understand this PR is preferred.)

vasild force-pushed on Jul 2, 2021

vasild commented at 10:41 AM on July 2, 2021: member

36cf2ca392...ba6d18861b: rebase in order to pull doc/i2p.md and add a note to it for I2P ports being forced to 0.

jonatack commented at 10:28 AM on July 5, 2021: member

Re-reviewing and testing.

in src/chainparams.h:11 in ba6d18861b outdated

   7 | @@ -8,11 +8,13 @@
   8 |  
   9 |  #include <chainparamsbase.h>
  10 |  #include <consensus/params.h>
  11 | +#include <netaddress.h>

jonatack commented at 10:38 AM on July 5, 2021:

25fda81f include netaddress.h and string in cpp file too?

vasild commented at 4:53 PM on July 5, 2021:

Removed the changes to chainparams.cpp.

in src/chainparams.h:85 in ba6d18861b outdated

  81 | @@ -80,6 +82,8 @@ class CChainParams
  82 |      const Consensus::Params& GetConsensus() const { return consensus; }
  83 |      const CMessageHeader::MessageStartChars& MessageStart() const { return pchMessageStart; }
  84 |      uint16_t GetDefaultPort() const { return nDefaultPort; }
  85 | +    uint16_t GetDefaultPort(Network net) const;

jonatack commented at 10:42 AM on July 5, 2021:

25fda81f9a22dd8ed97a64081175c24c88abaad2 maybe just inline this one as it is only one line

vasild commented at 4:54 PM on July 5, 2021:

Done.

in src/chainparams.cpp:554 in ba6d18861b outdated

 549 | +{
 550 | +    CNetAddr a;
 551 | +    if (a.SetSpecial(addr)) {
 552 | +        return GetDefaultPort(a.GetNetwork());
 553 | +    }
 554 | +    return GetDefaultPort();

jonatack commented at 10:46 AM on July 5, 2021:

25fda81f9a22dd8ed97a64081175c24c88abaad2 one return might be simpler/clearer

return a.SetSpecial(addr) ? GetDefaultPort(a.GetNetwork() : GetDefaultPort();

vasild commented at 4:54 PM on July 5, 2021:

Done, and moved to .h

vasild force-pushed on Jul 5, 2021

vasild commented at 11:46 AM on July 5, 2021: member

ba6d18861b...14db11782c: minor rewording in doc/i2p.md

in src/addrman.cpp:746 in 14db11782c outdated

 709 | +            if (id == -1) {
 710 | +                continue;
 711 | +            }
 712 | +            auto it = mapInfo.find(id);
 713 | +            if (it == mapInfo.end()) {
 714 | +                return;

jonatack commented at 2:19 PM on July 5, 2021:

44889de Here and line 754 below, I'm not sure whether it's better to assert or silently return.

jonatack commented at 3:48 PM on July 5, 2021:

(Asserts run fine for me in my testing so far).

vasild commented at 4:40 PM on July 5, 2021:

Asserts look scary to me. If those have to be asserts then I would deem this too risky for 22.0. Maybe I am pessimistic but I imagine 22.0 nodes not being able to start because I overlooked something in the I2P ports fiddling which is definitely not critical for node operation :bomb: :skull_and_crossbones:

jonatack commented at 3:38 PM on July 5, 2021: member

Tested ACK 14db11782c467c2585c552ec1797ad3b3ef70ab3 modulo the questions below. Rebased to master, backed up peers.dat, debug built with -DEBUG_ADDRMAN and ran addrman/net/i2p units at each commit that changes code, tested by restarting several times with i2p/tor/addrman logging, all 14 I2P addresses preserved, quickly connected to 10-11 I2P peers (roughly half in and half manual out), -netinfo/-addrinfo/getnetworkinfo/getnodeaddresses/getaddednodeinfo return as expected with port 0, running with the following asserts -- still running a node with them

<details><summary>asserts</summary><p>

diff --git a/src/addrman.cpp b/src/addrman.cpp
index f2d54c0adb..db0e7c2c2c 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -710,9 +710,7 @@ void CAddrMan::ResetI2PPorts()
                 continue;
             }
             auto it = mapInfo.find(id);
-            if (it == mapInfo.end()) {
-                return;
-            }
+            assert(it != mapInfo.end());
             auto& addr_info = it->second;
             if (!addr_info.IsI2P() || addr_info.GetPort() == I2P_SAM31_PORT) {
                 continue;
@@ -737,6 +735,7 @@ void CAddrMan::ResetI2PPorts()
 
             // Reposition from i to i_target, removing the entry from i_target (if any).
             ClearNew(bucket, i_target);
+            assert(vvNew[bucket][i_target] == -1);
             vvNew[bucket][i_target] = id;
             vvNew[bucket][i] = -1;
             addr_info = addr_info_newport;
@@ -750,9 +749,7 @@ void CAddrMan::ResetI2PPorts()
                 continue;
             }
             auto it = mapInfo.find(id);
-            if (it == mapInfo.end()) {
-                return;
-            }
+            assert(it != mapInfo.end());
             auto& addr_info = it->second;
             if (!addr_info.IsI2P() || addr_info.GetPort() == I2P_SAM31_PORT) {
                 continue;
@@ -790,10 +787,13 @@ void CAddrMan::ResetI2PPorts()
                 vvNew[new_bucket][new_bucket_i] = old_target_id;
                 ++nNew;
             }
+            assert(vvTried[bucket_target][i_target] == -1);
 
             vvTried[bucket_target][i_target] = id;
             vvTried[bucket][i] = -1;
             addr_info = addr_info_newport;
+            assert(addr_info.fInTried == true);
         }
     }
 }

</p></details>

This is much improved over my last review+testing at b336f12 as the issues mentioned in #22112 (comment) have been resolved.

I'm not 100% sure about the tried/new management and it may be good for an addrman expert to look over the changes. From what I can tell I don't see any issues, but noodling around adding things like, say, ++nTried at the bottom of the last loop in CAddrMan::ResetI2PPorts(), didn't break anything either.

Questions, should any of these be updated?

the -port config help in src/init.cpp
GetDefaultPort() in CConnman::ThreadDNSAddressSeed()
GetListenPort()

TODO: test bootstrapping from hardcoded I2P seeds.

vasild commented at 4:30 PM on July 5, 2021: member

... noodling around adding things like, say, ++nTried at the bottom of the last loop in CAddrMan::ResetI2PPorts(), didn't break anything either.

This should be detected by CAddrMan::Check() (only enabled if DEBUG_ADDRMAN is defined):

https://github.com/bitcoin/bitcoin/blob/271155984574a5bba9619d8f6da9bb0606d93f8c/src/addrman.cpp#L438

Questions, should any of these be updated?
* the `-port` config help in `src/init.cpp`

Add something like "not relevant for I2P"?

* `GetDefaultPort()` in CConnman::ThreadDNSAddressSeed()

* `GetListenPort()`

I think those are ok.

vasild force-pushed on Jul 5, 2021

jonatack commented at 5:08 PM on July 5, 2021: member

... noodling around adding things like, say, ++nTried at the bottom of the last loop in CAddrMan::ResetI2PPorts(), didn't break anything either.

This should be detected by CAddrMan::Check() (only enabled if DEBUG_ADDRMAN is defined):

https://github.com/bitcoin/bitcoin/blob/271155984574a5bba9619d8f6da9bb0606d93f8c/src/addrman.cpp#L438

Yes, I'll reverify if CAddrMan::Check_() is actually running the way I've been configuring.

jonatack commented at 10:09 AM on July 6, 2021: member

re-ACK 0b0ee030ea6f2b6508e8660dffc40b40c6aa2644 per git diff 14db117 0b0ee03

the -port config help in src/init.cpp

Add something like "not relevant for I2P"?

sgtm

in src/addrman.cpp:740 in 0b0ee030ea outdated

 699 | @@ -699,3 +700,100 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
 700 |      }
 701 |      return bits;
 702 |  }
 703 | +
 704 | +void CAddrMan::ResetI2PPorts()
 705 | +{
 706 | +    for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) {
 707 | +        for (int i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
 708 | +            const auto id = vvNew[bucket][i];

jonatack commented at 10:29 AM on July 6, 2021:

f108193c6381b4d It looks like the loops can be accelerated with a faster iterator comparison and by avoiding integer conversions.

<details><summary>suggestion</summary><p>

diff --git a/src/addrman.cpp b/src/addrman.cpp
index f2d54c0adb..41375e8320 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -703,8 +703,8 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
 
 void CAddrMan::ResetI2PPorts()
 {
-    for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) {
-        for (int i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
+    for (size_t bucket = 0; bucket != ADDRMAN_NEW_BUCKET_COUNT; ++bucket) {
+        for (size_t i = 0; i != ADDRMAN_BUCKET_SIZE; ++i) {
             const auto id = vvNew[bucket][i];
             if (id == -1) {
                 continue;
@@ -728,7 +728,7 @@ void CAddrMan::ResetI2PPorts()
             // re-evaluate that decision, but even if we could, CAddrInfo::GetNewBucket() does not
             // use CAddrInfo::GetKey() so it would end up in the same bucket as before the port
             // change.
-            const auto i_target = addr_info_newport.GetBucketPosition(nKey, true, bucket);
+            const size_t i_target = addr_info_newport.GetBucketPosition(nKey, true, bucket);
 
             if (i_target == i) { // No need to re-position.
                 addr_info = addr_info_newport;
@@ -743,8 +743,8 @@ void CAddrMan::ResetI2PPorts()
         }
     }
 
-    for (int bucket = 0; bucket < ADDRMAN_TRIED_BUCKET_COUNT; ++bucket) {
-        for (int i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
+    for (size_t bucket = 0; bucket != ADDRMAN_TRIED_BUCKET_COUNT; ++bucket) {
+        for (size_t i = 0; i != ADDRMAN_BUCKET_SIZE; ++i) {
             const auto id = vvTried[bucket][i];
             if (id == -1) {
                 continue;
@@ -763,8 +763,8 @@ void CAddrMan::ResetI2PPorts()
             // bucket and a position within that bucket. So a re-bucketing may be necessary.
             addr_info_newport.port = I2P_SAM31_PORT;
 
-            const auto bucket_target = addr_info_newport.GetTriedBucket(nKey, m_asmap);
-            const auto i_target = addr_info_newport.GetBucketPosition(nKey, false, bucket_target);
+            const size_t bucket_target = addr_info_newport.GetTriedBucket(nKey, m_asmap);
+            const size_t i_target = addr_info_newport.GetBucketPosition(nKey, false, bucket_target);
 
             if (bucket_target == bucket && i_target == i) { // No need to re-position.
                 addr_info = addr_info_newport;

</p></details>

jonatack commented at 10:37 AM on July 6, 2021:

(This isn't a hotspot but is something that could also be considered in places that are.)

laanwj commented at 3:24 PM on July 8, 2021:

Will leave it up to @vasild but it seems to me that integer and iterator conversions seem an unlikely source of performance problems.

(This isn't a hotspot but is something that could also be considered in places that are.)

Sure, it might have impact on tight loops that loop over large memory areas.

vasild commented at 9:50 AM on July 9, 2021:

ADDRMAN_NEW_BUCKET_COUNT is of type int.

for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) {

how is changing bucket to size_t going to make it faster? (otoh, it could produce some warning about comparing size_t to int)

Current code:

int GetFoo();
auto x = GetFoo();

So x will be defined as type int. How will explicitly defining it as size_t make it faster? (otoh, assigning from int to size_t could cause some compiler to warn about it)

I tried this:

#define N (1 << 10)

void foo()
{
    int array[N];
    for (int i = 0; i < N; ++i) {
        printf("%d\n", array[i]);
    }
}

Changing i from int to size_t produces identical assembler output:

(gdb) disas foo
Dump of assembler code for function _Z3foov:
   0x0000000000202220 <+0>:	push   %rbp
   0x0000000000202221 <+1>:	mov    %rsp,%rbp
   0x0000000000202224 <+4>:	push   %rbx
   0x0000000000202225 <+5>:	sub    $0x1008,%rsp
   0x000000000020222c <+12>:	xor    %ebx,%ebx
   0x000000000020222e <+14>:	xchg   %ax,%ax
   0x0000000000202230 <+16>:	mov    -0x1010(%rbp,%rbx,4),%esi
   0x0000000000202237 <+23>:	mov    $0x200cc7,%edi
   0x000000000020223c <+28>:	xor    %eax,%eax
   0x000000000020223e <+30>:	call   0x202670 <printf@plt>
   0x0000000000202243 <+35>:	add    $0x1,%rbx
   0x0000000000202247 <+39>:	cmp    $0x400,%rbx
   0x000000000020224e <+46>:	jne    0x202230 <_Z3foov+16>
   0x0000000000202250 <+48>:	add    $0x1008,%rsp
   0x0000000000202257 <+55>:	pop    %rbx
   0x0000000000202258 <+56>:	pop    %rbp
   0x0000000000202259 <+57>:	ret    
End of assembler dump.

DrahtBot added the label Needs rebase on Jul 8, 2021

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.

38f900290c

net: change I2P seeds' ports to 0 aeac3bce3e

net: distinguish default port per network

Change `CChainParams::GetDefaultPort()` to return 0 if the network is
I2P.

1f096f091e

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

4f432bd738

test: ensure I2P ports are handled as expected 41cda9d075

addrman: reset I2P ports to 0 when loading from disk

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.

e0a2b390c1

doc: mention that we enforce port=0 in I2P

Co-authored-by: Jon Atack <jon@atack.com>

4101ec9d2e

vasild force-pushed on Jul 9, 2021

vasild commented at 9:21 AM on July 9, 2021: member

0b0ee030ea...4101ec9d2e: rebase due to conflicts, clarify -port= help wrt I2P.

DrahtBot removed the label Needs rebase on Jul 9, 2021

in src/init.cpp:445 in 4101ec9d2e

 441 | @@ -442,7 +442,7 @@ void SetupServerArgs(ArgsManager& argsman)
 442 |      argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 443 |      argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 444 |      argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 445 | -    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);
 446 | +    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);

jonatack commented at 2:36 PM on July 9, 2021:

$ ./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. 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);

jonatack commented at 2:45 PM on July 9, 2021: member

re-ACK 4101ec9d2e05a35c35f587a28f1feee6cebcc61b 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.)

jonatack commented at 3:46 PM on July 9, 2021: member

Shut down the node, deleted the new peers.dat, recopied my backed-up peers.dat (cp ~/.bitcoin/peers-bak.dat ~/.bitcoin/peers.dat) and restarted. Unlike bootstrapping with no peers.dat and addnode peers, all of the I2P addresses in getnodeaddresses 0 i2p have port 0 (and as above, getaddednodeinfo as well).

2021-07-09T15:39:34Z Loaded 68521 addresses from peers.dat  2688ms
2021-07-09T15:39:34Z Loaded 2 addresses from "anchors.dat"

        ipv4    ipv6   onion     i2p   total   block  manual
in         0       0       0       4       4
out        7       0       5       6      18       2       8
total      7       0       5      10      22

$ ./src/bitcoin-cli -rpcwait -addrinfo
{
  "addresses_known": {
    "ipv4": 50863,
    "ipv6": 7727,
    "torv2": 0,
    "torv3": 5557,
    "i2p": 16,
    "total": 64163
  }
}

jonatack commented at 8:53 AM on July 10, 2021: member

Repeated the bootstrapping steps in #22112#pullrequestreview-703116399 but with no addnode peers and with -dnsseed=0.

2021-07-10T08:44:03Z Missing or invalid file ~/.bitcoin/peers.dat
2021-07-10T08:44:04Z DNS seeding disabled
2021-07-10T08:44:05Z Adding fixed seeds as -dnsseed=0, -addnode is not provided and all -seednode(s) attempted

        ipv4    ipv6   onion     i2p   total   block
in         0       0       0       2       2
out        9       0       1       0      10       2
total      9       0       1       2      12

$ ./src/bitcoin-cli -rpcwait -addrinfo
{
  "addresses_known": {
    "ipv4": 6281,
    "ipv6": 1076,
    "torv2": 0,
    "torv3": 58,
    "i2p": 5,
    "total": 7420
  }
}
$ ./src/bitcoin-cli getaddednodeinfo
[
]

All addresses returned by getnodeaddresses 0 i2p have port 0.

jonatack commented at 9:11 AM on July 10, 2021: member

After a few minutes, one new getnodeaddresses 0 i2p entry from an inbound peer had port 8333. Restarted the node and that address now has port 0. Edit: a day later, getnodeaddresses 0 i2p returns two more (inbound) I2P peers with port 8333. Of course, these inbound peers aren't running this branch.

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.

Screenshot from 2021-07-10 11-05-40

Screenshot from 2021-07-10 18-23-01

Screenshot from 2021-07-10 21-04-17

vasild commented at 10:27 AM on July 12, 2021: member

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 can connect to a node running this PR two times as the fix is when initiating a connection (master does not have it). Here is how it looks like:

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
  }
]

jonatack commented at 11:01 AM on July 12, 2021: member

Yes, I came to the same conclusions.

FWIW tested the previous push and this one for 3 days each.

laanwj commented at 12:19 PM on July 13, 2021: member

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

That's pretty easy to forget. Let's open an issue for this once this is merged.

vasild commented at 12:34 PM on July 13, 2021: member

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.

jonatack commented at 12:39 PM on July 13, 2021: member

Yeah, I'd remind you ;)

Might be good to have this in rc1 to sanity test it with some other nodes running it.

laanwj commented at 12:52 PM on July 13, 2021: member

Code review ACK 4101ec9d2e05a35c35f587a28f1feee6cebcc61b Have also loosely tested it by running different versions of this on a node for the last weeks, during which I have noticed no problem with I2P connectivity.

laanwj merged this on Jul 13, 2021

laanwj closed this on Jul 13, 2021

sidhujag referenced this in commit bb66e5aae7 on Jul 14, 2021

vasild deleted the branch on Jul 16, 2021

fanquake referenced this in commit 5cf28d5203 on Aug 3, 2021

Fabcien referenced this in commit 10df6b1378 on Feb 22, 2022

gwillen referenced this in commit 78c3a94eb8 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022