BIP54 proposes contraining the cointbase transaction nLockTime and nSequence fields. Our internal mining code has been doing this since #32155, but currently the fields are only communicated to IPC clients (see e.g. #33819).
This PR extends the getblocktemplate RPC to provide these fields.
Accompanying proposed extension to BIP54: https://github.com/bitcoin/bips/pull/2097
Even if BIP54 never activates, it makes sense to serve these new fields.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34419.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Added the coinbase nVersion.
Expand the getblocktemplate RPC result with two fields to prepare
for a possible BIP54 deployment in the future.
I dropped nVersion from the proposal, see https://github.com/bitcoin/bips/pull/2097#issuecomment-3997488850.
Although it's harmless to add, I also dropped it from the implementation here for clarity. It can be added in a followup.
Concept ACK
tACK e3d4f3f7feff584a5b5a74d4d3e4183242972650
This implements a BIP whose stated motivation is to help miners upgrade to be BIP 54 compatible. However, as i argued before, i don't think that adding these fields achieve the stated goal. Mining pool software already has all the necessary data to set the values appropriately (as evidenced by CKpool change here, ViaBTC-ancestor change here, or more generally the BIP 54 compatibility take-up). In fact, this change would introduce an artificial reliance on upgrading to a future Bitcoin Core version for BIP 54 compatibility, which actually makes migration an order of magnitude harder than just updating two hardcoded constants in mining pool software. Therefore i don't think we should make this change. Concept NACK.
@darosior would you object to this change if the references to BIP54 were removed? IIUC this PR also has non-BIP54 motivations, like making the IPC and RPC mining interfaces more consistent, and giving miners more information from the node so client code can be simpler and less error-prone.
I guess such an effort can be judged on its own, and would likely need to be a new PR since it would presumably overhaul both the motivation and the content of this PR. I remain skeptical that providing fields that we don't expect anybody to use is useful, but consistency between interfaces sounds good, sure.
(as evidenced by CKpool change here, ViaBTC-ancestor change here
I don't follow your argument. These are ad hoc implementations, which is inherently less safe than relying on getblocktemplate.
These changes also won't track any future soft fork on these fields, which would require yet another round of ad hoc updates.
In fact, this change would introduce an artificial reliance on upgrading to a future Bitcoin Core version for BIP 54 compatibility
No, it's a convenience, not a requirement. But if there is ever a soft fork to make BIP54 mandatory, then an upgrade does become mandatory - since otherwise a miner won't know when the new rules are actually enforced. So as long as this is merged before such time, it doesn't add any upgrade burden.
Closing this as up for grabs.
(as evidenced by CKpool change here, ViaBTC-ancestor change here
I don't follow your argument. These are ad hoc implementations, which is inherently less safe than relying on
getblocktemplate.These changes also won't track any future soft fork on these fields, which would require yet another round of ad hoc updates.
I think the comment just saying that the existence of those PRs proves that this PR is not necessary for BIP54 compatibility, even if it provides a safer, more convenient, more future-proof alternative to those approaches.
Overall, I agree with the pushback in #34419#pullrequestreview-4322245647 that the PR and release notes should not be advertising BIP54 compatibility, and maybe shouldn't mention BIP54 at all, because compatibility requirements are inherently confusing, and it's easy to imagine making the mistake that these changes are strictly required.
Also there is the possibility that if mining software is written depending on these changes, it might be less likely to get deployed if it requires a node update. (I believe this is the "artificial reliance" problem.)
But IMO, this change does seem like an improvement and it seems just not mentioning BIP54 in the release notes could make it uncontroversial if it got taken up again.
But IMO, this change does seem like an improvement and it seems just not mentioning BIP54 in the release notes could make it uncontroversial if it got taken up again.
In that case it would be best to add some additional fields to match what CoinbaseTx provides via IPC.
However, I expect some people to object to any change in getblocktemplate that doesn't have a corresponding BIP. And so this tends to go in circles. I'll just focus on IPC :-)