I2P: add support for transient addresses for outbound connections

vasild commented at 8:27 am on June 13, 2022: contributor

Add support for generating a transient, one-time I2P address for ourselves when making I2P outbound connection and discard it once the connection is closed.

Background

In I2P connections, the host that receives the connection knows the I2P address of the connection initiator. This is unlike the Tor network where the recipient does not know who is connecting to them, not even the initiator’s Tor address.

Persistent vs transient I2P addresses

Even if an I2P node is not accepting incoming connections, they are known to other nodes by their outgoing I2P address. This creates an opportunity to white-list given nodes or treat them differently based on their I2P address. However, this also creates an opportunity to fingerprint or analyze a given node because it always uses the same I2P address when it connects to other nodes. Thus, if a node is not accepting incoming I2P connections (-i2pacceptincoming=0) we will generate a transient (disposable), one-time I2P address for each new outgoing connection. That address is never going to be reused again, not even if reconnecting to the same peer later. If -i2pacceptincoming=1 is used then we will use the persistent listening address for outgoing connections and will advertise it to the peers we connect to (like before this PR).

DrahtBot added the label P2P on Jun 13, 2022

laanwj commented at 1:41 pm on June 13, 2022: member

Concept ACK. Interesting idea. Note that we do a similar thing for Tor by creating a new circuit for every outgoing connection (by providing random SOCKS5 auth).

kristapsk commented at 8:11 pm on June 13, 2022: contributor

Concept ACK

kristapsk commented at 10:24 am on June 14, 2022: contributor

Are there some noticable tradeoffs / downsides with this? Why not just enable this by default?

DrahtBot added the label Needs rebase on Jun 14, 2022

vasild commented at 11:14 am on June 14, 2022: contributor

I was just thinking the same.

The reason I set the default value of i2ptransientout to 0 is in case somebody is too attached to the persistent addresses or if the new code has a bug - let it mature for a release or two and then flip the default.

I don’t have a strong opinion, maybe it is ok to introduce it as i2ptransientout=1 as well.

On the low-level, each session requires an extra control socket:

with i2ptransientout=0 we need 1 + n_incoming + n_outgoing (one control socket to rule them all)
with i2ptransientout=1 we need 1 + n_incoming + 2 * n_outgoing (one control socket for all incoming + one control socket for each outgoing).

I think this is ok wrt max open sockets on the system because we don’t have 100s or 1000s of outgoing connections.

in doc/i2p.md:74 in 277111acc1 outdated

72+So, even if an I2P node is not accepting incoming connections, they are known
73+to other nodes by their outgoing I2P address. This creates an opportunity to
74+white-list given nodes or treat them differently based on their I2P address.
75+However, this also creates an opportunity to fingerprint or analyze a given node
76+because it always uses the same I2P address when it connects to other nodes. If
77+this is undesirable, then a node operator can use `-i2ptransientout` to generate

unknown commented at 11:54 am on June 14, 2022:

This creates an opportunity to white-list given nodes or treat them differently based on their I2P address. However, this also creates an opportunity to fingerprint or analyze a given node because it always uses the same I2P address when it connects to other nodes.

Why is this not possible with incoming connections and only outgoing connections? A node that accepts incoming connection with i2p can also be analyzed IMO

vasild commented at 12:05 pm on June 14, 2022:

Oh, well, it is also possible to analyze a node by connecting to it. That is kind of unavoidable and a node that wants to avoid this should not be listening.

in doc/i2p.md:83 in 277111acc1 outdated

81+
82+`-i2ptransientout` only affects addresses for outgoing connections and not the
83+address for accepting incoming connections (if `-i2pacceptincoming` is given).
84+This is because the address for accepting connections is advertised to the
85+Bitcoin network and it takes time for it to propagate and be known by other
86+nodes. So, it does not make sense to change it often - it must be persistent.

unknown commented at 11:54 am on June 14, 2022:

How often can this be changed if a user wants to do it?

vasild commented at 12:19 pm on June 14, 2022:

It is hard to give a concrete answer but it could be days before one’s address is well propagated through the network.

You can measure for yourself in your environment: delete ~/.bitcoin/i2p_private_key and restart bitcoind - it will generate a fresh I2P address for listening. Then monitor the incoming connections with something like bitcoin-cli getpeerinfo |jq 'map(select(.network == "i2p" and .inbound)) |length' or by enabling -debug=net and grepping debug.log for something like connection from.*i2p accepted.

jonatack commented at 12:36 pm on July 21, 2022:

suggestions

 0-In I2P connections, the host that receives the connection knows the I2P address
 1-of the connection initiator. This is unlike the Tor network where the recipient
 2-does not know who is connecting to them, not even the initiator's Tor address.
 3-
 4-So, even if an I2P node is not accepting incoming connections, they are known
 5-to other nodes by their outgoing I2P address. This creates an opportunity to
 6-white-list given nodes or treat them differently based on their I2P address.
 7-However, this also creates an opportunity to fingerprint or analyze a given node
 8-because it always uses the same I2P address when it connects to other nodes. If
 9-this is undesirable, then a node operator can use `-i2ptransientout` to generate
10-a transient (disposable), one-time I2P address for each new outgoing connection.
11-That address is never going to be reused again, not even if reconnecting to the
12-same peer later.
13-
14-`-i2ptransientout` only affects addresses for outgoing connections and not the
15-address for accepting incoming connections (if `-i2pacceptincoming` is given).
16-This is because the address for accepting connections is advertised to the
17-Bitcoin network and it takes time for it to propagate and be known by other
18-nodes. So, it does not make sense to change it often - it must be persistent.
19+With I2P connections, the connection receiver sees the I2P address of the
20+connection initiator, unlike the Tor network where the receiver does
21+not know who is connecting to them, not even the initiator's Tor address.
22+
23+Even if an I2P node does not accept incoming connections, it is known to other
24+nodes by its outgoing I2P address. If this address is always the same when it
25+connects to other nodes, this creates an opportunity to discriminate,
26+fingerprint or analyze it based on its I2P address.
27+
28+For this reason, the `-i2ptransientout` configuration option generates and
29+announces a new, transient I2P address for each new outbound connection. The
30+address will never be reused, not even if reconnecting to the same peer later.
31+
32+`-i2ptransientout` only affects addresses announced to outbound connections and
33+not the address for accepting inbound connections (provided that
34+`-i2pacceptincoming` is on).  This is done because the address for accepting
35+connections is advertised to the Bitcoin network and it takes time for it to
36+propagate and be known by other nodes. It does not make sense to change it
37+often -- it must be persistent.

vasild commented at 10:53 am on July 25, 2022:

Took some of this.

ghost commented at 11:59 am on June 14, 2022: none

Add support for generating a transient, one-time I2P address for ourselves when making I2P outbound connection and discard it once the connection is closed.

Concept ACK

vasild force-pushed on Jun 16, 2022

vasild commented at 11:56 am on June 16, 2022: contributor

ab9172846f...9be11f7496: rebase due to conflicts

DrahtBot removed the label Needs rebase on Jun 16, 2022

DrahtBot commented at 3:29 pm on June 16, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25174 (net/net_processing: Add thread safety related annotations for CNode and Peer by ajtowns)

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.

luke-jr commented at 9:11 pm on June 18, 2022: member

I suggest enabling it by default, and having a net permission flag to use the “real” source address (requires #17167 I guess?).

vasild force-pushed on Jun 28, 2022

vasild commented at 11:28 am on June 28, 2022: contributor

9be11f7496...a69528c9f6: rebase and change the default of -i2ptransientout to true.

vasild commented at 7:05 am on July 14, 2022: contributor

Further idea (out of the scope of this PR): periodically disconnect long lived outbound connections to Tor and to I2P if -i2ptransientout=1. Having a prolonged outbound connection to an adversary is the same as not changing our address for the duration of the connection. Even if, by chance, we reconnect to the same peer after the disconnect, they would not know it is us again.

vasild commented at 7:47 am on July 15, 2022: contributor

It may be desirable to have this in 24.0, feature freeze in mid August.

jonatack commented at 8:23 am on July 15, 2022: contributor

Will have a look!

It may be desirable to have this in 24.0, feature freeze in mid August

DrahtBot added the label Needs rebase on Jul 19, 2022

vasild commented at 1:05 pm on July 19, 2022: contributor

a69528c9f6...153ce63b98: rebase due to conflicts

vasild force-pushed on Jul 19, 2022

DrahtBot removed the label Needs rebase on Jul 19, 2022

in src/init.cpp:474 in 153ce63b98 outdated

470@@ -471,6 +471,7 @@ void SetupServerArgs(ArgsManager& argsman)
471     argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
472     argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
473     argsman.AddArg("-i2pacceptincoming", "If set and -i2psam is also set then incoming I2P connections are accepted via the SAM proxy. If this is not set but -i2psam is set then only outgoing connections will be made to the I2P network. Ignored if -i2psam is not set. Listening for incoming I2P connections is done through the SAM proxy, not by binding to a local address and port (default: 1)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
474+    argsman.AddArg("-i2ptransientout", strprintf("Generate a transient, one-time I2P address for ourselves when making I2P outbound connection and discard it once the connection is closed. Does not affect the address for accepting incoming I2P connections which is persistent. Ignored if -i2psam is not set. (default: %u)", DEFAULT_I2P_TRANSIENT_OUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

jonatack commented at 10:38 am on July 21, 2022:

88af514 the help could perhaps be shorter and more focused on what it does from a user’s point of view rather than on how

0argsman.AddArg("-i2ptransientout", strprintf("Announce a new, transient I2P address to each outbound I2P connection (default: %u). The address for accepting incoming I2P connections is unaffected and persistent. This option is ignored if -i2psam is not set.", DEFAULT_I2P_TRANSIENT_OUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

vasild commented at 10:39 am on July 25, 2022:

Took some of this. “use” is better than “announce” here because we do not actually announce our I2P address anywhere (for outgoing connections). The peer has knowledge of it from the underlying networking layer.

in src/net.h:690 in 153ce63b98 outdated

686@@ -667,6 +687,7 @@ class CConnman
687         std::vector<std::string> m_specified_outgoing;
688         std::vector<std::string> m_added_nodes;
689         bool m_i2p_accept_incoming;
690+        bool m_i2p_transient_out = DEFAULT_I2P_TRANSIENT_OUT;

jonatack commented at 10:45 am on July 21, 2022:

88af5140702ddd425f1da11126b9e799e7585d38 nit, I realize you’re following the surrounding code style, but for new code m_ prefixing (to distinguish data members from local vars in a class) seems unnecessary and needlessly verbose in this struct (feel free to ignore).

vasild commented at 10:40 am on July 25, 2022:

I will leave it as m_ - for consistency with surrounding and also, the developer notes just say: “Class member variables have a m_ prefix”.

jonatack commented at 11:42 am on July 29, 2022:

the developer notes just say: “Class member variables have a m_ prefix”.

While that is true, I’ve read a number of comments that suggest updating the dev notes with the feedback above (anyway, no big deal.)

in src/net.h:1103 in 153ce63b98 outdated

1099      */
1100     std::unique_ptr<i2p::sam::Session> m_i2p_sam_session;
1101 
1102+    /**
1103+     * Whether to make outbound I2P connections from a transient, one-time I2P address.
1104+     */

jonatack commented at 10:50 am on July 21, 2022:

88af5140702ddd425f1da11126b9e799e7585d38 I realize you’re following the surrounding code style, but for new code IIRC to describe a member we use //! Description... or /// Description...

1667347 same for the documentation of const bool m_transient;

04648910 and for std::unique_ptr<i2p::sam::Session> m_i2p_session

vasild commented at 10:50 am on July 25, 2022:

Any doxygen-compatible comment should be fine. The code already uses a rich mix of them.

jonatack commented at 11:42 am on July 29, 2022:

Maybe the style guide on doxygen docs should be removed if it is pointless (though I have been asked to follow it by reviewers). Anyway, only a nit.

in src/i2p.cpp:129 in 153ce63b98 outdated

126+}
127+
128+Session::Session(const CService& control_host, CThreadInterrupt* interrupt)
129+    : m_control_host{control_host},
130+      m_interrupt{interrupt},
131+      m_control_sock{std::make_unique<Sock>(INVALID_SOCKET)},

jonatack commented at 10:58 am on July 21, 2022:

71c4e62 and fe3df51d the diff on these two style commits is small enough to fold them into the other changes (-0 on whether they should be done)

vasild commented at 10:52 am on July 25, 2022:

Reviewers have asked before, and I think it is good practice to have style and logical changes in separate commits (most of the time).

jonatack commented at 11:43 am on July 29, 2022:

For larger changes, yes. YMMV; I’ve only ever been asked to do the opposite and squash them.

vasild commented at 11:41 am on August 9, 2022:

Squashed, I find it hard to review this way, but you are the reviewer after all 🧐

in src/net.cpp:496 in 153ce63b98 outdated

492+        const bool use_proxy{GetProxy(addrConnect.GetNetwork(), proxy)};
493         bool proxyConnectionFailed = false;
494 
495-        if (addrConnect.GetNetwork() == NET_I2P && m_i2p_sam_session.get() != nullptr) {
496+        if (addrConnect.GetNetwork() == NET_I2P && use_proxy) {
497+            auto sess = &m_i2p_sam_session;

jonatack commented at 12:02 pm on July 21, 2022:

0d82a1c3 would it be better to drop sess and the pointer juggling and just have two Connect() calls?

vasild commented at 10:53 am on July 25, 2022:

Done, it looks more straight-forward now.

in doc/i2p.md:47 in 153ce63b98 outdated

38@@ -39,6 +39,12 @@ Core configuration options:
39      Ignored if -i2psam is not set. Listening for incoming I2P
40      connections is done through the SAM proxy, not by binding to a
41      local address and port (default: 1)
42+
43+-i2ptransientout
44+     Generate a transient, one-time I2P address for ourselves when making
45+     I2P outbound connection and discard it once the connection is
46+     closed. Does not affect the address for accepting incoming I2P
47+     connections which is persistent. Ignored if -i2psam is not set.

jonatack commented at 12:22 pm on July 21, 2022:

In line with the suggestion in src/init.cpp

0-     Generate a transient, one-time I2P address for ourselves when making
1-     I2P outbound connection and discard it once the connection is
2-     closed. Does not affect the address for accepting incoming I2P
3-     connections which is persistent. Ignored if -i2psam is not set.
4+     Announce a new, transient I2P address to each outbound I2P connection
5+     (default: 1). The address for accepting incoming I2P connections
6+     is unaffected and persistent. This option is ignored if -i2psam
7+     is not set.

vasild commented at 10:53 am on July 25, 2022:

Done (some of it).

in doc/release-notes-25355.md:4 in 153ce63b98 outdated

0@@ -0,0 +1,5 @@
1+New settings
2+------------
3+
4+`-i2ptransientout` has been added to generate a new I2P address for each

jonatack commented at 12:42 pm on July 21, 2022:

It seems important to first announce the change of default behavior (maybe in the Notable Changes or P2P section): With I2P connections, a new, transient address is announced to each outbound peer by default. And then that a config option is added to allow toggling back to the previous behavior.

0A new `-i2ptransientout` configuration option has been added...

vasild commented at 11:13 am on July 25, 2022:

Done.

jonatack commented at 12:47 pm on July 21, 2022: contributor

Concept ACK with feedback below.

I think it would make sense to add the user-facing configuration option after the functionality has been added rather than before.

in src/i2p.cpp:408 in 153ce63b98 outdated

415     m_control_sock = std::move(sock);
416 
417-    LogPrintfCategory(BCLog::I2P, "SAM session created: session id=%s, my address=%s\n",
418-                      m_session_id, m_my_addr.ToString());
419+    Log("%s SAM session created: session id=%s, my address=%s",
420+        session_type,

jonatack commented at 10:51 am on July 23, 2022:

Running bitcoind on this branch

02022-07-23T10:29:26Z [i2p] persistent SAM session created: session id=21c6f9ca06, my address=zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p:0
12022-07-23T10:29:26Z AddLocal(zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p:0,4)
2
32022-07-23T10:29:32Z [i2p] transient SAM session created: session id=28dd7f10d7, my address=5rkqw4u7fieeaw5uq6opg3mnbxn362zc3djkttgxdxholykvugpa.b32.i2p:0
42022-07-23T10:29:34Z [i2p] Creating transient SAM session with 127.0.0.1:7656

0        m_transient ? "Transient" : "Persistent"

vasild commented at 10:54 am on July 25, 2022:

Done, using Capitalize().

jonatack commented at 1:49 pm on July 24, 2022: contributor

Been running this branch for a couple days and haven’t noticed any issues.

 02022-07-24T13:39:38Z [i2p] Creating transient SAM session with 127.0.0.1:7656
 12022-07-24T13:40:18Z [i2p] transient SAM session created: session id=1386b9ab4d, my address=oqdzoox4z6apbi6zey6ijztpxhl4c7ia2rlgmlyvn6gh5y3f7qma.b32.i2p:0
 22022-07-24T13:40:27Z [i2p] Creating transient SAM session with 127.0.0.1:7656
 32022-07-24T13:40:47Z [i2p] transient SAM session created: session id=5c4ad011cc, my address=edj6fribfj3vg5k6rw36lqdrvoclwpxkwbrig632t25nnrzjfrza.b32.i2p:0
 42022-07-24T13:41:00Z [i2p] Destroying session 5c4ad011cc
 52022-07-24T13:41:20Z [i2p] Destroying session 1386b9ab4d
 62022-07-24T13:41:20Z [i2p] Destroying session b0efeb7424
 72022-07-24T13:42:01Z [i2p] Creating transient SAM session with 127.0.0.1:7656
 82022-07-24T13:42:21Z [i2p] transient SAM session created: session id=cf078d4a94, my address=3mfzrp3idd73axoiinsbnq5kjrnsgqhmhw3y7ndr5ctwg47rlora.b32.i2p:0
 92022-07-24T13:42:30Z [i2p] Creating transient SAM session with 127.0.0.1:7656
102022-07-24T13:43:10Z [i2p] transient SAM session created: session id=36935751c5, my address=igdzgnvdqfu2yb7br4qvnvdlfpqdkkdagdton44mza7gwys4ulya.b32.i2p:0
112022-07-24T13:43:20Z New outbound peer connected: version: 70016, blocks=746321, peer=639, peeraddr={xxx].b32.i2p:0 (manual)
122022-07-24T13:43:21Z New outbound peer connected: version: 70015, blocks=746321, peer=640, peeraddr=157.230.233.211:8333 (outbound-full-relay)
132022-07-24T13:43:23Z [i2p] Creating transient SAM session with 127.0.0.1:7656
142022-07-24T13:43:37Z [i2p] Destroying session 36935751c5
152022-07-24T13:44:03Z [i2p] transient SAM session created: session id=ff1aaed962, my address=4gk6orbh5tf6cyc3l43nb6inxgvfcqu4pmteaigh3qpgbuxadayq.b32.i2p:0
162022-07-24T13:44:04Z [i2p] Creating transient SAM session with 127.0.0.1:7656
172022-07-24T13:44:24Z [i2p] transient SAM session created: session id=1f85a9eb0f, my address=yxl7j666clruej6qj6qyccr2jqkxo3lt5b4mhhqoczchyvg3hfma.b32.i2p:0
182022-07-24T13:44:42Z [i2p] Error connecting to jz3s4eurm5vzjresf4mwo7oni4bk36daolwxh4iqtewakylgkxmq.b32.i2p:0: "STREAM STATUS RESULT=CANT_REACH_PEER"
192022-07-24T13:44:42Z [i2p] Destroying session 1f85a9eb0f

in src/i2p.cpp:371 in 153ce63b98 outdated

365@@ -355,29 +366,48 @@ void Session::CreateIfNotCreatedAlready()
366         return;
367     }
368 
369-    Log("Creating SAM session with %s", m_control_host.ToString());
370+    const auto session_type = m_transient ? "transient" : "persistent";
371+
372+    Log("Creating %s SAM session with %s", session_type, m_control_host.ToString());

jonatack commented at 2:06 pm on July 24, 2022:

It might be handy to log the session id in this message to make it easier to match the session messages in a busy i2p log.

vasild commented at 10:54 am on July 25, 2022:

Nice, done!

vasild commented at 10:29 am on July 25, 2022: contributor

153ce63b98...506d59e2e1: rebase and address suggestions

I think it would make sense to add the user-facing configuration option after the functionality has been added rather than before.

That would require adding a line (the functionality) and changing it in the next commit (to make it conditional). That would be more work for reviewers and for history gazers (git blame). Should I?

vasild force-pushed on Jul 25, 2022

in src/i2p.cpp:409 in 506d59e2e1 outdated

416-    LogPrintfCategory(BCLog::I2P, "SAM session created: session id=%s, my address=%s\n",
417-                      m_session_id, m_my_addr.ToString());
418+    Log("%s SAM session created: session id=%s, my address=%s",
419+        Capitalize(session_type),
420+        m_session_id,
421+        m_my_addr.ToString());

jonatack commented at 10:57 am on July 29, 2022:

It might be be good to standardize between session <id> and session id=<id> in the logging.

02022-07-29T10:48:31Z [i2p] Creating transient SAM session 402ba97b02 with 127.0.0.1:7656
12022-07-29T10:48:51Z [i2p] Transient SAM session created: session id=402ba97b02, my address=3fuk2uj65qgnlxacjwnyde5v2b36ztorub3a2gzxrhzzswy56c3q.b32.i2p:0
22022-07-29T10:49:05Z [i2p] Persistent SAM session created: session id=2debde9a21, my address=zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p:0
32022-07-29T10:49:05Z AddLocal(zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p:0,4)
42022-07-29T10:51:31Z Syncing coinstatsindex with block chain from height 548451
52022-07-29T10:51:31Z [i2p] Destroying session 402ba97b02
62022-07-29T10:51:32Z [i2p] Creating transient SAM session 8b04293706 with 127.0.0.1:7656

nit, I think it would be more readable and a better use of screen space if these 3 args remained on one line.

vasild commented at 1:14 pm on July 29, 2022:

Done, now we print “SAM session xyz” from everywhere.

(clang-format formats it like that, given a line length limit)

jonatack commented at 4:01 pm on July 29, 2022:

Done, now we print “SAM session xyz” from everywhere.

Tested, nice!

(clang-format formats it like that, given a line length limit)

Don’t give it such a low limit :D

in doc/i2p.md:70 in 506d59e2e1 outdated

68+connection initiator. This is unlike the Tor network where the recipient does
69+not know who is connecting to them, not even the initiator's Tor address.
70+
71+Even if an I2P node is not accepting incoming connections, it is known to other
72+nodes by its outgoing I2P address. If this address is always the same, then
73+there is an opportunity to white-list a give node. However, this also creates an

jonatack commented at 11:09 am on July 29, 2022:

0there is an opportunity to whitelist a given node. However, this also creates an

Would suggest shortening this and avoiding repeating “a given node” (also, ISTM we’ve been avoiding the term “whitelist” the past couple years, e.g. with net permissions flag “noban” instead) with:

0Even if an I2P node does not accept incoming connections, it is known to other
1nodes by its outgoing I2P address. If the address is always the same when it
2connects to other nodes, this creates an opportunity to discriminate,
3fingerprint or analyze it based on its I2P address.

vasild commented at 1:16 pm on July 29, 2022:

Took the suggestion, but left the “whitelist” text.

in doc/i2p.md:83 in 506d59e2e1 outdated

81+
82+`-i2ptransientout` only affects addresses for outgoing connections and not the
83+address for accepting incoming connections (if `-i2pacceptincoming` is given).
84+This is because the address for accepting connections is advertised to the
85+Bitcoin network and it takes time for it to propagate and be known by other
86+nodes. So, it does not make sense to change it often - it must be persistent.

jonatack commented at 11:12 am on July 29, 2022:

Don’t begin a sentence with “This is because…” (suggest dropping it: “The address…”)

Idem for “So, …” (suggest dropping)

An emdash (" — " or " — “) should be used here, not a hyphen. An emdash can optionally be set off with spaces.

0nodes. It does not make sense to change it often — it must be persistent.

vasild commented at 1:17 pm on July 29, 2022:

Done, changed the minus to em-and :laughing:

in src/i2p.cpp:370 in 506d59e2e1 outdated

365@@ -355,29 +366,47 @@ void Session::CreateIfNotCreatedAlready()
366         return;
367     }
368 
369-    Log("Creating SAM session with %s", m_control_host.ToString());
370+    const auto session_type = m_transient ? "transient" : "persistent";
371+    const auto session_id = GetRandHash().GetHex().substr(0, 10); // full is an overkill, too verbose in the logs

jonatack commented at 11:21 am on July 29, 2022:

0    const auto session_id{GetRandHash().GetHex().substr(0, 10)}; // a full id would be overly verbose in the logs

(not sure if braced initialization is better here)

(in any case: s/an overkill/overkill/)

vasild commented at 1:19 pm on July 29, 2022:

{} forces stricter type checks. E.g. Foo x{expression_that_returns_Bar}; but it is pointless when auto is used. I find it more readable to use =.

Removed the “an “.

jonatack commented at 3:36 pm on July 29, 2022:

maybe s/const auto/const auto&/ (I’m not sure if compilers curerntly detect and optimize this to avoid a copy)

vasild commented at 1:35 pm on August 9, 2022:

No need for a reference: https://stackoverflow.com/questions/38043319/how-does-guaranteed-copy-elision-work#38043447

jonatack commented at 11:41 am on July 29, 2022: contributor

Tested ACK 506d59e2e18278635c9b55181415cc140caf82ae

Commit message “i2p: log the session id in the “Creating seesion” message” -> “session”
Would squash or drop the two style commits

Some suggestions follow, happy to re-ACK for them.

in doc/release-notes-25355.md:13 in 506d59e2e1 outdated

 8+  connection by default. (#25355)
 9+
10+New settings
11+------------
12+
13+- The `-i2ptransientout` option has been added to generate a new I2P address

jonatack commented at 11:46 am on July 29, 2022:

0- An `-i2ptransientout` configuration option has been added to opt out of the new default behavior of creating a new, transient I2P address

vasild commented at 1:24 pm on July 29, 2022:

I find the suggestion unnecessary long. Maybe append “(default: true)” to the current text?

jonatack commented at 3:26 pm on July 29, 2022:

Could drop “of the new default behavior” for brevity if you prefer, but opting out of the new default is the only reason for adding the config option so it seems best to describe it in this way.

vasild commented at 11:47 am on August 9, 2022:

If there is an option to opt out of X, I would expect enabling that option to opt out of X. This is not the case with -i2ptransientout, thus I think the above suggestion could be misleading. That option is to enable the usage of transient addresses. To opt out of that the user has to disable the option.

vasild commented at 4:59 pm on August 11, 2022:

Closing as not relevant anymore - -i2ptransientout was dropped.

in doc/i2p.md:75 in 506d59e2e1 outdated

73+there is an opportunity to white-list a give node. However, this also creates an
74+opportunity to discriminate, fingerprint or analyze a given node based on its
75+I2P address.
76+
77+For this reason, the `-i2ptransientout` configuration option generates and
78+uses a new, transient (disposable) I2P address for each new outbound connection.

jonatack commented at 11:50 am on July 29, 2022:

maybe s/generates and uses/creates/

vasild commented at 1:22 pm on July 29, 2022:

Done.

vasild force-pushed on Jul 29, 2022

vasild commented at 1:14 pm on July 29, 2022: contributor

506d59e2e1...acae0543fc: address suggestions

Invalidates ACK from @jonatack

in doc/i2p.md:80 in 1eb601d6f8 outdated

78+transient (disposable) I2P address for each new outbound connection. The address
79+will never be reused, not even if reconnecting to the same peer later.
80+
81+`-i2ptransientout` only affects addresses for outgoing connections and not the
82+address for accepting incoming connections (if `-i2pacceptincoming` is given).
83+The address for accepting connections is advertised to the Bitcoin network and

mzumsande commented at 9:05 pm on July 29, 2022:

Maybe mention that we’ll still self-advertise the persistent address on outbound connections if -i2pacceptincoming=1 and -i2ptransientout=1? In this case, making outbound connections transient doesn’t really add any privacy because we’ll self-advertise our persistent address on each connection, even though we create a transient address to connect with.

vasild commented at 11:54 am on August 9, 2022:

Good point! That behavior would be surprising and documenting it is sub-optimal. Better don’t advertise our persistent address over I2P outgoing connections that use transient addresses. I extended the commit net: create a transient I2P session if -i2ptransientout is given with the following diff to achieve that:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 74d1bf44d2..17c67ae7a5 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -2773,12 +2773,32 @@ void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr<const CBlo
 5     } else {
 6         LOCK(cs_main);
 7         mapBlockSource.erase(block->GetHash());
 8     }
 9 }
10 
11+/**
12+ * Decide if we should send our own address to a given peer as a self-advertisement.
13+ * We do the self-advertisement:
14+ * - initially after establishing the connection and
15+ * - periodically every ~24h (AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL).
16+ */
17+static bool ShouldSelfAdvertiseTo(const CNode& node)
18+{
19+    if (!fListen) {
20+        return false;
21+    }
22+
23+    // Don't self-advertise if this is an I2P outbound connection and -i2ptransientout=1.
24+    // If we do so, then we would self-advertise our persistent I2P address, used for
25+    // accepting incoming connections, which would defeat the purpose of using transient
26+    // addresses for outgoing connections.
27+    return !node.addrBind.IsI2P() || node.IsInboundConn() ||
28+           !gArgs.GetBoolArg("-i2ptransientout", DEFAULT_I2P_TRANSIENT_OUT);
29+}
30+
31 void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
32                                      const std::chrono::microseconds time_received,
33                                      const std::atomic<bool>& interruptMsgProc)
34 {
35     LogPrint(BCLog::NET, "received: %s (%u bytes) peer=%d\n", SanitizeString(msg_type), vRecv.size(), pfrom.GetId());
36 
37@@ -2924,14 +2944,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
38             // empty and no one will know who we are, so these mechanisms are
39             // important to help us connect to the network.
40             //
41             // We skip this for block-relay-only peers. We want to avoid
42             // potentially leaking addr information and we do not want to
43             // indicate to the peer that we will participate in addr relay.
44-            if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload())
45-            {
46+            if (ShouldSelfAdvertiseTo(pfrom) && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
47+
48                 CAddress addr{GetLocalAddress(pfrom.addr), peer->m_our_services, (uint32_t)GetAdjustedTime()};
49                 FastRandomContext insecure_rand;
50                 if (addr.IsRoutable())
51                 {
52                     LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString());
53                     PushAddress(*peer, addr, insecure_rand);
54@@ -4670,13 +4690,13 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
55 {
56     // Nothing to do for non-address-relay peers
57     if (!peer.m_addr_relay_enabled) return;
58 
59     LOCK(peer.m_addr_send_times_mutex);
60     // Periodically advertise our local address to the peer.
61-    if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload() &&
62+    if (ShouldSelfAdvertiseTo(node) && !m_chainman.ActiveChainstate().IsInitialBlockDownload() &&
63         peer.m_next_local_addr_send < current_time) {
64         // If we've sent before, clear the bloom filter for the peer, so that our
65         // self-announcement will actually go out.
66         // This might be unnecessary if the bloom filter has already rolled
67         // over since our last self-announcement, but there is only a small
68         // bandwidth cost that we can incur by doing this (which happens

Since this PR had two ACKs without the above, let me know if it is ok or if I should remove it (@jonatack, @mzumsande).

mzumsande commented at 1:52 pm on August 9, 2022:

This seems problematic to me: A new node with -i2pacceptincoming=1 (that doesn’t change the default -i2ptransientout value) will have a hard time getting inbound peers if it only self-advertises to inbound peers, but only receives inbound peers once it has successfully self-advertised.

Maybe it makes sense to instead soft-set -i2ptransientout to false if -i2pacceptincoming was chosen? Or, alternatively, just document this behavior, because, as you wrote above, “a node that wants to avoid this should not be listening.”

vasild commented at 4:38 pm on August 9, 2022:

A node could self-advertise its I2P address to an IPv4 peer if it only listens on I2P. However, if it also listens on IPv4 then it would prefer its IPv4 address. I guess that for the discussion here we should assume that I2P addresses get self-advertised only (mostly) over I2P connections.

From what you have pointed out it seems that -i2pacceptincoming=1 is kind of incompatible with -i2ptransientout=1 because if both are set, then:

if we self-advertise our persistent address in outgoing I2P connections, then that is very easy to detect and the same as -i2ptransientout=0 wrt privacy
if we don’t self-advertise in outgoing I2P connections then nobody is going to learn our listening I2P address and we will not get incoming connections, like -i2pacceptincoming=0

In current code we have -i2pacceptincoming default to 1. Assuming we don’t want to change that, then maybe introduce -i2ptransientout with a default of 0 and hard fail the startup if both are explicitly/manually set to 1 by the user? And flip -i2ptransientout to 1 if -i2pacceptincoming is switched off by the user?

How does this work for Tor? From what I read in the code, if a node is listening on a Tor service, then it would self-advertise its listening Tor address in outbound Tor connections. So, “the Tor peer does not know who is connecting to them” is not true in this case - the peer knows the connecting node’s Tor address (which is persistent).

mzumsande commented at 7:07 pm on August 9, 2022:

From what you have pointed out it seems that -i2pacceptincoming=1 is kind of incompatible with -i2ptransientout=1

Yes, I think that’s correct.

In current code we have -i2pacceptincoming default to 1. Assuming we don’t want to change that, then maybe introduce -i2ptransientout with a default of 0 and hard fail the startup if both are explicitly/manually set to 1 by the user? And flip -i2ptransientout to 1 if -i2pacceptincoming is switched off by the user?

That looks sensible to me.

As an alternative thought, I wonder if we would lose much if we’d instead tie the creation of transient addresses to -i2pacceptincoming=0 and not introduce a -i2ptransientout option at all. That would make things much simpler, but we wouldn’t support the use case of -i2pacceptincoming=0, -i2ptransientout=0 anymore (so that potential peers couldn’t whitelist our permanent address). But is there a lot utility in whitelisting i2p peers like that? I’m asking because if out peer doesn’t know who we are, they don’t really have a reason to whitelist us, and if they actually do know us, why would we need to connect to them via i2p in the first place?

How does this work for Tor? From what I read in the code, if a node is listening on a Tor service, then it would self-advertise its listening Tor address in outbound Tor connections. So, “the Tor peer does not know who is connecting to them” is not true in this case - the peer knows the connecting node’s Tor address (which is persistent).

That looks right, although I didn’t test it. But I think that if you want to have Tor inbound connections, it is not unreasonable to assume that you might want to let the network (including your outbound peers) know about that.

vasild commented at 11:56 am on August 10, 2022:

As an alternative thought…

That sounds even better to me. I like not adding new options :) Use transient addresses if -i2pacceptincoming=0, simple. That would make it work similar to Tor - if we are listening then we reveal our (Tor or I2P) listening address to the peers we connect to (outbound).

As for the downside of not being able to whitelist if -i2pacceptincoming=0 (and thus transient addresses are being used), that can be done later if #17167 gets in, and/or use the persistent address for making outbound connections to nodes given to -addnode= or -connect= even if transient ones are being used for other outbound connections (due to -i2pacceptincoming=0).

Makes sense?

mzumsande commented at 3:18 pm on August 10, 2022:

Makes sense?

Yes, I also like that option best! (and the transient address behavior could then be mentioned in the -i2pacceptincoming RPC help).

jonatack commented at 3:26 pm on August 10, 2022:

On initial read this SGTM, sensible defaults and not adding the configuration option would also be my preference. Will re-review with this discussion in mind.

vasild commented at 4:58 pm on August 11, 2022:

Done!

Just to answer that one:

if they actually do know us, why would we need to connect to them via i2p in the first place?

Because we may want to hide the fact that Bitcoin nodes are running on either machine. Also, for the added encryption and authentication based on the I2P address’ private key. For example, white-listing IPv4 addresses could be dangerous because, if spoofed, you may end up white-listing a malicious actor who has “taken” that IPv4 address. This is not possible with I2P. To spoof an I2P address one would have to steal the private key from the owner’s machine.

in src/i2p.cpp:370 in 22997f5c34 outdated

366@@ -367,13 +367,12 @@ void Session::CreateIfNotCreatedAlready()
367     }
368 
369     const auto session_type = m_transient ? "transient" : "persistent";
370+    const auto session_id = GetRandHash().GetHex().substr(0, 10); // full is overkill, too verbose in the logs

mzumsande commented at 9:14 pm on July 29, 2022:

22997f5c3443acb853cbb84a4ce27f0ac634ee14 could maybe be squashed, this line already moved in an earlier commit.

vasild commented at 11:54 am on August 9, 2022:

Squashed.

jonatack commented at 9:37 am on July 30, 2022: contributor

ACK acae0543fc8fb7954eadbd7019e446f31112211e

suggestions:

#25355 (review)
commit message “i2p: log the session id in the “Creating seesion” message” -> “session”
squash or drop the small style commits

I think it would make sense to add the user-facing configuration option after the functionality has been added rather than before.

That would require adding a line (the functionality) and changing it in the next commit (to make it conditional). That would be more work for reviewers and for history gazers (git blame). Should I?

At this point, no, to avoid re-reviewing, unless others request it.

mzumsande commented at 10:09 pm on August 1, 2022: contributor

ACK acae0543fc8fb7954eadbd7019e446f31112211e

I tested this on mainnet / by connecting two of my nodes manually, and it worked as expected without any problems.

achow101 added this to the milestone 24.0 on Aug 4, 2022

vasild force-pushed on Aug 9, 2022

vasild commented at 11:54 am on August 9, 2022: contributor

acae0543fc...2dc4683630: address suggestions

Invalidates ACKs from @jonatack, @mzumsande

vasild force-pushed on Aug 9, 2022

vasild commented at 12:09 pm on August 9, 2022: contributor

2dc4683630...18ee27ce7f: rebase due to conflicts

Previously invalidated ACKs from @jonatack, @mzumsande

DrahtBot added the label Needs rebase on Aug 9, 2022

DrahtBot removed the label Needs rebase on Aug 9, 2022

i2p: add support for creating transient sessions

Instead of providing our destination (private key) to the I2P proxy when
creating the session, ask it to generate one for us and do not save it
on disk.

2b781ad66e

vasild force-pushed on Aug 11, 2022

vasild commented at 4:52 pm on August 11, 2022: contributor

18ee27ce7f...6f3e418908: ditch -i2ptransientout and use transient addresses if -i2pacceptincoming=0. Based on the discussion in #25355 (review).

in src/net.cpp:494 in 8637023231 outdated

490     if (addrConnect.IsValid()) {
491+        const bool use_proxy{GetProxy(addrConnect.GetNetwork(), proxy)};
492         bool proxyConnectionFailed = false;
493 
494-        if (addrConnect.GetNetwork() == NET_I2P && m_i2p_sam_session.get() != nullptr) {
495+        if (addrConnect.GetNetwork() == NET_I2P && use_proxy) {

mzumsande commented at 8:32 pm on August 12, 2022:

The old codepath where m_i2p_sam_session exists was not conditional on use_proxy before. Am I right that this is not a change in behavior, because for I2P, the existence of m_i2p_sam_session implies that use_proxy is true so it was just not necessary to check before?

vasild commented at 10:50 am on August 16, 2022:

You are right. (s/implies/implied/ in your comment above)

The code before this PR would set m_i2p_sam_session IFF there is a proxy for NET_I2P, so m_i2p_sam_session.get() != nullptr was equivalent to GetProxy(NET_I2P, ...) == true and checking just the former sufficed.

In the code after this PR we may have a situation where there is a proxy for NET_I2P, but m_i2p_sam_session is empty. Thus we check if a proxy is set and either use m_i2p_sam_session or make a transient session.

mzumsande commented at 8:34 pm on August 12, 2022: contributor

Tested ACK 6f3e418908b1bc5fc39d9232b8c65b6141ba7634

in src/net.h:525 in a9d2d2c5eb outdated

524+          uint64_t nLocalHostNonceIn,
525+          const CAddress& addrBindIn,
526+          const std::string& addrNameIn,
527+          ConnectionType conn_type_in,
528+          bool inbound_onion,
529+          std::unique_ptr<i2p::sam::Session>&& i2p_sam_session = std::unique_ptr<i2p::sam::Session>());

achow101 commented at 8:09 pm on August 15, 2022:

In a9d2d2c5eb2bdfbb2ca0b0e46ea9633492382a7e “net: store an optional I2P session is CNode”

nit:

0          std::unique_ptr<i2p::sam::Session>&& i2p_sam_session = nullptr;

vasild commented at 11:09 am on August 16, 2022:

Done, thanks!

achow101 commented at 8:18 pm on August 15, 2022: member

ACK 6f3e418908b1bc5fc39d9232b8c65b6141ba7634

If you need to retouch, the commit message for a9d2d2c5eb2bdfbb2ca0b0e46ea9633492382a7e should say “in CNode” rather than “is CNode”.

net: store an optional I2P session in CNode

and destroy it when `CNode::m_sock` is closed.

I2P transient sessions are created per connection (i.e. per `CNode`) and
should be destroyed when the connection is closed. Storing the session
in `CNode` is a convenient way to destroy it together with the connection
socket (`CNode::m_sock`).

An alternative approach would be to store a list of all I2P sessions in
`CConnman` and from `CNode::CloseSocketDisconnect()` to somehow ask the
`CConnman` to destroy the relevant session.

a1580a04f5

net: use transient I2P session for outbound if -i2pacceptincoming=0

If not accepting I2P connections, then do not create
`CConnman::m_i2p_sam_session`.

When opening a new outbound I2P connection either use
`CConnman::m_i2p_sam_session` like before or create a temporary one and
store it in `CNode` for destruction later.

ae1e97ce86

test: add a test that -i2pacceptincoming=0 creates a transient session

The test is a bit primitive as it checks the Bitcoin Core log and
assumes that if it logs that it creates a transient session, then it
does that indeed.

A more thorough test would be to check that it indeed sends the
`SESSION CREATE ... DESTINATION=TRANSIENT` command and that it uses
the returned I2P address for connecting, even for repeated connections
to the same I2P peer. That would require a mocked SAM server (proxy)
implementation in Python.

3914e472f5

doc: document I2P transient addresses usage in doc/i2p.md 47c0d02f12

doc: add release notes about the I2P transient addresses d7ec30b648

i2p: log "SAM session" instead of "session"

This way the log messages are consistent with "Creating SAM session..."

59aa54f731

vasild force-pushed on Aug 16, 2022

vasild commented at 11:09 am on August 16, 2022: contributor

6f3e418908...59aa54f731: address minor suggestions (hopefully trivial to re-ack)

Invalidates ACKs from @mzumsande, @achow101

Previously invalidated ACK from @jonatack

mzumsande commented at 6:24 pm on August 16, 2022: contributor

ACK 59aa54f7312f3441692c89feed86b8756d9d6b7a (verified via range-diff that just a typo / unique_ptr initialisation were fixed)

achow101 commented at 8:31 pm on August 16, 2022: member

re-ACK 59aa54f7312f3441692c89feed86b8756d9d6b7a

in src/i2p.h:283 in 59aa54f731

274@@ -262,6 +275,12 @@ class Session
275      * SAM session id.
276      */
277     std::string m_session_id GUARDED_BY(m_mutex);
278+
279+    /**
280+     * Whether this is a transient session (the I2P private key will not be
281+     * read or written to disk).
282+     */
283+    const bool m_transient;

jonatack commented at 9:07 pm on August 16, 2022:

2b781ad6 Don’t make data members const.

0    bool m_transient;

vasild commented at 10:25 am on August 18, 2022:

That guideline says “They are not useful” (const data members), but they improve readability - at a glance one can tell that this variable is only assigned in the constructor and never modified afterwards. Without the const one would have to manually check all references to the variable to find the places where it is modified.

Then the guideline says that const data members make the type difficult to use because it is not copy-assignable. But this is a feature and exactly what we want - to avoid copying a persistent session to a transient one, effectively converting it. Anyway, i2p::sam::Session is already non-copy-able, so adding a const data member does not change its copy-ability.

jonatack commented at 10:02 pm on August 16, 2022: contributor

utACK 59aa54f7312f3441692c89feed86b8756d9d6b7a reviewed range diff, rebased to master, debug build + relevant tests + review at each commit

Will test tomorrow.

in src/i2p.cpp:406 in 59aa54f731

413     m_session_id = session_id;
414     m_control_sock = std::move(sock);
415 
416-    LogPrintfCategory(BCLog::I2P, "SAM session created: session id=%s, my address=%s\n",
417-                      m_session_id, m_my_addr.ToString());
418+    Log("%s SAM session %s created, my address=%s",

jonatack commented at 10:02 am on August 17, 2022:

Tested with -i2pacceptincoming on and off.

When logging the creation of a transient SAM session, it might be helpful to log to which peer, i.e. something like

Transient SAM session 74fc489e60 created to peer=<id>, peeraddr=<ip>, my address=4ejzt3gyxn3icwcn2s473tqayt32ti55e7zlliwusuzlppgagypq.b32.i2p:0

otherwise in a log with many i2p peers, it’s unclear to which peer the transient address is connecting to.

 02022-08-17T09:54:26Z [i2p] Transient SAM session 74fc489e60 created, my address=4ejzt3gyxn3icwcn2s473tqayt32ti55e7zlliwusuzlppgagypq.b32.i2p:0
 12022-08-17T09:54:28Z New outbound peer connected: version: 70016, blocks=749823, peer=8, peeraddr=83.240...6:8333 (outbound-full-relay)
 22022-08-17T09:54:28Z New outbound peer connected: version: 70016, blocks=749823, peer=9, peeraddr=190.2...4:8333 (outbound-full-relay)
 32022-08-17T09:54:34Z New outbound peer connected: version: 70016, blocks=749823, peer=13, peeraddr=31.42....9:8333 (outbound-full-relay)
 42022-08-17T09:54:34Z [i2p] Creating transient SAM session 15cf78cf17 with 127.0.0.1:7656
 52022-08-17T09:54:39Z New outbound peer connected: version: 70016, blocks=749823, peer=14, peeraddr=76.89....:8333 (outbound-full-relay)
 62022-08-17T09:54:54Z [i2p] Transient SAM session 15cf78cf17 created, my address=vnugpix23m4inijbw24d2s6nha6u3rzqpkfsvz3r2sonxvx7o6ma.b32.i2p:0
 72022-08-17T09:54:57Z [i2p] Destroying SAM session 15cf78cf17
 82022-08-17T09:54:58Z [i2p] Creating transient SAM session 6d503018cc with 127.0.0.1:7656
 92022-08-17T09:55:16Z New outbound peer connected: version: 70016, blocks=749823, peer=15, peeraddr=wwbw7nqr....7tpoqjswvcwa.b32.i2p:0 (manual)
102022-08-17T09:55:18Z [i2p] Transient SAM session 6d503018cc created, my address=h75ad7btqhxbkyapqsrnaprhtots5xxoq7yp6qjl3z77idzazg6q.b32.i2p:0
112022-08-17T09:55:33Z New outbound peer connected: version: 70016, blocks=749823, peer=17, peeraddr=jz3s4eurm5vzj...wakylgkxmq.b32.i2p:0 (manual)

Perhaps also similar for the Destroying SAM session <id> to peer=<id>, peeraddr=<addr> log message.

vasild commented at 10:08 am on August 18, 2022:

I agree that would be helpful. It would require seeping knowledge of the peer’s address from the higher level code in net.cpp down to i2p::sam::Session. However, a transient session may as well be used for more than one connection. We may at some point decide to use one transient session for all connections over 5 minutes, then ditch it and roll to another one.

The higher level code in net.cpp already logs the peer address just before creating the I2P session:

https://github.com/bitcoin/bitcoin/blob/a8f69541ad53a76d5f69044ba829069d225a4af1/src/net.cpp#L454-L456

So, it should already be possible to link the session id with the peer address, the two messages will be adjacent in the log:

0trying connection foozt3gyxn3icwcn2s473tqayt32ti55e7zlliwusuzlppgagypq.b32.i2p:0 ...
1Creating transient SAM session 15cf78cf17 ...

theStack commented at 10:59 am on August 19, 2022: contributor

Concept ACK

in doc/i2p.md:51 in 47c0d02f12 outdated

46@@ -47,9 +47,26 @@ In a typical situation, this suffices:
47 bitcoind -i2psam=127.0.0.1:7656
48 ```
49 
50-The first time Bitcoin Core connects to the I2P router, its I2P address (and
51-corresponding private key) will be automatically generated and saved in a file
52-named `i2p_private_key` in the Bitcoin Core data directory.
53+The first time Bitcoin Core connects to the I2P router, if
54+`-i2pacceptincoming=1`, then it will automatically generate a persistent I2P

naumenkogs commented at 9:10 am on August 25, 2022:

should we specify here what is default?

vasild commented at 9:42 am on August 25, 2022:

On one hand that would be useful, on the other hand if the source code is changed and this forgotten, they would go out of sync.

This PR has 4 ACKs. Should I open a followup PR with this change after this PR is merged or leave it as is?

naumenkogs commented at 9:58 am on August 26, 2022:

You are right, this is certainly an insufficient reason to invalidate 4 acks.

in doc/i2p.md:66 in 47c0d02f12 outdated

64+In I2P connections, the connection receiver sees the I2P address of the
65+connection initiator. This is unlike the Tor network where the recipient does
66+not know who is connecting to them and can't tell if two connections are from
67+the same peer or not.
68+
69+If an I2P node is not accepting incoming connections, then Bitcoin Core uses

naumenkogs commented at 9:12 am on August 25, 2022:

Not sure I’m reading it correctly. What if it does accept incoming connections? Why don’t do transient for outbound still?

vasild commented at 9:38 am on August 25, 2022:

Yes, I guess, you are reading it correctly :)

What if it does accept incoming connections?

Then for outbound it will use its persistent address.

Why don’t do transient for outbound still?

That is a good question and a subtle one. In earlier incarnations of this PR we did that, but then @mzumsande shot it down :gun:, pointing out that we would self-advertise our persistent address over the outbound connection that uses a transient one, defeating the purpose of using a transient address. OTOH if we do not self-advertise anything, then listening would be in vain because nobody would know our listening (persistent) address because we never told it to anybody. See #25355 (review) for the whole discussion.

naumenkogs commented at 9:12 am on August 25, 2022: member

Concept ACK, light code review ACK 47c0d02f126c73755288c3084402098567964329

achow101 merged this on Aug 26, 2022

achow101 closed this on Aug 26, 2022

sidhujag referenced this in commit 725b515355 on Aug 26, 2022

vasild deleted the branch on Aug 28, 2022

mzumsande commented at 10:45 pm on September 6, 2022: contributor

Post-Merge comment: There is no longer a -i2ptransientout option, it was forgotten to update the PR description according to the discussion. Since people will refer to this PR (e.g. in the OpTech newsletter, BitDevs etc.), I think it would be good to update the text of the OP post-merge.

vasild commented at 11:28 am on September 7, 2022: contributor

Updated, thanks!

fanquake commented at 7:09 am on September 12, 2022: member

Another post-merge comment here: https://github.com/bitcoin/bitcoin/commit/2b781ad66e34000037f589c71366c203255ed058#r83655059.

I think you want SIGNATURE_TYPE=7 in here, without it you’re going to get a DSA-SHA1 destination back, at least from Java I2P. See the SAM spec. Not sure what i2pd does. You can tell because a DSA-SHA1 destination will be a few bytes shorter than a Ed25519 one.

bitcoin locked this on Sep 12, 2023

I2P: add support for transient addresses for outbound connections #25355

Background

Persistent vs transient I2P addresses

Conflicts