💬 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
...

🤔 sipa reviewed a pull request: "refactor: remove dead branches in `SingletonClusterImpl`"
(https://github.com/bitcoin/bitcoin/pull/33768#pullrequestreview-3410906177)
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f

💬 stickies-v commented on issue "log: "Replaying blocks" / "Rolling forward" logging is rate-limited":
(https://github.com/bitcoin/bitcoin/issues/33769#issuecomment-3480392336)
Is there a good reason to unconditionally log this statement? It seems to me like a single unconditional "Replaying blocks from hash (height)" before starting the loop, and a conditional (debug) per-iteration statement would be sensible?

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486416019)
Indeed, the input 2 also gives 0 as result. The intent may have been that got indices 0 and 1 it makes no sense to jump, for the rest follow the computation (but 2 accidentally also gives 0).
In any case, I don't want to change product behavior to the slightest in this PR.

💬 murchandamus commented on pull request "mini miner: enable `Linearize` return package feerates":
(https://github.com/bitcoin/bitcoin/pull/33216#issuecomment-3480414914)
Concept ACK