i2p: make a time gap between creating transient sessions and using them #32065
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32065. 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. |
|
Thanks for CC'ing me. I also asked eyedeekay and orignal from i2pd to take a look. 10 standby sessions is a lot, except maybe at startup. If your I2P connections limit is 10, then you're doubling your network resource usage. 2 or 3 is probably enough, depending on how fast your connections churn and how long is long enough for a unused session to be up to give you the anonymity you're looking for. A couple of alternatives: int max = MAX_UNUSED_I2P_SESSIONS_SIZE;
if (uptime() > 15 minutes) max /= 3;or int max = min(MAX_UNUSED_I2P_SESSIONS_SIZE, 3 + MAX_I2P_SESSIONS - current_i2p_sessions.size());However, I'm also concerned about starting a whole bunch of sessions at once, especially at startup, and if the user started his router when he started bitcoin. If this change will double or triple or more the session-creation rate at startup, that's probably too fast. The Java router has some t 8000 unnel build rate-limiting that may kick in. I won't attempt any pseudocode for it, but some rate limiting in AddI2PSessionsIfNeeded() or some other special case for the first few minutes after startup is probably desirable. Concept ACK. |
|
Concept ACK |
|
I agree with zzz, the concept ACK is fine but I'm very concerned about having the session bank be 10 sessions long. I think the best method of managing the session bank sort of depends on the rate at which you'll need them. If you're creating sessions fast enough to need a bank of 10 sessions, then I think at some point you have no choice but to keep the sessions alive longer and re-use them more. On the other hand, if you aren't creating them fast enough to need a bank of 10 sessions, then keeping a bank of 10 of them is pretty wasteful. How you solve it is going to depend on the actual rates you expect people to need sessions. If the need for sessions is too unpredictable, then maybe the bank size limit needs to change size based on need from a very small number to a larger number and back. I'm going to take a closer look at the code here and try to make some more concrete suggestions. |
42f98c9 to
a22a3b3
Compare
|
A few notes:
Below is some data for when sessions are created, used and how long do they live. From startup to about 10 minutes after that, with this PR. It is just an example from my node. I guess timings will vary a lot between runs and in different environments.
Given the above, I changed |
There was a problem hiding this comment.
Two thoughts:
-
As far as I understand it (https://geti2p.net/en/about/performance/future), i2p tunnels last for 10 minutes by default. If we don't use these extra sessions before they expire, their creation would be a waste of ressources. I would image this to be particularly relevant in a scenario where i2p is used together with other networks. When we have no need for new regular outbounds because we are full and none of our peers disconnect, we make feeler connections every 2 minutes and extra block relay conns every 5 minutes - that's like 7 new connections every 10 minutes that would be divided over all reachable networks. Would be interesting to run with i2p and clearnet, and see how many of these tunnels expire before they are used.
-
When using
->, this may not have much of an effect right after startup -m_unused_i2p_sessionswill only grow if a connection attempt fails, for successful connections the newly formed connections will be used right away. Maybe it could make a sense to fill them_unused_i2p_sessionsbuffer with 4 connections right after startup?
|
I wonder what happens after the 10 minutes? For example, if a client does Thinking about this more, given that opening of new connections is done serially, one after another (not in parallel), and occasionally (except during startup) maybe it would be best to pre-create just one session? |
|
Tunnels get built when the SAM session is opened, and are automatically rebuilt when they expire, the router does that for you as long as the SAM session stays open. A session is just a pool of inbound and outbound tunnels. I assume that's what you are trying to do here, have the session open and ready before it's needed. |
a22a3b3 to
fccedfd
Compare
fccedfd to
ef525e8
Compare
|
|
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
48a636c to
859c259
Compare
|
|
Connecting to an I2P peer consists of creating a session (the `SESSION CREATE` command) and then connecting to the peer using that session (`STREAM CONNECT ID=session_id ...`). This change is only relevant for transient sessions because when a persistent session is used it is created once and used for all connections. Before this change Bitcoin Core would create the session and use it in quick succession. That is, the `SESSION CREATE` command would be immediately followed by `STREAM CONNECT`. This could ease network activity monitoring by an adversary. To mitigate that, this change creates a transient session upfront without an immediate demand for new sessions and later uses it. This creates a time gap between `SESSION CREATE` and `STREAM CONNECT`. Note that there is always some demand for new I2P connections due to disconnects. --- Summary of the changes in the code: * Create the session from the `Session` constructor (send `SESSION CREATE` to the I2P SAM proxy). This constructor was only called when transient sessions were needed and was immediately followed by `Connect()` which would have created the session. So this is a noop change if viewed in isolation. * Change `CConnman::m_unused_i2p_sessions` from a queue to a single entity. Given that normally `CConnman::ConnectNode()` is not executed concurrently by multiple threads, the queue could have had either 0 or 1 entry. Simplify the code by replacing the queue with a single session. * Every time we try to connect to any peer (not just I2P) pre-create a new spare I2P session. This way session creation is decoupled from the time when the session will be used (`STREAM CONNECT`).
859c259 to
632eb1a
Compare
|
|
|
Somehow I hadn't seen this PR yet. If I understand correctly, this change is only for if a node is configured with |
|
Yes. That is because if it is accepting incoming connections (has a permanent listening I2P address) then there is just one I2P session which is associated with that address and that session is used for all incoming and outgoing connections. |
Connecting to an I2P peer consists of creating a session (the
SESSION CREATEcommand) and then connecting to the peer using thatsession (
STREAM CONNECT ID=session_id ...).This change is only relevant for transient sessions because when a
persistent session is used it is created once and used for all
connections.
Before this change Bitcoin Core would create the session and use it in
quick succession. That is, the
SESSION CREATEcommand would beimmediately followed by
STREAM CONNECT. This could ease networkactivity monitoring by an adversary.
To mitigate that, this change creates sessions upfront without an
immediate demand for new sessions and later uses them. This creates a
time gap between
SESSION CREATEandSTREAM CONNECT. Note that thereis always some demand for new I2P connections due to disconnects.
Summary of the changes in the code:
Create the session from the
Sessionconstructor (sendSESSION CREATEto the I2P SAM proxy). This constructor was only called when transient
sessions were needed and was immediately followed by
Connect()whichwould have created the session. So this is a noop change if viewed
in isolation.
Every time we try to connect to any peer (not just I2P) create a new
I2P session, up to a limit (currently 10). This way a bunch of
sessions are created (
SESSION CREATE) at times that are decoupledfrom the time when sessions will be used (
STREAM CONNECT).Tweak the code in
CConnman::ConnectNode()to avoid creating sessionobjects while holding
m_unused_i2p_sessions_mutexbecause nowcreating
i2p::sam::Sessioninvolves network activity and could takea few seconds.
Before (note the timestamps):
2025-03-13T09:23:40Z [opencon] [i2p:info] Transient SAM session cd7ea49a7e created
2025-03-13T09:23:43Z [opencon] [i2p] -> STREAM CONNECT ID=cd7ea49a7e DESTINATION=...
2025-03-13T09:23:51Z [opencon] [i2p:info] Transient SAM session cb0956207e created
2025-03-13T09:23:52Z [opencon] [i2p] -> STREAM CONNECT ID=cb0956207e DESTINATION=...
2025-03-13T09:24:04Z [opencon] [i2p:info] Transient SAM session 35ad67f343 created
2025-03-13T09:24:04Z [opencon] [i2p] -> STREAM CONNECT ID=35ad67f343 DESTINATION=...
After:
2025-03-13T17:08:58Z [opencon] [i2p:info] Transient SAM session 16098cd4ea created
2025-03-13T17:09:34Z [opencon] [i2p] -> STREAM CONNECT ID=16098cd4ea DESTINATION=...
2025-03-13T17:09:38Z [opencon] [i2p:info] Transient SAM session 5d209bf1bc created
2025-03-13T17:09:59Z [opencon] [i2p] -> STREAM CONNECT ID=5d209bf1bc DESTINATION=...
2025-03-13T17:09:50Z [opencon] [i2p:info] Transient SAM session e78aa6d2e7 created
2025-03-13T17:12:09Z [opencon] [i2p] -> STREAM CONNECT ID=e78aa6d2e7 DESTINATION=...
This was suggested by @eyedeekay some time ago (in 2023 actually) and has been lingering in my TODO since then 👀. A further suggestion is to also add a time gap between the activity cease and the closing of the session. For this, when
CNodeis destroyedCNode::m_i2p_sam_sessionwould have to be moved elsewhere and destroyed at a later time. cc @zzzi2p