💬 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

💬 fanquake commented on issue "ci: short read: expected xxxxxxxxx bytes but got xxxxxxxxx: unexpected EOF":
(https://github.com/bitcoin/bitcoin/issues/33640#issuecomment-3411642707)
https://github.com/bitcoin/bitcoin/actions/runs/17769369035/job/50682078335?pr=33117

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

🤔 pablomartin4btc reviewed a pull request: "Fix Wayland visual glitches"
(https://github.com/bitcoin-core/gui/pull/904#pullrequestreview-3345811524)
Concept ACK

If you can, please check how this behave on #817.

👍 stickies-v approved a pull request: "Update leveldb subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33641#pullrequestreview-3345811711)
ACK 54ffe3de5b1d15f10516ea536a12e13cd7d338f3

Verified that subtree matches commit cad64b151dabe9ffe9771a54d7c9dbfb3355cefb, and the changes are trivial.

💬 hebasto commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2436716393)
> Since we already know the direction of development, why add more legacy code just for the sake of consistency?

It makes the current code more readable (at least for me).

👍 hebasto approved a pull request: "randomenv: Fix MinGW dllimport warning for `environ`"
(https://github.com/bitcoin/bitcoin/pull/33570#pullrequestreview-3345939519)
re-ACK 9610b0d1e28aeda02a2ddcf1f0591ae577c3e88e.

💬 hebasto commented on pull request "randomenv: Fix MinGW dllimport warning for `environ`":
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3411810718)
> We use mingw-w64 in the CI & Guix build, and it doesn't produce this warning.

GCC has no such a diagnostic: https://gcc.gnu.org/onlinedocs/gcc/Option-Summary.html.

Clang does: https://clang.llvm.org/docs/DiagnosticsReference.html#winconsistent-dllimport.