💬 hebasto commented on pull request "depends: remove xinerama extension from libxcb":
(https://github.com/bitcoin/bitcoin/pull/33217#issuecomment-3211615647)
Concept ACK.

💬 cedwies commented on pull request "doc: Remove wrong and redundant doxygen tag":
(https://github.com/bitcoin/bitcoin/pull/33236#issuecomment-3211635446)
ACK 966666d

Checked for other invalid tags with `git grep -n -E "@\[[^]]+\]" -- src | grep -v -E "@param\[(in|out|in,out)\]"`.
In `feerate.h`, removal of the param lines makes sense: no other functions in that file use `@param`, and the info was redundant with the signature.
In `coinselection.cpp` and `spend.h`, (corrected tags) (`@param[in]`, `@param[out]`) improve clarity where functions have many parameters with mixed roles.

💬 brunoerg commented on pull request "wallet: prevent accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2291837532)
fac503e7d551d1cbeb11cdb43b222f24c6de002e: Worth adding this field in the examples (`HelpExampleCli`)?

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

I also prefer throwing an error to the caller, but clamping is fine.

> Note just directly changing defaults in the .capnp file would not be backwards compatible.

For the time being I don't think we should hardcode these values in the `.capnp` file, since it's hard to change later. Instead I think we should document the capnp file, since it's the first thing future IPC implementations will look at. Maybe we can have a script that automatically
...

🤔 pablomartin4btc reviewed a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3141857359)
ACK aabf1f6093892d372544c8b5d55d260699dab69c

This is quite practical, went thru these errors.

🤔 brunoerg reviewed a pull request: "wallet: prevent accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3141879059)
I've reviewed the code and it looks great - Just a question: Instead of having this new field and prevent the import (any user have reported an accidental import?), couldn't we just throw a warning?

💬 brunoerg commented on pull request "wallet: prevent accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2291857733)
Also, it would need a release note.

💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2291867656)
```suggestion
for i, (amount_btc, _) in enumerate(TRANSACTIONS):
```

💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2291876302)
545aafb902cfe16194f90f7eb11cd5221b30015e: Do we need this `FEE_SATOSHIS`? Couldn't we use the default value from `send_to`?

💬 ryanofsky commented on pull request "miner: clamp options instead of asserting":
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3211763655)
> For the time being I don't think we should hardcode these values in the `.capnp` file, since it's hard to change later.

Makes sense, if we want flexibility to change the defaults over time, they should not be set in the .capnp file. Not setting them causes capnp to use 0 as the default.

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

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