💬 l0rinc commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2615700914)
> added you as a commit co-author

You need to add a [coauthor](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors#creating-co-authored-commits-on-the-command-line) with a fixed format, e.g. https://github.com/bitcoin/bitcoin/commit/8e4c66d0a7a0911c10dced0d6dd60ca7bd9545af

💬 ajtowns commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2615702010)
Duration works as an atomic fine -- we have `atomic<seconds>` and `atomic<microseconds>` in various places in net, net_processing and util/time. `atomic<time_point>` doesn't work because even if the duration is `noexcept` (necessary for atomic), that doesn't propagate to the time_point's constructor also being noexcept which is a requirement for wrapping in atomic.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615782689)
> I think here we should just check if there are any txs in the private broadcast queue, not that the node's specific tx is still in the private broadcast queue.

That makes sense. Having additional attempts that would be spent on other transactions in the worst case is no big deal in my opinion, but obviously the infinite loop possibility should be fixed.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788225)
I have reverted this change. My benchmarks show that the time per cost is quite similar to before, which I'm now mentioning in the commit message. This will be overhauled in #34023.

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

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788964)
I have just increased it to 1T iterations, which ought to be enough for anyone.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615789740)
Done, except renaming to `m_transaction_idxs`, because I'm too lazy to update all the affected commits right now. Will leave this open.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615790625)
It's to avoid reallocating; this formula gives the maximum number of dependencies a cluster with a given number of transactions can have. I have added a comment. Does this suffice?

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615790818)
Indeed! Added as comment.

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