🤔 stickies-v reviewed a pull request: "build: Enhance Ccache performance across worktrees and build trees"
(https://github.com/bitcoin/bitcoin/pull/30861#pullrequestreview-2586373527)
Definitely in favour of increasing ccache effectiveness, but I'm still figuring out if this approach is optimal. Is my understanding correct that with this PR, all we're doing is forcing the `CCACHE_NOHASHDIR=1` environment variable, instead of letting the user set it?
While not unreasonable, it does seem to make it difficult for the user to undo this (without disabling ccache entirely, which also isn't ideal), and I'm not sure if this wouldn't break any workflows? If my understanding is corr
...
(https://github.com/bitcoin/bitcoin/pull/30861#pullrequestreview-2586373527)
Definitely in favour of increasing ccache effectiveness, but I'm still figuring out if this approach is optimal. Is my understanding correct that with this PR, all we're doing is forcing the `CCACHE_NOHASHDIR=1` environment variable, instead of letting the user set it?
While not unreasonable, it does seem to make it difficult for the user to undo this (without disabling ccache entirely, which also isn't ideal), and I'm not sure if this wouldn't break any workflows? If my understanding is corr
...
💬 stickies-v commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937585908)
Perhaps this is obvious in CMake land, but I was confused by what `${CMAKE_COMMAND} -E env` is doing. I would have appreciated a docstring like the below:
> # Use `cmake -E env` as a cross-platform way to set the CCACHE_NOHASHDIR environment variable, increasing
> # ccache hit rate e.g. in case of multiple build directories or git worktrees.
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937585908)
Perhaps this is obvious in CMake land, but I was confused by what `${CMAKE_COMMAND} -E env` is doing. I would have appreciated a docstring like the below:
> # Use `cmake -E env` as a cross-platform way to set the CCACHE_NOHASHDIR environment variable, increasing
> # ccache hit rate e.g. in case of multiple build directories or git worktrees.
💬 stickies-v commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937227612)
I think we could drop this check entirely? As per [`<LANG>_COMPILER_LAUNCHER`](https://cmake.org/cmake/help/latest/prop_tgt/LANG_COMPILER_LAUNCHER.html#prop_tgt:%3CLANG%3E_COMPILER_LAUNCHER):
> The [Makefile Generators](https://cmake.org/cmake/help/latest/manual/cmake-generators.7.html#makefile-generators) and the [Ninja](https://cmake.org/cmake/help/latest/generator/Ninja.html#generator:Ninja) generator will run this tool and pass the compiler and its arguments to the tool.
If the genera
...
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937227612)
I think we could drop this check entirely? As per [`<LANG>_COMPILER_LAUNCHER`](https://cmake.org/cmake/help/latest/prop_tgt/LANG_COMPILER_LAUNCHER.html#prop_tgt:%3CLANG%3E_COMPILER_LAUNCHER):
> The [Makefile Generators](https://cmake.org/cmake/help/latest/manual/cmake-generators.7.html#makefile-generators) and the [Ninja](https://cmake.org/cmake/help/latest/generator/Ninja.html#generator:Ninja) generator will run this tool and pass the compiler and its arguments to the tool.
If the genera
...
💬 stickies-v commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937579582)
This does affect us wrt debug info. With this change, if you first compile in `tree1`, and then in `tree2`, inspecting the `.o` files in `tree2` will reference the source files in `tree1`. I don't know which, if any, use cases depend on that debug info being correct, but it's potentially annoying at least?
Because env vars take precedence over config files, and because we're appending this, I don't think users would be able to undo this if they need correct debug info?
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937579582)
This does affect us wrt debug info. With this change, if you first compile in `tree1`, and then in `tree2`, inspecting the `.o` files in `tree2` will reference the source files in `tree1`. I don't know which, if any, use cases depend on that debug info being correct, but it's potentially annoying at least?
Because env vars take precedence over config files, and because we're appending this, I don't think users would be able to undo this if they need correct debug info?
💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2627790028)
#30975 now passes, using a bunch of patches that will make their way here.
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2627790028)
#30975 now passes, using a bunch of patches that will make their way here.
💬 marcofleon commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#issuecomment-2627817524)
reACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b
Changes since I last reviewed:
1. Consolidating `AddOrphanParentAnnoucements` and `OrphanResolutionCandidate`. Checked that the logic ends up being the same with the updated version.
2. Making the `txrequest` fuzz target more robust. I ran it for a bit to be sure.
I'm also neutral on the `AddChildrenToWorkSet` decision for now. Maybe it's something that might be included/changed with a future orphan handling PR?
(https://github.com/bitcoin/bitcoin/pull/31666#issuecomment-2627817524)
reACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b
Changes since I last reviewed:
1. Consolidating `AddOrphanParentAnnoucements` and `OrphanResolutionCandidate`. Checked that the logic ends up being the same with the updated version.
2. Making the `txrequest` fuzz target more robust. I ran it for a bit to be sure.
I'm also neutral on the `AddChildrenToWorkSet` decision for now. Maybe it's something that might be included/changed with a future orphan handling PR?
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937622086)
> @ryanofsky do I understand correctly that for the `.capnp` there's no difference when making something an `std::optional`?
It depends on the type. In capnproto schemas primitive fields (numbers, bools, enums, unions) are not optional and are always present. If these fields are not assigned values they are just initialized to 0. So to represent a std::optional\<T> value where T is a primitive you have to change the capnproto schema to be able to represent std::nullopt differently somehow, un
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937622086)
> @ryanofsky do I understand correctly that for the `.capnp` there's no difference when making something an `std::optional`?
It depends on the type. In capnproto schemas primitive fields (numbers, bools, enums, unions) are not optional and are always present. If these fields are not assigned values they are just initialized to 0. So to represent a std::optional\<T> value where T is a primitive you have to change the capnproto schema to be able to represent std::nullopt differently somehow, un
...
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937644528)
In commit "miner: have waitNext return after 20 min on testnet" (22cf6cdb4190482b486752dae4e9dc3b44cfcb1e):
Note for a possible followup. I think this representation of the options struct should work fine for now as long as the client initializing the BlockWaitOptions struct is a c++ client using the libmultiprocess library.
However, this representation of the options struct might not be the most convenient for other clients, like a possible rust client, because by default these fields wil
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937644528)
In commit "miner: have waitNext return after 20 min on testnet" (22cf6cdb4190482b486752dae4e9dc3b44cfcb1e):
Note for a possible followup. I think this representation of the options struct should work fine for now as long as the client initializing the BlockWaitOptions struct is a c++ client using the libmultiprocess library.
However, this representation of the options struct might not be the most convenient for other clients, like a possible rust client, because by default these fields wil
...
💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627878471)
> All green! 🎉
This is nice! But I am surprised to see clang-tidy build succeeding. There were so much other clang-tidy output in the previous failing job aside from the missing input file errors fixed by the cmake change: https://cirrus-ci.com/task/4607289014878208?logs=ci#L7174. But I guess all the other new clang-tidy output was warnings not errors, so they don't cause the job to fail. Still planning on cleaning up the warnings anyway since I already fixed a lot of them locally.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627878471)
> All green! 🎉
This is nice! But I am surprised to see clang-tidy build succeeding. There were so much other clang-tidy output in the previous failing job aside from the missing input file errors fixed by the cmake change: https://cirrus-ci.com/task/4607289014878208?logs=ci#L7174. But I guess all the other new clang-tidy output was warnings not errors, so they don't cause the job to fail. Still planning on cleaning up the warnings anyway since I already fixed a lot of them locally.
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2627938830)
> @theuni can the change your are working on be a followup? It doesn't seem like this change will have any effect for developers unless they are using depends for development which I think few do normally. And even then the main effect is that the library is built with cmake flags instead of depends flags. The change would also increase complexity (would only increase not decrease changes in this PR) and would be easier to review and discuss separately.
Sorry, I just don't think this makes se
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2627938830)
> @theuni can the change your are working on be a followup? It doesn't seem like this change will have any effect for developers unless they are using depends for development which I think few do normally. And even then the main effect is that the library is built with cmake flags instead of depends flags. The change would also increase complexity (would only increase not decrease changes in this PR) and would be easier to review and discuss separately.
Sorry, I just don't think this makes se
...
🤔 stickies-v reviewed a pull request: "cmake: Install man pages for configured targets only"
(https://github.com/bitcoin/bitcoin/pull/31765#pullrequestreview-2587430487)
Approach ACK 5f6680769f017c9e12801dfa7af7d6568e53bdbe, thanks for addressing my issue!
Unfamiliar enough with CMake to determine if this is the best approach, but it's elegant and as per my testing it works well (except for my `component` comment).
(https://github.com/bitcoin/bitcoin/pull/31765#pullrequestreview-2587430487)
Approach ACK 5f6680769f017c9e12801dfa7af7d6568e53bdbe, thanks for addressing my issue!
Unfamiliar enough with CMake to determine if this is the best approach, but it's elegant and as per my testing it works well (except for my `component` comment).
💬 stickies-v commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#discussion_r1937696640)
I think we should add an optional `component` parameter. This would address the issue (see below) of installing just `GUI`, and also aligns well with potentially adding more components (e.g. one per target) in the future (out of scope for this PR).
<details>
<summary>git diff on 5f6680769f</summary>
```diff
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index eb816dbacc..ea026c25be 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -172,10 +172,11 @@ target_link_librar
...
(https://github.com/bitcoin/bitcoin/pull/31765#discussion_r1937696640)
I think we should add an optional `component` parameter. This would address the issue (see below) of installing just `GUI`, and also aligns well with potentially adding more components (e.g. one per target) in the future (out of scope for this PR).
<details>
<summary>git diff on 5f6680769f</summary>
```diff
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index eb816dbacc..ea026c25be 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -172,10 +172,11 @@ target_link_librar
...
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937718497)
re: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937644528
Actually never mind all of this, I am completely wrong. Capnproto **does** support setting default values, using syntax shown https://capnproto.org/language.html#structs, but for some reason I seemed to have a memory of it not supporting them. (It supports default values using a xor trick described https://capnproto.org/encoding.html#default-values).
So recommendation here could be to set default values in the capnpro
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937718497)
re: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937644528
Actually never mind all of this, I am completely wrong. Capnproto **does** support setting default values, using syntax shown https://capnproto.org/language.html#structs, but for some reason I seemed to have a memory of it not supporting them. (It supports default values using a xor trick described https://capnproto.org/encoding.html#default-values).
So recommendation here could be to set default values in the capnpro
...
💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628025117)
> enable a much simpler and unified approach
This sounds appealing...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628025117)
> enable a much simpler and unified approach
This sounds appealing...
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937739579)
Is it possible that `notifications.TipBlock()` has changed already, before the `m_tip_block_mutex` lock is grabbed? If so, we'd wait up to a full second before noticing?
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937739579)
Is it possible that `notifications.TipBlock()` has changed already, before the `m_tip_block_mutex` lock is grabbed? If so, we'd wait up to a full second before noticing?
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937741741)
Similarly here, is it possible the tip block has changed after the 1s wait above expired, but before `cs_main` is grabbed above? This could be easily detect by `tip_changed || block_template->block.hashPrevBlock != m_block_template->block.hashPrevBlock` here (the `tip_changed ||` is perhaps not even necessary?).
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937741741)
Similarly here, is it possible the tip block has changed after the 1s wait above expired, but before `cs_main` is grabbed above? This could be easily detect by `tip_changed || block_template->block.hashPrevBlock != m_block_template->block.hashPrevBlock` here (the `tip_changed ||` is perhaps not even necessary?).
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937600509)
(In case the `Assume()` above is correct, see my other comment).
It seems wasteful to call `TipBlock()` twice here. I'd suggest:
```c++
auto tip_block = notifications().TipBlock();
Assume(tip_block);
tip_changed = m_block_template->block.hashPrevBlock;
return tip_changed || chainman().m_interrupt;
```
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937600509)
(In case the `Assume()` above is correct, see my other comment).
It seems wasteful to call `TipBlock()` twice here. I'd suggest:
```c++
auto tip_block = notifications().TipBlock();
Assume(tip_block);
tip_changed = m_block_template->block.hashPrevBlock;
return tip_changed || chainman().m_interrupt;
```
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937596836)
The earlier discussion is unclear to me. Is it possible that `TipBlock()` returns `std::nullopt` under non-error conditions (including shutdown)? If so, there should not be an `Assume()` here. Those are for conditions we believe are not possible if the code is correct.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937596836)
The earlier discussion is unclear to me. Is it possible that `TipBlock()` returns `std::nullopt` under non-error conditions (including shutdown)? If so, there should not be an `Assume()` here. Those are for conditions we believe are not possible if the code is correct.
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628029322)
> So I don't know why we'd work on an approach here that we plan to change significantly later, as opposed to just fixing it up and using the simpler approach to begin with.
I might be misunderstanding but from everything I've seen, the change you are implementing is a superset of this change. That is why I am suggesting it could be a followup. No part of this change would be reverted or simplified by the additional work you are doing so it is a natural follow-up.
I don't think devs are ty
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628029322)
> So I don't know why we'd work on an approach here that we plan to change significantly later, as opposed to just fixing it up and using the simpler approach to begin with.
I might be misunderstanding but from everything I've seen, the change you are implementing is a superset of this change. That is why I am suggesting it could be a followup. No part of this change would be reverted or simplified by the additional work you are doing so it is a natural follow-up.
I don't think devs are ty
...
💬 Sjors commented on pull request "Pass custom DEP_OPTS and CONFIG_FLAGS to guix-build":
(https://github.com/bitcoin/bitcoin/pull/31763#issuecomment-2628038425)
A couple fights with bash and the linter later... (ok `${DEP_OPTS:+"$DEP_OPTS"}` instead of just `$DEP_OPTS`, fine)
(https://github.com/bitcoin/bitcoin/pull/31763#issuecomment-2628038425)
A couple fights with bash and the linter later... (ok `${DEP_OPTS:+"$DEP_OPTS"}` instead of just `$DEP_OPTS`, fine)