💬 Sjors commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463296694)
I share the concerns, but at the same time it will be a while before we ship a new release. So I prefer to just wait a few days / weeks to hear a clear answer from Luke himself, preferably here on Github and not some gated social media platform that I can't read :-)
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463296694)
I share the concerns, but at the same time it will be a while before we ship a new release. So I prefer to just wait a few days / weeks to hear a clear answer from Luke himself, preferably here on Github and not some gated social media platform that I can't read :-)
💬 polespinasa commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3463307304)
Is this necessary? doesn't #31664 and the mempool forecaster fix this?
If the block estimator does not have enough info, it can fallback to the mempool forecaster that will return the min feerate to enter the block template it creates?
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3463307304)
Is this necessary? doesn't #31664 and the mempool forecaster fix this?
If the block estimator does not have enough info, it can fallback to the mempool forecaster that will return the min feerate to enter the block template it creates?
💬 ajtowns commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2474399807)
I think this would be better phrased as requirements for implementers of the interface, rather than the user of the interface. ie: "For a given `CNode`, the methods will be called in the following order:". (Both `CNode` and `NetEventsInterface` are interfaces the net module offers, so the appropriate documentation is how they work and what you need to do to implement NetEventsInterface, not how net.cpp needs to be implemented)
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2474399807)
I think this would be better phrased as requirements for implementers of the interface, rather than the user of the interface. ie: "For a given `CNode`, the methods will be called in the following order:". (Both `CNode` and `NetEventsInterface` are interfaces the net module offers, so the appropriate documentation is how they work and what you need to do to implement NetEventsInterface, not how net.cpp needs to be implemented)
💬 ajtowns commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2474449300)
I'd phrase this as:
Destroying objects from this list works as follows (see DisconnectNodes(), DeleteNode()):
* we will close the socket, release locks, and reset global info relating to the connection
* the node will be moved from `m_nodes` to `m_nodes_disconnected`, preventing future calls to `ProcessMessages()` and `SendMessages()` for this node, and decrementing the reference count
* we will wait for GetRefCount() to return `<= 0` indicating there are no other references to this nod
...
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2474449300)
I'd phrase this as:
Destroying objects from this list works as follows (see DisconnectNodes(), DeleteNode()):
* we will close the socket, release locks, and reset global info relating to the connection
* the node will be moved from `m_nodes` to `m_nodes_disconnected`, preventing future calls to `ProcessMessages()` and `SendMessages()` for this node, and decrementing the reference count
* we will wait for GetRefCount() to return `<= 0` indicating there are no other references to this nod
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474919127)
> I mean pick up that it's being written by multiple threads if something breaks the assumptions in this test. It seems like an ok idea. Or am I wrong?
I don't think there is any broken assumption. `BlockWorkers()` ensures that all threads except one are blocked. We could go further and check that all tasks submitted post the `BlockWorkers` call are executed by the same thread too, but that seems to be an overkill to me. We ensure that in another way.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474919127)
> I mean pick up that it's being written by multiple threads if something breaks the assumptions in this test. It seems like an ok idea. Or am I wrong?
I don't think there is any broken assumption. `BlockWorkers()` ensures that all threads except one are blocked. We could go further and check that all tasks submitted post the `BlockWorkers` call are executed by the same thread too, but that seems to be an overkill to me. We ensure that in another way.
💬 ajtowns commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2474923790)
Why monitor cpu time rather than real time? Isn't time spent waiting for disk access while looking up the utxo set equally problematic? Waiting on another thread's lock is arguably less problematic (it's the other thread that's delaying things), but if an attacker is deliberately triggering lock contention somehow to delay processing other thread's messages, that would still be a problem.
Adding OS-specific logic rather than just using either real time or a steady clock seems over-complicated
...
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2474923790)
Why monitor cpu time rather than real time? Isn't time spent waiting for disk access while looking up the utxo set equally problematic? Waiting on another thread's lock is arguably less problematic (it's the other thread that's delaying things), but if an attacker is deliberately triggering lock contention somehow to delay processing other thread's messages, that would still be a problem.
Adding OS-specific logic rather than just using either real time or a steady clock seems over-complicated
...
💬 mzumsande commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3463416565)
> If we are being connected to, I think our self-announcement won't be rate-limited because we'll wait until receiving getaddr, call SetupAddressRelay, and then in SendMessages, our self-announcement will replace one of the existing entries in the getaddr response. There's no time-gap in this case afaict.
True, I have observed this effect too recently, it isn't really clean though - I don't think anyone intended it to work such that a GETADDR answers would contain 999 addresses from addrman a
...
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3463416565)
> If we are being connected to, I think our self-announcement won't be rate-limited because we'll wait until receiving getaddr, call SetupAddressRelay, and then in SendMessages, our self-announcement will replace one of the existing entries in the getaddr response. There's no time-gap in this case afaict.
True, I have observed this effect too recently, it isn't really clean though - I don't think anyone intended it to work such that a GETADDR answers would contain 999 addresses from addrman a
...
💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2475026718)
The explicit was suggested in https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226976728. As far as I know it's good practice, so that the compiler can't make implicit conversions to resolve the parameters of a function. Also see: https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/doc/developer-notes.md?plain=1#L835
The initializer list was suggested in https://github.com/bitcoin/bitcoin/pull/25968#pullrequestreview-1096400728, I think it's consistent w
...
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2475026718)
The explicit was suggested in https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226976728. As far as I know it's good practice, so that the compiler can't make implicit conversions to resolve the parameters of a function. Also see: https://github.com/bitcoin/bitcoin/blob/292ea0eb8982faef460c210bd4215d603f487463/doc/developer-notes.md?plain=1#L835
The initializer list was suggested in https://github.com/bitcoin/bitcoin/pull/25968#pullrequestreview-1096400728, I think it's consistent w
...
💬 ajtowns commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-3463449884)
> In many circumstances, using lots of our CPU is a desirable property and should be rewarded, not punished - also outside of IBD. Some of the most undesirable peers are spy nodes that never send us anything.
Spy nodes can use lots of CPU time, via `inv_to_send` sorting costs.
We probably don't want to allocate the time to validate accepted blocks/transactions against a peer; but perhaps would like to allocate time for rejected blocks/transactions.
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-3463449884)
> In many circumstances, using lots of our CPU is a desirable property and should be rewarded, not punished - also outside of IBD. Some of the most undesirable peers are spy nodes that never send us anything.
Spy nodes can use lots of CPU time, via `inv_to_send` sorting costs.
We probably don't want to allocate the time to validate accepted blocks/transactions against a peer; but perhaps would like to allocate time for rejected blocks/transactions.
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2475076045)
I don't think so - it looks like CMAKE_SKIP_INSTALL_RPATH was already in the file. I believe I'm just removing CMAKE_SKIP_BUILD_RPATH. The diff display seems a bit misleading here since the rebase shifted line numbers. Let me know if I'm wrong here but that's what it looks like to me.
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2475076045)
I don't think so - it looks like CMAKE_SKIP_INSTALL_RPATH was already in the file. I believe I'm just removing CMAKE_SKIP_BUILD_RPATH. The diff display seems a bit misleading here since the rebase shifted line numbers. Let me know if I'm wrong here but that's what it looks like to me.
👍 instagibbs approved a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3394857863)
ACK 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3394857863)
ACK 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474270872)
Adds some GetFeerateDiagram coverage that will get hit in various contexts including fuzz
```
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index b0c9ab043c..3dcc0dd3e4 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -426,7 +426,13 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
+ const auto score_with_topo{GetSortedScoreWithTopology()};
+
+
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474270872)
Adds some GetFeerateDiagram coverage that will get hit in various contexts including fuzz
```
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index b0c9ab043c..3dcc0dd3e4 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -426,7 +426,13 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
+ const auto score_with_topo{GetSortedScoreWithTopology()};
+
+
...
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474321911)
without comment I found this hard to understand
```suggestion
// If we don't set the # of sig ops in the CTxMemPoolEntry, template creation fails during sanity checks
AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(t));
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474321911)
without comment I found this hard to understand
```suggestion
// If we don't set the # of sig ops in the CTxMemPoolEntry, template creation fails during sanity checks
AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(t));
```
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474299026)
could we get this magic number explained?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474299026)
could we get this magic number explained?
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474280989)
what does dummy mean here?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474280989)
what does dummy mean here?
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474910301)
```suggestion
# It should also limit cluster sizes during replacement
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474910301)
```suggestion
# It should also limit cluster sizes during replacement
```
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474913651)
```suggestion
# Multiply fee by 5, which should easily cover the cost to replace (but is still too large a cluster). Otherwise, use the target vsize at 10sat/vB
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474913651)
```suggestion
# Multiply fee by 5, which should easily cover the cost to replace (but is still too large a cluster). Otherwise, use the target vsize at 10sat/vB
```
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475071643)
Since this harness is making blocks, I think it'd be good to have sigops naturally appearing in the constructed blocks. Ran this for a bit, seemed to work?
```
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 8a5863c5d5..413ab0db4c 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -291,9 +291,18 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
tx_mut.vin.push_back(in);
}
+
+ // Check
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475071643)
Since this harness is making blocks, I think it'd be good to have sigops naturally appearing in the constructed blocks. Ran this for a bit, seemed to work?
```
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 8a5863c5d5..413ab0db4c 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -291,9 +291,18 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
tx_mut.vin.push_back(in);
}
+
+ // Check
...
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475016196)
just extended it to clusters of size three and made log more sensible
```
diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index 759e3cb07d..c54534f6f8 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -94,5 +94,5 @@ class PackageRBFTest(BitcoinTestFramework):
def test_package_rbf_basic(self):
- self.log.info("Test that a child can pay to replace its parents' conflicts of cluster
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475016196)
just extended it to clusters of size three and made log more sensible
```
diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index 759e3cb07d..c54534f6f8 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -94,5 +94,5 @@ class PackageRBFTest(BitcoinTestFramework):
def test_package_rbf_basic(self):
- self.log.info("Test that a child can pay to replace its parents' conflicts of cluster
...
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474283513)
no-op trimming makes sense too (probably a smarter of way of having this hit than I suggest here)
```suggestion
tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0U, tx_pool.DynamicMemoryUsage() * 2));
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474283513)
no-op trimming makes sense too (probably a smarter of way of having this hit than I suggest here)
```suggestion
tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0U, tx_pool.DynamicMemoryUsage() * 2));
```