2555a3950f p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta)
Pull request description:
If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted.
Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`.
Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`.
If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration.
ACKs for top commit:
mzumsande:
Code Review ACK 2555a3950f
achow101:
ACK 2555a3950f
vasild:
ACK 2555a3950f
Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3
c9d548c91f net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e9 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20 net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59a scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91f
achow101:
ACK c9d548c91f
jonatack:
re-ACK c9d548c91f only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91f
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
that was resolved in PR27036 "test: Remove last uses of snprintf and simplify"
and while here, fix up 2 words in docs to make the spelling linter green again.
691eaf8873 Pass MSG_MORE flag when sending non-final network messages (Matt Whitlock)
Pull request description:
**N.B.:** This is my second attempt at introducing this optimization. #12519 (2018) was closed in deference to switching to doing gathering socket writes using `sendmsg(2)`, which I agree would have superior performance due to fewer syscalls, but that work was apparently abandoned in late 2018. Ever since, Bitcoin Core has continued writing tons of runt packets to the wire. Can we proceed with my halfway solution for now?
----
Since Nagle's algorithm is disabled, each and every call to `send(2)` can potentially generate a separate TCP segment on the wire. This is especially inefficient when sending the tiny header preceding each message payload.
Linux implements a `MSG_MORE` flag that tells the kernel not to push the passed data immediately to the connected peer but rather to collect it in the socket's internal transmit buffer where it can be combined with data from successive calls to `send(2)`. Where available, specify this flag when calling `send(2)` in `CConnman::SocketSendData(CNode &)` if the data buffer being sent is not the last one in `node.vSendMsg`.
ACKs for top commit:
sipa:
ACK 691eaf8873
vasild:
ACK 691eaf8873
Tree-SHA512: 9a7f46bc12edbf78d488f05d1c46760110a24c95af74b627d2604fcd198fa3f511c5956bac36d0034e88c632d432f7d394147e667a11b027af0a30f70a546d70
dfc01ccd73 net: simplify the call to vProcessMsg.splice() (Vasil Dimov)
Pull request description:
At the time when
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
```
is called, `it` is certainly `pnode->vRecvMsg.end()` which makes the call equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());
```
which is equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);
```
Thus, use the latter. Further, maybe irrelevant, but the latter has constant complexity while the original code is `O(length of vRecvMsg)`.
ACKs for top commit:
theStack:
Code-review ACK dfc01ccd73
MarcoFalke:
review ACK dfc01ccd73🐑
jonatack:
Light review ACK dfc01ccd73
Tree-SHA512: 9f4eb61d1caf4af9a61ba2f54b915fcfe406db62c58ab1ec42f736505b6792e9379a83d0458d6cc04f289edcec070b7c962f94a920ab51701c3cab103152866f
At the time when
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
```
is called, `it` is certainly `pnode->vRecvMsg.end()` which makes the
call equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());
```
which is equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);
```
Thus, use the latter. Further, maybe irrelevant, but the latter has
constant complexity while the original code is `O(length of vRecvMsg)`.
The functionality of the old size() is covered by the new Size()
when no arguments are specified, so this does not change behavior.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Previously, we'd only load fixed seeds if we'd not
know any addresses at all. This change makes it possible
to change -onlynet abruptly, e.g. from -onlynet=onion to
-onlynet=i2p and still find peers.
In the case of `i2pacceptincoming=0` we use transient addresses
(destinations) for ourselves for each outbound connection. It may
happen that we
* create the session (and thus our address/destination too)
* fail to connect to the particular peer (e.g. if they are offline)
* dispose the unused session.
This puts unnecessary load on the I2P network because session creation
is not cheap. Is exaggerated if `onlynet=i2p` is used in which case we
will be trying to connect to I2P peers more often.
To help with this, save the created but unused sessions and pick them
later instead of creating new ones.
Alleviates: https://github.com/bitcoin/bitcoin/issues/26754
Since Nagle's algorithm is disabled, each and every call to send(2) can potentially generate a separate TCP segment on the wire. This is especially inefficient when sending the tiny header preceding each message payload.
Linux implements a MSG_MORE flag that tells the kernel not to push the passed data immediately to the connected peer but rather to collect it in the socket's internal transmit buffer where it can be combined with data from successive calls to send(2). Where available, specify this flag when calling send(2) in CConnman::SocketSendData(CNode &) if the data buffer being sent is not the last one in node.vSendMsg.
Both methods do the same thing, so simplify to having just one.
`ToString()` is too generic in this case and it is unclear what it does,
given that there are similar methods:
`ToStringAddr()` (inherited from `CNetAddr`),
`ToStringPort()` and
`ToStringAddrPort()`.
Both methods do the same thing, so simplify to having just one.
Further, `CService` inherits `CNetAddr` and `CService::ToString()`
overrides `CNetAddr::ToString()` but the latter is not virtual which
may be confusing. Avoid such a confusion by not having non-virtual
methods with the same names in inheritance.
"IP" stands for "Internet Protocol".
"IP address" is sometimes shortened to just "IP" or "address".
However, Tor or I2P addresses are not "IP addresses", nor "IPs".
Thus, use "Addr" instead of "IP" for addresses that could be IP, Tor or
I2P addresses:
`CService::ToStringIPPort()` -> `CService::ToStringAddrPort()`
`CNetAddr::ToStringIP()` -> `CNetAddr::ToStringAddr()`
-BEGIN VERIFY SCRIPT-
sed -i 's/ToStringIPPort/ToStringAddrPort/g' -- $(git grep -l ToStringIPPort src)
sed -i 's/ToStringIP/ToStringAddr/g' -- $(git grep -l ToStringIP src)
-END VERIFY SCRIPT-
b527b54950 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() (Vasil Dimov)
29f66f7682 moveonly: move SetSocketNonBlocking() from netbase to util/sock (Vasil Dimov)
b4bac55679 net: convert standalone IsSelectableSocket() to Sock::IsSelectable() (Vasil Dimov)
5db7d2ca0a moveonly: move IsSelectableSocket() from compat.h to sock.{h,cpp} (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
* convert standalone `IsSelectableSocket()` to `Sock::IsSelectable()`
* convert standalone `SetSocketNonBlocking()` to `Sock::SetNonBlocking()`
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
jonatack:
ACK b527b54950 review/debug build/unit tests at each commit, cross-referenced the changes with `man select` and `man errno`, ran a signet node on the last commit with ip4/ip6//tor/i2p/cjdns and network connections were nominal
dergoegge:
Code review ACK b527b54950
Tree-SHA512: af783ce558c7a89e173f7ab323fb3517103d765c19b5d14de29f64706b4e1fea3653492e8ea73ae972699986aaddf2ae72c7cfaa7dad7614254283083b7d2632
9cbfe40d8a net: remove useless call to IsReachable() from CConnman::Bind() (Vasil Dimov)
Pull request description:
`CConnman::Bind()` is called without `BF_EXPLICIT` only when passed
either `0.0.0.0` or `::`. For those addresses `IsReachable()` is always
true (regardless of the `-onlynet=` setting!), meaning that the `if`
condition never evaluates to true.
`IsReachable()` is always true for the "any" IPv4 and IPv6 addresses
because `CNetAddr::GetNetwork()` returns `NET_UNROUTABLE` instead of
`NET_IPV4` or `NET_IPV6` and the network `NET_UNROUTABLE` is always
considered reachable.
It follows that `BF_EXPLICIT` is unnecessary, remove it too.
ACKs for top commit:
naumenkogs:
ACK 9cbfe40d8a
aureleoules:
ACK 9cbfe40d8a
mzumsande:
ACK 9cbfe40d8a
Tree-SHA512: 4e53ee8a73ddd133fd4ff25635135b65e5c19d1fc56fe5c30337406560664616c0adff414dca47602948919f34c81073aae6bfc2871509f3912663d86750928e
d575a675cc net_processing: add thread safety annotation for m_highest_fast_announce (Anthony Towns)
0ae7987f68 net_processing: add thread safety annotations for PeerManagerImpl members accessed only via the msgproc thread (Anthony Towns)
a66a7ccb82 net_processing: add thread safety annotations for Peer members accessed only via the msgproc thread (Anthony Towns)
bf12abe454 net: drop cs_sendProcessing (Anthony Towns)
1e78f566d5 net: add NetEventsInterface::g_msgproc_mutex (Anthony Towns)
Pull request description:
There are many cases where we assume message processing is single-threaded in order for how we access node-related memory to be safe. Add an explicit mutex that we can use to document this, which allows the compiler to catch any cases where we try to access that memory from other threads and break that assumption.
ACKs for top commit:
MarcoFalke:
review ACK d575a675cc 📽
dergoegge:
Code review ACK d575a675cc
w0xlt:
ACK d575a675cc
vasild:
ACK d575a675cc modulo the missing runtime checks
Tree-SHA512: b886d1aa4adf318ae64e32ccaf3d508dbb79d6eed3f1fa9d8b2ed96f3c72a3d38cd0f12e05826c9832a2a1302988adfd2b43ea9691aa844f37d8f5c37ff20e05
fa521c9603 Use steady clock for all millis bench logging (MacroFake)
Pull request description:
Currently `GetTimeMillis` is used for bench logging in milliseconds integral precision. Replace it to use a steady clock that is type-safe and steady.
Microsecond or float precision can be done in a follow-up.
ACKs for top commit:
fanquake:
ACK fa521c9603 - started making the same change.
Tree-SHA512: 86a810e496fc663f815acb8771a6c770331593715cde85370226685bc50c13e8e987e3c5efd0b4e48b36ebd2372255357b709204bac750d41e94a9f7d9897fa6
There are many cases where we assume message processing is
single-threaded in order for how we access node-related memory to be
safe. Add an explicit mutex that we can use to document this, which allows
the compiler to catch any cases where we try to access that memory from
other threads and break that assumption.
385f5a4c3f p2p: Don't query DNS seeds when both IPv4 and IPv6 are unreachable (Martin Zumsande)
91f0a7fbb7 p2p: add only reachable addresses to addrman (Martin Zumsande)
Pull request description:
Currently, `-onlynet` does not work well in connection with initial peer discovery, because DNS seeds only resolve to IPv6 and IPv4 adresses:
With `-onlynet=i2p`, we would load clearnet addresses from DNS seeds into addrman, be content our addrman isn't empty so we don't try to query hardcoded seeds (although these exist for i2p!), and never attempt to make an automatic outbound connection.
With `-onlynet=onion` and `-proxy` set, we wouldn't load addresses via DNS, but will make AddrFetch connections (through a tor exit node) to a random clearnet peer the DNS seed resolves to (see https://github.com/bitcoin/bitcoin/issues/6808#issuecomment-147652505), thus breaching the `-onlynet` preference of the user - this has been reported in the two issues listed below.
This PR proposes two changes:
1.) Don't load addresses that are unreachable (so that we wouldn't connect to them) into addrman. This is already the case for addresses received via p2p addr messages, this PR implements the same for addresses received from DNS seeds and fixed seeds. This means that in the case of `-onlynet=onion`, we wouldn't load fixed seed IPv4 addresses into addrman, only the onion ones.
2.) Skip trying the DNS seeds if neither IPv4 nor IPv6 are reachable and move directly to adding the hardcoded seeds from networks we can connect to. This is done by soft-setting `-dnsseed` to 0 in this case, unless `-dnsseed=1` was explicitly specified, in which case we abort with an `InitError`.
Fixes#6808Fixes#12344
ACKs for top commit:
naumenkogs:
utACK 385f5a4c3f
vasild:
ACK 385f5a4c3f
Tree-SHA512: 33a8c29faccb2d9b937b017dba4ef72c10e05e458ccf258f1aed3893bcc37c2e984ec8de998d2ecfa54282abbf44a132e97d98bbcc24a0dcf1871566016a9b91
This happens, for example, if the user specified -onlynet=onion or
-onlynet=i2p. DNS seeds only resolve to IPv4 / IPv6 addresses,
making their answers useless to us, since we don't want to make
connections to these.
If, within the DNS seed thread, we'd instead do fallback AddrFetch
connections to one of the clearnet addresses the DNS seed resolves to,
we might get usable addresses from other networks
if lucky, but would be violating our -onlynet user preference
in doing so.
Therefore, in this case it is better to rely on fixed seeds for networks we
want to connect to.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
We will not make outgoing connection to peers that are unreachable
(e.g. because of -onlynet configuration).
Therefore, it makes no sense to add them to addrman in the first place.
While this is already the case for addresses received via p2p addr
messages, this commit does the same for addresses received
from fixed seeds.
Dereferencing a unique_ptr is not necessarily thread safe. The reason
these are safe is because their values are set at construction and do
not change later; so mark them as const and set them via the initializer
list to guarantee that.
The (V1)TransportSerializer instance CNode::m_serializer is used from
multiple threads via PushMessage without protection by a mutex. This
is only thread safe because the class does not have any mutable state,
so document that by marking the methods and the object as "const".
59aa54f731 i2p: log "SAM session" instead of "session" (Vasil Dimov)
d7ec30b648 doc: add release notes about the I2P transient addresses (Vasil Dimov)
47c0d02f12 doc: document I2P transient addresses usage in doc/i2p.md (Vasil Dimov)
3914e472f5 test: add a test that -i2pacceptincoming=0 creates a transient session (Vasil Dimov)
ae1e97ce86 net: use transient I2P session for outbound if -i2pacceptincoming=0 (Vasil Dimov)
a1580a04f5 net: store an optional I2P session in CNode (Vasil Dimov)
2b781ad66e i2p: add support for creating transient sessions (Vasil Dimov)
Pull request description:
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. If this is undesirable, then a node operator can use the newly introduced `-i2ptransientout` to 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.
ACKs for top commit:
mzumsande:
ACK 59aa54f731 (verified via range-diff that just a typo / `unique_ptr` initialisation were fixed)
achow101:
re-ACK 59aa54f731
jonatack:
utACK 59aa54f731 reviewed range diff, rebased to master, debug build + relevant tests + review at each commit
Tree-SHA512: 2be9b9dd7502b2d44a75e095aaece61700766bff9af0a2846c29ca4e152b0a92bdfa30f61e8e32b6edb1225f74f1a78d19b7bf069f00b8f8173e69705414a93e
`CConnman::Bind()` is called without `BF_EXPLICIT` only when passed
either `0.0.0.0` or `::`. For those addresses `IsReachable()` is always
true (regardless of the `-onlynet=` setting!), meaning that the `if`
condition never evaluates to true.
`IsReachable()` is always true for the "any" IPv4 and IPv6 addresses
because `CNetAddr::GetNetwork()` returns `NET_UNROUTABLE` instead of
`NET_IPV4` or `NET_IPV6` and the network `NET_UNROUTABLE` is always
considered reachable.
It follows that `BF_EXPLICIT` is unnecessary, remove it too.
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.
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.
There is no need to use two variables `ret` and `addr` of the same type
`CService` and assign one to the other in a strange way like
`ret = CService{addr}`.
fa74e726c4 refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) (MacroFake)
fa3b3cb9b5 Expose underlying clock in CThreadInterrupt (MacroFake)
Pull request description:
This gets rid of the `value*1000` manual conversion.
ACKs for top commit:
naumenkogs:
utACK fa74e726c4
dergoegge:
Code review ACK fa74e726c4
Tree-SHA512: 90409c05c25f0dd2f1c4dead78f707ebfd78b7d84ea4db9fcefd9c4958a1a3338ac657cd9e99eb8b47d52d4485fa3c947dce4ee1559fb56ae65878685e1ed9a3
facc2fa7b8 Use AutoFile where possible (MacroFake)
6666803c89 streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8
fanquake:
ACK facc2fa7b8
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
b2733ab6a8 net: add new method Sock::Listen() that wraps listen() (Vasil Dimov)
3ad7de225e net: add new method Sock::Bind() that wraps bind() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Add new methods `Sock::Bind()` and `Sock::Listen()` that wrap `bind()` and `listen()`.
This will help to increase `Sock` usage and make more code mockable.
ACKs for top commit:
pk-b2:
ACK b2733ab6a8
laanwj:
Code review ACK b2733ab6a8
Tree-SHA512: c6e737606703e2106fe60cc000cfbbae3a7f43deadb25f70531e2cac0457e0b0581440279d14c76c492eb85c12af4adde52c30baf74542c41597e419817488e8
a8d6abba5e net: change GetBindAddress() to take Sock argument (Vasil Dimov)
748dbcd9f2 net: add new method Sock::GetSockName() that wraps getsockname() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Wrap the syscall `getsockname()` in `Sock::GetSockName()` and change `GetBindAddress()` to take a `Sock` argument so that it can use the wrapper.
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
laanwj:
Code review ACK a8d6abba5e
Tree-SHA512: 3a73463258c0057487fb3fd67215816b03a1c5160f45e45930eaeef86bb3611ec385794cdb08339aa074feba8ad67cd2bfd3836f6cbd40834e15d933214a05dc
99b9e5f3a9 p2p: always set nTime for self-advertisements (Martin Zumsande)
Pull request description:
This logic was recently changed in 0cfc0cd322 to overwrite `addrLocal` with the address they gave us when self-advertising to an inbound peer. But if we don't also change `nTime` again from the default `TIME_INIT`, our peer will not relay our advertised address any further.
ACKs for top commit:
naumenkogs:
ACK 99b9e5f3a9
laanwj:
Code review ACK 99b9e5f3a9
vasild:
ACK 99b9e5f3a9
Tree-SHA512: 4c7ea51cc77ddaa4b3537962ad2ad085f7ef5322982d3b1f5baecb852719eb99dd578436ca63432cb6b0a4fbd8b59fca793caf326c4663a4d6f34301e8146aa2
6e68ccbefe net: use Sock::WaitMany() instead of CConnman::SocketEvents() (Vasil Dimov)
ae263460ba net: introduce Sock::WaitMany() (Vasil Dimov)
cc74459768 net: also wait for exceptional events in Sock::Wait() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
`Sock::Wait()` waits for IO events on one socket. Introduce a similar `virtual` method `WaitMany()` that waits simultaneously for IO events on more than one socket.
Use `WaitMany()` instead of `CConnman::SocketEvents()` (and ditch the latter). Given that the former is a `virtual` method, it can be mocked by unit and fuzz tests. This will help to make bigger parts of `CConnman` testable (unit and fuzz).
ACKs for top commit:
laanwj:
Code review ACK 6e68ccbefe
jonatack:
re-ACK 6e68ccbefe per `git range-diff e18fd47 6747729 6e68ccb`, and verified rebase to master and debug build
Tree-SHA512: 917fb6ad880d64d3af1ebb301c06fbd01afd8ff043f49e4055a088ebed6affb7ffe1dcf59292d822f10de5f323b6d52d557cb081dd7434634995f9148efcf08f
If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.
ce893c0497 doc: Update developer notes (Anthony Towns)
d2852917ee sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37 net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497🐦
hebasto:
ACK ce893c0497
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
Rename `GenerateSelectSet()` to `GenerateWaitSockets()` and adapt it to
generate a wait data suitable for `Sock::WaitMany()`. Then call it from
`CConnman::SocketHandler()` and feed the generated data to
`Sock::WaitMany()`.
This way `CConnman::SocketHandler()` can be unit tested because
`Sock::WaitMany()` is mockable, so the usage of real sockets can be
avoided.
Resolves https://github.com/bitcoin/bitcoin/issues/21744
292828cd77 [test] Test addr cache for multiple onion binds (dergoegge)
3382905bef [net] Seed addr cache randomizer with port from binding address (dergoegge)
f10e80b6e4 [net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed addr cache randomizer (dergoegge)
Pull request description:
The addr cache id randomizer is currently supposed to be seeded with the network of the inbound connection and the local socket (only the address is used not the port): a8098f2cef/src/net.cpp (L2800-L2804)
For inbound onion connections `CNode::addr.GetNetwork()` returns `NET_UNROUTABLE` and `CNode::addrBind` is set to `127.0.0.1:<onion bind port>`. This results in the same addr cache for all inbound connections on 127.0.0.1 binds.
To avoid the same addr cache across all onion and other 127.0.0.1 binds, we should seed the addr cache randomizer with the correct network for inbound onion connections (using `CNode::ConnectedThroughNetwork()`) as well as the port of `CNode::addrBind`.
ACKs for top commit:
sipa:
utACK 292828cd77
mzumsande:
Code Review ACK 292828cd77
naumenkogs:
utACK 292828cd77
Tree-SHA512: d0be13bab6bc121c2926d4b168687f6c2ed4ce0c9dd19be71eb4886adeba8afc3daacdc4e232a0ba3b03a89d69b618abc5595b69abd1ad0c476d825bc6ea1f9f
This is more consistent with the other functions, as well as with the
logging output itself. If we want to make this change, we should do it
before it's all over the place.
e11cdc9303 logging: Add log severity level to net.cpp (klementtan)
a8290649a6 logging: Add severity level to logs. (klementtan)
Pull request description:
**Overview**: This PR introduces a new macro, `LogPrintLevel`, that allows developers to add logs with the severity level. Additionally, it will also print the log category if it is specified.
Sample log:
```
2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XX.XX.XXX.XXX:YYYYY lastseen=2.7hrs
```
**Motivation**: This feature was suggested in #20576 and I believe that it will bring the following benefits:
* Allow for easier filtering of logs in `debug.log`
* Can be extended to allow users to select the minimum level of logs they would like to view (not in the scope of this PR)
**Details**:
* New log format. `... [category:level]...`. ie:
* Do not print category if `category == NONE`
* Do not print level if `level == NONE`
* If `category == NONE` and `level == NONE`, do not print any fields (current behaviour)
* Previous logging functions:
* `LogPrintf`: no changes in log as it calls `LogPrintf_` with `category = NONE` and `level = NONE`
* `LogPrint`: prints additional `[category]` field as it calls `LogPrintf_` with `category = category` and `level = NONE`
* `net.cpp`: As a proof of concept, updated logs with obvious severity (ie prefixed with `Warning/Error:..`) to use the new logging with severity.
**Testing**:
* Compiling and running `bitcoind` with this PR should instantly display logs with the category name (ie `net/tor/...`)
* Grepping for `net:debug` in `debug.log` should display the updated logs with severity level:
<details>
<summary>Code</summary>
```
$ grep "net:debug" debug.log
2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XXX:YYY lastseen=2.7hrs
2022-03-04T16:41:16Z [opencon] [net:debug] trying connection XXX:YYY lastseen=16.9hrs
2022-03-04T16:41:17Z [opencon] [net:debug] trying connection XXX:YYY lastseen=93.2hrs
2022-03-04T16:41:18Z [opencon] [net:debug] trying connection XXX:YYY lastseen=2.7hrs
```
</details>
ACKs for top commit:
laanwj:
Code review and lightly tested ACK e11cdc9303
Tree-SHA512: 89a8c86667ccc0688e5acfdbd399aac1f5bec9f978a160e40b0210b0d9b8fdc338479583fc5bd2e2bc785821363f174f578d52136d228e8f638a20abbf0a568f
where all the other logging actions in src/net.{h,cpp} are located.
StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be
inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(),
called in turn from StartScheduledTasks() with a scheduleEvery delta of 45
seconds, called at the end of AppInitMain() on bitcoind startup.
This allows dropping `#include <logging.h>` from net.h, which can improve
compile time/speed. Currently, none of the other includes in net.h use
logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined.
ab1ea29ba1 refactor: make GetRand a template, remove GetRandInt (pasta)
Pull request description:
makes GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from GetRand(std::numeric_limits<uint64_t>::max() -> GetRand<uint64_t>()
ACKs for top commit:
laanwj:
Code review ACK ab1ea29ba1
Tree-SHA512: db5082a0e21783389f1be898ae73e097b31ab48cab1a2c0e29348a4adeb545d4098193aa72a547c6baa6e8205699aafec38d6a27b3d65522fb3246f91b4daae9
e71c51b27d refactor: rename command -> message type in comments in the src/net* files (Shashwat)
2b09593bdd scripted-diff: Rename message command to message type (Shashwat)
Pull request description:
This PR is a follow-up to #24078.
> a message is not a command, but simply a message of some type
The first commit covers the message_command variable name and comments not addressed in the original PR in `src/net*` files.
The second commit goes beyond the original `src/net*` limit of #24078 and does similar changes in the `src/rpc/net.cpp` file.
ACKs for top commit:
MarcoFalke:
review ACK e71c51b27d💥
Tree-SHA512: 24015d132c00f15239e5d3dc7aedae904ae3103a90920bb09e984ff57723402763f697d886322f78e42a0cb46808cb6bc9d4905561dc6ddee9961168f8324b05
f64aa9c411 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.
Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
709af67add p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex (w0xlt)
8be75fd0f0 p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex` (w0xlt)
a237a065cc scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex (w0xlt)
Pull request description:
Related to #19303, this PR gets rid of the RecursiveMutex `cs_totalBytesSent` and also adds `AssertLockNotHeld` macros combined with `LOCKS_EXCLUDED` thread safety annotations to avoid recursive locking.
ACKs for top commit:
jonatack:
ACK 709af67add per `git range-diff 7a4ac71 eff7918 709af67`, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative
vasild:
ACK 709af67add
hebasto:
ACK 709af67add, tested on Ubuntu 22.04.
Tree-SHA512: 560b4e6c92b1511911d69185207df6ee809db09b96d97f96430d8d2595dc05c98cc691aaec8a58ef87cf2ab0a98675c210b8ce0be3dedb81e31114bbbfdfd8be
36f814c0e8 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e63 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3 [net] Move asmap into NetGroupManager (John Newbery)
17c24d4580 [init] Add netgroupman to node.context (John Newbery)
9b3836710b [build] Add netgroup.cpp|h (John Newbery)
Pull request description:
The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can't be updated by other components), but it is still passed out of addrman as a reference to const, and used by `CNetAddress` to calculate the group and AS of the net address.
This RFC PR proposes to move all asmap data and logic into a new `NetGroupManager` component. This is initialized at startup, and the client components addrman and connman simply call `NetGroupManager::GetGroup(const CAddress&)` and `NetGroupManager::GetMappedAS(const CAddress&)` to get the net group and AS of an address.
ACKs for top commit:
mzumsande:
Code Review ACK 36f814c0e8
jnewbery:
CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed 36f814c0e8, so I've reverted to that.
dergoegge:
Code review ACK 36f814c0e8
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
Add more fs::path operator/ and operator+ overloads to prevent unsafe
string->path conversions on Windows that would cause strings to be
decoded according to the current Windows locale & code page instead of
the correct string encoding.
Update application code to deal with loss of implicit string->path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
We'll move the transaction relay data into Peer in subsequent commits,
but the inbound eviction logic needs to know if the peer is relaying
txs and if the peer has loaded a bloom filter.
This is currently redundant information with m_tx_relay->fRelayTxes,
but when m_tx_relay is moved into net_processing, then we'll need these
separate fields in CNode.
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
`std::function` variable called `CaptureMessage` whose value can be
changed by unit tests, should they need to inspect message contents.
