💬 brunoerg commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2291906741)
3d8a1895fcbc6802e1692fd57a4f63a948fda302: nit: Since we don't have legacy wallet anymore, I'm not sure how effective is this comment.

🤔 brunoerg reviewed a pull request: "wallet, refactor: Remove Legacy check and error"
(https://github.com/bitcoin/bitcoin/pull/33082#pullrequestreview-3141968423)
code review ACK 3d8a1895fcbc6802e1692fd57a4f63a948fda302

- Checked that the unecessary calls to `LoadWallet` were removed from the tests - I didn't find any other occurance apart of `TestLoadWallet`.

💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3211783052)
@achow101:

> I generally disagree with the approach in this PR. It's doing a lot of refactoring that feels too much like refactoring for the sake of refactoring. I think this can be dropped from the milestone.

### Commit order

**I understand that given the current order of commits, the test refactorings seem less motivated.** Order used to be refactorings + main commit + illustrative test + additional log test (https://github.com/bitcoin/bitcoin/compare/37405238a8c4fe4dff82781c91fb66839
...

💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3211797289)
(As I said on IRC, I respect the decision to drop it from the milestone. There's enough moving parts in the release process. It would've been neat to fix ahead of bumping `REDOWNLOAD_BUFFER_SIZE`, the tests on master will not fail when it happens).

💬 hebasto commented on pull request "depends: remove xinerama extension from libxcb":
(https://github.com/bitcoin/bitcoin/pull/33217#issuecomment-3211875540)
Here are related Qt issues and commits:
- [QTBUG-69411](https://bugreports.qt.io/browse/QTBUG-69411)
- [QTBUG-86082](https://bugreports.qt.io/browse/QTBUG-86082)
- https://github.com/qt/qtbase/commit/c91d1fdc100cda88b94217153def52ab7fe63d21

🤔 hebasto reviewed a pull request: "depends: remove xinerama extension from libxcb"
(https://github.com/bitcoin/bitcoin/pull/33217#pullrequestreview-3142128074)
My Guix builds for Linux hosts:
```
aarch64
591acd2a1f17197ebf8da6c347cb976d5cf1faae174722e00bca5e54596dd8a9 guix-build-decc3671c88b/output/aarch64-linux-gnu/SHA256SUMS.part
f8075cbb6f4c09673aad64c79793c97f47209ea3b8b695992cfd8b16e98e3ff6 guix-build-decc3671c88b/output/aarch64-linux-gnu/bitcoin-decc3671c88b-aarch64-linux-gnu-debug.tar.gz
dc45ee866d0848bdb19dfbae47f591d78bf552604b866e521fe8dbc7acd169e1 guix-build-decc3671c88b/output/aarch64-linux-gnu/bitcoin-decc3671c88b-aarch64-linux-gnu
...

👍 hebasto approved a pull request: "depends: remove xinerama extension from libxcb"
(https://github.com/bitcoin/bitcoin/pull/33217#pullrequestreview-3142184801)
ACK decc3671c88bb3acdf137c6bc46892f99319104e.

This PR doesn’t affect the `bitcoin-qt` executable, since `libxcb-xinerama.so` isn’t used even in the current master branch @ 7d9789401be4c91a9eb3c1112564a6524bdc4f70.

🤔 w0xlt reviewed a pull request: "doc: Remove wrong and redundant doxygen tag"
(https://github.com/bitcoin/bitcoin/pull/33236#pullrequestreview-3142195080)
ACK https://github.com/bitcoin/bitcoin/pull/33236/commits/966666de9a6211b8748f43d682490c924e132e58

💬 achow101 commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#issuecomment-3212021123)
ACK a602f6fb7bf5f9e57299f4d6e246c82379fad8d2

🤔 jonatack reviewed a pull request: "IPC followups for PR 31802"
(https://github.com/bitcoin/bitcoin/pull/33233#pullrequestreview-3142237551)
LGTM, though suggest renaming the PR title to

`doc: follow-ups to "Add bitcoin-{node,gui} to release binaries for IPC"`

and adding #31802 to the description.

💬 achow101 commented on pull request "miner: clamp options instead of asserting":
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3212033066)
ACK 7392b8b084be14b75536887b7ff216152d0a3307

🚀 achow101 merged a pull request: "miner: clamp options instead of asserting"
(https://github.com/bitcoin/bitcoin/pull/33222)

✅ achow101 closed an issue: "spendable is true for UTXO of private key disabled wallet"
(https://github.com/bitcoin/bitcoin/issues/33110)

💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2292382365)
Ok, I'll remove it.

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3212632223)
Given that the current PR already demonstrates the production code is still correct, I tend to agree that we don't have to rush. Would have been cleaner, but it's not urgent.

I do think however that the refactorings are necessary - but I'd keep the commits before the final fix, and if other reviewers think it's too risky, maybe we can do it in a separate PR - unifying it with @danielabrozzoni's change and your other related proposal either through a tracking issue or pushing the rest as draft
...

💬 ajtowns commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3212871951)
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442

Unless there's an explicit schedule for the removal (eg `-paytxfee is deprecated and will be fully removed in v31.0`), this phrasing seems more accurate. Probably the testnet3 deprecation should also either be scheduled or changed to "is expected to be removed" as well.

👍 pablomartin4btc approved a pull request: "Fix compatibility with `-debuglogfile` command-line option"
(https://github.com/bitcoin-core/gui/pull/884#pullrequestreview-3143078214)
ACK c0d28c8f5b150a03de75155a0961b3d9b2695ed6

`LogInstance().m_file_path` [is being set](https://github.com/bitcoin/bitcoin/blob/78351ed083b1113091d42d3dbb173d2200fbcc4b/src/init/common.cpp#L49) in `init::SetLoggingOptions(args)`.

Need to restart some CIs (few were cancelled when PR got closed)?

💬 Sjors commented on pull request "miner: clamp options instead of asserting":
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3213170422)
> However, in case 0 is a valid value for either of these options, we may want to set a different, invalid default value like -1 so the server can detect whether the client set the option, and use its own default if the client hasn't filled it in.

In that case perhaps it's better if we do hardcode defaults, especially if we can pick safe ones.

💬 Sjors commented on pull request "doc: follow-ups to "Add bitcoin-{node,gui} to release binaries for IPC"":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3213172304)
@jonatack done, although I wasn't expecting there to be only doc changes.

💬 0xB10C commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2292858152)
nit:
```suggestion
uses: actions/checkout@v5
```
See https://github.com/bitcoin/bitcoin/pull/33171