💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489550775)
I did that for the longer values, for shorter values with only a few ones is not really need.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489552705)
Added median check as well. Much lower than the avg!

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489553665)
Added explanatory comments

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489555855)
Oops, fixed!

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489557279)
Done

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489560183)
All cycles go from 1 to n-1; 0 is the genesis.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2489569251)
A fuzz test for this functionality (essentially a one-liner) seems a bit too much.

🚀 fanquake merged a pull request: "test: cover invalid codesep positions for signature in taproot"
(https://github.com/bitcoin/bitcoin/pull/32301)

⚠️ Sjors opened an issue: "ci: spurious linter failure?"
(https://github.com/bitcoin/bitcoin/issues/33773)
Run is marked as cancelled but looks fine:

- https://github.com/bitcoin/bitcoin/actions/runs/19062940342/job/54446647440?pr=32876

💬 alexanderwiederin commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3485080149)
re-ACK https://github.com/bitcoin/bitcoin/commit/6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969

🚀 fanquake merged a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445)

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3485151628)
This looks like a nice and small CI-only/test-only change to flatten the way for https://github.com/bitcoin/bitcoin/issues/30210. There is already a pull open for the guix changes in https://github.com/bitcoin/bitcoin/pull/33593, which can then pick up any follow-ups from here and also conclude by removing the msvcrt CI config and msvcrt workarounds in the code.

review ACK 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf 🐂

<details><summary>Show signature</summary>

Signature:

```
untrusted
...

💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2489816883)
Is there a way to do this without copy-pasting/duplicating > 100 lines of CI config (where both copies need to be kept in sync).

🤔 vasild reviewed a pull request: "[wip] wallet: Add separate balance info for non-mempool wallet txs"
(https://github.com/bitcoin/bitcoin/pull/33671#pullrequestreview-3415535438)
Concept ACK

This extends the `getbalances` RPC. Would be good to also update the GUI (not necessary in this PR).

The new type of transactions would have somehow to be fed to the wallet so that they would be returned by `wallet.GetTXOs()`, called by `GetBalance()`. How would that be done?

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2489849930)
I guess yaml anchors could be used, but this would increase the diff and review effort again when it is deleted. An alternative would be to use a matrix, but I guess this requires depending both cross-test tasks on both cross-compile tasks? I'd say the current version is fine, but no strong opinion.

💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3485238772)
> This one simply addresses your comments https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3460836590 and https://github.com/bitcoin/bitcoin/pull/33593#pullrequestreview-3398698978.

Somewhat, but if this is the approach, it means the not only the compiler, but also the version of the headers being tested isn't going to match Guix?

💬 maflcko commented on issue "ci: spurious linter failure?":
(https://github.com/bitcoin/bitcoin/issues/33773#issuecomment-3485279327)
This simply times out, because the Ubuntu PPAs are so slow/rate-limited/ddos'd:

The task starts at 8:50, but download only concludes after 9:06:

* https://github.com/bitcoin/bitcoin/actions/runs/19062940342/job/54446647440?pr=32876#step:4:1110

`#11 951.6 Fetched 173 MB in 6min 31s (443 kB/s)`

* https://github.com/bitcoin/bitcoin/actions/runs/19062940342/job/54446647440?pr=32876#step:4:298:

`#11 549.5 Fetched 181 MB in 7min 56s (380 kB/s)`


See https://github.com/bitcoin/bitcoin/pull/33744
...

💬 stringintech commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2489983149)
> but the remaining code won't be called anyways

But previously we would call `InvalidChainFound()` (and the rest of the code after the `m_chain.Contains(to_mark_failed)` check) on the last disconnected tip even if we were interrupted; right?

💬 stringintech commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#issuecomment-3485329617)
Concept ACK

💬 l0rinc commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3485386361)
thank you for the reviews.
rfm?