💬 ryanofsky commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3211413482)
Code review ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442. I don't think it is good for the release notes and the runtime warning message to say two different things. I'd also be happy if release notes were updated to match the runtime warning, instead of vice versa. Whatever is more accurate is better.

💬 cedwies commented on pull request "coinselection: Tiebreak SRD eviction by weight":
(https://github.com/bitcoin/bitcoin/pull/33223#issuecomment-3211450233)
ACK 0dee81b

I ran the SRD tests, all pass as expected. To validate the comparator change, I also re-ran the fourth SRD test using the old comparator, and it fails (as expected). This confirms that the test setup is effective at catching the bug.
The new comparator correctly enforces the weight limit, and the test demonstrates that the previous implementation of the comparator can only succeed by chance (~1 in a billion). Looks good to me.

🤔 l0rinc requested changes to a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3141565411)
Concept ACK, but approach NACK 132a59698e2df2e25fdbae3a6fba308b131419d2

I only did a code review pass on it, please let me know if my concerns are off, happy to ack if I was mistaken.
My biggest concern currently is silently dropping values by partial duplication, e.g. `-whitebind=[::1]:8335=relay -whitebind=[::1]:8335=relay,addr`.

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2291660042)
isn't it confusing that in case of service duplicates the it's not obvious which `m_flags` is ignored?

What do we expect to happen in case of something like `-whitebind=127.0.0.1:8335=relay,noban -whitebind=127.0.0.1:8335=relay` vs `-whitebind=127.0.0.1:8335=relay -whitebind=127.0.0.1:8335=relay,noban`?
Or even worse, `-whitebind=127.0.0.1:8335=all -whitebind=127.0.0.1:8335=relay,mempool`?

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2291649231)
* I'd appreciate a test that checks what happens in this case, as @jonatack also mentioned
* a release‑notes line under P2P noting that repeated -bind and -whitebind arguments are now deduplicated

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2291645849)
the `v` prefix doesn't make a lot of sense anymore, if they're not vectors

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