Since the removal of NODISCARD in 81d5af42f4dba5b68a597536cad7f61894dc22a3,
the only attributes def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
ef5014d256638735b292672c774446db4003f03b style: wrap long lines in CNode creation and add some comments (Vasil Dimov)
b68349164827f14c472201cad54c4e19a3321261 scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex (Vasil Dimov)
c41a1162ac4da437c5d755e8fe2bf636bed22b0f net: use Sock in CNode (Vasil Dimov)
c5dd72e146dd8fa77d29c8689a42322a4d1ec780 fuzz: move FuzzedSock earlier in src/test/fuzz/util.h (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`.
This will help mocking / testing / fuzzing more code.
ACKs for top commit:
jonatack:
re-ACK ef5014d256638735b292672c774446db4003f03b changes since last review are the removal of an unneeded dtor and the addition of a style commit
w0xlt:
reACK ef5014d
PastaPastaPasta:
utACK ef5014d256638735b292672c774446db4003f03b, I have reviewed the code, and believe it makes sense to merge
theStack:
Cod-review ACK ef5014d256638735b292672c774446db4003f03b
Tree-SHA512: 7f5414dd339cd2f16f7cbdc5fcec238d68b6d50072934aea10b901f409da28ff1ece6db6e899196616aa8127b8b25ab5b86d000bdcee58b4cadd7a3c1cf560c5
fa5d2e678c809c26bd40d7e7c171529d3ffb5903 Remove unused char serialize (MarcoFalke)
fa24493d6394b3a477535f480664c9596f18e3c5 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217b725ada35107b4ad646d250228355c span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
sipa:
re-utACK fa5d2e678c809c26bd40d7e7c171529d3ffb5903
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
6bf6e9fd9dece67878595a5f3361851c25833c49 net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c9efa620c0cce73503772805cc1fa82 net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85ae5404adef23a52719d804a5c36b1e8 net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)
Pull request description:
_This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._
Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.
ACKs for top commit:
laanwj:
Code review ACK 6bf6e9fd9dece67878595a5f3361851c25833c49
jamesob:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
jonatack:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
w0xlt:
tACK 6bf6e9f
Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
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()), ...);
^ ~~~~~~~~~~~ ~~~~~~
66545da2008cd9e806e41b74522ded259cd64f86 Remove support for double serialization (Pieter Wuille)
fff1cae43af959a601cf2558cb3c77f3c2b1aa80 Convert uses of double-serialization to {En,De}codeDouble (Pieter Wuille)
afd964d70b6f7583ecf89c380f80db07f5b66a60 Convert existing float encoding tests (Pieter Wuille)
bda33f98e2f32f2411fb0a8f5fb4f0a32abdf7d4 Add unit tests for serfloat module (Pieter Wuille)
2be4cd94f4c7d92a4287971233a20d68db81c9c9 Add platform-independent float encoder/decoder (Pieter Wuille)
e40224d0c77674348bf0a518365208bc118f39a4 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 66545da2008cd9e806e41b74522ded259cd64f86
practicalswift:
cr re-ACK 66545da2008cd9e806e41b74522ded259cd64f86
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.
40316a37cb02cf8a9a8b2cbd4d7153ffa57e7ec5 test: add I2P test for a runaway SAM proxy (Vasil Dimov)
2d8ac779708322e1235e823edfc9c8f6e2dd65e4 fuzz: add tests for the I2P Session public interface (Vasil Dimov)
9947e44de0cbd79e99d883443a9ac441d8c69713 i2p: use pointers to Sock to accommodate mocking (Vasil Dimov)
82d360b5a88d9057b6c09b61cd69e426c7a2412d net: change ConnectSocketDirectly() to take a Sock argument (Vasil Dimov)
b5861100f85fef77b00f55dcdf01ffb4a2a112d8 net: add connect() and getsockopt() wrappers to Sock (Vasil Dimov)
5a887d49b2b39e59d7cce8e9d5b89c21ad694f8b fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN (Vasil Dimov)
3088f83d016e7ebb6e6aa559e6326fa0ef0d6282 fuzz: extend FuzzedSock::Recv() to support MSG_PEEK (Vasil Dimov)
9b05c49ade729311a0f4388a109530ff8d0ed1f9 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 40316a37cb02cf8a9a8b2cbd4d7153ffa57e7ec5
MarcoFalke:
Concept ACK 40316a37cb
jonatack:
re-ACK 40316a37cb02cf8a9a8b2cbd4d7153ffa57e7ec5 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 40316a37cb02cf8a9a8b2cbd4d7153ffa57e7ec5
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`.
fa576b4532814b4bca1936d170cabd01fbc51960 Move MakeNoLogFileContext to common libtest_util, and use it in bench (MarcoFalke)
Pull request description:
To avoid verbose code duplication, which may lead to accidental mishaps https://github.com/bitcoin/bitcoin/pull/20998/files#r563624041.
Also fix a nit I found in https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.
ACKs for top commit:
dongcarl:
Light Code-Review ACK fa576b4532814b4bca1936d170cabd01fbc51960
fanquake:
ACK fa576b4532814b4bca1936d170cabd01fbc51960
Tree-SHA512: d39ac9c0957813ebb20ed13bd25a8ef8469377ce2651245638bd761c796feac2a17e30dd16f1e5c8db57737fb918c81d56a3d784c33258275e426a1b11e69eb2
-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-
fa292724598c273867bc6dbf311f1440fe2541ba Remove redundant MakeUCharSpan wrappers (MarcoFalke)
faf4aa2f47c0de4f3a0c5f5fe5b3ec32f611eefd Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke)
fada14b948cac147198e3b685b5dd8cb72dc2911 Treat CDataStream bytes as uint8_t (MarcoFalke)
fa8bdb048e65cae2d26bea3f991717a856e2fb39 refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke)
faa96f841fe45bc49ebb6e07ac82a129fa9c40bf 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 fa292724598c273867bc6dbf311f1440fe2541ba
theStack:
ACK fa292724598c273867bc6dbf311f1440fe2541ba 🍾
Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
fad3d7625aa1c2b6c343946e709e87e7168f9d9d fuzz: Avoid initializing version to less than MIN_PEER_PROTO_VERSION (MarcoFalke)
fa99e33aebed0109630474e11183b0726b410c2e 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 fad3d7625aa1c2b6c343946e709e87e7168f9d9d
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
