💬 Eunovo commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470743677)
Line 198

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470743689)
sounds good, taken. thanks!

💬 john-moffett commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231)
Concept ACK based on @achow101 's comment and the currently not "fairly selected" Bitcoin nodes.

Here's what I get with multiple runs:

| Seed | 30.99.0 | 30.0.0 | 29.2.0 | 29.1.0 | 29.0.0 | 28.99.0 | 28.2.0 | 28.1.0 | 28.0.0 | 27.99.0 | 27.2.0 | 27.1.0 | 27.0.0 | 26.2.0 | 26.1.0 | 26.0.0 | 25.1.0 | 25.0.0 | 24.0.1 | 24.0.0 | 23.1.0 | 23.0.0 | 22.0.0 | 0.21.1 | 0.20.1 | 0.20.0 | 0.17.1 | 0.12.1 |
|---|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:
...

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470749427)
Sure, pushed. Thanks!

💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470750271)
I was thinking like this
```diff
diff --git a/src/util/threadpool.h b/src/util/threadpool.h
index 94409facd5..dc1a218abd 100644
--- a/src/util/threadpool.h
+++ b/src/util/threadpool.h
@@ -123,19 +123,19 @@ public:
*
* Must be called from a controller (non-worker) thread.
*/
- void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+ void Stop(bool join_threads = true) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
// Notify workers and join them.
std::ve
...

💬 gmaxwell commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458091139)
ACK 9c3291ff9bd713e1a92dae2c388a83e4a107bf7e

This DNS seed is clearly violating policy-- which was put into place to make discourage abuse of seeds for attempting eclipse attacks and network surveillance. Removing it should be entirely uncontroversial. The change should also be backported.

💬 ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470765602)
Thanks for your swift response, you highlighted multiple lines, so I commented them all that’s why it failed for me.
It should pass because we still return (The issue with commenting that line is that subsequent wait for same block template will return instantly). We can add a test case to verify that after interrupting a wait, you can wait again.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470765839)
You know, we could call `ProcessTask()` during `Stop()` too, which would be a strict improvement over the current locking-wait behavior in master (since we would shut down faster by actively processing pending requests on the shutdown thread as well). This is something that hasn’t been possible until now.
Still, this is material for a follow-up; would say that it is better to keep this PR as contained as possible.

💬 ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470770963)
The line is important, it created a new transaction in the mempool it's what will make the test fail when we don't really interrupt a wait.
Another thing that could be an indicator is also chain tip change.

🤔 hodlinator reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3382451037)
Reviewed 5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2464905650)
nit: Avoid explicit `\n`.
```suggestion
LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)", pfrom.GetId(), pfrom.LogIP(fLogIPs), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
```

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466726988)
This seems to be lacking precision now that we ignore some `CMPCTBLOCK` messages.

<details>
<summary>Could check which block..</summary>


```diff
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -904,7 +904,7 @@ class CompactBlocksTest(BitcoinTestFramework):
node = self.nodes[0]
assert len(self.utxos)

- def announce_cmpct_block(node, peer, txn_count):
+ def announce_cmpct_block(node, peer, txn_count, expect
...

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2464997684)
remark: Wondered whether it would be fair to mark as `Misbehaving()` or disconnect them, but there's a tiny chance they may have recently been an HB peer (as remarked in https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3456385354).

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466672619)
This line is unnecessary, it can be replaced with something like:

```suggestion
self.assert_highbandwidth_states(self.nodes[0], idx=0, hb_to=True, hb_from=True)
```

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466779388)
Duplicated from other test in same file, could be made class-local instead?
(Last arg is always left at default in this new outer test function).

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466749706)
nit:
```suggestion
# match the given parameters for the peer at idx of a given node
```

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466826137)
Maybe use a comment to point out that since we only send the header and not the whole block, we are not marked as an HB peer?

And/or use `assert_highbandwidth_states(node, idx=-1, hb_to=False)` another time at the end of this block to verify/clarify.

💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2465045694)
👆

💬 ryanofsky commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3458122631)
re: https://github.com/bitcoin/bitcoin/issues/33575#issue-3496102450

> 2. Add a new `BlockTemplate::interruptWait()` to be able to interrupt `waitNext()` without destroying the template.

This is implemented in #33676 and could use some testing, but hopefully will help resolve this issue and #33554.

💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470784762)
That is a good idea, but not related to my suggestion here :). Yes, that would be a good followup.
I don't think we should have a method that would cause the node to crash if `Start` is called more than once. I disagree and think just calling `Stop` at the beginning of `Start` would be safer than the current implementation.