e8691056c0 test: Unroll `&&` conditions in macros (rustaceanrob)
Pull request description:
Picked from #35713. Given that I think this is a strict debugging improvement, I opened as a separate pull:
Using `&&` in `BOOST_CHECK` is problematic as failures will not indicate which condition failed. By unrolling these checks, the user knows exactly which expression is the failing case.
As an example, here is a line that would be particularly hard to debug if it failed:
```
src/test/net_tests.cpp
BOOST_CHECK((*ret)[1] && (*ret)[1]->m_type == "headers" && std::ranges::equal((*ret)[1]->m_recv, MakeByteSpan(msg_data_2)));
```
If any one of these conditions fail, the whole expression fails, with no values printed or indication as to which condition failed.
This is also required when using test macros that support value decomposition, which requires `&&` and `||` are `delete`. Examples include `BOOST_TEST`, doctest, Catch2, etc.
ref: https://catch2-temp.readthedocs.io/en/latest/assertions.html#other-limitations
ref: https://fekir.info/post/decomposing-an-expression/
ACKs for top commit:
maflcko:
re-ACK e8691056c0🌽
ismaelsadeeq:
reACK e8691056c0
sedited:
ACK e8691056c0
Tree-SHA512: 9eb74cecd47ee4fdc3f53beb7d50d5056d543303d023c68b8d47cbe52d37f1156488c8b943faf68dd52c192c43626037bbf08172e7cc24753e0f6070db6e3ab2
Using `&&` in `BOOST_CHECK` is problematic as failures will not indicate
which condition failed. By unrolling these checks, the user knows
exactly which expression is the failing case.
As an example, here is a line that would be particularly hard to debug
if it failed:
```
src/test/net_tests.cpp
BOOST_CHECK((*ret)[1] && (*ret)[1]->m_type == "headers" && std::ranges::equal((*ret)[1]->m_recv, MakeByteSpan(msg_data_2)));
```
If any one of these conditions fail, the whole expression fails, with no
values printed or indication as to which condition failed.
This is also required when using test macros that support value
decomposition, which requires `&&` and `||` are `delete`. Examples
include `BOOST_TEST`, doctest, Catch2, etc.
ref: https://catch2-temp.readthedocs.io/en/latest/assertions.html#other-limitations
ref: https://fekir.info/post/decomposing-an-expression/
dab7f2c984 test: cover -externalip/onlynet interaction in functional test (will)
657a5aa3f3 test: cover -externalip bypassing -onlynet (will)
8c87e32bd3 net: let -externalip bypass -onlynet (will)
f4af02e827 net: add an add_even_if_unreachable argument to AddLocal (will)
Pull request description:
`-onlynet` is documented to restrict automatic outbound connections, but it also currently prevents `-externalip` addresses from being advertised when their network is not in the `-onlynet` set. This happens because `AddLocal()` rejects addresses outside `g_reachable_nets`, regardless of whether the address was explicitly configured by the user.
Previous attempts to fix this (#24835 and #25690) removed the `g_reachable_nets` check from `AddLocal()`.
This PR instead adds an explicit `add_even_if_unreachable` argument to `AddLocal()`. The argument defaults to `false`, and is set to `true` only when adding addresses from `-externalip`.
As a result, explicitly configured `-externalip` addresses can still be advertised even when their network is excluded by `-onlynet`, while discovered, mapped, bound, Tor control, and I2P SAM addresses continue to use the existing reachable-network filter.
This keeps the fix scoped to `-externalip` and addresses the concern raised in #25690:
> I think it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network
The branch adds unit coverage for `AddLocal()` and functional coverage in `p2p_addr_selfannouncement.py` for `-onlynet=ipv4 -externalip=<onion>`.
Fixes: #25336Fixes: #25669
ACKs for top commit:
achow101:
ACK dab7f2c984
mzumsande:
re-ACK dab7f2c984
w0xlt:
ACK dab7f2c984
Tree-SHA512: a4ac9334b85da8b6902d3850e21d3a1c9d7dce70bcb79182448c8d5684e24462cd6e440385af7aa4420d9582e4dff9dc9e827ca7a6da0363fff2d3c531784d9b
Add unit coverage for the -onlynet/-externalip interaction.
Check that an unreachable address still fails with normal AddLocal()
arguments, while the same address succeeds when the unreachable-net
override is set for explicit local-address configuration.
BIP434 defines FEATURE messages which are sent between VERSION and VERACK
to indicate support for new P2P protocol features. This commit provides
the infrastructure for easily using BIP434 negotiation when implementing
such new P2P protocol features. Note that advertised protocol version
is bumped to 70017, as per BIP434's specification.
Signed overflow on nScore updates is undefined behavior. Use
SaturatingAdd in AddLocal() and SeenLocal() so increments saturate at
INT_MAX instead of overflowing.
Add unit test coverage for saturation in both code paths.
It is best if the internal addrman database is not modified with
information coming from private broadcast connections because that
information can potentially later be sent via other connections.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Avoid providing the entire unit test framework dependency to tests that only
require access to the HasReason utility class.
E.g. reverselock_tests.cpp, sync_tests.cpp, util_check_tests.cpp, util_string_tests.cpp,
and script_parse_tests.cpp only require access to HasReason and nothing else.
fa0677d131 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb3956 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec2 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735 test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131
achow101:
ACK fa0677d131
janb84:
re ACK fa0677d131
sipa:
crACK fa0677d131
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
Calling this low-level function from tests is confusing, and also makes
it harder to change the peer manager implementation.
So juse use the pre-existing test helpers to achieve the same.
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.
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-
d871d77825 test: Remove non-portable IPv6 test (Hennadii Stepanov)
Pull request description:
On Illumos-based systems, such as OpenIndiana and OmniOS, the assumption that "the default zone ID of 0 can be omitted for the default scope" is incorrect. As a result, `getaddrinfo("fe80::1%0", ...)` returns the `EAI_NONAME` error instead of resolving to "fe80::1".
See: https://www.illumos.org/man/3SOCKET/getaddrinfo.
This PR removes the problematic code introduced in https://github.com/bitcoin/bitcoin/pull/19951.
ACKs for top commit:
fanquake:
ACK d871d77825
Tree-SHA512: 2ef5c22f826d16661deb1d6d005cbda64179e8b83be43d3d6ac51caff02187cf224355c9da144ff110a6ae2cb68f0338ea9b62af8e0f9f1014a518cf9dad6ab5
* `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`.
On Illumos-based systems, such as OpenIndiana and SmartOS, the
assumption that "the default zone ID of 0 can be omitted for the default
scope" is incorrect. As a result, `getaddrinfo("fe80::1%0", ...)`
returns the `EAI_NONAME` error.
See: https://www.illumos.org/man/3SOCKET/getaddrinfo.
The following scripted-diff commit will replace ParseHex("...") with "..."_hex_u8, but this replacement will not work in cases where vectors are needed instead of arrays, and is not ideal in cases where std::byte is accepted.
For example, it is currently necessary to use _hex_v_u8 when calling CScript operator<< because that operator does not currently support std::array or std::byte.
Conversely, it is incorrect to use _hex_v instead of _hex in net_processing.cpp for the MakeAndPushMessage argument, because if the argument is a std::vector it is considered variable-length and serialized with a size prefix, but if the argument is a std::array or Span is it considered fixed length and serialized without a prefix.
By the same logic, it is also safe to change the NUMS_H constant in pubkey.cpp from a std::vector to std::array because it is never serialized.
948238a683 test: Remove FastRandomContext global (Ryan Ofsky)
fa0fe08eca scripted-diff: [test] Use g_rng/m_rng directly (MarcoFalke)
fa54cab473 test: refactor: Accept any RandomNumberGenerator in RandMoney (MarcoFalke)
68f77dd21e test: refactor: Pass rng parameters to test functions (Ryan Ofsky)
fa19af555d test: refactor: Move g_insecure_rand_ctx.Reseed out of the helper that calls MakeRandDeterministicDANGEROUS (MarcoFalke)
3dc527f460 test: refactor: Give unit test functions access to test state (Ryan Ofsky)
fab023e177 test: refactor: Make unsigned promotion explicit (MarcoFalke)
fa2cb654ec test: Add m_rng alias for the global random context (MarcoFalke)
fae7e3791c test: Correct the random seed log on a prevector test failure (MarcoFalke)
Pull request description:
This is mostly a style-cleanup for the tests' random generation:
1) `g_insecure_rand_ctx` in the tests is problematic, because the name is a leftover when the generator was indeed insecure. However, now the generator is *deterministic*, because the seed is either passed in or printed (c.f. RANDOM_CTX_SEED). Stating that deterministic randomness is insecure in the tests seems redundant at best. Fix it by just using `m_rng` for the name.
2) The global random context has many one-line aliases, such as `InsecureRand32`. This is problematic, because the same line of code may use the context directly and through a wrapper at the same time. For example in net_tests (see below). This inconsistency is harmless, but confusing. Fix it by just removing the one-line aliases.
```
src/test/net_tests.cpp: auto msg_data_1 = g_insecure_rand_ctx.randbytes<uint8_t>(InsecureRandRange(100000));
````
3) The wrapper for randmoney has the same problem that the same unit test uses the context directly and through a wrapper at the same time. Also, it has a single type of Rng hardcoded. Fix it by accepting any type.
ACKs for top commit:
hodlinator:
ACK 948238a683
ryanofsky:
Code review ACK 948238a683. Only changes since last review were changing a comments a little bit.
marcofleon:
Code review ACK 948238a683. Only changes since my last review are the improvements in `prevector_tests`.
Tree-SHA512: 69c6b46a42cb743138ee8c87ff26a588dbe083e3efb3dca49b8a133ba5d3b09e8bf01c590ec7e121a7d77cb1fd7dcacd927a9ca139ac65e1f7c6d1ec46f93b57
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
The nVersion field is unused, so remove it.
This is also required for future commits.
Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
99990194ce Remove WithParams serialization helper (MarcoFalke)
ffffb4af83 scripted-diff: Use ser params operator (MarcoFalke)
fae9054793 test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp (MarcoFalke)
Pull request description:
Every serialization parameter struct already has the `SER_PARAMS_OPFUNC`, except for one in the tests.
For consistency, and to remove verbose code, convert the test to `SER_PARAMS_OPFUNC`, and use it everywhere, then remove the `WithParams` helper.
ACKs for top commit:
ajtowns:
reACK 99990194ce
TheCharlatan:
Re-ACK 99990194ce
Tree-SHA512: be9cae4225a502486fe8d552aaf4b2cd2904a9f73cce9d931c6b7c757594ff1982fcc2c30d00d012cd12b0a9531fd609f8bcd7c94b811e965ac087eb8a3589d3
0e6f6ebc06 net: remove unused CConnman::FindNode(const CSubNet&) (Vasil Dimov)
9482cb780f netbase: possibly change the result of LookupSubNet() to CJDNS (Vasil Dimov)
53afa68026 net: move MaybeFlipIPv6toCJDNS() from net to netbase (Vasil Dimov)
6e308651c4 net: move IsReachable() code to netbase and encapsulate it (Vasil Dimov)
c42ded3d9b fuzz: ConsumeNetAddr(): avoid IPv6 addresses that look like CJDNS (Vasil Dimov)
64d6f77907 net: put CJDNS prefix byte in a constant (Vasil Dimov)
Pull request description:
`LookupSubNet()` would treat addresses that start with `fc` as IPv6 even if `-cjdnsreachable` is set. This creates the following problems where it is called:
* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6 `fc...` subnet and the match will never succeed.
* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in `banlist.json`. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6) would not match a peer (`fc00::1`, CJDNS).
* `RPCConsole::unbanSelectedNode()`: in the GUI the ban entries go through `CSubNet::ToString()` and back via `LookupSubNet()`. Then it must match whatever is stored in `BanMan`, otherwise it is impossible to unban via the GUI.
These were uncovered by https://github.com/bitcoin/bitcoin/pull/26859.
Thus, flip the result of `LookupSubNet()` to CJDNS if the network base address starts with `fc` and `-cjdnsreachable` is set. Since subnetting/masking does not make sense for CJDNS (the address is "random" bytes, like Tor and I2P, there is no hierarchy) treat `fc.../mask` as an invalid `CSubNet`.
To achieve that, `MaybeFlipIPv6toCJDNS()` has to be moved from `net` to `netbase` and thus also `IsReachable()`. In the process of moving `IsReachable()`, `SetReachable()` and `vfLimited[]` encapsulate those in a class.
ACKs for top commit:
jonatack:
Code review ACK 0e6f6ebc06
achow101:
ACK 0e6f6ebc06
mzumsande:
re-ACK 0e6f6ebc06
Tree-SHA512: 4767a60dc882916de4c8b110ce8de208ff3f58daaa0b560e6547d72e604d07c4157e72cf98b237228310fc05c0a3922f446674492e2ba02e990a272d288bd566
`vfLimited`, `IsReachable()`, `SetReachable()` need not be in the `net`
module. Move them to `netbase` because they will be needed in
`LookupSubNet()` to possibly flip the result to CJDNS (if that network
is reachable).
In the process, encapsulate them in a class.
`NET_UNROUTABLE` and `NET_INTERNAL` are no longer ignored when adding
or removing reachable networks. This was unnecessary.
See also https://github.com/bitcoin/bips/pull/1498
The benefit is a simpler implementation:
- The protocol state machine does not need separate states for garbage
authentication and version phases.
- The special case of "ignoring the ignore bit" is removed.
- The freedom to choose the contents of the garbage authentication
packet is removed. This simplifies testing.
3f4e1bb9ae tests: fix incorrect assumption in v2transport_test (Pieter Wuille)
Pull request description:
One part of the current `v2transport_test` introduced in #28196 assumes that if a bit gets modified in a message, failure should instantly be detected after sending that message. This is not correct in case the length descriptor is modified, as that may cause the receiver to need more data first. Fix this by sending more messages until failure actually occurs.
Discovered in https://github.com/bitcoin/bitcoin/pull/27495#issuecomment-1719934041.
ACKs for top commit:
theStack:
ACK 3f4e1bb9ae
Tree-SHA512: faa90bf91996cbaaef62d764e746cb222eaf6796316b0d0e13709e528750b7c0ef09172f7fecfe814dbb8c136c5259f65ca1ac79318e6768a0bfc4e626a63249
The protocol.h file contains many non-consensus related definitions and
should thus not be part of the libbitcoinkernel. This commit makes
protocol.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>
This removes the ability for BIP324Cipher to generate its own key, moving that
responsibility to the caller (mostly, V2Transport). This allows us to write
the random-key V2Transport constructor by delegating to the explicit-key one.
