I2P: add support for transient addresses for outbound connections #25355
Conversation
|
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). |
|
Concept ACK |
|
Are there some noticable tradeoffs / downsides with this? Why not just enable this by default? |
|
I was just thinking the same. The reason I set the default value of I don't have a strong opinion, maybe it is ok to introduce it as On the low-level, each session requires an extra control socket:
I think this is ok wrt max open sockets on the system because we don't have 100s or 1000s of outgoing connections. |
Concept ACK |
vasild
force-pushed
the
i2p_transient_outbound_addr
branch
from
ab91728
to
9be11f7
Compare
|
|
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
|
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
the
i2p_transient_outbound_addr
branch
from
9be11f7
to
a69528c
Compare
|
|
|
Further idea (out of the scope of this PR): periodically disconnect long lived outbound connections to Tor and to I2P if |
|
It may be desirable to have this in 24.0, feature freeze in mid August. |
|
Will have a look!
… It may be desirable to have this in 24.0, feature freeze in mid August
|
|
|
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.
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.
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.
This way the log messages are consistent with "Creating SAM session..."
vasild
force-pushed
the
i2p_transient_outbound_addr
branch
from
6f3e418
to
59aa54f
Compare
|
Invalidates ACKs from @mzumsande, @achow101 Previously invalidated ACK from @jonatack |
|
ACK 59aa54f (verified via range-diff that just a typo / |
|
re-ACK 59aa54f |
| * Whether this is a transient session (the I2P private key will not be | ||
| * read or written to disk). | ||
| */ | ||
| const bool m_transient; |
There was a problem hiding this comment.
2b781ad Don't make data members const.
| const bool m_transient; | |
| bool m_transient; |
There was a problem hiding this comment.
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.
|
|
||
| m_my_addr = CService(DestBinToAddr(MyDestination()), I2P_SAM31_PORT); | ||
| m_session_id = session_id; | ||
| m_control_sock = std::move(sock); | ||
|
|
||
| LogPrintfCategory(BCLog::I2P, "SAM session created: session id=%s, my address=%s\n", | ||
| m_session_id, m_my_addr.ToString()); | ||
| Log("%s SAM session %s created, my address=%s", |
There was a problem hiding this comment.
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.
2022-08-17T09:54:26Z [i2p] Transient SAM session 74fc489e60 created, my address=4ejzt3gyxn3icwcn2s473tqayt32ti55e7zlliwusuzlppgagypq.b32.i2p:0
2022-08-17T09:54:28Z New outbound peer connected: version: 70016, blocks=749823, peer=8, peeraddr=83.240...6:8333 (outbound-full-relay)
2022-08-17T09:54:28Z New outbound peer connected: version: 70016, blocks=749823, peer=9, peeraddr=190.2...4:8333 (outbound-full-relay)
2022-08-17T09:54:34Z New outbound peer connected: version: 70016, blocks=749823, peer=13, peeraddr=31.42....9:8333 (outbound-full-relay)
2022-08-17T09:54:34Z [i2p] Creating transient SAM session 15cf78cf17 with 127.0.0.1:7656
2022-08-17T09:54:39Z New outbound peer connected: version: 70016, blocks=749823, peer=14, peeraddr=76.89....:8333 (outbound-full-relay)
2022-08-17T09:54:54Z [i2p] Transient SAM session 15cf78cf17 created, my address=vnugpix23m4inijbw24d2s6nha6u3rzqpkfsvz3r2sonxvx7o6ma.b32.i2p:0
2022-08-17T09:54:57Z [i2p] Destroying SAM session 15cf78cf17
2022-08-17T09:54:58Z [i2p] Creating transient SAM session 6d503018cc with 127.0.0.1:7656
2022-08-17T09:55:16Z New outbound peer connected: version: 70016, blocks=749823, peer=15, peeraddr=wwbw7nqr....7tpoqjswvcwa.b32.i2p:0 (manual)
2022-08-17T09:55:18Z [i2p] Transient SAM session 6d503018cc created, my address=h75ad7btqhxbkyapqsrnaprhtots5xxoq7yp6qjl3z77idzazg6q.b32.i2p:0
2022-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.
There was a problem hiding this comment.
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:
Lines 454 to 456 in a8f6954
So, it should already be possible to link the session id with the peer address, the two messages will be adjacent in the log:
trying connection foozt3gyxn3icwcn2s473tqayt32ti55e7zlliwusuzlppgagypq.b32.i2p:0 ...
Creating transient SAM session 15cf78cf17 ...
|
Concept ACK |
| corresponding private key) will be automatically generated and saved in a file | ||
| named `i2p_private_key` in the Bitcoin Core data directory. | ||
| The first time Bitcoin Core connects to the I2P router, if | ||
| `-i2pacceptincoming=1`, then it will automatically generate a persistent I2P |
There was a problem hiding this comment.
should we specify here what is default?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
You are right, this is certainly an insufficient reason to invalidate 4 acks.
| not know who is connecting to them and can't tell if two connections are from | ||
| the same peer or not. | ||
|
|
||
| If an I2P node is not accepting incoming connections, then Bitcoin Core uses |
There was a problem hiding this comment.
Not sure I'm reading it correctly. What if it does accept incoming connections? Why don't do transient for outbound still?
There was a problem hiding this comment.
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 🔫, 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 (comment) for the whole discussion.
|
Concept ACK, light code review ACK 47c0d02 |
…bound connections
|
Post-Merge comment: There is no longer a |
|
Updated, thanks! |
|
Another post-merge comment here: 2b781ad#r83655059.
|
ghost
mentioned this pull request
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=1is 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).