💬 m3dwards commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249732228)
Further, a lot of the YAML is actually setting things up like caching which is not only essential for reasonable performance but also is vendor specific and so couldn't really exist in reproducible CI scripts either written as they are or in cmake.
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249732228)
Further, a lot of the YAML is actually setting things up like caching which is not only essential for reasonable performance but also is vendor specific and so couldn't really exist in reproducible CI scripts either written as they are or in cmake.
🤔 ismaelsadeeq reviewed a pull request: "build: suggest -DENABLE_IPC=OFF when missing capnp"
(https://github.com/bitcoin/bitcoin/pull/33290#pullrequestreview-3181213833)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33290#pullrequestreview-3181213833)
Concept ACK
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3249777657)
> To complete migration from self-hosted to hosted for this repo, the backport branches `27.x`, `28.x` and `29.x` would also need their CI ported, but these are left for followups to this change (and pending review/changes here first).
27.x looks close to EOL in one month, according to https://bitcoincore.org/en/lifecycle/, so I guess it could just die with the old CI?
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3249777657)
> To complete migration from self-hosted to hosted for this repo, the backport branches `27.x`, `28.x` and `29.x` would also need their CI ported, but these are left for followups to this change (and pending review/changes here first).
27.x looks close to EOL in one month, according to https://bitcoincore.org/en/lifecycle/, so I guess it could just die with the old CI?
💬 fanquake commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3249782168)
Yea. I think it's fine to leave `27.x` as is, this just needs backporting to `29.x` & `28.x`.
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3249782168)
Yea. I think it's fine to leave `27.x` as is, this just needs backporting to `29.x` & `28.x`.
📝 fanquake opened a pull request: "[29.x] *san CI backports"
(https://github.com/bitcoin/bitcoin/pull/33294)
Backports:
* #32999
* #33099
* #33258
(https://github.com/bitcoin/bitcoin/pull/33294)
Backports:
* #32999
* #33099
* #33258
💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319395069)
"enabled" feels a bit like it can change at runtime, but this is a check about whether it has been compiled in, so I think the name is correct
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319395069)
"enabled" feels a bit like it can change at runtime, but this is a check about whether it has been compiled in, so I think the name is correct
💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319397753)
> currently
I don't think it will ever be possible to run them with tidy, also it doesn't make sense to do?
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319397753)
> currently
I don't think it will ever be possible to run them with tidy, also it doesn't make sense to do?
💬 purpleKarrot commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3249812522)
Where is it defined or documented why `CMAKE_POSITION_INDEPENDENT_CODE` is set to `ON` in the first place?
A better approach would be to not override user preferences at all and set the `POSITION_INDEPENDENT_CODE` target property explicitly on targets that need it.
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3249812522)
Where is it defined or documented why `CMAKE_POSITION_INDEPENDENT_CODE` is set to `ON` in the first place?
A better approach would be to not override user preferences at all and set the `POSITION_INDEPENDENT_CODE` target property explicitly on targets that need it.
💬 maflcko commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3249828430)
lgtm
Should be fine either way and shouldn't matter much, because the set of devs without capnp doing fuzzing is limited
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3249828430)
lgtm
Should be fine either way and shouldn't matter much, because the set of devs without capnp doing fuzzing is limited
💬 purpleKarrot commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249833121)
> Further, a lot of the YAML is actually setting things up like caching which is not only essential for reasonable performance but also is vendor specific and so couldn't really exist in reproducible CI scripts either written as they are or in cmake.
I don't see how setting up a cache could require YAML in a way that is not possible to reproduce in the cmake language. I explicitly covered caching in my presentation.
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249833121)
> Further, a lot of the YAML is actually setting things up like caching which is not only essential for reasonable performance but also is vendor specific and so couldn't really exist in reproducible CI scripts either written as they are or in cmake.
I don't see how setting up a cache could require YAML in a way that is not possible to reproduce in the cmake language. I explicitly covered caching in my presentation.
💬 purpleKarrot commented on issue "CI: Cmake warnings should be errors":
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-3249855202)
> It is not possible for errors to exist, because pull requests will not be merged when errors exist in the CI.
This is just a matter of configuration. The same mechanism that prevents merging when errors exist could be used to prevent merging when warnings exist.
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-3249855202)
> It is not possible for errors to exist, because pull requests will not be merged when errors exist in the CI.
This is just a matter of configuration. The same mechanism that prevents merging when errors exist could be used to prevent merging when warnings exist.
💬 stickies-v commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3249932464)
> A better approach would be to not override user preferences at all
Thanks for the review. I think I agree. I already tried to improve that behaviour by not overriding `CMAKE_POSITION_INDEPENDENT_CODE` when it was set by the user, but it seems better to not change it at all, as you say.
I've force-pushed to use our existing `core_interface` to propagate `INTERFACE_POSITION_INDEPENDENT_CODE` to all our targets, since I think the current behaviour of using PIE for all targets is sensible. W
...
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3249932464)
> A better approach would be to not override user preferences at all
Thanks for the review. I think I agree. I already tried to improve that behaviour by not overriding `CMAKE_POSITION_INDEPENDENT_CODE` when it was set by the user, but it seems better to not change it at all, as you say.
I've force-pushed to use our existing `core_interface` to propagate `INTERFACE_POSITION_INDEPENDENT_CODE` to all our targets, since I think the current behaviour of using PIE for all targets is sensible. W
...
💬 purpleKarrot commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3249972507)
> I think the current behaviour of using PIE for all targets is sensible
I am not convinced and I really would like to know the rationale for that. Maybe @hebasto or @theuni knows?
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3249972507)
> I think the current behaviour of using PIE for all targets is sensible
I am not convinced and I really would like to know the rationale for that. Maybe @hebasto or @theuni knows?
💬 mzumsande commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3249980322)
I see unconditional log messages `[net:warning] pcp: Could not receive response: Connection refused (111)` every 5 minutes (in an environment where PCP is expected to not work), and it's a bit spammy. How about a exponential backoff, similar to how it's done in `torcontrol.cpp`?
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3249980322)
I see unconditional log messages `[net:warning] pcp: Could not receive response: Connection refused (111)` every 5 minutes (in an environment where PCP is expected to not work), and it's a bit spammy. How about a exponential backoff, similar to how it's done in `torcontrol.cpp`?
💬 darosior commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3250010277)
Good idea. I got annoyed by it as well, and @instagibbs also mentions it above.
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3250010277)
Good idea. I got annoyed by it as well, and @instagibbs also mentions it above.
💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319596500)
It looks like the maintainer wants to be replaced. Since people have been opening pull requests, hopefully they'll find a replacement.
So far `pycapnp` seems to work fine up to Python 3.12 and we support a minimum of 3.10, so we have some time.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319596500)
It looks like the maintainer wants to be replaced. Since people have been opening pull requests, hopefully they'll find a replacement.
So far `pycapnp` seems to work fine up to Python 3.12 and we support a minimum of 3.10, so we have some time.
💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319606567)
Right, we use _enable_ in cmake to decide if we want to _compile_. The test suite care about whether it was compiled.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319606567)
Right, we use _enable_ in cmake to decide if we want to _compile_. The test suite care about whether it was compiled.
💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319612545)
I don't think we can remove this, because the `import` has to be at the top of the file and the `skip_if_no_py_capnp` happens later.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2319612545)
I don't think we can remove this, because the `import` has to be at the top of the file and the `skip_if_no_py_capnp` happens later.
💬 Sjors commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3250085705)
I'm currently trying a time-machine at `f3e74ef25d479ed77f7cc029d312425b7776c3b0` (`gnu: clisp: Actually fix failing test`, see https://lists.gnu.org/archive/html/guix-patches/2025-05/msg00088.html - page currently doesn't load for me). I'll check if that gets me past the test failure and produces working binaries.
I still don't know how to install an individual stubborn package like this from a substitute server, maybe @dongcarl does?
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3250085705)
I'm currently trying a time-machine at `f3e74ef25d479ed77f7cc029d312425b7776c3b0` (`gnu: clisp: Actually fix failing test`, see https://lists.gnu.org/archive/html/guix-patches/2025-05/msg00088.html - page currently doesn't load for me). I'll check if that gets me past the test failure and produces working binaries.
I still don't know how to install an individual stubborn package like this from a substitute server, maybe @dongcarl does?
💬 ajtowns commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3250125812)
Backport at https://github.com/ajtowns/bitcoin/tree/202509-29x-backport-compactblock ; don't think it's urgent enough to try pushing for 29.2 though?
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3250125812)
Backport at https://github.com/ajtowns/bitcoin/tree/202509-29x-backport-compactblock ; don't think it's urgent enough to try pushing for 29.2 though?