102867c587 net: change CNetAddr::ip to have flexible size (Vasil Dimov)
1ea57ad674 net: don't accept non-left-contiguous netmasks (Vasil Dimov)
Pull request description:
(chopped off from #19031 to ease review)
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Do not accept
invalid netmasks that have 0-bits followed by 1-bits and only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
ACKs for top commit:
sipa:
utACK 102867c587
MarcoFalke:
Concept ACK 102867c587
ryanofsky:
Code review ACK 102867c587. Just many suggested updates since last review. Thanks for following up on everything!
jonatack:
re-ACK 102867c587 diff review, code review, build/tests/running bitcoind with ipv4/ipv6/onion peers
kallewoof:
ACK 102867c587
Tree-SHA512: d60bf716cecf8d3e8146d2f90f897ebe956befb16f711a24cfe680024c5afc758fb9e4a0a22066b42f7630d52cf916318bedbcbc069ae07092d5250a11e8f762
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
1a8f0d5a74 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de630354f [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)
Pull request description:
Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.
Needed for https://github.com/bitcoin/bitcoin/pull/16698.
ACKs for top commit:
ajtowns:
ACK 1a8f0d5a74
MarcoFalke:
re-ACK 1a8f0d5a74
naumenkogs:
ACK 1a8f0d5, and let's merge it and come back to it later.
Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
a5bca13 Bugfix: Include <memory> for std::unique_ptr (Luke Dashjr)
Pull request description:
Not sure why all these includes were missing, but it's breaking builds for some users:
https://bugs.gentoo.org/show_bug.cgi?id=652142
(Added to all files with a reference to `std::unique_ptr`)
Tree-SHA512: 8a2c67513ca07b9bb52c34e8a20b15e56f8af2530310d9ee9b0a69694dd05e02e7a3683f14101a2685d457672b56addec591a0bb83900a0eb8e2a43d43200509
Support is added to serialize arrays of type char or unsigned char directly,
without any wrappers. All invocations of the FLATDATA wrappers that are
obsoleted by this are removed.
This includes a patch by Russell Yanofsky to make char casting type safe.
The serialization of CSubNet is changed to serialize a bool directly rather
than though FLATDATA. This makes the serialization independent of the size
of the bool type (and will use 1 byte everywhere).
This adds the listening address on which incoming connections were received to the
CNode and CNodeStats structures.
The address is reported in `getpeerinfo`.
This can be useful for distinguishing connections received on different listening ports
(e.g. when using a different listening port for Tor hidden service connections)
or different networks.
Remove the nType and nVersion as parameters to all serialization methods
and functions. There is only one place where it's read and has an impact
(in CAddress), and even there it does not impact any of the recursively
invoked serializers.
Instead, the few places that need nType or nVersion are changed to read
it directly from the stream object, through GetType() and GetVersion()
methods which are added to all stream classes.
There are only a few uses of `insecure_random` outside the tests.
This PR replaces uses of insecure_random (and its accompanying global
state) in the core code with an FastRandomContext that is automatically
seeded on creation.
This is meant to be used for inner loops. The FastRandomContext
can be in the outer scope, or the class itself, then rand32() is used
inside the loop. Useful e.g. for pushing addresses in CNode or the fee
rounding, or randomization for coin selection.
As a context is created per purpose, thus it gets rid of
cross-thread unprotected shared usage of a single set of globals, this
should also get rid of the potential race conditions.
- I'd say TxMempool::check is not called enough to warrant using a special
fast random context, this is switched to GetRand() (open for
discussion...)
- The use of `insecure_rand` in ConnectThroughProxy has been replaced by
an atomic integer counter. The only goal here is to have a different
credentials pair for each connection to go on a different Tor circuit,
it does not need to be random nor unpredictable.
- To avoid having a FastRandomContext on every CNode, the context is
passed into PushAddress as appropriate.
There remains an insecure_random for test usage in `test_random.h`.
CConnman then passes the current best height into CNode at creation time.
This way CConnman/CNode have no dependency on main for height, and the signals
only move in one direction.
This also helps to prevent identity leakage a tiny bit. Before this change, an
attacker could theoretically make 2 connections on different interfaces. They
would connect fully on one, and only establish the initial connection on the
other. Once they receive a new block, they would relay it to your first
connection, and immediately commence the version handshake on the second. Since
the new block height is reflected immediately, they could attempt to learn
whether the two connections were correlated.
This is, of course, incredibly unlikely to work due to the small timings
involved and receipt from other senders. But it doesn't hurt to lock-in
nBestHeight at the time of connection, rather than letting the remote choose
the time.
