🤔 rkrux reviewed a pull request: "rpc: Note in fundrawtransaction doc, fee rate is for package"
(https://github.com/bitcoin/bitcoin/pull/32607#pullrequestreview-2891529215)
ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
(https://github.com/bitcoin/bitcoin/pull/32607#pullrequestreview-2891529215)
ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123283423)
Default behaviour in powershell is to continue on error. When calling a powershell cmdlet you can set `$ErrorActionPreference = 'Stop'` but being as this is a call to an external process I don't believe that approach works here, hence why checking the exit code.
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123283423)
Default behaviour in powershell is to continue on error. When calling a powershell cmdlet you can set `$ErrorActionPreference = 'Stop'` but being as this is a call to an external process I don't believe that approach works here, hence why checking the exit code.
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123286507)
Which now worries me about elsewhere we have used powershell, if it will exit like we think it should.
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123286507)
Which now worries me about elsewhere we have used powershell, if it will exit like we think it should.
💬 maflcko commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123287437)
I am just using vanilla clang-format, but if there is an interaction issue with an IDE, it could make sense to set AfterStruct to false or true explicitly. Happy to review such a pull.
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123287437)
I am just using vanilla clang-format, but if there is an interaction issue with an IDE, it could make sense to set AfterStruct to false or true explicitly. Happy to review such a pull.
💬 fanquake commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123291372)
> Default behaviour in powershell is to continue on error
Do we have that turned off for all of our CIs?
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2123291372)
> Default behaviour in powershell is to continue on error
Do we have that turned off for all of our CIs?
💬 l0rinc commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123301215)
I see there isn't a clear winner in the source code for where struct braces should be placed - do you think we should format them differently than classes? If we want to unify the formatting with classes, are you willing to review a scripted diff formatting the existing struct braces (whitespace only)?
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123301215)
I see there isn't a clear winner in the source code for where struct braces should be placed - do you think we should format them differently than classes? If we want to unify the formatting with classes, are you willing to review a scripted diff formatting the existing struct braces (whitespace only)?
💬 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
(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.
(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?
(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.
(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.
(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.
(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?
(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.
(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)?
(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
(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
?
(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?
(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
...
(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
(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