💬 ryanofsky commented on pull request "Add read-only mode to sqlite db and use in `bitcoin-wallet`":
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3013382781)
Note IIRC, the SQLITE_OPEN_READONLY flag only guarantees that sqlite won't try to write to the main database file, not any wal or jrn files associated with the database. So if the goal is being able to access wallets on read-only filesystems this may not always work and the [immutable](https://www.sqlite.org/uri.html#uriimmutable) mode might need to be used instead.
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3013382781)
Note IIRC, the SQLITE_OPEN_READONLY flag only guarantees that sqlite won't try to write to the main database file, not any wal or jrn files associated with the database. So if the goal is being able to access wallets on read-only filesystems this may not always work and the [immutable](https://www.sqlite.org/uri.html#uriimmutable) mode might need to be used instead.
💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#issuecomment-3013414743)
[8d257ed](https://github.com/bitcoin/bitcoin/commit/8d257ed5937a47e1a60b7fe085f2e984eb60a956) to [1154618](https://github.com/bitcoin/bitcoin/commit/11546183c70def6c0aa539642fd1c9ada3d46840) ([compare](https://github.com/bitcoin/bitcoin/compare/8d257ed5937a47e1a60b7fe085f2e984eb60a956..11546183c70def6c0aa539642fd1c9ada3d46840)) - Addresses review comments from @maflcko and @l0rinc. Thanks!
(https://github.com/bitcoin/bitcoin/pull/32677#issuecomment-3013414743)
[8d257ed](https://github.com/bitcoin/bitcoin/commit/8d257ed5937a47e1a60b7fe085f2e984eb60a956) to [1154618](https://github.com/bitcoin/bitcoin/commit/11546183c70def6c0aa539642fd1c9ada3d46840) ([compare](https://github.com/bitcoin/bitcoin/compare/8d257ed5937a47e1a60b7fe085f2e984eb60a956..11546183c70def6c0aa539642fd1c9ada3d46840)) - Addresses review comments from @maflcko and @l0rinc. Thanks!
📝 maflcko opened a pull request: " fuzz: Make process_message(s) more deterministic "
(https://github.com/bitcoin/bitcoin/pull/32822)
`process_message(s)` are the least stable fuzz targets, according to OSS-Fuzz.
Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
# Testing
Needs coverage compilation, as explained in `./contrib/devtools/README.md`. And then, using 32 threads:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ process_messages 32
```
Each commit can be reverted to see more non-determinism
...
(https://github.com/bitcoin/bitcoin/pull/32822)
`process_message(s)` are the least stable fuzz targets, according to OSS-Fuzz.
Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
# Testing
Needs coverage compilation, as explained in `./contrib/devtools/README.md`. And then, using 32 threads:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ process_messages 32
```
Each commit can be reverted to see more non-determinism
...
💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3013497916)
for testing, the single-check doesn't hit here and can be quite expensive, so it could be disabled via:
```diff
diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
index 3eeb121db0..f729740a3a 100644
--- a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
+++ b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
@@ -219,13 +219,6 @@ The coverage was not deterministic between runs.
if !entry
...
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3013497916)
for testing, the single-check doesn't hit here and can be quite expensive, so it could be disabled via:
```diff
diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
index 3eeb121db0..f729740a3a 100644
--- a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
+++ b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
@@ -219,13 +219,6 @@ The coverage was not deterministic between runs.
if !entry
...
🤔 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