1. It didn't actually disable asm usage in our code. Regardless of the setting,
asm is used in random.cpp and support/cleanse.cpp.
2. The value wasn't forwarded to libsecp as a user might have reasonably
expected.
3. We now have the DISABLE_OPTIMIZED_SHA256 define which is what disable-asm
actually did in practice.
If there is any desire, we can hook DISABLE_OPTIMIZED_SHA256 up to a new
configure option that actually does what it says.
e073f1dfda test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6)
367bb7a80c wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6)
Pull request description:
I think the expected behaviour of `getrawchangeaddress` and `getnewaddress` RPCs is that their failure should not affect keypool in any way. At least that's how legacy wallets work, you can confirm this behaviour by running `wallet_keypool.py --legacy-wallet` on master with e073f1dfda applied on top. However running `wallet_keypool.py --descriptors` on the same commit results in the following failure:
```
File "/path/to/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/path/to/bitcoin/test/functional/wallet_keypool.py", line 114, in run_test
assert_equal(kp_size_before, kp_size_after)
File "/path/to/bitcoin/test/functional/test_framework/util.py", line 57, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not([18, 24] == [19, 24])
```
This happens because we pass `nIndex` (which is a class member) into `GetReservedDestination` and since it's passed by reference we get an updated value back, so `nIndex` won't be equal `-1` anymore, no matter if the function failed or succeeded. This means that `ReturnDestination` (called by dtor of `ReserveDestination`) will try to return something we did not actually reserve.
The fix is to simply use a temporary variable instead of a class member and only update `nIndex` when `op_address` actually has value, basically do it the same way we do for other class members (`address` and `fInternal`) already.
ACKs for top commit:
achow101:
ACK e073f1dfda
josibake:
ACK e073f1dfda
Tree-SHA512: 1128288a60dd4d8f306ef6f7ac66cdfeae3c9cc35c66ecada2d78fa61ac759f2a757b70fc3976ba8b5081200942b58dfabc184c01ccf911af40ba8c145344651
0487f91a20 test: Fix intermittent failure in rpc_net.py --v2transport (stratospher)
Pull request description:
Fixes#29508.
Make sure that v2 handshake is complete before comparing getpeerinfo outputs so that `transport_protocol_type` isn't stuck at 'detecting'.
This is done by adding a wait_until statement till `transport_protocol_type = v2` so that bitcoind waits until the v2 handshake is complete. (on the python side, this is ensured by default since `wait_for_handshake = True` inside `add_p2p_connection()`)
ACKs for top commit:
Sjors:
ACK 0487f91a20
mzumsande:
Code Review ACK 0487f91a20
achow101:
ACK 0487f91a20
vasild:
ACK 0487f91a20
Tree-SHA512: 44dd646a61cd38da243f527df7321e22d1821c2b090be43673027746098caf450c6671708ed731ba257952df6b5886e64c9c2f9686a82f6ef0f25780b7a87d3d
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.
Make sure that v2 handshake is complete before comparing getpeerinfo
outputs so that `transport_protocol_type` isn't stuck at 'detecting'.
- on the python side, this is ensured by default
`wait_for_handshake = True` inside `add_p2p_connection()`.
- on the c++ side, add a wait_until statement till
`transport_protocol_type = v2` so that v2 handshake is complete.
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
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
1484998b6b ci: print python version on win64 native job (Max Edwards)
Pull request description:
Adds python version output to the Win64 Native CI job on Github Actions. Also clarifies that one of the versions already printed is the VCToolsVersion.
Before:
After:
Should the individual python test runners print the python version instead or also?
ACKs for top commit:
hebasto:
ACK 1484998b6b.
Tree-SHA512: 6d084ff4a667156fa8797450de83bbcf596ddd3b2fa8ec04c1ca9a532a6fec716817b66da34db4ea0184bd802ef613e2b8f6142be9a511c5397785cfbfede0c3
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
ad7584d8b6 serialization: replace char-is-int8_t autoconf detection with c++20 concept (Cory Fields)
Pull request description:
Doesn't depend on #29263, but it's really only relevant after that one's merged.
This removes the only remaining autoconf macro in our serialization code (after #29263), so it can now be used trivially and safely out-of-tree.
~Our code does not currently contain any concepts, but couldn't find any discussion or docs about avoiding them. I guess we'll see if this blows up our c-i.~
Edit: Ignore this. ajtowns pointed out that we're already using a few concepts.
This was introduced in #13580. Please check my logic on this as I'm unable to test on a SmartOS system. Even better would be a confirmation from someone who can build there.
ACKs for top commit:
Empact:
Code review ACK ad7584d8b6
Tree-SHA512: 1faf65c900700efb1cf3092c607a2230321b393cb2f029fbfb94bc8e50df1dabd7a9e4b91e3b34f0d2f3471aaf18ee7e56d91869db5c5f4bae84da95443e1120
b052b2d1f2 build: remove -Wdocumentation conditional (fanquake)
Pull request description:
Now that `--enable-suppress-external-warnings` is on by default, we can drop it. CIs are all already building with this flag.
ACKs for top commit:
Empact:
Code review ACK b052b2d1f2
theuni:
utACK b052b2d1f2
Tree-SHA512: 8b55f366dfeece082090fb87de67d8811967f4c89987a346431b2deb73c3c94401b59ec98bb1cbf790e18894f3d4c4aebb57cbc5fbf931c1046bf40239bc7a58
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()'.
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).
fccfdb25b2 doc: Update OpenBSD build docs to 7.4 (Jesse Barton)
Pull request description:
Updated OpenBSD Build doc for 7.4 after testing all build options. No issues on my end.
Also added a note about referring to depends/README.md for detailed instructions on required dependencies.
This was added in reference to a conversation in #29443
ACKs for top commit:
fanquake:
ACK fccfdb25b2
theStack:
lgtm ACK fccfdb25b2
Tree-SHA512: be6d22b605140b37a71e11c5bbed54f60655832d78cd3cb221eddc77c7621a65c0d71baf436f90819be536d9b5dbf1a0b2c82b6b23d62356addc495403f2ba35
bf5662c678 test: enable v2 for python p2p depending on global --v2transport flag (Martin Zumsande)
6e9e39da43 test: Don't use v2transport when it's too slow. (Martin Zumsande)
87549c8f89 test: enable p2p_invalid_messages.py with v2transport (Martin Zumsande)
5fc9db504b test: enable p2p_sendtxrcncl.py with v2transport (Martin Zumsande)
Pull request description:
#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.
To do that, a few tests need to be adjusted.
In addition, I added a commit to always use v1 in a few select subtests that send a large number of large messages (e.g. large reorgs). These tests don't have a fundamental problem with v2 but become very slow due to the unoptimised python ChaCha20 implementation (~30 minutes on my computer, so probably not suitable to be run in the CI).
As a result, `python3 test_runner.py --v2transport` should succeed and use `v2` everywhere (unless v1 is chosen explicitly).
[Edit]: To make the "test each commit" CI pass, several test fixes were squashed into the last commit, which actually enables v2 p2p for `P2PConnection`. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review.
ACKs for top commit:
fjahr:
tACK bf5662c678
vasild:
ACK bf5662c678
stratospher:
reACK bf5662c6.
theStack:
Tested ACK bf5662c678
Tree-SHA512: 4f5a08248ba8a755f7d0f48deb2b79bef03292345cacb7deef01be955481093800e4e56ff218ea56734eef5de1fb3ab0f04657447ea27d393bb536539d7b289d
fa3a4102ef fuzz: Set -rss_limit_mb=8000 for generate as well (MarcoFalke)
fa4e396e1d fuzz: Generate with random libFuzzer settings (MarcoFalke)
Pull request description:
Sometimes a libFuzzer setting like `-use_value_profile=1` helps [0], sometimes it hurts [1].
[0] https://github.com/bitcoin/bitcoin/pull/20789#issuecomment-752961937
[1] https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1645976254
By picking a random value, it is ensured that at least some of the runs will have the beneficial configuration set.
Also, set `-max_total_time` to prevent slow fuzz targets from getting a larger time share, or possibly peg to a single core for a long time and block the python script from exiting for a long time. This can be improved in the future. For example, the python script can exit after some time (https://github.com/bitcoin/bitcoin/pull/20752#discussion_r549248791). Alternatively, it can measure if coverage progress was made and run for less time if no progress has been made recently anyway, so that more time can be spent on targets that are new or still make progress.
ACKs for top commit:
murchandamus:
utACK fa3a4102ef
dergoegge:
utACK fa3a4102ef
brunoerg:
light ACK fa3a4102ef
Tree-SHA512: bfd04a76ca09aec612397bae5f3f263a608faa7087697169bd4c506c8195c4d2dd84ddc7fcd3ebbc75771eab618fad840af819114968ca3668fc730092376768
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
Tested and used all build options on OpenBSD 7.4 with no issues.
Added a note about referring to depends/README.md for detailed instructions on required dependencies.
This was added in reference to a conversation in #29443
0fbf051fec depends: fix BDB compilation on OpenBSD (Sebastian Falbesoner)
Pull request description:
Compiling C++ code with `-D_XOPEN_SOURCE=600` causes problems on OpenBSD. If that define is set, the C++ standard header detection routine in BDB's configure script fails due to a missing type name for `locale_t` (see https://gist.github.com/theStack/b41884e31ebc5cdca3220bcaa674cb70 for the relevant config.log part).
This results in `HAVE_CXX_STDHEADERS` not being defined, which then it turn leads to the inclusion of `<iostream.h>` (rather than `<iostream>`), which doesn't exist, as described in #28963.
According to a mailing list post discussing a similar problem [1], "OpenBSD provides the POSIX APIs by default", so we don't need this define anyway and can remove it. This fixes the BDB build problem as described in issue #28963. See also f87e75ae71 for a similar fix for google's flatbuffer project.
Tested on OpenBSD 7.4 with clang 13.0.0. Fixes#28963.
[1] https://www.mail-archive.com/tech@openbsd.org/msg63386.html
ACKs for top commit:
fanquake:
ACK 0fbf051fec
Tree-SHA512: 02139e9081ed855e067bfba8c81b54c657417576e553cc1035a916ada9be049358f5e14d756d5f234c5226bd7e943f61c6ae8990c1b152f9125681b7b777c9b3
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
5f240ab2e8 test: Add option to skip unit tests for the test runner (Martin Zumsande)
Pull request description:
In the python `test_runner`, it's possible to disable specific functional tests (or just enable a few specific ones), but the unit tests for the python test framework cannot be skipped.
Add this option (`--skipunit` or `-u`), it would save some time for devs not interested in running those every time.
ACKs for top commit:
fjahr:
re-ACK 5f240ab2e8
tdb3:
Code review and tested ACK 5f240ab2e8
stratospher:
tested ACK 5f240ab.
Tree-SHA512: f7c9cfefc18a6510e24ca4601309b40fdf4180a4c5fe592be9cf7607be6541784b283c46c8d6e60740ff3eba83025dd5d0db36e55bf8bad1404b38120859e113
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
