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

💬 fanquake commented on issue "ci: short read: expected xxxxxxxxx bytes but got xxxxxxxxx: unexpected EOF":
(https://github.com/bitcoin/bitcoin/issues/33640#issuecomment-3411520463)
IRC yesterday: https://github.com/bitcoin/bitcoin/actions/runs/18534138379/job/52824610461

📝 fanquake opened a pull request: "doc: archive release notes for v28.3"
(https://github.com/bitcoin/bitcoin/pull/33642)

👍 stickies-v approved a pull request: "doc: archive release notes for v28.3"
(https://github.com/bitcoin/bitcoin/pull/33642#pullrequestreview-3345767371)
ACK ceea24b92153d799dfaed1874c86d91c5d002d68 - matches https://github.com/bitcoin/bitcoin/blob/da5f5de4055ecad75490820c0f51db007a0a7d8f/doc/release-notes.md