mining: add always_add_coinbase_commitment option #35393

Previously the CoinbaseTx returned by the Mining IPC interface always included the witness field and SegWit OP_RETURN output. This is unnecessary for empty blocks, and for blocks without any SegWit spends.

This PR omits them for blocks with SegWit spends, but gives clients the option to keep them. RPC behavior is unchanged.

Strictly speaking it's a breaking change for <= v31 IPC clients, because they can't opt out of the new behavior. The interface is experimental so this is fine in general.

This is a breaking change for current versions of SRI, see #35393 (comment).

Two alternatives are worth considering:

1. Always omit them for IPC clients, don't expose always_add_coinbase_commitment (it's still needed internally for the RPC exception)
2. Keep the current behavior of always adding these fields; empty blocks don't need the space savings

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35393.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

@plebhash do you know of the top of your head how SRI deals with empty blocks? Does it always add a witness to the coinbase? If so, then perhaps option (2) in the PR description is better.

With #35392 closed, I dropped the backwards compatibility test and folded aa31e971570bd53d282fa89b49f3ac840bf636ca mining: add always_add_coinbase_commitment option into e3706fc9c5a89893da78d79cac5d0103c3cc7dff mining: default coinbase commitment option off. The original branch is at pr/35393.0

I clarified the release note:

It's important that IPC clients DO NOT unconditionally add a witness to the coinbase transaction they pass to submitSolution().

Unfortunately SRI unconditionally sets the coinbase witness to all zeros. That's already known to be unsafe for future soft forks, but this PR would break the current version of SRI for empty blocks too. They have a fast release cycle though.

Actually the SRI compatibility issue is worse than I thought. It requires a Stratum v2 spec change: https://github.com/stratum-mining/sv2-spec/issues/166.

The Template Provider role is typically the IPC client. It needs to pass the (optional) witness commitment onward to the Job Declarator Client (or Pool) role. Since SRI now has a built-in template provider role, they could just ignore this part of the spec. But then sv2-tp is obsoleted.

Although I think it would be straight-forward to roll out, I don't see much movement on this spec change.

I don't think this change is useful enough to require downstream projects to change their priority.

I could switch the default to true, but then it becomes one of those options where the default is bad.

So instead I'm just going to close this. We can revisit if this part of the Stratum v2 spec is updated.

@plebhash do you know of the top of your head how SRI deals with empty blocks? Does it always add a witness to the coinbase? If so, then perhaps option (2) in the PR description is better.

currently, SRI always assumes the Coinbase Witness is 0x000...000

in order for it to use a different value, we would need to add a new NewTemplate.coinbase_witness field on the spec, as discussed in https://github.com/stratum-mining/sv2-spec/discussions/156#discussioncomment-14277641

nevertheless, the Witness Commitment sent by TP arrives via NewTemplate.coinbase_tx_outputs and remains untouched by SRI

The problem is that you need either both (witness and OP_RETURN commitment) or neither. SRI could rely on the absense of the OP_RETURN to infer that the coinbase shouldn't have a witness, but that feels brittle.