💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937527468)
Agreed about `MillisecondsDouble::max()`. Note here that we rely to have such a max value that when we add to it the result is the same max value (not overflow or cause unexpected things).
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937527468)
Agreed about `MillisecondsDouble::max()`. Note here that we rely to have such a max value that when we add to it the result is the same max value (not overflow or cause unexpected things).
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937550966)
Just discussed this with @ismaelsadeeq a bit more.
The hard part is not tracking improvements to the mempool (that is ultimately what all of cluster mempool is, making this explicit), but know which improvements affect the top 1 MvB of the mempool and to what extent. My thinking was that we can't really maintain an index for that, as doing so would be less efficient than just recomputing when it's needed.
But maybe that is not the case. We could keep track at all times of (a) the sum of th
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937550966)
Just discussed this with @ismaelsadeeq a bit more.
The hard part is not tracking improvements to the mempool (that is ultimately what all of cluster mempool is, making this explicit), but know which improvements affect the top 1 MvB of the mempool and to what extent. My thinking was that we can't really maintain an index for that, as doing so would be less efficient than just recomputing when it's needed.
But maybe that is not the case. We could keep track at all times of (a) the sum of th
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937555381)
@ryanofsky do I understand correctly that for the `.capnp` there's no difference when making something an `std::optional`?
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937555381)
@ryanofsky do I understand correctly that for the `.capnp` there's no difference when making something an `std::optional`?
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937566000)
Changed the commit message.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937566000)
Changed the commit message.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937568342)
Changed to "nothing"
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937568342)
Changed to "nothing"
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937577466)
I turned `Assert` into `Assume`. When comparing an std::optional<uint256> with a <uint256> it will be considered unequal if the optional is empty. https://en.cppreference.com/w/cpp/utility/optional/operator_cmp (24)
This would trigger a spurious `tip_changed`. But since it can only happen during shutdown, it's not a problem. And not fatal in any case.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937577466)
I turned `Assert` into `Assume`. When comparing an std::optional<uint256> with a <uint256> it will be considered unequal if the optional is empty. https://en.cppreference.com/w/cpp/utility/optional/operator_cmp (24)
This would trigger a spurious `tip_changed`. But since it can only happen during shutdown, it's not a problem. And not fatal in any case.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937579881)
I adjusted the comment.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937579881)
I adjusted the comment.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2627759233)
Rebased and addressed new feedback.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2627759233)
Rebased and addressed new feedback.
💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627776503)
All green! 🎉
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627776503)
All green! 🎉
💬 stickies-v commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937099597)
nit
```suggestion
# Ccache, is supported only by the Makefile and Ninja generators.
```
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1937099597)
nit
```suggestion
# Ccache, is supported only by the Makefile and Ninja generators.
```
🤔 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
...