💬 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)
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628041793)
> > enable a much simpler and unified approach
>
> This sounds appealing...
To be sure, it is good change, and should be adopted. But unless you are using depends locally it will not affect you at all, and even if you are using depends locally it's just gives some minor quality-of-life improvements, such as you only have to run one make command instead of two if you modify the libmultiprocess subtree locally, and you only have to change flags one place instead of two places if you are chan
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628041793)
> > enable a much simpler and unified approach
>
> This sounds appealing...
To be sure, it is good change, and should be adopted. But unless you are using depends locally it will not affect you at all, and even if you are using depends locally it's just gives some minor quality-of-life improvements, such as you only have to run one make command instead of two if you modify the libmultiprocess subtree locally, and you only have to change flags one place instead of two places if you are chan
...
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628042227)
I think we do have some miscommunication going on, because yes, I don't think we're understanding each other.
I'm suggesting that the `WITH_LIBMULTIPROCESS` option can go away entirely. That means that there are no longer 2 ways to build the multiprocess code. Meaning that everyone gets the same build, same flags, etc. No need to pass tsan for tsan builds, for example. It will just work.
> The only significant differences with your additional change is that flags will be shared between the
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628042227)
I think we do have some miscommunication going on, because yes, I don't think we're understanding each other.
I'm suggesting that the `WITH_LIBMULTIPROCESS` option can go away entirely. That means that there are no longer 2 ways to build the multiprocess code. Meaning that everyone gets the same build, same flags, etc. No need to pass tsan for tsan builds, for example. It will just work.
> The only significant differences with your additional change is that flags will be shared between the
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937755456)
I'd really like this to be easy to use for a Rust client as early as possible. It seems better to revert to the magic value for now.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937755456)
I'd really like this to be easy to use for a Rust client as early as possible. It seems better to revert to the magic value for now.
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628060836)
> To be sure, it is good change, and should be adopted. But unless you are using depends locally it will not affect you at all, and even if you are using depends locally it's just gives some minor quality-of-life improvements, such as you only have to run one make command instead of two if you modify the libmultiprocess subtree locally, and you only have to change flags one place instead of two places if you are changing flags and care whether the flags are apply to libmultiprocess runtime libra
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628060836)
> To be sure, it is good change, and should be adopted. But unless you are using depends locally it will not affect you at all, and even if you are using depends locally it's just gives some minor quality-of-life improvements, such as you only have to run one make command instead of two if you modify the libmultiprocess subtree locally, and you only have to change flags one place instead of two places if you are changing flags and care whether the flags are apply to libmultiprocess runtime libra
...
💬 fjahr commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-2628068442)
> Reading through this issue, it seems we are either waiting for bitcoin core to upgrade to using libevent 2.2.1, or we are waiting for libevent to be removed entirely as a dependency.
I would open this PR for review if libevent 2.2.1 was available but it's in Alpha since May 2023: https://github.com/libevent/libevent/releases So upgrading libevent isn't really an option yet but the removal is in the works, see #31194
> However, in the interim it would be really very helpful if, instead of
...
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-2628068442)
> Reading through this issue, it seems we are either waiting for bitcoin core to upgrade to using libevent 2.2.1, or we are waiting for libevent to be removed entirely as a dependency.
I would open this PR for review if libevent 2.2.1 was available but it's in Alpha since May 2023: https://github.com/libevent/libevent/releases So upgrading libevent isn't really an option yet but the removal is in the works, see #31194
> However, in the interim it would be really very helpful if, instead of
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937772174)
The assume is correct, but the assert was a bit overzealous. `waitNext()` is a method on `BlockTemplate`. You can't generate a template before there's a tip, so it's impossible for this fail.
It's possible that the node is shutdown when `TipBlock()` still returns `std::nullopt`, but in that scenario there can't be a `BlockTemplate`. The value doesn't get unset during shutdown.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937772174)
The assume is correct, but the assert was a bit overzealous. `waitNext()` is a method on `BlockTemplate`. You can't generate a template before there's a tip, so it's impossible for this fail.
It's possible that the node is shutdown when `TipBlock()` still returns `std::nullopt`, but in that scenario there can't be a `BlockTemplate`. The value doesn't get unset during shutdown.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937777590)
Taken (with some changes)
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937777590)
Taken (with some changes)
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937779528)
Got it.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937779528)
Got it.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2628133425)
I went back to using the `MAX_MONEY` magic value and added a helper `check_tip_changed()` to make sure we check for a tip update _before_ waiting.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2628133425)
I went back to using the `MAX_MONEY` magic value and added a helper `check_tip_changed()` to make sure we check for a tip update _before_ waiting.