💬 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

💬 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