We don't mark RelayTransaction as const. Even though it doesn't mutate
PeerManagerImpl state, it _is_ mutating the internal state of a CNode
object, by updating setInventoryTxToSend. In a subsequent commit, that
field will be moved to the Peer object, which is owned by
PeerMangerImpl.
This requires PeerManagerImpl::ReattemptInitialBroadcast() to no longer
be const.
1f05dbd06d util: Avoid invalid integer negation in ValueFromAmount: make ValueFromAmount(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() (practicalswift)
7cc75c9ba3 util: Avoid invalid integer negation in FormatMoney: make FormatMoney(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() (practicalswift)
Pull request description:
Avoid invalid integer negation in `FormatMoney` and `ValueFromAmount`.
Fixes#20402.
Before this patch:
```
$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
$ make -C src/ test/test_bitcoin
$ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
core_write.cpp:21:29: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
(aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in
test/rpc_tests.cpp(186): error: in "rpc_tests/rpc_format_monetary_values":
check ValueFromAmount(std::numeric_limits<CAmount>::min()).write() == "-92233720368.54775808" has failed
[--92233720368.-54775808 != -92233720368.54775808]
util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
(aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior util/moneystr.cpp:16:34 in
test/util_tests.cpp(1188): error: in "util_tests/util_FormatMoney":
check FormatMoney(std::numeric_limits<CAmount>::min()) == "-92233720368.54775808" has failed
[--92233720368.-54775808 != -92233720368.54775808]
```
After this patch:
```
$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
$ make -C src/ test/test_bitcoin
$ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
```
ACKs for top commit:
laanwj:
re-ACK 1f05dbd06d
Tree-SHA512: 5aaeb8e2178f1597921f53c12bdfc2f3d5993d10c41658dcd25943e54e8cc2116a411bc71d928f890b33bc0b3761a8ee4449b0532bce41125b6c60692808c8c3
7d3343fb8e cli: update -netinfo help doc following the merge of 882ce251 (Jon Atack)
ef614bb408 cli: small -netinfo simplification and performance improvement (Jon Atack)
6b45ef3233 cli: improve -netinfo invalid argument error message (Jon Atack)
3732404afa cli: warn in help that -netinfo is not intended to be a stable API (Jon Atack)
7afdd72258 cli: enable -netinfo help to run without a remote server (Jon Atack)
Pull request description:
A few updates, some per IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-07.html#l-87 with respect to -netinfo:
- enable `-netinfo help` to run without a remote server
- warn in `-netinfo help` that -netinfo is not intended to be a stable API
- improve the -netinfo invalid argument error message
- make a performance improvement and simplification I noticed after the merge of #20764
- update the -netinfo help doc following the merge of #21192
-----
How to test manually: 🔬🧪📈
1. check out and build this branch locally; if you need help, don't hesitate to refer to https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core#pull-down-the-code-locally or https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests
2. while it is compiling, look at the code changes
3. stop signet (if it is running) with `./src/bitcoin-cli -signet stop`
4. once the build is completed, run `./src/bitcoin-cli -signet -netinfo help`
5. the help should be printed even though the signet server is not running
6. near the top you should see the new warning, "This human-readable interface will change regularly and is not intended to be a stable API" as well as a bit more description about the integer argument values.
7. start signet with `./src/bitcoind -signet`
8. test the improved invalid argument error message if you run `./src/bitcoin-cli -signet -netinfo 256` or `./src/bitcoin-cli -signet -netinfo a` (valid values are from 0 to 255)
9. leave review feedback or `ACK <commit hash>` -- done 🍻
ACKs for top commit:
michaelfolkson:
Re-ACK 7d3343fb8e
pinheadmz:
RE-ACK 7d3343fb8e
Tree-SHA512: 28c5e9f295ffccba5c2a70faac4987d45f35d4758cf8f10daa767e83212316c4cfc65930e4066f7ad627e9d15b92d43439d1ba9c2f755dfde61885c6a70aa155
366e3e1f89 fuzz: Add FUZZED_SOCKET_FAKE_LATENCY mode to FuzzedSock to allow for fuzzing timeout logic (practicalswift)
b22d4c1607 fuzz: Add fuzzing harness for Socks5(...) (practicalswift)
Pull request description:
Add [regression fuzz harness](https://twitter.com/kayseesee/status/1205287895923212289) for CVE-2017-18350. This fuzzing harness would have found CVE-2017-18350 within a minute of fuzzing :)
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
vasild:
ACK 366e3e1f89
Tree-SHA512: 5d8e1863b635efd10ccb11678b71472ba1523c3ef16affa7f9cd638635c1a9c307e28f432d5b87eb0c9cd1c3c1aeafbb24fa7ae86fe4e5090fda2e20d542b6ca
a701fcf01f net: Do not skip the I2P network from GetNetworkNames() (Vasil Dimov)
0181e24439 net: recognize I2P from ParseNetwork() so that -onlynet=i2p works (Vasil Dimov)
b905363fa8 net: accept incoming I2P connections from CConnman (Vasil Dimov)
0635233a1e net: make outgoing I2P connections from CConnman (Vasil Dimov)
9559bd1404 net: add I2P to the reachability map (Vasil Dimov)
76c35c60f3 init: introduce I2P connectivity options (Vasil Dimov)
c22daa2ecf net: implement the necessary parts of the I2P SAM protocol (Vasil Dimov)
5bac7e45e1 net: extend Sock with a method to check whether connected (Vasil Dimov)
42c779f503 net: extend Sock with methods for robust send & read until terminator (Vasil Dimov)
ea1845315a net: extend Sock::Wait() to report a timeout (Vasil Dimov)
78fdfbea66 net: dedup MSG_NOSIGNAL and MSG_DONTWAIT definitions (Vasil Dimov)
34bcfab562 net: move the constant maxWait out of InterruptibleRecv() (Vasil Dimov)
cff65c4a27 net: extend CNetAddr::SetSpecial() to support I2P (Vasil Dimov)
f6c267db3b net: avoid unnecessary GetBindAddress() call (Vasil Dimov)
7c224fdac4 net: isolate the protocol-agnostic part of CConnman::AcceptConnection() (Vasil Dimov)
1f75a653dd net: get the bind address earlier in CConnman::AcceptConnection() (Vasil Dimov)
25605895af net: check for invalid socket earlier in CConnman::AcceptConnection() (Vasil Dimov)
545bc5f81d util: fix WriteBinaryFile() claiming success even if error occurred (Vasil Dimov)
8b6e4b3b23 util: fix ReadBinaryFile() returning partial contents (Vasil Dimov)
4cba2fdafa util: extract {Read,Write}BinaryFile() to its own files (Vasil Dimov)
Pull request description:
Add I2P support by using the [I2P SAM](https://geti2p.net/en/docs/api/samv3) 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:
```
-i2psam=<ip:port>
I2P SAM proxy to reach I2P peers and accept I2P connections (default:
none)
-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)
```
# 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`.
```
util: extract {Read,Write}BinaryFile() to its own files
util: fix ReadBinaryFile() returning partial contents
util: 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.
```
net: check for invalid socket earlier in CConnman::AcceptConnection()
net: get the bind address earlier in CConnman::AcceptConnection()
net: isolate the protocol-agnostic part of CConnman::AcceptConnection()
net: avoid unnecessary GetBindAddress() call
```
## Implement the I2P [SAM](https://geti2p.net/en/docs/api/samv3) protocol (not all of it)
Just the parts that would enable us to make outgoing and accept incoming I2P connections.
```
net: extend CNetAddr::SetSpecial() to support I2P
net: move the constant maxWait out of InterruptibleRecv()
net: dedup MSG_NOSIGNAL and MSG_DONTWAIT definitions
net: extend Sock::Wait() to report a timeout
net: extend Sock with methods for robust send & read until terminator
net: extend Sock with a method to check whether connected
net: 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.
```
init: introduce I2P connectivity options
net: add I2P to the reachability map
net: make outgoing I2P connections from CConnman
net: accept incoming I2P connections from CConnman
net: recognize I2P from ParseNetwork() so that -onlynet=i2p works
net: Do not skip the I2P network from GetNetworkNames()
```
ACKs for top commit:
laanwj:
re-ACK a701fcf01f
jonatack:
re-ACK a701fcf01f 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
Tree-SHA512: de42090c9c0bf23b43b5839f5b4fc4b3a2657bde1e45c796b5f3c7bf83cb8ec6ca4278f8a89e45108ece92f9b573cafea3b42a06bc09076b40a196c909b6610e
e3e0a2432c Add benchmark to write JSON into a string (Martin Ankerl)
Pull request description:
The benchmark `BlockToJsonVerbose` only tests generating (and destroying)
the JSON data structure, but serializing into a string is also a
performance critical aspect of the RPC calls.
Extracts test setup into a `struct TestBlockAndIndex`, and uses it in
both `BlockToJsonVerbose` and `BlockToJsonVerboseWrite`.
Also, use `ankerl::nanobench::doNotOptimizeAway` to make sure the compiler
can't optimize the result of the calls away.
Here are benchmark results on my Intel i7-8700:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 71,807,017.00 | 13.93 | 0.4% | 555,782,961.00 | 220,788,645.00 | 2.517 | 102,279,341.00 | 0.4% | 0.80 | `BlockToJsonVerbose`
| 27,916,835.00 | 35.82 | 0.1% | 235,084,034.00 | 89,033,525.00 | 2.640 | 42,911,139.00 | 0.3% | 0.32 | `BlockToJsonVerboseWrite`
ACKs for top commit:
laanwj:
Code review ACK e3e0a2432c
Tree-SHA512: bc4d6d1588d47d4bd7af8e7908e44b8561bc391a2d73eccd7c0aa37fc40e8a9ce1fa1f3c29b416eef24a73c6bce3036839c0bbfe1b8dbd6d1bba3718b7ca5383
So that help texts include "i2p" in:
* `./bitcoind -help` (in `-onlynet` description)
* `getpeerinfo` RPC
* `getnetworkinfo` RPC
Co-authored-by: Jon Atack <jon@atack.com>
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.
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
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.
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.
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>
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`.
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()`).
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.
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.
`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()`.
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.
Extract `ReadBinaryFile()` and `WriteBinaryFile()` from `torcontrol.cpp`
to its own `readwritefile.{h,cpp}` files, so that it can be reused from
other modules.
faa06ecc9c build: Bump minimum Qt version to 5.9.5 (Hennadii Stepanov)
Pull request description:
Close#20104.
ACKs for top commit:
laanwj:
Code review ACK faa06ecc9c
jarolrod:
ACK faa06ecc9c
fanquake:
ACK faa06ecc9c - this should be ok to do now.
Tree-SHA512: 7295472b5fd37ffb30f044e88c39d375a5a5187d3f2d44d4e73d0eb0c7fd923cf9949c2ddab6cddd8c5da7e375fff38112b6ea9779da4fecce6f024d05ba9c08
Collects all the orphan handling globals into a single member var in
net_processing, and ensures access is encapuslated into the interface
functions. Also adds doxygen comments for methods.
All the interesting functionality of AddOrphanTx is already in other
functions, so call those functions directly in the one place that
AddOrphanTx was used.
Rather than checking net_processing's internal implementation of
AddOrphanTx, test txorphanage's exported AddTx interface. Note that
this means AddToCompactExtraTransactions is no longer tested here.
EraseOrphansFor was called both with and without g_cs_orphans held,
correct that so that it's always called with it already held.
LimitOrphanTxSize was always called with g_cs_orphans held, so
add annotations and don't lock it a second time.
