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

👍 hebasto approved a pull request: " test: [refactor] Use reference over ptr to chainman "
(https://github.com/bitcoin/bitcoin/pull/33840#pullrequestreview-3448108401)
ACK 7a4901c9029687ad2b37f6d929d4c8fe96c15db3, I have reviewed the code and it looks OK.

💬 maflcko commented on pull request " test: [refactor] Use reference over ptr to chainman ":
(https://github.com/bitcoin/bitcoin/pull/33840#discussion_r2514270102)
I think that is clear from reading the code, so will leave as-is for now. No opinion about `string_view`. Will leave as-is for now, to not invalidate the two reviews.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514270707)
Done

💬 Eunovo commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2514271610)
> There's also our `thread_local` clang-tidy plugin: https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy.

I don't see why we would need a thread local variable with a non-trivial desctructor to implement this.

> i remember that in the past we had tons of issues with thread-local variables. They've been a pain in the ass on some platforms, and decided to not use them except for one case. Not sure if this is still the case, but if not, this needs to be updated:
> ht
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514273574)
I think the comparison below should be enough, and I want to make it as simple as possible to later drop the deprecated method.

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514274303)
As a progression towards `getCoinbase` & deprecation, this implementation could invoke ExtractCoinbaseTemplate`, and return the `witness` field (after uint256 -> unsigned char array type conversion)

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3516981508)
> There is no direct C++ unit test

No strong opinion on whether we need one.