I2P network optimizations

[vasild]

• Reuse an I2P transient session instead of discarding it if we failed to connect to the desired peer. This means we never used the generated address (destination), whose creation is not cheap. This does not mean that we will use the same address for more than one peer.
• Lower the number of tunnels for transient sessions.
• Explicitly specify the number of tunnels for persistent sessions instead of relying on the defaults which differ between I2P routers. This way we get consistent behavior with all routers.

Alleviates: #26754

(I have not tested this with i2pd, yet)

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack, mzumsande
Concept ACK	1440000bytes, zzzi2p, sipa
Approach ACK	w0xlt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
• #26441 (rpc, p2p: add addpermissionflags RPC and allow whitelisting outbound by brunoerg)
• #25515 (net, test: Virtualise CConnman and add initial PeerManager unit tests by dergoegge)
• #24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
• #24545 (BIP324: Enable v2 P2P encrypted transport 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.

[vasild]

If it is desired to rate limit the creation of new sessions, we should (for i2pacceptincoming=0 aka transient sessions):

• keep track of the rate at which we create new sessions
• when closing a connection, if the rate is too high, instead of discarding the session put it back to a pool of reused sessions
• when creating new session, if the rate is too high and if the reused sessions pool is non-empty, then pick one from there

Reading #26754 I get the impression that this is not necessary. Further, it would be a compromise with "one address per connection" and would be somewhat more complicated (to implement and maintain).

[jonatack]

Thanks @vasild, will have a look and test with i2pd.

[ghost]

Concept ACK

[vasild]

With i2pacceptincoming=0 onlynet=i2p and this PR (57fd27f):

• In the first 5 minutes 10 sessions were created (= one session creation per 30 seconds). During that time 19 connection failures occurred, whose sessions were reused for subsequent attempts due to this PR. Without it, it would be 29 (10+19) instead of 10.
• In the next 18 hours 264 sessions were created (= one session creation per 4 minutes). During that time 234 connection failures occurred.

Without onlynet=i2p the numbers would be much lower since other networks would also be tried.

@zzzi2p, does that look reasonable?

[zzzi2p]

ACK, thank you very much.

This helps my thesis that it was mainly a startup problem that would drive the router into congestion. The key is you've gone from 290 tunnels at startup down to 22, which is a massive massive improvement.

A couple notes:

• You should monitor/test the reliability of connections over transient tunnels with *.quantity=1. If it doesn't meet your requirements, retest with *.quantity=2 to see if that improves things enough to be worth the additional resource usage.
• You should monitor/test the performance of non-transient tunnels with *.quantity=3. If it doesn't meet your requirements, try 4 or 5 to see if that improves things enough to be worth the additional resource usage.
• Agreed you want to avoid a pool with lots of unused sessions for a long time, either by design or some check. Also maybe close all idle sessions once you get to 10 good ones? Up to you, you're on the right track here.
• Try to get an ack from i2pd's @orignal

[orignal]

Looks good

[w0xlt]

Approach ACK

[jonatack]

Testing ATM with i2pd 2.45.0 now 2.45.0_1.

Without onlynet=i2p the numbers would be much lower since other networks would also be tried.

We discourage using only I2P enough in doc/i2p.md and this article (and Tor is so popular with node operators) that I wonder if anyone is running bitcoind only over I2P. Two that did reported it took 1-2 months for IBD.

When running with all of the networks (clearnet/tor/i2p/cjdns), I rarely see more than 1 outbound I2P connection.

For instance, spun up a node running -i2pacceptincoming=0 and only one single transient destination was created in the first hour.

---

Update: now running a node with i2p listening off and onlynet=tor/i2p/cjdns without clearnet and seeing ~3 transient destinations created/hour with grep "Creating transient SAM session" ~/.bitcoin/debug.log.

[sipa]

Concept ACK

[mzumsande]

Concept ACK
Not an i2p expert, so these questions are more to improve my own understanding:

Why exactly is session creation not cheap? I guess it's cheap locally, but the expensive part is that the new destination needs to be propagated through the network, others will probe for it etc.?

Does the reduction to inbound.quantity=1 outbound.quantity=1 of this PR affect our ability to help the i2p network by routing other non-bitcoin traffic? Or would that got through seperate tunnels?

[orignal]

Why exactly is session creation not cheap?

Every session creates few tunnels. Tunnels creation is not cheap, because asymmetric crypto and KDF for each hop.

[jonatack]

Why exactly is session creation not cheap?

Per the OP in #26754: ...some limit on the number of addresses (aka destinations or tunnels) built by the bitcoin client is necessary. I2P Destinations are designed to be long-lived, it's not like in Tor where you can create tons of circuits. Waiting for tunnels to be built for each connection also adds a huge amount of delay to connection setup time. A high number of addresses will also result in excessive CPU and bandwidth usage by your router to build the tunnels for each. Additionally, other routers will reject your excessive tunnel building, which will increase your resource usage even more as your router retries. And, of course, the overhead applies to every connection attempt, not just every successful connection.

Locally, I'm seeing between 45-100 seconds for a successful transient I2P connection to be established, based on the "Creating transient SAM session xyz" and "Transient SAM session xyz created" logging in src/i2p.cpp.

[zzzi2p]

@mzumsande re: why not cheap, in addition to what orignal said:

• a tunnel is a bandwidth commitment by 3 peers, who have finite capacity, for 10 minutes
• the asymmetric crypto is both locally and at each peer
• the tunnel build request and response themselves require bandwidth
• if any peer rejects or drops the request, esp. in times of congestion, the whole build process begins again
• each peer doesn't "know" if the other peers agreed, and if it agrees must allocate capacity for the tunnel regardless

you can see how careful congestion strategies in the router are required to prevent congestion collapse, where failed tunnel builds lead to more failed builds

"local" or "client" tunnels created by SAM are different from "transit" or "participating" tunnels created by others, and for the most part, one won't affect the other, until we approach bandwidth or CPU limits.

[vasild]

You should monitor/test the reliability of connections over transient tunnels with *.quantity=1

I ran two nodes for about 20 hours each - one with *.quantity=1 and one with *.quantity=3. I recorded the time when each I2P session was created and its duration (time between Creating transient SAM session X and Destroying SAM session X). Running with i2pacceptincoming=0 onlynet=i2p. Here are the results:

It looks ok - the points in the "1 tunnel" case are not visibly lower than in the "3 tunnel" case. I guess if connections are dropped frequently and unexpectedly due to low tunnel count, then the durations would be lower.

[vasild]

57fd27fd36...3c1de032de: address suggestions

[jonatack]

ACK 3c1de03

I ran two nodes for about 20 hours each - one with *.quantity=1 and one with *.quantity=3.
It looks ok - the points in the "1 tunnel" case are not visibly lower than in the "3 tunnel" case.

Nice! Were these two nodes running at the same time (to maybe reflect the same network conditions)?

[jonatack]

Testing with i2pd 2.45.0_1 (edit: 2.45.1 was just released; I upgraded, and I think confirm that I'm still seeing the i2pd issue). Will try to measure bandwidth before/after this change as described in #26838 (comment).

[vasild]

Were these two nodes running at the same time

Yes, and also on the same (java) router.

scripts to reproduce session duration monitoring

#!/usr/local/bin/bash

# Dump shell script to parse `debug.log` and produce an output like "session_start_seconds_since_startup session_duration" per line. Use like: ./parse.sh debug.log > durations3

log=${1}

b=""

for sess in $(sed -En 's/^.*Creating transient SAM session ([^ ]+) .*$/\1/p' < $log) ; do

    beg=$(sed -En "s/^([^ ]+).*Creating transient SAM session ${sess}.*$/\1/p" < $log)
    end=$(sed -En "s/^([^ ]+).*Destroying SAM session ${sess}.*$/\1/p" < $log)
    if [ -z "$end" ] ; then
        continue
    fi
    # convert a timestamp like 2023-01-09T16:28:40Z to number of seconds since epoch, this only works on freebsd, linux's data(1) does not accept -f IIRC
    beg_s=$(date -j -f "%Y-%m-%dT%TZ" $beg +%s)
    if [ -z "$b" ] ; then
        b=$beg_s
    fi
    end_s=$(date -j -f "%Y-%m-%dT%TZ" $end +%s)
    echo "$((beg_s - b)) $((end_s - beg_s))"
done

# gnuplot script to visualize the result from the above parsing script. Use like: gnuplot this_file.gnuplot
set terminal svg size 600,400 dynamic mouse standalone background "#ffffff"
set output "3tunnel_timeline.svg"
set title "Session duration - 3 tunnels"
set xlabel "Session creation time [minutes since startup]"
set ylabel "Session duration [minutes]"
set logscale y
set yrange [0:2000]
plot "durations3" using ($1/60):($2/60) title "" with points pointtype 7 pointsize 1 linecolor rgb "#ff0000"

[jonatack]

Thanks @vasild.

I'd be curious if anyone testing this branch sees this added logging return a value > 0 (it's only been 0 for me so far with -i2pacceptincoming=0, -onlynet=i2p and several addnoded peers to see 10-14 i2p peers, thus m_unused_i2p_sessions is either empty or contains one session).

+++ b/src/net.cpp
@@ -500,11 +500,13 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
                     LOCK(m_unused_i2p_sessions_mutex);
                     if (m_unused_i2p_sessions.empty()) {
+                        LogPrintf("m_unused_i2p_sessions size: 0\n");
                         i2p_transient_session =
                             std::make_unique<i2p::sam::Session>(proxy.proxy, &interruptNet);
                     } else {
                         i2p_transient_session.swap(m_unused_i2p_sessions.front());
                         m_unused_i2p_sessions.pop();
+                        LogPrintf("m_unused_i2p_sessions size after swap+pop: %i\n", m_unused_i2p_sessions.size());

[zzzi2p]

More fixes on the i2p/i2pd side: Neither project was stopping tunnel building if the client timed out and closed the control socket waiting for SESSION STATUS. Fixed in both projects.

i2pd was waiting a minimum of 20 sec before responding with SESSION_STATUS. Reduced to 3 sec.

Java i2p changes: i2p/i2p.i2p@841e277
i2pd changes: PurpleI2P/i2pd@7146a4d

[vasild]

I'd be curious if anyone testing this branch sees this added logging return a value > 0

This comment #26837 (comment):

If run by N threads, then the size can be at most N, if all of the threads race into it.

Let me elaborate. This is in ConnectNode() which is only called from OpenNetworkConnection() which can be called from more than one thread. If this code is run from just one thread then the size of the list can be either 0 or 1 and your logs will always print 0. If run by N threads it may happen that all of them execute this at the same time - check that the list is empty and create a new session, and then if all those N connections fail, then each thread will append its session to the list, making it with size N. It is ok, we run just a couple of threads (not 100s or 1000s, not even 10s).

You could get a printout >0 - maybe if you addnode ... onetry at the same time as a normal connection is attempted from the b-opencon thread. I guess it would be very rare and difficult to reproduce.

[jonatack]

Thanks @vasild, that confirms what I was thinking.

Java i2p changes: i2p/i2p.i2p@841e277 i2pd changes: PurpleI2P/i2pd@7146a4d

Happy to test i2pd as soon as the new release is out. I'm still seeing the issue I reported with i2pd version 2.45.1 (0.9.57).

[fanquake]

What are the next steps here? Does this just need more sanity checking from anyone who actually runs with i2p? I assume whatever upstream releases we may/may not have been waiting for have happened by now?

[vasild]

Like usual, after this is deemed to have had enough review and testing it can be merged. It is not tied to or dependent on I2P releases.

[jonatack]

FWIW, I2P adoption by Bitcoin nodes seems to have been increasing significantly over the past 3 months. From 200-350 in November to over 1100 active peers now, possibly spurred in part by Umbrel and Raspiblitz adding I2P support in December.

Anyone else like to have a look at testing/reviewing this PR?

[mzumsande]

Light ACK 3c1de03

I ran a -onlynet=i2p node with this branch for a while, with both options for -i2pacceptincoming and didn't encounter any problems - also reviewed the code and checked that the changes make sense in light of the available i2p documentation and its recommendations.

However, I'm not an i2p expert so I probably wouldn't have spotted any more sophisticated problems if there were any.

In light of #26754 (comment) it seems that backport may not be that urgent anymore.

[fanquake]

In light of #26754 (comment) it seems that backport may not be that urgent anymore.

Yea, that does seem the case (no urgency), however I think pulling these into 24.x is still worthwhile. I'll add the changes here to #26878 shortly.

[jonatack]

I'm still seeing the issue I reported with i2pd version 2.45.1 (0.9.57).

No longer seeing the previously daily issue since a few days with i2pd version 2.46.0 and 2.46.1.

[fanquake]

Added to #26878 for backporting to 24.x.