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

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514303921)
This returns the full `scriptPubKey` as generated by `GenerateCoinbaseCommitment`. Although you can derive that from the `witness` field, I don't think one should.

🚀 fanquake merged a pull request: " test: [refactor] Use reference over ptr to chainman "
(https://github.com/bitcoin/bitcoin/pull/33840)

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514313165)
Let's also wait and see what comes out of https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514193806 because if I have to modify the signature of `submitSolution()` (to return a `CBlock`), that's the kind of breaking change I think is worth making.

💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514314492)
My reasoning was to add extra assurance that the deprecated and new method gives the same result, it's a simple test. But it's fine as it is.

💬 TheCharlatan commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517090033)
Concept ACK

Thank you for contributing this @ryanofsky! We've discussed doing something similar a few months ago. This is close to how I hoped to implement the logging capabilities, but reviewers highlighted some inconsistencies with this approach: A secondary logger object cannot receive its own non-global options. The docstrings do at least still explain this even with this change, I am not sure if it is really less surprising for users. Maybe it would be preferable to push forward #30342 b
...

💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514393534)
No need to revive https://github.com/bitcoin/bitcoin/pull/33374 I saw it yesterday and will push an update here soon.
We already made a copy of the block here, we should copy the whole template, it is a cheap copy I think.