🤔 stickies-v reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2958036547)
This [thread](https://github.com/bitcoin/bitcoin/pull/32604/files#r2164992810) nerdsniped me into reimplementing this PR with a different approach, leaving the scheduled resetting to `CScheduler`. This feels like a much more natural fit to me, and allows a couple of other improvements. See branch [here](https://github.com/stickies-v/bitcoin/commits/2025-06/schedule-ratelimit-reset/), or diff [here](https://github.com/Crypt-iQ/bitcoin/compare/2ac8696b53e455dd27c8341828404a23b5cb68a9..9fb36ba46bcc
...
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2958036547)
This [thread](https://github.com/bitcoin/bitcoin/pull/32604/files#r2164992810) nerdsniped me into reimplementing this PR with a different approach, leaving the scheduled resetting to `CScheduler`. This feels like a much more natural fit to me, and allows a couple of other improvements. See branch [here](https://github.com/stickies-v/bitcoin/commits/2025-06/schedule-ratelimit-reset/), or diff [here](https://github.com/Crypt-iQ/bitcoin/compare/2ac8696b53e455dd27c8341828404a23b5cb68a9..9fb36ba46bcc
...
💬 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_r2172385738)
Sorry for the wait, branch is ready: https://github.com/stickies-v/bitcoin/commits/2025-06/schedule-ratelimit-reset/ . I added the "fixup" comments as commits that should be squashable into the current branch as-is (requiring rebase for the commits above, though). Hopefully the isolation of the changes makes this easier to review and potentially adopt into this PR.
The main change is that I've carved out scheduling from `LogRateLimiter` (leaving that to `CScheduler`), which allows quite a bit
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2172385738)
Sorry for the wait, branch is ready: https://github.com/stickies-v/bitcoin/commits/2025-06/schedule-ratelimit-reset/ . I added the "fixup" comments as commits that should be squashable into the current branch as-is (requiring rebase for the commits above, though). Hopefully the isolation of the changes makes this easier to review and potentially adopt into this PR.
The main change is that I've carved out scheduling from `LogRateLimiter` (leaving that to `CScheduler`), which allows quite a bit
...
🤔 hebasto reviewed a pull request: "doc: add alpine build instructions"
(https://github.com/bitcoin/bitcoin/pull/32559#pullrequestreview-2967102495)
Tested 64986e1a44e108628b245ca977d2b8eb69ed5580 on Alpine Linux v3.22.
(https://github.com/bitcoin/bitcoin/pull/32559#pullrequestreview-2967102495)
Tested 64986e1a44e108628b245ca977d2b8eb69ed5580 on Alpine Linux v3.22.
💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172347865)
FWIW, `linux-headers` is a dependency of `boost-dev` on my system.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172347865)
FWIW, `linux-headers` is a dependency of `boost-dev` on my system.
💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172354607)
not even a nit: `gmake`, which explicitly refers to GNU Make, also works.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172354607)
not even a nit: `gmake`, which explicitly refers to GNU Make, also works.
💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172363200)
However, depends build the `systemtap` package. The subsequent configuration results in:
```sh
# cmake -B build --toolchain depends/x86_64-pc-linux-musl/toolchain.cmake
<snip>
USDT tracing ........................ ON
<snip>
```
And the build succeeds.
cc @0xB10C
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172363200)
However, depends build the `systemtap` package. The subsequent configuration results in:
```sh
# cmake -B build --toolchain depends/x86_64-pc-linux-musl/toolchain.cmake
<snip>
USDT tracing ........................ ON
<snip>
```
And the build succeeds.
cc @0xB10C
💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172422391)
1. I think we should consider a note from `apk`:
```
Executing ninja-build-1.12.1-r1.post-install
* this only installs ninja to /usr/lib/ninja-build/bin/ninja
* add that to your path to use it, or invoke it directly.
* for most uses, you want samurai instead:
* $ apk add samurai
* which has a "ninja" executable compatible with ninja.
```
and install `samurai`. Otherwise, Qt still complains:
```
CMake Warning at qtbase/cmake/QtBuildHelpers.cmake:12 (message):
The officially support
...
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2172422391)
1. I think we should consider a note from `apk`:
```
Executing ninja-build-1.12.1-r1.post-install
* this only installs ninja to /usr/lib/ninja-build/bin/ninja
* add that to your path to use it, or invoke it directly.
* for most uses, you want samurai instead:
* $ apk add samurai
* which has a "ninja" executable compatible with ninja.
```
and install `samurai`. Otherwise, Qt still complains:
```
CMake Warning at qtbase/cmake/QtBuildHelpers.cmake:12 (message):
The officially support
...
💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#issuecomment-3013738127)
Additionally,
```
# ldd ./build-dep/bin/bitcoin-qt
/lib/ld-musl-x86_64.so.1 (0x77d3de93a000)
Error loading shared library libfontconfig.so.1: No such file or directory (needed by ./build-dep/bin/bitcoin-qt)
Error loading shared library libfreetype.so.6: No such file or directory (needed by ./build-dep/bin/bitcoin-qt)
Error loading shared library libxkbcommon.so.0: No such file or directory (needed by ./build-dep/bin/bitcoin-qt)
Error loading shared library libxkbcommon-x11.so.0: No such
...
(https://github.com/bitcoin/bitcoin/pull/32559#issuecomment-3013738127)
Additionally,
```
# ldd ./build-dep/bin/bitcoin-qt
/lib/ld-musl-x86_64.so.1 (0x77d3de93a000)
Error loading shared library libfontconfig.so.1: No such file or directory (needed by ./build-dep/bin/bitcoin-qt)
Error loading shared library libfreetype.so.6: No such file or directory (needed by ./build-dep/bin/bitcoin-qt)
Error loading shared library libxkbcommon.so.0: No such file or directory (needed by ./build-dep/bin/bitcoin-qt)
Error loading shared library libxkbcommon-x11.so.0: No such
...
📝 pablomartin4btc opened a pull request: "test: Fix wait_for_getheaders() call in test_outbound_eviction_blocks_relay_only()"
(https://github.com/bitcoin/bitcoin/pull/32823)
This change avoids relying on `tip_header.hash`, which is `None` when the header is deserialized from hex during `CBlockHeader()` construction.
Instead, `tip_header.rehash()` explicitly computes the hash, making the test behavior more robust.
Using the explicit `rehash()` avoids depending on `wait_for_getheaders()` falling back to any received message, thus making the test more deterministic.
This is a follow-up to #32742.
Also, as noted in a previous review [comment](https://github.co
...
(https://github.com/bitcoin/bitcoin/pull/32823)
This change avoids relying on `tip_header.hash`, which is `None` when the header is deserialized from hex during `CBlockHeader()` construction.
Instead, `tip_header.rehash()` explicitly computes the hash, making the test behavior more robust.
Using the explicit `rehash()` avoids depending on `wait_for_getheaders()` falling back to any received message, thus making the test more deterministic.
This is a follow-up to #32742.
Also, as noted in a previous review [comment](https://github.co
...
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2172530750)
(Forgot to mention but I changed from `to_be_stripped` + `stripping` to `queue` + `flattening`).
https://github.com/bitcoin/bitcoin/blob/2bb86e371c21ca996cf8c1fbc80bc9b39f3d1dec/src/script/miniscript.h#L536-L544
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2172530750)
(Forgot to mention but I changed from `to_be_stripped` + `stripping` to `queue` + `flattening`).
https://github.com/bitcoin/bitcoin/blob/2bb86e371c21ca996cf8c1fbc80bc9b39f3d1dec/src/script/miniscript.h#L536-L544
💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2172558100)
The line wasn't necessary, I removed it in 43d2613. Thanks for pointing this out though as the DrahtBot message wasn't clear to me!
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2172558100)
The line wasn't necessary, I removed it in 43d2613. Thanks for pointing this out though as the DrahtBot message wasn't clear to me!
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013959756)
@sipa simulation looks solid from a few minutes going through it, I think it's a good addition
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013959756)
@sipa simulation looks solid from a few minutes going through it, I think it's a good addition
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2172597431)
older comment
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2172597431)
older comment
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2172705028)
Hmm, sorry for the very very late response @andrewtoth. I missed this message completely.
> I still think we would want to abandon the work queue if we are interrupted instead of waiting for it to finish, no? My naive approach of not waiting for the queue to be empty does not work though.
Yeah, I don’t think that’s safe. Other threads might be waiting on the tasks' futures to complete, so exiting without notifying them would leave them blocked forever.
What we could do (and I think you me
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2172705028)
Hmm, sorry for the very very late response @andrewtoth. I missed this message completely.
> I still think we would want to abandon the work queue if we are interrupted instead of waiting for it to finish, no? My naive approach of not waiting for the queue to be empty does not work though.
Yeah, I don’t think that’s safe. Other threads might be waiting on the tasks' futures to complete, so exiting without notifying them would leave them blocked forever.
What we could do (and I think you me
...
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2172711323)
> Can we modify this to be more generic and return a type? And add logic to collect all returned values into a shared vector which can then be atomically swapped out by an observer? Possibly not in this PR, but if this will be split out into a generic thread pool.
We could keep track of the task futures' promises, if that's what you're referring to. In other words, this void() function is just a wrapper that executes a generic function which sets the result inside the caller’s future.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2172711323)
> Can we modify this to be more generic and return a type? And add logic to collect all returned values into a shared vector which can then be atomically swapped out by an observer? Possibly not in this PR, but if this will be split out into a generic thread pool.
We could keep track of the task futures' promises, if that's what you're referring to. In other words, this void() function is just a wrapper that executes a generic function which sets the result inside the caller’s future.
💬 achow101 commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#issuecomment-3014267780)
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
(https://github.com/bitcoin/bitcoin/pull/32638#issuecomment-3014267780)
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
🚀 achow101 merged a pull request: "blocks: force hash validations on disk read"
(https://github.com/bitcoin/bitcoin/pull/32638)
(https://github.com/bitcoin/bitcoin/pull/32638)
💬 achow101 commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-3014283854)
ACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-3014283854)
ACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
🚀 achow101 merged a pull request: "test: added fuzz coverage for consensus/merkle.cpp"
(https://github.com/bitcoin/bitcoin/pull/32243)
(https://github.com/bitcoin/bitcoin/pull/32243)
💬 achow101 commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449)
> `walletpassphrase` is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to `CWallet::Lock()` after the user needs a private key
`WalletContext` has its own `CScheduler` as well. I think it would probably make more sense to drop `HTTPRPCTimer` altogether and have the RPC schedule the lock by itself instead of taking a trip through the interfaces.
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449)
> `walletpassphrase` is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to `CWallet::Lock()` after the user needs a private key
`WalletContext` has its own `CScheduler` as well. I think it would probably make more sense to drop `HTTPRPCTimer` altogether and have the RPC schedule the lock by itself instead of taking a trip through the interfaces.