🤔 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

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486365567)
Generally yes, but here I wanted to copy the [original code](https://github.com/bitcoin/bitcoin/pull/33637/files#diff-2e16a7ac99a65c1617112c3c326fba499b0010fe6c48b92b59bbc9696d883585L104) as closely as possible

💬 optout21 commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3480347281)
utACK 5cbb9a40873203ea5a4dd0aa7127c9756da2f607

An evident, localized micro-optimization in a hot-path. I haven't run measurements.

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486378183)
Pushed anyway, thanks

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486378880)
Pushed anyway, thanks

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2486379425)
done, thanks