💬 ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470690366)
Good idea, will do if retouching.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470690366)
Good idea, will do if retouching.
💬 niteshbalusu11 commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458027438)
Concept ACK
Because it's been verified that this happens.
> Have independently confirmed that `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us` is not returning any Core v30 nodes.
If @luke-jr is willing to change this then we don't have to remove.
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458027438)
Concept ACK
Because it's been verified that this happens.
> Have independently confirmed that `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us` is not returning any Core v30 nodes.
If @luke-jr is willing to change this then we don't have to remove.
💬 hsjoberg commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458038934)
utACK 9c3291ff9bd713e1a92dae2c388a83e4a107bf7e.
Luke has repeatedly called "Bitcoin Core" bad actors in public.
Given this hostility, it stands to reason that his DNS seed should not be in the software.
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458038934)
utACK 9c3291ff9bd713e1a92dae2c388a83e4a107bf7e.
Luke has repeatedly called "Bitcoin Core" bad actors in public.
Given this hostility, it stands to reason that his DNS seed should not be in the software.
📝 isrod opened a pull request: "Log ZMQ bind error at Error level, abort startup on ZMQ init error (#33715)"
(https://github.com/bitcoin/bitcoin/pull/33727)
Fixes #33715
(https://github.com/bitcoin/bitcoin/pull/33727)
Fixes #33715
💬 maflcko commented on issue "ci: Where to run heavy and high-maintenance CI tasks?":
(https://github.com/bitcoin/bitcoin/issues/33668#issuecomment-3458046048)
I went ahead and pushed the msan -O0, and the s390x task to https://github.com/maflcko/bitcoin-core-nightly/actions/runs/18885767497/job/53900576177. However, they are so slow on GHA, that they time out after 6 hours.
Possibly with a CTest Dashboard to upload results, those tasks could be run on faster or at least dedicated hardware without timeouts.
(https://github.com/bitcoin/bitcoin/issues/33668#issuecomment-3458046048)
I went ahead and pushed the msan -O0, and the s390x task to https://github.com/maflcko/bitcoin-core-nightly/actions/runs/18885767497/job/53900576177. However, they are so slow on GHA, that they time out after 6 hours.
Possibly with a CTest Dashboard to upload results, those tasks could be run on faster or at least dedicated hardware without timeouts.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470732607)
> Could the same be done via a bool parameter to Stop to skip joining?
If we do that, the threads wouldn't remain in the `m_workers` vector anymore. They'd be swapped out on the first call. That means we wouldn't be able to wait for them to finish later on, which would be quite bad since we’d lose track of when all requests are fulfilled before destroying the backend objects (that's basically what joining the threads mean for us right now). And this could lead to requests accessing null point
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470732607)
> Could the same be done via a bool parameter to Stop to skip joining?
If we do that, the threads wouldn't remain in the `m_workers` vector anymore. They'd be swapped out on the first call. That means we wouldn't be able to wait for them to finish later on, which would be quite bad since we’d lose track of when all requests are fulfilled before destroying the backend objects (that's basically what joining the threads mean for us right now). And this could lead to requests accessing null point
...
💬 pinheadmz commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458058470)
> [!CAUTION]
> Ok at this point there is NO LONGER A NEED for more comments about an individual's trustworthiness.
This PR is starting to get brigaded and comments that do not ADD ADDITIONAL INFORMATION or help the maintainers do their job will probably get hidden and may result in a ban
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458058470)
> [!CAUTION]
> Ok at this point there is NO LONGER A NEED for more comments about an individual's trustworthiness.
This PR is starting to get brigaded and comments that do not ADD ADDITIONAL INFORMATION or help the maintainers do their job will probably get hidden and may result in a ban
💬 Eunovo commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470739542)
Comment out line 497 and rebuild. The interface_ipc tests still pass successfully. They fail if you modify them to use two waitNext calls like so
```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..a3dc559008 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -199,10 +199,14 @@ class IPCInterfaceTest(BitcoinTestFramework):
wait_task = asyncio.create_task(wait_for_block())
...
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470739542)
Comment out line 497 and rebuild. The interface_ipc tests still pass successfully. They fail if you modify them to use two waitNext calls like so
```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..a3dc559008 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -199,10 +199,14 @@ class IPCInterfaceTest(BitcoinTestFramework):
wait_task = asyncio.create_task(wait_for_block())
...
💬 Eunovo commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2470743677)
Line 198
(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!
(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 |
|---|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:|---:
...
(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!
(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
...
(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.
(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.
(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.
(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.
(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
(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);
```
(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
...
(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
...