vasild
commented at 12:49 pm on December 17, 2020:
member
Add I2P support by using the I2P SAM protocol. Unlike Tor, for incoming connections we get the I2P address of the peer (and they also receive ours when we are the connection initiator).
Two new options are added:
0 -i2psam=<ip:port>
1 I2P SAM proxy to reach I2P peers and accept I2P connections (default:
2 none)
3 4 -i2pacceptincoming
5 If set and -i2psam is also set then incoming I2P connections are
6 accepted via the SAM proxy. If this is not set but -i2psam is set
7 then only outgoing connections will be made to the I2P network.
8 Ignored if -i2psam is not set. Notice that listening for incoming
9 I2P connections is done through the SAM proxy, not by binding to
10 a local address and port (default: true)
Overview of the changes
Make ReadBinary() and WriteBinary() reusable
We would need to dump the I2P private key to a file and read it back later. Move those two functions out of torcontrol.cpp.
0util: extract {Read,Write}BinaryFile() to its own files
1util: fix ReadBinaryFile() returning partial contents
2util: fix WriteBinaryFile() claiming success even if error occurred
Split CConnman::AcceptConnection()
Most of CConnman::AcceptConnection() is agnostic of how the socket was accepted. The other part of it deals with the details of the accept(2) system call. Split those so that the protocol-agnostic part can be reused if we accept a socket by other means.
0net: check for invalid socket earlier in CConnman::AcceptConnection()
1net: get the bind address earlier in CConnman::AcceptConnection()
2net: isolate the protocol-agnostic part of CConnman::AcceptConnection()
3net: avoid unnecessary GetBindAddress() call
Just the parts that would enable us to make outgoing and accept incoming I2P connections.
0net: extend CNetAddr::SetSpecial() to support I2P
1net: move the constant maxWait out of InterruptibleRecv()
2net: dedup MSG_NOSIGNAL and MSG_DONTWAIT definitions
3net: extend Sock::Wait() to report a timeout
4net: extend Sock with methods for robust send & read until terminator
5net: extend Sock with a method to check whether connected
6net: implement the necessary parts of the I2P SAM protocol
Use I2P SAM to connect to and accept connections from I2P peers
Profit from all of the preceding commits.
0init: introduce I2P connectivity options
1net: add I2P to the reachability map
2net: make outgoing I2P connections from CConnman
3net: accept incoming I2P connections from CConnman
4net: recognize I2P from ParseNetwork() so that -onlynet=i2p works
5net: Do not skip the I2P network from GetNetworkNames()
DrahtBot added the label
Build system
on Dec 17, 2020
DrahtBot added the label
Docs
on Dec 17, 2020
DrahtBot added the label
P2P
on Dec 17, 2020
DrahtBot added the label
RPC/REST/ZMQ
on Dec 17, 2020
DrahtBot added the label
Utils/log/libs
on Dec 17, 2020
laanwj
commented at 3:36 pm on December 17, 2020:
member
Awesome work, concept ACK.
DrahtBot
commented at 8:13 pm on December 17, 2020:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#19288 (fuzz: Add fuzzing harness for TorController by practicalswift)
#19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)
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.
naumenkogs
commented at 9:05 am on December 18, 2020:
member
Concept ACK
Once this is enabled, we should clearly communicate that using a non-popular overlay has privacy/security side-effects
practicalswift
commented at 3:09 pm on December 18, 2020:
contributor
I like the abstraction (Sock) you’ve added on top of the socket API. I’m adding something similar in #19203 (see MockableSocket and FuzzedSocket) to allow for fuzzing the more low-level parts of our networking code (in this specific case a regression fuzz harness for CVE-2017-18350).
If you have time: please check that PR out. It would be great if your socket abstraction also covered the needs of the the “fuzzed socket” use case. Ideally I’d like to implement my low-level networking code fuzzers using your socket abstraction (something along the lines of class FuzzedSock : public Sock) :)
Aside from of the I2P work I think your socket abstraction would be valuable on a stand-alone PR basis :)
in
src/util/readwritefile.cpp:29
in
a0dc3a2d32outdated
lontivero
commented at 6:58 pm on December 27, 2020:
Q: in case retval contains the maximum allowed data shouldn’t it get out of the loop? I mean, I know this is how the previous version of this method worked but I would like to understand why is this how it is.
Yes, it should get out of the loop and it does due to retval.size() <= maxsize.
It is true that if maxsize==10 and fread() returns 7 bytes and then 5 bytes, this function will return a retval that contains 12 bytes, exceeding maxsize. This is also how it works in master right now.
For the purposes of this PR it suffices to move ReadBinaryFile() out of torcontrol.cpp so that it can be reused by other code. So I tried to keep changes to the minimum - moved the function and only fixed a gross bug (commit util: fix ReadBinaryFile() returning partial contents).
in
src/util/readwritefile.cpp:47
in
a0dc3a2d32outdated
lontivero
commented at 8:43 pm on December 27, 2020:
This makes my, my=, my== and so on until my===== all equivalent. Is this ok? I would have expected partial padding to be invalid.
lontivero
commented at 8:20 pm on December 28, 2020:
Clarification: I am asking this because if this is allow then invalid addresses could be allowed by SetSpecial, I mean, something like udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna=.b32.i2p would be okay.
Hah, good catch! It occurred to me that we need not to support decoding base32 strings without padding in general and instead we can append ==== to the 52 chars before .b32.i2p and decode it as padded base32 string.
This simplifies this PR as I dropped one commit from it: util: support DecodeBase32() without padding. Also, added a check that the address is 52 base32 chars since we expect that adding 4 = symbols will make it multiple of 8. You suggested such a check elsewhere.
lontivero
commented at 8:44 pm on December 27, 2020:
contributor
Is this on Windows? Which compiler version? Is it upset that WSAEAGAIN equals to WSAEWOULDBLOCK and so we end up doing something like err != 5 && err != 5?
I checked that gcc 10.2.1, 9.3.0 and 8.4.0 don’t emit the warning if we do err != 5 && err != 6 && err != 5, so I just reordered the expressions as a simple fixup ro this. Will include in the next push.
jonatack
commented at 10:22 am on December 29, 2020:
jonatack
commented at 4:23 pm on December 29, 2020:
Yes, thank you – tested that it is fixed in 188ba34 and in 2ae504c
DrahtBot added the label
Needs rebase
on Dec 28, 2020
vasild force-pushed
on Dec 29, 2020
vasild
commented at 9:40 am on December 29, 2020:
member
a0dc3a2d3…188ba34b4: rebased to resolve conflicts with master
DrahtBot removed the label
Needs rebase
on Dec 29, 2020
vasild force-pushed
on Dec 29, 2020
vasild
commented at 12:59 pm on December 29, 2020:
member
188ba34b4…f0577c4d7:
disallow partial padding when decoding .b32.i2p addresses
fix gcc warning (hopefully)
vasild force-pushed
on Dec 29, 2020
vasild
commented at 1:27 pm on December 29, 2020:
member
f0577c4d7…2ae504c4b: allow uppercase and mixedcase I2P addresses, suggestion
in
src/netbase.cpp:1144
in
06286e5b83outdated
1140+ // at a time is about 50 times slower.
1141+
1142+ for (;;) {
1143+ char buf[512];
1144+
1145+ const ssize_t peek_ret = recv(socket, buf, sizeof(buf), MSG_PEEK);
lontivero
commented at 10:14 pm on December 30, 2020:
Time ago Winsock used to have problems with peeking from sockets. According to the KB article below (old) recv with MSG_PEEK:
The peek operation will report the number of bytes up until the first buffer boundary. The bytes remaining in the other boundaries might never be reported, resulting in an incorrect count of data for code algorithms that depend upon the peek values to be accurate. Subsequent peek attempts will not reveal the “hidden” data, which can still be received from the buffers.
I don’t know if this is still a problem with winsock2 but even in that case it is probably not a problem this algorithm doesn’t depend on the accuracy of the peek_ret value. Anyway, something to have in mind.
lontivero
commented at 10:15 pm on December 30, 2020:
contributor
Partial review
vasild force-pushed
on Dec 31, 2020
vasild
commented at 2:47 pm on December 31, 2020:
member
2ae504c4b…9445dd490: consider it a timeout if the current time is exactly equal to the deadline
DrahtBot added the label
Needs rebase
on Jan 2, 2021
luke-jr
commented at 0:22 am on January 3, 2021:
member
Do we actually want to share our address, though? I would think an anonymous connection is strictly better?
vasild
commented at 1:39 pm on January 8, 2021:
member
Do we actually want to share our address, though?
In I2P, like in IP, connections have “source address”.
I would think an anonymous connection is strictly better?
Why? In a P2P network peers are supposed to connect to each other, right? Not hide from each other? If we don’t want connections to us, then we don’t listen on the I2P address (-i2pacceptincoming=0) and nobody can reach back.
vasild force-pushed
on Jan 9, 2021
vasild
commented at 12:25 pm on January 9, 2021:
member
9445dd490…1cced4679: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Jan 9, 2021
DrahtBot
commented at 11:46 am on January 13, 2021:
member
🕵️ @harding@hebasto have been requested to review this pull request as specified in the REVIEWERS file.
Yes, if no RESULT is present then this function will throw an exception, which is ok. I think we should expect any reply from the I2P proxy, including malicious one.
I think it is not worth to add isOk() method because it would be used in just one place - here and reply.Get("RESULT") != "OK" is readable enough.
lontivero
commented at 4:55 pm on January 13, 2021:
contributor
Partial review.
felipsoarez
commented at 4:15 pm on January 15, 2021:
none
utACK
jonatack
commented at 4:38 pm on January 15, 2021:
member
Concept ACK. This builds and runs cleanly. If anyone is running an i2p service, ping me on irc to try connecting to each other.
in
src/init.cpp:451
in
1cced46791outdated
446@@ -447,7 +447,9 @@ void SetupServerArgs(NodeContext& node)
447 argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
448 argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h). Limit does not apply to peers with 'download' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
449 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);
450- argsman.AddArg("-onlynet=<net>", "Make outgoing connections only through network <net> (ipv4, ipv6 or onion). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
451+ argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
452+ 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. Notice that listening for incoming I2P connections is done through the SAM proxy, not by binding to a local address and port (default: true)", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
mshalabi1990
commented at 12:03 pm on January 17, 2021:
none
It needs to be annymous to full work or else it will never work fully decentralized
vasild force-pushed
on Jan 17, 2021
vasild
commented at 12:24 pm on January 17, 2021:
member
1cced4679…45e571315: rebase and address suggestions
jonatack
commented at 4:11 pm on January 17, 2021:
member
Stepping through the commits and building one-by-one.
In 87ebd74SocketEvents()interrupt is used like an “in” param but it is passed by reference like an “out” param
0+++ b/src/net.h
1@@ -1069,8 +1069,9 @@ private:
2 * [@param](/bitcoin-bitcoin/contributor/param/)[in,out] sockets When the function is called this is expected to contain the
3 * sockets that should be checked for readiness. Upon return only ready sockets are
4 * left in it (non-ready sockets are removed).
5+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] interrupt Cancel the operation if this is signaled.
6 */
7- void SocketEvents(Sockets& sockets);
8+ void SocketEvents(Sockets& sockets, CThreadInterrupt& interrupt);
jonatack
commented at 4:42 pm on January 17, 2021:
member
Starting from ad56288e9b2f, it seems the various std::chrono::milliseconds timeout params can be passed by value.
jonatack
commented at 5:59 pm on January 17, 2021:
member
Consider dropping a3b33637f as the renaming is to a generic name that is less useful for searching for the variable in the code (false positives go from none to many), or renaming to a more unique name but the current one seems fine; this isn’t new code.
Edit: same feedback for ccc5966a; consider dropping the change or using a slightly less generic name, like permission_flags. This isn’t new code, so you aren’t required to rename the variables.
jonatack
commented at 6:01 pm on January 17, 2021:
member
Reviewed up to a3b33637f234c0f66bacd7afe0fd0dbe, one fixup in addition to the comments above.
in
src/netaddress.cpp:289
in
45e571315aoutdated
283@@ -275,6 +284,33 @@ bool CNetAddr::SetSpecial(const std::string& str)
284 return false;
285 }
286287+bool CNetAddr::SetI2P(const std::string& str)
288+{
289+ // I2P addresses that we support consist of 52 base32 characters + .b32.i2p.
jonatack
commented at 7:48 pm on January 17, 2021:
484dc6559 suggest adding quotes and/or removing the trailing “.” (if you add quotes here, maybe also line 298)
0 // I2P addresses that we support consist of 52 base32 characters + ".b32.i2p"
jonatack
commented at 9:31 pm on January 17, 2021:
member
Reviewed up to 383bb95d49
dunxen
commented at 7:45 am on January 18, 2021:
contributor
Concept ACK. Built and working with no major issues.
vasild force-pushed
on Jan 18, 2021
vasild
commented at 4:04 pm on January 18, 2021:
member
45e571315…b49a4a06e: address review suggestions
SocketEvents() interrupt is used like an “in” param but it is passed by reference like an “out” param
The sleep_for() method of CThreadInterrupt is not const, thus the variable is passed by non-const reference. I changed the comment to say @param[in,out].
Starting from ad56288, it seems the various std::chrono::milliseconds timeout params can be passed by value.
I am not sure those are trivial types like int, double, etc. Thus I pass them by const reference.
Consider dropping a3b3363 as the renaming…
Dropped the two renames, reducing the size of this PR.
Note: if the above were posted as “review comments” attached to some line, then the replies would be next to them and also they could be hidden/collapsed once resolved.
vasild
commented at 4:40 pm on January 18, 2021:
member
A few notes:
We use SAM version 3.1 (HELLO VERSION MIN=3.1 MAX=3.1) because it is the maximum supported by the C++ i2p daemon (as of i2pd 2.35.0).
Ports seem to be ignored, so I default them to 8333. SAM 3.2 defines FROM_PORT and TO_PORT, but we use SAM 3.1. As a consequence we can connect to any port on a given I2P address - once we listen on foo.b32.i2p, a peer can connect to us on foo.b32.i2p:8333, foo.b32.i2p:1234, foo.b32.i2p:80, etc.
RemoveLocal() is not called if the connection to the I2P daemon dies, e.g. the I2P daemon is shut down. In order to stop advertising our I2P address (via other connections, if we have such ones, e.g. IPv4) we need to do that. Probably Session::Accept() needs to be split to two methods: Session:Listen() and Session::Accept().Fixed.
jonatack
commented at 11:53 pm on January 18, 2021:
member
I am not sure those are trivial types like int, double, etc. Thus I pass them by const reference.
IIUC according to https://en.cppreference.com/w/cpp/chrono/duration “The only data stored in a duration is a tick count of type Rep”: std::chrono::milliseconds | duration</*signed integer type of at least 45 bits*/, std::milli>. If that is correct, it would be nice to avoid a situation like with CAmounts, which are a cheaply copied value but are passed around by reference to const all over this codebase, presumably because early on someone thought it wasn’t cheap and then that style was replicated in code changes down the line. Not a huge issue but we may as well start off on the right foot.
vasild force-pushed
on Jan 22, 2021
vasild
commented at 8:41 am on January 22, 2021:
member
b49a4a06e…39021d931:
Split the Accept() method to Listen() and Accept(), so that the caller from net.cpp can do AddLocal() when we are listening and RemoveLocal() when listening fails (e.g. the I2P proxy is shut down).
Do not AddLocal() after we connect to a I2P peer - should only do this when listening. As a result the newly added function AddLocalIfNotKnown() is not needed and thus dropped.
After a failure to listen, accept or connect - check whether the control socket is still connected and if not, then destroy the session. This is not strictly necessary, but makes us detect a possible I2P proxy shutdown earlier.
If -listen=0 is given, then soft-flip -i2pacceptincoming from 1 to 0. Still, if -listen=0 -i2pacceptincoming=1 is explicitly given, then allow that as there is no technical reason it would not work as expected (listen for and accept only I2P connections).
vasild
commented at 1:25 pm on January 22, 2021:
member
39021d931…7da1e29fc: rebase due to conflicts
vasild force-pushed
on Jan 22, 2021
DrahtBot added the label
Needs rebase
on Jan 22, 2021
DrahtBot removed the label
Needs rebase
on Jan 22, 2021
DrahtBot added the label
Needs rebase
on Jan 26, 2021
laanwj
commented at 11:37 am on January 26, 2021:
member
I’m still testing this on one of my nodes. It is working great. I’ve had a few connections to other I2P peers.
I’m testing with the Java implementation of I2P, it has some typical java problems like high CPU and memory use. Compared to Tor at least. Of course, this is not the fault of this PR :slightly_smiling_face:
Another thing (that @jonatack) noted is that I2P has higher latency than Tor onion services. At least I suppose this is an inherent problem with I2P and not the code here. Network latency is not generally a problem for bitcoin but as eviction decisions are made based on (among other things) ping times. This might be something to look into, but not necessarily in this PR, as a first step adding I2P support at all is a good and self-contained change.
jonatack
commented at 1:06 pm on January 26, 2021:
member
Another thing (that @jonatack) noted is that I2P has higher latency than Tor onion services. At least I suppose this is an inherent problem with I2P and not the code here. Network latency is not generally a problem for bitcoin but as eviction decisions are made based on (among other things) ping times. This might be something to look into, but not necessarily in this PR.
lontivero
commented at 2:33 pm on January 28, 2021:
contributor
utACK7da1e29fcc8a4fdff19f24d5be501ce4c0eeaa21
vasild
commented at 10:38 am on January 29, 2021:
member
7da1e29fc…08d0c27d7: rebase due to conflicts
vasild force-pushed
on Jan 29, 2021
vasild force-pushed
on Jan 29, 2021
vasild
commented at 10:41 am on January 29, 2021:
member
08d0c27d7…2caf1bff3: pass std::chrono variables by value instead of by const reference, as per suggestion. Thanks, @jonatack!
DrahtBot removed the label
Needs rebase
on Jan 29, 2021
laanwj removed the label
Build system
on Feb 3, 2021
laanwj removed the label
Docs
on Feb 3, 2021
laanwj removed the label
RPC/REST/ZMQ
on Feb 3, 2021
laanwj removed the label
Utils/log/libs
on Feb 3, 2021
laanwj added the label
Feature
on Feb 3, 2021
jonatack
commented at 5:15 pm on February 3, 2021:
member
I’d like to help move this forward. Is this pull independent of #20788?
laanwj added this to the milestone 22.0
on Feb 3, 2021
lontivero
commented at 2:56 am on February 4, 2021:
contributor
This PR is independent of #20788. However, #20788 was born as a subset of this PR because it adds goodness independently of this PR. Some of the suggestions made there and here (drop commits with renames, for example) have been made here but not in #20788 yet. Anyway, imo i think merging #20788 first could help to reduce the size of this one.
jonatack
commented at 3:13 pm on February 5, 2021:
member
@lontivero thank you, that is very helpful; will finishing reviewing here then as it is ahead of #20788 in its updates. @vasild you can drop my -netinfo commit now that #20764 is merged, sorry for the rebase (but it is now i2p-ready :sunglasses:)
DrahtBot added the label
Needs rebase
on Feb 5, 2021
fluffypony
commented at 3:48 pm on February 5, 2021:
contributor
It’s VERY lightweight, even compared to i2pd and the full i2p router, implements SAM, and bundles a JVM.
vasild force-pushed
on Feb 6, 2021
vasild
commented at 1:52 pm on February 6, 2021:
member
2caf1bff3…2f71ee2b2: rebase due to conflicts and drop the last commit cli: add i2p network to -netinfo because an enhanced version of it is now in master via #20764.
@jonatack, excellent, reducing the size of this PR! Getting #20788 merged will reduce it further.
DrahtBot removed the label
Needs rebase
on Feb 6, 2021
DrahtBot added the label
Needs rebase
on Feb 11, 2021
jonatack
commented at 1:26 pm on February 11, 2021:
member
Noting here some IRC discussions on installing I2P as a reference for later, e.g. a future doc/i2p.md, or for people looking to install it:
DrahtBot removed the label
Needs rebase
on Feb 13, 2021
vasild
commented at 1:17 pm on February 13, 2021:
member
2f71ee2b2…a456bd3f2:
Remove the Sock definition from this PR as it is now merged in master.
Do not extract CConnman::SocketEvents() as a standalone function since it is not needed anymore - in all places we wait for an event on one socket and for this we can use Sock::Wait().
ExtendSock::Wait() to report to the caller whether a timeout occurred or one of the requested events.
Add SendComplete() and RecvUntilTerminator() as Sock methods instead of as standalone functions whose first argument is the socket.
When generating our pub/priv keys (aka I2P destination) use 7 instead of EdDSA_SHA512_Ed25519 since i2pd <2.24.0 only understand the numeric one (thanks to @sdaftuar for nailing this!)
laanwj
commented at 10:37 pm on February 13, 2021:
member
Updated my I2P-using node to the new version (a456bd3f296f18e463e36048c866c404b69363b6) for testing.
jonatack
commented at 0:20 am on February 14, 2021:
member
Testing a Clang 9 build of a456bd3f296f18e463e36048c866c404b69363b6 with i2pd 2.35 did not work
02021-02-13T23:31:35Z I2P: SAM session created: session id=86eaxxaxxxx, my address=zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtyxxxxxxxx.b32.i2p:8333
12021-02-13T23:31:35Z AddLocal(zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtyxxxxxxxx.b32.i2p:8333,2)
2Illegal instruction
0Debian 5.10.13-1 (2021-02-06) x86_64 GNU/Linux
0i2pd version 2.35.0 (0.9.48)
1Boost version 1.74.0
2OpenSSL 1.1.1i 8 Dec 2020
Made two further attempts with the same result :crying_cat_face:…it’s after 1 am and I have not yet looked into the code changes since the last push, but then built with the same config on 2f71ee2b2 (previous push) and it’s working again…:cat:
laanwj
commented at 10:53 am on February 14, 2021:
member
Can you give a gdb backtrace please? With disassembly (disass) at the crash location, if possible. The post common cause of Illegal Instruction is your compiler generating extension instructions that the specific CPU can’t handle. I don’t understand how it could be introduced here though.
jonatack
commented at 2:54 pm on February 14, 2021:
member
Hm, gdb didn’t hit the issue (I’ll try again) but valgrind did.
02021-02-14T14:45:19Z New outbound peer connected: version: 70015, blocks=670586, peer=1, peeraddr=144.76.81.194:8333 (outbound-full-relay)
12021-02-14T14:45:39Z I2P: SAM session created: session id=qqqq, my address=qqqq.b32.i2p:8333 22021-02-14T14:45:41Z UpdateTip: new best=00000000000000000009e0b532bf3ff388c9d7c4489b0bcf2631f75195609c57 height=670580 version=0x3fff0000 log2_work=92.668088 tx=616177992 date='2021-02-14T13:15:07Z' progress=0.999969 cache=1.5MiB(10863txo)
32021-02-14T14:45:41Z AddLocal(qqqq.b32.i2p:8333,2)
4==5137== valgrind: Unrecognised instruction at address 0x17e85a. 5==5137== at 0x17E85A: CConnman::ConnectNode(CAddress, char const*, bool, ConnectionType) (net.cpp:437)
6==5137== by 0x188529: CConnman::OpenNetworkConnection(CAddress const&, bool, CSemaphoreGrant*, char const*, ConnectionType) (net.cpp:2152)
7==5137== by 0x192136: CConnman::ThreadOpenAddedConnections() (net.cpp:2119)
8==5137== by 0x1AABAD: __invoke_impl<void, void (CConnman::*&)(), CConnman *&> (invoke.h:73)
9==5137== by 0x1AABAD: __invoke<void (CConnman::*&)(), CConnman *&> (invoke.h:95)
10==5137== by 0x1AABAD: __call<void, 0> (functional:416)
11==5137== by 0x1AABAD: operator()<, void> (functional:499)
12==5137== by 0x1AABAD: __invoke_impl<void, std::_Bind<void (CConnman::*(CConnman *))()>&> (invoke.h:60)
13==5137== by 0x1AABAD: __invoke_r<void, std::_Bind<void (CConnman::*(CConnman *))()>&> (invoke.h:110)
14==5137== by 0x1AABAD: std::_Function_handler<void (), std::_Bind<void (CConnman::*(CConnman*))()>>::_M_invoke(std::_Any_data const&) (std_function.h:291)
15==5137== by 0x16A038: operator() (std_function.h:622)
16==5137== by 0x16A038: void TraceThread<std::function<void ()>>(char const*, std::function<void ()>) (system.h:470)
17==5137== by 0x1AAD7F: __invoke_impl<void, void (*)(const char *, std::function<void ()>), const char *, std::function<void ()>> (invoke.h:60)
18==5137== by 0x1AAD7F: __invoke<void (*)(const char *, std::function<void ()>), const char *, std::function<void ()>> (invoke.h:95)
19==5137== by 0x1AAD7F: _M_invoke<0, 1, 2> (thread:264)
20==5137== by 0x1AAD7F: operator() (thread:271)
21==5137== by 0x1AAD7F: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void ()>), char const*, std::function<void ()>>>>::_M_run() (thread:215)
22==5137== by 0x4DE1ECF: ??? (in/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
23==5137== by 0x4875EA6: start_thread (pthread_create.c:477)
24==5137== by 0x513DDEE: clone (clone.S:95)
25==5137== Your program just tried to execute an instruction that Valgrind
26==5137== did not recognise. There are two possible reasons for this.27==5137==1. Your program has a bug and erroneously jumped to a non-code
28==5137== location. If you are running Memcheck and you just saw a
29==5137== warning about a bad jump, it's probably your program's fault.30==5137==2. The instruction is legitimate but Valgrind doesn't handle it,31==5137== i.e. it's Valgrind's fault. If you think this is the caseor32==5137== you are not sure, please let us know and we'll try to fix it.33==5137== Either way, Valgrind will now raise a SIGILL signal which will
34==5137== probably kill your program.35==5137==36==5137== Process terminating with default action of signal4 (SIGILL)
37==5137== Illegal opcode at address 0x17E85A38==5137== at 0x17E85A: CConnman::ConnectNode(CAddress, char const*, bool, ConnectionType) (net.cpp:437)
39==5137== by 0x188529: CConnman::OpenNetworkConnection(CAddress const&, bool, CSemaphoreGrant*, char const*, ConnectionType) (net.cpp:2152)
40==5137== by 0x192136: CConnman::ThreadOpenAddedConnections() (net.cpp:2119)
41==5137== by 0x1AABAD: __invoke_impl<void, void (CConnman::*&)(), CConnman *&> (invoke.h:73)
42==5137== by 0x1AABAD: __invoke<void (CConnman::*&)(), CConnman *&> (invoke.h:95)
43==5137== by 0x1AABAD: __call<void, 0> (functional:416)
44==5137== by 0x1AABAD: operator()<, void> (functional:499)
45==5137== by 0x1AABAD: __invoke_impl<void, std::_Bind<void (CConnman::*(CConnman *))()>&> (invoke.h:60)
46==5137== by 0x1AABAD: __invoke_r<void, std::_Bind<void (CConnman::*(CConnman *))()>&> (invoke.h:110)
47==5137== by 0x1AABAD: std::_Function_handler<void (), std::_Bind<void (CConnman::*(CConnman*))()>>::_M_invoke(std::_Any_data const&) (std_function.h:291)
48==5137== by 0x16A038: operator() (std_function.h:622)
49==5137== by 0x16A038: void TraceThread<std::function<void ()>>(char const*, std::function<void ()>) (system.h:470)
50==5137== by 0x1AAD7F: __invoke_impl<void, void (*)(const char *, std::function<void ()>), const char *, std::function<void ()>> (invoke.h:60)
51==5137== by 0x1AAD7F: __invoke<void (*)(const char *, std::function<void ()>), const char *, std::function<void ()>> (invoke.h:95)
52==5137== by 0x1AAD7F: _M_invoke<0, 1, 2> (thread:264)
53==5137== by 0x1AAD7F: operator() (thread:271)
54==5137== by 0x1AAD7F: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void ()>), char const*, std::function<void ()>>>>::_M_run() (thread:215)
55==5137== by 0x4DE1ECF: ??? (in/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
56==5137== by 0x4875EA6: start_thread (pthread_create.c:477)
57==5137== by 0x513DDEE: clone (clone.S:95)
58==5137==59==5137== HEAP SUMMARY:
60==5137==in use at exit: 238,728,213 bytes in1,629,048 blocks
61==5137== total heap usage: 9,884,111 allocs, 8,255,063 frees, 4,818,980,752 bytes allocated
62==5137==63==5137== LEAK SUMMARY:
64==5137== definitely lost: 562,000 bytes in138 blocks
65==5137== indirectly lost: 0 bytes in0 blocks
66==5137== possibly lost: 7,987,784 bytes in414 blocks
67==5137== still reachable: 230,178,429 bytes in1,628,496 blocks
68==5137== of which reachable via heuristic:
69==5137== length64 : 213,760 bytes in360 blocks
70==5137== newarray : 72 bytes in1 blocks
71==5137== suppressed: 0 bytes in0 blocks
72==5137== Rerun with --leak-check=full to see details of leaked memory
73==5137==74==5137== Use --track-origins=yes to see where uninitialised values come from
75==5137== For lists of detected and suppressed errors, rerun with: -s
76==5137== ERROR SUMMARY: 9 errors from 1 contexts (suppressed: 0 from 0)
77Illegal instruction
jonatack
commented at 3:57 pm on February 14, 2021:
member
Ok, hit it with gdb
02021-02-14T15:33:36Z I2P: SAM session created: session id=qqqq, my address=h3r6bkn4...q.b32.i2p:833312021-02-14T15:33:36Z AddLocal(h3r6bkn4...q.b32.i2p:8333,2)
23Thread18"b-addcon" received signal SIGSEGV, Segmentation fault.4[Switching to Thread0x7fff39ed7700 (LWP 54120)]
50x0000555555621edein CConnman::ConnectNode (this=0x5555566e3ec0, addrConnect=...,
6 pszDest=0x7fff28002e50"h3r6bkn4...q.b32.i2p", fCountFailure=false, conn_type=ConnectionType::MANUAL) at net.cpp:4377437*sock = std::move(conn.sock);
0(gdb) bt
1[#0](/bitcoin-bitcoin/0/) 0x0000555555621ede in CConnman::ConnectNode (this=0x5555566e3ec0, addrConnect=..., 2 pszDest=0x7fff28002e50"qqqq.b32.i2p", fCountFailure=false, conn_type=ConnectionType::MANUAL) at net.cpp:437 3[#1](/bitcoin-bitcoin/1/) 0x0000555555629c1e in CConnman::OpenNetworkConnection (this=0x5555566e3ec0, addrConnect=..., fCountFailure=false, grantOutbound=0x7fff39ed6830, 4 pszDest=0x7fff28002e50"qqqq.b32.i2p", conn_type=ConnectionType::MANUAL) at net.cpp:2152 5[#2](/bitcoin-bitcoin/2/) 0x000055555563195a in CConnman::ThreadOpenAddedConnections (this=0x5555566e3ec0) at net.cpp:2119 6[#3](/bitcoin-bitcoin/3/) 0x00005555556902c8 in std::__invoke_impl<void, void (CConnman::*&)(), CConnman*&> ( 7 __f=@0x5555603a3f60: (void (CConnman::*)(CConnman *const)) 0x5555556317c0<CConnman::ThreadOpenAddedConnections()>, __t=@0x5555603a3f70: 0x5555566e3ec0)
8 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:73 9[#4](/bitcoin-bitcoin/4/) 0x0000555555690163 in std::__invoke<void (CConnman::*&)(), CConnman*&> (10 __fn=@0x5555603a3f60: (void (CConnman::*)(CConnman *const)) 0x5555556317c0<CConnman::ThreadOpenAddedConnections()>, __args=@0x5555603a3f70: 0x5555566e3ec0)
11 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:9512[#5](/bitcoin-bitcoin/5/) 0x00005555556900f4 in std::_Bind<void (CConnman::*(CConnman*))()>::__call<void, , 0ul>(std::tuple<>&&, std::_Index_tuple<0ul>) (this=0x5555603a3f60, __args=...)13 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/functional:41614[#6](/bitcoin-bitcoin/6/) 0x0000555555690077 in std::_Bind<void (CConnman::*(CConnman*))()>::operator()<, void>() (this=0x5555603a3f60)15 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/functional:49916[#7](/bitcoin-bitcoin/7/) 0x000055555568ffce in std::__invoke_impl<void, std::_Bind<void (CConnman::*(CConnman*))()>&>(std::__invoke_other, std::_Bind<void (CConnman::*(CConnman*))()>&) (17 __f=...) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:6018[#8](/bitcoin-bitcoin/8/) 0x000055555568ff1e in std::__invoke_r<void, std::_Bind<void (CConnman::*(CConnman*))()>&>(std::_Bind<void (CConnman::*(CConnman*))()>&) (__fn=...)19 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:11020[#9](/bitcoin-bitcoin/9/) 0x000055555568fbae in std::_Function_handler<void (), std::_Bind<void (CConnman::*(CConnman*))()> >::_M_invoke(std::_Any_data const&) (__functor=...)21 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:29122[#10](/bitcoin-bitcoin/10/) 0x0000555555614dff in std::function<void ()>::operator()() const (this=0x7fff39ed6b48)23 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/std_function.h:62224[#11](/bitcoin-bitcoin/11/) 0x00005555555d4f40 in TraceThread<std::function<void ()> >(char const*, std::function<void ()>) (name=0x55555611dc37 "addcon", func=...) at ./util/system.h:47025[#12](/bitcoin-bitcoin/12/) 0x0000555555691182 in std::__invoke_impl<void, void (*)(char const*, std::function<void ()>), char const*, std::function<void ()> >(std::__invoke_other, void (*&&)(char const*, std::function<void ()>), char const*&&, std::function<void ()>&&) (26 __f=@0x5555603a41f0: 0x5555555d4eb0<TraceThread<std::function<void ()>>(char const*, std::function<void ()>)>, __args=..., __args=...)
27 at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:6028[#13](/bitcoin-bitcoin/13/) 0x0000555555690f78 in std::__invoke<void (*)(char const*, std::function<void ()>), char const*, std::function<void ()> >(void (*&&)(char const*, std::function<void ()>), char const*&&, std::function<void ()>&&) (__fn=@0x5555603a41f0: 0x5555555d4eb0 <TraceThread<std::function<void ()> >(char const*, std::function<void ()>)>, 29 __args=..., __args=...) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:9530[#14](/bitcoin-bitcoin/14/) 0x0000555555690eff in std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void ()>), char const*, std::function<void ()> > >::_M_invoke<0ul, 1ul, 2ul>(std::_Index_tuple<0ul, 1ul, 2ul>) (this=0x5555603a41c8) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/thread:26431[#15](/bitcoin-bitcoin/15/) 0x0000555555690e66 in std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void ()>), char const*, std::function<void ()> > >::operator()() (32 this=0x5555603a41c8) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/thread:27133[#16](/bitcoin-bitcoin/16/) 0x00005555556909ed in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void ()>), char const*, std::function<void ()> > > >::_M_run() (this=0x5555603a41c0) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/thread:21534[#17](/bitcoin-bitcoin/17/) 0x00007ffff79ffed0 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.635[#18](/bitcoin-bitcoin/18/) 0x00007ffff7f8aea7 in start_thread (arg=<optimized out>) at pthread_create.c:47736[#19](/bitcoin-bitcoin/19/) 0x00007ffff7709def in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Yes, that is an invalid way to assign a new value to unique_ptr that is not set. :bomb: Fixed!
jonatack
commented at 9:37 am on February 15, 2021:
Makes sense. Building the new push.
laanwj
commented at 7:24 pm on February 14, 2021:
member
So it hits an illegal instruction on this line:
0 0x0000555555621ede <+2638>: mov (%rax),%rcx
This is really strange. That looks like a perfectly basic x86 instruction. I could understand a segmentation fault here, but illegal instruction is strange. At least it is in code that is affected by this PR.
jonatack
commented at 9:01 am on February 15, 2021:
member
The previous tests were with clang 9; seeing the same segfault with gcc 10.2.1:
DrahtBot removed the label
Needs rebase
on Feb 15, 2021
vasild force-pushed
on Feb 16, 2021
vasild
commented at 3:18 pm on February 16, 2021:
member
433d9a9f1...2a7bb343a: fix failing rpc_net.py
laanwj
commented at 8:02 am on February 17, 2021:
member
I’m trying with i2pd now on FreeBSD. Not having much luck. I could install it and start it quite easily (much more easily than the Java one):
0$ pkg install i2pd
1# add i2pd_enable="YES" and optionally i2pd_flags="--bandwidth X"
2$ vi /etc/rc.conf
3$ service i2pd start
4$ i2pd --version
5i2pd version 2.33.0 (0.9.47)
That’s it. SAM is enabled on port 7656.
Edit: below issue is fixed
However, it doesn’t seem to succesfully start the I2P listener. It looks like it hangs after sending the session create, doesn’t get a reply.
02021-02-17T07:52:35Z I2P: Creating SAM session with 127.0.0.1:7656
1⋮
22021-02-17T07:55:35Z I2P: Error listening: Receive timeout (received 0 bytes without terminator before that)
32021-02-17T07:55:35Z I2P: Control socket error: not connected
On the i2pd side:
008:55:36@682/error - NetDb: runtime exception: No known routers, reseed seems to be totally failed
108:55:36@270/debug - SAM: new connection from 127.0.0.1:14315
208:55:36@270/debug - SAM: handshake HELLO VERSION MIN=3.1 MAX=3.1
308:55:36@270/debug - SAM: session create: STYLE=STREAM ID=b66590c3b4 DESTINATION=[snip]
408:55:36@270/warn - Clients: Local destination [snip].b32.i2p exists
508:55:37@7/warn - Tunnel: can't find any router, skip creating tunnel
608:55:37@7/debug - Tunnels: Creating destination outbound tunnel...
7⋮
809:00:07@7/error - Tunnels: Can't create inbound tunnel, no peers available
909:00:07@7/debug - Tunnels: Creating destination inbound tunnel...
1009:00:07@7/error - Tunnels: Can't select next hop for G[snip]=
1109:00:07@7/error - Tunnels: Can't create inbound tunnel, no peers available
The latter part keeps repeating and repeating. It looks like a problem on the i2pd side not the bitcoind one. I have no networking issues, IPv4 and IPv6 and Tor works on the host. Anyone with ideas?
jonatack
commented at 9:03 am on February 17, 2021:
member
If this is the issue Suhas encountered, upgrade to i2pd v2.35 (latest release, build from source or on Debian it’s in the testing sources).
laanwj
commented at 9:07 am on February 17, 2021:
member
I don’t think it’s the same issue. He had a much older version (2.23 instead of 2.33), which had problems handling the commands from bitcoind. My i2pd seems completely broken, it doesn’t get any peer connections.
The whole “runtime exception: No known routers, reseed seems to be totally failed” message gives only one result on Google, some Russian github thread. Argh.
Edit: oh, it could have been something with clock skew. After running ntpdate europe.pool.ntp.org and restarting the service it seems to be doing more!
010:14:29@122/error - SSU: clock skew detected 4294967197. Check your clock
Edit.2 yes, that fixed it!:
02021-02-17T09:21:44Z I2P: SAM session created: session id=be83961cec, my address=[snip].b32.i2p:8333
12021-02-17T09:21:44Z AddLocal([snip].b32.i2p:8333,2)
laanwj
commented at 8:45 am on February 22, 2021:
member
Code review and extensively manually tested ACK2a7bb343ac77f2bf52ea1a8959a22ef266d49aa6
I think it would be nice to have unit tests for the Reply parsing in i2p.cpp and maybe the alt-Base64 parsing, however this can be done in a later PR (and doesn’t need to hold up progress here). Maybe someone will even look into fuzzing.
jonatack
commented at 11:02 am on February 25, 2021:
member
Update, about halfway through reviewing the commits. Each builds cleanly and no comments so far.
I’ve been testing this these past weeks using i2pd 2.35. Apparently, i2pd 2.36 was released last week and I plan to test with it.
The only oddity I’ve been seeing from the first day of testing until now is inbound I2P peers can regularly have two connections, present for a few minutes. I haven’t looked further into it yet. Here are two screenshots taken on February 14 and 20:
ghost
commented at 8:15 pm on February 25, 2021:
none
Don’t see any i2p_private_key file created in .bitcoin/testnet3
Logs:
laanwj
commented at 8:26 pm on February 25, 2021:
member
@prayank23 Strange. Adding debug=i2p to your config might give some more information in the log as to why it’s not working.
The windows linker issue seems unrelated (you could create a new issue for it if you want).
ghost
commented at 9:29 pm on February 25, 2021:
none
laanwj
commented at 9:39 pm on February 25, 2021:
member
It doesn’t look like it’s a SAM proxy you’re connecting to, but a HTTP proxy (it’s clearly speaking HTTP). From what I remember the Java I2P router doesn’t start a SAM service by default and you need to add it.
fanquake
commented at 2:00 am on February 26, 2021:
member
Unable to build for Windows
The Windows linking issue has been fixed in master in #21226. This PR just needs rebasing.
vasild
commented at 4:32 pm on February 26, 2021:
member
@jonatack, next time you observe two incoming connections from the same peer, try to capture the output of bitcoin-cli getpeerinfo |jq 'map(select(.network == "i2p")) |map({inbound: .inbound, addr: .addr, addrbind: .addrbind})'. I suspect it has something to do with the ports.
@prayank23, the SAM proxy is usually listening on port 7656. Surely it can be changed to anything, but I guess you did not change it and your config i2psam=127.0.0.1:7658 is pointing to the HTTP proxy, as @laanwj mentioned.
jonatack
commented at 5:06 pm on February 26, 2021:
member
@prayank23 yes, fwiw I’m building and reviewing the PR after rebasing to current master (let it run last night building and testing each commit and they are all clean/green), as mentioned above, if you do that it should(:tm:) build on Windows.
@vasild sure np. Should finish reviewing the current branch today.
in
src/netaddress.h:156
in
2a7bb343acoutdated
150@@ -151,7 +151,16 @@ class CNetAddr
151152 bool SetInternal(const std::string& name);
153154- bool SetSpecial(const std::string &strName); // for Tor addresses
155+ /**
156+ * Parse a Tor or I2P address and set this object to it.
157+ * @param[in] name Address to parse, for example
jonatack
commented at 5:56 pm on February 26, 2021:
2ee63a5 For the six SetSpecial, SetTor and SetI2P declarations and definitions, I think it would be clearer to use the same param name. It’s currently sometimes name and sometimes str and different between the declarations and the definitions for SetTor and SetI2P. (I’d propose const std::string& addr for all six.)
167+ "Send interrupted (sent only %u of %u bytes before that)", sent, data.size()));
168+ }
169+
170+ // Wait for a short while (or the socket to become ready for sending) before retrying
171+ // if nothing was sent.
172+ const auto timeout = std::min(deadline - now, std::chrono::milliseconds{MAX_WAIT_FOR_IO});
jonatack
commented at 7:57 pm on February 26, 2021:
45cfaf0d296b6b6f9d4c86d4ec1b4c4392947b36 here and also line 248, maybe use a different name for the timeout local variable (maybe wait_time or wait_timeout) than the timeout input param
183+ bool terminator_found = false;
184+
185+ // We must not consume any bytes past the terminator from the socket.
186+ // One option is to read one byte at a time and check if we have read a terminator.
187+ // However that is very slow. Instead, we peek at what is in the socket and only read
188+ // as much bytes as possible without crossing the terminator.
jonatack
commented at 7:59 pm on February 26, 2021:
45cfaf0
0 // as many bytes as possible without crossing the terminator.
(In general, in these two added functions Sock::SendComplete() and Sock::RecvUntilTerminator(), I would personally find it more clear and reassuring if uniform initialization with explicit typing was used.)
In this case we can use try_len which is size_t and we already checked that it equals to read_ret just above this snippet.
in
src/i2p.h:198
in
2a7bb343acoutdated
193+ * @throws std::runtime_error if an error occurs
194+ */
195+ void CreateIfNotCreatedAlready() EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
196+
197+ /**
198+ * Open a new connection to the SAM proxy and issue "STREAM ACCEPT" request using the existent
jonatack
commented at 9:05 pm on February 26, 2021:
6da5c7eb32b51ab89f27a84e6c5e33db0e8e3e6e
0 * Open a new connection to the SAM proxy and issue "STREAM ACCEPT" request using the existing
445@@ -446,7 +446,9 @@ void SetupServerArgs(NodeContext& node)
446 argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
447 argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h). Limit does not apply to peers with 'download' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
448 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);
449- argsman.AddArg("-onlynet=<net>", "Make outgoing connections only through network <net> (" + Join(GetNetworkNames(), ", ") + "). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks. Warning: if it is used with ipv4 or ipv6 but not onion and the -onion or -proxy option is set, then outbound onion connections will still be made; use -noonion or -onion=0 to disable outbound onion connections in this case.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
450+ argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
451+ 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. Notice that listening for incoming I2P connections is done through the SAM proxy, not by binding to a local address and port (default: 1)", ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
jonatack
commented at 10:19 pm on February 26, 2021:
300a654feb8cc68fc12bc8baff0f69ef031bd51f s/Notice/Note/ (or just omit “Notice that”)
0 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_BOOL, OptionsCategory::CONNECTION);
@luke-jr I think it’s the opposite, brace (uniform) initialization raises compile time warnings, e.g. for narrowing and conversions, and named casts provide compile time checks as well.
“Use brace initialization to convert arithmetic types (e.g., int64{x}). This is the safest approach because code will not compile if conversion can result in information loss. The syntax is also concise. Use static_cast as the equivalent of a C-style cast that does value conversion.”
“Brace initialization makes it clear that the type conversion was intended and also prevents conversions between types that might result in loss of precision. (It is a compilation error to try to initialize a float from a double in this fashion, for example.)”
The thing is that here GetDefaultPort() returns int and CService constructor takes uint16_t argument. So we would get a justified warning that we want to silence. The proper fix would be to change GetDefaultPort() to return uint16_t which is out of the scope of this PR.
I don’t see a point in using a brace initialization for the warning and then to silence the warning with a static_cast.
0int x =300;
12// no warning
3uint16_t c1 = x;
45// non-constant-expression cannot be narrowed from type 'int' to 'uint16_t'
6uint16_t c2{x};
78// no warning
9uint16_t c3{static_cast<uint16_t>(x)};
I agree it can be considered out of scope to change GetDefaultPort(), but the suggested (ugly!) change would make explicit the type mismatch and maybe encourage improving it. As mentioned in the parent comment, none of these are blockers, just items I noted while reviewing.
Proposed #21328 to address it without requiring any changes here.
jonatack
commented at 7:48 pm on February 27, 2021:
member
ACK2a7bb343ac77f2bf52ea1a8959a22ef266d49aa6 modulo a few comments below and more suggestions in the top commit at https://github.com/jonatack/bitcoin/commits/pr20685-review. In addition, it looks like src/i2p.{h,cpp} could benefit from unit test coverage. After rebasing to current master at b54a10e777f91208, I checked that each of the commits is hygienic, e.g. debug-builds cleanly with clang 9 and unit tests pass. I have also been running a node on this branch on and off since a few months with i2pd 2.35.0, and apart from the same peer being able to make more than one inbound connection, it appears to be running well and the 2 outbound and 1 inbound I2P peer connections I’ve been seeing appear to be at least as robust as the other ones, though generally with higher ping times (which I believe is inherent to the I2P network and not related to this change).
None of my suggestions below or in the linked branch are blockers but I’m happy to re-review very responsively if you update. That said, this seems to be working robustly and I think it would be good to merge this soon and not too late in this release cycle, with test coverage added in a follow-up.
practicalswift
commented at 7:53 pm on February 28, 2021:
contributor
I think it would be nice to have unit tests for the Reply parsing in i2p.cpp and maybe the alt-Base64 parsing, however this can be done in a later PR (and doesn’t need to hold up progress here). Maybe someone will even look into fuzzing.
Agreed! I plan to add a fuzzing harness for the I2P Reply parsing code (unless @vasild plans to do it as part of this PR of course :)).
Shameless plug: a fuzzing harness for the Tor controller reply parsing code was submitted in June last year in the still open PR #19288. Review welcome! :)
ghost
commented at 10:14 pm on February 28, 2021:
none
Finally i2p_private_key created :)
It was port: 7656 and I had to use i2pd instead of i2prouter. Thanks for the help @laanwj@vasild@jonatack@fanquake Thanks. Will try to build for Windows. I think we will also need a i2p.md file in docs for everyone to setup i2p service for Bitcoin Core.
jonatack
commented at 10:27 pm on February 28, 2021:
member
I think we will also need a i2p.md file in docs for everyone to setup i2p service for Bitcoin Core.
@prayank23 yes, discussed above at #20685 (comment)
laanwj
commented at 10:59 am on March 1, 2021:
member
I would recommend against extending the scope of this PR.
Let’s make sure that this change is correct, address remaining comments, and then merge it. It’s useful to have this on the branch so that other people can work on it.
Documentation and additional testing can be added in a later PR.
jonatack
commented at 11:27 am on March 1, 2021:
member
It’s useful to have this on the branch so that other people can work on it.
util: extract {Read,Write}BinaryFile() to its own files
Extract `ReadBinaryFile()` and `WriteBinaryFile()` from `torcontrol.cpp`
to its own `readwritefile.{h,cpp}` files, so that it can be reused from
other modules.
If an error occurs and `fread()` returns `0` (nothing was read) then the
code before this patch would have returned "success" with a partially
read contents of the file.
8b6e4b3b23
util: fix WriteBinaryFile() claiming success even if error occurred
`fclose()` is flushing any buffered data to disk, so if it fails then
that could mean that the data was not completely written to disk.
Thus, check if `fclose()` succeeds and only then claim success from
`WriteBinaryFile()`.
545bc5f81d
net: check for invalid socket earlier in CConnman::AcceptConnection()
This check is related to an `accept()` failure. So do the check earlier,
closer to the `accept()` call.
This will allow to isolate the `accept()`-specific code at the beginning
of `CConnman::AcceptConnection()` and reuse the code that follows it.
25605895af
net: get the bind address earlier in CConnman::AcceptConnection()
Call `GetBindAddress()` earlier in `CConnman::AcceptConnection()`. That
is specific to the TCP protocol and makes the code below it reusable for
other protocols, if the caller provides `addr_bind`, retrieved by other
means.
1f75a653dd
net: isolate the protocol-agnostic part of CConnman::AcceptConnection()
Isolate the second half of `CConnman::AcceptConnection()` into a new
separate method, which could be reused if we accept incoming connections
by other means than `accept()` (first half of
`CConnman::AcceptConnection()`).
7c224fdac4
net: avoid unnecessary GetBindAddress() call
Our local (bind) address is already saved in `CNode::addrBind` and there
is no need to re-retrieve it again with `GetBindAddress()`.
Also, for I2P connections `CNode::addrBind` would contain our I2P
address, but `GetBindAddress()` would return something like
`127.0.0.1:RANDOM_PORT`.
f6c267db3b
vasild force-pushed
on Mar 1, 2021
vasild
commented at 12:14 pm on March 1, 2021:
member
2a7bb343a...e41cb7dbd: rebase to fix the windows fuzz build issue
net: extend CNetAddr::SetSpecial() to support I2P
Recognize also I2P addresses in the form `base32hashofpublickey.b32.i2p`
from `CNetAddr::SetSpecial()`.
This makes `Lookup()` support them, which in turn makes it possible to
manually connect to an I2P node by using
`-proxy=i2p_socks5_proxy:port -addnode=i2p_address.b32.i2p:port`
Co-authored-by: Lucas Ontivero <lucasontivero@gmail.com>
cff65c4a27
net: move the constant maxWait out of InterruptibleRecv()
Move `maxWait` out of `InterruptibleRecv()` and rename it to
`MAX_WAIT_FOR_IO` so that it can be reused by other code.
34bcfab562
net: dedup MSG_NOSIGNAL and MSG_DONTWAIT definitions
Deduplicate `MSG_NOSIGNAL` and `MSG_DONTWAIT` definitions from `net.cpp`
and `netbase.cpp` to `compat.h` where they can also be reused by other
code.
78fdfbea66
net: extend Sock::Wait() to report a timeout
Previously `Sock::Wait()` would not have signaled to the caller whether
a timeout or one of the requested events occurred since that was not
needed by any of the callers.
Such functionality will be needed in the I2P implementation, thus extend
the `Sock::Wait()` method.
ea1845315a
net: extend Sock with methods for robust send & read until terminator
Introduce two high level, convenience methods in the `Sock` class:
* `SendComplete()`: keep trying to send the specified data until either
successfully sent all of it, timeout or interrupted.
* `RecvUntilTerminator()`: read until a terminator is encountered (never
after it), timeout or interrupted.
These will be convenient in the I2P SAM implementation.
`SendComplete()` can also be used in the SOCKS5 implementation instead
of calling `send()` directly.
42c779f503
net: extend Sock with a method to check whether connected
This will be convenient in the I2P SAM implementation.
5bac7e45e1
vasild force-pushed
on Mar 1, 2021
vasild
commented at 5:08 pm on March 1, 2021:
member
e41cb7dbd...f181f24ca: address suggestions
(no tests yet)
net: implement the necessary parts of the I2P SAM protocol
Implement the following commands from the I2P SAM protocol:
* HELLO: needed for all of the remaining ones
* DEST GENERATE: to generate our private key and destination
* NAMING LOOKUP: to convert .i2p addresses to destinations
* SESSION CREATE: needed for STREAM CONNECT and STREAM ACCEPT
* STREAM CONNECT: to make outgoing connections
* STREAM ACCEPT: to accept incoming connections
c22daa2ecf
init: introduce I2P connectivity options
Introduce two new options to reach the I2P network:
* `-i2psam=<ip:port>` point to the I2P SAM proxy. If this is set then
the I2P network is considered reachable and we can make outgoing
connections to I2P peers via that proxy. We listen for and accept
incoming connections from I2P peers if the below is set in addition to
`-i2psam=<ip:port>`
* `-i2pacceptincoming` if this is set together with `-i2psam=<ip:port>`
then we accept incoming I2P connections via the I2P SAM proxy.
76c35c60f3
net: add I2P to the reachability map
Update `CNetAddr::GetReachabilityFrom()` to recognize the I2P network so
that we would prefer to advertise our I2P address to I2P peers.
9559bd1404
net: make outgoing I2P connections from CConnman0635233a1e
net: accept incoming I2P connections from CConnmanb905363fa8
net: recognize I2P from ParseNetwork() so that -onlynet=i2p works0181e24439
net: Do not skip the I2P network from GetNetworkNames()
So that help texts include "i2p" in:
* `./bitcoind -help` (in `-onlynet` description)
* `getpeerinfo` RPC
* `getnetworkinfo` RPC
Co-authored-by: Jon Atack <jon@atack.com>
a701fcf01f
vasild force-pushed
on Mar 1, 2021
vasild
commented at 5:30 pm on March 1, 2021:
member
f181f24ca...a701fcf01: pet the linter which seems to be upset by R"(foo "%s" bar")
jonatack approved
jonatack
commented at 6:18 pm on March 1, 2021:
member
re-ACKa701fcf01f3ea9a12e869bfa52321302cf68351c reviewed diff per git range-diff ad89812 2a7bb34 a701fcf, debug built and launched bitcoind with i2pd v2.35 running a dual I2P+Torv3 service with the I2P config settings listed below (did not test onlynet=i2p); operation appears nominal (same as it has been these past weeks), and tested the bitcoind help outputs grepping for -i i2p and the rpc getpeerinfo and getnetworkinfo helps
tested with i2pd on Debian 5.10.13-1 (2021-02-06) x86_64 GNU/Linux
0$ i2pd --version
1i2pd version 2.35.0 (0.9.48)
2Boost version 1.74.0
3OpenSSL 1.1.1i 8 Dec 2020
help output grepping for -i i2p
0 -i2pacceptincoming
1 If set and -i2psam is also set then incoming I2P connections are
2 accepted via the SAM proxy. If this is not set but -i2psam is set
3 then only outgoing connections will be made to the I2P network.
4 Ignored if -i2psam is not set. Listening for incoming I2P
5 connections is done through the SAM proxy, not by binding to a
6 local address and port (default: 1)
7 8 -i2psam=<ip:port>
9 I2P SAM proxy to reach I2P peers and accept I2P connections (default:
10 none)
1112 -onlynet=<net>
13 Make outgoing connections only through network <net> (ipv4, ipv6, onion,
14 i2p). Incoming connections are not affected by this option. This
15 option can be specified multiple times to allow multiple
16 networks. Warning: if it is used with non-onion networks and the
17 -onion or -proxy option is set, then outbound onion connections
18 will still be made; use -noonion or -onion=0 to disable outbound
19 onion connections in this case.
2021 -debug=<category>
22 Output debugging information (default: -nodebug, supplying <category> is
23 optional). If <category> is not supplied or if <category> = 1,
24 output all debugging information. <category> can be: net, tor,
25 mempool, http, bench, zmq, walletdb, rpc, estimatefee, addrman,
26 selectcoins, reindex, cmpctblock, rand, prune, proxy, mempoolrej,
27 libevent, coindb, qt, leveldb, validation, i2p. This option can
28 be specified multiple times to output multiple categories.
I plan to add a fuzzing harness for the I2P Reply parsing code (unless @vasild plans to do it as part of this PR of course :)).
I am adding some basic fuzzing of the i2p::sam::Session public interface using FuzzedSock (not to Reply()). Will open a PR soonish…
That’s great news! Looking forward to reviewing it: don’t hesitate to ping me when ready :)
jonatack
commented at 10:04 am on March 5, 2021:
member
@vasild here are a couple more screenshots of being connected twice to the same I2P peer (one example of inbound+outbound, which can be persistent (and not unique to I2P peers IIRC, here we are each manually addnode-ing each other I believe), and one example of double inbound, which doesn’t usually last more than ~5 minutes AFAICT but happens the most often, I see it a few times a day).
The addrbind in every case is my local I2P address.
vasild
commented at 12:54 pm on March 8, 2021:
member
jonatack
commented at 7:25 pm on March 8, 2021:
member
(did not test onlynet=i2p)
Since writing that, I’ve been running onlynet=i2p with onlynet=onion and it seems to be working well.
MarcoFalke referenced this in commit
18cd0888ef
on Mar 19, 2021
rebroad
commented at 1:48 pm on May 3, 2021:
contributor
@jonatack those i2p pings look much higher than the tor pings. Is that normal? Does i2p offer any advantage over tor?
jonatack
commented at 2:05 pm on May 3, 2021:
member
@rebroad indeed (see #21261). An advantage is potentially better decentralization, network robustness and censorship resistance, e.g. I2P may be operational when Tor isn’t or has degraded operation, like in January and February.
vasild
commented at 2:38 pm on May 3, 2021:
member
In addition, I2P connections have a “source address” - it is certain that the peer who connects from a given I2P address to us possesses the private key that corresponds to that I2P address. This can be used for white-listing “friends”. It is a stronger guarantee than IP addresses (which can be spoofed by e.g. your ISP).
I noticed that when I’m running with -externalip, I don’t get my i2p address listed as a local address (presumably because fDiscover gets defaulted to false and then anything less than LOCAL_MANUAL is ignored in AddLocal()), even though my onion address is unaffected by the setting. Changing this to LOCAL_MANUAL seems to fix this issue.
Sorry I should have been more clear – I was using -externalip with an ipv4 address (this node is listening on an ipv4 address, onion address, and i2p address). However I noticed that with -externalip enabled, the AddLocal for the i2p address didn’t work.
Presumably if I set -discover=1 explicitly then it would have worked? But I was just surprised that there’s a difference between what we do for onion vs i2p.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-14 12:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
