💬 polespinasa commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2474566135)
this changes feel out of scope for this pr
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2474566135)
this changes feel out of scope for this pr
💬 gmaxwell 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-3463141404)
Odd. I connected to it in a loop and eventually got a "/Satoshi:29.2.0/Knots:20251010/" -- also other knots versions and yet yours shows no knots at all?
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463141404)
Odd. I connected to it in a loop and eventually got a "/Satoshi:29.2.0/Knots:20251010/" -- also other knots versions and yet yours shows no knots at all?
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474671206)
true. Closing this.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474671206)
true. Closing this.
💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474735112)
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?
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474735112)
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?
💬 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
```