An unnecessary check was added to the block mutation tests
in #29412 where IsBlockMutated is returning true for the invalid
reasons: we try to check mutation via transaction duplication,
but the merkle root is not updated before the check, therefore
the check fails because the provided root and the computed root
differ, but not because the block contains the same transaction twice.
The check is meaningless so it can be removed.
d8087adc7e [test] IsBlockMutated unit tests (dergoegge)
1ed2c98297 Add transaction_identifier::size to allow Span conversion (dergoegge)
1ec6bbeb8d [validation] Cache merkle root and witness commitment checks (dergoegge)
5bf4f5ba32 [test] Add regression test for #27608 (dergoegge)
49257c0304 [net processing] Don't process mutated blocks (dergoegge)
2d8495e080 [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge)
66abce1d98 [validation] Introduce IsBlockMutated (dergoegge)
e7669e1343 [refactor] Cleanup merkle root checks (dergoegge)
95bddb930a [validation] Isolate merkle root checks (dergoegge)
Pull request description:
This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks.
We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message.
We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. https://github.com/bitcoin/bitcoin/pull/27608 for which a regression test is included in this PR).
ACKs for top commit:
achow101:
ACK d8087adc7e
maflcko:
ACK d8087adc7e🏄
fjahr:
Code review ACK d8087adc7e
sr-gi:
Code review ACK d8087adc7e
Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f
51bc1c7126 test: Remove Windows-specific code from `system_tests/run_command` (Hennadii Stepanov)
Pull request description:
The removed code has been dead since https://github.com/bitcoin/bitcoin/pull/28967.
Required as a precondition for replacing Boost.Process with [cpp-subprocess](https://github.com/bitcoin/bitcoin/pull/28981) to make diff for this code meaningful and reviewable.
The plan is to reintroduce Windows-specific code in this test simultaneously with enabling Windows support in cpp-subprocess.
ACKs for top commit:
Sjors:
utACK 51bc1c7126
theStack:
Code-review ACK 51bc1c7126
Tree-SHA512: 0e3875c4dc20564332555633daf2227223b10dc3d052557635eced2734575d1e0252fb19e46ea6e6c47a15c51c345f70b6d437e33435abcd0e4fcf29edb50887
During shutdown, already queue events dispatched from the backend such
'numConnectionsChanged' and 'networkActiveChanged' could try to access
the clientModel object, which might not exist because we manually delete
it inside 'BitcoinApplication::requestShutdown()'.
Turn the `std::vector` to `std::array` because it is cheaper and
allows us to have the number of the messages as a compile time
constant: `ALL_NET_MESSAGE_TYPES.size()` which can be used in
future code to build other `std::array`s with that size.
These replace our platform-specific mess in favor of c++20 endian detection
via std::endian and internal byteswap functions when necessary.
They no longer rely on autoconf detection.
Rather than a complicated set of tests to decide which bswap functions to
use, always prefer the compiler built-ins when available.
These builtins and fallbacks can all be removed once we're using c++23, which
adds std::byteswap.
This code has been dead since https://github.com/bitcoin/bitcoin/pull/28967.
Required as a precondition for replacing Boost.Process with
cpp-subprocess to make diff for this code meaningful and reviewable.
The plan is to reintroduce Windows-specific code in this test
simultaneously with enabling Windows support in cpp-subprocess.
We preemptively perform a block mutation check before further processing
a block message (similar to early sanity checks on other messsage
types). The main reasons for this change are as follows:
- `CBlock::GetHash()` is a foot-gun without a prior mutation check, as
the hash returned only commits to the header but not to the actual
transactions (`CBlock::vtx`) contained in the block.
- We have observed attacks that abused mutated blocks in the past, which
could have been prevented by simply not processing mutated blocks
(e.g. https://github.com/bitcoin/bitcoin/pull/27608).
d2fe90571e test: Drop `x` modifier in `fsbridge::fopen` call for mingw builds (Hennadii Stepanov)
Pull request description:
The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the `x` modifier for the [`_wfopen()`](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-170) function.
Fixes https://github.com/bitcoin/bitcoin/issues/29014.
ACKs for top commit:
maflcko:
ACK d2fe90571e
fanquake:
ACK d2fe90571e - the plan here should still be to migrate to the newer windows runtime.
Tree-SHA512: 0269b66531e58c093ecda3a3e355a20ee8274e165d7e010f8f125881b3c8d4cfe801abdca4605d81efd3b2dbe9a81896968971f6f53da7f6c6093b76b47c5bc9
b03b20685a Fix CI-detected codespell warnings (Lőrinc)
Pull request description:
Split out the typo fixes encountered in https://github.com/bitcoin/bitcoin/pull/29458 to a separate PR.
ACKs for top commit:
maflcko:
ACK b03b20685a
Tree-SHA512: 99b6fac01ba2ae6e6de9c50d2b481387899844a4b3a77d544c7b8afe7cfd25251a982329688d4739cde8b98ad35afcfd49be7c7cc3dad9bdff1d5915861a206d
faa30a4c56 rpc: Do not wait for headers inside loadtxoutset (MarcoFalke)
Pull request description:
While the `loadtxoutset` default 10 minute timeout is convenient when it is sufficient, it may cause hassle where it is not. For example:
* When P2P connections are missing, it seems better to abort early than wait for the timeout.
* When the 10 minute timeout is not sufficient, the RPC will have to be called again, so a check or loop is needed outside the RPC either way. So might as well remove the loop inside the RPC.
ACKs for top commit:
fjahr:
ACK faa30a4c56
theStack:
Code-review ACK faa30a4c56
pablomartin4btc:
tACK faa30a4c56
TheCharlatan:
ACK faa30a4c56
Tree-SHA512: 9167c7d8b2889bb3fd369de4acd2cc4d24a2fe225018d82bd9568ecd737093f6e19be7cc62815b574137b61076a6f773c29bff75398991b5cd702423aab2322b
fa58ae74ea refactor: Add missing include for USE_BDB, USE_SQLITE to bench/wallet_ismine.cpp (MarcoFalke)
fa31908ea8 lint: Check for missing or redundant bitcoin-config.h includes (MarcoFalke)
fa63b0e351 lint: Make lint error easier to spot in output (MarcoFalke)
fa770fd368 doc: Add missing RUST_BACKTRACE=1 (MarcoFalke)
fa10051267 lint: Add get_subtrees() helper (MarcoFalke)
Pull request description:
Missing `bitcoin-config.h` includes are problematic, because the build could silently pass, but produce an unintended result. For example, a slower fallback algorithm could be picked, even though `bitcoin-config.h` indicates that a faster feature is available and should be used.
As the build succeeds silently, this problem is not possible to detect with iwyu.
Thus, fix this by using a linter based on grepping the source code.
ACKs for top commit:
theuni:
Weak ACK fa58ae74ea.
TheCharlatan:
ACK fa58ae74ea
hebasto:
ACK fa58ae74ea, tested on Ubuntu 23.10 -- it catches bugs properly. I didn't review rust code changes.
Tree-SHA512: cf4346f81ea5b8c215da6004cb2403d1aaf569589613c305d8ba00329b82b3841da94fe1a69815ce15f2edecbef9b031758ec9b6433564976190e3cf91ec8181
Before this change it was possible but awkward to create ParamStream streams
with multiple parameter objects. After this change it is straightforward.
The change to support multiple parameters is implemented by letting
ParamsStream contain substream instances, instead of just references to
external substreams. So a side-effect of this change is that ParamStream can
now accept rvalue stream arguments and be easier to use in some other cases. A
test for rvalues is added in this commit, and some simplifications to non-test
code are made in the next commit.
Move parameter argument after stream argument so will be possible to accept
multiple variadic parameter arguments in the following commit.
Also reverse template parameter order for consistency.
Drop unnecessary ParamsStream references from CTransaction and
CMutableTransaction constructors. This just couples these classes unnecessarily
to the ParamsStream class, making the ParamsStream class harder to modify, and
making the transaction classes in some cases (depending on parameter order)
unable to work with stream classes that have multiple parameters set.
9d1dbbd4ce scripted-diff: Fix bitcoin_config_h includes (TheCharlatan)
Pull request description:
As mentioned in https://github.com/bitcoin/bitcoin/pull/26924#issuecomment-1403449932 and https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399, it is currently not safe to remove `bitcoin-config.h` includes from headers because some unrelated file might be depending on it.
See also #26972 for discussion.
Solve this by including the file directly everywhere it's required, regardless of whether or not it's already included by another header.
There should be no functional change here, but it will allow us to safely remove includes from headers in the future.
~I'm afraid it's a bit tedious to reproduce these commits, but it's reasonably straightforward:~
Edit: See note below
```bash
# All commands executed from the src/ subdir.
# Collect all tokens from bitcoin-config.h.in
# Isolate the tokens and remove blank lines
# Replace newlines with | and remove the last trailing one
# Collect all files which use these tokens
# Filter out subprojects (proper forwarding can be verified from Makefiles)
# Filter out .rc files
# Save to a text file
git grep -E -l `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-with-config-include.txt
# Find all files from the above list which don't include bitcoin-config.h
git grep -L -E "config/bitcoin-config.h" -- `cat files-with-config-include.txt`
# Include them manually with the exception of some files in crypto:
# crypto/sha256_arm_shani.cpp crypto/sha256_avx2.cpp crypto/sha256_sse41.cpp crypto/sha256_x86_shani.cpp
# These are exceptions which don't use bitcoin-config.h, rather the Makefile.am adds these cppflags manually.
# Commit changes. This should match the first commit of this PR.
# Use the same search as above to find all files which DON'T use any config tokens
git grep -E -L `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-without-config-include.txt
# Manually remove the includes and commit changes. This should match the second commit of this PR.
```
Edit: I'll keep this old description for posterity, but the manual approach has been replaced with a scripted diff from TheCharlatan
ACKs for top commit:
maflcko:
ACK 9d1dbbd4ce🚪
TheCharlatan:
ACK 9d1dbbd4ce
hebasto:
ACK 9d1dbbd4ce, I have reviewed the code and it looks OK.
fanquake:
ACK 9d1dbbd4ce
Tree-SHA512: f11ddc4ae6a887f96b954a6b77f310558ddb271088a3fda3edc833669c4251b7f392515224bbb8e5f67eb2c799b4ffed3b07d96454e82ec635c686d0df545872
