Age | Commit message (Collapse) | Author |
|
|
|
|
|
521693378b86aaae5af1646c3a18a902cc835c69 build: move sha256_sse4 into libbitcoin_crypto_base (fanquake)
Pull request description:
Followup to discussion in #29407.
Drops `LIBBITCOIN_CRYPTO_SSE4`.
ACKs for top commit:
theuni:
utACK 521693378b86aaae5af1646c3a18a902cc835c69.
hebasto:
ACK 521693378b86aaae5af1646c3a18a902cc835c69.
TheCharlatan:
ACK 521693378b86aaae5af1646c3a18a902cc835c69
Tree-SHA512: 44889340b7647409a439ebe97012f66383e0e5f3d99200ffd55c124e91d3ed164f02b736ff9dafca2d9ba7e365fcdc79aff054471d4bc240d035b58659407420
|
|
Followup to discussion in #29407.
Drops LIBBITCOIN_CRYPTO_SSE4.
|
|
modernization
86b7f28d6c507155a9d3a15487ee883989b88943 serialization: use internal endian conversion functions (Cory Fields)
432b18ca8d0654318a8d882b28b20af2cb2d2e5d serialization: detect byteswap builtins without autoconf tests (Cory Fields)
297367b3bb062c57142747719ac9bf2e12717ce9 crypto: replace CountBits with std::bit_width (Cory Fields)
52f9bba889fd9b50a0543fd9fedc389592cdc7e5 crypto: replace non-standard CLZ builtins with c++20's bit_width (Cory Fields)
Pull request description:
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking showed that not all compilers are capable of detecting and optimizing byteswap functions, so compiler builtins are instead used where possible. However, they're now detected via macros rather than autoconf checks.
This[ matches how libc++ implements std::byteswap for c++23](https://github.com/llvm/llvm-project/blob/main/libcxx/include/__bit/byteswap.h#L26).
I suggest we move/rename `compat/endian.h`, but I left that out of this PR to avoid bikeshedding.
#29057 pointed out some irregularities in benchmarks. After messing with various compilers and configs for a few weeks with these changes, I'm of the opinion that we can't win on every platform every time, so we should take the code that makes sense going forward. That said, if any real-world slowdowns are caused here, we should obviously investigate.
ACKs for top commit:
maflcko:
ACK 86b7f28d6c507155a9d3a15487ee883989b88943 📘
fanquake:
ACK 86b7f28d6c507155a9d3a15487ee883989b88943 - we can finish pruning out the __builtin_clz* checks/usage once the minisketch code has been updated. This is more good cleanup pre-CMake & for the kernal.
Tree-SHA512: 715a32ec190c70505ffbce70bfe81fc7b6aa33e376b60292e801f60cf17025aabfcab4e8c53ebb2e28ffc5cf4c20b74fe3dd8548371ad772085c13aec8b7970e
|
|
25eab523897e790f4f4d7b49cdbf19d13e3b0fcc fuzz: add target for local addresses (brunoerg)
Pull request description:
This PR adds fuzz target for local address functions - (`AddLocal`, `RemoveLocal`, `SeenLocal`, `IsLocal`)
ACKs for top commit:
dergoegge:
ACK 25eab523897e790f4f4d7b49cdbf19d13e3b0fcc
vasild:
ACK 25eab523897e790f4f4d7b49cdbf19d13e3b0fcc
Tree-SHA512: 24faaab86dcd8835ba0e2d81fb6322a39a9266c7edf66415dbc4421754054f47efb6e0de4efdc7ea026b0686792658e86a526f7cf27cbc6cf9ed0c4aed376f97
|
|
docs
efb70cd6452ce1f0d9f5464bec837b09ed5c2a78 doc: correct function name in AssumeUTXO design docs (jrakibi)
Pull request description:
Corrected the function name from `CompleteSnapshotValidation()` to `MaybeCompleteSnapshotValidation()` in the assumeutxo design documentation.
This change ensures that the documentation accurately reflects the actual function name used in the code
ACKs for top commit:
Empact:
ACK https://github.com/bitcoin/bitcoin/pull/29518/commits/efb70cd6452ce1f0d9f5464bec837b09ed5c2a78
Tree-SHA512: 68b9be3ba710d91a2a955189e227f86b46ccb6a2a13c345d46f276cec6ff12b77ebf9814c4bcb00db7c17e221510e4a2e71175c78a6faf0e0e3159c761bc9b94
|
|
disable-asm option
f8a06f7a02be83e9b76a1b31f1b66a965dbedfce doc: remove references to disable-asm option now that it's gone (Cory Fields)
376f0f6d0798c10f09266d609afea3ada1b99f9b build: remove confusing and inconsistent disable-asm option (Cory Fields)
Pull request description:
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.
Additionally, this is one of the last (THE last?) remaining uses of autoconf defines in our crypto code. As such it seems like low-hanging fruit.
ACKs for top commit:
fanquake:
ACK f8a06f7a02be83e9b76a1b31f1b66a965dbedfce
Tree-SHA512: 4a99c2130225acbe9dc7399ed572a04ca155cbfa3eef8178a632ba533017d264691e6482cceb1d8f9c5d768619d99a2466dea4b82b27b18b872bceae91b92fbb
|
|
a8c3454ba137dfac20b3c89bc558374de0524114 test: speedup bip324_cipher.py unit test (Sebastian Falbesoner)
Pull request description:
Executing the unit tests for the bip324_cipher.py module currently takes quite long (>60 seconds on my older notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops in `test_fschacha20poly1305aead`:
https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/crypto/bip324_cipher.py#L193-L194
https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/crypto/bip324_cipher.py#L198-L199
Their sole purpose is increasing the FSChaCha20Poly1305 packet counter in order to trigger rekeying, i.e. the actual encryption/decryption is not relevant, as the result is thrown away. This commit speeds up the tests by supporting to pass "None" as plaintext/ciphertext, indicating to the routines that no actual encryption/decryption should be done.
The approach here is a bit hacky, a cleaner alternative would probably be to introduce a special `seek`/`skip_packets` method and not touch the encrypt/decrypt routines, but that seemed overkill to me only for speeding up a unit test. Open for suggestions.
master branch:
```
$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
----------------------------------------------------------------------
Ran 2 tests in 64.658s
```
PR branch:
```
$ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py
..
----------------------------------------------------------------------
Ran 2 tests in 0.822s
```
ACKs for top commit:
delta1:
Concept ACK a8c3454
epiccurious:
Tested ACK a8c3454ba137dfac20b3c89bc558374de0524114.
achow101:
ACK a8c3454ba137dfac20b3c89bc558374de0524114
marcofleon:
ACK a8c3454ba137dfac20b3c89bc558374de0524114. The comments at the top of `bip324_cipher.py` specify that this should only be used for testing, so I think this optimization makes sense in that context.
cbergqvist:
ACK a8c3454!
stratospher:
ACK a8c3454. I think it's worth it because of the significant speedup in the unit test.
Tree-SHA512: 737dd805a850be6e035aa3c6d9e2c5b5b5e89ddc564f84a045c37e0238fef6419912de7c902139b64914abdd647c649fe02a694f1a5e1741d7d4459c041caccc
|
|
|
|
validation_tests
6ee3997d03e456655e3c44abf1e15270c423ed41 test: removes unnecessary check from validation_tests (Sergi Delgado Segura)
Pull request description:
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.
Notice that a proper check to test the duplication case is added a few lines later, so this check is just meaningless and can be removed. Check https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1506490281 for context.
ACKs for top commit:
maflcko:
ACK 6ee3997d03e456655e3c44abf1e15270c423ed41
dergoegge:
utACK 6ee3997d03e456655e3c44abf1e15270c423ed41
BrandonOdiwuor:
utACK 6ee3997d03e456655e3c44abf1e15270c423ed41
Tree-SHA512: e4627668091dda5f589e4c15edac39dc84aabc9b34b8f7fadbf512beb7111d5477e1b69567a34b4a657e48ba66dfb864db5ff37c9bbe3ff24cd32931b2dd89e6
|
|
The comment about sha256_sse4::Transform is believed to be old and stale.
|
|
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.
|
|
`getnewaddress` failures should not affect keypools for descriptor wallets
e073f1dfda7a2a2cb2be9fe2a1d576f122596021 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6)
367bb7a80cc71130995672c853d4a6e0134721d6 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 e073f1dfda7a2a2cb2be9fe2a1d576f122596021 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 e073f1dfda7a2a2cb2be9fe2a1d576f122596021
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/29510/commits/e073f1dfda7a2a2cb2be9fe2a1d576f122596021
Tree-SHA512: 1128288a60dd4d8f306ef6f7ac66cdfeae3c9cc35c66ecada2d78fa61ac759f2a757b70fc3976ba8b5081200942b58dfabc184c01ccf911af40ba8c145344651
|
|
--v2transport
0487f91a2046c0d6f91ccaedeb00fbefa635c66d 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 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
mzumsande:
Code Review ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
achow101:
ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
vasild:
ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d
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>
|
|
d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc [test] IsBlockMutated unit tests (dergoegge)
1ed2c9829700054526156297552bb47230406098 Add transaction_identifier::size to allow Span conversion (dergoegge)
1ec6bbeb8d27d31647d1433ccb87b362f6d81f90 [validation] Cache merkle root and witness commitment checks (dergoegge)
5bf4f5ba32da4627f152b54d266df9b2aa930457 [test] Add regression test for #27608 (dergoegge)
49257c0304828a185c273fcb99742c54bbef0c8e [net processing] Don't process mutated blocks (dergoegge)
2d8495e0800f5332748cd50eaad801ff77671bba [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge)
66abce1d98115e41f394bc4f4f52341960ddc839 [validation] Introduce IsBlockMutated (dergoegge)
e7669e1343aec4298fd30d539847963e6fa5619c [refactor] Cleanup merkle root checks (dergoegge)
95bddb930aa72edd40fdff52cf447202995b0dce [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 d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
maflcko:
ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc 🏄
fjahr:
Code review ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
sr-gi:
Code review ACK https://github.com/bitcoin/bitcoin/commit/d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc
Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f
|
|
1484998b6b08c93714325952bd94dd8a2de446ae 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:
![Screenshot 2024-02-28 at 13 47 50](https://github.com/bitcoin/bitcoin/assets/1204616/e01bbba8-e2ad-419f-95d1-925d54b3e87a)
After:
![Screenshot 2024-02-28 at 13 54 22](https://github.com/bitcoin/bitcoin/assets/1204616/e8917376-c8ca-443e-91c7-a73064bd787b)
Should the individual python test runners print the python version instead or also?
ACKs for top commit:
hebasto:
ACK 1484998b6b08c93714325952bd94dd8a2de446ae.
Tree-SHA512: 6d084ff4a667156fa8797450de83bbcf596ddd3b2fa8ec04c1ca9a532a6fec716817b66da34db4ea0184bd802ef613e2b8f6142be9a511c5397785cfbfede0c3
|
|
`system_tests/run_command`
51bc1c7126d6e130bc40c529fb71ae6486da0492 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 51bc1c7126d6e130bc40c529fb71ae6486da0492
theStack:
Code-review ACK 51bc1c7126d6e130bc40c529fb71ae6486da0492
Tree-SHA512: 0e3875c4dc20564332555633daf2227223b10dc3d052557635eced2734575d1e0252fb19e46ea6e6c47a15c51c345f70b6d437e33435abcd0e4fcf29edb50887
|
|
5d45552fd4303f8d668ffbc50cce1053485aeead Squashed 'src/crc32c/' changes from 0bac72c455..b60d2b7334 (fanquake)
Pull request description:
Update the crc32c subtree. Includes:
* https://github.com/bitcoin-core/crc32c-subtree/pull/6
Which fixes #29178.
ACKs for top commit:
hebasto:
ACK 359a8d98468aa4f00be349ccbfc869d797ee807d.
theuni:
ACK 359a8d98468aa4f00be349ccbfc869d797ee807d
dergoegge:
ACK 359a8d98468aa4f00be349ccbfc869d797ee807d
Tree-SHA512: 2cec81a34ad26bbbc298aea5daffa41e56114d31cc2eb5fe486f46a77c3467bba22bdeca1c52ae97220e119d98818304272fc6337442af55282accabcd4c5833
|
|
detection with c++20 concept
ad7584d8b60119ca3717117a1eb6a16d753c5d74 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 https://github.com/bitcoin/bitcoin/commit/ad7584d8b60119ca3717117a1eb6a16d753c5d74
Tree-SHA512: 1faf65c900700efb1cf3092c607a2230321b393cb2f029fbfb94bc8e50df1dabd7a9e4b91e3b34f0d2f3471aaf18ee7e56d91869db5c5f4bae84da95443e1120
|
|
b052b2d1f2b220582a933eb5fa6a28144bed07d8 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 https://github.com/bitcoin/bitcoin/pull/29486/commits/b052b2d1f2b220582a933eb5fa6a28144bed07d8
theuni:
utACK b052b2d1f2b220582a933eb5fa6a28144bed07d8
Tree-SHA512: 8b55f366dfeece082090fb87de67d8811967f4c89987a346431b2deb73c3c94401b59ec98bb1cbf790e18894f3d4c4aebb57cbc5fbf931c1046bf40239bc7a58
|
|
|
|
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.
|
|
`getrawchangeaddress`/`getnewaddress` failures
|
|
`GetReservedDestination` fails
|
|
6fa61e35320ac2bc623a9c9ca11b270b34e2d05a doc: Fix Broken Links (Justin Dhillon)
Pull request description:
### Summery
Here is what I have fixed:
http://voorloopnul.com/blog/a-python-netstat-in-less-than-100-lines-of-code/
--> https://web.archive.org/web/20190424172231/http://voorloopnul.com/blog/a-python-netstat-in-less-than-100-lines-of-code/
### Support my work
These links were found with [link-inspector](https://github.com/justindhillon/link-inspector). If you find this PR useful, give the repo a ⭐
ACKs for top commit:
fjahr:
ACK 6fa61e35320ac2bc623a9c9ca11b270b34e2d05a
Tree-SHA512: ba83badfc8a89f33813801f749bcd7ad41d4c9c817ece76f3bb1b60f24c28e99cfccc485a0ba059ec2c1134e8ffb5fa37fdc9835e553229ee5b1167c9b2e8d1f
|
|
|
|
This removes the only remaining autoconf macro in our serialization code,
so it can now be used trivially and safely out-of-tree.
|
|
|
|
b60d2b7334 Merge bitcoin-core/crc32c-subtree#6: Fix UBSan "misaligned-pointer-use" warning on aarch64
1ac401e32b Fix UBSan "misaligned-pointer-use" warning on aarch64
git-subtree-dir: src/crc32c
git-subtree-split: b60d2b733406cc64025095c6c2cb3933e222b529
|
|
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.
|
|
|
|
|
|
Slight performance improvement by avoiding duplicate work.
|
|
|
|
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).
|
|
|
|
|
|
|
|
|
|
fccfdb25b2c337bbd8b283c27493f10d5e02b5d4 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 fccfdb25b2c337bbd8b283c27493f10d5e02b5d4
theStack:
lgtm ACK fccfdb25b2c337bbd8b283c27493f10d5e02b5d4
Tree-SHA512: be6d22b605140b37a71e11c5bbed54f60655832d78cd3cb221eddc77c7621a65c0d71baf436f90819be536d9b5dbf1a0b2c82b6b23d62356addc495403f2ba35
|
|
Now that --enable-suppress-external-warnings is on by default, we can
drop it.
|
|
--v2transport is enabled
bf5662c678455fb47c402f8520215726ddfe7a94 test: enable v2 for python p2p depending on global --v2transport flag (Martin Zumsande)
6e9e39da434f8dafacee4cba068daf499bdb4cc2 test: Don't use v2transport when it's too slow. (Martin Zumsande)
87549c8f89fe8c9f404b74c5a40b5ee54c5a966d test: enable p2p_invalid_messages.py with v2transport (Martin Zumsande)
5fc9db504b9ac784019e7e8c215c31abfccb62b6 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 bf5662c678455fb47c402f8520215726ddfe7a94
vasild:
ACK bf5662c678455fb47c402f8520215726ddfe7a94
stratospher:
reACK bf5662c6.
theStack:
Tested ACK bf5662c678455fb47c402f8520215726ddfe7a94
Tree-SHA512: 4f5a08248ba8a755f7d0f48deb2b79bef03292345cacb7deef01be955481093800e4e56ff218ea56734eef5de1fb3ab0f04657447ea27d393bb536539d7b289d
|
|
fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde fuzz: Set -rss_limit_mb=8000 for generate as well (MarcoFalke)
fa4e396e1da8e5b04a5f906b95017b969ea37bae 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 fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde
dergoegge:
utACK fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde
brunoerg:
light ACK fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde
Tree-SHA512: bfd04a76ca09aec612397bae5f3f263a608faa7087697169bd4c506c8195c4d2dd84ddc7fcd3ebbc75771eab618fad840af819114968ca3668fc730092376768
|
|
call for MinGW builds
d2fe90571e98e02617682561ea82acb1e2647827 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 d2fe90571e98e02617682561ea82acb1e2647827
fanquake:
ACK d2fe90571e98e02617682561ea82acb1e2647827 - the plan here should still be to migrate to the newer windows runtime.
Tree-SHA512: 0269b66531e58c093ecda3a3e355a20ee8274e165d7e010f8f125881b3c8d4cfe801abdca4605d81efd3b2dbe9a81896968971f6f53da7f6c6093b76b47c5bc9
|
|
bit_width is a drop-in replacement with an exact meaning in c++, so there is
no need to continue testing/fuzzing/benchmarking.
|