💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615791108)
I was considering that already, done.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615793339)
The point here is to prevent against potential bugs added in the future, which result in optimality just never being reached anymore. Absent bounds on expected iteration count, this would render a large portion of this test powerless (because anything in `is_optimal`, which includes all quality checks) won't be reached anymore.

Added a comment. Does this explain it?

💬 ajtowns commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#discussion_r2615793433)
Would it be better to provide an `operator""_txid` and `_wtxid` along the lines of hexliterals in util/strencodings.h? They could be made available only to test code that way.

Adding `contstexpr` to the constructors in primitives/transaction_identifier.h and writing:

```c++
consteval auto operator""_txid(const char* str, size_t n) { return Txid::FromUint256(uint256{std::string_view{str, n}}); }
consteval auto operator""_wtxid(const char* str, size_t n) { return Wtxid::FromUint256(uint256
...

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615794234)
I have not done this (yet), because it's hard to maintain in #34023, where the cost function becomes a lot more complicated.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615794420)
Done.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615795954)
Good observation, I have added a comment.

(historically, `MakeTopological` was actually using `MergeSequence` before an overhaul, which may explain why this wasn't explained already)

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615796069)
Done.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615796436)
This was a result of commit surgery gone wrong. Fixed.

💬 l0rinc commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#discussion_r2615797682)
Had a similar attempt in https://github.com/bitcoin/bitcoin/pull/31991#discussion_r1995691590 - what do you think @maflcko?

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3648454810)
I have addressed most of @instagibbs's comments above, and also rewritten `SpanningForestState::SanityCheck` to no longer rely on walking clusters. It's a bit more verbose now, but maybe easier to follow.

⚠️ sajadvilsin-cmyk opened an issue: "ok"
(https://github.com/bitcoin/bitcoin/issues/34065)

⚠️ sajadvilsin-cmyk opened an issue: "ok"
(https://github.com/bitcoin/bitcoin/issues/34066)

⚠️ sajadvilsin-cmyk opened an issue: "ok"
(https://github.com/bitcoin/bitcoin/issues/34067)

⚠️ sajadvilsin-cmyk opened an issue: "ok"
(https://github.com/bitcoin/bitcoin/issues/34068)

⚠️ sajadvilsin-cmyk opened an issue: "ok"
(https://github.com/bitcoin/bitcoin/issues/34069)

⚠️ sajadvilsin-cmyk opened an issue: "ok"
(https://github.com/bitcoin/bitcoin/issues/34070)

⚠️ sajadvilsin-cmyk opened an issue: "ok"
(https://github.com/bitcoin/bitcoin/issues/34071)

📝 sajadvilsin-cmyk opened a pull request: "Create sajad"
(https://github.com/bitcoin/bitcoin/pull/34072)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 ajtowns commented on pull request "refactor: Use NodeClock::time_point for m_addr_token_timestamp":
(https://github.com/bitcoin/bitcoin/pull/34059#issuecomment-3648529415)
What do you think of getting all the `peer->foo` to `peer.foo` noise done first, rather than doing it bit by bit? Here's a scripted-diff approach that might be easy to review: https://github.com/ajtowns/bitcoin/commits/202512-peer-by-ref/

💬 ajtowns commented on pull request "logging: API improvements":
(https://github.com/bitcoin/bitcoin/pull/34038#discussion_r2615873425)
Changed it to just support both include or debug as aliases indefinitely. (Can remove the `include=` alias when the deprecatedrpc option is removed)

I don't think it makes sense to ship a bitcoind/bitcoin-cli pair that don't work together under some configuration param (ie `bitcoin-cli -named include="['net']"` doesn't work because cli won't parse it as json, and `debug=["net"]` won't work because you're running with `deprecatedrpc=logging`)

💬 ajtowns commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#issuecomment-3648572091)
> it seems a bit absurd to check every `50ms` for a timeout of 20 minutes. Even the initial waiting time `m_peer_connect_timeout` from `ShouldRunInactivityChecks()` for a new peer doesn't really be checked _that_ frequently.

I think if you're running a listening node with many real inbound peers, on mainnet where there seems to be maybe 4.5 tx/s, you probably expect to receive socket data from each peer every 2 seconds or so (announcing the ~9 txs that have been received in that time). With 8
...