👍 ryanofsky approved a pull request: "miner: clamp options instead of asserting"
(https://github.com/bitcoin/bitcoin/pull/33222#pullrequestreview-3141107720)
Code review ACK 7392b8b084be14b75536887b7ff216152d0a3307. I think ideally this would throw an exception and return a clear error to the caller, or maybe log as stickies suggested, but clamping is much better than crashing.

👍 pablomartin4btc approved a pull request: "doc: Remove wrong and redundant doxygen tag"
(https://github.com/bitcoin/bitcoin/pull/33236#pullrequestreview-3141191001)
ACK 966666de9a6211b8748f43d682490c924e132e58

Checked that there aren't more pending corrections left for `@param` (and for others by looking for "`@[`").

The corrections match the specification in the doxygen-compatible comments [section](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-doxygen-compatible-comments) on the dev-notes and in doxygen itself on the [param command](https://www.doxygen.nl/manual/commands.html#cmdparam).

💬 maflcko commented on issue "depends: `native_libmultiprocess` fails to build on OpenBSD":
(https://github.com/bitcoin/bitcoin/issues/33219#issuecomment-3211086241)
upstream fix (from the previous comment) is at https://github.com/bitcoin-core/libmultiprocess/pull/195

💬 ryanofsky commented on pull request "miner: clamp options instead of asserting":
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3211088504)
@Sjors maybe you have thoughts on this?

It would probably be good if we set default field values in the capnp file:

https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/ipc/capnp/mining.capnp#L40-L41

that match the c++ defaults:

https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/node/types.h#L40-L49

Note just directly changing defaults in the .capnp file would not be backwards compatible. It would cause problems if
...

💬 mzumsande commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#discussion_r2291461327)
> After the kill, we jump back exactly 2 blocks to `start_height`. I guess this is due to the clean shutdown in `restart_node()` on line 319 in the preceding test code above having fully committed that state to disk.

I think the previous `gettxoutsetinfo` (which flushes the chainstate) is the last time. But I added a restart, so that the added test doen't depend on the previous subtests. I also added a comment / did the rename.

💬 mzumsande commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#issuecomment-3211161834)
[4625b4b](https://github.com/bitcoin/bitcoin/commit/4625b4b8f772de1cd852003645c75bfa68c6735a) to [a602f6f](https://github.com/bitcoin/bitcoin/commit/a602f6fb7bf5f9e57299f4d6e246c82379fad8d2): small changes to the test

💬 BenWestgate commented on pull request "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32":
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3211192805)
> This could still be implemented—even as part of this PR.
>
> The wallet generates the seed, stores it, and then derives the BIP32 master key by deterministically combining the seed with the user-provided passphrase. This approach allows users to back up the raw seed while still requiring the passphrase for wallet recovery.
>
> Or am I missing something ?

Not part of the BIP-0093 spec.

If a passphrase is used it becomes a nested 2 of 2(passphrase, codex32 secret) causing funds loss
...

💬 fanquake commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211193252)
You'll also need to add the `capnp` install instructions for Alpine and Arch in `build-unix.md`.

💬 BenWestgate commented on pull request "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32":
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3211231715)
> It seems to me that codex32 only plays a role in (1). And so importing it should be done with `addhdkey`. Exporting could be done with `gethdkeys`.

It would be a usability and compatibility improvement to begin exporting `hdseed` in the codex32 secret format as:

1. Easier to type, write and speak than Base58.
2. Error correcting, hand-verifiable checksum
3. Easier to extract the payload seed bytes without needing base58 conversions.

The 4 character identifier when exporting `hdseed`
...

💬 achow101 commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3211267309)
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.

💬 Sjors commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211285558)
Arch instruction taken from: https://capnproto.org/install.html

For Alpine I added `capnproto` and `capnproto-dev` since they both exist, but didn't test if the latter is needed.

💬 maflcko commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211316649)
> For Alpine I added `capnproto` and `capnproto-dev` since they both exist, but didn't test if the latter is needed.

dev installs the other: https://cirrus-ci.com/task/6500081345495040?logs=install#L0

💬 maflcko commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211318865)
lgtm ACK de65c86572c5ca96e388a444aae9aa73bbd8860a

🤔 janb84 reviewed a pull request: "IPC followups for PR 31802"
(https://github.com/bitcoin/bitcoin/pull/33233#pullrequestreview-3141476594)
re ACK de65c86572c5ca96e388a444aae9aa73bbd8860a

💬 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