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

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3648673018)
> TBH I don't know of code that would directly benefit from combining ErrorString with std::expected because I think validation and wallet code are better off using util::Result

If you don't see any benefits keep your implementation, a generalisation can be implemented later if required 😄

💬 ajtowns commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2616011262)
Should these log messages include "I2P" somewhere? Otherwise figuring out what SAM relates to may be confusing. The help text for `-i2psam` describes it as an "I2P SAM proxy".

💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2616086274)
doc string can be changed. No risk to keep it but also no point to add it imo. Can just be a normal function that only rate limits >= Info Level and only used within this file.

But just move forward its fine

💬 w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2616151939)
> can this result in an infinite loop or disconnect/reconnects?

From what I understand, yes. That's the point.

💬 maflcko commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2616285391)
I may be missing something obvious. https://godbolt.org/z/8GM97aYhs seems to compile fine. And https://eel.is/c++draft/time.duration.cons doesn't mention `noexcept`. Can you give a minimal reproducer, or link to the std docs?

💬 yuvicc commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#issuecomment-3649377448)
ACK 8ca9997e48bb2067ea83fabb1c640af72178f97c