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
979d41bfab qa: Fix Windows logging bug (Hennadii Stepanov)
Pull request description:
The regex `(.*)` was capturing `\r` from subprocess output on Windows, causing the closing parenthesis in logs to wrap to the next line.
For [example](https://github.com/hebasto/bitcoin/actions/runs/20993438084/job/60350204808):
```
208/454 - feature_bip68_sequence.py passed, Duration: 10 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system
)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system
)
211/454 - rpc_packages.py passed, Duration: 8 s
212/454 - rpc_bind.py --nonloopback skipped (not on a Linux system
)
213/454 - p2p_feefilter.py passed, Duration: 4 s
```
Stripping whitespace from the regex match fixes the formatting. [See](https://github.com/hebasto/bitcoin/actions/runs/20993564177/job/60350024373):
```
208/454 - feature_bip68_sequence.py passed, Duration: 9 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system)
211/454 - rpc_bind.py --nonloopback skipped (not on a Linux system)
212/454 - rpc_packages.py passed, Duration: 7 s
```
ACKs for top commit:
maflcko:
lgtm ACK 979d41bfab
l0rinc:
lightly tested ACK 979d41bfab
Tree-SHA512: bafe1937a519e45e4cab395bae622acf65220f313c773a0729ba7dccc3a0a048602f1c04b3e8cdd80d2cf68ae36cef802a819530485d5a745db8abcadf141f68
3e340672ec test: use ephemeral ports in p2p_private_broadcast.py (w0xlt)
Pull request description:
The test `p2p_private_broadcast.py` gets some Python P2P nodes to listen and instructs the SOCKS5 proxy to redirect connections to them instead of to the requested addresses. This way the `bitcoind` which uses the proxy is tricked to think it has connected to real routable internet IP addresses or `.onion` addresses.
Picking the ports where to Python P2P nodes to listen however is tricky to be done in a non-conflicting way, given that other tests may run in parallel. https://github.com/bitcoin/bitcoin/pull/34186 made it possible to let the OS select a free port, so use that in
`p2p_private_broadcast.py`.
---
_Suggested in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875_
ACKs for top commit:
l0rinc:
code review ACK 3e340672ec
polespinasa:
tACK 3e340672ec
mzumsande:
utACK 3e340672ec
Tree-SHA512: e94efd33a1845e1767aaada55f91c60bc5fc1166c281ef578a391e95e2791a922d84aa6ed1ce06e7d6ca1a65f84da52fd79d9b2f40705c1944a53c67b7392e4d
d09a19fd41 test: add coverage for issue 34206 (Greg Sanders)
4c7cfd37ad wallet: remove erroneous-on-reorg Assume() (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/34206
I'm not certain the test is worth keeping, but included it for now to show minimal example that crashes without fix. Can be removed.
ACKs for top commit:
bensig:
ACK d09a19fd41
dergoegge:
utACK d09a19fd41
Tree-SHA512: 7eac19e97be6db8e38af396c406066fdcec532332e685a38bb33f0a988701c7bd5a0967f51426737fd56972847b761a3d873495928ff66efa8512fb267a9622b
fac70ea8b5 fuzz: Exclude too expensive inputs in miniscript_string target (MarcoFalke)
fa90786478 iwyu: Fix includes for test/fuzz/util/descriptor module (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/30498
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
For example this one will take several seconds (the flamegraph shows the time is spent in minscipt `NoDupCheck`):
```
curl -fLO 'https://raw.githubusercontent.com/bitcoin-core/qa-assets/eac1c57614d7823bcd6079814749f72018aea438/fuzz_corpora/miniscript_string/41bae50cffd1741150a1b330d02ab09f46ff8cd1'
FUZZ=miniscript_string /usr/bin/time ./bld-cmake/bin/fuzz ./41bae50cffd1741150a1b330d02ab09f46ff8cd1
```
Inspecting the inputs shows that it has many sub frags, so rejecting based on `HasTooManySubFrag` should be sufficient.
ACKs for top commit:
darosior:
ACK fac70ea8b5
brunoerg:
code review ACK fac70ea8b5
dergoegge:
utACK fac70ea8b5
Tree-SHA512: 7f1e0d9ce24d67ec63e5b7c2dd194efa51f38beb013564690afe0f920e5ff1980c85ce344828c0dc3f34b6851db7fe72a76b1a775c6d51c94fb91431834f453b
da56ef239b clusterlin: minimize chunks (feature) (Pieter Wuille)
Pull request description:
Part of #30289.
This was split off from #34023, because it's not really an optimization but a feature. The feature existed pre-SFL, so this brings SFL to parity in terms of functionality with the old code.
The idea is that while optimality - as achieved by SFL before this PR - guarantees a linearization whose feerate diagram is optimal, it may be possible to split chunks into smaller equal-feerate parts. This is desirable because even though it doesn't change the diagram, it provides more flexibility for optimization (binpacking is easier when the pieces are smaller).
Thus, this PR introduces the stronger notion of "minimality": optimal chunks, which are also split into their smallest possible pieces. To accomplish that, an additional step in the SFL algorithm is added which aims to split chunks into minimal equal-feerate parts where possible, without introducing circular dependencies between them. It works based on the observation that if an (already otherwise optimal) chunk has a way of being split into two equal-feerate parts, and T is a given transaction in the chunk, then we can find the split in two steps:
* One time, pretend T has $\epsilon$ higher feerate than it really has. If a split exists with T in the top part, this will find it.
* The other time, pretend T has $\epsilon$ lower feerate than it really has. If a split exists with T in the bottom part, this will find it.
So we try both on each found optimal chunk. If neither works, the chunk is minimal. If one works, recurse into the split chunks to split them further.
ACKs for top commit:
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/34259/commits/da56ef239b12786e3a177afda14352dda4a70bc6
marcofleon:
crACK da56ef239b
Tree-SHA512: 2e94d6b78725f5f9470a939dedef46450b85c4e5e6f30cba0b038622ec2b417380747e8df923d1f303706602ab6d834350716df9678de144f857e3a8d163f6c2
f62568c97c ci: Install `pyzmq` for functional tests on Windows (Hennadii Stepanov)
Pull request description:
This PR enables `interface_zmq.py` on Windows by installing the required `pyzmq` package.
ACKs for top commit:
maflcko:
lgtm ACK f62568c97c
Tree-SHA512: a57728ef82f1ff15a3ed1a659e98ad2d10f69693772649eaa97e1803e18f2f68e99782be3cb149322d1bf72607ec103e1837db1db39e9bd8c2a0a0863463635d
7fc465ece8 doc: fix incorrect description of `PackageMempoolChecks` (ismaelsadeeq)
1412b779ad refactor: execute `PackageMempoolChecks` during package rbf only (ismaelsadeeq)
Pull request description:
This is a simple PR that fixes the incorrect description of what is done in `PackageMempoolChecks`
> // Enforce package mempool ancestor/descendant limits (distinct from individual
> // ancestor/descendant limits done in PreChecks) and run Package RBF checks.
After cluster mempool, we no longer enforce ancestor/descendant limits in both `PreChecks` and `PackageMempoolChecks`; instead, cluster limit is enforced in `PackageMempoolChecks`.
This PR fixes the incorrect comment by;
- Making it clear why it is necessary to have two calls of `CheckMempoolPolicyLimts` in both `PackageMempoolChecks` and after in `AcceptMultipleTransactionsInternal` by executing `PackageMempoolChecks` only during package RBF only. No need to jump into the next subroutine when there is no conflict.
- Renames `PackageMempoolChecks` to `PackageRBFChecks`; the method name is self-explanatory now, hence no need for a description comment.
ACKs for top commit:
yashbhutwala:
ACK 7fc465ece8
instagibbs:
ACK 7fc465ece8
glozow:
utACK 7fc465ece8
Tree-SHA512: 38655f9d05be54cadd224fad376da9871a85efc7801306b58d4f51aee658036cdce2ab406143a3439d7211fc9bb0fc86bd330852e8926d79660944872b8fae8d
fa2959e16d test: Fail on self-check warnings in test_runner.py (MarcoFalke)
Pull request description:
I don't see a reason to start running the tests, if the test_runner detects warnings during the self-check.
Usually, this will just lead to a possibly confusing test failure after some wasted time anyway.
So just fail fast before even running any tests.
If there was a reason to ignore the warnings, a new option could trivially be added:
```py
parser.add_argument("--ignore-self-check-warnings", dest="ignore_warnings", default=False, action="store_true",
help="Ignore test runner warnings about self-checks before running the tests")
```
However, I don't see the need.
ACKs for top commit:
hodlinator:
utACK fa2959e16d
Tree-SHA512: 3f2e1af9ba06cd805c69e8c40149214a8a13af286ba6315c346e23d0f1ddae8e850103b6967d2d9799a095f6a8ec8802c3c773af8d7123598e8887f56c764fb5
fa5032f0e4 ci: Run feature_unsupported_utxo_db.py on Windows (MarcoFalke)
Pull request description:
The feature_unsupported_utxo_db.py test is skipped on Windows, because the ancient exe used in the test does not support UTF-8.
One way to fix this could be to rework the test_runner to supply an ASCII temp dir. However, this is tedious and probably not worth it for a test that is close to being deleted: The test only checks that a `-reindex-chainstate` is sufficient, but at this point, it seems fine to accept users having to go through a full `-reindex`, or even full re-download.
I am not deleting it just yet, because it is still minimally useful: E.g. https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2599106040. Though, I think the next time an issue pops up with this test, it can be considered for deletion.
ACKs for top commit:
hebasto:
ACK fa5032f0e4.
Tree-SHA512: eb3be204a1238caa93c7c2947171ed3c1c2717ccf95e5a58102791dbecfcd8710e0d89f001e266c192734bb4326ec6ca6e0df3128ad0d1e76cfe0f34f486a08e
fa1d17d56c refactor: Use uint64_t over size_t for serialize corruption check in fees.dat (MarcoFalke)
Pull request description:
Serialization should not behave differently on different architectures. See also the related commit 3789215f73.
However, on fees.dat file corruption, 32-bit builds may run into an unsigned integer overflow and report the wrong corruption reason, or may even silently continue after the corruption.
This is a bit hard to reproduce, because 32-bit platforms are rare and most of them don't support running the unsigned integer overflow sanitizer. So the possible options to reproduce are:
* Run on armhf and manually annotate the code to detect the overflow
* Run on i386 with the integer sanitizer (possibly via `podman run -it --rm --platform linux/i386 'debian:trixie'`)
* Run the integer sanitizer on any 64-bit platform and manually replace type in the affected line by `uint32_t`
Afterwards, the steps to reproduce are:
```
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev -y
cmake -B ./bld-cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero --preset=dev-mode
cmake --build ./bld-cmake --parallel $(nproc)
curl -fLO 'https://github.com/bitcoin-core/qa-assets/raw/b5ad78e070e4cf36beb415d7b490d948d70ba73f/fuzz_corpora/policy_estimator_io/607473137013139e3676e30ec4b29639e673fa9b'
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=policy_estimator_io ./bld-cmake/bin/fuzz ./607473137013139e3676e30ec4b29639e673fa9b
```
The output will be something like:
```
/b-c/src/policy/fees/block_policy_estimator.cpp:448:25: runtime error: unsigned integer overflow: 346685954 * 219 cannot be represented in type 'unsigned int'
#0 0x5b0b1bbe in TxConfirmStats::Read(AutoFile&, unsigned int) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:448:25
#1 0x5b0b7d3f in CBlockPolicyEstimator::Read(AutoFile&) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:1037:29
#2 0x592a9783 in policy_estimator_io_fuzz_target(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/./test/fuzz/policy_estimator_io.cpp:32:32
#3 0x5896ba8e in void std::__invoke_impl<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(std::__invoke_other, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:61:14
#4 0x5896b8eb in std::enable_if<is_invocable_r_v<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>, void>::type std::__invoke_r<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:111:2
#5 0x5896b44b in std::_Function_handler<void (std::span<unsigned char const, 4294967295u>), void (*)(std::span<unsigned char const, 4294967295u>)>::_M_invoke(std::_Any_data const&, std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:290:9
#6 0x59845c95 in std::function<void (std::span<unsigned char const, 4294967295u>)>::operator()(std::span<unsigned char const, 4294967295u>) const /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:591:9
#7 0x5983a0da in test_one_input(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5
#8 0x5983cb80 in main /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:271:13
#9 0xf75aecc2 (/lib/i386-linux-gnu/libc.so.6+0x24cc2) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
#10 0xf75aed87 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x24d87) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
#11 0x58932db6 in _start (/b-c/bld-cmake/bin/fuzz+0x235ddb6) (BuildId: 7d8d83a77923f14e99c0de64acbc5f5bfc2cce9b)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /b-c/src/policy/fees/block_policy_estimator.cpp:448:25
```
Note: This is marked a "refactor", because the code change does not affect 64-bit builds, and on the still remaining rare 32-bit builds today it is extremely unlikely to happen in production.
ACKs for top commit:
bensig:
ACK fa1d17d56c
ismaelsadeeq:
utACK fa1d17d56c
luke-jr:
Also, utACK fa1d17d56c as an improvement.
Tree-SHA512: 696bf8e0dbe4777c84cb90e313c7f8f9ee90d4b3e64de1222f8472b2d9d0f3a0f6f027fda743dd6ca8c6aab94f404db7a65bb562a76000d9c33a8a39de28d8d4
2cade5d5d1 [miniminer] stop assuming ancestor fees >= self fees (glozow)
Pull request description:
These assertions exist to detect double-deducting values when we update descendants. However, negative fees are possible with `prioritisetransaction` so it doesn't make sense to check this.
Leave the check for sizes because those are never negative.
Fixes#34234
ACKs for top commit:
instagibbs:
ACK 2cade5d5d1
dergoegge:
utACK 2cade5d5d1
Tree-SHA512: 935bbc8bd9a0d508eea43bb49aa43c22735e3f2c1012598f6843e229c13b76e44f9fd3eb8b61c437fa0b32353b4e7b15afa3e31002bdfa382d3d711d16419fde
eeaf28dbe0 wallet: test: Failed migration cleanup (David Gumberg)
Pull request description:
Prior to https://github.com/bitcoin/bitcoin/pull/34156, an issue existed where if migration of a wallet with a relative pathname failed, the relatively specified path where the legacy wallet is would be deleted. This issue predates #32273, because the relative pathnames get stacked together, e.g. "../../", the copy conflict bug that caused migration to abort early instead of getting far enough to attempt clean-up that was fixed in #32273 is avoided.
This is a functional test demonstrating that we handle failed migration clean-up correctly for relatively-named wallets. To see the issue, you can backport this test onto 29.x: https://github.com/davidgumberg/bitcoin/tree/2026-01-07-rel-migration-test-backport
I've also added an absolute path failed migration cleanup test. WRT this and #34156, absolute paths exhibit similar behavior to unnamed wallets. Because of the name-conflict bug prior to #32273 an absolute-path migration would fail no matter what because migration would attempt to copy a file to a destination that already exists. But after #32273, absolute-path migration gets past there, and if it fails for some other reason, the same behavior that's fixed in #34156 occurs where the directory containing the wallet file is deleted.
ACKs for top commit:
achow101:
ACK eeaf28dbe0
furszy:
ACK eeaf28dbe0
rkrux:
lgtm ACK eeaf28dbe0
Tree-SHA512: ee366fe526d0328654a86c2e9e6f228ca81554c8f8a78c259fa7aab90f024f9e5694ecf3f1d188938355f4e6d351c5a6a8ad236701bdd0ce63005e5d42c15e15
cbf0bd35bb test: migration, avoid backup name mismatch in default_wallet_failure (furszy)
Pull request description:
This is a possible test failure, pushing it in case the CI starts complaining.
The change affects only test code; no cpp logic is involved.
The `test_default_wallet_failure` migration test calls the function
`migrate_and_get_rpc()`, which sets the mock time internally. But, at the
same time, the test already caches the mock time value, to later use it
in the backup existence check.
Setting the mock time twice can lead to a name mismatch during the
mentioned check (diff timestamp == diff backup names), which could
cause the test to fail.
The fix is very simple, just need to call the migration RPC directly.
Since the test expects the migration to fail, `migrate_and_get_rpc()` is
unnecessary here. I'm surprised the CI hasn't complained about this yet.
ACKs for top commit:
achow101:
ACK cbf0bd35bb
bensig:
ACK cbf0bd35bb
Tree-SHA512: 10b43a491b8ad0c5bf53e423b7d7587fc631551bf5d598e145e1defe9d8e5786c0869a9aee26209e63ccafd828ece34fc40c75abe246c1301b9f17467d64ef28
f78f6f1dc8 wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow)
Pull request description:
As pointed out in https://github.com/bitcoin/bitcoin/pull/34156#issuecomment-3716728670, it is possible for `createfromdump` to also accidentally delete the entire wallets directory if the wallet name is the empty string and the dumpfile contains a checksum error.
This is also fixed by removing the files created by only removing the directory for named wallets, and avoiding the use of `fs::remove_all`.
ACKs for top commit:
waketraindev:
lgtm ACK f78f6f1dc8
polespinasa:
code review and tACK f78f6f1dc8
rkrux:
Code review and tACK f78f6f1dc8
willcl-ark:
ACK f78f6f1dc8
pablomartin4btc:
ACK f78f6f1dc8
Tree-SHA512: ff1e7668131ec3632c67d990c99e8fddff28605e7e553c7e20695e61017c88476c3636e22f2007e763a00d527e80e4d1d3d45409f6678d28729b8397430bfe7a
b7c34d08dd test: coverage for migration failure when last sync is beyond prune height (furszy)
82caa8193a wallet: migration, fix watch-only and solvables wallets names (furszy)
d70b159c42 wallet: improve post-migration logging (furszy)
f011e0f068 test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy)
36093bde63 test: add coverage for unnamed wallet migration failure (furszy)
f4c7e28e80 wallet: fix unnamed wallet migration failure (furszy)
4ed0693a3f wallet: RestoreWallet failure, erase only what was created (furszy)
Pull request description:
Minimal fix for #34128.
The issue occurs during the migration of a legacy unnamed wallet
(the legacy "default" wallet). When the migration fails, the cleanup
logic is triggered to roll back the state, which involves erasing the
newly created descriptor wallets directories. Normally, this only
affects the parent directories of named wallets, since they each
reside in their own directories. However, because the unnamed
wallet resides directly in the top-level `/wallets/` folder, this
logic accidentally deletes the main directory.
The fix ensures that only the wallet.dat file of the unnamed wallet
is touched and restored, preserving the wallet in BDB format and
leaving the main `/wallets/` directory intact.
#### Story Line:
#32273 fixed a different set of issues and, in doing so, uncovered
this one.
Before the mentioned PR, backups were stored in the same directory
as the wallet.dat file. On a migration failure, the backup was then
copied to the top-level `/wallets/` directory. For the unnamed legacy
wallet, the wallet directory is the `/wallets/` directory, so the source
and destination paths were identical. As a result, we threw early in the
`fs::copy_file` call ([here](https://github.com/bitcoin/bitcoin/blob/29.x/src/wallet/wallet.cpp#L4572)) because the file already existed, as we
were trying to copy the file onto itself. This caused the cleanup logic
to abort early on and never reach the removal line.
#### Testing Notes:
Cherry-pick the test commit on top of master and run it. You will
see the failure and realize the reason by reading the test code.
ACKs for top commit:
achow101:
ACK b7c34d08dd
davidgumberg:
crACK https://github.com/bitcoin/bitcoin/commit/b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb
w0xlt:
ACK https://github.com/bitcoin/bitcoin/commit/b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb
willcl-ark:
ACK b7c34d08dd
Tree-SHA512: d0be14c0ed6417f999c3f2f429652c2407097d0cc18453c91653e57ae4b5375b327ad3b2553d9ea6ff46a3ae00cdbd5ab325b94eba763072c4fc5a773b85618b
1808b5aaf7 clusterlin: remove unused FixLinearization (cleanup) (Pieter Wuille)
34a77138b7 txgraph: permit non-topological clusters to defer fixing (optimization) (Pieter Wuille)
3380e0cbb5 txgraph: use PostLinearize less prior to linearizing (Pieter Wuille)
62dd88624a txgraph: drop NEEDS_SPLIT_ACCEPTABLE (simplification) (Pieter Wuille)
01ffcf464a clusterlin: support fixing linearizations (feature) (Pieter Wuille)
Pull request description:
Part of #30289, follow-up to #32545.
This gets rid of `FixLinearization()` by integrating the functionality into `Linearize()`, and makes txgraph exploit that (by delaying fixing of clusters until their first re-linearization). It also reduces (but does not eliminate) the number of calls to `PostLinearize`, as the SFL linearization effectively performs something very similar to postlinearization when loading in an existing linearization already.
ACKs for top commit:
instagibbs:
reACK 1808b5aaf7
marcofleon:
code review ACK 1808b5aaf7
Tree-SHA512: 81cd9549de2968f5126079cbb532e2cb052ea8157c9c9ce37fd39ad2294105d7c79ee8d946c3d8f7af5b2119299a232c448b42a33e1e43ccc778a5b52957e387
merge-script