CConnman::Stop() resets semOutbound, yet m_reconnections is not
cleared in Stop. Each ReconnectionInfo contains a grant member
that points to the memory that semOutbound pointed to and ~CConnman
will attempt to access the grant field (memory that was already
freed) when destroying m_reconnections. Fix this by calling
m_reconnections.clear() in CConnman::Stop() and add appropriate
annotations.
b63428ac9c rpc: refactor: use more (Maybe)Arg<std::string_view> (stickies-v)
037830ca0d refactor: increase string_view usage (stickies-v)
b3bf18f0ba rpc: refactor: use string_view in Arg/MaybeArg (stickies-v)
Pull request description:
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](d127b25199 (diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a32374R32))) with raw pointers being implemented.
This PR aims to improve on this by returning a trivially copyable `std::string_view` (`Arg`) or `std::optional<std::string_view>` (`MaybeArg`), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using `std::is_trivially_copyable_v` instead of defining the types manually.
In cases where functions currently take a `const std::string&` and it would be too much work / touching consensus logic to update them (`signmessage.cpp`), a `std::string` copy is made (which was already happening anyway).
The last 2 commits increase usage of the `{Arg,MaybeArg}<std::string_view>` helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.
ACKs for top commit:
maflcko:
re-ACK b63428ac9c🎉
achow101:
ACK b63428ac9c
pablomartin4btc:
re-ACK [b63428a](b63428ac9c)
w0xlt:
reACK b63428ac9c
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
c76de2eea1 net: support overriding the proxy selection in ConnectNode() (Vasil Dimov)
Pull request description:
Normally `ConnectNode()` would choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.
Also have `OpenNetworkConnection()` return whether the connection succeeded or not. This can be used when the caller needs to keep track of how many (successful) connections were opened.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.
ACKs for top commit:
stratospher:
ACK c76de2e.
optout21:
ACK c76de2eea1
mzumsande:
Code Review ACK c76de2eea1
andrewtoth:
ACK c76de2eea1
Tree-SHA512: 1d266e4280cdb1d0599971fa8b5da58b1b7451635be46abb15c0b823a1e18cf6e7bcba4a365ad198e6fd1afee4097d81a54253fa680c8b386ca6b9d68d795ff0
0f7d4ee4e8 p2p: Use different inbound inv timer per network (Martin Zumsande)
94db966a3b net: use generic network key for addrcache (Martin Zumsande)
Pull request description:
Currently, `NextInvToInbounds` schedules each round of `inv` at the same time for all inbound peers. It's being done this way because with a separate timer per peer (like it's done for outbounds), an attacker could do multiple connections to learn about the time a transaction arrived. (#13298).
However, having a single timer for inbounds of all networks is also an obvious fingerprinting vector: Connecting to a suspected pair of privacy-network and clearnet addresses and observing the `inv` pattern makes it trivial to confirm or refute that they are the same node.
This PR changes it such that a separate timer is used for each network.
It uses the existing method from `getaddr` caching and generalizes it to be saved in a new field `m_network_key` in `CNode` which will be used for both `getaddr` caching and `inv` scheduling, and can also be used for any future anti-fingerprinting measures.
ACKs for top commit:
sipa:
utACK 0f7d4ee4e8
stratospher:
reACK 0f7d4ee.
naiyoma:
Tested ACK 0f7d4ee4e8
danielabrozzoni:
reACK 0f7d4ee4e8
Tree-SHA512: e197c3005b2522051db432948874320b74c23e01e66988ee1ee11917dac0923f58c1252fa47da24e68b08d7a355d8e5e0a3ccdfa6e4324cb901f21dfa880cd9c
Update select functions that take a const std::string& to take a
std::string_view instead. In a next commit, this allows us to use
the {Arg,MaybeArg}<std::string_view> helper.
Modernizes interface by not forcing users to deal with raw pointers,
without adding copying overhead. Generalizes the logic of whether
we return by value or by optional/pointer.
In cases where functions take a `const std::string&` and it would
be too much work to update them, a string copy is made (which was
already happening anyway).
Normally `ConnectNode()` would choose whether to use a proxy and which
one. Make it possible to override this from the callers and same for
`OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers
through the Tor SOCKS5 proxy.
Also have `OpenNetworkConnection()` return whether the connection
succeeded or not. This can be used when the caller needs to keep track
of how many (successful) connections were opened.
87e7f37918 doc: clarify peer address in getpeerinfo and addnode RPC help (Vasil Dimov)
2a4450ccbb net: change FindNode() to not return a node and rename it (Vasil Dimov)
4268abae1a net: avoid recursive m_nodes_mutex lock in DisconnectNode() (Vasil Dimov)
3a4d1a25cf net: merge AlreadyConnectedToAddress() and FindNode(CNetAddr) (Vasil Dimov)
Pull request description:
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.
Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the return value.
Remove a recursive lock on `m_nodes_mutex`.
Rename `FindNode()` to better describe what it does.
ACKs for top commit:
achow101:
ACK 87e7f37918
furszy:
Code review ACK 87e7f37918
hodlinator:
re-ACK 87e7f37918
Tree-SHA512: 44fb64cd1226eca124ed1f447b4a1ebc42cc5c9e8561fc91949bbeaeaa7fa16fcfd664e85ce142e5abe62cb64197c178ca4ca93b3b3217b913e3c498d0b7d1c9
All callers of `CConnman::FindNode()` use its return value `CNode*` only
as a boolean null/notnull. So change that method to return `bool`.
This removes the dangerous pattern of handling a `CNode` object (the
return value of `FindNode()`) without holding `CConnman::m_nodes_mutex`
and without having that object's reference count incremented for the
duration of the usage.
Also rename the method to better describe what it does.
Have `CConnman::DisconnectNode()` iterate `m_nodes` itself instead of
using `FindNode()`. This avoids recursive mutex lock and drops the only
caller of `FindNode()` which used the return value for something else
than a boolean found/notfound.
0802398e74 fuzz: make it possible to mock (fuzz) CThreadInterrupt (Vasil Dimov)
6d9e5d130d fuzz: add CConnman::SocketHandler() to the tests (Vasil Dimov)
3265df63a4 fuzz: add CConnman::InitBinds() to the tests (Vasil Dimov)
91cbf4dbd8 fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests (Vasil Dimov)
50da7432ec fuzz: add CConnman::OpenNetworkConnection() to the tests (Vasil Dimov)
e6a917c8f8 fuzz: add Fuzzed NetEventsInterface and use it in connman tests (Vasil Dimov)
e883b37768 fuzz: set the output argument of FuzzedSock::Accept() (Vasil Dimov)
Pull request description:
Extend `CConnman` fuzz tests to also exercise the methods `OpenNetworkConnection()`, `CreateNodeFromAcceptedSocket()`, `InitBinds()` and `SocketHandler()`.
Previously fuzzing those methods would have resulted in real socket functions being called in the operating system which is undesirable during fuzzing. Now that https://github.com/bitcoin/bitcoin/pull/21878 is complete all those are mocked to a fuzzed socket and a fuzzed DNS resolver (see how `CreateSock` and `g_dns_lookup` are replaced in the first commit).
ACKs for top commit:
achow101:
ACK 0802398e74
jonatack:
Review re-ACK 0802398e74
dergoegge:
Code review ACK 0802398e74
Tree-SHA512: a717d4e79f42bacf2b029c821fdc265e10e4e5c41af77cd4cb452cc5720ec83c62789d5b3dfafd39a22cc8c0500b18169aa7864d497dded729a32ab863dd6c4d
`CConnman::AlreadyConnectedToAddress()` is the only caller of
`CConnman::FindNode(CNetAddr)`, so merge the two in one function.
The unit test that checked whether `AlreadyConnectedToAddress()` ignores
the port is now unnecessary because now the function takes a `CNetAddr`
argument. It has no access to the port.
The generic key can also be used in other places
where behavior between different network identities should
be uncorrelated to avoid fingerprinting.
This also changes RANDOMIZER_ID - since it is not
being persisted to disk, there are no compatibility issues.
67f632b6de net: remove unnecessary casts in socket operations (Matthew Zipkin)
Pull request description:
During review of https://github.com/bitcoin/bitcoin/pull/32747 several casting operations were questioned in existing code that had been copied or moved. That lead me to find a few other similar casts in the codebase.
It turns out that since the `Sock` class wraps syscalls with its own internal casting (see https://github.com/bitcoin/bitcoin/pull/24357 and https://github.com/bitcoin/bitcoin/pull/20788 written in 2020-2022) we no longer need to cast the arguments when calling these functions. The original argument-casts are old and were cleaned up a bit in https://github.com/bitcoin/bitcoin/pull/12855 written in 2018.
The casting is only needed for windows compatibility, where those syscalls require a data argument to be of type `char*` specifically:
https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-getsockopt
```
int getsockopt(
[in] SOCKET s,
[in] int level,
[in] int optname,
[out] char *optval,
[in, out] int *optlen
);
```
but on POSIX the argument is `void*`:
https://www.man7.org/linux/man-pages/man2/getsockopt.2.html
```
int getsockopt(socklen *restrict optlen;
int sockfd, int level, int optname,
void optval[_Nullable restrict *optlen],
socklen_t *restrict optlen);
```
ACKs for top commit:
Raimo33:
ACK 67f632b6de
achow101:
ACK 67f632b6de
hodlinator:
ACK 67f632b6de
vasild:
ACK 67f632b6de
davidgumberg:
ACK 67f632b6de
Tree-SHA512: c326d7242698b8d4d019f630fb6281398da2773c4e5aad1e3bba093a012c2119ad8815f42bd009e61a9a90db9b8e6ed5c75174aac059c9df83dd3aa5618a9ba6
Tor inbound connections do not reveal the peer's actual network address.
Therefore do not apply whitelist permissions to them.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
These methods in the Sock class wrap corresponding syscalls,
accepting void* arguments and casting to char* internally, which is
needed for Windows support and ignored on other platforms because
the syscall itself accepts void*:
Send()
Recv()
GetSockOpt()
SetSockOpt()
Currently this code is not called in unit tests. Calling should make it
possible to write tests for things like IPC exceptions being thrown during
shutdown.
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
function should only be used in trusted contexts. This helps avoid
accidental privacy leaks by preventing the uncached version from being
used in non-trusted scenarios, like P2P.
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
* Make the methods of `CThreadInterrupt` virtual and store a pointer to
it in `CConnman`, thus making it possible to override with a mocked
instance.
* Initialize `CConnman::m_interrupt_net` from the constructor, making it
possible for callers to supply mocked version.
* Introduce `FuzzedThreadInterrupt` and `ConsumeThreadInterrupt()` and
use them in `src/test/fuzz/connman.cpp` and `src/test/fuzz/i2p.cpp`.
This improves the CPU utilization of the `connman` fuzz test.
As a nice side effect, the `std::shared_ptr` used for
`CConnman::m_interrupt_net` resolves the possible lifetime issues with
it (see the removed comment for that variable).
6f7052a7b9 threading: semaphore: move CountingSemaphoreGrant to its own header (Cory Fields)
fd15469892 threading: semaphore: remove temporary convenience types (Cory Fields)
1f89e2a49a scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones (Cory Fields)
f21365c4fc threading: replace CountingSemaphore with std::counting_semaphore (Cory Fields)
1acacfbad7 threading: make CountingSemaphore/CountingSemaphoreGrant template types (Cory Fields)
e6ce5f9e78 scripted-diff: rename CSemaphore and CSemaphoreGrant (Cory Fields)
793166d381 wallet: change the write semaphore to a BinarySemaphore (Cory Fields)
6790ad27f1 scripted-diff: rename CSemaphoreGrant and CSemaphore for net (Cory Fields)
d870bc9451 threading: add temporary semaphore aliases (Cory Fields)
7b816c4e00 threading: rename CSemaphore methods to match std::semaphore (Cory Fields)
Pull request description:
This is relatively simple, but done in a bunch of commits to enable scripted diffs.
I wanted to add a semaphore in a branch I've been working on, but it was unclear if I should use `std::counting_semaphore` or stick with our old `CSemaphore`. I couldn't decide, so I just decided to remove all doubt and get rid of ours :)
This replaces our old `CSemaphore` with `std::counting_semaphore` everywhere we used it. `CSemaphoreGrant` is still there as an RAII wrapper, but is now called `CountingSemaphoreGrant` and `BinarySemaphoreGrant` to match. Those have been moved out of `sync.h` to their own file.
ACKs for top commit:
purpleKarrot:
ACK 6f7052a7b9
achow101:
ACK 6f7052a7b9
TheCharlatan:
ACK 6f7052a7b9
hebasto:
ACK 6f7052a7b9, I have reviewed the code and it looks OK.
Tree-SHA512: 5975d13aa21739174e3a22c544620ae3f36345f172b51612346d3b7baf0a07c39ef6fd54f647c87878c21a67951b347a5d4a5f90e897f3f6c0db360a3779d0df
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
iffaddrs.h that implements one, but not the other.
`CConnman::AlreadyConnectedToAddress()` searches the existent nodes by
address or by address-and-port:
```cpp
FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort())
```
but:
* if there is a match by just the address, then the address-and-port
search will not be evaluated and the whole condition will be `true`
* if the there is no node with the same address, then the second search
by address-and-port will not find a match either.
The search by address-and-port is comparing against `CNode::m_addr_name`
which could be a hostname, e.g. `"node.foobar.com:8333"`, but
`addr.ToStringAddrPort()` is always going to be numeric.
6869fb4170 net: Block v2->v1 transport downgrade if !CConnman::fNetworkActive (Hodlinator)
Pull request description:
We might have just set `CNode::fDisconnect` in the first loop because of `!CConnman::fNetworkActive`.
Attempting to reconnect using v1 transport just because `fNetworkActive` was set to `false` at the "right" stage in the v2 handshake does not make sense.
Issue [discovered](https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1930908304) by davidgumberg.
ACKs for top commit:
davidgumberg:
Tested and Reviewed ACK 6869fb4170
mabu44:
ACK 6869fb4170
stratospher:
ACK 6869fb4. I've reviewed the code but don't have strong preference for this branch vs master since only functional change is just a single log not being printed in a low probability scenario (we happen to be attempting v2 connection when P2P network activity is being turned off).
vasild:
ACK 6869fb4170
Tree-SHA512: 54f596e54c5a6546f2c3fec2609aa8d10dec3adcf1001ca16666d8b374b8d79d64397f46c90d9b3915b4e91a5041b6ced3044fd2a5b4fb4aa7282eb51f61296a
eb0724f0de doc: banman: reference past vuln due to unbounded banlist (Antoine Poinsot)
ad616b6c01 doc: net: mention past vulnerability as rationale to limit incoming message size (Antoine Poinsot)
4489117c3f doc: txrequest: point to past censorship vulnerability in tx re-request handling (Antoine Poinsot)
68ac9542c4 doc: net_proc: reference past DoS vulnerability in orphan processing (Antoine Poinsot)
c02d9f6dd5 doc: net_proc: reference past defect regarding invalid GETDATA types (Antoine Poinsot)
5e3d9f21df doc: validation: add a reference to historical header spam vulnerability (Antoine Poinsot)
Pull request description:
It is useful when reading code to have context about why it is written or behaves the way it does. Some instances in this PR may seem obvious but i think nonetheless offer important context to anyone willing to change (or review a change to) this code.
ACKs for top commit:
ryanofsky:
Code review ACK eb0724f0de. No changes since last review other than rebase
Tree-SHA512: 271902f45b8130d44153d793bc1096cd22b6ce05494e67c665a5bc45754e3fc72573d303ec8fc7db4098d473760282ddbf0c1cf316947539501dfd8d7d5b8828
We might have just set CNode::fDisconnect in the first loop because of being offline.
Also caches CConnman::fNetworkActive in case it's changed concurrently with our own thread.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
a85e8c0e61 doc: Add some general documentation about negated options (Ryan Ofsky)
490c8fa178 doc: Add release notes summarizing negated option behavior changes. (Ryan Ofsky)
458ef0a11b refactor: Avoid using IsArgSet() on -connect list option (Ryan Ofsky)
752ab9c3c6 test: Add test to make sure -noconnect disables -dnsseed and -listen by default (Ryan Ofsky)
3c2920ec98 refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options (Ryan Ofsky)
d05668922a refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options (Ryan Ofsky)
3d1e8ca53a Normalize inconsistent -noexternalip behavior (Ryan Ofsky)
ecd590d4c1 Normalize inconsistent -noonlynet behavior (Ryan Ofsky)
5544a19f86 Fix nonsensical bitcoin-cli -norpcwallet behavior (Ryan Ofsky)
6e8e7f433f Fix nonsensical -noasmap behavior (Ryan Ofsky)
b6ab350806 Fix nonsensical -notest behavior (Ryan Ofsky)
6768389917 Fix nonsensical -norpcwhitelist behavior (Ryan Ofsky)
e03409c70f Fix nonsensical -norpcbind and -norpcallowip behavior (Ryan Ofsky)
40c4899bc2 Fix nonsensical -nobind and -nowhitebind behavior (Ryan Ofsky)
5453e66fd9 Fix nonsensical -noseednode behavior (Ryan Ofsky)
Pull request description:
The PR changes behavior of negated `-noseednode`, `-nobind`, `-nowhitebind`, `-norpcbind`, `-norpcallowip`, `-norpcwhitelist`, `-notest`, `-noasmap`, `-norpcwallet`, `-noonlynet`, and `-noexternalip` options, so negating these options just clears previously specified values doesn't have other side effects.
Negating options on the command line can be a useful way of resetting options that may have been set earlier in the command line or config file. But before this change, negating these options wouldn't fully reset them, and would have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.
Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the `IsArgSet()` function. Using `IsArgSet()` tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by `GetArgs()`, because when these settings are negated, `IsArgSet()` will return true but `GetArgs()` will return an empty list. This PR eliminates all uses of `IsArgSet()` and `GetArgs()` together, and followup PR #17783 makes it an error to use `IsArgSet()` on list settings, since calling `IsArgSet()` is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.
ACKs for top commit:
achow101:
ACK a85e8c0e61
danielabrozzoni:
re-ACK a85e8c0e61
hodlinator:
re-ACK a85e8c0e61
Tree-SHA512: dd4b19faac923aeaa647b1c241d929609ce8242b43e3b7bc32523cc48ec92a83ac0dc5aee79f1eba8794535e0314b96cb151fd04ac973671a1ebb9b52dd16697
Treat specifying -noseednode the same as not specifying any -seednode value,
instead of enabling the seed node timeout and log messages, and waiting longer
to add other seeds.
* `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`,
thus change its argument.
* Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a
dummy `CAddress` from `CService`, so use `CService` instead.
* `GetBindAddress()` only needs to return `CService`.
* `CNode::addrBind` only needs to be `CService`.
0f716f2889 qa: cover PROTOCOL_ERROR variant in PCP unit tests (Antoine Poinsot)
fc700bb47f test: Add tests for PCP and NATPMP implementations (laanwj)
caf9521033 net: Use mockable steady clock in PCP implementation (laanwj)
03648321ec util: Add mockable steady_clock (laanwj)
ab1d3ece02 net: Add optional length checking to CService::SetSockAddr (laanwj)
Pull request description:
Add a NodeSteadyClock, a steady_clock that can be mocked with millisecond precision. Use this in the PCP implementation.
Then add a mock for a simple scriptable UDP server,, which is used to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.
Includes "net: Add optional length checking to CService::SetSockAddr" from #31014 as a prerequisite.
ACKs for top commit:
darosior:
re-ACK 0f716f2889
i-am-yuvi:
Concept ACK 0f716f2889
achow101:
ACK 0f716f2889
Tree-SHA512: 6f91b24e6fe46a3fded7a13972efd77c98e6ef235f8898e4ae44068c5df32d1cdabb22cb66c351b338dc98cb2073b624e43607a28107f4999302bfbe7a138229
In almost all cases (the only exception is `getifaddrs`), we know the
size of the data passed into SetSockAddr, so we can check this to be
what is expected.
