💬 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));
```
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475098091)
OK this was a mistake on my part, @hodlinator's suggested change passes, because segwit node's index is *not* `0` any more, it just got connected so it's `-1`, and any ways, for the test that follows it should not matter if the peer is HB or not, since the peer sends a header and waits for getdata, so this is solicited. Should I add separate test cases for both? For now, I'll just drop this line.
It also seems weird that we'll accept a CMPCTBLOCK from someone that hasn't given us a SENDCMPCT
...
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475098091)
OK this was a mistake on my part, @hodlinator's suggested change passes, because segwit node's index is *not* `0` any more, it just got connected so it's `-1`, and any ways, for the test that follows it should not matter if the peer is HB or not, since the peer sends a header and waits for getdata, so this is solicited. Should I add separate test cases for both? For now, I'll just drop this line.
It also seems weird that we'll accept a CMPCTBLOCK from someone that hasn't given us a SENDCMPCT
...
💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475135383)
Sorry for being unclear. I don't think there is any broken assumption either.
However, if we made this a non-atomic as suggested, then *if* something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475135383)
Sorry for being unclear. I don't think there is any broken assumption either.
However, if we made this a non-atomic as suggested, then *if* something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3463589829)
Guix build for c4ea4210abb20afa3724a2dc1384b0b91d099b31:
3c68ce9be2dfa7c2b20962f119ac48d760e142162da5d340011e496f099ee38c dist-archive/bitcoin-c4ea4210abb2.tar.gz
2ccb113dd38bd2ca5db8dd5ec20540c09f149e03789086b738dc62ab74e6892c x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu-debug.tar.gz
a4aed426dd114dab2ed1f62904eda03805ee0e516c83cebd366d4e7e50387da4 x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu.tar.gz
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3463589829)
Guix build for c4ea4210abb20afa3724a2dc1384b0b91d099b31:
3c68ce9be2dfa7c2b20962f119ac48d760e142162da5d340011e496f099ee38c dist-archive/bitcoin-c4ea4210abb2.tar.gz
2ccb113dd38bd2ca5db8dd5ec20540c09f149e03789086b738dc62ab74e6892c x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu-debug.tar.gz
a4aed426dd114dab2ed1f62904eda03805ee0e516c83cebd366d4e7e50387da4 x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu.tar.gz
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2475155565)
@trevarj
Would it work with the recursion in `make-mingw-w64/implementation`?
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2475155565)
@trevarj
Would it work with the recursion in `make-mingw-w64/implementation`?
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3463610029)
Thanks for your review @ryanofsky I will address the comments soon.
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3463610029)
Thanks for your review @ryanofsky I will address the comments soon.
⚠️ DoctorBuzz1 opened an issue: "Limit "Bulk Dust" with a default filter or consensus.."
(https://github.com/bitcoin/bitcoin/issues/33737)
I’m exploring a potential default filter or consensus-level rule (since a large number of people believe that default filters don't work) to discourage UTXO-bloat patterns without touching Script, witness data, or the block size limit.
The idea is to target “bulk dust” transactions — those that create large numbers of extremely small outputs — which are the main cause of long-term UTXO set growth.
These types of "bulk dust" transactions have been the No. 1 reason cited for wanting to expand th
...
(https://github.com/bitcoin/bitcoin/issues/33737)
I’m exploring a potential default filter or consensus-level rule (since a large number of people believe that default filters don't work) to discourage UTXO-bloat patterns without touching Script, witness data, or the block size limit.
The idea is to target “bulk dust” transactions — those that create large numbers of extremely small outputs — which are the main cause of long-term UTXO set growth.
These types of "bulk dust" transactions have been the No. 1 reason cited for wanting to expand th
...
💬 parabitdev 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-3463668325)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that serve
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463668325)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that serve
...
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475267452)
Good catch, in the branch I'll push soon I address this by checking both a solicited cmpctblock and unsolicited one.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475267452)
Good catch, in the branch I'll push soon I address this by checking both a solicited cmpctblock and unsolicited one.