🤔 ismaelsadeeq reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3339641710)
Review the main implementation e6315c24326016cfaee5bd046e8b2e4e1088ac6b

I've not noticed any major issue, dropped a few minor comments and nits.
If any comment become stale after the update feel free to ignore it

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432258434)
In "Add new (unused) limits for cluster size/count" 2801e80528a3a1c2949a8fda6338882613a673e5


nit: be specific it is in virtual size
```suggestion
argsman.AddArg("-limitclustersize=<n>", strprintf("Do not accept transactions whose size with all in-mempool connected transactions exceeds <n> virtual kilobytes (default: %u)", DEFAULT_CLUSTER_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

```

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432032703)
In "Add sigops adjusted weight calculator" f2eff17c6c4fc945f6fd761564212802107a1d7d

nit: use snake case.

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432321152)
In "test: update feature_rbf.py replacement test" 429bdbecfde93c54374fb3098e357e18556e7e21

nit: This should live in `functional/test_framework/mempool_util.py`

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432516613)
In "Do not allow mempool clusters to exceed configured limits" 5a388c0d595b2318fea4b1dce977e2d5ff1abc48

I try to not use the magic number by trimming using

`pool.DynamicMemoryUsage() - usage_of_5 - usage_of_7` but test fail because tx 6 is also evicted.
However when I subtract only the usage of 5 the test succeed.

I grab the usages by computing the delta in memory usage after insertion of the tx.

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432430377)
In "Do not allow mempool clusters to exceed configured limits" 5a388c0d595b2318fea4b1dce977e2d5ff1abc48

If you define the max cluster count in mempool util it can be reused here.

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432661442)
In "Check cluster limits when using -walletrejectlongchains" 1102ac7f74ac2f48760b46be58c7deb70fa727cf

nit: also state that it should be safe to create a new change set here because we lock mempool cs.

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2435949023)
In "Select transactions for blocks based on chunk feerate" a8be743aeb42ec8ab613f822989a11a2f2ce70ac


nit: It will be better for us replace package with chunk here and other places in the miner?

```diff
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index 1bfe7714440..e80d55c1f31 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -192,12 +192,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
return std::move(pblocktemplate);
}

-bool Bloc
...

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2436048271)
In "test: rewrite PopulateMempool to not violate mempool policy (cluster size) limits" afb9003bc602a77ed15bf4e79277d960937a5c28

nitty-nit: instead of undo, spent outputs might be better?

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2436033865)
In "Select transactions for blocks based on chunk feerate" a8be743aeb42ec8ab613f822989a11a2f2ce70ac

nit: maybe reserve again after clear here?

💬 fanquake commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3411433463)
> prevents users from achieving optimal performance,

Clang on Windows produces better performing binaries than MSVC?

💬 fanquake commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3411437444)
> I Though, I am not sure if we should vendor it ourselves.

Yea. I think this only becomes a discussion if we are also using it for both architectures. We wont vendor a whole new toolchain just for aarch64.

💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3411444974)
> > prevents users from achieving optimal performance,
>
> Clang on Windows produces better performing binaries than MSVC?

Yes. I forgot to put this link into my previous comment: https://github.com/bitcoin-core/secp256k1/pull/1681.

💬 fanquake commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3411451999)
> Yes. I forgot to put this link into my previous comment: https://github.com/bitcoin-core/secp256k1/pull/1681.

Have you seen the same from actually running Core, or it's benchmarks, or an IBD?

💬 hebasto commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-3411465250)
> I don't understand this change. It seems setting CC_FOR_BUILD to CC is exactly the opposite of what we want to do?
>
> @hebasto's original change makes more sense to me:
>
> ```
> BUILD = $(shell env --unset CC ./config.guess)
> ```
>
> That ignores what's set in CC when detecting the native platform, which seems like the correct behavior to me.

Reverted to the initial variant.

💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3411481697)
> > Yes. I forgot to put this link into my previous comment: [bitcoin-core/secp256k1#1681](https://github.com/bitcoin-core/secp256k1/pull/1681).
>
> Have you seen the same from actually running Core, or it's benchmarks, or an IBD?

I'll refresh and post benchmarks shortly.

💬 fanquake commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3411488504)
```bash

D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1531,33): warning : variable 'ext_flag' may be uninitialized when used here [-Wconditional-uninitialized] [D:\a\bitcoin\bitcoin\build\src\bitcoin_consensus.vcxproj]
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1733,10): note: in instantiation of function template specialization 'SignatureHashSchnorr<CTransaction>' requested here
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1481,21): note: initialize the variable 'ext_flag' to
...

🤔 instagibbs reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3345064005)
reviewed through 10872f7ec923803f711cd2c3af93a0e17121330e

focused on commit structure and comprehension only, next will be focusing on testing

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2436069486)
729cec4f6ddb880210b7e0011fff8b0a5f88c933

micro-nit: readability of format prior to this commit was better, can it remain a multiline ctor for MiniMinerMempoolEntry with annotations?

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2436500068)
10872f7ec923803f711cd2c3af93a0e17121330e

You're still looking up ancestors in TRUC prior to knowing how large the resulting graph would be, but you're only doing it once per package tx, so the computation is bounded nicely.

Maybe leave a comment to that effect or stick that in the commit message?