💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3411395080)
Rebased to refresh the CI.

🤔 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?