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

🤔 ismaelsadeeq reviewed a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3180738153)
Code review ACK 235016f5b78ba9f472b56df0825690307fffc7e6 🤖

Comments are suggestion for improvement and can come in a followup

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319026367)
In "test: add is_ipc_compiled() and skip_if_no_ipc() functions" a5e470e134b8fb60264c349d8580e6fb0175b7c2

nitty-nit feel free to ignore

In commit message body
's/ function tests /functional tests /g'

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319254646)
In "tests: add functional tests for IPC interface" 592e253f76764f108e9c34fa0da32451091b946a

This should be removed since we now have `skip_if_no_py_capnp`

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319334707)
In "tests: add functional tests for IPC interface" 592e253f76764f108e9c34fa0da32451091b946a

It will be nice if we use defined constants or use define new ones for the magic numbers, or use response from rpc
It will help in improving the readability of the test.
```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 6ff11cbe0f8..912fa3a459b 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -97,18 +97,21 @@ cla
...

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319240482)
In "test: add is_ipc_compiled() and skip_if_no_ipc() functions" a5e470e134b8fb60264c349d8580e6fb0175b7c2

nit:
's/compiled/enabled/g'

💬 maflcko commented on issue "CI: Cmake warnings should be errors":
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-3249731950)
> Turning warnings into errors is the wrong approach.

The thinking behind this is that all warnings should be addressed in one way or another, otherwise there wouldn't be something to warn about.

One way to achieve this is by turning warnings into errors and then addressing the errors.

💬 m3dwards commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249732228)
Further, a lot of the YAML is actually setting things up like caching which is not only essential for reasonable performance but also is vendor specific and so couldn't really exist in reproducible CI scripts either written as they are or in cmake.

🤔 ismaelsadeeq reviewed a pull request: "build: suggest -DENABLE_IPC=OFF when missing capnp"
(https://github.com/bitcoin/bitcoin/pull/33290#pullrequestreview-3181213833)
Concept ACK

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3249777657)
> To complete migration from self-hosted to hosted for this repo, the backport branches `27.x`, `28.x` and `29.x` would also need their CI ported, but these are left for followups to this change (and pending review/changes here first).

27.x looks close to EOL in one month, according to https://bitcoincore.org/en/lifecycle/, so I guess it could just die with the old CI?

💬 fanquake commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3249782168)
Yea. I think it's fine to leave `27.x` as is, this just needs backporting to `29.x` & `28.x`.

📝 fanquake opened a pull request: "[29.x] *san CI backports"
(https://github.com/bitcoin/bitcoin/pull/33294)
Backports:
* #32999
* #33099
* #33258

💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319395069)
"enabled" feels a bit like it can change at runtime, but this is a check about whether it has been compiled in, so I think the name is correct

💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319397753)
> currently

I don't think it will ever be possible to run them with tidy, also it doesn't make sense to do?

💬 purpleKarrot commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3249812522)
Where is it defined or documented why `CMAKE_POSITION_INDEPENDENT_CODE` is set to `ON` in the first place?

A better approach would be to not override user preferences at all and set the `POSITION_INDEPENDENT_CODE` target property explicitly on targets that need it.

💬 maflcko commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3249828430)
lgtm

Should be fine either way and shouldn't matter much, because the set of devs without capnp doing fuzzing is limited