💬 maflcko commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3479685859)
Though, I wonder what the use case is. If someone knows a header, it seems likely they also know about the previous headers and a feature to support asking for them would have no users?

💬 maflcko commented on issue "test: break `feature_block` into subtests?":
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3479692292)
Some functional tests are defined as "extended":

```
--extended run the extended test suite in addition to the basic tests

💬 maflcko commented on pull request "ci: gha: Set debug_pull_request_number_str annotation":
(https://github.com/bitcoin/bitcoin/pull/33754#issuecomment-3479741328)
> lgtm ack [fa9d0f9](https://github.com/bitcoin/bitcoin/commit/fa9d0f994b45a94e3f26c01e395c58ff59f47f43)

The ack needs to be in all-upper case for the scripts to pick it up :sweat_smile:

🤔 frankomosh reviewed a pull request: "ci: Call docker exec from Python script to fix word splitting"
(https://github.com/bitcoin/bitcoin/pull/33732#pullrequestreview-3410289108)
Concept ACK

💬 frankomosh commented on pull request "ci: Call docker exec from Python script to fix word splitting":
(https://github.com/bitcoin/bitcoin/pull/33732#discussion_r2485943158)
nit: Should all non-zero exits be treated as "nothing to copy from"? am making the assumption that rsync could fail for other reasons

💬 l0rinc commented on pull request "ci: gha: Set debug_pull_request_number_str annotation":
(https://github.com/bitcoin/bitcoin/pull/33754#issuecomment-3479767050)
code review ACK fa9d0f994b45a94e3f26c01e395c58ff59f47f43

💬 Sjors commented on pull request "miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-3479773495)
Looks like `fuzz/process_message.cpp` and `fuzz/process_messages.cpp` were missing `include_dummy_extranonce = true`.

💬 stringintech commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3479798341)
re-ACK 01cc20f

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2485972867)
I see you've reverted this after the rebase - was it on purpose?

💬 A-Manning commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3479817272)
>Though, I wonder what the use case is. If someone knows a header, it seems likely they also know about the previous headers and a feature to support asking for them would have no users?

Knowing a header only means that you know the previous block hash. If you want to request headers from best tip to genesis, There is no existing mechanism other than requesting headers one-at-a-time. This makes it possible to *batch* request headers from tip to genesis, which is much more efficient than reque
...

💬 maflcko commented on pull request "ci: Call docker exec from Python script to fix word splitting":
(https://github.com/bitcoin/bitcoin/pull/33732#discussion_r2485987246)
This is just moving the code one-to-one, but possibly `rsync`, along with the rsync command passing, could be required here. (IIRC the error message may have been better worded to reflect the intention: `rsync not found, no need to copy from ...`)

At least on macOS, it seems to pass: https://github.com/bitcoin/bitcoin/actions/runs/18913390006/job/53990076461?pr=33732#step:7:134

```
+ rsync --recursive --perms --stats --human-readable /Users/runner/work/bitcoin/bitcoin/ /Users/runner/work/
...

💬 maflcko commented on pull request "ci: Call docker exec from Python script to fix word splitting":
(https://github.com/bitcoin/bitcoin/pull/33732#discussion_r2485988430)
(added a commit to require rsync)

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

I have re-rebased [my previous ack](https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3208833064), solved the conflicts and did a sort reset compared to this, the only differences were whitespace changes, comment fixes, [static cast update](https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2269932154).
Not sure if https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2485972867 was on purpose or not, but either i
...

🤔 maflcko reviewed a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3410406728)
should the depends build instructions be updated to mention this?

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2486031033)
nit: Could consider naming this `ci_win64` (and the file), as this will be the default eventually, going forward.

💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479891594)
> A new -Warray-bounds compiler diagnostic should be evaluated and addressed.

If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3479895077)
@plebhash's account has been restored and his comments and issues came back.

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479912512)
> If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.

I think it is just one of the many gcc false-positive warnings. Not sure if it makes sense to document or investigate them individually, given that they are hard to reproduce in light of changing the compiler version or the compile options even slightly.

💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486058259)
Given the current stats on initializer list formatting, I don't think there's enough reason to open a PR to turn the tide by changing src/.clang-format.

```shell
# BeforeColon (current default in src/.clang-format, style partially objected to in this thread):
$ grep -zoP "\n( {4})+( {2})m_\w+[({]" -r src/ |wc -l
135

# AfterColon (what I expected to be most common):
$ grep -zoP "\n( {4})+m_\w+[({]" -r src/ |wc -l
117

# BeforeComma (BetaMax of initializer lists, leads to smallest di
...

💬 optout21 commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486059320)
The 'Flush for Prune' has changed here to False, and now all lines have False value here. Can the False line be preserved, or is it OK in this sample output?