💬 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)
(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.
(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)
(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.
(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?
(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.
(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 :-)
(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.
(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.
(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
(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) {
```
(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.
(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.
(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?
(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
(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)
(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)
💬 hebasto commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486958734)
The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486958734)
The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
💬 hebasto commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486967777)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486967777)
Concept ACK.
💬 fanquake commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486969063)
> The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
Could you clarify. Reading those docs, `<prefix>/(cmake|CMake)/` is only valid on Windows installation trees. Not Unix, which is what is being changed here?
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486969063)
> The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
Could you clarify. Reading those docs, `<prefix>/(cmake|CMake)/` is only valid on Windows installation trees. Not Unix, which is what is being changed here?
💬 hebasto commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486973223)
> > The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
>
> Could you clarify. Reading those docs, `<prefix>/(cmake|CMake)/` is only valid on Windows installation trees. Not Unix, which is what is being changed here?
You're right. Already updated my comment.
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3486973223)
> > The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
>
> Could you clarify. Reading those docs, `<prefix>/(cmake|CMake)/` is only valid on Windows installation trees. Not Unix, which is what is being changed here?
You're right. Already updated my comment.