💬 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.
💬 ryanofsky commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2535004079)
In commit "net: move I2P-accept-incoming code from CConnman to SockMan" (3e91c8b502d43d656d9f3207cb550a0c03d1429e)
s/Singal/Signal
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r2535004079)
In commit "net: move I2P-accept-incoming code from CConnman to SockMan" (3e91c8b502d43d656d9f3207cb550a0c03d1429e)
s/Singal/Signal
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534678956)
In commit "Check cluster limits when using -walletrejectlongchains"
Nit, and only if you touch up:
```c++
changeset->StageAddition(tx, /*fee=*/0, /*time=*/0, /*entry_height=*/0, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/0, LockPoints{});
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534678956)
In commit "Check cluster limits when using -walletrejectlongchains"
Nit, and only if you touch up:
```c++
changeset->StageAddition(tx, /*fee=*/0, /*time=*/0, /*entry_height=*/0, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/0, LockPoints{});
```
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534753297)
In commit "Add new (unused) limits for cluster size/count"
Nitty nit: with package relay, the "existing in-mempool transactions" part is perhaps misleading, as in you could have a cluster of 63 transactions, be relayed a package of 2 connecting to it, and it would be rejected.
Suggestion: "Do not accept transactions into mempool which are directly or indirectly connected to <n> or more other unconfirmed transactions".
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534753297)
In commit "Add new (unused) limits for cluster size/count"
Nitty nit: with package relay, the "existing in-mempool transactions" part is perhaps misleading, as in you could have a cluster of 63 transactions, be relayed a package of 2 connecting to it, and it would be rejected.
Suggestion: "Do not accept transactions into mempool which are directly or indirectly connected to <n> or more other unconfirmed transactions".