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

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514301870)
As a progression towards `getCoinbase` & deprecation, this implementation could internally also do what is suggested in the deprecation node:
invoke `ExtractCoinbaseTemplate`, scan outputs for the SegWit marker, and return the index.

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517016509)
Concept ACK

The added new method makes sense. The deprecating methods are only being marked in the header comments, they are kept, there is no formal plan for deprecation. But there is a fair plan for switching users, and keeping them is not an extra burden.