444409ff2b ci: Reduce Alpine musl task to md runner size (MarcoFalke)
fa6b2e9efe ci: Turn centos config into alpine musl config (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/33437
Historically, the centos task was added to add CI coverage for old packages and 32-bit depends builds, but both are now covered by different tasks.
The CentOS task aligns with Ubuntu/Debian CI tasks in terms of libc usage, but (slightly) differs in package naming and update philosophy. I am not aware of the task ever discovering a centos-related issue, so it seems fine to recycle it into an Alpine Linux task.
The main difference would be that musl libc is now used. Also, busybox is used in Alpine, so in theory the busybox install could be removed from the arm CI task in the future.
Packaging considerations: All packages should roughly be the same (gcc remains at version 14, python remains at version 3.12, etc). Also, all packages are from the Alpine main track, coming with 2 years of support. The only exception is the py3-pip package (https://pkgs.alpinelinux.org/packages?name=py3-pip&branch=v3.22&repo=&arch=riscv64) from the community track, however, I don't expect any issues arising from that.
ACKs for top commit:
janb84:
reACK 444409ff2b
willcl-ark:
ACK 444409ff2b
Tree-SHA512: fd1a1da0fd766591e44a57dbdb84f9b3b47ca92113a429bba139ee5fef54714b8fe509c321e7b3a470c29b4af7d9eab9786e1660b9effb862ecea52824f458aa
3a10d700bc test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH` flag (brunoerg)
Pull request description:
This PR adds a test case for `GetTransactionSigOpCost` to check that P2SH sig ops are only counted when `SCRIPT_VERIFY_P2SH` flag is set.
Kills the following [mutant](https://corecheck.dev/mutation/src/consensus/tx_verify.cpp#L150):
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 9d09872597..cc7cdaaf8f 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
if (tx.IsCoinBase())
return nSigOps;
- if (flags & SCRIPT_VERIFY_P2SH) {
+ if (1==1) {
nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
}
```
ACKs for top commit:
l0rinc:
Tested ACK 3a10d700bc
maflcko:
re-lgtm ACK 3a10d700bc
instagibbs:
ACK 3a10d700bc
janb84:
tested ACK 3a10d700bc
Tree-SHA512: f560b4f9f2ce5c5fdd0a86e7e1f8ea27a8c6fda0327a6186a0c21e2c06ef13beeb017686db1688cace68812a01701abe46e8e1a095afefc6f2aed6ed96ba8288
e4c04f7759 ci: add libcpp hardening flags to macOS fuzz job (fanquake)
Pull request description:
Follows up to https://github.com/bitcoin/bitcoin/pull/33425#issuecomment-3323149107.
ACKs for top commit:
maflcko:
lgtm ACK e4c04f7759. The qa-assets repo has a libc++ debug run, so this isn't required, but it seems fast enough to not hurt.
Tree-SHA512: 6c0dc90528ca867df49027eebf2d1c417a7395f9f94779076ace48e3e4b21771e7d99e8b3ed75ca56da87153418a446075429aa6b9ec5cd4b6b8cb5b0c25c1d7
ac599c4a9c test: Test MuSig2 in the wallet (Ava Chow)
68ef954c4c wallet: Keep secnonces in DescriptorScriptPubKeyMan (Ava Chow)
4a273edda0 sign: Create MuSig2 signatures for known MuSig2 aggregate keys (Ava Chow)
258db93889 sign: Add CreateMuSig2AggregateSig (Ava Chow)
bf69442b3f sign: Add CreateMuSig2PartialSig (Ava Chow)
512b17fc56 sign: Add CreateMuSig2Nonce (Ava Chow)
82ea67c607 musig: Add MuSig2AggregatePubkeys variant that validates the aggregate (Ava Chow)
d99a081679 psbt: MuSig2 data in Fill/FromSignatureData (Ava Chow)
4d8b4f5336 signingprovider: Add musig2 secnonces (Ava Chow)
c06a1dc86f Add MuSig2SecNonce class for secure allocation of musig nonces (Ava Chow)
9baff05e49 sign: Include taproot output key's KeyOriginInfo in sigdata (Ava Chow)
4b24bfeab9 pubkey: Return tweaks from BIP32 derivation (Ava Chow)
f14876213a musig: Move synthetic xpub construction to its own function (Ava Chow)
fb8720f1e0 sign: Refactor Schnorr sighash computation out of CreateSchnorrSig (Ava Chow)
a4cfddda64 tests: Clarify why musig derivation adds a pubkey and xpub (Ava Chow)
39a63bf2e7 descriptors: Add a doxygen comment for has_hardened output_parameter (Ava Chow)
2320184d0e descriptors: Fix meaning of any_key_parsed (Ava Chow)
Pull request description:
This PR implements MuSig2 signing so that the wallet can receive and spend from imported `musig(0` descriptors.
The libsecp musig module is enabled so that it can be used for all of the MuSig2 cryptography.
Secnonces are handled in a separate class which holds the libsecp secnonce object in a `secure_unique_ptr`. Since secnonces must not be used, this class has no serialization and will only live in memory. A restart of the software will require a restart of the MuSig2 signing process.
ACKs for top commit:
fjahr:
tACK ac599c4a9c
rkrux:
lgtm tACK ac599c4a9c
theStack:
Code-review ACK ac599c4a9c🗝️
Tree-SHA512: 626b9adc42ed2403e2f4405321eb9ce009a829c07d968e95ab288fe4940b195b0af35ca279a4a7fa51af76e55382bad6f63a23bca14a84140559b3c667e7041e
0626b90f50 multiprocess: align our logging with libmultiprocess's (Cory Fields)
9d068225ee multiprocess: update multiprocess EventLoop construction to use options (Cory Fields)
Pull request description:
This fixes https://github.com/bitcoin-core/libmultiprocess/issues/215 on Core's side. ~It depends on https://github.com/bitcoin-core/libmultiprocess/pull/220 being merged upstream, and a PR to update our subtree. I've included a subtree merge from my repo here for now, but will rebase on top of the merge from upstream once it's in.~ Edit: Rebased on top of #33518.
For context: before https://github.com/bitcoin-core/libmultiprocess/pull/220, libmultiprocess serializesd every log message parameter, even if that message was ultimately going to be discarded. The upstream PR accomplished 2 main things:
- Creates logging categories, similar to Core's
- Using macros, avoids serializing parameters for disabled log levels.
That allowed the expensive serialization to be skipped, but the default log level is `Trace`. This PR updates our usage of libmultiprocess options to honor our log categories and levels.
Because of the substantial unnecessary overhead (see the [flamegraphs](https://github.com/bitcoin-core/libmultiprocess/issues/215). Logging accounts for 50% of my application's cpu time, and nearly 10% of bitcoin-node's, both of which go to ~0% once fixed), it'd be a shame to ship the first multiprocess binaries without this fixed. So I propose that we also backport this (and the required libmultiprocess subtree merge) ~to v30. Sorry about the timing~ :(
Edit: Didn't make it for v30, but it would still make sense to backport for a v30.1.
ACKs for top commit:
Sjors:
ACK 0626b90f50
TheCharlatan:
ACK 0626b90f50
sipa:
utACK 0626b90f50
Tree-SHA512: 70b63b62d1f6de547f4d4775538d7bcaf32f57d8a72c5b26762b57755810c8be6942d9dfebab43cf1c1d8d025a555f72a48e9ebf3d84f8d40d6592ca801cda5d
7b544341c0 test: change log rate limit version gate from 299900 to 290100 (Eugene Siegel)
Pull request description:
Change the version gate from 299900 to 290100 for bypassing the log rate limit in case an explicit version is set in the functional test framework.
See discussion here: https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2287838255
ACKs for top commit:
maflcko:
lgtm ACK 7b544341c0
janb84:
ACK 7b544341c0
stickies-v:
ACK 7b544341c0
Tree-SHA512: c07c8741dfdeca87c49748b7082c2ecb829da391908316f35daef7292bc017814a89f04e16e738f3a105541bbc38e4feb5bca3fb6ab718a1dc1de7c70a9c8a58
023cd5a546 txgraph: add SingletonClusterImpl (mem optimization) (Pieter Wuille)
e346250732 txgraph: give Clusters a range of intended tx counts (preparation) (Pieter Wuille)
e93b0f09cc txgraph: abstract out creation of empty Clusters (refactor) (Pieter Wuille)
6baf12621f txgraph: comment fixes (doc fix) (Pieter Wuille)
726b995739 txgraph: make Cluster an abstract class (refactor) (Pieter Wuille)
2602d89edd txgraph: avoid accessing other Cluster internals (refactor) (Pieter Wuille)
04c808ac4c txgraph: expose memory usage estimate function (feature) (Pieter Wuille)
7680bb8fd4 txgraph: keep track of Cluster memory usage (preparation) (Pieter Wuille)
4ba562e5f4 txgraph: keep data structures compact (mem optimization) (Pieter Wuille)
bb5cb222ae depgraph: add memory usage control (feature) (Pieter Wuille)
b1637a90de txgraph: avoid holes in DepGraph positions (mem optimization) (Pieter Wuille)
2b1d302508 txgraph: move some sanity checks from Cluster to TxGraphImpl (refactor) (Pieter Wuille)
d40302fbaf txgraph: Make level of Cluster implicit (optimization) (Pieter Wuille)
Pull request description:
Part of #30289.
This adds a few optimizations to reduce `TxGraph`'s memory usage, and makes sure that dynamic memory it uses doesn't linger after shrinking clusters. Finally, it exposes a function `GetMainMemoryUsage()` to compute `TxGraph`'s approximate memory usage.
It makes the `Cluster` type abstract, with two instances (`SingletonClusterImpl` for 1-transaction clusters, and `GenericClusterImpl` for others).
On my 64-bit system, I obtain the following numbers:
* `SingletonClusterImpl`: 48 bytes, plus 16 bytes malloc overhead in its `unique_ptr`, plus 8-byte pointer in `m_clusters`
* `GenericClusterImpl`: 104 bytes, plus 16 bytes malloc overhead in its `unique_ptr`, plus 8-byte pointer in `m_clusters`, plus 72 bytes malloc overhead inside its vectors and `DepGraph`, plus 40 bytes per transaction in those.
* `TxGraphImpl::Entry`: 72 bytes per transaction
* `TxGraphImpl::ChunkData`: 8 bytes, plus 56 bytes in `std::set` overhead + malloc overhead, all per chunk.
* `TxGraph::Ref`: 16 bytes per transaction
This overall amounts to 200 bytes per cluster, plus 64 bytes per chunk, plus 128 bytes per transaction, but only 224 bytes overall per singleton cluster.
ACKs for top commit:
l0rinc:
code review reACK 023cd5a546
instagibbs:
reACK 023cd5a546
ismaelsadeeq:
reACK 023cd5a546🚢
glozow:
reACK 023cd5a546
Tree-SHA512: c957b27f47318be7c25d71453df2ae9d4e7bf21dab13b6e5e975cca122a221a99b15c584872491225785d276a9165f090675ee0f4460a2775bd3271933e3b246
bc706955d7 ci: expose all ACTIONS_* vars (willcl-ark)
Pull request description:
When using `docker buildx build` in conjunction with the `gha` backend cache type (as we do in our CI) it's important to specify the URL and TOKEN needed to authenticate.
On Cirrus runners this is working with only `ACTIONS_CACHE_URL` and `ACTIONS_RUNTIME_TOKEN`, but this is not enough for the GitHub backend.
Fix this by exporting all `ACTIONS_*` variables.
This fixes docker build layer cache restore/save on forks or where GH-hosted runners are being used, and addresses https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3324707093
ACKs for top commit:
m3dwards:
ACK bc706955d7
maflcko:
lgtm ACK bc706955d7
Tree-SHA512: 13e973bb1c1ca5448dd6c3c176fb5ce39c725886ba2012d3253158205309a7038a1430135b37400e1f2f69408a9d0f4e2b3c5f0515154a593ec382ab7db10266
fa6fd16f36 ci: Use native platform for win-cross task (MarcoFalke)
Pull request description:
Forcing the architecture to amd64 is no longer required. Dropping it should have some benefits:
* Faster CI speed on other arches (riscv64, arm, ...)
* Unlock the CI task to run on riscv64 at all
ACKs for top commit:
hebasto:
ACK fa6fd16f36, tested on Ubuntu 24.04, RISC-V.
Tree-SHA512: 68a3fc90cc22ab085d6946deb106e50b22e06eebc61523a9dcb53b38a50021a19da26cc29e2cd20f4673ffc5cc10f441dacca7cc799782258351609d9fa04969
671b774d1b depends: Use $(package)_file_name when downloading from the fallback (Ava Chow)
Pull request description:
The server hosting the fallbacks uses `make download` so the files are only available with their overridden names rather than the original name on the upstream source. We should therefore also use the overridden name when downloading from the fallback.
Fixes https://github.com/bitcoin-core/bitcoincore.org/issues/1168
ACKs for top commit:
theuni:
utACK 671b774d1b. I was going to PR the same change.
janb84:
ut ACK 671b774d1b
hebasto:
ACK 671b774d1b, tested with the following patch:
Tree-SHA512: ba010adb64900d8d748487cc1a658e2b163872354f4e7b38c4dfc37a14fcb22fec4379a635d2c6788c64dd46bef0d94aa3eb6f522ec700680e886d5468678031
This adds a specialized Cluster implementation for singleton clusters, saving
a significant amount of memory by avoiding the need for m_depgraph, m_mapping,
and m_linearization, and their overheads.
This adds 4 functions to Cluster to help implement Merge() and Split() without
needing access to the internals of the other Cluster. This is a preparation for
a follow-up that will make Clusters a virtual class whose internals are abstracted
away.
This reduces per-Cluster memory usage by making Clusters not aware of their
own level. Instead, track it either in calling code, or infer it based on
the transactions in them.
Without this change, logging (even if unused) may account for a
substantial portion of bitcoin-node's and/or client's runtime cpu usage, due
to libmultiprocess's expensive message serialization.
This (along with some recent upstream changes) avoids the overhead by opting
out of log handling for messages that we're not interested in.
Info, Warning, and Error are logged unconditionally to match our behavior
elsewhere. See BCLog::Logger::GetCategoryLogLevel .
f6567527d8 doc: bump the template macOS version (kevkevinpal)
Pull request description:
Motivated by https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3361601497
The minimum version of MacOS for this repo is now 14 and above so it makes sense to update the issue template to reflect that.
We are now using a higher version but since it is just a bug template, there is no need to put the lowest version we support.
ACKs for top commit:
maflcko:
lgtm ACK f6567527d8
l0rinc:
ACK f6567527d8
janb84:
ACK f6567527d8
Tree-SHA512: 701b161bda25245996c94b6d2119b5cc85a34917551dcf8c92ffacf3aa80fa7fe84bb3497edc7e600c5b2443de13a6f6107fc7289721e585b16c4972d07a796c
e4335a3192 Revert "depends: Update URL for `qrencode` package source tarball" (Ava Chow)
a89a822e6e Revert "depends: Use hash instead of file name for package download stamp" (Ava Chow)
Pull request description:
The new URL breaks CI on the current release branches, see https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380802351.
The old URL also no longer exists so the tarball is fetched from the depends sources cache that we host, but the original tarball has already been overwritten on there. We will need to manually reinstate the original tarball.
ACKs for top commit:
m3dwards:
utACK e4335a3192
maflcko:
review ACK e4335a3192💤
glozow:
ACK e4335a3192
Tree-SHA512: a5028342d77b4768daaec8688acd364795d683aed2bea0407c7827d44f814a97d50cc3b30c2de2a8296a2b212115fe1e76c57685a74e93387fc57afdabb93bd2
a1226bc760 doc: how to update a subtree (Sjors Provoost)
Pull request description:
We have instructions on how to verify a subtree update, but not on how to perform one.
ACKs for top commit:
yuvicc:
ACK a1226bc760
achow101:
ACK a1226bc760
janb84:
ACK a1226bc760
furszy:
ACK a1226bc760
Tree-SHA512: ba3ccc56a9f1c7f461e0db9699612e1fd64b7c72bfd1dae63d4cb830db416871a493820d3a7924c19b6ce353fc20c5fe07578b053dec6ea68273a007cbebc512
ceeb53adcd ci: Properly include $FILE_ENV in DEPENDS_HASH (Ava Chow)
Pull request description:
$FILE_ENV has a full relative path already, prepending with ci/test/ results in a non-existent path which means that DEPENDS_HASH was not actually committing to the test's environment file.
ACKs for top commit:
maflcko:
lgtm ACK ceeb53adcd
Tree-SHA512: 80a7a23676ff8bf2f48a7d3c5897217f11d7d4d4f8a54897d2b7c42689585d2d63e45fad2b8f4c442111f128a87eeb6edeac2b25c79862e6bc035eeb1ebc7f4e
b35341b9ba Update ci.yml (Coder)
Pull request description:
Release notes:https://github.com/actions/download-artifact/releases/tag/v5.0.0
Change:
uses: actions/download-artifact@v4 -> uses: actions/download-artifact@v5
ACKs for top commit:
maflcko:
lgtm ACK b35341b9ba
willcl-ark:
ACK b35341b9ba
hebasto:
ACK b35341b9ba, I have reviewed the code and it looks OK.
Tree-SHA512: f82dd0fe3ca8d431b9ff6ef9f23a4f2e92a1463c6f55fbe9b46b9e13750d311bd2aa915a8570f76600363b3a1ccbf394c95216cfac0f6db30846d9be7ec7c4cf
$FILE_ENV has a full relative path already, prepending with ci/test/
results in a non-existent path which means that DEPENDS_HASH was not
actually committing to the test's environment file.
156927903d ci: Check windows manifests for all executables (Max Edwards)
e1a1b14c93 ci: use a more generic way of finding mt.exe (Max Edwards)
7ae0497eef ci: remove 3rd party js from windows dll gha job (Max Edwards)
Pull request description:
The windows job uses the external dependency `ilammy/msvc-dev-cmd` which runs javascript. We use this to put various tools on the path such as `MSBuild.exe` and `mt.exe`. We can remove this dependency and use `vswhere.exe` directly to find these tools and create a "[Developer command prompt](https://github.com/microsoft/vswhere/wiki/Start-Developer-Command-Prompt#using-powershell)" as someone would on their dev machine.
While in this area of the code, this PR also runs some additional manifest checks on the windows binaries.
Fixes: #32508
ACKs for top commit:
davidgumberg:
crACK 156927903d
hebasto:
ACK 156927903d.
Tree-SHA512: df640dff27579a1c95daddc5a5ba8fd655bbd0a6f2aff74d0f63439c7185c0b18a90abfee3f1f032fe833cd19b822ef71812f44b24c4c044222e46d01c271864
652424ad16 test: additional test coverage for script_verify_flags (Anthony Towns)
417437eb01 script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns)
3cbbcb66ef script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns)
bddcadee82 script/verify_flags: make script_verify_flags type safe (Anthony Towns)
a5ead122fe script/interpreter: introduce script_verify_flags typename (Anthony Towns)
4577fb2b1e rpc: have getdeploymentinfo report script verify flags (Anthony Towns)
a3986935f0 validation: export GetBlockScriptFlags() (Anthony Towns)
5db8cd2d37 Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns)
Pull request description:
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](d4a86277ed/src/script/interpreter.h (L175-L195)). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo.
ACKs for top commit:
instagibbs:
reACK 652424ad16
achow101:
ACK 652424ad16
darosior:
ACK 652424ad16
theStack:
Code-review ACK 652424ad16🎏
Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f
3635d62f5a chain: make use of pskip in LastCommonAncestor (optimization) (Pieter Wuille)
2e09d66fbb tests: add unit tests for CBlockIndex::GetAncestor and LastCommonAncestor (Pieter Wuille)
Pull request description:
In theory, the `LastCommonAncestor` function in chain.cpp can take $\mathcal{O}(n)$ time, walking over the entire chain, if the forking point is very early, which could take ~milliseconds. I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.
This PR modifies the algorithm to make use of the `CBlockIndex::pskip` skip pointers to find the forking point in sublinear time (a simulation shows that for heights up to $34 \cdot 4^k - 2$ and $k \geq 8$, no more than $k^2 + 10k + 13$ steps are ever needed), in a way that should be nearly free - at worst the same number of memory accesses should be made, with a tiny increase in computation.
As it appears we didn't really have tests for this function, unit tests are added for that function as well as `CBlockIndex::GetAncestor()`.
This is inspired by https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2394877881
ACKs for top commit:
optout21:
ACK 3635d62f5a
achow101:
ACK 3635d62f5a
vasild:
ACK 3635d62f5a
mzumsande:
Code Review ACK 3635d62f5a
furszy:
ACK 3635d62f5a
stratospher:
ACK 3635d62f5a.
Tree-SHA512: f9b7dea1e34c1cc1ec1da3fb9e90c4acbf4aaf0f04768844f538201efa6b11eeeefc97b720509e78c21878977192e2c4031fd8974151667e2e756247002b8164
