💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2291661657)
We could modernize this to C++20
```C++
std::weak_ordering operator<=>(NetWhitebindPermissions const& other) const
```

nit: formatting is off

💬 brunoerg commented on pull request "coinselection: Optimize BnB exploration":
(https://github.com/bitcoin/bitcoin/pull/32150#discussion_r2291715313)
In 7596e491563d98aab10e9e0baad4850a6197844a: FYI my mutation testing bot reported the following mutant as not killed:

```diff
@@ -201,7 +201,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
should_shift = true;
// The amount exceeding the selection_target (the "excess"), would be dropped to the fees: it is waste.
CAmount curr_excess = curr_amount - selection_target;
- CAmount curr_waste = curr_selecti
...

💬 l0rinc commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#issuecomment-3211515455)
Not everyone knows what "33106" is. To avoid it being interpreted as trying to hide something, could you please detail the changes in the PR title and description to help the reviewers and provide full transparency?

👍 hodlinator approved a pull request: "doc: unify `datacarriersize` warning with release notes"
(https://github.com/bitcoin/bitcoin/pull/33224#pullrequestreview-3141743326)
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442

Makes wording consistent with release notes (end of line):

https://github.com/bitcoin/bitcoin/blob/f5f853d952542ebd45339a270a98362696877657/doc/release-notes-32406.md?plain=1#L1

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