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

💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514193806)
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#issuecomment-3516890731)
There is no direct C++ unit test for `getCoinbase`/`ExtractCoinbaseTemplate`, would it make sense to add one? Or is the interface test sufficient (`test/functional/interface_ipc.py`)?

📝 fanquake opened a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851)
Pulled out of #33537.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3516899959)
Note that the third commit seems to conflict with what is proposed for the BlockTemplate Manager in #33421, see https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3448014534. The plan there is to copy the template `CBlock` and apply the solution to that copy.

The choice between modifying in place and copying is not material to this pull request, and the code and test can be modified later, but it's probably better to decide which direction to go first.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514218703)
Sorry that's leftover debugging code.

📝 hebasto opened a pull request: "test, refactor: Fix `-Warray-bounds` warning"
(https://github.com/bitcoin/bitcoin/pull/33852)
This warning is triggered by GCC 14 when cross-compiling for Windows.

Split from https://github.com/bitcoin/bitcoin/pull/33775/ and https://github.com/bitcoin/bitcoin/pull/33764.

📝 TheCharlatan opened a pull request: "kernel: Allow null arguments for serialized data"
(https://github.com/bitcoin/bitcoin/pull/33853)
An empty span constructed from an empty vector may have a null data pointer depending on the implementation. Remove the BITCOINKERNEL_ARG_NONNULL requirement for these arguments and instead handle such null arguments in the implementation.

Also cherry-picked from #33845 to show that CI task passing now.

💬 maflcko commented on pull request "test, refactor: Fix `-Warray-bounds` warning":
(https://github.com/bitcoin/bitcoin/pull/33852#issuecomment-3516945514)
see https://github.com/bitcoin/bitcoin/pull/33840, which has a few more?

💬 hebasto commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3516949395)
cc @laanwj

💬 maflcko commented on pull request " test: [refactor] Use reference over ptr to chainman ":
(https://github.com/bitcoin/bitcoin/pull/33840#discussion_r2514251165)
The goal is to just change the line, so that it matches all other test cases in this file. Happy to review a follow-up to apply the clang-tidy use-ranges to the `src/test` directory (or all files). But I want to keep this one minimal for now.

💬 hebasto commented on pull request "test, refactor: Fix `-Warray-bounds` warning":
(https://github.com/bitcoin/bitcoin/pull/33852#issuecomment-3516954225)
> see #33840, which has a few more?

Closing in favour of #33840.

✅ hebasto closed a pull request: "test, refactor: Fix `-Warray-bounds` warning"
(https://github.com/bitcoin/bitcoin/pull/33852)

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514258528)
In case of `has_witness`, `coinbase_commitment` and `coinbase_template.witness` could be directly compared here.

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514261887)
```suggestion
* @note deprecated: use fields from getCoinbase() output
```