💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163853656)
The point of the docstring is to explain _why_ we don't ratelimit, which is because we don't want IBD tip logs to be suppressed. Documenting that absolutely seems helpful to me.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163853656)
The point of the docstring is to explain _why_ we don't ratelimit, which is because we don't want IBD tip logs to be suppressed. Documenting that absolutely seems helpful to me.
💬 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_r2163859080)
Yes, that part does seem helpful and we can't express it with code, we should keep that part!
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163859080)
Yes, that part does seem helpful and we can't express it with code, we should keep that part!
💬 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_r2163862219)
I replied to the wrong thread here, thanks for the explanation, please resolve it.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163862219)
I replied to the wrong thread here, thanks for the explanation, please resolve it.
💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3000290032)
@hodlinator thank you for your review!
I like the idea of converting `ProcessingResult` to an enum, I think it makes the code easier to read.
As for `pow_validated_headers`, I don’t have a strong preference between using a span or an argument for I/O just yet. I’ll take a closer look at #32579 over the next few days and should have a more informed opinion afterward :) I’d also be interested to hear what other reviewers think!
Latest push:
- style improvements based on review comments
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3000290032)
@hodlinator thank you for your review!
I like the idea of converting `ProcessingResult` to an enum, I think it makes the code easier to read.
As for `pow_validated_headers`, I don’t have a strong preference between using a span or an argument for I/O just yet. I’ll take a closer look at #32579 over the next few days and should have a more informed opinion afterward :) I’d also be interested to hear what other reviewers think!
Latest push:
- style improvements based on review comments
💬 fjahr commented on pull request "init: make `-blockmaxweight` startup option debug only":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-3000301728)
ACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Seems like moving this to debug is in line with what we do with test-only RPC.
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-3000301728)
ACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Seems like moving this to debug is in line with what we do with test-only RPC.
💬 maflcko commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3000327354)
review ACK abd0749fae7259a292cb652fc9974c8cdcdd4d9d 🌘
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK abd0749fae72
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3000327354)
review ACK abd0749fae7259a292cb652fc9974c8cdcdd4d9d 🌘
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK abd0749fae72
...
💬 willcl-ark commented on pull request "build: add root dir to CMAKE_SYSTEM_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3000412860)
> > `CMAKE_SYSTEM_PREFIX_PATH` is not intended to be modified by the project; use [`CMAKE_PREFIX_PATH`](https://cmake.org/cmake/help/latest/variable/CMAKE_PREFIX_PATH.html#variable:CMAKE_PREFIX_PATH) for this.
>
> Why not follow that?
The initial approach was to add it back where it was removed. But I have tested `CMAKE_PREFIX_PATH` locally and this works too. Happy to stick to cmake guidelines and so have switched to that now in e27a94596f2a1f5e04722a16165717cc6e891d36
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3000412860)
> > `CMAKE_SYSTEM_PREFIX_PATH` is not intended to be modified by the project; use [`CMAKE_PREFIX_PATH`](https://cmake.org/cmake/help/latest/variable/CMAKE_PREFIX_PATH.html#variable:CMAKE_PREFIX_PATH) for this.
>
> Why not follow that?
The initial approach was to add it back where it was removed. But I have tested `CMAKE_PREFIX_PATH` locally and this works too. Happy to stick to cmake guidelines and so have switched to that now in e27a94596f2a1f5e04722a16165717cc6e891d36
💬 willcl-ark commented on pull request "build: add root dir to CMAKE_SYSTEM_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#discussion_r2163954096)
done in e27a94596f2a1f5e04722a16165717cc6e891d36
(https://github.com/bitcoin/bitcoin/pull/32798#discussion_r2163954096)
done in e27a94596f2a1f5e04722a16165717cc6e891d36
🚀 fanquake merged a pull request: "lsan: add more Qt suppressions"
(https://github.com/bitcoin/bitcoin/pull/32780)
(https://github.com/bitcoin/bitcoin/pull/32780)
💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3000460860)
OP or commit message should probably explicitly link https://github.com/bitcoin/bips/blob/master/bip-0054.md#specification
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3000460860)
OP or commit message should probably explicitly link https://github.com/bitcoin/bips/blob/master/bip-0054.md#specification
✅ fanquake closed an issue: "ci: Support running from a worktree"
(https://github.com/bitcoin/bitcoin/issues/30028)
(https://github.com/bitcoin/bitcoin/issues/30028)
🚀 fanquake merged a pull request: "ci: Allow running CI in worktrees"
(https://github.com/bitcoin/bitcoin/pull/32767)
(https://github.com/bitcoin/bitcoin/pull/32767)
👍 hebasto approved a pull request: "build: add root dir to CMAKE_SYSTEM_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2953832427)
ACK e27a94596f2a1f5e04722a16165717cc6e891d36, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/32798#pullrequestreview-2953832427)
ACK e27a94596f2a1f5e04722a16165717cc6e891d36, I have reviewed the code and it looks OK.
💬 hebasto commented on pull request "build: add root dir to CMAKE_SYSTEM_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#discussion_r2164000499)
To be precise, "Adding it back ... on other platforms" does not occur on every platform that supports our depends build subsystem. According to the CMake source code, [`Platform/UnixPaths`](https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/UnixPaths.cmake) is processed on all platforms: Linux, Darwin, and all *BSD.
(https://github.com/bitcoin/bitcoin/pull/32798#discussion_r2164000499)
To be precise, "Adding it back ... on other platforms" does not occur on every platform that supports our depends build subsystem. According to the CMake source code, [`Platform/UnixPaths`](https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/UnixPaths.cmake) is processed on all platforms: Linux, Darwin, and all *BSD.
⚠️ plebhash opened an issue: "IPC via TCP Sockets"
(https://github.com/bitcoin/bitcoin/issues/32802)
### Please describe the feature you'd like to see added.
Interprocess communication via TCP Sockets
### Is your feature related to a problem, if so please describe it.
I'm dockerizing @Sjors 's Stratum V2 sidecar (`bitcoin-mine`) such that it's able to reach a Bitcoin Core node running on the host.
My host is macOS, and Docker is Linux, which means they don't have a shared kernel and their Unix socket descriptors don't match.
### Describe the solution you'd like
IPC over TCP sockets.
###
...
(https://github.com/bitcoin/bitcoin/issues/32802)
### Please describe the feature you'd like to see added.
Interprocess communication via TCP Sockets
### Is your feature related to a problem, if so please describe it.
I'm dockerizing @Sjors 's Stratum V2 sidecar (`bitcoin-mine`) such that it's able to reach a Bitcoin Core node running on the host.
My host is macOS, and Docker is Linux, which means they don't have a shared kernel and their Unix socket descriptors don't match.
### Describe the solution you'd like
IPC over TCP sockets.
###
...
💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3000571610)
concept and approach ACK
I have a preference for breaking out the new checks into its own function since if we intend it to someday soon(TM) be a consensus rule it would have to be pulled out anyways. https://github.com/instagibbs/bitcoin/commit/5d63372f75c4a2403c4032b63e5c604ee96c5a40
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3000571610)
concept and approach ACK
I have a preference for breaking out the new checks into its own function since if we intend it to someday soon(TM) be a consensus rule it would have to be pulled out anyways. https://github.com/instagibbs/bitcoin/commit/5d63372f75c4a2403c4032b63e5c604ee96c5a40
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2164070208)
Could also just Assume it's higher than `FeeFrac{1, 1}` like later
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2164070208)
Could also just Assume it's higher than `FeeFrac{1, 1}` like later
💬 Sjors commented on issue "IPC via TCP Sockets":
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3000589480)
`bitcoin-mine` refers to the Template Provider application I wrote in https://github.com/Sjors/bitcoin/pull/48. This is not intended to be include in Bitcoin Core.
It's based on the `bitcoin-mine` demo proposed by @ryanofsky in #30437. This may or may not be included in v30, but it doesn't really matter if this particular demo adds TCP support.
What does matter is that the `-ipcbind` argument of `bitcoin-node` gets TCP support.
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3000589480)
`bitcoin-mine` refers to the Template Provider application I wrote in https://github.com/Sjors/bitcoin/pull/48. This is not intended to be include in Bitcoin Core.
It's based on the `bitcoin-mine` demo proposed by @ryanofsky in #30437. This may or may not be included in v30, but it doesn't really matter if this particular demo adds TCP support.
What does matter is that the `-ipcbind` argument of `bitcoin-node` gets TCP support.
🤔 rkrux reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2953212032)
I have finished reviewing the `descriptor: Add MuSigPubkeyProvider` commit - 4039be3f9b78c1ddb75c04f4a8002d36c4b05f79
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2953212032)
I have finished reviewing the `descriptor: Add MuSigPubkeyProvider` commit - 4039be3f9b78c1ddb75c04f4a8002d36c4b05f79
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2163953475)
I see, `out` does need to be filled. In that case, there is an option to cache the `dummy` signing provider on line 629 when the aggregate pubkey & provider are being created (and cached) in case of non ranged participants. This could later be `FlatSigningProvider::Merge`'d with the `out` signing provider on line 672 so that `out` has information for both the participants and the aggregate.
The reason I find moving this snippet inside the `else` block here appealing is that we don't need to d
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2163953475)
I see, `out` does need to be filled. In that case, there is an option to cache the `dummy` signing provider on line 629 when the aggregate pubkey & provider are being created (and cached) in case of non ranged participants. This could later be `FlatSigningProvider::Merge`'d with the `out` signing provider on line 672 so that `out` has information for both the participants and the aggregate.
The reason I find moving this snippet inside the `else` block here appealing is that we don't need to d
...