💬 maflcko commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123310144)
No opinion from my side. I think anything is fine here

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123324973)
Doesn't appear to be turned off on our CI and I think it should be.

In Powershell 7.4 and up there is now a setting `PSNativeCommandUseErrorActionPreference` which should make calling native commands respect the `ErrorActionPreference` so we should set both of those, globally if possible. I will look into it.

💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2934471353)
> The tidy job is failing because it doesn't like the logging macros being used in lambda functions. It seems like this is pre-existing so I'm not sure why it's failing now.

It happens after commit dfbc3e46b8661b112a91c5f00a4dce439f4f5914 and looks unrelated. Maybe just use the workaround from ./src/util/check.h?

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123353856)
I'm wrong about powershell cmdlets: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference

Github sets `ErrorActionPreference` to `stop` but it doesn't appear that they set `PSNativeCommandUseErrorActionPreference` which is why calling native executables (like mt.exe) can still fail silently.

💬 fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2934494918)
> nit: can drop "RFC:" from the second commit?

Dropped.

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2934495128)
Concept ACK, but I'm not yet sure about the approach. Let me know if I misunderstood something important here.

💬 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_r2123334010)
Do we ever clear `m_ratelimiters`, or does that map only grow over the lifetime of the process?

💬 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_r2123314635)
I'm not sure introducing two extra category flags is a good abstraction here - feels noisy.
Wouldn’t it be cleaner to decide based on exposure to the attack surface instead? For example, during IBD (or any state with zero peer connections) we could leave all logs unrestricted, and always enable rate-limiting when node is reachable by peers.

💬 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_r2123243686)
When I’m benchmarking IBD, I don’t want any artificial size or rate limits.
Since the attack only matters when peers can influence us, could we simply disable rate-limiting for all logs during IBD/reindexes/assumeutxo?

------

Should the docs explicitly state the 1 MiB-per-hour cap, or could we replace the boolean flag with a numeric “MiB per hour” parameter (retaining 0 and 1 as the current meanings)?

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123366159)
> > Default behaviour in powershell is to continue on error
>
> Do we have that turned off for all of our CIs?

We did: https://github.com/bitcoin/bitcoin/blob/e872a566f251c73908de8b6d243c94a6679c2eac/.github/workflows/ci.yml#L24-L28

💬 fanquake commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2123366909)
> Since the attack only matters when peers can influence us, could we simply disable rate-limiting for all logs during IBD

?

💬 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-2934543453)
It fails at this step, in the `generate_public_nonces` function:

```
print("\n🐮 Requesting pubnonce (Round 1)")
signer_1.generate_public_nonces(psbt)
```

Just in case, I restarted the signing session by having Bitcoin Core make a fresh `musig2_pubnonces` for its key.

Are your e2e tests running against the latest version of this branch?

🤔 rkrux reviewed a pull request: "rpc: Use type-safe HelpException"
(https://github.com/bitcoin/bitcoin/pull/32660#pullrequestreview-2891673768)
ACK fa0cf42380dec17e73d38aa69dd70662a0bc344a

<details>
<summary>
Diff for testing
</summary>

```diff
➜ bitcoin git:(2506-rpc-help) ✗ git diff --color | cat
diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp
index 5597f8d237..18af6d110e 100644
--- a/src/rpc/signmessage.cpp
+++ b/src/rpc/signmessage.cpp
@@ -17,7 +17,7 @@
static RPCHelpMan verifymessage()
{
return RPCHelpMan{"verifymessage",
- "Verify a signed message.",
+ "\nVerify a signed mes
...

💬 rkrux commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123375103)
Ah! Took me some time to understand how this fix worked. In case of erroneous help text, a `NonFatalCheckError` exception is thrown from here that was earlier caught by the catch-all handler in `server.cpp`. Now only `HelpException` is caught and the rest, including `NonFatalCheckError`, is bubbled up all the way to the RPC response. https://github.com/bitcoin/bitcoin/blob/e872a566f251c73908de8b6d243c94a6679c2eac/src/rpc/util.cpp#L795-L796

💬 rkrux commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123391836)
Nit: Maybe returning the result by throwing an exception is obvious to others but it wasn't to me. Does it necessarily need to be called `HelpException`? Calling it `HelpResult` might make it more explicit or `HelpResultException` (not my preference).

<details>
<summary>
Diff
</summary>

```diff
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index d5c790e89b..bd99615bbf 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -96,7 +96,7 @@ std::string CRPCTable::help(const
...

💬 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_r2123404055)
Can you be more specific :)?

💬 fanquake commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2934599327)
Guix Build:
```bash
9607cec72e511df72d1d07ef850015172d6c2707604f46694c019cd37903c65b guix-build-74dfa4155037/output/aarch64-linux-gnu/SHA256SUMS.part
30916e57efaa2bfd4265d840cde7049c863c4d35cec9d5e70681e7bc428e2d14 guix-build-74dfa4155037/output/aarch64-linux-gnu/bitcoin-74dfa4155037-aarch64-linux-gnu-debug.tar.gz
c462783e946c5e3e61a6a4ab4be1b8ddb6db9be07194d3241b43a8cc4cbe81f7 guix-build-74dfa4155037/output/aarch64-linux-gnu/bitcoin-74dfa4155037-aarch64-linux-gnu.tar.gz
23b60343ce14152f
...

💬 0xB10C commented on pull request "test: add skip_if_running_under_valgrind()":
(https://github.com/bitcoin/bitcoin/pull/32492#issuecomment-2934603491)
LGTM post-merge ACK

💬 maflcko commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123414365)
thx, I may change to `HelpResult`, if I have to re-touch for other reasons.

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123414640)
@hebasto but there are a few places where we use pwsh, which will still fail fast if using exclusively cmdlets but doesn't appear to fail when calling native executables.

Here is a commit to demonstrate it won't always failfast: https://github.com/bitcoin/bitcoin/commit/4bf10310fb4ec4d6c2f479f008c487cca1f90c27

and the job run that didn't fail: https://github.com/m3dwards/bitcoin/actions/runs/15415032095/job/43375709475

Suggest we set `PSNativeCommandUseErrorActionPreference` to `true` e
...