💬 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

💬 fanquake commented on pull request "guix: Build for macOS using Clang only":
(https://github.com/bitcoin/bitcoin/pull/32764#issuecomment-3480370515)
Looks like this still using the GNU binutils for building native tools.

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3480373301)
Code review reACK 9a387e9175d34d1eaf83b9008f5e857d1eb04b8e

Finalized the pointer to reference migration since last ack.

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

Re-acked (after 7 mins :D ), only minor/formatting changes since last review

👍 hodlinator approved a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3410905600)
re-ACK 9a387e9175d34d1eaf83b9008f5e857d1eb04b8e

Changes since previous review (https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3409949311):

* Re-added `Assert` in response to https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2485972867
I was slightly worried the lock lambda would become more likely to return by-value, instead of by-reference, but `CBlockIndex` has `protected`/`delete`d copying:
https://github.com/bitcoin/bitcoin/blob/9a387e9175d34d1eaf83b9008f5e857d
...