💬 l0rinc commented on pull request "random: scope environ extern to macOS, BSDs and Illumos":
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3486434684)
ACK 79d6f458e2300e1f47b94467cda233e1c761f8be

Thanks @hebasto for checking the other platforms.

💬 ismaelsadeeq commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486443571)
reACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 👾

There have been numerous changes to the header API since my last ACK.
I've reviewed the changes using the [compare diff](https://github.com/bitcoin/bitcoin/compare/a755e00a13541b3b5a707cf385f1cbec0449c6a9..6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969)

<details>
<summary>Changes</summary>


* Fixed typo in `Write`
* `LoggingConnection` now does not take Logging Options; a clear error message is returned or thrown when the logger connectio
...

⚠️ Sjors opened an issue: "Mining interface tracking issue"
(https://github.com/bitcoin/bitcoin/issues/33777)
The mining interface is defined in:
- https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/mining.h
- https://github.com/bitcoin/bitcoin/blob/master/src/ipc/capnp/mining.capnp

Clients that are known to depend on it:
- https://github.com/stratum-mining/sv2-tp

At this state it's OK to make breaking changes, such as deleting a method or changing its signature. But ideally we should do so once in a major release, collecting a bunch of fixes.

Non-breaking changes can be made any time, suc
...

🚀 hebasto merged a pull request: "random: scope environ extern to macOS, BSDs and Illumos"
(https://github.com/bitcoin/bitcoin/pull/33714)

💬 fanquake commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486609787)
ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 - soon we'll be running bitcoin (kernel)

💬 theStack commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3486623443)
Concept ACK

Nit for the future: putting the move-only changes on the existing code (i.e. introduction of the `validate_snapshot_import` and `complete_background_validation` methods) into an own refactoring commit would make reviewing this a bit easier, I think.

🚀 fanquake merged a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595)

💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3486709813)
reACK a1df8547ca0711d99f05d1b31bc758a8068ba512

It's quite a heavy refactor, I did my best to review all the changes. Someone else should also review it to be sure.
The latest https://corecheck.dev/bitcoin/bitcoin/pulls/31713 Sonar warnings seem like false positives.

💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3486739651)
Thanks @ryanofsky, given that this is in review for more than 1 year, I will address your concerns in a follow-up.
rfm?

💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3486744593)
Thanks for the review @ryanofsky and the coverage report @brunoerg

I will update the PR shortly with the suggestions and study the report.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2491098502)
In 1a844b8ffd351d14b59de4cbae99b6e639068bb9 _bitcoin-mine: Extend example to mine a block_: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3486757229)
re-ACK a2365f130c36f386caa8470528f39a34ac394798

Since my last review of e1f139b, commits 8bf2269efa and b094d38454 were absorbed into #33201 which added direct coverage of IPC via a functional test. This also allowed some simplification of `test/functional/interface_ipc_mining.py` which this PR adds.

I initially wanted to suggest folding this test into `interface_ipc.py`, but the latter has a dependency on PyCap.

💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3486760754)
@151henry151

Sorry, but one more rebase is needed.

💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486783430)
post-merge ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969

💬 brunoerg commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2491199977)
084bfbc1ec7f8f64f54d231bb641285622311b59: nit: you can get rid of `num_iterations`.
```suggestion
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10) {
```

💬 stringintech commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#issuecomment-3486895544)
3e7d272 to 67740df - rebased and resolved conflict with #30595.

📝 fanquake opened a pull request: "depends: add zeromq patch to fix mingw CMake file install location"
(https://github.com/bitcoin/bitcoin/pull/33778)
Currently the zeromq CMake files for mingw are installed to `x86_64-w64-mingw32/CMake`, when they should be in `x86_64-w64-mingw32/lib/cmake`, so take the patch from upstream that puts them in the right place. Noticed this while looking at other depends changes, that might cleanup / remove dirs.

💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3486939962)
I think we've never seen a header-version specific bug in the windows-cross compilation, so trying to fit the major versions in the CI on a best-effort basis seems sufficient for now? If more specific matching is needed for better testing a specific scenario, or for llvm builds, it seems sufficient to do as a follow-up?

👍 theStack approved a pull request: "test: add option to skip large re-org test in feature_block"
(https://github.com/bitcoin/bitcoin/pull/33003#pullrequestreview-3417493966)
Concept and code-review ACK 8810642b571e1d8482375e962a1129b691d5d226

💬 theStack commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#discussion_r2491234209)
nit: this part could be moved inside the if-block below as well, as there is no need to disable v2transport if the re-org is skipped (though it currently doesn't matter much, as re-orging is the last part of the functional tests anyway)