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

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514282845)
`getCoinbase()` returns a `CoinbaseTemplate` struct which has an `output` field, which itself is an array with zero or more elements.

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

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514286773)
The `witness` field could also be used, isn't it?

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514290067)
Refactoring these deprecated methods would add to the review burden. And there's still ongoing discussion above about whether this PR should even keep them

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514291696)
```suggestion
* @note deprecated. Scan outputs from getCoinbase() outputs field for the
```