37d49f5de6 doc: mention Miniscript expressions inside reference (Pieter Wuille)
771f7642bc doc: fix typo in descriptors.md (Pieter Wuille)
708b84999b doc: reference descriptor BIPs in descriptors.md (Pieter Wuille)
8f2a869a19 doc: do not list descriptor RPCs or history (Pieter Wuille)
65a8b6c2ef doc: mention musig() in descriptors.md (Pieter Wuille)
Pull request description:
This brings doc/descriptors.md up to date:
* Stop trying to exhaustively list all RPCs that involve descriptors. They're used everywhere.
* Stop trying to give the history of descriptor support, we have release notes for that.
* Mention that wallets are now built around descriptors (especially with legacy wallets gone).
* Mention `musig()` KEY expressions in the specification part.
* Mention the miniscript expressions in the specification part.
* Reference the relevant output descriptor BIPs in the text.
ACKs for top commit:
achow101:
ACK 37d49f5de6
darosior:
utACK 37d49f5de6
Tree-SHA512: 2581be9b5d1a7099806d6f830b3a452505f8493d0e493a80b8a50e383f93f3e2c8a2d72a64fdae0adfe63d3c2eeb54a61a059108cd861e58c3d85f2bc576364b
2a7a4f608a depends: Allow building Qt packages after interruption (Hennadii Stepanov)
Pull request description:
If a build is interrupted, a standard `mkdir` command may fail if the directory already exists. Switching to `mkdir -p` ensures the build can resume gracefully.
Fix#34712.
ACKs for top commit:
sedited:
ACK 2a7a4f608a
Tree-SHA512: fad2dce89a7cb68a8a539924d98698fe650802d19c84f216fa65e3c23c1a33ab6acf9f4c98c27381194c2958efa92e9dd8fb5e6bd40098efbcc60f156fd45370
5c005363a8 test: improve `wallet_backup` test (rkrux)
04d9515748 test: improve `wallet_assumeutxo` func test (rkrux)
Pull request description:
Relates to #34354
While the actual fix of the issue is in another PR, this one improves the
affected tests by trying to reduce the chain notifications that
need to be processed while simulating erroneous wallet restoration
scenarios.
ACKs for top commit:
maflcko:
lgtm ACK 5c005363a8
furszy:
ACK 5c005363a8
w0xlt:
ACK 5c005363a8
brunoerg:
code review ACK 5c005363a8
Tree-SHA512: 176e3ea8275c7aa082af695f5b76d82c079ff9a7178855b4ce95504edb8ce89b59a772e2d38dd43e997a5bea3d64be700b74cfec7bbc6992538f837877ab7222
f51665bee7 psbt: validate pubkeys in MuSig2 pubnonce/partial sig deserialization (tboy1337)
Pull request description:
The previous fix for invalid MuSig2 pubkeys (bitcoin/bitcoin#34010) only
addressed the PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS field. However, the
PSBT_IN_MUSIG2_PUB_NONCE and PSBT_IN_MUSIG2_PARTIAL_SIG fields also
deserialize pubkeys without validation, which could lead to crashes when
invalid pubkeys are processed.
This commit adds validation to the DeserializeMuSig2ParticipantDataIdentifier
function to ensure all pubkeys in MuSig2 pubnonce and partial signature
fields are fully valid elliptic curve points.
The fix:
- Validates both aggregate and participant pubkeys in MuSig2 pubnonce and
partial signature deserialization
- Throws std::ios_base::failure with descriptive error messages for invalid
pubkeys
- Prevents potential crashes from invalid elliptic curve points
- Maintains backward compatibility for valid PSBTs
This completes the fix for issues [#33999](https://github.com/bitcoin/bitcoin/issues/33999) and [#34201](https://github.com/bitcoin/bitcoin/issues/34201).
ACKs for top commit:
rkrux:
lgtm ACK f51665bee7
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/34219/commits/f51665bee72c26d3f3cc6813b6c02adad5f0af6a
darosior:
utACK f51665bee7
Tree-SHA512: 8454d77a05aa003a3121b0a5975e8a000125ee0d62343bfa625a75db113358ba7a210ae0376ca1666957b7de7005e06e5a54c95170430ee5e9e1416719b8af53
1c1de334e9 test: avoid interface_ipc.py race and null pointer dereference (Ryan Ofsky)
Pull request description:
Avoid race condition in `run_deprecated_mining_test` caused by creating and immediately destroying an unused worker thread. This is leading to test failures reported by maflcko in #34711
ACKs for top commit:
optout21:
utACK 1c1de334e9
l0rinc:
Tested ACK 1c1de334e9
w0xlt:
ACK 1c1de334e9
enirox001:
ACK 1c1de33
Tree-SHA512: d0af9676a46e991a3f4fda3795c02d1998d30de24991436b8ada425585c6699ff32a7057ca7a0ef2925f782fd3bf73e0381f5d4325e4f1c09f487fce1de49e45
b87a1c27c9 doc: Improve dependencies.md IPC documentation (Ryan Ofsky)
Pull request description:
Improve dependencies.md to document IPC dependencies better ([preview](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-depdoc/doc/dependencies.md#build-1)). Specific changes are listed in the commit message.
This PR is based on #33623 by willcl-ark which made similar changes in the 29.x branch. This PR could also be backported to 30.x (it merges cleanly, and master and 30.x both have the same version requirements).
ACKs for top commit:
l0rinc:
ACK b87a1c27c9
sedited:
ACK b87a1c27c9
Tree-SHA512: 566b5372d189f0ad04f978ddefbd8c200dcc19b25af02c73275c5faf7529943680ec59703bda11640cf7920466b4cdd46305cac4d3770e2303de37694ae78909
fa7612f253 ci: Download script_assets_test.json for Windows CI (MarcoFalke)
7777a13306 test: Move Fetching-print to download_from_url util (MarcoFalke)
faf96286ce test: move-only download_from_url to stand-alone util file (MarcoFalke)
fa0cc1c5a4 test: [doc] Remove outdated comment (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/34670 by adding a new `download_script_assets` Python helper and calling it.
ACKs for top commit:
hebasto:
re-ACK fa7612f253.
janb84:
re ACK fa7612f253
hodlinator:
utACK fa7612f253
Tree-SHA512: 73c2cb3a31f231174566fb880b82de92734b1679ef000f8d793d774b7e5f5a7b8c7994a3998ca78821115bdc80c16aada69cf596e92c083cf9b9a95c7cee16ea
```bash
15 | #warning The ClangTidyModuleRegistry.h header is deprecated and will be removed in LLVM 24. All of the symbols it used to define have been moved into ClangTidyModule.h.
| ^~~~~~~
[100%] Linking CXX shared module libbitcoin-tidy.so
```
fa36adeb71 ci: [refactor] Drop last use of pwsh (MarcoFalke)
fae31b1e2f ci: [refactor] Move github_import_vs_env to python script (MarcoFalke)
Pull request description:
The use of pwsh was a bit confusing and inconsistent around the exit code. See also https://github.com/bitcoin/bitcoin/pull/32672
I think it is fine to drop it and purely use Bash/Python.
This also moves a bit of code from the github yaml to the python script.
ACKs for top commit:
m3dwards:
Looks good! re-ACK https://github.com/bitcoin/bitcoin/commit/fa36adeb719b5d2a6ce8e1b69d3bbd8e2bd70349
hodlinator:
re-ACK fa36adeb71
Tree-SHA512: 78edffc60c58c476b0acca5224150169d154b0b818114844a04295af9ba19b7cdf1fb2afb738f6cafd6172f0f477d546018ebf95061eb5bd8bbb35e065a129d4
fab51e470e test: Move valgrind.supp to the other sanitizer_suppressions files (MarcoFalke)
fa9cf81d39 test: Add missing resolve() to valgrind.supp file (MarcoFalke)
Pull request description:
(see commit message for details)
Can be tested via:
```
./bld-cmake/test/functional/feature_framework_testshell.py --valgrind
```
Before:
```
bitcoind exited with status 1 during initialization. ==1735813== FATAL: can't open suppressions file "/b-c/b-c-2/bld-cmake/contrib/valgrind.supp"
```
After: (passes)
Also, move the suppression file to all the other suppression files.
ACKs for top commit:
frankomosh:
Re-ACK fab51e470e
Tree-SHA512: d3e3e130c0e2292ca3ab9e80d2ebec6b4edc7914280ed90fb4af8a65cd1c9edd19064d86375a6787b864925fe0e47bab2321f89b9be8d1613a0aebf4ec2443fe
fa18be2f2b test: Fix typo (MarcoFalke)
fac932698f ci: Set TEST_RUNNER_PORT_MIN in test-each after cirrus runner switch (MarcoFalke)
Pull request description:
This is needed after the recent switch to cirrus runners in the task in commit c8c9c1e617.
Otherwise, the CI will fail:
```
node1 stderr Error: Unable to bind to 127.0.0.1:12321 on this computer. Bitcoin Core is probably already running.
```
(https://github.com/bitcoin/bitcoin/actions/runs/22398358349/job/64837827234?pr=31723#step:9:2605)
Also, include a random second commit, so that the CI task is run in this pull.
ACKs for top commit:
l0rinc:
ACK fa18be2f2b
willcl-ark:
ACK fa18be2f2b
Tree-SHA512: 6b63f645bf62d3e951ca155cddf3dc562b7ce675ccae4f9179e2202679685b5c147844eb350bd219b173fe2bb976376d0caa073d3e827a48c13aa015f4745b2c
fa6af85634 refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke)
fa692974ac util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke)
Pull request description:
The call to `tcgetattr` may fail with `ENOTTY`, leaving the struct possibly uninitialized (UB).
Fix this UB by returning early when `isatty` fails, or when `tcgetattr` fails. (Same for Windows)
This can be tested by a command that fails valgrind before the change and passes after:
```
echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime
ACKs for top commit:
achow101:
ACK fa6af85634
l0rinc:
lightly tested code review ACK fa6af85634
sedited:
ACK fa6af85634
Tree-SHA512: 76e2fbcb6c323b17736ee057dbd5e932b2e8cbb7d9fe4488c1dc7ab6ea928a3cde7e72ca0a63f8c8c78871ccb8b669263b712c0e1b530d88f2d45ea41f071201
45133c589a doc: clarify `git range-diff` add/delete output (Lőrinc)
Pull request description:
### Problem
Range diffs in git are useful after PR rebases, but it has an easy-to-misread failure mode: if it cannot match a commit between the old and new ranges, it will show the old commit as removed (<) and the new commit as added (>), without showing any patch contents for that commit.
It can look like there were no code changes when in reality the commit was just treated as unrelated and needs full re-review.
### Example
```bash
git fetch upstream ff338fdb53a66ab40a36e1277e7371941fc89840 dd76338a57b9b1169ac27f7b783d6d0d4c6e38ab
git range-diff ff338fdb53a6...dd76338a57b9
```
This produced output like:
```patch
1: 0ca4295f2e = 93: 139aa4b27e bench: add on-disk `HaveInputs` benchmark
2: 4b32181dbb < -: ---------- test: add `HaveInputs` call-path unit tests
-: ---------- > 94: 277c57f0c5 test: add `HaveInputs` call-path unit tests
3: 8c57687f86 ! 95: c0c94ec986 dbwrapper: have `Read` and `Exists` reuse `ReadRaw`
@@ Metadata
## Commit message ##
dbwrapper: have `Read` and `Exists` reuse `ReadRaw`
- `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl` (except that it copies the resulting string on success, but that will be needed for caching anyway).
+ `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl`.
```
Even though the subject matches, there is no diff shown because the commits did not match - the reviewer could think that only the commit message was changed.
This should be treated as **unmatched** rather than **unchanged**.
If you expected a match, you can try increasing the search effort:
```bash
git range-diff --creation-factor=95 ff338fdb53a6...dd76338a57b9
```
which would show for example:
```patch
1: 0ca4295f2e = 93: 139aa4b27e bench: add on-disk `HaveInputs` benchmark
2: 4b32181dbb ! 94: 277c57f0c5 test: add `HaveInputs` call-path unit tests
@@ Commit message
The tests document that `HaveInputs()` consults the cache first and that a cache miss pulls from the backing view via `GetCoin()`.
+ Co-authored-by: Novo <eunovo9@gmail.com>
+
## src/test/coins_tests.cpp ##
@@ src/test/coins_tests.cpp: BOOST_FIXTURE_TEST_CASE(ccoins_flush_behavior, FlushTest)
}
}
-+BOOST_AUTO_TEST_CASE(ccoins_haveinputs_cache_miss_uses_base_getcoin)
++BOOST_AUTO_TEST_CASE(ccoins_cache_behavior)
```
### Fix
This PR updates `doc/productivity.md` to raise awareness and document this pitfall and mentions `--creation-factor` as a knob to try when the output seems unexpectedly empty.
ACKs for top commit:
maflcko:
review ACK 45133c589a🏦
Sjors:
ACK 45133c589a
rkrux:
crACK 45133c5
sedited:
ACK 45133c589a
Tree-SHA512: 52dcf6db51425a3ac9789627f80233fb1e3437f7a351acf4a761504d9917837aa1ff8c964605a842ee099fae9842946784f7603f9bffa7051429b2f04b7900be
0a8d303d66 test: fix test_limit_enforcement_package (Greg Sanders)
Pull request description:
The current test has a couple issues:
1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission
2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present.
Fix the bug and add a few assertions to protect against regressions.
ACKs for top commit:
bensig:
ACK 0a8d303d66
achow101:
ACK 0a8d303d66
sipa:
ACK 0a8d303d66
Tree-SHA512: 0ba184d82edc5a502e9119a6876e80c4564c876fa51ee39293d47bd30c18bf3ded50fbd2f6f2a3394784fad05d8f6370a90682068b30358b077280abd2477252
e5f0613503 net processing: Check if we are in ibd before processing block for txdownloadman (sedited)
ce8b692897 Add functional test exercising tx downloadman recently confirmed filter (Lőrinc)
Pull request description:
Calculating the rolling bloom filters for the txorphanage takes some CPU time from the scheduler thread. This can be observed for example in [this flamegraph](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-172/20066462508/mainnet-default-instrumented-base-flamegraph.svg?x=920203898521&y=780), where handling the filter takes about 2.6% of total time (and most of the scheduler thread's time).
During ibd the entries in the tx download bloom filter are just continuously rolled over and aren't consumed, since no mempool entries are created by incoming transactions from peers during ibd. The mempool does accept transactions via RPC, or the wallet at the time, however these don't interact with the orphanage and the txdownloadman, because adding anything to those is guarded by IsInitialBlockDownload() checks as well.
We're usually latching ibd to false a few blocks before catching up to the tip, so this should also not significantly degrade the performance of the filter once fully caught up.
ACKs for top commit:
l0rinc:
ACK e5f0613503
instagibbs:
ACK e5f0613503
fjahr:
Code review ACK e5f0613503
Tree-SHA512: d667e677f5723c438cdf5b34f0f9c1ade7cc1b2e98530c23f14384514daa38217c4e7c3b756194b6831b590a487449c4514b52bf0fb461ae8083061722824270
744d47fcee clusterlin: adopt trained cost model (feature) (Pieter Wuille)
4eefdfc5b7 clusterlin: rescale costs (preparation) (Pieter Wuille)
ecc9a84f85 clusterlin: use 'cost' terminology instead of 'iters' (refactor) (Pieter Wuille)
9e7129df29 clusterlin: introduce CostModel class (preparation) (Pieter Wuille)
Pull request description:
Part of #30289, replaces earlier #34138.
This introduces a more accurate cost model for SFL, to control how much CPU time is spent inside the algorithm for clusters that cannot be linearized perfectly within a reasonable amount of time.
The goal is having a metric for the amount of work performed, so that txmempool can impose limits on that work: a lower bound that is always performed (unless optimality is reached before that point, of course), and an upper bound to limit the latency and total CPU time spent on this. There are conflicting design goals here:
* On the one hand, it seems ideal if this metric is closely correlated to actual CPU time, because otherwise the limits become inaccurate.
* On the other hand, it seems a nightmare to have the metric be platform/system dependent, as it makes network-wide reasoning nearly impossible. It's expected that slower systems take longer to do the same thing; this holds for everything, and we don't need to compensate for this.
There are multiple solutions to this:
* One extreme is just measuring the time. This is very accurate, but extremely platform dependent, and also non-deterministic due to random scheduling/cache effects.
* The other extreme is using a very abstract metric like counting how many times certain loops/function inside the algorithm run. That is what is implemented in master right now, just counting the sum of the numbers of transactions updated across all `UpdateChunks()` calls. It however necessarily fails to account for significant portions of runtime spent elsewhere, resulting in a rather wide range of "ns per cost" values.
* This PR takes a middle ground, counting many function calls / branches / loops, with weights that were determined through benchmarking on an average on a number of systems.
Specifically, the cost model was obtained by:
* For a variety of machines:
* Running a fixed collection of ~385000 clusters found through random generation and fuzzing, optimizing for difficulty of linearization.
* Linearize each 1000-5000 times, with different random seeds. Sometimes without input linearization, sometimes with a bad one.
* Gather cycle counts for each of the operations included in this cost model, broken down by their parameters.
* Correct the data by subtracting the runtime of obtaining the cycle count.
* Drop the 5% top and bottom samples from each cycle count dataset, and compute the average of the remaining samples.
* For each operation, fit a least-squares linear function approximation through the samples.
* Rescale all machine expressions to make their total time match, as we only care about relative cost of each operation.
* Take the per-operation average of operation expressions across all machines, to construct expressions for an average machine.
* Approximate the result with integer coefficients.
The benchmarks were performed by `l0rinc <pap.lorinc@gmail.com>` and myself, on AMD Ryzen 5950X, AMD Ryzen 7995WX, AMD Ryzen 9980X, Apple M4 Max, Intel Core i5-12500H, Intel Core Ultra 7 155H, Intel N150 (Umbrel), Intel Core i7-7700, Intel Core i9-9900K, Intel Haswell (VPS, virtualized), Intel Xeon E5-2637, ARM Cortex-A76 (Raspberry Pi 5), ARM Cortex-A72 (Raspberry Pi 4).
Based on final benchmarking, the "acceptable" iteration count (which is the minimum spent on every cluster) is to 75000 units, which corresponds to roughly 50 μs on Ryzen 5950X and similar modern desktop hardware.
ACKs for top commit:
instagibbs:
ACK 744d47fcee
murchandamus:
reACK 744d47fcee
Tree-SHA512: 5cb37a6bdd930389937c435f910410c3581e53ce609b9b594a8dc89601e6fca6e6e26216e961acfe9540581f889c14bf289b6a08438a2d7adafd696fc81ff517
62e378584e guix: don't export TZ twice (fanquake)
badcf1c68d guix: fix typo in guix-codesign (fanquake)
Pull request description:
Remove a double export of `TZ` and fix a typo.
ACKs for top commit:
janb84:
ACK 62e378584e
hebasto:
ACK 62e378584e, I have reviewed the code and it looks OK.
Tree-SHA512: 6e0b66d20db2b0535bea7d29a28782d0ddd423eaace5f2aa708a227985d465e33def10e9f8ac5ea5482fe640ea0df4ca9df8e9f417bf5d040867564fc60aebd0
fa4cb96bde test: Set assert_debug_log timeout to 0 (MarcoFalke)
Pull request description:
The `assert_debug_log` helper is meant to be a context manager. However, it has a default timeout of 2 seconds, and can act as a silent polling/wait/sync function. This is confusing and brittle, and leads to intermittent bugs such as https://github.com/bitcoin/bitcoin/pull/34571.
Fix all issues, by setting the default timeout to zero. Then adjust all call sites to either use this correctly like a context manager, or adjust them explicitly to specify a timeout, to document that this is a polling sync function.
ACKs for top commit:
hodlinator:
re-ACK fa4cb96bde
brunoerg:
ACK fa4cb96bde
Tree-SHA512: 111a45c84327c3afea98fd9f8de13dd7d11969b2309471b4ad21694b290a3734f6e1eda75a41b39613b0995c5b7075298085cea2ca06aa07d4385eb8d72fe95b
fb3e1bf9c9 test: check LoadBlockIndex correctly recomputes invalidity flags (stratospher)
29740c06ac validation: remove BLOCK_FAILED_MASK (stratospher)
b5b2956bda validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk (stratospher)
37bc207852 validation: stop using BLOCK_FAILED_CHILD (stratospher)
120c631e16 refactor: use clearer variables in InvalidateBlock() (stratospher)
18f11695c7 validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock (stratospher)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32173
even though we have a distinction between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase,
we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them.
Since there is no functional difference between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` and it's added
code complexity to correctly categorise them (ex: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914366243, https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-565506585), we could just remove it.
Looking for conceptual feedback on whether it's better to improve handling of `BLOCK_FAILED_CHILD` in the codebase or remove `BLOCK_FAILED_CHILD`.
Of less relevance, but it would also fix a `reconsiderblock` crash that could happen in the situation mentioned in https://github.com/bitcoin/bitcoin/issues/32173#issuecomment-2767030982
Similar attempt in the past in https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-568073859
ACKs for top commit:
stickies-v:
re-ACK fb3e1bf9c9
alexanderwiederin:
ACK fb3e1bf9c9
mzumsande:
re-ACK fb3e1bf9c9
Tree-SHA512: e97b739885c40a8c021966438e9767cc02bc183056236d6a8c64f6819347ae70c0fbcd71cc2528917560d9f4fd56aed45faf1b6c75d98de7b08b621693a97fbc
fa48d42163 test: Stricter unit test (MarcoFalke)
fa626bd143 util: Remove brittle and confusing sp::Popen(std::string) (MarcoFalke)
Pull request description:
The subprocess Popen call that accepts a full `std::string` has many issues:
* It promotes brittle and broken code, where spaces are not properly quoted. Example: https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065
* The internally used `util::split` function does incorrectly split on spaces, instead of using `shlex.split`.
* It is redundant and not needed, because a vector interface already exists.
Fix all issues by removing it and just using the vector interface.
This pull request should not change any behavior: Note that the command taken from `gArgs.GetArg("-signer", "")` is still passed through the `sp::util::split` helper, just like before. Fixing that is left for a follow-up, so that this change here is basically just a refactor.
This also fixes a unit test bug as a side-effect: Fixes https://github.com/bitcoin/bitcoin/issues/32574.
ACKs for top commit:
janb84:
cr ACK fa48d42163
fjahr:
Code review ACK fa48d42163
hebasto:
re-ACK fa48d42163.
Tree-SHA512: 3d29226977c9392502f9361e2bd42b471ad03761bbf6a94ef6e545cbe4492ad5858da1ac9cc64b2791aacb9b6e6f3c3f63dbcc3a2bf45f6a13b5bc33eddf8c2b
c2fcf25069 clusterlin: inline GetReachable into Deactivate (optimization) (Pieter Wuille)
d90f98ab4a clusterlin: inline UpdateChunk into (De)Activate (optimization) (Pieter Wuille)
b684f954bb clusterlin: unidirectional MakeTopological initially (optimization) (Pieter Wuille)
1daa600c1c clusterlin: track suboptimal chunks (optimization) (Pieter Wuille)
63b06d5523 clusterlin: keep track of active children (optimization) (Pieter Wuille)
ae16485aa9 clusterlin: special-case self-merges (optimization) (Pieter Wuille)
3221f1a074 clusterlin: make MergeSequence take SetIdx (simplification) (Pieter Wuille)
7194de3f7c clusterlin: precompute reachable sets (optimization) (Pieter Wuille)
6f898dbb8b clusterlin: simplify PickMergeCandidate (optimization) (Pieter Wuille)
dcf458ffb9 clusterlin: split up OptimizeStep (refactor) (Pieter Wuille)
cbd684a471 clusterlin: abstract out functions from MergeStep (refactor) (Pieter Wuille)
b75574a653 clusterlin: improve TxData::dep_top_idx type (optimization) (Pieter Wuille)
73cbd15d45 clusterlin: get rid of DepData (optimization) (Pieter Wuille)
7c6f63a8a9 clusterlin: pool SetInfos (preparation) (Pieter Wuille)
20e2f3e96d scripted-diff: rename _rep -> _idx in SFL (Pieter Wuille)
268fcb6a53 clusterlin: add more Assumes and sanity checks (tests) (Pieter Wuille)
d69c9f56ea clusterlin: count chunk deps without loop (optimization) (Pieter Wuille)
f66fa69ce0 clusterlin: split tx/chunk dep counting (preparation) (Pieter Wuille)
900e459778 clusterlin: avoid depgraph argument in SanityCheck (cleanup) (Pieter Wuille)
666b37970f clusterlin: fix type to count dependencies (Pieter Wuille)
Pull request description:
Follow-up to #32545, part of #30289.
This contains many of the optimizations that were originally part of #32545 itself.
Here is a list of commits and the geometric average of the benchmark timings. Note that these aren't worst cases, but because of the nature of the optimizations here, I do expect them to apply roughly equally to all kinds of clusters. In other words, the relative improvement shown by these numbers should be representative:
| commit title | ns per optimal linearization |
|:--|--:|
| clusterlin: split tx/chunk dep counting (preparation) | 24,760.30 |
| clusterlin: count chunk deps without loop (optimization) | 24,677.64 |
| scripted-diff: rename _rep -> _idx in SFL | 24,640.08 |
| clusterlin: get rid of DepData, reuse sets (optimization) | 24,389.01 |
| clusterlin: improve TxData::dep_top_idx type (optimization) | 22,578.58 |
| clusterlin: abstract out functions from MergeStep (refactor) | 22,577.15 |
| clusterlin: split up OptimizeStep (refactor) | 22,981.11 |
| clusterlin: simplify PickMergeCandidate (optimization) | 22,018.63 |
| clusterlin: precompute reachable sets (optimization) | 21,194.91 |
| clusterlin: make MergeSequence take SetIdx (simplification) | 21,135.60 |
| clusterlin: special-case self-merges (optimization) | 20,588.13 |
| clusterlin: keep track of active children (optimization) | 13,911.22 |
| clusterlin: track suboptimal chunks (optimization) | 13,629.42 |
| clusterlin: unidirectional MakeTopological initially (optimization) | 12,796.57 |
| clusterlin: inline UpdateChunk into (De)Activate (optimization) | 12,706.35 |
| clusterlin: inline GetReachable into Deactivate (optimization) | 12,525.66 |
And to show that they apply to all clusters roughly similarly:
<img width="1239" height="640" alt="output(1)" src="https://github.com/user-attachments/assets/edd73937-3f87-4582-b2b9-eaed7e6ff352" />
ACKs for top commit:
instagibbs:
reACK c2fcf25069
ajtowns:
reACK c2fcf25069
Tree-SHA512: e8920f7952a6681b4c1d70c864bd0e9784127ae4fd7c740be3e24a473f72e83544d2293066ed9b3e685b755febd6bbbc6c7da0c9b6ef3699b05eaa8d5bc073a0
eb510f8678 ci: fail fast in test-each-commit script (Lőrinc)
04c4d71008 ci: remove commit count limit from `test-each-commit` (Lőrinc)
Pull request description:
### Problem
`test-each-commit` currently tests only a limited number of ancestor commits in a PR, so failures introduced deeper in the commit stack might be missed.
### Fix
Remove the max-count limit so `test-each-commit` runs the full build + unit + functional test flow on every non-head PR commit, while keeping the PR tip excluded because it is already covered by the normal CI jobs.
It will also stop after the first failure to surface the root cause sooner and keep logs readable when testing ancestor commits.
### Examples
* Example failure 10 commits deep: https://github.com/l0rinc/bitcoin/actions/runs/21390976651/job/61577575033?pr=105 in https://github.com/l0rinc/bitcoin/pull/105
* Example pass with >7 dummy commits: https://github.com/l0rinc/bitcoin/actions/runs/21392557521/job/61595159841?pr=106 in https://github.com/l0rinc/bitcoin/pull/106
---------
Note: this PR has gone through a few iterations, the latest one just extends the existing job.
ACKs for top commit:
maflcko:
lgtm ACK eb510f8678🕓
hebasto:
re-ACK eb510f8678.
willcl-ark:
ACK eb510f8678
Tree-SHA512: 5aadafd32daad610ce882277802c390642dc34f7d5bfa71d4b503ee007942d1ebafce2a3430ea5fd2af6673c83f9aee42450043be4722d7c02407d90920f8bce
211111b804 test: Avoid empty errmsg in JSONRPCException (MarcoFalke)
Pull request description:
It is unclear why the fallback should be an empty message, when it is better to include all rpc_error details that are available.
Also, include the http status.
This allows to revert commit 6354b4fd7f, because it is no longer needed.
Can be tested by running this diff:
```diff
diff --git a/test/functional/wallet_disable.py b/test/functional/wallet_disable.py
index dbcccd4778..9717a2d248 100755
--- a/test/functional/wallet_disable.py
+++ b/test/functional/wallet_disable.py
@@ -18,9 +18,8 @@ class DisableWalletTest (BitcoinTestFramework):
self.extra_args = [["-disablewallet"]]
self.wallet_names = []
- def run_test (self):
- # Make sure wallet is really disabled
- assert_raises_rpc_error(-32601, 'Method not found', self.nodes[0].getwalletinfo)
+ def run_test(self):
+ self.nodes[0].getwalletinfo()
x = self.nodes[0].validateaddress('3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
assert x['isvalid'] == False
x = self.nodes[0].validateaddress('mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ')
ACKs for top commit:
ismaelsadeeq:
utACK 211111b804
furszy:
utACK 211111b804
hodlinator:
ACK 211111b804
rkrux:
tACK 211111b804
Tree-SHA512: 92067aaaa61a887398e38f587004ba31070acc6a9dbd003e9e35393e8c049e786177afa43da05d2d653af340b6c69803c4323061d73d417219bcdf37729e4b66
c413cf12c5 ci: Split vcpkg tools cache into restore/save (willcl-ark)
Pull request description:
The vcpkg tools cache was using the combined actions/cache action, which by default saves on every run regardless of branch. Split it into the restore/save pattern used by the other caches, so that saves only happen on default branch pushes.
This will have little impact in bitcoin/bitcoin (which uses few branches), but on forks, if you don't update master branch frequently (which saves all caches), then all cache space will eventually be taken up by multiple vckpg tools caches, resulting in bad cache hit rates in all other jobs.
ACKs for top commit:
maflcko:
lgtm ACK c413cf12c5
Tree-SHA512: e28a43b1aa17ce7f0a19d16b98efed0372004d83e4d7e92a126f642599d7e1a94684032a48a3b380b3a7c970c313c92bfe30146b977e33f81b45fe70b49755e3
2cb7e99dee test: also reset CConnman::m_private_broadcast in tests (Vasil Dimov)
91b7c874e2 test: add ConnmanTestMsg convenience method Reset() (Vasil Dimov)
Pull request description:
Member variables of `CConnman::m_private_broadcast` (introduced in
https://github.com/bitcoin/bitcoin/pull/29415) could influence the tests
which creates non-determinism if the same instance of `CConnman` is used
for repeated test iterations.
So, reset the state of `CConnman::m_private_broadcast` from
`ConnmanTestMsg::Reset()`. Currently this affects the fuzz tests
`process_message` and `process_messages`.
Reported in https://github.com/bitcoin/bitcoin/issues/34476#issuecomment-3849088794
ACKs for top commit:
maflcko:
review ACK 2cb7e99dee🚙
Crypt-iQ:
tACK 2cb7e99dee
frankomosh:
Code Review ACK 2cb7e99dee
brunoerg:
code review ACK 2cb7e99dee
Tree-SHA512: 0f4b114542da8dc611689457ce67034c15cbfe409b006b2db72bc74078ee9513f5ce3d0e6e67d37c127cfa0a5170fe72fe3ea45ce2a61d45a358dd11bd1881f8
eafd530d20 kernel: avoid potential duplicate object in shared library/binary (Cory Fields)
24c3b47010 build: add kernel-specific warnings (Cory Fields)
Pull request description:
This is a revival of https://github.com/bitcoin/bitcoin/pull/31807
Introduces the [-Wunique-object-duplication](https://clang.llvm.org/docs/DiagnosticsReference.html#wunique-object-duplication) warning flag available in clang-21 for usage when building the kernel library. It warns of potential duplicate objects in shared libraries. REDUCE_EXPORTS needs to be ON to trigger it.
Though we have a C API now that manages exporting symbols, I think it is prudent to also avoid any duplicate symbols on the internal c++ side in case we ever to decide to expose some of its headers. It also not clear that all linkers would handle these cases correctly even in the current internal usage.
ACKs for top commit:
fanquake:
ACK eafd530d20
hebasto:
ACK eafd530d20.
Tree-SHA512: 81961b50f0268dbe076497e130857f5b4b9151c748d107ec15158d1511dd25bce745e0beeb127b9cea51cb2edd78032735600606a75f7ff8a3fd572acced42e0
faba426b3b lint: Flatten lint image entry points (MarcoFalke)
1111fff91c lint: Add missing --platform=linux to docker build command (MarcoFalke)
Pull request description:
Two fixups to the lint container:
* Add a missing `--platform=linux` to avoid running a non-native arch, like s390x, which happens with podman if such a container was most recently used.
* Flatten the entry points to remove the bash-based one:
Previously, an additional entry point into the container that spawned a bash was supported. The bash had an alias `lint` to run all lint scripts. However, such a use-case seems limited (because it only runs inside the container), inflexible (because it only allows running all lint scripts), and possibly brittle (because it can miss re-building the image when the cache is stale). So remove it and just offer the single entry point via the `./ci/lint.py` script.
If there is a use-case to skip the image building, it should be trivial to add an env var setting the the lint Python script like `DANGER_SKIP_IMAGE_RE_BUILD=1` (or so) in the future.
ACKs for top commit:
willcl-ark:
ACK faba426b3b
Tree-SHA512: 9afda16723c215602c6c42fa3a286d1828c887c8f6ff9512c8ec162ec8997789695f0c464d389cae94e67acf8b5e0f1a55e2ee0d60131a2eee091cf281f91514
65134c7e5f depends: Prefix include path for headers-only `systemtap` package (Hennadii Stepanov)
94a692b6aa cmake: Add missed `USDT::headers` (Hennadii Stepanov)
b5375c44ed depends: Prefix include path for headers-only `boost` package (Hennadii Stepanov)
d73378ffcc cmake: Add missed `Boost::headers` (Hennadii Stepanov)
Pull request description:
Currently, header-only dependencies in the depends subsystem are installed into the standard `include/` subdirectory. This inadvertently exposes their headers to the compiler via `-I` flags brought in by other dependencies (e.g., `libevent` or `sqlite`). This "include path pollution" masks missing dependencies in the build configuration. While the build might succeed by accident due to this overlap, it creates a fragile state. If the overlapping library is removed, the build will break, or, worse, the compiler may silently fall back to the host system's default paths (e.g., `/usr/include`).
This PR improves build system security and hygiene by enforcing strict, distinguished include paths for header-only dependencies. The missing dependencies revealed by this change (`Boost::headers`, `USDT::headers`) have been fixed in separate commits.
ACKs for top commit:
theuni:
re-ACK 65134c7e5f
fanquake:
ACK 65134c7e5f
Tree-SHA512: 41667b46c3bd2f872951a5651b30f7d1468f49f1265196b7868233ed44b2eb0e33f1f69a1af348b55f07a8d1f594e276eb49b724e80b8eae85aed1c9bacae197
6f113cb184 txgraph: use fallback order to sort chunks (feature) (Pieter Wuille)
0a3351947e txgraph: use fallback order when linearizing (feature) (Pieter Wuille)
fba004a3df txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille)
941c432a46 txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille)
39d0052cbf clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille)
8bfbba3207 txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille)
e0bc73ba92 clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille)
6c1bcb2c7c txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille)
7427c7d098 txgraph: update chunk index on Compact (preparation) (Pieter Wuille)
3ddafceb9a txgraph: initialize Ref in AddTransaction (preparation) (Pieter Wuille)
Pull request description:
Part of #30289.
TxGraph's fundamental responsibility is deciding the order of transactions in the mempool. It relies on the `cluster_linearize.h` code to optimize it, but there can and often will be many different orderings that are essentially equivalent from a quality perspective, so we have to pick one. At a high level, the solution will involve one or more of:
* Deciding based on **internal identifiers** (`Cluster::m_sequence`, `DepGraphIndex`). This is very simple, but risks leaking information about transaction receive order.
* Deciding **randomly**, which is private, but may interfere with relay expectations, block propagation, and ability to monitor network behavior.
* Deciding **based on txid**, which is private and deterministic, but risks incentivizing grinding to get an edge (though we haven't really seen such behavior).
* Deciding **based on size** (e.g. prefer smaller transactions), which is somewhat related to quality, but not unconditionally (depending on mempool layout, the ideal ordering might call for smaller transactions first, last, or anywhere in between). It's also not a strong ordering as there can be many identically-sized transactions. However, if it were to encourage grinding behavior, incentivizing smaller transactions is probably not a bad thing.
As of #32545, the current behavior is primarily picking randomly, though inconsistently, as some code paths also use internal identifiers and size. #33335 sought to change it to use random (preferring size in a few places), with the downsides listed above.
This PR is an alternative to that, which changes the order to tie-break based on size everywhere possible, and use lowest-txid-first as final fallback. This is fully deterministic: for any given set of mempool transactions, if all linearized optimally, the transaction order exposed by TxGraph is deterministic.
The transactions within a chunk are sorted according to:
1. `PostLinearize` (which improves sub-chunk order), using an initial linearization created using the rules 2-5 below.
2. Topology (parents before children).
3. Individual transaction feerate (high to low)
4. Individual transaction weight (small to large)
5. Txid (low to high txid)
The chunks within a cluster are sorted according to:
1. Topology (chunks after their dependencies)
2. Chunk feerate (high to low)
3. Chunk weight (small to large)
4. Max-txid (chunk with lowest maximum-txid first)
The chunks across clusters are sorted according to:
1. Feerate (high to low)
2. Equal-feerate-chunk-prefix weight (small to large)
3. Max-txid (chunk with lowest maximum-txid first)
The equal-feerate-chunk-prefix weight of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is a well-defined approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order.
ACKs for top commit:
ajtowns:
reACK 6f113cb184 it was good before and now it's better
instagibbs:
ACK 6f113cb184
marcofleon:
light crACK 6f113cb184
Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
2ccfdb582b build: avoid exporting secp256k1 symbols (Cory Fields)
Pull request description:
Take advantage of the [new secp256k1 option to avoid visibility attributes](https://github.com/bitcoin-core/secp256k1/pull/1696) on API functions.
While most users of a shared libsecp always want API functions exported so that they can actually be linked against, we always build it statically. When that static lib is linked into a (static or shared) libbitcoinkernel, by default its symbols end up exported there as well.
As libsecp is an implementation detail of the kernel (and any future Core lib), its symbols should never be exported.
[This was the intended use for the above PR](https://github.com/bitcoin-core/secp256k1/pull/1696#issuecomment-3028838988), looks like we just forgot to follow-up and actually hook it up.
This is most easily tested by building with `-DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON` (with or without `-DREDUCE_EXPORTS=ON`) and inspecting via:
```bash
nm -CD lib/libbitcoinkernel.so | grep secp
```
Before this change, secp's symbols will show up there. After, they should be absent.
This should finally solve secp symbol visibility once and for all :)
ACKs for top commit:
hebasto:
ACK 2ccfdb582b, this is implemented exactly as I [tested](https://github.com/bitcoin-core/secp256k1/pull/1696#pullrequestreview-3033584362) the upstream PR. Tested on Fedora 43.
stickies-v:
tACK 2ccfdb582b
Tree-SHA512: 664ea7a6f811c2743ad1b4d8913c61aab9b358931ee77895d35cdf8a5607fbb08facda085877c53d731afbf42a7220dcc752fc365a7625ee679c1547e1c674d0
fa8c89511d Fixup TODO comment in feature_dbcrash.py; remove unnecessary sleep (MarcoFalke)
Pull request description:
Fixup some stale comments:
* The `60 seconds` is outdated. It should say 120 seconds. However, just clarify that there is a timeout.
* The TODO seems to imply that a timeout (failure to restart) can happen. However, I don't think we've seen it happen. So there isn't anything to do right now. Just remove the `TODO`, but keep the advice.
Also, remove an unnecessary `time.sleep(1)`. If there is a need for it, a comment should explain why.
ACKs for top commit:
l0rinc:
ACK fa8c89511d
Tree-SHA512: 5ee13b48fc4a5802f3fadb125d71118e01d2cb08ede9d310d6ed13acd8fb7b03185cad73c475c617054c4c4423156ea927a32d0e3a670c3cc13339b552dc8a5c
fe0b1513a7 test: add a test for txgraph staging (Hao Xu)
ef253a9d3d test: add block builder tests for txgraph (Hao Xu)
4a1ac31e97 test: add a chunk test for txgraph (Hao Xu)
Pull request description:
Add tests for cluster chunks, including:
- txgraph_chunk_chain test: test chunk implementation for a simple chain style graph .
- txgraph_staging test: test the staging feature for a basic graph.
ACKs for top commit:
instagibbs:
reACK fe0b1513a7
sipa:
reACK fe0b1513a7
Tree-SHA512: 01010a3b4e4163849df2912d1393be74d26eb199d0544cfbef58a498aca5153463a118f55a2f1cad2995552b74210031e659de8df6b88cbcffdffd2a1b464990
fa88ac3f4f doc: Clarify why performance-move-const-arg.CheckTriviallyCopyableMove=false (MarcoFalke)
Pull request description:
Without this doc, there is a risk that the setting will be turned off, see https://github.com/bitcoin/bitcoin/pull/34514.
The reason to disable it is to catch logic bugs, even on trivially copyable types:
```cpp
#include <utility>
void Eat(int&& food) { food = 0; };
int main() {
int food{2};
Eat(std::move(food));
Eat(std::move(food)); // This should err
}
```
ACKs for top commit:
l0rinc:
ACK fa88ac3f4f
hebasto:
ACK fa88ac3f4f.
sedited:
ACK fa88ac3f4f
Tree-SHA512: d1bda846a10190a2936084a06bd87418c6a3e4ababc298e4beb9bc9e1190bff430cbe973475d634eda5ef7863571c89bfa4b78ff63fcbd9ac10c42fd9d5fa23a
fa6801366d refactor: [rpc] Remove confusing and brittle integral casts (take 2) (MarcoFalke)
Pull request description:
Second take for cases which I seem to have missed in commit 94ddc2dced.
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:
* `static_cast<bool>(fCoinBase)`, because `bool{fCoinBase}` only works on modern compilers.
This hardening refactor does not fix any bugs and does not change any behavior.
ACKs for top commit:
Sjors:
ACK fa6801366d
sedited:
ACK fa6801366d
Tree-SHA512: 77f03f496ea2a42987720cb4a36eb4e7a0d5b512ed7f029e41dd54c04fb4c1680f7cc13514acbc9f1bae991be4de3cf31c5a0d27c016a030f5749d132603f71e
76dae5d691 cmake: Replace recursive globbing with explicit globbing in folders (Hennadii Stepanov)
88d9092571 cmake: Create subdirectories in build tree in advance (Hennadii Stepanov)
Pull request description:
While reviewing https://github.com/bitcoin/bitcoin/pull/32697, I noticed that symlink creation fails when the target subdirectory does not exist. In such cases, `file(CREATE_LINK ... COPY_ON_ERROR SYMBOLIC)` falls back to copying, which implicitly creates the required path. As a result, a single file is copied instead of symlinked for subdirectories in `test/functional`.
This PR ensures that necessary subdirectories are created in advance, so that subsequent symlink creation does not fail due to missing paths.
For example:
- on the master branch:
```
$ cmake -B build
$ ls -l build/test/functional/mocks/
total 8
-rwxrwxr-x 1 hebasto hebasto 2683 Jul 3 14:11 invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto 64 Jul 3 14:11 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto 60 Jul 3 14:11 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto 57 Jul 3 14:11 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
```
- with this PR:
```
$ cmake -B build
$ ls -l build/test/functional/mocks/
total 4
lrwxrwxrwx 1 hebasto hebasto 65 Jul 3 13:51 invalid_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto 64 Jul 3 13:51 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto 60 Jul 3 13:51 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto 57 Jul 3 13:51 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
```
ACKs for top commit:
m3dwards:
ACK 76dae5d691
sedited:
ACK 76dae5d691
janb84:
re ACK 76dae5d691
Tree-SHA512: a720a68752c72390b9452b192b06d09e41cac1080d32cfe3c2caabb65949626771e0709e7193f69677bd24a3c747e368c2323d9c857d4aa97e1890cc463850ed
b65a3d8009 iwyu: Fix patch to prefer `<cstdint>` (Hennadii Stepanov)
Pull request description:
The goal of the [patch](https://github.com/bitcoin/bitcoin/blob/master/ci/test/01_iwyu.patch) is to suggest C++ headers rather than their C counterparts. However, for fixed width integer types, the patched IWYU currently suggests `<cinttypes>` where `<cstdint>` is sufficient.
This PR fixes this behavior.
ACKs for top commit:
maflcko:
lgtm ACK b65a3d8009
furszy:
utACK b65a3d8009
willcl-ark:
utACK b65a3d8009
Tree-SHA512: 695efdd44b92a642401738572e49c8b6591aa4463d387107fdf3d2f7c9c4b39f4097cb82413752caf9e8890dcca7246a894e562a1dd17023b05a7e455705beac
faa4ab113c ci: Drop valgrind fuzz from GHA matrix (MarcoFalke)
Pull request description:
The valgrind fuzz task is problematic, because:
* It is redundant with the msan fuzz task, which has std lib hardening enabled, so often UB is diagnosed before it even happens in the valgrind task.
* All issues so far found by the valgrind fuzz task were also found by the hardened msan fuzz task.
* All other issues were false-positives, which are hard to debug, and confusing and tedious to work around.
I don't think there is any value in asking pull request authors to debug valgrind false-positives that they triggered by accident. So remove the task for now.
I know that there are some devs, who like to keep the task, but if the task is kept, it should come with clear instructions on how to deal with false-postives in pull requests.
I am not proposing to remove the config itself, and I am happy to continue maintaining it, like it was done before. However, as of now, running it in the GHA matrix is of negative or questionable benefit.
ACKs for top commit:
l0rinc:
ACK faa4ab113c
fanquake:
ACK faa4ab113c - hopefully we can revisit re-adding soon. To be clear, I don't agree with the rationale from #34304, or the initial changes there. The case here, and the fact that it is causing disruption in this repo, is more pressing.
Tree-SHA512: 59272f4b5b01c3b8ee6078ea635441f17776d4d8923f1adacdabdbb00bd2eb0234b30dc5b27938e29f8e79b3c3bebed5f339ae36c2c8fb17ea9d3a2884bee986
-Werror is added to the previous releases job, given it runs on Ubuntu
22.04, which uses an older CMake.
`--compile-no-warning-as-error` can be used, if needed, in future, to
suppress the `CMAKE_COMPILE_WARNING_AS_ERROR` behaviour from a CI
config.
CMAKE_COMPILE_WARNING_AS_ERROR was added to CMake in 3.24.
See https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html.
Co-authored-by: willcl-ark <will8clark@gmail.com>
1f8f7d477a Change BlockRequestAllowed() to take ref (optout)
Pull request description:
As [suggested here](https://github.com/bitcoin/bitcoin/pull/34416#discussion_r2745302958), a minor refactor of `PeerManagerImpl::BlockRequestAllowed()` to take reference parameter (instead of pointer). The motivation is to make the code safer, by minimizing the risk of null-dereference, and to be more consistent.
The change is local to the `PeerManagerImpl::BlockRequestAllowed()` class.
Related to #34440.
ACKs for top commit:
maflcko:
review ACK 1f8f7d477a🎐
l0rinc:
tested ACK 1f8f7d477a
stickies-v:
ACK 1f8f7d477a
Tree-SHA512: 9c2de2d3e7d067e018db7ec54c79f512ccc1da54574d4fb362f6697ee6e235783779d7094cf20856cd34e08a1dbc74609d8351fe7b2287cd8ec0c836ef07be19
42ee31e80c doc: fix broken bpftrace installation link (jayvaliya)
Pull request description:
The bpftrace project has moved from the `iovisor` organization to its own
`bpftrace` organization on GitHub. The old installation documentation link
(`https://github.com/iovisor/bpftrace/blob/master/INSTALL.md`) is now broken (404).
The project also restructured their documentation - installation instructions
are now in the README.md under the "Quick Start" section rather than a
separate INSTALL.md file.
ACKs for top commit:
maflcko:
lgtm ACK 42ee31e80c
Tree-SHA512: 5320e747b77fe9bb347a98396364f3942214b6d3ee69707f62452df0aaaa2f6cfd58fc3b6155f64c4acb00f1a4dba8a751618805e75b823454c73ac93877ad4f
fad9dd1a88 test: kernel test fixups (MarcoFalke)
fabb58d42d test: Use clang-tidy named args for create_chainman (MarcoFalke)
fa51594c5c refactor: Small style fixups in src/kernel/bitcoinkernel.cpp (MarcoFalke)
Pull request description:
Just some small style and test fixups after https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3420542946
ACKs for top commit:
stickies-v:
re-ACK fad9dd1a88
frankomosh:
Code Review ACK fad9dd1a88. All changes are sound refactoring with no functional issues. Nice improvements to readability (named args in create_chainman, span.data(), range checks now properly require non-empty).
Tree-SHA512: 0a92e871b4db75a590acad39672594625e402895bc0d36635d36ec2fe8dce7cc2c5cb6ebf2a92bc14617d94648b84bffb95ff783cea71bd91ac4a9871ef5dbef
fa33acec89 Revert "valgrind: add suppression for bug 472219" (MarcoFalke)
Pull request description:
Now that several Alpine releases are out, it seems fine to remove this suppression.
This should work since Alpine 3.19: https://pkgs.alpinelinux.org/packages?name=valgrind&branch=v3.19&repo=&arch=aarch64, which just dropped out of `main` support (https://alpinelinux.org/releases/)
ACKs for top commit:
fanquake:
ACK fa33acec89
Tree-SHA512: 2a58475d20f8214c012b77efb7fe145e810a1506736341f8debea74dabd31499899a968bc2d4a88008b2a3878b4fe5e48fa14c83a2e943a91943bf0e1e159b6f
4dfb6eef70 test: Add DERSIG tests to script_tests (billymcbip)
884978f389 test: Fix a STRICTENC test in script_tests (billymcbip)
527e8ca7b5 test: Remove outdated comment in script_tests (billymcbip)
Pull request description:
1. Remove a comment referencing a file that no longer exists in the codebase: `script_invalid.json`.
2. Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in `CHECKMULTISIG`. Use `OP_1` for the second signature instead of `OP_0`.
3. Copy existing `STRICTENC` tests and change the flag to `DERSIG`. `DERSIG` is a consensus flag (unlike `STRICTENC`), so it'd be good to have dedicated test cases.
`script_tests` pass on my end.
ACKs for top commit:
darosior:
reACK 4dfb6eef70
sipa:
ACK 4dfb6eef70
Tree-SHA512: aea4aa5199530804561e9f597f69d6cffd7a40d4830919f9371fe97e4d04d8f10e8ed29b91e65e007a3f6e3a38e2881f88dff25831e741ad7592a12ed02b801a
bbbb78a4f2 ci: Print verbose build error message in test-each-commit (MarcoFalke)
2222dadabb ci: [refactor] Allow overwriting check option in run helper (MarcoFalke)
Pull request description:
Currently, the build error in the test-each-commit task is not too nice. E.g. https://github.com/bitcoin/bitcoin/actions/runs/21509735101/job/61973587699#step:8:10464:
```
...
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txvalidation_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txvalidationcache_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_block_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_chainstate_tests.cpp.o
[ 75%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_chainstatemanager_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_flush_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_test_fixture.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/db_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/coinselector_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/coinselection_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/feebumper_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/group_outputs_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/ismine_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/psbt_wallet_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/scriptpubkeyman_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/spend_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_rpc_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_transaction_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/walletdb_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/walletload_tests.cpp.o
[ 76%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/ipc/test/ipc_tests.cpp.o
[ 77%] Linking CXX executable ../../bin/test_bitcoin
[ 87%] Built target test_bitcoin
gmake: *** [Makefile:146: all] Error 2
Command '['cmake', '--build', 'ci_build', '-j', '4']' returned non-zero exit status 2.
error: cannot rebase: Your index contains uncommitted changes.
warning: execution failed: git merge --no-commit origin/master && python3 ./.github/ci-test-each-commit-exec.py && git reset --hard
and made changes to the index and/or the working tree.
You can fix the problem, and then run
git rebase --continue
Error: Process completed with exit code 1.
```
Fix it by just using the same approach that the other CI tasks are using:
https://github.com/bitcoin/bitcoin/blob/01651324f4e540f6b96cff31e89752c3f9417293/ci/test/03_test_script.sh#L143-L146
ACKs for top commit:
willcl-ark:
ACK bbbb78a4f2
Tree-SHA512: ae54ee62f53e060ed42f76ca59daf7a017bd12495e171efed03d1f5dda969db4f7e2e2c9ed7a178ff5fa9a5baa55ab6a7b30b3ab6b81d5279fe937006ac8228f
7528d18796 ci: show more verbose ccache stats (will)
580e9eefe3 ci: bump CCACHE_MAXSIZE to 2G (will)
Pull request description:
Currently some CI jobs don't have great ccache hitrates which we should try to improve: https://willcl-ark.github.io/bitcoin-core-ci-stats/graph/ccache/
- bump ccache maxsize to 2GB in all jobs. We have 150GB shared cache to use, so this should be OK at maximum of 36GB total (current jobset).
- print more verbose ccache stats in the CI logs
The idea is that increasing the cache size to > 2x needed size should eliminate any cache thrashing which might be taking place on master builds when we save the cache. Additionally, larger caches result in more hits in general.
ACKs for top commit:
maflcko:
lgtm ACK 7528d18796
Tree-SHA512: ae00a05159e3f38d24aebc50a2576c5f11241b1196058a4ca2f5f78909795b891bef20cdc4412f512a2ba09cc9ec65afa5132ac0509b54845dd84f933528500c
3c8f5e48f7 ci: Treat SHA1 LLVM signing key as warning (will)
Pull request description:
The current SHA1 LLVM signing key is considered not secure since
2026-02-01T00:00:00Z which makes this run fail when downloading
packages.
See: https://github.com/llvm/llvm-project/issues/153385
Apply the fix from the issue to temporarily to treat this error as a
warning, until the upstream key can be updated.
This PR should be reverted once the upstream key is updated.
ACKs for top commit:
hebasto:
ACK 3c8f5e48f7, tested by running the "iwyu" CI job locally on Ubuntu 25.10 after burning all podman's caches.
Tree-SHA512: fbccf98bfd73cb338670f1ceea994d277d746acbc88b9b90a403d9a59d82abda0f3ba34c4d484b70926340c2d0c873259f930c36ccd4f9d18bb1d22d49ee70c4
4fab35cf88 miniscript: correct and_v() properties (Antoine Poinsot)
Pull request description:
`and_v()` must never be 'd'. This is not a bug fix since this was unreachable in valid Miniscripts: the first sub of an `and_v()` must be of type V, which conflicts with (i.e. never has) property 'd'.
ACKs for top commit:
sipa:
ACK 4fab35cf88. Fuzzed for 2 months worth of CPU time.
achow101:
ACK 4fab35cf88
Tree-SHA512: 8932ad2c9188747299cb9147ff097dca8d078ce7bdd0caefa71ee2724ff81d9bef836664211c2081519a45afd50c539974d67c2a3a1a42a65a3b10b1daef8cbe
d3e681bc06 fuzz: Use `__AFL_SHM_ID` for naming test directories (marcofleon)
Pull request description:
During long multicore fuzzing campaigns with AFL++, stale datadirs can eventually accumulate from time outs, resulting in disk running out of space (see https://github.com/bitcoin/bitcoin/issues/28811). The easiest way to reproduce this is by running our `utxo_total_supply` target using multiple cores with AFL++ and observing the crashes that occur because of all the directories in `/tmp/test_common\ bitcoin/utxo_total_supply/`.
Fix this by using the AFL++ shared memory ID to name the test dirs and cleaning it up before each setup. This ID is unique per AFL++ instance, so multiple cores can run in parallel without conflicts.
Fixes https://github.com/bitcoin/bitcoin/issues/28811
ACKs for top commit:
maflcko:
lgtm ACK d3e681bc06
dergoegge:
utACK d3e681bc06
Tree-SHA512: 420373e5f8a63c84797303ba2ef6657dfe9dacf9c2f3d818524421c24681a0e984c212ecb706217d93f67c2ec16b146a2d37fddcbd6918b2e5e9f634f5e13c10
Newer versions of the macOS SDK, have introduced code like:
```cpp
if defined(__has_feature) && __has_feature(modules)
define USE_CLANG_TYPES 1
else
define USE_CLANG_TYPES 0
endif
if USE_CLANG_TYPES
include <sys/_types/_ptrdiff_t.h>
include <sys/_types/_size_t.h>
include <sys/_types/_va_list.h>
include <sys/_types/_wchar_t.h>
endif
```
which is currently causing compile failures due to undeclared types,
which manifest when C++ modules are enabled. Note that the usage of
"modules" in LLVM is can be a bit ambiguous, see:
https://github.com/llvm/llvm-project/issues/55891.
For now, explcitly disable cxx modules using `-fno-cxx-modules`. This
resolves the include/compilation issues.
Related discussion:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116827https://github.com/llvm/llvm-project/pull/150349https://github.com/llvm/llvm-project/issues/57432
fad042235b refactor: Remove remaining std::bind, check via clang-tidy (MarcoFalke)
Pull request description:
`std::bind` has many issues:
* It is verbose in a meaningless way
* Overriden args are silently accepted and dropped at runtime without a compile error. Same for accidental duplicates.
One could use `std::bind_front` similar to commit fa267551c4. Though, I think the remaining cases are better off with lambdas.
So do that here, and enable the `modernize-avoid-bind` clang-tidy rule to avoid `std::bind` bugs in the future.
ACKs for top commit:
fjahr:
Code review ACK fad042235b
purpleKarrot:
Code review ACK fad042235b
Tree-SHA512: 38b17e26eda3ae47d84a8c34298309dc1eeb4ed434fda58b5803ef031c4c2edfb17222f5208f37af727bf340e32b37c7f81784f461d2b65fbc6227f3cd53eea4
c8abac9941 ci: mount .git dir rw (ci)
Pull request description:
On merges to master we set LINT_CI_SANITY_CHECK_COMMIT_SIG (when "GITHUB_REPOSITORY == bitcoin/bitcoin") which runs verify-commits.py.
This requires write access to the .git directory.
Make the mounted .git directory writable.
This is currently not run on PR branches or locally which caused a miss during review.
Ideally we can have the same checks running in PRs as on merges to master to avoid future discrepancies like this.
ACKs for top commit:
maflcko:
lgtm ACK c8abac9941
l0rinc:
untested code review ACK c8abac9941
Tree-SHA512: 7ae4f63227ecffe1dc9003454a7473d6d592550af2e1c899457f34a947e5604b04c13319fb8979f36789ae7787bed62066be60697d163ad5ebedde3fbe8ce45f
5aeaa71c77 lint: pass args from lint.py to cargo run in container (will)
c17a2adb8d lint: upgrade lint scripts for worktrees (will)
Pull request description:
Fixes#29972
Use a single script to run the linter locally or in CI.
Works from inside a worktree.
ACKs for top commit:
maflcko:
review ACK 5aeaa71c77🔒
davidgumberg:
code review and lightly tested reACK 5aeaa71
l0rinc:
Tested (+ lightly reviewed) ACK 5aeaa71c77
Tree-SHA512: 7c11f649b4752739d31c4f9e6306a98bd2e615b27a0819bbb5e7d9284b9e28bd9f424e145f16361f672f1a63441a1ae2f901c4f99759e997b72a4bf2d56d8d39
This is used for argument parsing in the retry script, however we don't
use the script with any arguments. So remove the unused code, and the
dependency on gnu-getopt.
This came up in the context of adding new CI jobs, where gnu-getopt
might not be available, or working properly. It seemed easier to just
remove the unused code, than look for more workarounds.
3f5211cba8 test: remove child_one/child_two (w)txid variables (naiyoma)
7cfe790820 test: replace ValidWitnessMalleatedTx class with function (naiyoma)
81675a781f test: use pre-generated chain (naiyoma)
Pull request description:
This PR refactors ` ValidWitnessMalleatedTx` class into a `build_malleated_tx_package` function. As a result, two tests are updated: `mempool_accept_wtxid` and `p2p_p2p_private_broadcast`. Also included are a few small refactors in mempool_accept_wtxid , (switching to MiniWallet, using a pre-mined chain, using txid directly.)
Together, these changes reduce complexity and improve test runtime.
ACKs for top commit:
stratospher:
reACK 3f5211c.
cedwies:
reACK 3f5211c
maflcko:
review ACK 3f5211cba8👥
rkrux:
ACK 3f5211cba8
Tree-SHA512: 1fd02be3432fef6b68e54fbe8b15ed56d2699580bb13d0777b21f9cbe4c6d33bbb710541e3ca2fc93eab771d17bf1c427e4b08fa216d561bdb320cc6b36ac8fc
14e56970cb Merge bitcoin-core/secp256k1#1794: ecmult: Use size_t for array indices
c7a52400d6 Merge bitcoin-core/secp256k1#1809: release cleanup: bump version after 0.7.1
ae7eb729c0 release cleanup: bump version after 0.7.1
1a53f4961f Merge bitcoin-core/secp256k1#1808: Prepare for 0.7.1
20a209f11c release: prepare for 0.7.1
c4b6a81a60 changelog: update in preparation for the v0.7.1 release
ebb35882da Merge bitcoin-core/secp256k1#1796: bench: fail early if user inputs invalid value for SECP256K1_BENCH_ITERS
c09215f7af bench: fail early if user inputs invalid value for SECP256K1_BENCH_ITERS
471e3a130d Merge bitcoin-core/secp256k1#1800: sage: verify Eisenstein integer connection for GLV constants
29ac4d8491 sage: verify Eisenstein integer connection for GLV constants
4721e077b4 Merge bitcoin-core/secp256k1#1793: doc/bench: added help text for SECP256K1_BENCH_ITERS env var for bench_ecmult
bd5ced1fe1 doc/bench: added help text for SECP256K1_BENCH_ITERS env var for bench_ecmult
47eb70959a ecmult: Use size_t for array indices in _odd_multiplies_table
bb1d199de5 ecmult: Use size_t for array indices into tables
2d9137ce9d Merge bitcoin-core/secp256k1#1764: group: Avoid using infinity field directly in other modules
f9a944ff2d Merge bitcoin-core/secp256k1#1790: doc: include arg -DSECP256K1_USE_EXTERNAL_DEFAULT_CALLBACKS=ON for cmake
0406cfc4d1 doc: include arg -DUSE_EXTERNAL_DEFAULT_CALLBACKS=1 for cmake
8d445730ec Merge bitcoin-core/secp256k1#1783: Add VERIFY_CHECKs and documentation that flags must be 0 or 1
aa2a39c1a7 Merge bitcoin-core/secp256k1#1778: doc/bench: Added cmake build options to bench error messages
540fec8ae9 Merge bitcoin-core/secp256k1#1788: test: split monolithic ellswift test into independent cases
d822b29021 test: split monolithic ellswift test into independent cases
ae00c552df Add VERIFY_CHECKs that flags are 0 or 1
5c75183344 Merge bitcoin-core/secp256k1#1784: refactor: remove ret from secp256k1_ec_pubkey_serialize
be5e4f02fd Merge bitcoin-core/secp256k1#1779: Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL
3daab83a60 refactor: remove ret from secp256k1_ec_pubkey_serialize
8bcda186d2 test: Add non-NULL checks for "pointer of array" API functions
5a08c1bcdc Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL
3b5b03f301 doc/bench: Added cmake build options to bench error messages
e7f7083b53 Merge bitcoin-core/secp256k1#1774: refactor: split up internal pubkey serialization function into compressed/uncompressed variants
b6c2a3cd77 Merge bitcoin-core/secp256k1#1761: ecmult_multi: reduce strauss memory usage by 30%
f5e815f430 remove secp256k1_eckey_pubkey_serialize function
0d3659c547 use new `_eckey_pubkey_serialize{33,65}` functions in modules (ellswift,musig)
adb76f82ea use new `_eckey_pubkey_serialize{33,65}` functions in public API
fc7458ca3e introduce `secp256k1_eckey_pubkey_serialize{33,65}` functions
c8206b1ce6 Merge bitcoin-core/secp256k1#1771: ci: Use Python virtual environment in "x86_64-macos-native" job
f252da7e6e ci: Use Python virtual environment in "x86_64-macos-native" job
115b135fe8 Merge bitcoin-core/secp256k1#1763: bench: Use `ALIGNMENT` macro instead of hardcoded value
2f73e5281d group: Avoid using infinity field directly in other modules
153eea20c2 bench: Use `ALIGNMENT` macro instead of hardcoded value
26166c4f5f ecmult_multi: reduce strauss memory usage by 30%
7a2fff85e8 Merge bitcoin-core/secp256k1#1758: ci: Drop workaround for Valgrind older than 3.20.0
43e7b115f7 Merge bitcoin-core/secp256k1#1759: ci: Switch to macOS 15 Sequoia Intel-based image
8bc50b72ff ci: Switch to macOS 15 Sequoia Intel-based image
c09519f0e3 ci: Drop workaround for Valgrind older than 3.20.0
git-subtree-dir: src/secp256k1
git-subtree-split: 14e56970cba37ffe4ee992c1e08707a16e22e345
a73a3ec553 doc: fix invalid arg name hints for bugprone validation (Lőrinc)
Pull request description:
The extra leading `=` or missing trailing `=` prevented clang-tidy's `bugprone-argument-comment` check from validating the parameter name, as it only matches comments formatted strictly as `/*parameter_name=*/` (see https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html).
I have considered doing a scripted diff, but the values I found aren't so numerous and can easily be reviewed manually.
ACKs for top commit:
b-l-u-e:
ACK a73a3ec tested and saw that argument comments now use the strict "/*param=*/" format required by bugprone-argument-comment
Sjors:
utACK a73a3ec553
maflcko:
review ACK a73a3ec553🍦
Tree-SHA512: 31177934d645116f381668a0f945028d7e04fab1fc6185dd0e3b7451aab71f89f1e4dd07246db667d1c4734eea3e5d73433c8b0e09181b3ece47dacc8677401e
Change this so we catch the case where the capnp shared libs have been
updated, and can no-longer be loaded by the Python module, resulting in
a skipped test, even though pycapnp is installed. i.e:
```bash
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 20, in <module>
import capnp # type: ignore[import] # noqa: F401
^^^^^^^^^^^^
File "/usr/local/lib64/python3.14/site-packages/capnp/__init__.py", line 36, in <module>
from .version import version as __version__
File "/usr/local/lib64/python3.14/site-packages/capnp/version.py", line 1, in <module>
from .lib.capnp import _CAPNP_VERSION_MAJOR as LIBCAPNP_VERSION_MAJOR # noqa: F401
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: libcapnpc.so.1.0.1: cannot open shared object file: No such file or directory
```
Failing in this way should make it clear that `pycapnp` needs to be
reinstalled/rebuilt.
If `pycapnp` is not installed, the test still skips as expected:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py skipped (capnp module not available.)
TEST | STATUS | DURATION
interface_ipc.py | ○ Skipped | 0 s
```
Fixes: #34016.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
eeee3755f8 fuzz: Return chrono point from ConsumeTime(), Add ConsumeDuration() (MarcoFalke)
faa5a9ebad fuzz: Use min option in ConsumeTime (MarcoFalke)
Pull request description:
Returning a raw i64 is a bit confusing when it comes to chrono types. For example, in the addrman fuzz tests, the `time_penalty` is not a time point, but a duration.
Also, all call-sites assume second resolution right now, so document that better by returning `NodeSeconds` from `ConsumeTime(...)` and `std::chrono::seconds` from `ConsumeDuration(...)`.
ACKs for top commit:
l0rinc:
ACK eeee3755f8
Crypt-iQ:
crACK eeee3755f8
Tree-SHA512: 25dd779a1bf79fa42c6e69db0f0593ad4daa4c0d746e8e82a26bdd65391a27c38e484431056d4e2207b542c511a71cb536c259809728a7166b8d304c0490e321
ccf9172ab3 util: Remove `FilterHeaderHasher` (rustaceanrob)
Pull request description:
With respect to `std::unordered_map` documentation, the `Hash` type
defined in the template is over the `Key` and not `T`, the value. This
hasher is incorrectly named as the `FilterHeader` is the value within this map.
I consider this a bug as opposed to a refactor as the key and value
relationship is implied to be `filter header -> block hash` when it is
the opposite.
Further, the hasher for the key already exists via `BlockHasher`.
ref: https://en.cppreference.com/w/cpp/container/unordered_map.html
ACKs for top commit:
andrewtoth:
ACK ccf9172ab3
maflcko:
lgtm ACK ccf9172ab3
ismaelsadeeq:
ACK ccf9172ab3👍🏾
Tree-SHA512: 607602391bf337d4e25b04a6a643fa32c3ab4599009b181b46ecdb0705e8ff2af89a6192042453c9e8e44abcb2150589019f02c5c944ecdff41322c3e0ad45ac
fa15a8d2d0 doc: Explain that low-effort pull requests may be closed (MarcoFalke)
Pull request description:
Lately, there seems to be a rise in low-effort pull requests. For example, where a contributor does not seem to understand the changes they are submitting, or it becomes clear that they have not tested the changes at all.
I don't think such pull requests are helpful, as they extract precious review time, which could be better spent on reviewing pull requests by reviewers who care about understanding the changes they are submitting, and who ensure their changes are sound and tested.
So document that such low-effort pull request may be closed.
ACKs for top commit:
l0rinc:
ACK fa15a8d2d0
willcl-ark:
ACK fa15a8d2d0
dergoegge:
ACK fa15a8d2d0
pinheadmz:
ACK fa15a8d2d0
Tree-SHA512: ba880f61c90c95e1e9007e337bad1a612a53ca85448f0ebfe97b34139489f22e5f709b8a0e302b11f71213e3b7863ab36ebd89b5c11cd550022d96493f917dd7
40735450c0 Remove unused epochguard.h (Suhas Daftuar)
1a8494d16c Rework CTxMemPool::GetChildren() to not use epochs (Suhas Daftuar)
Pull request description:
Since #33591, the epoch-based graph traversal optimization logic is only used for `CTxMempool::GetChildren()`, a function that is only used in RPC code and tests. Rewrite it without epochs, and remove `util/epochguard.h` itself, as that was its last use.
This allows us to reduce per-transaction memory usage by 8 bytes, for no material loss. With the new TxGraph-based mempool implementation, I also don't foresee future uses for it, as TxGraph can do even better by using BitSet-based traversal tracking.
ACKs for top commit:
ajtowns:
ACK 40735450c0
instagibbs:
ACK 40735450c0
l0rinc:
code review ACK 40735450c0
Tree-SHA512: 7ce7c04835cd2425a71c4fd47f316b6fb7381caa27383de7ecc4aa81100fcf7bc5e062699b307c08e0b853b35f06710d9ac761d6e660af9f9331e708d36f2fe0
9a9d797ef6 kernel: Add support for block headers (yuvicc)
b851ff6cae kernel: Add Handle/View pattern for BlockValidationState (yuvicc)
Pull request description:
Adds a new `btck_BlockHeader` type and associated functions to create, access, and validate block headers. Block headers will have their own type (`btck_BlockHeader`) that can be created from raw data, copied, and queried for all the standard header fields (hash, prev hash, timestamp, bits, version, nonce). We can also extract headers from full blocks or block tree entries.
The first commit here refactors `BlockValidationState` to use Handle/View pattern so external code can own them, which is required for the header processing in the API.
#### New Block Header API
- **`btck_BlockHeader` type**: Opaque handle for block headers
- **Header methods**:
- `btck_block_header_create()`: Create header from 80-byte serialized data
- `btck_block_header_copy()`: Copy block headers
- `btck_block_header_destroy()`: Destroy header object
- `btck_block_header_get_hash()`: Calculate block hash
- `btck_block_header_get_prev_hash()`: Get previous block hash
- `btck_block_header_get_timestamp()`: Get block timestamp
- `btck_block_header_get_bits()`: Get difficulty target (compact format)
- `btck_block_header_get_version()`: Get block version
- `btck_block_header_get_nonce()`: Get proof-of-work nonce
- `btck_block_get_header()`: Extract header from a full block
- `btck_block_tree_entry_get_block_header()`: Get header associated with a block tree entry
- **Header Processing Methods:**
- **`btck_chainstate_manager_process_block_header()`**: Validates and processes a block header without requiring the full block. This performs proof-of-work verification, timestamp validation, and updates the internal chain state.
- **`btck_chainstate_manager_get_best_entry()`**: Returns the block tree entry with the most cumulative proof-of-work.
Why `btck_chainstate_manager_get_best_entry()` is included alongside header validation? Just as we have logic to get the tip for block validation (so you can request more blocks extending your best from your peers), we need the equivalent for header validation. To make header validation worthwhile, knowing what the best current header is seems useful—it tells you what headers to request next from peers.
### Testing
Added tests in `test_kernel.cpp` that cover creating headers from raw data, extracting all header fields, and processing headers through the chainstate manager.
CC sedited
ACKs for top commit:
stringintech:
re-ACK 9a9d797e
sedited:
Re-ACK 9a9d797ef6
janb84:
ACK 9a9d797ef6
Tree-SHA512: 1dde9ef860543c906d1bb5e604f0d2956e7382fcbb55090686261b2277270a1fd3826f02ecf1749b2774da66e88f686c7845172b4c68b62259e7a7aee0825fa2
c9ce1c7c4a test: Fix P2PK script test (billymcbip)
Pull request description:
I found another script_tests case that isn't behaving the way it was meant to. It's a P2PK spend where we add an `OP_NOP8` to the scriptSig to make it non-push-only. The test should check that [`scriptSig.IsPushOnly()`](https://github.com/bitcoin/bitcoin/blob/691dc830c669061923a7eec677415013f75fd5d8/src/script/interpreter.cpp#L2055) is only enforced in P2SH mode when the scriptPubKey actually matches the P2SH pattern. To test this, we need to **turn on the P2SH flag**.
ACKs for top commit:
sipa:
ACK c9ce1c7c4a
darosior:
utACK c9ce1c7c4a
Tree-SHA512: 0af1d7b4651478349abc97cf0c009488cf5af5f97135382f7dd37cef0ef9b563192244330899a54ee7e0296bf03ba702e37a7aa15248c5c0ab4745095efc2402
1137debb85 doc: mempool: fix `removeUnchecked` incorrect comment (ismaelsadeeq)
Pull request description:
`CTxMemPool::removeUnchecked` description comment is stale and incorrect; the behaviour being described no longer applies in the post-cluster world. This PR is a simple fix that attempts to correctly describe what is being done in removeUnchecked.
ACKs for top commit:
instagibbs:
ACK 1137debb85
sipa:
ACK 1137debb85
Tree-SHA512: e410be57a83df50df01fcd6d7b07d08f0fe5a2abd229974f1ad269bb2e301608fd0d3912af349e2971f9a8abdbaa8e90c46d4832ec7b6858639642742b31a618
faa18dceba refactor: Use std::bind_front over std::bind (MarcoFalke)
Pull request description:
`std::bind` has many issues:
* It is verbosely listing all placeholders, but in a meaningless way, because it doesn't name the args or their types.
* It silently ignores args passed to it, when one arg is overridden. For example [1] compiles fine on current master.
* Accidentally duplicated placeholders compile fine as well.
* Usually the placeholders aren't even needed.
* This makes it hard to review, understand, and maintain.
Fix all issues by using `std::bind_front` from C++20, which allows to drop the brittle `_1, _2, ...` placeholders. The replacement should be correct, if the trailing placeholders are ordered.
Introducing the same silent bug on top of this pull request [2] will now lead to a compile failure.
----
[1]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 694fb535b5..7661dd361e 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, CTxDestination{}, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2));
```
[2]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 578713c0ab..84cced741c 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind_front(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this, CTxDestination{}));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind_front(NotifyTransactionChanged, this));
ACKs for top commit:
janb84:
cr ACK faa18dceba
fjahr:
Code review ACK faa18dceba
hebasto:
ACK faa18dceba, I have reviewed the code and it looks OK.
Tree-SHA512: 9dd13f49527e143a2beafbaae80b1358981f07a2ce20d25cffb1853089a32ff71639e6d718d1d193754522f9ac04e3e168ba017d5fc67a11a5918e79a92b3461
de509c6df9 iwyu: Add missed line to IWYU patch (Hennadii Stepanov)
Pull request description:
This PR makes IWYU suggest `<cassert>` over `<assert.h>`.
Fixes https://github.com/bitcoin/bitcoin/issues/34237.
ACKs for top commit:
maflcko:
lgtm ACK de509c6df9
Tree-SHA512: edba91eaf36992f684be2920f5da8c13a25ba6d79b879b92193e2af106cd454a64d7c4cf9dabc25675490df9edbccff1fd54c9f393e984a3a7a628b1c65f6c53
9482f00df0 chore: Update outdated GitHub Actions versions (Padraic Slattery)
Pull request description:
This PR updates outdated GitHub Action versions to ensure compatibility and improve functionality. The following changes are made to the GitHub Actions:
- `actions/upload-artifact` updated from v4 to v6
- `actions/cache` updated from v4 to v5
- `actions/download-artifact` updated from v5 to v7
The updates are necessary to support newer environments and features, and ensure consistent behavior across different workflows. The changes will be tested in the CI pipeline of the pull request.
ACKs for top commit:
fanquake:
ACK 9482f00df0
Tree-SHA512: 248e79162c5b2748e1a367d87a360d62eb961c24b4f8060bb932ef99a79ef10cab3e65175c092226c90140f31686fb9424911e6609729cb186b304b598a9af44
03f363d378 doc: Document IWYU workaround (Hennadii Stepanov)
Pull request description:
This PR addresses the following comments:
- https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2640003086:
> it would be good to reduce and report this bug upstream. Otherwise, wide-spread use of iwyu in this code-base seems risky.
- https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2640035350:
> Would have been good if it was documented, rather than adding undocumented workarounds for buggy tools.
ACKs for top commit:
maflcko:
lgtm ACK 03f363d378
sedited:
ACK 03f363d378
Tree-SHA512: 160a963c07f853995c8b4741a6ccca1d8431a576c760fca082116cebde4d133f7c8ec51f09e8f85f54428f86bad2635e1bd708177eecf71feb0bf1489f1e2b3e
0dafc0d83c clang-format: use AngleBracket for main includes (stickies-v)
Pull request description:
This project uses angle brackets instead of quotes for project-specific headers. Setting [`MainIncludeChar`](https://clang.llvm.org/docs/ClangFormatStyleOptions.html#mainincludechar) enables `clang-format` to automatically detect the main header, so it can be kept as the top group of includes.
For example, without this change, `clang-format` would demote `<signet.h>` from being the main header in `src/signet.cpp`. With this change, the order is preserved.
On 5e49f5d63c:
```
% clang-format src/signet.cpp | head -n 15
// Copyright (c) 2019-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <consensus/merkle.h>
#include <consensus/params.h>
#include <consensus/validation.h>
#include <logging.h>
#include <primitives/block.h>
#include <primitives/transaction.h>
#include <script/interpreter.h>
#include <script/script.h>
#include <signet.h>
#include <streams.h>
#include <uint256.h>
```
With this PR:
```
% clang-format src/signet.cpp | head -n 10
// Copyright (c) 2019-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <signet.h>
#include <consensus/merkle.h>
#include <consensus/params.h>
#include <consensus/validation.h>
#include <logging.h>
```
Note: `AngleBracket` `requires clang-format 19`, and will cause older versions (including our current minimum llvm version `17`) to fail
ACKs for top commit:
maflcko:
review ACK 0dafc0d83c
sedited:
Nice, ACK 0dafc0d83c
hebasto:
ACK 0dafc0d83c, tested on Ubuntu 25.10.
Tree-SHA512: c0876f505ec188f76e435af0731c411c66266b83e4c08528d0637263abcd84b3968ee6fbfa72630192f1a0cd2728af873d3d6c32f93ab8b228222fad16f232be
merge-script
