This should make it easier for the fuzz engine to explore multisig code
paths. See discussion in https://github.com/bitcoin/bitcoin/issues/23105
The downside is that all fuzz inputs that use ConsumeScript are now
invalidated and need to be re-generated.
Another downside may be that most multisig scripts from ConsumeScript are
using likely not fully valid pubkeys.
Moved implementations of `ConsumeTxMemPoolEntry`, `ContainsSpentInput`, `ConsumeNetAddr`, and the methods(open, read, write, seek, close) of FuzzedFileProvider from test/fuzz/util.h to test/fuzz/util.cpp.
* Assert when a type is missing
* Add missing WitnessV1Taproot
* Limit WitnessUnknown to version [2, 16], to avoid abiguity
* Limit WitnessUnknown to size [2, 40], to avoid invalid sizes
The return type is already enforced to be void by the
ternary operator:
./test/fuzz/util.h:47:25: error: right operand to ? is void, but left operand is of type *OTHER_TYPE*
((i++ == call_index ? callables() : void()), ...);
^ ~~~~~~~~~~~ ~~~~~~
66545da200 Remove support for double serialization (Pieter Wuille)
fff1cae43a Convert uses of double-serialization to {En,De}codeDouble (Pieter Wuille)
afd964d70b Convert existing float encoding tests (Pieter Wuille)
bda33f98e2 Add unit tests for serfloat module (Pieter Wuille)
2be4cd94f4 Add platform-independent float encoder/decoder (Pieter Wuille)
e40224d0c7 Remove unused float serialization (MarcoFalke)
Pull request description:
Based on #21981.
This adds a software-based platform-independent float/double encoder/decoder (platform independent in the sense that it only uses arithmetic and library calls, but never inspects the binary representation). This should strengthen our guarantee that encoded float/double values are portable across platforms. It then removes the functionality to serialize doubles from serialize.h, and replaces its only (non-test) use for fee estimation data serialization with the software encoder.
At least on x86/ARM, the only difference should be how certain NaN values are encoded/decoded (but not *whether* they are NaN or not).
It comes with tests that verify on is_iec559 platforms (which are the only ones we support, at least for now) that the serialized bytes exactly match the binary representation of floats in memory (for non-NaN).
ACKs for top commit:
laanwj:
Code review re-ACK 66545da200
practicalswift:
cr re-ACK 66545da200
Tree-SHA512: 62ad9adc26e28707b2eb12a919feefd4fd10cf9032652dbb1ca1cc97638ac21de89e240858e80d293d5112685c623e58affa3d316a9783ff0e6d291977a141f5
When building for Android, _GNU_SOURCE will be defined, but it doesn't
actually have the fopencookie() function, or define the
cookie_io_functions_t type.
For now just skip trying to use it if we are building for Android.
Should fix #22062.
Move the `FuzzedSock`'s implementation from `src/test/fuzz/util.h` to
`src/test/fuzz/util.cpp`.
A separate interface and implementation make the code more readable for
consumers who don't need to (better not) know the implementation
details.
40316a37cb test: add I2P test for a runaway SAM proxy (Vasil Dimov)
2d8ac77970 fuzz: add tests for the I2P Session public interface (Vasil Dimov)
9947e44de0 i2p: use pointers to Sock to accommodate mocking (Vasil Dimov)
82d360b5a8 net: change ConnectSocketDirectly() to take a Sock argument (Vasil Dimov)
b5861100f8 net: add connect() and getsockopt() wrappers to Sock (Vasil Dimov)
5a887d49b2 fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN (Vasil Dimov)
3088f83d01 fuzz: extend FuzzedSock::Recv() to support MSG_PEEK (Vasil Dimov)
9b05c49ade fuzz: implement unimplemented FuzzedSock methods (Vasil Dimov)
Pull request description:
Change the networking code and the I2P code to be fully mockable and use `FuzzedSocket` to fuzz the I2P methods `Listen()`, `Accept()` and `Connect()`.
Add a mocked `Sock` implementation that returns a predefined data on reads and use it for a regression unit test for the bug fixed in https://github.com/bitcoin/bitcoin/pull/21407.
ACKs for top commit:
practicalswift:
Tested ACK 40316a37cb
MarcoFalke:
Concept ACK 40316a37cb
jonatack:
re-ACK 40316a37cb reviewed `git range-diff 01bb3afb 23c861d 40316a3` and the new unit test commit, debug built, ran unit tests, ran bitcoind with an I2P service and network operation with seven I2P peers (2 in, 5 out) is looking nominal
laanwj:
Code review ACK 40316a37cb
Tree-SHA512: 7fc4f129849e16e0c7e16662d9f4d35dfcc369bb31450ee369a2b97bdca95285533bee7787983e881e5a3d248f912afb42b4a2299d5860ace7129b0b19623cc8
If `recv(2)` returns an error (`-1`) and sets `errno` to a temporary
error like `EAGAIN` a proper application code is expected to retry the
operation.
If the fuzz data is exhausted, then `FuzzedSock::Recv()` will keep
returning `-1` and setting `errno` to the first element of
`recv_errnos[]` which happened to be `EAGAIN`. This may continue forever
or cause the fuzz test to run for a long time before some higher level
application "receive timeout" is triggered.
Thus, put `ECONNREFUSED` as first element of `recv_errnos[]`.
A conforming `recv(2)` call is supposed to return the same data on a
call following `recv(..., MSG_PEEK)`. Extend `FuzzedSock::Recv()` to do
that.
For simplicity we only return 1 byte when `MSG_PEEK` is used. If we
would return a buffer of N bytes, then we would have to keep track how
many of them were consumed on subsequent non-`MSG_PEEK` calls.
We want `Get()` to always return the same value, otherwise it will look
like the `FuzzedSock` implementation itself is broken. So assign
`m_socket` a random number in the `FuzzedSock` constructor.
There is nothing to fuzz about the `Get()` and `Release()` methods, so
use the ones from the base class `Sock`.
`Reset()` is just setting our socket to `INVALID_SOCKET`. We don't want
to use the base `Reset()` because it will close `m_socket` and given
that our `m_socket` is just a random number it may end up closing a real
opened file descriptor if it coincides with our random `m_socket`.
-BEGIN VERIFY SCRIPT-
# Rename
sed -i -e 's/MakeFuzzingContext/MakeNoLogFileContext/g' $(git grep -l MakeFuzzingContext)
# Bump the copyright of touched files in this scripted diff to avoid touching them again later
./contrib/devtools/copyright_header.py update ./src/test/fuzz/
-END VERIFY SCRIPT-
fa29272459 Remove redundant MakeUCharSpan wrappers (MarcoFalke)
faf4aa2f47 Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke)
fada14b948 Treat CDataStream bytes as uint8_t (MarcoFalke)
fa8bdb048e refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke)
faa96f841f Remove unused CDataStream methods (MarcoFalke)
Pull request description:
Using `uint8_t` for raw bytes has a style benefit:
* The signedness is clear from reading the code, as it does not depend on the architecture
Other clean-ups in this pull include:
* Remove unused methods
* Constructor is simplified with `Span`
* Remove `Init()` member in favor of C++11 member initialization
ACKs for top commit:
laanwj:
code review ACK fa29272459
theStack:
ACK fa29272459🍾
Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
fad3d7625a fuzz: Avoid initializing version to less than MIN_PEER_PROTO_VERSION (MarcoFalke)
fa99e33aeb fuzz: move-only FillNode implementation to cpp file (MarcoFalke)
Pull request description:
This fixes a fuzz bug introduced in #20881. Previously the nodes in the fuzz tests had their version initialized to a constant (`PROTOCOL_VERSION`). After #20881, the nodes have their version initialized to an arbitrary signed integer. This is problematic for several reasons:
* Both `nVersion` and `m_greatest_common_version` may be initialized to `0`. If a `version` message is processed, this leads to a crash, because `m_greatest_common_version` must be `INIT_PROTO_VERSION` while the `version` message is processed. See #20138
* The "valid" range for `nVersion` is `[MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()]` (see check in net_processing)
* The "valid" range for `m_greatest_common_version` is `std::min(nVersion, PROTOCOL_VERSION)` (see net_processing)
Fix all issues by initializing `nVersion` and `m_greatest_common_version` to their valid ranges.
-----
The crashers, if someone wants to try this at home:
```
( echo 'dmVyc2lvbgAWFhYWFhYWFhYWFhYWFhYWFhYWFhZp/29uAPX//xYWFhYWFhYWFhYWFhYWFhYWFhYW
FhYWFhYWaW9uAOr1//8WFhYWFha0ZXJzaW9uAPX//wAAAAAAABAAAAAAAAAAAAC0ZXJzaW9uAPX/
/wBPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT08AAAAAABAAAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAAAAAAAAAAAACgAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAB2ZXJzaW9uAACDJIO9vXYKAAAAAAAAAAAAAAAAAAAAAAB2ZfS1qmu1qhUVFWs=' | base64 --decode > /tmp/a ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/a
```
```
( echo 'dmVyc2lvbgD//wAhTmiqN///NDcAAACENDL/iv//8DYAAHL///////79/RtcAJqamhqa/QEAAAD/
///+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZyq8SaGVhZGVycwAAAAD/NDcAAACENDL/iv//
8DYAAHL///////79/RtcAJqamhqa/QEAAAD////+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZ
yq8SaGVhZGVycwAAAADPAQAAACAGIm5GERoLWS1wb3J061u/KMNPOkwFXqZ///b5IgIAAD+5ubkb
XD5hZGRyAJqamhqasP0BAAAAAAAAAP0BAAAAIf39/R0dHQAAAAAAMgAA///7//+gXqZ///b5IgIA
AD+5ubm5ubm5AAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAFgAAAAAAAAAAAAlBmv39/f1/f39B
f39hZGRyAG5vAACaLgAdGzY2zwEAAAAgBiJuRhEaC1ktcG9ydOtbvyjDTzpMBV6mf//2+SICAAA/
ubm5G1w+YWRkcgCampoamrD9AQAAAAAAAAD9AQAAACH9/f0dHR0AAAAAADIAAP//+///oF6mf//2
+SICAAA/ubm5ubm5uQAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAABYAAAAAAAAAAAAJQZr9/f39
f39/QX9/YWRkcgBubwAAmi4AHRs2NjY2NjY2NjYCAgI2NgIA/f39/f39Nv39/TUmABxc' | base64 --decode > /tmp/b ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/b
```
ACKs for top commit:
practicalswift:
cr ACK fad3d7625a
Tree-SHA512: ea64ee99b94d8e619e3949d2d21252c1236412c0e40f44f2b73595ca70cd2da0bdab005fb1a54f65fb291e7b07fdd33577ce4a3a078ca933246b511ebcb0e52a
Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:
1. Calling InitializeFuzzingContext, which implicitly constructs a static
const BasicTestingSetup
2. Directly constructing a static const BasicTestingSetup in the initialize_*
function
3. Directly constructing a static TestingSetup and reproducing the
initialization arguments (I'm assuming because
InitializeFuzzingContext only initializes a BasicTestingSetup)
The new, relatively-simple MakeFuzzingContext function allows us to
consolidate these methods of initialization by being flexible enough to
be used in all situations. It:
1. Is templated so that we can choose to initialize any of
the *TestingSetup classes
2. Has sane defaults which are often used in fuzzers but are also
easily overridable
3. Returns a unique_ptr, explicitly transferring ownership to the caller
to deal with according to its situation
86c495223f net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack)
6609eb8cb5 net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack)
993d1ecd19 test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack)
Pull request description:
The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in #20197:
- asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary
- fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor
- drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status
- adds a public getter `IsInboundOnion()` that also allows unit testing it
- adds unit test coverage
ACKs for top commit:
sipa:
utACK 86c495223f
LarryRuane:
ACK 86c495223f
vasild:
ACK 86c495223f
MarcoFalke:
review ACK 86c495223f🐍
Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204
17a5f172fa fuzz: Make addrman fuzzing harness deterministic (practicalswift)
Pull request description:
Make `CAddrMan` fuzzing harness deterministic.
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:
Crypt-iQ:
utACK 17a5f172fa
Tree-SHA512: 725f983745233e9b616782247fa18847e483c074ca4336a5beea8a9009128c3a74b4d50a12662d8ca2177c2e1fc5fc121834df6b459ac0af43c931d77ef7c4d8
