💬 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_r2163818101)
Semantically, I agree. But practically, I think it's fine as is. Conditional vs unconditional are at this point fairly established concepts in our logging code in that they indicate whether or not logging categories need to be enabled or not. Rate limiting should in practice never occur, so for all intents and purposes, it is indeed unconditional.
How would you suggest to improve this? I can't think of an improvement.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163818101)
Semantically, I agree. But practically, I think it's fine as is. Conditional vs unconditional are at this point fairly established concepts in our logging code in that they indicate whether or not logging categories need to be enabled or not. Rate limiting should in practice never occur, so for all intents and purposes, it is indeed unconditional.
How would you suggest to improve this? I can't think of an improvement.
💬 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_r2163825735)
That would mean the rate limiting warnings are no longer logged to console and callbacks (`NeedsRateLimiting` has side effects), so I think keeping this as-is is better.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163825735)
That would mean the rate limiting warnings are no longer logged to console and callbacks (`NeedsRateLimiting` has side effects), so I think keeping this as-is is better.
💬 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_r2163837664)
I don't consider "established" a relevant argument in this case - it just prevents new contributors from understanding the codebase. But if you think this isn't confusing, I don't mind resolving the comment, I did eventually understand it, just got sidetracked by the contradiction and wanted to simplify it to the next person. If we do correct it, we could be more specific instead of overly generalizing it (ie specify the restrictions instead of claiming there are none).
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163837664)
I don't consider "established" a relevant argument in this case - it just prevents new contributors from understanding the codebase. But if you think this isn't confusing, I don't mind resolving the comment, I did eventually understand it, just got sidetracked by the contradiction and wanted to simplify it to the next person. If we do correct it, we could be more specific instead of overly generalizing it (ie specify the restrictions instead of claiming there are none).
💬 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_r2163843028)
What do you mean with "dead parameter names"? This variable defines the default log level, which indeed is debug level. If you run `bitcoind -debug=net`, you'll start seeing net debug logs, you don't have to set `-loglevel=debug`.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163843028)
What do you mean with "dead parameter names"? This variable defines the default log level, which indeed is debug level. If you run `bitcoind -debug=net`, you'll start seeing net debug logs, you don't have to set `-loglevel=debug`.
💬 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_r2163843866)
(I'm on a phone now, can't check the source)
Ah, so there's a side-effect here that wasn't obvious to me, thanks for clarifying.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163843866)
(I'm on a phone now, can't check the source)
Ah, so there's a side-effect here that wasn't obvious to me, thanks for clarifying.
💬 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