👍 laanwj approved a pull request: "refactor: Return uint64_t from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/33724#pullrequestreview-3447733525)
Code review ACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
Changes LGTM
- `test: Remove outdated comment`: trivial, no risk
- `refactor: [rpc] Remove cast when reporting serialized size`: obvious ACK. probably was a workaround for an old univalue version
- `move-only: Move CBlockFileInfo to kernel namespace`: good catch! verified move-only
- `refactor: Use fixed size ints over (un)signed ints for serialized values`: long-overdue change. There should be no difference in generated code (but i did no
...

💬 fanquake commented on pull request "build: Bump g++ minimum supported version to 12":
(https://github.com/bitcoin/bitcoin/pull/33842#issuecomment-3516552057)
Are we keeping `gccbug_90348` (for `-fstack-reuse` bugs), even though it will no-longer reproduce with our supported versions of GCC?

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3516563445)
> Should probably be based on #33775.

Sure. Rebased on on #33775 and drafted for now.

📝 hebasto converted_to_draft a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764)
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.

For more details about this migration, see:
- https://github.com/bitcoin/bitcoin/issues/30210
- https://github.com/bitcoin/bitcoin/pull/33593

A few items are outside the scope of this PR and are left for follow-up work:
1. The version of Debian's [cross-compiler](https://packages.debian.org/trixie/g++-mingw-w64-ucrt64) is 14.2.0, which differs from v
...

💬 pinheadmz commented on issue "ci: failure in `wallet_basic.py` KeyError: 'ischange'":
(https://github.com/bitcoin/bitcoin/issues/33848#issuecomment-3516579110)
This is the only such failure so far since merge?

👍 laanwj approved a pull request: "depends: drop qtbase_avoid_native_float16 qt patch"
(https://github.com/bitcoin/bitcoin/pull/33850#pullrequestreview-3447771251)
Code review ACK 169f93d2ac887a7295e87f0a69c2407cdd0a936e
Good to get rid of this patch. Using native float16 should be fine (and might even be a speed-up) on platforms that have this. If not, it's a good cleanup.

👍 rkrux approved a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3447791811)
crACK e346ecae830e10310979e5f64de63e043a383ff1

This advisory note is helpful.

💬 fanquake commented on pull request "guix: produce a `-static-pie` bitcoind":
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3516621838)
> Binary would be bigger

Yes ~2mb. I've added a comparison of bitcoind size for master vs this change, to the PR description.

🤔 rkrux reviewed a pull request: "Musig2 tests"
(https://github.com/bitcoin/bitcoin/pull/32724#pullrequestreview-3447877705)
> but having more detailed tests for the MuSig2 class itself improves test reporting/coverage.

There isn't a MuSig2 class per-se.

Concept ACK fa65a57a826192ffae48f62f10c216068e8236f4

💬 rkrux commented on pull request "Musig2 tests":
(https://github.com/bitcoin/bitcoin/pull/32724#discussion_r2514091946)
Nit: s/expected_xpub/expected_aggregate_xpub

```diff
- std::string expected_xpub;
+ std::string expected_aggregate_xpub;
```

💬 rkrux commented on pull request "Musig2 tests":
(https://github.com/bitcoin/bitcoin/pull/32724#discussion_r2514088966)
There's a utility function in the master branch that can be used instead of hardcoding.

```diff
--- a/src/test/musig2_tests.cpp
+++ b/src/test/musig2_tests.cpp
@@ -81,12 +81,7 @@ BOOST_AUTO_TEST_CASE(bip328)
BOOST_CHECK_MESSAGE(combined_keys == test.expected_aggregate_pubkey, "Test vector " << i << ": Aggregate pubkey mismatch");

// Create extended public key
- CExtPubKey extpub;
- extpub.nDepth = 0;
- std::fill(std::begin(extpub.vchFingerprint), std::e
...

💬 laanwj commented on pull request "guix: produce a more static `bitcoin-qt`":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3516832333)
i think making libxcb static is fine, as (as far as i know) no Linux distributions that make breaking interface changes to these. Nor does xcb have global hardcoded configuration such as paths, like the font libraries.

💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3516832835)
The following in the PR description had me confused:

> Client requests for block templates will specify the maximum age of the template in seconds; that is, how fresh they want the template to be:

I thought it meant that templates have an expiration time, but from 8af9ede0608f5bb4fe3eddf7d3fe4fbb57a4649d it's clear that the caller specifies how old cached templates can be (via `max_template_age`).

If you're having trouble with mock time in the tests / fuzzer, it might help to add some d
...

💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514084547)
In a620c0a69e6d2b0d29f0714f2c6c0bff33e2b4bf _miner: add `SimilarOptions` function_: both these comments should be in the header, as they explain both what this function does and why it's useful.

Maybe call it `ShareableOptions`, described as:

> Limit comparison to assembly options that impact the final block (without dummy coinbase scriptPubKey, with solution). Ignore options that can safely be shared between different callers.

💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514061828)
In 2ef43703998f679fc752a77727dd5c0824f6a90b _miner: add block creation time point to `CBlockTemplate`_: the most relevant anchor for creation time is what was in the mempool at that moment. So I would suggest moving this above `addPackageTxs`.

Since `cs_main` is locked throughout this function, this currently doesn't make a difference, but if we ever make locks more granular I could imagine taking a lock on just the mempool and setting the template timestamp at that exact moment.

(Somethin
...

💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514110184)
In a620c0a69e6d2b0d29f0714f2c6c0bff33e2b4bf _miner: add `SimilarOptions` function_: this seems brittle when we add options later.

Perhaps you can copy `a`, then set the shareable properties of `a_copy` to that of `b` and then do a regular comparison.

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514179494)
Could you add a short text before the value to the printout? Or if this is a debug-leftover, remove it.

💬 Sjors commented on something "":
(https://github.com/bitcoin/bitcoin/commit/9500dca555f7394cd3e51821312309d5b9633539#r170272499)
In 9500dca555f7394cd3e51821312309d5b9633539 _interfaces: create block template via block template cache_: how does this interact with #33745? The third commit there adds a comment and test that `submitSolution()` intentionally modifies the block in place, so that the client can call `getBlock()` afterwards.

I don't mind changing that behavior, but then I'll need to revive #33374 or change `submitSolution()` to optionally return the full block.

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514186161)
Maybe extract this part into a separate method; that can also serve as an example on how to build the cb tx from the getCoinbase() result, and leave the flow of this test more readable

💬 furszy commented on issue "ci: failure in `wallet_basic.py` KeyError: 'ischange'":
(https://github.com/bitcoin/bitcoin/issues/33848#issuecomment-3516865698)
Haven't tested it but most likely is a missing queue sync. E.g. call `self.nodes[0].syncwithvalidationinterfacequeue()` after line 541.