💬 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".
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534735892)
In commit "Add transactions to txgraph, but without cluster dependencies"
Nit: add an `AssertLockHeld(m_pool.cs);` here too?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534735892)
In commit "Add transactions to txgraph, but without cluster dependencies"
Nit: add an `AssertLockHeld(m_pool.cs);` here too?
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535009153)
In commit "Select transactions for blocks based on chunk feerate"
Nit: now that the chunk finding logic is inside txgraph, there is no actual heap anymore (it internally just iterates the feerate-sorted chunk index, but that's an implementation detail).
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535009153)
In commit "Select transactions for blocks based on chunk feerate"
Nit: now that the chunk finding logic is inside txgraph, there is no actual heap anymore (it internally just iterates the feerate-sorted chunk index, but that's an implementation detail).
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535138491)
In commit " [squashme] Use outputs and structured bindings for CalculateAncestorD…"
Squash the squashme?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535138491)
In commit " [squashme] Use outputs and structured bindings for CalculateAncestorD…"
Squash the squashme?
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535130273)
In commit "Use cluster linearization for transaction relay sort order"
Nit: if "mining score" means "chunk feerate", then this description is not complete. There is also the possibility of a and b having the same chunk feerate, being in distinct clusters, and a's cluster being given priority.
Though, I think all of this can be treated as a TxGraph implementation detail now. What about:
```
/* Return `true` if hasha should be considered sooner than hashb, namely when:
* a is not i
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535130273)
In commit "Use cluster linearization for transaction relay sort order"
Nit: if "mining score" means "chunk feerate", then this description is not complete. There is also the possibility of a and b having the same chunk feerate, being in distinct clusters, and a's cluster being given priority.
Though, I think all of this can be treated as a TxGraph implementation detail now. What about:
```
/* Return `true` if hasha should be considered sooner than hashb, namely when:
* a is not i
...
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535342513)
In commit "fuzz: remove comparison between mini_miner block construction and miner"
Ok. This commit belongs much earlier in the PR, though (just before "Select transactions for blocks based on chunk feerate"). Otherwise, the range of commits in between will fail fuzz tests if run long enough.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535342513)
In commit "fuzz: remove comparison between mini_miner block construction and miner"
Ok. This commit belongs much earlier in the PR, though (just before "Select transactions for blocks based on chunk feerate"). Otherwise, the range of commits in between will fail fuzz tests if run long enough.
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535291747)
In commit "squashme: only output weight, not vsize, for cluster/chunk information"
Squash the squashme?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535291747)
In commit "squashme: only output weight, not vsize, for cluster/chunk information"
Squash the squashme?
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535278410)
In commit "Rework RBF and TRUC validation"
The `GetParents` function is only introduced in the next commit.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535278410)
In commit "Rework RBF and TRUC validation"
The `GetParents` function is only introduced in the next commit.
📝 joshdoman opened a pull request: "[kernel] Expose reusable `PrecomputedTransactionData` in script validation"
(https://github.com/bitcoin/bitcoin/pull/33891)
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bit
...
(https://github.com/bitcoin/bitcoin/pull/33891)
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bit
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535364299)
> Not sure why we'd want to reduce dependencies here, what's the advantage of that?
I wrote it above; the idea is to be able to use this low-level class and tests elsewhere, outside the project, just by pulling a few files without dragging in all our unit test framework machinery, which has tons other dependencies.
If we were talking about a substantial improvement, I’d agree with you, but here it’s just a 2-line diff with no behavior change. And for me, that makes the rationale for includin
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535364299)
> Not sure why we'd want to reduce dependencies here, what's the advantage of that?
I wrote it above; the idea is to be able to use this low-level class and tests elsewhere, outside the project, just by pulling a few files without dragging in all our unit test framework machinery, which has tons other dependencies.
If we were talking about a substantial improvement, I’d agree with you, but here it’s just a 2-line diff with no behavior change. And for me, that makes the rationale for includin
...