💬 ryanofsky commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249470528)
This is a nice idea and approach seems ok if it works in practice. I think a more general approach might to have the bitcoin build set a variable the libmultiprocess build can use to show better error messages like:

```
set(MP_SUBPROJECT_ERROR "Configure cmake with with -DENABLE_IPC=OFF if you do not need IPC functionality.")
```

It could append this message if it can't find the cap'n proto package, or doesn't find a compatible version (for https://github.com/bitcoin-core/libmultiproces
...

⚠️ maflcko opened an issue: "ci: GHA fallback centos task runs out of space"
(https://github.com/bitcoin/bitcoin/issues/33293)
https://github.com/bitcoin-core/gui/actions/runs/17433074590/job/49495964258?pr=884#step:8:3785

```
...
+ eval 'TEST_RUNNER_EXTRA=()'
++ TEST_RUNNER_EXTRA=()
+ LD_LIBRARY_PATH=/home/runner/work/_temp/depends/x86_64-pc-linux-gnu/lib
+ /home/runner/work/_temp/build/test/functional/test_runner.py --ci -j4 --tmpdirprefix /home/runner/work/_temp/ci/scratch/test_runner/ --ansi --combinedlogslen=99999999 --timeout-factor=40 --quiet --failfast
WARNING! There may be insufficient free space in /home/runn
...

💬 maflcko commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3249499925)
It may be possible to free up some space with something like https://github.com/google/oss-fuzz/blob/14cab84abbc736e24fede3f0267a8e220fb4974e/.github/workflows/project_tests.yml#L65-L74

💬 maflcko commented on pull request "Fix compatibility with `-debuglogfile` command-line option":
(https://github.com/bitcoin-core/gui/pull/884#issuecomment-3249501476)
(ci failure can be ignored, see https://github.com/bitcoin/bitcoin/issues/33293)

💬 willcl-ark commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3249508043)
```
Free disk space:
+ [[ ci_native_centos == \c\i\_\n\a\t\i\v\e\_\a\s\a\n ]]
++ /home/runner/work/_temp/depends/config.guess
Filesystem Size Used Avail Use% Mounted on
overlay 72G 48G 25G 67% /
tmpfs 64M 0 64M 0% /dev
shm 64M 0 64M 0% /dev/shm
/dev/root 72G 48G 25G 67% /etc/hosts
tmpfs 7.9G 0 7.9G 0% /proc/acpi
tmpfs 7.9G 0 7.9G 0% /proc/scsi
tmpfs 7.9G 0 7.9G 0% /sys/firmw
...

🤔 theStack reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3180916078)
Reviewed up to 38d74136c6552ae746bec6e05b8ec3069cd581ce, left a few non-blocking suggestions below.

I think the order of commits d65c8df972de55ade06557cabb1b8972d4169fb1 ... 38d74136c6552ae746bec6e05b8ec3069cd581ce is currently slightly confusing for reviewers, as it doesn't reflect the protocol flow, i.e. the signature aggregation function `CreateMuSig2AggregateSig` should ideally be introduced _after_ the partial signature creation function `CreateMuSig2PartialSig`.

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319159859)
in commit a7901710a6b222cbdeba474bbe8b78788841e58a: it's only a few lines of code, but to deduplicate with the next commit (introducing `CreateMuSig2PartialSig`), could introduce a helper function like `MuSig2SessionId`

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319145295)
in commit d65c8df972de55ade06557cabb1b8972d4169fb1: naming nit: `participants` doesn't say much imho, could rename to `participant_pubkeys` (or just `part_pubkeys` / `pubkeys`, if that's too long) to be more specific

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319173890)
in 38d74136c6552ae746bec6e05b8ec3069cd581ce: if `our_pubkey` is not contained in the `pubkeys` list, I suppose we want to return early with `std::nullopt`, rather than accessing at index 0?

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319170915)
in commit 38d74136c6552ae746bec6e05b8ec3069cd581ce: nit, could rename to something like `our_pubkey_index` to be more expressive

💬 ryanofsky commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3249543838)
It seems like there's not much downside to this change, other than that it may need to be reverted later if an IPC fuzz test is added. And I think if a fuzz test is added we would also want to revert https://github.com/google/oss-fuzz/pull/13854?

Only Sjors above https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210571615 expressed a preference for not making this change and maflcko https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210147334 raised an early question about
...

💬 Sjors commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249574033)
@ryanofsky I like your approach better since it can be used under more circumstances, such as not having the right version. But we're close to branch-off so maybe this approach suffices for now.

💬 Sjors commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3249576984)
I'm fine with this change if no fuzz PR is merged before branch-off.

🚀 glozow merged a pull request: "[29.x] finalise v29.1"
(https://github.com/bitcoin/bitcoin/pull/33271)

💬 purpleKarrot commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249588115)
> I quite like the idea of all of CI being defined as Github Actions

I prefer the idea that all CI is decoupled from the project, and that both the project and the CI are implemented against a defined interface: the cmake project definition. I presented this approach at C++Now 2025 as [Effective CTest](https://schedule.cppnow.org/session/2025/effective-ctest/).

Defining CI in a YAML file that is part of the project essentially states "it works on my machine(s)". The project will start to suppo
...

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3249601891)
Concept ACK ae645ef858702ff85dab4b1ac6296d53b573c81b, started reviewing.

Maybe the PR description could be updated to remove the following references as their detailed implementations were done in prior PRs.
> This PR implements MuSig2 descriptors (BIP 390), derivation (BIP 328), and PSBT fields (BIP 373)

👍 stickies-v approved a pull request: "[29.x] finalise v29.1"
(https://github.com/bitcoin/bitcoin/pull/33271#pullrequestreview-3181043104)
ACK 084c95a18c9978c0a047ffe219a9eef8ab327ea6

Backport commits aren't clean, but the changes lgtm:
- 6448ebb5a7c942949a70ffc4a1d2a93338fac130 backported from 966666de9a6211b8748f43d682490c924e132e58: merge-conflict because d3b8a54a81209420ef6447dd4581e1b6b8550647 changed the `CFeeRate` docstring
- 99ab2e70e782bf5ca753ad636f69642da6054283 backported from 509ffea40abbc706ef8b8fc449b7de8677fc5096: merge-conflict because of the added `ninja-build` from 30dd1f1644e0441b5310f1eceecfd6a5abc45f68

...

💬 stickies-v commented on pull request "[29.x] finalise v29.1":
(https://github.com/bitcoin/bitcoin/pull/33271#discussion_r2319230965)
nit: it seems we don't have consistent manner of labeling GUI PRs, with a quick grep showing existing approaches like:
- `#<gui-pr>`
- `gui#<gui-pr>`
- `#gui<gui-pr>`
- `bitcoin-core/gui#<gui-pr>`

No strong view, but perhaps using `gui#<gui-pr>` here would make sense, since it probably confuses people not familiar with our dual-repo setup:

```
### Gui

- gui#864 Crash fix, disconnect numBlocksChanged() signal during shutdown
- gui#868 Replace stray tfm::format to cerr with qWarning
...

💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249719463)
> Defining CI in a YAML file that is part of the project essentially states "it works on my machine(s)". The project will start to support variables that are defined in the CI configuration only and it will derive from the default cmake workflow (example: running ctest directly will fail and a special script or a custom target is required to run all tests).

I think this is a pre-existing problem. It was never possible to run all tests (fuzz, functional, lint) from the build system (autotools, c
...

💬 purpleKarrot commented on issue "CI: Cmake warnings should be errors":
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-3249720985)
> It is brittle to silently ignore Cmake warnings in the CI.

I think the desire to turn warnings into errors on CI (both configure warnings and build warnings) is due to the fact that CI does not perform any analysis of the build output. If the build result was displayed on a [dashboard](https://open.cdash.org/index.php?project=CMake), you would not want to turn warnings into errors, because it would take away useful statistics.
Turning warnings into errors is the wrong approach. Consider how t
...