💬 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
...

💬 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.
...

💬 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

💬 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.

💬 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.

💬 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

💬 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{});
```

💬 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".

💬 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?

💬 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).

💬 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?

💬 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
...

💬 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.

💬 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?

💬 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.

📝 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
...

💬 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
...

💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3543642952)
@ryanofsky should we close this?

💬 achow101 commented on pull request "wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members":
(https://github.com/bitcoin/bitcoin/pull/32763#discussion_r2535396633)
Done

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2535398529)
had a closer look now, it actually is straightforward using `GetDebugMessage()`. So I added the log.