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

📝 Eunovo opened a pull request: "validation: fix assumevalid is ignored during reindex"
(https://github.com/bitcoin/bitcoin/pull/33854)
During a reindex, the assumevalid setting may be ignored for the following reasons:
- The assumevalid block is missing from the block index. This can occur if the block could not be downloaded before a previous IBD run was interrupted.
- The chainwork of the best header is below `minimumchainwork`. This happens when the previous IBD was stopped before connecting enough blocks to reach the required `minimumchainwork`. See [Issue #31494](https://github.com/bitcoin/bitcoin/issues/31494).

As a
...

✅ Eunovo closed a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615)

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3517155840)
Thanks for the reviews and suggestions @l0rinc @stickies-v @hodlinator @mzumsande @TheCharlatan @stratospher .
I have implemented an approach that uses the headers-sync approach in https://github.com/bitcoin/bitcoin/pull/33854 and will be closing this PR.

💬 glozow commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3517174948)
> Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB.

Can you explain why it's not good practice? I agree it's not as human-readable but I don't software minds either way. Not to be snarky, but I think the most widely adopted usage of this RPC interface must be to handle what it returns, including converting it before printing to users.

> This PR aims to show a PoC of how we could migrate to sat/vb in a backwards c
...