💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535106877)
Missed to fully answer your previous comment related to this change, sorry. I didn't do it to not include `setup_common.h`, as it comes with way more dependencies than just `HasReason()`.
The idea is to try to reduce large Bitcoin-Core repo dependencies in this low level class, mainly when they do not add much value (this one just let us remove two lines of code with no behavior change), so this class and tests can be easily used elsewhere.
We could move `HasReason()` to another test utils f
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535106877)
Missed to fully answer your previous comment related to this change, sorry. I didn't do it to not include `setup_common.h`, as it comes with way more dependencies than just `HasReason()`.
The idea is to try to reduce large Bitcoin-Core repo dependencies in this low level class, mainly when they do not add much value (this one just let us remove two lines of code with no behavior change), so this class and tests can be easily used elsewhere.
We could move `HasReason()` to another test utils f
...
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3543318128)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
I left a few nits and follow-up ideas, but the code is correct as it is, every suggestion can be applied in a later PR.
Especially since making the methods `static` enables eliminating a few parameters along the call chain - which are orthogonal to this change.
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3543318128)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
I left a few nits and follow-up ideas, but the code is correct as it is, every suggestion can be applied in a later PR.
Especially since making the methods `static` enables eliminating a few parameters along the call chain - which are orthogonal to this change.
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534906456)
> nor does BlockManager instance retain references to the parameters
First I though we might as well make it `const` to add more weight to the claim that `LIFETIMEBOUND` isn't needed here:
```suggestion
bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
```
But it turns out the internally called `GetFirstBlock` can actually be `static` (which would document `LIFETIMEBOUND` a lot better, since w
...
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534906456)
> nor does BlockManager instance retain references to the parameters
First I though we might as well make it `const` to add more weight to the claim that `LIFETIMEBOUND` isn't needed here:
```suggestion
bool CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
```
But it turns out the internally called `GetFirstBlock` can actually be `static` (which would document `LIFETIMEBOUND` a lot better, since w
...
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534972779)
given that call-site `CHECK_NONFATAL`, can we simplify this to:
```suggestion
return *Assert(last_block);
```
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534972779)
given that call-site `CHECK_NONFATAL`, can we simplify this to:
```suggestion
return *Assert(last_block);
```
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534940354)
Good find, this parameter was added in https://github.com/bitcoin/bitcoin/pull/27607/files#diff-ed3f90693a242b38b9719af171de8f55183576957676dfa358945bea22276bd5R232 where [`lower_block`](https://github.com/bitcoin/bitcoin/pull/27607/files#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR413) was already a possible return value (cc: @furszy, @ryanofsky, @TheCharlatan, @mzumsande)
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2534940354)
Good find, this parameter was added in https://github.com/bitcoin/bitcoin/pull/27607/files#diff-ed3f90693a242b38b9719af171de8f55183576957676dfa358945bea22276bd5R232 where [`lower_block`](https://github.com/bitcoin/bitcoin/pull/27607/files#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR413) was already a possible return value (cc: @furszy, @ryanofsky, @TheCharlatan, @mzumsande)
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535004423)
nit: `first_available_block` is also a non-nullable reference, please consider this alternative if you touch again:
```C++
CBlockIndex& first_available_block = *chainman->ActiveChain()[height_to_prune + 1];
CBlockIndex* last_pruned_block = first_available_block.pprev;
func_prune_blocks(last_pruned_block);
// 3) The last block not pruned is in-between upper-block and the genesis block
BOOST_CHECK_EQUAL(&BlockManager::GetFirstBlock(tip, BLOCK_HAVE_DATA), &first_available_block);
BOOST_CHE
...
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535004423)
nit: `first_available_block` is also a non-nullable reference, please consider this alternative if you touch again:
```C++
CBlockIndex& first_available_block = *chainman->ActiveChain()[height_to_prune + 1];
CBlockIndex* last_pruned_block = first_available_block.pprev;
func_prune_blocks(last_pruned_block);
// 3) The last block not pruned is in-between upper-block and the genesis block
BOOST_CHECK_EQUAL(&BlockManager::GetFirstBlock(tip, BLOCK_HAVE_DATA), &first_available_block);
BOOST_CHE
...
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535010215)
While there are no direct unit tests for `BlockManager::GetFirstBlock` setting a `lower_block`, https://github.com/bitcoin/bitcoin/blob/3789215f73466606eb111714f596a2a5e9bb1933/src/test/blockmanager_tests.cpp#L127 exercises that path implicitly.
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535010215)
While there are no direct unit tests for `BlockManager::GetFirstBlock` setting a `lower_block`, https://github.com/bitcoin/bitcoin/blob/3789215f73466606eb111714f596a2a5e9bb1933/src/test/blockmanager_tests.cpp#L127 exercises that path implicitly.
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535030418)
nit: this path is never exercised in the unit tests
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535030418)
nit: this path is never exercised in the unit tests
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535057647)
You mean similarly to
https://github.com/bitcoin/bitcoin/blob/99d012ec80a4415e1a37218fb4933550276b9a0a/src/rpc/blockchain.cpp#L867-L868
?
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2535057647)
You mean similarly to
https://github.com/bitcoin/bitcoin/blob/99d012ec80a4415e1a37218fb4933550276b9a0a/src/rpc/blockchain.cpp#L867-L868
?
💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3543410242)
Code review ACK a1f76230209629c5141984aaac1cc4ba7b4f761a
The PR helps provide clearer failure output, since only missing expected messages are reported.
The lazy initialization part is not necessarily an optimization - I see it primarily as code simplification. That is why I suggested using a list comprehension for `remaining` on each iteration instead of the tail-pop optimization.
But I am also fine with it as is.
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3543410242)
Code review ACK a1f76230209629c5141984aaac1cc4ba7b4f761a
The PR helps provide clearer failure output, since only missing expected messages are reported.
The lazy initialization part is not necessarily an optimization - I see it primarily as code simplification. That is why I suggested using a list comprehension for `remaining` on each iteration instead of the tail-pop optimization.
But I am also fine with it as is.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535227041)
While I like the extra coverage, I’m not fully convinced about adding this bit of non-determinism. That’s why I didn’t include it in the last push. It could make reproducing failures trickier, since we’d be adding another dimension to all tests; core counts behave differently across environments.
Probably a good middle ground could be adding a separate test that picks a random worker count and prints the available core count on failure. That gives us some extra coverage without making the oth
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535227041)
While I like the extra coverage, I’m not fully convinced about adding this bit of non-determinism. That’s why I didn’t include it in the last push. It could make reproducing failures trickier, since we’d be adding another dimension to all tests; core counts behave differently across environments.
Probably a good middle ground could be adding a separate test that picks a random worker count and prints the available core count on failure. That gives us some extra coverage without making the oth
...
💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535258707)
This is needed in multiple methods here, it would simplify the diff and it provides a higher level primitive instead of repeating and reimplementing what we have already built a helper for.
Not sure why we'd want to reduce dependencies here, what's the advantage of that? I value clean code a lot more, especially for a test that doesn't even have performance requirements?
We can of course move `HasReason` in a separate PR, but I think we can use it here before that as well.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535258707)
This is needed in multiple methods here, it would simplify the diff and it provides a higher level primitive instead of repeating and reimplementing what we have already built a helper for.
Not sure why we'd want to reduce dependencies here, what's the advantage of that? I value clean code a lot more, especially for a test that doesn't even have performance requirements?
We can of course move `HasReason` in a separate PR, but I think we can use it here before that as well.
💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535271000)
It's not about coverage, but rather avoiding bias that we may introduce by hard-coding arbitrary values.
I prefer a situation that is "hard to reproduce" over something that we won't find because of hard-coded values.
> core counts behave differently across environments
Exactly, let's exercise those differences, that's exactly what we want from our tests, to ruthlessly try to break the code, right?
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535271000)
It's not about coverage, but rather avoiding bias that we may introduce by hard-coding arbitrary values.
I prefer a situation that is "hard to reproduce" over something that we won't find because of hard-coded values.
> core counts behave differently across environments
Exactly, let's exercise those differences, that's exactly what we want from our tests, to ruthlessly try to break the code, right?
🤔 ryanofsky reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-3472979975)
Code review ACK 6091e947504a13df9ee3310e0fce4585c79d19db
I'm about halfway through reviewing this, and so far it seems like has a lot of nice code improvements in addition to conceptual benefits mentioned earlier https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3483436641.
One thing I don't like about the API however, is the use of integer connection ids and introduction of hash maps to to tie connection ids, sockets, and application data together:
- `CConnman::m_listen_permissions`
...
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-3472979975)
Code review ACK 6091e947504a13df9ee3310e0fce4585c79d19db
I'm about halfway through reviewing this, and so far it seems like has a lot of nice code improvements in addition to conceptual benefits mentioned earlier https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3483436641.
One thing I don't like about the API however, is the use of integer connection ids and introduction of hash maps to to tie connection ids, sockets, and application data together:
- `CConnman::m_listen_permissions`
...
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534490390)
In commit "style: modernize the style of SockMan::BindListenPort()" (6ba0aab8658a7bd553a96e441aa1d5e016e086c3)
Could be good to note in commit message what behaviors this is changing, so it clear they are intentional. After this commit:
- err_msg / strError output is no longer set if there are minor errors like failing to set SO_REUSEADDR
- "Bound to [address]" log message is dropped and moved to caller
- Other log messages are changed slightly
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534490390)
In commit "style: modernize the style of SockMan::BindListenPort()" (6ba0aab8658a7bd553a96e441aa1d5e016e086c3)
Could be good to note in commit message what behaviors this is changing, so it clear they are intentional. After this commit:
- err_msg / strError output is no longer set if there are minor errors like failing to set SO_REUSEADDR
- "Bound to [address]" log message is dropped and moved to caller
- Other log messages are changed slightly
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534287896)
In commit "net: separate the listening socket from the permissions" (1b2a87b3700eb114b8849e05bc488394fa8651bc)
It would seem less fragile to just replace the `m_permissions` field with a more generic field like `std::any m_metadata` or `std::any m_user_data` instead of requiring the application to maintain a separate unordered map and needing to deal with conditions when the map and vector could get out of sync (https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946267787, https://git
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534287896)
In commit "net: separate the listening socket from the permissions" (1b2a87b3700eb114b8849e05bc488394fa8651bc)
It would seem less fragile to just replace the `m_permissions` field with a more generic field like `std::any m_metadata` or `std::any m_user_data` instead of requiring the application to maintain a separate unordered map and needing to deal with conditions when the map and vector could get out of sync (https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946267787, https://git
...
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534857809)
In commit "net: move CConnman-specific parts away from ThreadI2PAcceptIncoming()" (07d731b25219cd27a9428d443902f450acc9423c)
Names and comments seem confusing and maybe inaccurate:
- The name STOP_LISTENING implies I2P was previously listening but now stopped, but the comment implies it might have never been listening.
- The name STOP_LISTENING seems like it could happen for reasons other than failing (like shutting down), so it's inconsistent with the comment which only mentions failing.
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534857809)
In commit "net: move CConnman-specific parts away from ThreadI2PAcceptIncoming()" (07d731b25219cd27a9428d443902f450acc9423c)
Names and comments seem confusing and maybe inaccurate:
- The name STOP_LISTENING implies I2P was previously listening but now stopped, but the comment implies it might have never been listening.
- The name STOP_LISTENING seems like it could happen for reasons other than failing (like shutting down), so it's inconsistent with the comment which only mentions failing.
...
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534545942)
In commit "net: split CConnman::AcceptConnection() off CConnman" (3e881168755392d78d2f9817fe643aadba39aa56)
re: https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1932221561
> Yes, good observation! Indeed the flip will do nothing on the invalid `0.0.0.0` `addr_accepted`. [...]
IMO this information would be useful to mention in the commit message
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2534545942)
In commit "net: split CConnman::AcceptConnection() off CConnman" (3e881168755392d78d2f9817fe643aadba39aa56)
re: https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1932221561
> Yes, good observation! Indeed the flip will do nothing on the invalid `0.0.0.0` `addr_accepted`. [...]
IMO this information would be useful to mention in the commit message
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2535195143)
In commit "net: move sockets from CNode to SockMan" (a59b12aba139aa6003e65ef5842f537a99ae4340)
This commit is larger than the other commits. It would be nice if there's way to break it up since a few things are moving at the same time.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2535195143)
In commit "net: move sockets from CNode to SockMan" (a59b12aba139aa6003e65ef5842f537a99ae4340)
This commit is larger than the other commits. It would be nice if there's way to break it up since a few things are moving at the same time.
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2535119439)
In commit "net: index nodes in CConnman by id" (63751341d1ba7ba144e221bb8f25fec6293b08b3)
Not important but seems like the `ForNode` and `DisconnectNode` methods changed here could be simpler if they used the `GetNodeById` helper method introduced in the next commit (3bb0f2ddfb88e6b8d1aa5cff77416630ad11a381). Might be good to backport it.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2535119439)
In commit "net: index nodes in CConnman by id" (63751341d1ba7ba144e221bb8f25fec6293b08b3)
Not important but seems like the `ForNode` and `DisconnectNode` methods changed here could be simpler if they used the `GetNodeById` helper method introduced in the next commit (3bb0f2ddfb88e6b8d1aa5cff77416630ad11a381). Might be good to backport it.