💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486117743)
I've never used tracing before, my understanding is that @0xB10C uses these for monitoring mostly.
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486117743)
I've never used tracing before, my understanding is that @0xB10C uses these for monitoring mostly.
💬 TheCharlatan commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486129322)
I'm observing a memory leak in this fuzz test similar to the one we had for the thread pool. Over there, we disabled logging and instantiate only a single pool instance: https://github.com/bitcoin/bitcoin/pull/33689/files#diff-68602d972fe2b027e3987eff0042c27f3f00fc161b7fe871bdb147571f348298R49-R58 . Maybe similar things can be done here?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486129322)
I'm observing a memory leak in this fuzz test similar to the one we had for the thread pool. Over there, we disabled logging and instantiate only a single pool instance: https://github.com/bitcoin/bitcoin/pull/33689/files#diff-68602d972fe2b027e3987eff0042c27f3f00fc161b7fe871bdb147571f348298R49-R58 . Maybe similar things can be done here?
📝 l0rinc opened a pull request: "refactor: remove dead branches in `SingletonClusterImpl`"
(https://github.com/bitcoin/bitcoin/pull/33768)
`SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
`TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency. Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
Found during review: http
...
(https://github.com/bitcoin/bitcoin/pull/33768)
`SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
`TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency. Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
Found during review: http
...
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2486133442)
pushed a follow-up to https://github.com/bitcoin/bitcoin/pull/33768
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2486133442)
pushed a follow-up to https://github.com/bitcoin/bitcoin/pull/33768
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486137554)
Thanks for reporting, that explains why I was seeing
```bash
bench_bitcoin(10874,0x207c84800) malloc: Failed to allocate segment from range group - out of space
```
and
```bash
bitcoind(70369,0x16f5ff000) malloc: Failed to allocate segment from range group - out of space
```
recently (cc: @maflcko)
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486137554)
Thanks for reporting, that explains why I was seeing
```bash
bench_bitcoin(10874,0x207c84800) malloc: Failed to allocate segment from range group - out of space
```
and
```bash
bitcoind(70369,0x16f5ff000) malloc: Failed to allocate segment from range group - out of space
```
recently (cc: @maflcko)
💬 maflcko commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#issuecomment-3480077208)
Sorry for missing the review request earlier.
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
<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+krxU1A3Yux4bpwZNLvVBK
...
(https://github.com/bitcoin/bitcoin/pull/32517#issuecomment-3480077208)
Sorry for missing the review request earlier.
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
<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+krxU1A3Yux4bpwZNLvVBK
...
💬 optout21 commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486176254)
> This is the actual output of running the script, looks like it wasn't updated in a while
I think that the scripts only enables the trace and listens to it, but it does not trigger a particular use case.
If trace outputs are displayed with these changes, that should be sufficient check (i.e., tracing a FORCE_SYNC is not crucial).
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486176254)
> This is the actual output of running the script, looks like it wasn't updated in a while
I think that the scripts only enables the trace and listens to it, but it does not trigger a particular use case.
If trace outputs are displayed with these changes, that should be sufficient check (i.e., tracing a FORCE_SYNC is not crucial).
💬 maflcko commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486176942)
Nice. Thanks for taking the suggestion.
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486176942)
Nice. Thanks for taking the suggestion.
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480094304)
> should the depends build instructions be updated to mention this?
I'd prefer updating the docs once `depends/hosts/mingw32.mk` is adjusted so that compilers don't need to be specified explicitly.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480094304)
> should the depends build instructions be updated to mention this?
I'd prefer updating the docs once `depends/hosts/mingw32.mk` is adjusted so that compilers don't need to be specified explicitly.
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480102332)
> > If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.
>
> I think it is just one of the many gcc false-positive warnings.
That sounds plausible, though I'm not entirely convinced at this point.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480102332)
> > If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.
>
> I think it is just one of the many gcc false-positive warnings.
That sounds plausible, though I'm not entirely convinced at this point.
💬 maflcko commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486195510)
Interesting, how would a fuzz target lead to a crash in bench or bitcoind? Did you run it in parallel?
Though, the suggestion to disable logging is correct, because while fuzzing, we probably don't want to spend cycles on log formatting. I guess those logs only end up in the buffer which causes the memory to grow? (There should be a `DEFAULT_MAX_LOG_BUFFER`, so if buffering is the issue, the memory should be limited)
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486195510)
Interesting, how would a fuzz target lead to a crash in bench or bitcoind? Did you run it in parallel?
Though, the suggestion to disable logging is correct, because while fuzzing, we probably don't want to spend cycles on log formatting. I guess those logs only end up in the buffer which causes the memory to grow? (There should be a `DEFAULT_MAX_LOG_BUFFER`, so if buffering is the issue, the memory should be limited)
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480133245)
From https://github.com/bitcoin/bitcoin/actions/runs/19014374041/job/54300038967:
```
In file included from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/array:43,
from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/span:43,
from /home/admin/actions-runner/_work/_temp/src/span.h:10,
from /home/admin/actions-runner/_work/_temp/src/uint256.h:10,
from /home/admin/actions-runner/_work/_temp/src/consensus/params.h:10
...
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480133245)
From https://github.com/bitcoin/bitcoin/actions/runs/19014374041/job/54300038967:
```
In file included from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/array:43,
from /usr/lib/gcc/x86_64-w64-mingw32ucrt/14/include/c++/span:43,
from /home/admin/actions-runner/_work/_temp/src/span.h:10,
from /home/admin/actions-runner/_work/_temp/src/uint256.h:10,
from /home/admin/actions-runner/_work/_temp/src/consensus/params.h:10
...
⚠️ fanquake opened an issue: "log: "Replaying blocks" / "Rolling forward" logging is rate-limited"
(https://github.com/bitcoin/bitcoin/issues/33769)
This should probably be excluded from rate-limiting:
```bash
2025-11-03T11:42:20Z Rolling forward 000000004a2ed49fca14f2a937747a307110a8da0a5052dd9065548c36aaaf17 (9627)
2025-11-03T11:42:20Z Rolling forward 0000000019bee828395d1d6a283362222991a5bcfa37604b381ca0db25b1c191 (9628)
2025-11-03T11:42:20Z Rolling forward 000000008b57d05f433a6fcf0942b7fef226678fc0a6a623088fd846f48274e0 (9629)
2025-11-03T11:42:20Z Rolling forward 00000000de934300732c23fa07ebc4778d9b64a14d998e6eb3cffbefe01ad950 (9630)
[*]
...
(https://github.com/bitcoin/bitcoin/issues/33769)
This should probably be excluded from rate-limiting:
```bash
2025-11-03T11:42:20Z Rolling forward 000000004a2ed49fca14f2a937747a307110a8da0a5052dd9065548c36aaaf17 (9627)
2025-11-03T11:42:20Z Rolling forward 0000000019bee828395d1d6a283362222991a5bcfa37604b381ca0db25b1c191 (9628)
2025-11-03T11:42:20Z Rolling forward 000000008b57d05f433a6fcf0942b7fef226678fc0a6a623088fd846f48274e0 (9629)
2025-11-03T11:42:20Z Rolling forward 00000000de934300732c23fa07ebc4778d9b64a14d998e6eb3cffbefe01ad950 (9630)
[*]
...
💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480170953)
> This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.
I'm not sure whether to Concept ACK/NACK this, as according to https://github.com/mstorsjo/llvm-mingw/issues/523, we are considering switching to using LLVM for Windows builds (has that been discussed at length somewhere)?
If that is that case, then it's not clear that churning a bunch of code, adding new GDD CIs, and code to Guix, is worthwhile, if
...
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480170953)
> This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.
I'm not sure whether to Concept ACK/NACK this, as according to https://github.com/mstorsjo/llvm-mingw/issues/523, we are considering switching to using LLVM for Windows builds (has that been discussed at length somewhere)?
If that is that case, then it's not clear that churning a bunch of code, adding new GDD CIs, and code to Guix, is worthwhile, if
...
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480197598)
> ... we are considering switching to using LLVM for Windows builds (has that been discussed at length somewhere)?
The only discussion I'm aware of is in https://github.com/bitcoin/bitcoin/issues/31388, and no conclusion has been reached so far.
On the other hand, there seems to be agreement on https://github.com/bitcoin/bitcoin/issues/30210.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480197598)
> ... we are considering switching to using LLVM for Windows builds (has that been discussed at length somewhere)?
The only discussion I'm aware of is in https://github.com/bitcoin/bitcoin/issues/31388, and no conclusion has been reached so far.
On the other hand, there seems to be agreement on https://github.com/bitcoin/bitcoin/issues/30210.
💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473)
Thanks for catching it and sorry for not specifying in my push comment!
I dropped it on purpose because it caused a compilation warning:
```
[ 73%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/headers_sync_chainwork_tests.cpp.o
/home/daniela/Developer/BitcoinCore/bitcoin/src/test/headers_sync_chainwork_tests.cpp:55:37: warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
55 | const CBlockIndex& chain_start{*Assert(WIT
...
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473)
Thanks for catching it and sorry for not specifying in my push comment!
I dropped it on purpose because it caused a compilation warning:
```
[ 73%] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/headers_sync_chainwork_tests.cpp.o
/home/daniela/Developer/BitcoinCore/bitcoin/src/test/headers_sync_chainwork_tests.cpp:55:37: warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
55 | const CBlockIndex& chain_start{*Assert(WIT
...
💬 optout21 commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#issuecomment-3480221357)
utACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41
A performance optimization, in a sensitive place but well localized.
(https://github.com/bitcoin/bitcoin/pull/33602#issuecomment-3480221357)
utACK 0ac969cddfdba52f7947e9b140ef36e2b19c2c41
A performance optimization, in a sensitive place but well localized.
💬 stickies-v commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2486152875)
This code isn't touched much, the last commit touching `zmqError` is from 2021. I don't think any authors or reviewers would assume `zmqError` means `zmqDebug` without looking at the underlying code.
On further inspection, it seems like none of these messages should be `debug` to begin with? They all look like `warning` or `error` to me? Finally, `LogZmq{Error,Warning}` would be a better name.
Suggested (untested) diff to address all:
<details>
<summary>git diff on 05661ef06b</summary>
...
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2486152875)
This code isn't touched much, the last commit touching `zmqError` is from 2021. I don't think any authors or reviewers would assume `zmqError` means `zmqDebug` without looking at the underlying code.
On further inspection, it seems like none of these messages should be `debug` to begin with? They all look like `warning` or `error` to me? Finally, `LogZmq{Error,Warning}` would be a better name.
Suggested (untested) diff to address all:
<details>
<summary>git diff on 05661ef06b</summary>
...
💬 TheCharlatan commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486301326)
The following patch seems to stabilize memory consumption:
```diff
diff --git a/src/test/fuzz/inputfetcher.cpp b/src/test/fuzz/inputfetcher.cpp
index cd2a0f5c68..609b8e1191 100644
--- a/src/test/fuzz/inputfetcher.cpp
+++ b/src/test/fuzz/inputfetcher.cpp
@@ -43 +43,10 @@ struct NoAccessCoinsView : CCoinsView
-FUZZ_TARGET(inputfetcher)
+std::optional<InputFetcher> g_fetcher{};
+
+static void setup_threadpool_test()
+{
+ LogInstance().DisableLogging();
+ g_fetcher.emplace(3);
+}
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486301326)
The following patch seems to stabilize memory consumption:
```diff
diff --git a/src/test/fuzz/inputfetcher.cpp b/src/test/fuzz/inputfetcher.cpp
index cd2a0f5c68..609b8e1191 100644
--- a/src/test/fuzz/inputfetcher.cpp
+++ b/src/test/fuzz/inputfetcher.cpp
@@ -43 +43,10 @@ struct NoAccessCoinsView : CCoinsView
-FUZZ_TARGET(inputfetcher)
+std::optional<InputFetcher> g_fetcher{};
+
+static void setup_threadpool_test()
+{
+ LogInstance().DisableLogging();
+ g_fetcher.emplace(3);
+}
...
💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486310359)
Can you try my original [suggested](https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226981997) to put the `Assert` inside?
```C++
const CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
```
it works for me locally
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486310359)
Can you try my original [suggested](https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226981997) to put the `Assert` inside?
```C++
const CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
```
it works for me locally