⚠️ fanquake opened an issue: "upstream: capnp V2 doesn't support compilation with GCC (yet?)"
(https://github.com/bitcoin/bitcoin/issues/32669)
See https://github.com/capnproto/capnproto/pull/2304, and the discussion from the maintainer. Seems like the interest in supporting GCC is pretty secondary:
> Is there any way we can get a CI build that tests GCC support? Otherwise I'm sure it'll just bitrot again.
> OK, seems fine to merge now but we can't really claim that GCC is "supported" until we have CI tests since it'll probably bitrot.
When that PR lands, it looks like they be supporting GCC 14.2 and later.
Assuming we are going to s
...
(https://github.com/bitcoin/bitcoin/issues/32669)
See https://github.com/capnproto/capnproto/pull/2304, and the discussion from the maintainer. Seems like the interest in supporting GCC is pretty secondary:
> Is there any way we can get a CI build that tests GCC support? Otherwise I'm sure it'll just bitrot again.
> OK, seems fine to merge now but we can't really claim that GCC is "supported" until we have CI tests since it'll probably bitrot.
When that PR lands, it looks like they be supporting GCC 14.2 and later.
Assuming we are going to s
...
💬 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-2934239293)
I see. So instead I registered the policy by tweaking Moosig. That worked and returned an HMAC.
I then hardcode that HMAC into `moosig.py` along with the PSBT generated by Bitcoin Core and try to make it request a pubnonce. For this I removed the `HotMusig2Cosigner`.
The device recognizes the policy, destination address, amount and fees which I then approve. But then it fails again:
```
🐮 Requesting pubnonces (Round 1)
Traceback (most recent call last):
File "/Users/sjors/dev/moos
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2934239293)
I see. So instead I registered the policy by tweaking Moosig. That worked and returned an HMAC.
I then hardcode that HMAC into `moosig.py` along with the PSBT generated by Bitcoin Core and try to make it request a pubnonce. For this I removed the `HotMusig2Cosigner`.
The device recognizes the policy, destination address, amount and fees which I then approve. But then it fails again:
```
🐮 Requesting pubnonces (Round 1)
Traceback (most recent call last):
File "/Users/sjors/dev/moos
...
💬 l0rinc commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123224744)
weird, I thought formatter falls back (my IDE delegates to clang-format and does that) - see https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072683803
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2123224744)
weird, I thought formatter falls back (my IDE delegates to clang-format and does that) - see https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072683803
💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#discussion_r2123235531)
Just to be clear, you mean switching the native_previous_releases job from `ubuntu:22.04` to `debian:bookworm`?
(https://github.com/bitcoin/bitcoin/pull/32595#discussion_r2123235531)
Just to be clear, you mean switching the native_previous_releases job from `ubuntu:22.04` to `debian:bookworm`?
💬 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-2934326504)
If only requesting a pubnonce to the device, this should happen silently (without on-screen interaction). So if you're validating the transaction in what is supposed to be round 1, it ain't round 1 :)
For reference, here's the [code in our e2e tests](https://github.com/LedgerHQ/app-bitcoin-new/blob/de2c15d2a9b11d61c1c9ded973545cc0e2ef3e36/tests/test_e2e_musig2.py#L43-L225) with core, if that helps.
Also note that as MuSig2 is a stateful process, if you try round 2 and it fails for any reas
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2934326504)
If only requesting a pubnonce to the device, this should happen silently (without on-screen interaction). So if you're validating the transaction in what is supposed to be round 1, it ain't round 1 :)
For reference, here's the [code in our e2e tests](https://github.com/LedgerHQ/app-bitcoin-new/blob/de2c15d2a9b11d61c1c9ded973545cc0e2ef3e36/tests/test_e2e_musig2.py#L43-L225) with core, if that helps.
Also note that as MuSig2 is a stateful process, if you try round 2 and it fails for any reas
...
🤔 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)?