💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937500659)
It's probably fast enough that that can be done as an add-on, updated after every mempool change, but limited to every N milliseconds or so.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937500659)
It's probably fast enough that that can be done as an add-on, updated after every mempool change, but limited to every N milliseconds or so.
💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937503273)
@sipa, I imagine that an RBF over any transaction in the last built block template be an indicator that the previously built template improve.
And since we would be using fee rate diagram for RBF's after cluster mempool, we could use use RBF's as determinant?
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937503273)
@sipa, I imagine that an RBF over any transaction in the last built block template be an indicator that the previously built template improve.
And since we would be using fee rate diagram for RBF's after cluster mempool, we could use use RBF's as determinant?
💬 sr-gi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937515621)
I don't have a strong opinion on this.
I've checked and looks like no other test uses this, or re-defines it. I'd lean towards leaving it locally to avoid bloating `p2p.py`, but I'm open to move it if there is support for it
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937515621)
I don't have a strong opinion on this.
I've checked and looks like no other test uses this, or re-defines it. I'd lean towards leaving it locally to avoid bloating `p2p.py`, but I'm open to move it if there is support for it
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937516539)
You mean "never generate a template" when run from the tests, right? Why is that? The default timeout is `inf` so `now < deadline` should be `true`?
> So it has to use <=, but without the break here the test will freeze, because time doesn't move.
Right, but it is not too difficult to advance the time in unit tests.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937516539)
You mean "never generate a template" when run from the tests, right? Why is that? The default timeout is `inf` so `now < deadline` should be `true`?
> So it has to use <=, but without the break here the test will freeze, because time doesn't move.
Right, but it is not too difficult to advance the time in unit tests.
💬 tdb3 commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937519743)
Works for me. If/when the file is touched again, it could be moved then.
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937519743)
Works for me. If/when the file is touched again, it could be moved then.
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937522651)
Functionality-wise I think the behavior of the production code is ok. I am concerned about its readability and maintainability. It just looks confusing (to me).
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937522651)
Functionality-wise I think the behavior of the production code is ok. I am concerned about its readability and maintainability. It just looks confusing (to me).
💬 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?