💬 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.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937808429)
I added check before we start the wait.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937808429)
I added check before we start the wait.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937809044)
If that happens we'll catch it at the start of the next loop, with the above change.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937809044)
If that happens we'll catch it at the start of the next loop, with the above change.
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628139870)
> I'm suggesting that the `WITH_LIBMULTIPROCESS` option can go away entirely.
If you look at the PR, you can see that `WITH_LIBMULTIPROCESS` option adds 5 lines of code and 2 of them are `endif()`. So I wouldn't consider that a real coup. Also I don't think `WITH_LIBMULTIPROCESS` should be dropped just because depends will not use it, because having it makes it easier to develop and submit changes to the upstream repository, and makes sense to support other packaging systems (see https://gith
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2628139870)
> I'm suggesting that the `WITH_LIBMULTIPROCESS` option can go away entirely.
If you look at the PR, you can see that `WITH_LIBMULTIPROCESS` option adds 5 lines of code and 2 of them are `endif()`. So I wouldn't consider that a real coup. Also I don't think `WITH_LIBMULTIPROCESS` should be dropped just because depends will not use it, because having it makes it easier to develop and submit changes to the upstream repository, and makes sense to support other packaging systems (see https://gith
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937818071)
`m_tip_block_mutex` and `cs_main` locks don't jive well, so I limited the places where we actually check
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937818071)
`m_tip_block_mutex` and `cs_main` locks don't jive well, so I limited the places where we actually check
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937834069)
A comment to remember this.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937834069)
A comment to remember this.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937839814)
It would require a second thread, because the main thread of the unit test is waiting on `waitNext()`.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937839814)
It would require a second thread, because the main thread of the unit test is waiting on `waitNext()`.
💬 achow101 commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2628184000)
ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2628184000)
ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9
💬 achow101 commented on pull request "test: Added coverage to the waitfornewblock rpc":
(https://github.com/bitcoin/bitcoin/pull/31746#issuecomment-2628229678)
ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2
(https://github.com/bitcoin/bitcoin/pull/31746#issuecomment-2628229678)
ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2
🚀 achow101 merged a pull request: "rpc: have getblocktemplate mintime account for timewarp"
(https://github.com/bitcoin/bitcoin/pull/31600)
(https://github.com/bitcoin/bitcoin/pull/31600)