🤔 glozow reviewed a pull request: "[29.x] finalise v29.1"
(https://github.com/bitcoin/bitcoin/pull/33271#pullrequestreview-3180870194)
ACK 084c95a18c9978c0a047ffe219a9eef8ab327ea6

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

This all looks correct to me

💬 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
...