✅ pinheadmz closed an issue: "⚠️ General optimization suggestion"
(https://github.com/bitcoin/bitcoin/issues/33722)

💬 willcl-ark commented on issue "Failure to bind to ZMQ addresses is swallowed in debug logs":
(https://github.com/bitcoin/bitcoin/issues/33715#issuecomment-3456782171)
I agree on both counts. If a user has configured zmq but it fails to bind I think we should log this at a higher level than debug, and I agree that the node aborting startup is most likely the correct action to take.

💬 waketraindev commented on pull request "addrman, net: filter during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2469805737)
Thanks for running mutation testing and for review!

I'm thinking:
```cpp
const std::set<CAddress> vExpectedAddresses = {addr1, addr3, addr4, addr5};
BOOST_CHECK_EQUAL(vAddrPolicyPort.size(), vExpectedAddresses.size());
BOOST_CHECK(std::all_of(vAddrPolicyPort.begin(), vAddrPolicyPort.end(), [&](const CAddress& a){ return vExpectedAddresses.contains(a); }));
BOOST_CHECK(std::none_of(vAddrPolicyPort.begin(), vAddrPolicyPort.end(), policyPortPolicy));
```

💬 waketraindev commented on pull request "addrman, net: filter during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#issuecomment-3456841808)
Added test verifying only expected addresses are returned and filtered ones excluded.

💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469840049)
Isn't this comment already implied by `GUARDED_BY(m_mutex)`? Also it's written by the control thread and read by worker threads, so of course it must only be modified while holding the mutex. This comment doesn't add anything IMO.

💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469885604)
I'm unclear what the purpose of this function is. It interrupts the workers, like `Stop`, but doesn't join them or wait for them to finish.
If the idea is to clear the workqueue and park the threads without being able to add more tasks, it does not accomplish that since the threads will exit their loops as well and not be parked on the cv.

👍 brunoerg approved a pull request: "test: Avoid shutdown race in NetworkThread"
(https://github.com/bitcoin/bitcoin/pull/33140#pullrequestreview-3389226941)
code review ACK fa6db79302d28e6b31b07b24580430614ffcd0e8

I could not reproduce the issue intermittently via the provided script, but I could see the race when adding a sleep before calling `run_forever()`. Anyway, regardless of it, waiting until the thread is actually running is a best practice.

💬 brunoerg commented on pull request "test: Avoid shutdown race in NetworkThread":
(https://github.com/bitcoin/bitcoin/pull/33140#discussion_r2469916322)
Not related but why we don't call `set_event_loop` when running it?

```py
class NetworkThread(threading.Thread):
network_event_loop = None

def __init__(self):
super().__init__(name="NetworkThread")
# There is only one event loop and no more than one thread must be created
assert not self.network_event_loop

NetworkThread.listeners = {}
NetworkThread.protos = {}
if platform.system() == 'Windows':
asyncio.set_event
...

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-3456941699)
Re-ACK 5be5a45847f710451a74aed10f6e4efcbace4b45

✅ hebasto closed a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445)

📝 hebasto reopened a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445)
This PR:

1. Updates to [IWYU 0.25](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.25), which is compatible with Clang 21.

2. Fixes new "modernize-use-default-member-init" warnings. The warning in `interpreter.cpp` is a [false positive](https://github.com/llvm/llvm-project/issues/160394), so it has been suppressed.

🤔 glozow reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3388723571)
The empty commits can be dropped. You could prefix some commits with "prep" or "cleanup," but I think it's pretty clear already.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469541272)
In 4c359dabd2a7ca5a5d7469070f55e66a88196291

Is this benchmark supposed to be deleted? It seems like it was written a long time ago.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469577973)
in 83c8753abf9c2af75cfbacd5488605332da588a4, I think this is missing a few words?

```suggestion
/** How many linearization iterations required for TxGraph clusters to have "acceptable" quality, if they cannot be optimally linearized with fewer iterations. */
```

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469628917)
2812471c2bfce6215d6d58c1defd39c7a050edbd: Maybe deserves a comment

```suggestion
// PrioritiseTransaction calls stack on previous ones. Set the new transaction fee to be current modified fee + feedelta.
m_txgraph->SetTransactionFee(*it, it->GetModifiedFee() + nFeeDelta);
```

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469865825)
in 0e25736d6e0

It would help the fuzzer run faster to just call `check` once at the end, instead of after each round of potential removals.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469950583)
If not squashed, perhaps move it earlier so that the test can have the correct string when introduced?

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469612014)
nit 91d9bfcca62bd6c168476f6af442bd7049ff872e

I was going to complain about this magic number, maybe squash 83c8753abf9c2af75cfbacd5488605332da588a4 into its parent?

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469945254)
It seems a bit unfortunate that, prior to 675fa79bb280129fb87e5d1884e670a35a03f5cd, there isn't a cap on ancestor count when we call `CalculateMemPoolAncestors`. Could the rework be done earlier?

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469604073)
Yes, calling `UpdateModifiedFee(0)` does nothing. I think it's fine to keep this :shrug: