💬 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);
+}
...

💬 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

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486317255)
Are we sure we're not just masking a real problem with the disabled logger?

💬 optout21 commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486328436)
nit: `--i` is preferred

💬 TheCharlatan commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486334639)
The logger is way less problematic in terms of its effect on the memory growing, and I find it difficult to really pin its effect. Having a global input fetcher that does not get instantiated with every fuzzer iteration has an immediate and clear effect. It is not clear to me if we are are actually leaking anything through the threads, or if creating and destroying thousands of threads per second puts too much pressure on the os (same for the threadpool).

🤔 l0rinc requested changes to a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661#pullrequestreview-3407000419)
Concept ACK, during review I also understood this behavior better :)
I have a few suggestions for simplification and am throwing you into the deep end by asking for a property based (fuzz) test as well to check certain invariants. Not a must-have, but seeks simple enough. I also suggest simplifying the distribution test to just check for the median instead.

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2483243259)
👍 for the static removal, but why remove the `inline` keyword?

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2483265868)
This seems unused

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486192203)
I need more context, how did we get to these numbers exactly?
Do we really need to be this precise? Wouldn't it suffice to check that their median is below nHeight/2?

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486172116)
nit: can this be `if (height <= 2)` as well? It's likely outside the scope of the change though

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486181169)
we could add comments explaining the magic values:
\# height <= 2
\# powers of 2
\# odd/even heights
\# absurdly big values or whatever

Otherwise it's not obvious why 4 is 0 and 5 is 1 and there is no 6 but 7 is 1 again...

I need to understand why we chose these numbers, especially since the PR is meant to document the change

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486214751)
this sounds like a `BOOST_REQUIRE_EQUAL` instead

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486186460)
I like the binary representation, but maybe we could use `BOOST_CHECK_EQUAL` insead (helps with the reason when they fail) and maybe we could align these to simplify finding the differences in their endings:
```C++
BOOST_CHECK_EQUAL(GetSkipHeight(0b0001011101101000),
0b0001011101100000);
BOOST_CHECK_EQUAL(GetSkipHeight(0b0001011101101001),
0b0001011101000001);
BOOST_CHECK_EQUAL(GetSkipHeight(0b0110101101011000),

...

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486224185)
maybe this would also work
```suggestion
std::vector<std::unique_ptr<CBlockIndex>> block_index;
block_index.push_back(std::make_unique<CBlockIndex>());
```

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486256971)
how come we're iterating one less than before?

```suggestion
for(int i = 1; i <= n; ++i) {
```

🤔 stickies-v reviewed a pull request: "rest: Query predecessor headers using negative count param"
(https://github.com/bitcoin/bitcoin/pull/33752#pullrequestreview-3410832257)
> There is no existing mechanism other than requesting headers one-at-a-time.

Request the starting hash with `GET /rest/blockhashbyheight/<HEIGHT>.<bin|hex|json>` and then request headers upward with `/rest/headers/`?

Concept NACK, unless I'm misunderstanding the limitations/use case.

💬 optout21 commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486341559)
nit: indentation seems off (extra indent)

👋 l0rinc's pull request is ready for review: "refactor: remove dead branches in `SingletonClusterImpl`"
(https://github.com/bitcoin/bitcoin/pull/33768)

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486358996)
Ah! Of course, this way the Assert is inside a function

Sorry, I pushed again and I'll have you to re-ACK, I really preferred having an assertion there

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3480341842)
In my last push: re-introduce Assert in header sync tests, see https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2485972867

Sorry for another push after the acks, and thanks for the reviews