💬 maflcko commented on pull request "clang-format: Set Bitcoin Core IncludeCategories":
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547289151)
I think this is just the common approach to go from local to global, and also used in this codebase historically. This means:

* Priority -1 for the build-config (not sure why this is special). Changing it can be done in a different pull. Here I just want to document the existing order.
* Priority 1 for Bitcoin Core stuff
* Priority 2 for third-party libs (boost, QT)
* Priority 3 for the c++ stdlib (includes without a dot in them)

💬 davidgumberg commented on pull request "test: Retry download in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/33915#issuecomment-3559587991)
ACK https://github.com/bitcoin/bitcoin/pull/33915/commits/fad06f3bb436a97683e8248bfda1bd0d9998c899

Nice! I've also had issues with this locally, retrying once seems a good idea.

💬 maflcko commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3559605455)
> My preference would be (1), but limit documentation / support to the most recent macOS version. It can be limited to the minimum tooling required to briefly run fuzzers.

Sounds good. So I guess this is up-for-grabs, and a dev with the latest macOS can just update the docs (https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer) with the steps that work with that version?

💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-3559611938)
Just added https://github.com/bitcoin/bitcoin/pull/33916 to the review list. This PR adds a fuzz target for the `TransactionCanBeBumped` function from `feebumper`.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-3559618090)
<!-- begin push-33 -->
Rebased 6bab3c091b6e9f8a90255066e7fb7f063eede5bc -> 8e4ceb66a97fa14afc932d12f944662638ed87af ([`pr/ipc-gui.32`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-gui.32) -> [`pr/ipc-gui.33`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-gui.33), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-gui.32-rebase..pr/ipc-gui.33))<!-- end --> on top of base PR

🤔 mzumsande reviewed a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3489487344)
> CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node, then the call should still work. As commented above, colleagues here requested that -netinfo continue to work in such cases, and we patched it. There doesn't seem to be any reason not to do the same here, rather than choosing to break it.

I don't agree with that. I don't see `bitcoin-cli` and `bitcoind` as completely se
...

💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547342770)
nit:

```suggestion
for (const auto &tx : resp.txn) tx_requested_size += tx->ComputeTotalSize();
```

💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3559651569)
@maflcko friendly review ping :)

💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3559658957)
> I tried rebasing onto #33785 but it wouldn't fix the warning.

Checked this by:
```
₿ git pr 32740
Switched to branch 'pr/32740'
```
Rebasing onto the merge commit of #33785:
```
₿ git rebase HEAD~8 --onto 490cb056f651f4247731e39efab35ef8e8a59833
```
Adding back `const` in commit 9bfcde056df7698ddf7ce9d958d5a9d3f3d0b43a:

ARM 32 build and Linux->Windows cross builds succeed on CI with GNU (GCC) 13.3:
https://github.com/hodlinator/bitcoin/actions/runs/19529971162/job/55910436956#s
...

💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2547377106)
(Maybe `ConsumeBit(LE|BE)` would be more appropriate than `DecodeBit(LE|BE)`).

🤔 w0xlt reviewed a pull request: "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3489553822)
This PR eliminates the ambiguity of the boolean return value and improves the clarity of both the internal and kernel APIs.

ACK https://github.com/bitcoin/bitcoin/pull/33856/commits/1f589e1af162c2c2705f0404496c549785f2f545

💬 brunoerg commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#discussion_r2547388515)
nit:
```suggestion
std::vector<uint256> merkle_path = TransactionMerklePath(block, /*position=*/0);
```

💬 maflcko commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2547406121)
> Now that #33785 has been merged we hopefully can use `const& ... Assert(` in this PR as long as it's rebased.

The `-Wno-error=dangling-reference` was removed again in https://github.com/bitcoin/bitcoin/pull/33840/files

👍 brunoerg approved a pull request: "test: add unit test coverage for the empty leaves path in MerkleComputation"
(https://github.com/bitcoin/bitcoin/pull/33858#pullrequestreview-3489581346)
code review ACK ffcae82a68104c1992964b26c592b62cbca391bf

It's just a simple test addition to exercise `MerkleComputation` with an empty vector of leaves, it's ok. Note that it just checks the path is empty which is fine since there is a proposal to remove the unused `pmutated` and `proot` (https://github.com/bitcoin/bitcoin/pull/33805).

💬 maflcko commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3559732640)
concept ACK, fwiw

💬 maflcko commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3559735371)
Looks like there are a bunch of code review comments. Are you still working on this?

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

Only removed `const` from `HeadersGeneratorSetup::chain_start` to work around GCC 13 issue since my previous ACK (https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3410905600).

💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2547438257)
Aha, that explains why I was getting other results when not rebasing onto latest master (https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3559658957).

💬 brunoerg commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3559744710)
> Sounds good. So I guess this is up-for-grabs, and a dev with the latest macOS can just update the docs (https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer) with the steps that work with that version?

ACK on doing it.

💬 maflcko commented on pull request "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG":
(https://github.com/bitcoin/bitcoin/pull/33888#issuecomment-3559762513)
Looks like it worked, fwiw: https://github.com/bitcoin/bitcoin/actions/runs/19545916567/job/55966862251#step:6:151:

```
+ '[' 1 = 1 ']'
+ git log HEAD~10 -1 --format=%H
+ git log HEAD~10 -1 --format=%H
+ mapfile -t KEYS
+ git config user.email ci@ci.ci
+ git config user.name ci
+ gpg --keyserver hkps://keys.openpgp.org --recv-keys E777299FC265DD04793070EB944D35F9AC3DB76A D1DBF2C4B96F2DEBF4C16654410108112E7EA81F 152812300785C96444D3334D17565732E08E5E41 6B002C6EA3F91B1B0DF0C9BC8F617F1200
...