b527b54950 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() (Vasil Dimov)
29f66f7682 moveonly: move SetSocketNonBlocking() from netbase to util/sock (Vasil Dimov)
b4bac55679 net: convert standalone IsSelectableSocket() to Sock::IsSelectable() (Vasil Dimov)
5db7d2ca0a moveonly: move IsSelectableSocket() from compat.h to sock.{h,cpp} (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
* convert standalone `IsSelectableSocket()` to `Sock::IsSelectable()`
* convert standalone `SetSocketNonBlocking()` to `Sock::SetNonBlocking()`
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
jonatack:
ACK b527b54950 review/debug build/unit tests at each commit, cross-referenced the changes with `man select` and `man errno`, ran a signet node on the last commit with ip4/ip6//tor/i2p/cjdns and network connections were nominal
dergoegge:
Code review ACK b527b54950
Tree-SHA512: af783ce558c7a89e173f7ab323fb3517103d765c19b5d14de29f64706b4e1fea3653492e8ea73ae972699986aaddf2ae72c7cfaa7dad7614254283083b7d2632
There are many cases where we assume message processing is
single-threaded in order for how we access node-related memory to be
safe. Add an explicit mutex that we can use to document this, which allows
the compiler to catch any cases where we try to access that memory from
other threads and break that assumption.
facc2fa7b8 Use AutoFile where possible (MacroFake)
6666803c89 streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8
fanquake:
ACK facc2fa7b8
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
b2733ab6a8 net: add new method Sock::Listen() that wraps listen() (Vasil Dimov)
3ad7de225e net: add new method Sock::Bind() that wraps bind() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Add new methods `Sock::Bind()` and `Sock::Listen()` that wrap `bind()` and `listen()`.
This will help to increase `Sock` usage and make more code mockable.
ACKs for top commit:
pk-b2:
ACK b2733ab6a8
laanwj:
Code review ACK b2733ab6a8
Tree-SHA512: c6e737606703e2106fe60cc000cfbbae3a7f43deadb25f70531e2cac0457e0b0581440279d14c76c492eb85c12af4adde52c30baf74542c41597e419817488e8
a8d6abba5e net: change GetBindAddress() to take Sock argument (Vasil Dimov)
748dbcd9f2 net: add new method Sock::GetSockName() that wraps getsockname() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Wrap the syscall `getsockname()` in `Sock::GetSockName()` and change `GetBindAddress()` to take a `Sock` argument so that it can use the wrapper.
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
laanwj:
Code review ACK a8d6abba5e
Tree-SHA512: 3a73463258c0057487fb3fd67215816b03a1c5160f45e45930eaeef86bb3611ec385794cdb08339aa074feba8ad67cd2bfd3836f6cbd40834e15d933214a05dc
Outside of `Sock`, `Sock::Reset()` was used in just one place (in
`i2p.cpp`) which can use the assignment operator instead.
This simplifies the public `Sock` API by having one method less.
6e68ccbefe net: use Sock::WaitMany() instead of CConnman::SocketEvents() (Vasil Dimov)
ae263460ba net: introduce Sock::WaitMany() (Vasil Dimov)
cc74459768 net: also wait for exceptional events in Sock::Wait() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
`Sock::Wait()` waits for IO events on one socket. Introduce a similar `virtual` method `WaitMany()` that waits simultaneously for IO events on more than one socket.
Use `WaitMany()` instead of `CConnman::SocketEvents()` (and ditch the latter). Given that the former is a `virtual` method, it can be mocked by unit and fuzz tests. This will help to make bigger parts of `CConnman` testable (unit and fuzz).
ACKs for top commit:
laanwj:
Code review ACK 6e68ccbefe
jonatack:
re-ACK 6e68ccbefe per `git range-diff e18fd47 6747729 6e68ccb`, and verified rebase to master and debug build
Tree-SHA512: 917fb6ad880d64d3af1ebb301c06fbd01afd8ff043f49e4055a088ebed6affb7ffe1dcf59292d822f10de5f323b6d52d557cb081dd7434634995f9148efcf08f
It allows waiting concurrently on more than one socket. Being a
`virtual` `Sock` method it can be overriden by tests.
Will be used to replace `CConnman::SocketEvents()`.
Since the removal of NODISCARD in 81d5af42f4,
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
ef5014d256 style: wrap long lines in CNode creation and add some comments (Vasil Dimov)
b683491648 scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex (Vasil Dimov)
c41a1162ac net: use Sock in CNode (Vasil Dimov)
c5dd72e146 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 ef5014d256 changes since last review are the removal of an unneeded dtor and the addition of a style commit
w0xlt:
reACK ef5014d
PastaPastaPasta:
utACK ef5014d256, I have reviewed the code, and believe it makes sense to merge
theStack:
Cod-review ACK ef5014d256
Tree-SHA512: 7f5414dd339cd2f16f7cbdc5fcec238d68b6d50072934aea10b901f409da28ff1ece6db6e899196616aa8127b8b25ab5b86d000bdcee58b4cadd7a3c1cf560c5
fa5d2e678c Remove unused char serialize (MarcoFalke)
fa24493d63 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217 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 fa5d2e678c
sipa:
re-utACK fa5d2e678c
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
6bf6e9fd9d net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85a 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 6bf6e9fd9d
jamesob:
ACK 6bf6e9fd9d ([`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 6bf6e9fd9d 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()), ...);
^ ~~~~~~~~~~~ ~~~~~~
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.
