fab1f4b800d007bd4756b2519c64e1506ffe0d6c rpc: [mempool] Remove erroneous Univalue integral casts (MarcoFalke)
Pull request description:
Casting without reason can only be confusing (because it is not needed), or wrong (because it does the wrong thing).
For example, the added test that adds a positive chunk prioritization will fail:
```
AssertionError: not(-1.94936096 == 41.000312)
```
Fix all issues by removing the erroneous casts, and by adding a test to check against regressions.
ACKs for top commit:
rkrux:
tACK fab1f4b800d007bd4756b2519c64e1506ffe0d6c
pablomartin4btc:
ACK fab1f4b800d007bd4756b2519c64e1506ffe0d6c
glozow:
ACK fab1f4b800d007bd4756b2519c64e1506ffe0d6c
Tree-SHA512: b03c888ec07a8bdff25f7ded67f253b2a8edd83adf08980416e2ac8ac1b36ad952cc5828be833d19f64a55abab62d7a1c6f181bc5f1388ed08cc178b4aaec6ee
337b4a23690bd20eaf513aa29e8d5122f6b8a129 Remove stale rationale paragraph (flack)
Pull request description:
It belonged to the note removed in #33892
ACKs for top commit:
instagibbs:
ACK 337b4a23690bd20eaf513aa29e8d5122f6b8a129
Tree-SHA512: 3cb1d3b87aa42ff92130af10ce2c286c0d83cbfdf17096d47b540ffe8e1a9a4727aedb8d477599fbff0002d7e262a6a52549dcccfa38dbe61281c221cf26cae2
fa66e2d07a4b87d62382a54acf5fab6af77be24e refactor: [rpc] Remove confusing and brittle integral casts (MarcoFalke)
Pull request description:
When constructing an UniValue from integral values, historically (long ago), in some cases casts where needed. With the current UniValue constructor, only very few are actually needed.
Keeping the unused casts around is:
* confusing, because code readers do not understand why they are needed
* brittle, because some may copy them into new places, where they will lead to hard-to-find logic bugs, such as the ones fixed in pull https://github.com/bitcoin/bitcoin/pull/34112
So fix all issues by removing them, except for a few cases, where casting was required:
* `ret.pushKV("coinbase", static_cast<bool>(coin->fCoinBase));`, or
* `static_cast<std::underlying_type_t<decltype(info.nServices)>>(info.nServices)`.
This hardening refactor does not fix any bugs and does not change any behavior.
ACKs for top commit:
sedited:
ACK fa66e2d07a4b87d62382a54acf5fab6af77be24e
rkrux:
ACK fa66e2d07a4b87d62382a54acf5fab6af77be24e
Tree-SHA512: 13c9c59ad021ea03cdabe10d58850cef96d792634c499e62227cc2e7e5cace066ebd9a8ef3f979eaba98cadf8a525c6e6df909a07115559c0450bd9fc3a9763e
44e006d4383155f254f908ada91c2d9a7a65db6c [kernel] Expose reusable PrecomputedTransactionData in script valid (Josh Doman)
Pull request description:
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bitcoinkernel/issues/46). The design of this PR is inspired by @sedited's comments.
The PR introduces three new APIs for managing the `btck_PrecomputedTransactionData` object:
```c
/**
* @brief Create precomputed transaction data for script verification.
*
* @param[in] tx_to Non-null.
* @param[in] spent_outputs Nullable for non-taproot verification. Points to an array of
* outputs spent by the transaction.
* @param[in] spent_outputs_len Length of the spent_outputs array.
* @return The precomputed data, or null on error.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_create(
const btck_Transaction* tx_to,
const btck_TransactionOutput** spent_outputs, size_t spent_outputs_len) BITCOINKERNEL_ARG_NONNULL(1);
/**
* @brief Copy precomputed transaction data.
*
* @param[in] precomputed_txdata Non-null.
* @return The copied precomputed transaction data.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_copy(
const btck_PrecomputedTransactionData* precomputed_txdata) BITCOINKERNEL_ARG_NONNULL(1);
/**
* Destroy the precomputed transaction data.
*/
void btck_precomputed_transaction_data_destroy(btck_PrecomputedTransactionData* precomputed_txdata);
```
The PR also modifies `btck_script_pubkey_verify` so that it accepts `precomputed_txdata` instead of `spent_outputs`:
```c
/**
* @brief Verify if the input at input_index of tx_to spends the script pubkey
* under the constraints specified by flags. If the
* `btck_ScriptVerificationFlags_WITNESS` flag is set in the flags bitfield, the
* amount parameter is used. If the taproot flag is set, the precomputed data
* must contain the spent outputs.
*
* @param[in] script_pubkey Non-null, script pubkey to be spent.
* @param[in] amount Amount of the script pubkey's associated output. May be zero if
* the witness flag is not set.
* @param[in] tx_to Non-null, transaction spending the script_pubkey.
* @param[in] precomputed_txdata Nullable if the taproot flag is not set. Otherwise, precomputed data
* for tx_to with the spent outputs must be provided.
* @param[in] input_index Index of the input in tx_to spending the script_pubkey.
* @param[in] flags Bitfield of btck_ScriptVerificationFlags controlling validation constraints.
* @param[out] status Nullable, will be set to an error code if the operation fails, or OK otherwise.
* @return 1 if the script is valid, 0 otherwise.
*/
int btck_script_pubkey_verify(
const btck_ScriptPubkey* script_pubkey,
int64_t amount,
const btck_Transaction* tx_to,
const btck_PrecomputedTransactionData* precomputed_txdata,
unsigned int input_index,
btck_ScriptVerificationFlags flags,
btck_ScriptVerifyStatus* status) BITCOINKERNEL_ARG_NONNULL(1, 3);
```
As before, an error is thrown if the taproot flag is set and `spent_outputs` is not provided in `precomputed_txdata` (or `precomputed_txdata` is null). For simple single-input non-taproot verification, `precomputed_txdata` may be null, and the kernel will construct the precomputed data on-the-fly.
Both the C++ wrapper and the test suite are updated with the new API. Tests cover both `precomputed_txdata` reuse and nullability.
Appreciate feedback on this concept / approach!
ACKs for top commit:
sedited:
Re-ACK 44e006d4383155f254f908ada91c2d9a7a65db6c
stringintech:
ACK 44e006d
Tree-SHA512: 1ed435173e6ff4ec82bc603194cf182c685cb79f167439a442b9b179a32f6c189c358f04d4cb56d153fab04e3424a11b73c31680e42b87b8a6efcc3ccefc366c
5646e6c0d3581f12419913b88745f51c7a3161b9 index: restrict index helper function to namespace (Martin Zumsande)
032f3503e3fe8e144640904670867afc18a4bbbf index, refactor: deduplicate LookUpOne (Martin Zumsande)
a67d3eb91d5ef840fc49167d7048a33872ecddf8 index: deduplicate Hash / Height handling (Martin Zumsande)
Pull request description:
The logic for `DBHashKey` / `DBHeightKey` handling and lookup of entries is shared by `coinstatsindex` and `blockfilterindex`, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/db_key.h` (using templates for the index-specific `DBVal`).
ACKs for top commit:
fjahr:
re-ACK 5646e6c0d3581f12419913b88745f51c7a3161b9
furszy:
utACK 5646e6c0d3581f12419913b88745f51c7a3161b9
sedited:
ACK 5646e6c0d3581f12419913b88745f51c7a3161b9
Tree-SHA512: 6f41684d6a9fd9bb01239e9f2e39a12837554f247a677eadcc242f0c1a2d44a79979f87249c4e0305ef1aa708d7056e56dfc40e1509c6d6aec2714f202fd2e09
e44dec027ceec2a5f74b65636689a51833d78a94 add release note about supporing non-TRUC <minrelay txns (Greg Sanders)
1488315d76ee40b9d021b7d0ecd01207eee4a426 policy: Allow any transaction version with < minrelay (Greg Sanders)
Pull request description:
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.
In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the anti-pinning protections.
ACKs for top commit:
ajtowns:
ACK e44dec027ceec2a5f74b65636689a51833d78a94 - lgtm
ismaelsadeeq:
ACK e44dec027ceec2a5f74b65636689a51833d78a94
Tree-SHA512: 6fd1a2429c55ca844d9bd669ea797e29eca3f544f0b5d3484743d3c1cdf4364f7c7a058aaf707bcfd94b84c621bea03228cb39487cbc23912b9e0980a1e5b451
fa727e3ec984106371eeedb34d7bbbbc3dcce4ff test: Avoid hard time.sleep(1) in feature_init.py (MarcoFalke)
Pull request description:
Using a hard-coded `time.sleep` in the tests is usually confusing and brittle. For example, the one in `break_wait_test`:
* Is confusing, because it does not explain why it is needed.
* On fast hardware will just lead to a useless delay.
* On slow hardware may lead to an intermittent, and confusing test failure.
Fix all issues by replacing it with the proper condition to wait on.
ACKs for top commit:
Sjors:
utACK fa727e3ec984106371eeedb34d7bbbbc3dcce4ff
rkrux:
tACK fa727e3
janb84:
tACK fa727e3ec984106371eeedb34d7bbbbc3dcce4ff
Tree-SHA512: 7b59496a1b9b8044548423ad517ff03e98521685cf65499cd0ef499d6fd3d72ad374c92ca815436675ed6ae7be508a5a1afce699b804a384d7aee6a195d8d972
This function is a duplicate of HasEncryptionKeys().
-BEGIN VERIFY SCRIPT-
sed -i '/bool IsCrypted() const;/d' src/wallet/wallet.h
sed -i '/^bool CWallet::IsCrypted() const$/,/^}$/{/^}$/N;d;}' src/wallet/wallet.cpp
sed -i --regexp-extended 's/IsCrypted\(\)/HasEncryptionKeys()/g' $(git ls-files '*.cpp' '*.h')
-END VERIFY SCRIPT-
217dbbbb5e38ab582ee0b3ef37fe9e99d887d7c8 test: Add musig failure scenarios (Fabian Jahr)
c9519c260b7a13d2a8104ff9842205655aa65ace musig: Check session id reuse (Fabian Jahr)
e755614be586999206fe73b21adfa8b5f8dd0360 sign: Remove duplicate sigversion check (Fabian Jahr)
0f7f0692ca1e60231cd7ba65aa3606c8da33afca musig: Move MUSIG_CHAINCODE to musig.cpp (Fabian Jahr)
Pull request description:
This is a follow-up to #29675 and primarily adds test coverage for some of the most prominent failure cases in the last commit.
The following commits address a few left-over nit comments that didn't make it in before merge.
ACKs for top commit:
achow101:
ACK 217dbbbb5e38ab582ee0b3ef37fe9e99d887d7c8
rkrux:
lgtm ACK 217dbbb
Tree-SHA512: d73807bc31791ef1825c42f127c7ddfbc70b2b7cf782bc11341666e32e86b787ffc7aed64caea992909cef3a85fc6629282d8209c173aadec77f72fd0da96c45
1ed8e7616527c69dbaa9904cda59e3b73c29fa5d rpc, doc: clarify the response of listtransactions RPC (rkrux)
Pull request description:
I noticed this behaviour while perf testing PR #27286 and it was not something that I expected, updating the doc to make it present in the RPCHelp command.
ACKs for top commit:
achow101:
ACK 1ed8e7616527c69dbaa9904cda59e3b73c29fa5d
furszy:
ACK 1ed8e7616527c69dbaa9904cda59e3b73c29fa5d
musaHaruna:
ACK [1ed8e76](1ed8e76165) since my last review. New changes looks good, it's much easier to understand as well, looking at it from a user's perspective.
Tree-SHA512: 893a8e259201ac2140f46f827d81e681d2ec478c9571cceb10864aaa1b941991ce2263357d7c2b0024c04a9f8fbc372a020104b26e022c96289d271675947033
1841bf9cb67bca99273b3efa38633113422f5882 test: address self-announcement (0xb10c)
Pull request description:
Test that a node sends a self-announcement with its external IP to in- and outbound peers after connection open and again sometime later.
Since the code for the test is mostly the same for addr and addrv2 messages, I opted to add a new test file instead of having duplicate code in `p2p_addr_relay.py` and `p2p_addrv2_relay.py`.
ACKs for top commit:
Bicaru20:
ACK 1841bf9cb6
achow101:
ACK 1841bf9cb67bca99273b3efa38633113422f5882
rkrux:
ACK 1841bf9
fjahr:
Code review ACK 1841bf9cb67bca99273b3efa38633113422f5882
Tree-SHA512: 692a01e9f10eb55ee870de623e85182a10a75225766e0f0251ad5d9e369537ec27ca6e06905374190f3afe00ba6f71ae72f262228baaa535238a87160e1ce4f1
56750c4f87d089c6a3f093eb2bf2edd07170d4a8 iwyu, clang-format: Sort includes (Hennadii Stepanov)
2c78814e0e182853ce44d9fd63d24ee6cab5223e ci: Add IWYU job (Hennadii Stepanov)
94e4f04d7cf4b0fef9a28d3771e73f1dc9fb0528 cmake: Fix target name (Hennadii Stepanov)
0f81e005197fa4201a38e635ddf8c5dcc12a3878 cmake: Make `codegen` target dependent on `generate_build_info` (Hennadii Stepanov)
73f7844cdb1e225099223a355d88da0522d7d69b iwyu: Add patch to prefer C++ headers over C counterparts (Hennadii Stepanov)
7a65437e23706e4820392dc456c3acccbf196dd6 iwyu: Add patch to prefer angled brackets over quotes for includes (Hennadii Stepanov)
Pull request description:
This PR separates the IWYU checks into its own CI job to provide faster feedback to developers. No other changes are made to the treatment of IWYU warnings. The existing “tidy” CI job will no longer run IWYU.
See also the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Based on ideas from https://github.com/bitcoin/bitcoin/pull/32953.
ACKs for top commit:
maflcko:
review ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8 🌄
sedited:
ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8
Tree-SHA512: af15326b6d0c5d1e11346ac64939644936c65eb9466cd1a17ab5da347d39aef10f7ab33b39fbca31ad291b0b4b54639b147b24410f4f86197e4a776049882694
d7de5b109f69293b375729363b4e5038f3fb6b15 logs: show reindex progress in `ImportBlocks` (Lőrinc)
Pull request description:
### Summary
When triggering a reindex, users have no indication of progress.
### Fix
This patch precomputes the total number of block files so progress can be shown.
Instead of only displaying which block file is being processed, it now shows the percent complete.
### Reproducer + expected results
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Release && make -C build -j && ./build/bin/bitcoind -datadir=demo -reindex
```
Before, the block files were shown one-by-one, there's no way to see how much work is left:
```
Reindexing block file blk00000.dat...
Loaded 119920 blocks from external file in 1228ms
Reindexing block file blk00001.dat...
Loaded 10671 blocks from external file in 284ms
Reindexing block file blk00002.dat...
Loaded 5459 blocks from external file in 263ms
Reindexing block file blk00003.dat...
Loaded 5595 blocks from external file in 267ms
```
After the change we add a percentage:
```
Reindexing block file blk00000.dat (0% complete)...
Loaded 119920 blocks from external file in 1255ms
Reindexing block file blk00001.dat (1% complete)...
Loaded 10671 blocks from external file in 303ms
Reindexing block file blk00002.dat (2% complete)...
Loaded 5459 blocks from external file in 278ms
Reindexing block file blk00003.dat (3% complete)...
Loaded 5595 blocks from external file in 285ms
```
ACKs for top commit:
enirox001:
Concept ACK d7de5b1
rkrux:
lgtm ACK d7de5b109f69293b375729363b4e5038f3fb6b15
danielabrozzoni:
tACK d7de5b109f69293b375729363b4e5038f3fb6b15 - code reviewed and tested on my archival node.
maflcko:
review ACK d7de5b109f69293b375729363b4e5038f3fb6b15 💇
Tree-SHA512: 359a539b781ad8b73e2a616c951567062a76be27cf90e5b88bb5309295af9cd7994e327f185bacc1482b43b892b38329593b4043a5e71d8800e3e4b7a3954310
356883f0e48be59bcb154096cef82cbf3f0dd9d8 qa-tests: Log expected output in debug (Hodlinator)
7427a03b5ac9e746c0262a4b2952d2f1bdbd88d0 qa-tests: Add test for timeouts due to missing init errors (Hodlinator)
d7f703c1f1a88e2fa7226adf43c45a794b4e07c6 refactor(qa-tests): Extract InternalDurationTestMixin for use in next commit (Hodlinator)
69bcfcad8c3d1464352ba58bad38225cbd195f18 fix(qa-tests): Bring back decoding of exception field (Hodlinator)
fb43b2f8cc4ce53315fd3c84c6a8457388664ddf qa: Improve assert_start_raises_init_error output (Hodlinator)
Pull request description:
Raising a new exception from within a Python `except`-block, as `assert_start_raises_init_error()` does, causes the interpreter to generate extra error output which is unnecessary in this case.
<details><summary>Example output before & after this PR</summary>
Before:
```
2025-07-08T20:05:48.407001Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_node.py", line 686, in assert_start_raises_init_error
ret = self.process.wait(timeout=self.rpc_timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/nix/store/fqm9bqqlmaqqr02qbalm1bazp810qfiw-python3-3.12.9/lib/python3.12/subprocess.py", line 1266, in wait
return self._wait(timeout=timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/nix/store/fqm9bqqlmaqqr02qbalm1bazp810qfiw-python3-3.12.9/lib/python3.12/subprocess.py", line 2053, in _wait
raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command '['/home/hodlinator/bitcoin/build/bin/bitcoind', '-datadir=/tmp/bitcoin_func_test_v96lkcq8/eb2665c7/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-debugexclude=rand', '-uacomment=testnode0', '-disablewallet', '-logthreadnames', '-logsourcelocations', '-loglevel=trace', '-v2transport=0']' timed out after 3 seconds
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 186, in main
self.setup()
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 358, in setup
self.setup_network()
File "/home/hodlinator/bitcoin/build/test/functional/feature_framework_startup_failures.py", line 151, in setup_network
self.nodes[0].assert_start_raises_init_error()
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_node.py", line 716, in assert_start_raises_init_error
self._raise_assertion_error(assert_msg)
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_node.py", line 196, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] bitcoind should have exited within 3s with an error
```
After:
```
2025-07-08T20:09:15.330589Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 186, in main
self.setup()
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 358, in setup
self.setup_network()
File "/home/hodlinator/bitcoin/build/test/functional/feature_framework_startup_failures.py", line 151, in setup_network
self.nodes[0].assert_start_raises_init_error()
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_node.py", line 720, in assert_start_raises_init_error
self._raise_assertion_error(assert_msg)
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_node.py", line 196, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] bitcoind should have exited within 3s with an error (cmd: ['/home/hodlinator/bitcoin/build/bin/bitcoind', '-datadir=/tmp/bitcoin_func_test_v96lkcq8/eb2665c7/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-debugexclude=rand', '-uacomment=testnode0', '-disablewallet', '-logthreadnames', '-logsourcelocations', '-loglevel=trace', '-v2transport=0'])
```
</details>
---
Can be tested on this PR by:
1. Execute test containing new test case:
```shell
build/test/functional/feature_framework_startup_failures.py -ldebug > after.log
```
2. Drop first commit which contains the fix.
3. Re-run test:
```shell
build/test/functional/feature_framework_startup_failures.py -ldebug > before.log
```
4. Diff logs, focusing on `TestInitErrorTimeout OUTPUT` sections.
---
Found while testing #32835 using the suggested method (https://github.com/bitcoin/bitcoin/pull/32835#issue-3188748624) which triggered expected timeouts, but with the extra error noise.
ACKs for top commit:
l0rinc:
ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8
ryanofsky:
Code review ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8. Thanks for the updates! Just rearranged commits and made minor changes in "missing init errors" test since last review
furszy:
Code ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8
Tree-SHA512: 01f2f1f6a5e79cf83a39a143cfb8b2bb8360e0402e91a97a7df8254309fd4436a55468d11825093c052010bfce57f3461d912a578cd2594114aba435ab48b999
d3a479cb077d9a9a4451dc4ecb43fe0daf94f172 kernel: Move BlockInfo to a kernel file (TheCharlatan)
d69a582e72ea6eee67b5f659dd57c75e936a98dc kernel: Remove some unnecessary non-kernel includes (TheCharlatan)
Pull request description:
Found these while attempting to isolate the kernel library sources into their own repository. There still is no mechanism for preventing including headers into the kernel library that don't belong to kernel modules, but it is also fairly straight forward to correct manually for now. However, the changes here might be incomplete.
ACKs for top commit:
hebasto:
re-ACK d3a479cb077d9a9a4451dc4ecb43fe0daf94f172.
maflcko:
review ACK d3a479cb077d9a9a4451dc4ecb43fe0daf94f172 🦏
janb84:
ACK d3a479cb077d9a9a4451dc4ecb43fe0daf94f172
Tree-SHA512: b2a40aa758437a4e72648fe38ca308c0bea3a7d8559c62182cd3daa2858de62b7418afe4b9054ebdb88082036bc1691803c2b3b2dacd0ff2208a9ffdcba0e7e9
This should avoid having to include interfaces/chain.h from a kernel
module. interfaces/chain.h in turn includes a bunch of non-kernel
headers, that break the desired library topology and might introduce
entanglement regressions.
Specifically gets rid of batchpriority, chainparams, script/sign.h and
system includes.
Also take the opportunity of cleaning up the headers for the effected
files and adding them to the iwyu-enforced set.
fa4cb13b52030c2e55c6bea170649ab69d75f758 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f29774872d18febc0df38831a6e45f3de69cc scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a9921235d1ecf8c3c7dc1836ec68131) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
rkrux:
re-ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
janb84:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
1e94e562f76e6152dffb2a2d07dc3429137098b5 refactor: enable `readability-container-contains` clang-tidy rule (Lőrinc)
fd9f1accbda9e81b2c5290b2056b25f02152a607 Fix compilation for old Boost versions (Lőrinc)
Pull request description:
Replace the last few instances of `.count() != 0` and `.count() == 0` and bare `count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* other bare `std::unordered_set` and `std::map` count calls.
Also fixes https://github.com/bitcoin/bitcoin/issues/34101 by reverting `boost::multi_index::contains` calls not available in our minimum supported version.
With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent future regressions.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33192
ACKs for top commit:
hebasto:
ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5.
pablomartin4btc:
re-ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5
janb84:
ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5
rkrux:
re-ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5
Tree-SHA512: d54a7821d319bf0d60b6c3a870917464a7d5b9279c6a86708c03a3516ec23bbf18f0e83de62b3b2b1607de96e1470f1144b4918d69a6c770e6b7e09863e7dbac
fa336053aada79d13cd771ce025857256814465e Move ci_exec to the Python script (MarcoFalke)
fa83555d163ff7fdcdaaa0e34bfa3eaa41fa6dfc ci: Require rsync to pass (MarcoFalke)
eeee02ea53dd1a3fb2eb62acd68fbd797d9b9ba8 ci: Untangle CI_EXEC bash function (MarcoFalke)
fa21fd1dc2e5649f8c4e7c04d28312beb51761fb ci: Move macos snippet under DANGER_RUN_CI_ON_HOST (MarcoFalke)
fa37559ac5b7bf83eefa30e7770ccae9fd19556b ci: Document the retry script in PATH (MarcoFalke)
666675e95fe823b7809f64508aea5b57b1867c19 ci: Move folder creation and docker kill to Python script (MarcoFalke)
Pull request description:
The remaining `ci/test/02_run_container.sh` is fine, but has a bunch of shellcheck SC2086 word splitting violations.
This is fine currently, because the only place that needed them had additional escaping, and all other commands happened to split fine on spaces.
However, this may change in the future. So fix it now, by rewriting it in Python, which is recommended in the dev notes.
ACKs for top commit:
frankomosh:
Code Review ACK [fa33605](fa336053aa)
m3dwards:
ACK fa336053aada79d13cd771ce025857256814465e
Tree-SHA512: 472decb13edca75566dffe49b9b3f554ab977fa60ec7902d5a060fe53381aee8606a10ff0c990a62ee2454dc6d9430cc064f58320b9043070b7bf08845413bf4
f480c1e7177744d11b058c3a9422975d7ec1af46 build: Update minimum required Boost version (Hennadii Stepanov)
Pull request description:
Building with Boost 1.73.0 has been [broken](https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3594758109) since https://github.com/bitcoin/bitcoin/pull/31122 was merged.
This PR updates the minimum required Boost version to 1.74.0.
According to https://repology.org/project/boost/versions, none of the major distros are affected by this change.
ACKs for top commit:
maflcko:
lgtm ACK f480c1e7177744d11b058c3a9422975d7ec1af46
l0rinc:
untested ACK f480c1e7177744d11b058c3a9422975d7ec1af46
pablomartin4btc:
utACK f480c1e7177744d11b058c3a9422975d7ec1af46
janb84:
ut ACK f480c1e7177744d11b058c3a9422975d7ec1af46
Tree-SHA512: 6af72a001a566fb5a7b60e23bdb9619e87f277a1a3928ceb304bd35b8b35f56e4d38f25983db9a8732ecdb957cec9850a0fcf6719f3a65d903872e63d80b4d7c
75bdb925f404f41874adf0fcefca0f1641fcb4e6 clusterlin: drop support for improvable chunking (simplification) (Pieter Wuille)
91399a79122cb6bb256e634049ac83f53154b64b clusterlin: remove unused MergeLinearizations (cleanup) (Pieter Wuille)
5ce280074512f79a4b3851d8d453f337fa97be46 clusterlin: randomize equal-feerate parts of linearization (privacy) (Pieter Wuille)
13aad26b78481f2abd6d5e3ae6218e46e4d0060a clusterlin: randomize various decisions in SFL (feature) (Pieter Wuille)
ddbfa4dfac7b30d0dc462b29bd4c89b9152d0381 clusterlin: keep FIFO queue of improvable chunks (preparation) (Pieter Wuille)
3efc94d6564deca4e5aaf5219adb71302f522657 clusterlin: replace cluster linearization with SFL (feature) (Pieter Wuille)
6a8fa821b80cf71e199b085c60d77f6ca533f6ee clusterlin: add support for loading existing linearization (feature) (Pieter Wuille)
da48ed9f348a8a93679fd623838059f04ac9e53c clusterlin: ReadLinearization for non-topological (tests) (Pieter Wuille)
c461259fb62982fd054cdf8e8fac8016e8dd9a04 clusterlin: add class implementing SFL state (preparation) (Pieter Wuille)
95bfe7d574cfc69bb35e386b88e2c3026b8a7315 clusterlin: replace benchmarks with SFL-hard ones (bench) (Pieter Wuille)
86dd550a9b706195a9d3d6124be3f7d569ccbffe clusterlin: add known-correct optimal linearization tests (tests) (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289.
This replaces the cluster linearization algorithm introduced in #30126 and #30286 (a combination of LIMO with candidate-set search), with a completely different algorithm: [spanning-forest linearization](https://delvingbitcoin.org/t/spanning-forest-cluster-linearization/1419/1), which appears to have much better performance for hard clusters. See [this post](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303/68) for a comparison between various linearization algorithms, and [this post](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303/73) for benchmarks comparing them. Replaying historical mempool data on it shows that it can effectively linearize every observed cluster up to 64 transactions optimally within tens of microseconds, though pathological examples can be created which take longer.
The algorithm is effectively a very specialized version of the [simplex algorithm](https://en.wikipedia.org/wiki/Simplex_algorithm) to the problem of finding high-feerate topological subsets of clusters, but modified to find all consecutive such subsets concurrently rather than just the first one. See the post above for how it is related.
It represents the cluster as partitioned into a set of chunks, each with a spanning tree of its internal dependencies connecting the transactions. Randomized improvements are made by selecting dependencies to add and remove to these spanning trees, merging and splitting chunks, until no more improvements are possible, or a computation budget is reached. Like simplex, it does not necessarily make progress in every step, and thus has no upper bound on its runtime to find optimal, but randomization makes long runtimes very unlikely, and additionally makes it hard to adversarially construct clusters in which the algorithm reliably makes bad choices.
ACKs for top commit:
instagibbs:
reACK 75bdb925f404f41874adf0fcefca0f1641fcb4e6
marcofleon:
reACK 75bdb925f404f41874adf0fcefca0f1641fcb4e6
Tree-SHA512: 189d85b34f0eb847562af7da724c61e39f0a785e24ebe2d4c8ee44698d02bd17842d699987d282a79bd1de30f50de28ec0f11d594ebbfa499f6a9b9ce35aecd8
Replace the last few instances of `.count() != 0` and `.count() == 0` and `.count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* other bare `std::unordered_set` and `std::map` count calls.
With no remaining violations, enable the `readability-container-contains`
clang-tidy check to prevent future regressions.
With MergeLinearizations() gone and the LIMO-based Linearize() replaced by SFL, we do not
need a class (LinearizationChunking) that can maintain an incrementally-improving chunk
set anymore.
Replace it with a function (ChunkLinearizationInfo) that just computes the chunks as
SetInfos once, and returns them as a vector. This simplifies several call sites too.
This places equal-feerate chunks (with no dependencies between them) in random
order in the linearization output, hiding information about DepGraph insertion
order from the output. Likewise, it randomizes the order of transactions within
chunks for the same reason.
This introduces a local RNG inside the SFL state, which is used to randomize
various decisions inside the algorithm, in order to make it hard to create
pathological clusters which predictably have bad performance.
The decisions being randomized are:
* When deciding what chunk to attempt to split, the queue order is
randomized.
* When deciding which dependency to split on, a uniformly random one is
chosen among those with higher top feerate than bottom feerate within
the chosen chunk.
* When deciding which chunks to merge, a uniformly random one among those
with the higher feerate difference is picked.
* When merging two chunks, a uniformly random dependency between them is
now activated.
* When making the state topological, the queue of chunks to process is
randomized.
This introduces a queue of chunks that still need processing, in both
MakeTopological() and OptimizationStep(). This is simultaneously:
* A preparation for introducing randomization, by allowing permuting the
queue.
* An improvement to the fairness of suboptimal solutions, by distributing
the work more fairly over chunks.
* An optimization, by avoiding retrying chunks over and over again which
are already known to be optimal.
This replaces the existing LIMO linearization algorithm (which internally uses
ancestor set finding and candidate set finding) with the much more performant
spanning-forest linearization algorithm.
This removes the old candidate-set search algorithm, and several of its tests,
benchmarks, and needed utility code.
The worst case time per cost is similar to the previous algorithm, so
ACCEPTABLE_ITERS is unchanged.
Rather than using an ad-hoc no-dependency copy of the graph when a potentially
non-topological linearization is needed in the clusterlin fuzz test, add this
directly as a feature in ReadLinearization().
This is preparation for a later commit where another use for such a function
is added.
This adds a data structure representing the optimization state for the spanning-forest
linearization algorithm (SFL), plus a fuzz test for its correctness.
This is preparation for switching over Linearize() to use this algorithm.
See https://delvingbitcoin.org/t/spanning-forest-cluster-linearization/1419 for
a description of the algorithm.
