🤔 hodlinator reviewed a pull request: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051#pullrequestreview-2892483202)
Concept ACK 93b07997e9a38523f5ab850aa32ca57983fd2552

💬 hodlinator commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r2123888732)
Is there any risk that this log message becomes spammy if we keep the peer connected? - Should we do something like reset `state.m_downloading_since` to `current_time`, clear `state.vBlocksInFlight` or add another `state`-field to throttle the log?

💬 ryanofsky commented on issue "upstream: capnp V2 doesn't support compilation with GCC (yet?)":
(https://github.com/bitcoin/bitcoin/issues/32669#issuecomment-2935513870)
Thanks for finding this! I think best source of information about the v2 branch is probably https://capnproto.org/news/#whats-planned-for-20

My impression about the branch is that it's not intending to implement many significant changes, but more to be an API break and allow cleaning up some things like promise cancellations and EOF handling that they haven't been able to fix due to wanting to preserve backwards compatibility. They are also requiring C++20 and dropping support for -fno-exceptio
...

👍 willcl-ark approved a pull request: "depends: use "mkdir -p" when installing xproto"
(https://github.com/bitcoin/bitcoin/pull/32568#pullrequestreview-2892672240)
ACK df9ebbf659d5d1282289f36d7f9ee7103aa33a17

Sorry, my guix setup was to blame here. This is now fixed and I finally get matching hashes:

```
src/core/bitcoin on  pr-32568 [$] via △ v3.31.6 via 🐍 v3.12.10 via ❄️ impure (nix-shell-env) took 1h13m34s
❯ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
c07d19f1279ac1255fff06fff094eea425aa2d228b2cf4fef8ffa366fac344dc guix-build-df9ebbf659d5/output/aarch64-linux-gnu/SHA2
...

💬 tnndbtc commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#discussion_r2124010335)
@maflcko Oh, you mean the data point I generated. I ran the original test with --randomseed=298652989016148828, which is a good randomseed, and output the calculated result of **fee**:

rand_fee = float(fee_increment) * (1.1892 ** random.randint(0, 28))

fee = min_fee - fee_increment + satoshi_round(rand_fee, rounding=ROUND_DOWN) # output this fee as the data point that can be used to generate deterministic result.

Then, for each calling of function **small_txpuzzle_randfee** , s
...

🤔 rkrux reviewed a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2892695590)
ACK 6135e05

```
git range-diff 11d0513...6135e05
```

The PR is a refactor that improves code readability and robustness upto some extent.

💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2935594462)
> Are your e2e tests running against the latest version of this branch?

The container is rebuilt every time a PR is merged, and the musig2 test never failed over the last 6+ months since they were added.

I rebuilt the container today and confirmed it works on the tagged commit for 2.4.0 release ([this](https://github.com/LedgerHQ/app-bitcoin-new/actions/runs/15418931868/job/43388642661) is the CI run).

> But when I pass a PSBT without the Bitcoin Core public nonce, it goes ahead and doe
...

💬 rkrux commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2124061046)
TIL, ty!

💬 instagibbs commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2124068316)
done

💬 instagibbs commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2124083652)
Actually, this strategy is unneeded. Both merkle checks can fail due to collisions, and either may need full blocks fetched if they're from the first peer. Pushed a change that is simpler and cleaned up READ_STATUS_CHECKBLOCK_FAILED which is no longer necessary.

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2935730736)
> If only some nonces are present, then the device assumes Round 1 already happened, but it will fail to run Round 2.

Ok, that explains issue (2).

But not why it failed with (1), because the policy variant with `older` fallback triggers the error without a public nonce from Bitcoin Core for the key path.

> The workflow I'm expecting is that the coordinator would give the same PSBT in parallel to all the signers, which execute round 1, then all nonces are added to the PSBT, and eah signe
...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2124158984)
@stickies-v, my mistake, I quickly wanted to make sure this doesn't affect my IBD benchmarks, didn't have time to review it in detail yet

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2124163227)
Thanks for clarifying

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2124177620)
> Same here?

Please see https://github.com/include-what-you-use/include-what-you-use/issues/1764.

💬 theStack commented on pull request "[Draft/POC] Add secp256k1-based HPKE (Hybrid Public Key Encryption) For Payjoin v2":
(https://github.com/bitcoin/bitcoin/pull/32617#issuecomment-2935923079)
Fore more context to reviewers, could add a link to [BIP 77](https://github.com/bitcoin/bips/blob/master/bip-0077.md) in the PR description, particularly to the cryptography part: https://github.com/bitcoin/bips/blob/master/bip-0077.md#secp256k1-hybrid-public-key-encryption

> Full HPKE Modes: All four HPKE modes are supported – Base, PSK, Auth, and AuthPSK.

Do we need all of the four modes for Payjoin v2 support? Didn't look in-depth yet, but at least BIP77 contains only the two modes "Bas
...

💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2935928425)
> But not why it failed with (1), because the policy variant with `older` fallback triggers the error without a public nonce from Bitcoin Core for the key path.

Please feel free to [open an issue](https://github.com/LedgerHQ/app-bitcoin-new/issues) and I'll investigate.

> > The workflow I'm expecting is that the coordinator would give the same PSBT in parallel to all the signers, which execute round 1, then all nonces are added to the PSBT, and eah signer is invoked again and produces the
...

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2936000601)
> > But not why it failed with (1), because the policy variant with `older` fallback triggers the error without a public nonce from Bitcoin Core for the key path.
>
> Please feel free to [open an issue](https://github.com/LedgerHQ/app-bitcoin-new/issues) and I'll investigate.

I'll test it once more and will open an issue if needed.

> Verifying if 'my nonce is present' is a substantial amount of work, while checking if 'some nonces' are present is trivial by iterating once through the PS
...

👍 l0rinc approved a pull request: "clang-tidy: Apply modernize-deprecated-headers"
(https://github.com/bitcoin/bitcoin/pull/32673#pullrequestreview-2892956742)
ACK facb152697b8d7b75a9e6108f8896f774b06b35f

I like that it's more consistent now, thanks for fixing it.
Since a few header names have changed now, the only suggestion I have is to keep the simplest ordering that would minimize merge and review conflicts in the future, e.g. a new commit on top with alphabetical ordering (or removal) where we also usually separate internal and external dependencies:

<details>
<summary>Suggested sorting</summary>

```
diff --git a/src/bech32.h b/src/bec
...

💬 l0rinc commented on pull request "clang-tidy: Apply modernize-deprecated-headers":
(https://github.com/bitcoin/bitcoin/pull/32673#discussion_r2124179503)
should we re-sort these after the change?

💬 l0rinc commented on pull request "clang-tidy: Apply modernize-deprecated-headers":
(https://github.com/bitcoin/bitcoin/pull/32673#discussion_r2124186698)
this should likely go the the bottom include stack now, right?