🤔 rkrux reviewed a pull request: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-2996982345)
I believe that the PR title and more importantly the PR description should be updated to remove the stuff related to watch-only as that code removal was done in another PR. The last paragraph of the description can be kept.

💬 rkrux commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2192124748)
Dropped the commit.

📝 Sjors opened a pull request: "test: use notarized v28.2 binaries and fix macOS detection"
(https://github.com/bitcoin/bitcoin/pull/32922)
Since https://github.com/bitcoin/bitcoin/pull/31407 guix builds are signed and notarized. This was included in v29 and backported to v28.

This PR bumps the v28.0 previous release binary to v28.2 and adjust the test that uses it. Additionally it no longer manually code signs binaries >= v28.2.

While testing on an M4 mac and redownloading all the binaries, I noticed that `platform == "arm64-apple-darwin"` doesn't actually work. So the first commit switches this to use `platform.system().lowe
...

💬 Sjors commented on pull request "test: less ambiguous error if bitcoind is missing":
(https://github.com/bitcoin/bitcoin/pull/32921#discussion_r2192218937)
Done

💬 Sjors commented on pull request "test: less ambiguous error if bitcoind is missing":
(https://github.com/bitcoin/bitcoin/pull/32921#issuecomment-3048496358)
If you're testing this on Apple Silicon, you may also need #32922.

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2192233867)
Renamed to `PSBTFillOptions`

💬 maflcko commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192252386)
the code is still wrong. Usually, it doesn't make sense to compare two values of completely unrelated types and a type-safe language would prevent this class of issues.

Also, have you tested this code branch?

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192291415)
Oh oops, I did test it, but then broke it when switching the commit order.

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#issuecomment-3048632834)
cc @kdmukai

💬 Sjors commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2192333715)
1a1b478ca31be1f670754a47da17863271e46b7b: missed one occurance of `platform`, see #32922

💬 Sjors commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#issuecomment-3048672022)
67a6b20d5030ee51bf6d5e0fd77c9bdc8bafa96b missed one occurrence of `platform` which broke code-signing on Apple Silicon. You'd only notice that if you delete previously downloaded and already signed binaries. Fixed in #32922.

👍 dergoegge approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2997492533)
Code review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9

👍 ryanofsky approved a pull request: "cmake: Use `AUTHOR_WARNING` for warnings"
(https://github.com/bitcoin/bitcoin/pull/32865#pullrequestreview-2997583190)
Code review ACK bf8bf7408c30b54d953c2b983a66c693e5eb6f21.

IIUC, the last 3 commits passing `-Werror=dev` to cmake in CI, guix, and developer presents seem like clearly good changes, because they turn cmake internal warnings and `message(AUTHOR_WARNING "...")` statements into errors which stop the build so they can't be ignored.

The other commit 02955f3af3bcdc9d978784cad6a98badf5d1117b is changing our current `message(WARNING "...")` statements into `message(AUTHOR_WARNING "...")` statement
...

💬 ryanofsky commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3048932121)
> Not sure I understand your NACK. The warnings will still be shown to builders; using `AUTHOR_WARNING` just gives us a proper mechanism to turn warnings into errors.

I think luke's concern might be that author warnings can be suppressed with `-Wno-dev` and might be suppressed by default in certain environments, since author warnings are only supposed to be relevant to project maintainers, not to users.

In 02955f3af3bcdc9d978784cad6a98badf5d1117b we are turning normal warnings about ignore
...

✅ ryanofsky closed a pull request: "POC: IPC tracing interface"
(https://github.com/bitcoin/bitcoin/pull/32898)

💬 ryanofsky commented on pull request "POC: IPC tracing interface":
(https://github.com/bitcoin/bitcoin/pull/32898#issuecomment-3048947083)
> Awesome, thanks! I'll play around with this over the next few weeks.

Great! Will close this PR since I'm not planning to do more work on it, and we can continue to discuss in https://github.com/bitcoin-core/libmultiprocess/issues/185

💬 furszy commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3048985280)
> > I don't think the test is useful.
> > Other than that, concept ACK.
>
> You can ignore it.

Maybe give another look at the test code before saying that? https://github.com/bitcoin/bitcoin/commit/21ade546808fd757bf451db74cea1da3755f6613

The test isn't testing the indexes code — just its own test-only implementation of it. It was essentially testing itself.

🤔 furszy reviewed a pull request: "wallet: remove dead code in legacy wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32758#pullrequestreview-2997688329)
ACK 150b5c99ca11885ef15d04139d919d734e2c211a

⚠️ fanquake opened an issue: "guix: Windows build is non-deterministic across build architectures"
(https://github.com/bitcoin/bitcoin/issues/32923)
This has been the case since https://github.com/bitcoin/bitcoin/commit/8578fabb95face25d9fa7dfaad38d0a2857c2481.

Guix build on `x86_64` & `aarch64` @ 01f908195589 (prior merge):
```bash
0324606fdfc525e2acf6ffc5b5327021df6b47de107dbeb51f54162a3dead570 guix-build-01f908195589/output/dist-archive/bitcoin-01f908195589.tar.gz
e9be0c0c6ab0d856c2176ff0da9c4fb3599f8bd0d578138262fde7307691c28b guix-build-01f908195589/output/x86_64-w64-mingw32/SHA256SUMS.part
b27afe43704fa3045f1acebd967f282781f5d1907b4
...

💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3049041023)
The non-determinism issue is not being introduced with this change, it's already present in master, see #32923. Given that, It'd be good to move ahead here, so we can finished up the backports for `29.1`.