👍 instagibbs approved a pull request: "Cluster mempool"
(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()};
+
+
...

💬 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));
```

💬 instagibbs commented on pull request "Cluster mempool":
(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?

💬 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
```

💬 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
```

💬 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
...

💬 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
...

💬 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));
```

💬 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
...

💬 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.

💬 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

💬 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`?

💬 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.

⚠️ 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
...

💬 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
...

💬 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.

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475307568)
> 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.

But as you said, the thread sanitizer would likely complain about that. We’re accessing the same variable from different threads (main and worker). The main issue, however, will probably be r
...

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475344730)
+1 for separate test cases for solicited / unsolicited. I agree the test can be improved in a dedicated PR.