-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
I2P network optimizations #26837
I2P network optimizations #26837
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
If it is desired to rate limit the creation of new sessions, we should (for
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). |
Thanks @vasild, will have a look and test with i2pd. |
Concept ACK |
With
Without @zzzi2p, does that look reasonable? |
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:
|
Looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK
Testing ATM with i2pd
We discourage using only I2P enough in 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 Update: now running a node with i2p listening off and onlynet=tor/i2p/cjdns without clearnet and seeing ~3 transient destinations created/hour with |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Every session creates few tunnels. Tunnels creation is not cheap, because asymmetric crypto and KDF for each hop. |
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 |
@mzumsande re: why not cheap, in addition to what orignal said:
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. |
I ran two nodes for about 20 hours each - one with 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. |
In the case of `i2pacceptincoming=0` we use transient addresses (destinations) for ourselves for each outbound connection. It may happen that we * create the session (and thus our address/destination too) * fail to connect to the particular peer (e.g. if they are offline) * dispose the unused session. This puts unnecessary load on the I2P network because session creation is not cheap. Is exaggerated if `onlynet=i2p` is used in which case we will be trying to connect to I2P peers more often. To help with this, save the created but unused sessions and pick them later instead of creating new ones. Alleviates: bitcoin#26754
This will lower the load on the I2P network. Since we use one transient session for connecting to just one peer, a higher number of tunnels is unnecessary. This was suggested in: bitcoin#26754 (comment) bitcoin#26754 (comment) The options are documented in: https://geti2p.net/en/docs/protocol/i2cp#options A tunnel is unidirectional, so even if we make a single outbound connection we still need an inbound tunnel to receive the messages sent to us over that connection. Alleviates: bitcoin#26754
The default number of tunnels in the Java implementation is 2 and in the C++ i2pd it is 5. Pick a mid-number (3) and explicitly set it in order to get a consistent behavior with both routers. Do this for persistent sessions which are created once at startup and can be used to open up to ~10 outbound connections and can accept up to ~125 incoming connections. Transient sessions already set number of tunnels to 1. Suggested in: bitcoin#26754 (comment) https://geti2p.net/en/docs/api/samv3 Alleviates: bitcoin#26754
57fd27f
to
3c1de03
Compare
|
ACK 3c1de03
Nice! Were these two nodes running at the same time (to maybe reflect the same network conditions)? |
Testing with i2pd 2.45.0_1 (edit: 2.45.1 was just released; I upgraded, and I |
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" |
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 +++ 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()); |
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 |
This comment #26837 (comment):
Let me elaborate. This is in You could get a printout >0 - maybe if you |
Thanks @vasild, that confirms what I was thinking.
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). |
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? |
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. |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Yea, that does seem the case (no urgency), however I think pulling these into |
No longer seeing the previously daily issue since a few days with i2pd version 2.46.0 and 2.46.1. |
Added to #26878 for backporting to 24.x. |
In the case of `i2pacceptincoming=0` we use transient addresses (destinations) for ourselves for each outbound connection. It may happen that we * create the session (and thus our address/destination too) * fail to connect to the particular peer (e.g. if they are offline) * dispose the unused session. This puts unnecessary load on the I2P network because session creation is not cheap. Is exaggerated if `onlynet=i2p` is used in which case we will be trying to connect to I2P peers more often. To help with this, save the created but unused sessions and pick them later instead of creating new ones. Alleviates: bitcoin#26754 Github-Pull: bitcoin#26837 Rebased-From: b906b64
This will lower the load on the I2P network. Since we use one transient session for connecting to just one peer, a higher number of tunnels is unnecessary. This was suggested in: bitcoin#26754 (comment) bitcoin#26754 (comment) The options are documented in: https://geti2p.net/en/docs/protocol/i2cp#options A tunnel is unidirectional, so even if we make a single outbound connection we still need an inbound tunnel to receive the messages sent to us over that connection. Alleviates: bitcoin#26754 Github-Pull: bitcoin#26837 Rebased-From: 801b405
The default number of tunnels in the Java implementation is 2 and in the C++ i2pd it is 5. Pick a mid-number (3) and explicitly set it in order to get a consistent behavior with both routers. Do this for persistent sessions which are created once at startup and can be used to open up to ~10 outbound connections and can accept up to ~125 incoming connections. Transient sessions already set number of tunnels to 1. Suggested in: bitcoin#26754 (comment) https://geti2p.net/en/docs/api/samv3 Alleviates: bitcoin#26754 Github-Pull: bitcoin#26837 Rebased-From: 3c1de03
784a754 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow) debcfe3 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow) ccc72fe wallet: Be able to unlock the wallet for migration (Andrew Chow) 50dd8b1 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow) 648b062 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow) ab3bd45 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov) 29cdf42 i2p: lower the number of tunnels for transient sessions (Vasil Dimov) 5027e93 i2p: reuse created I2P sessions if not used (Vasil Dimov) a62c541 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) 64e7db6 Zero out wallet master key upon lock (John Moffett) b7e242e Correctly limit overview transaction list (John Moffett) cff6718 depends: fix systemtap download URL (fanquake) 7cf73df Add missing includes to fix gcc-13 compile error (MarcoFalke) 07397cd addrdb: Only call Serialize() once (Martin Zumsande) 91f83db hash: add HashedSourceWriter (Martin Zumsande) 5c824ac For feebump, ignore abandoned descendant spends (John Moffett) 428dcd5 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow) cbcdafa test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner) 342abfb wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner) Pull request description: Backports: * #26595 * #26675 * #26679 * #26761 * #26837 * #26909 * #26924 * #26944 * bitcoin-core/gui#704 * #27053 * #27080 ACKs for top commit: instagibbs: ACK 784a754 achow101: ACK 784a754 hebasto: ACK 784a754, I've made backporting locally and got a diff between my branch and this PR as follows: Tree-SHA512: 8ea84aa02d7907ff1e202e1302b441ce9ed2198bf383620ad40056a5d7e8ea88e1047abef0b92d85648016bf9b3195c974be3806ccebd85bef4f85c326869e43
Alleviates: #26754
(I have not tested this with i2pd, yet)