bip54: add coinbase locktime and sequence to GBT

[Sjors]

BIP54 proposes constraining the coinbase transaction nLockTime and nSequence fields.

Bitcoin Core's internal mining code has been doing this since v30.0 (see bitcoin/bitcoin#32155), but currently the fields are only communicated to IPC clients (i.e. Stratum v2, see e.g. bitcoin/bitcoin#33819).

This PR extends BIP54 with the following BIP22 fields (getblocktemplate RPC):

• lock_time
• sequence

Setting these fields makes miners forward compatible with BIP54, if it's ever activated, but is not the same as version bit signaling.

Reference implementation: bitcoin/bitcoin#34419

Mailinglist post: https://groups.google.com/g/bitcoindev/c/znBz5MA7_Bo/m/CY2uMIenAgAJ

The BIP22 "See Also" section is updated to link to these changes.

[darosior]

I sympathize with the motivation here, as i considered extending GBT with those fields a year ago, but eventually decided against. The reason is i don't think it solves a real problem. If a pool wants to be forward-compatible with BIP 54 it already has all necessary information (the block height). If a pool does not want to spend the time to make the small tweak to their mining pool software, this is not helping.

I also don't think it hurts, and i can hear the argument that if these fields are given consensus meaning, then GBT "should"( ™️ ) provide these fields. I just think it does not help with BIP 54 forward compatibility.

Regardless of the desirability of these additions, i think those should be specified as part of the GBT BIP, not BIP 54.

[darosior]

Meta-comment: Sjors already knows what i think as we've been discussing it for a couple weeks, but we are curious to hear from others.

[melvincarvalho]

BIP54’s consensus fixes are currently uncontroversial.

Bundling new getblocktemplate additions into it changes that and expands the policy surface. Those additions should be specified separately, where they belong.

[Sjors]

@darosior I tried to make it a separate BIP initially, and it just led to confusion: Sjors#1. I'm not opposed to making it a separate BIP, but it needs to be kept in sync with BIP54 and they have to refer to each other. So it just seems simpler to keep them in one place.

If a pool does not want to spend the time to make the small tweak to their mining pool software, this is not helping.

This seems a bit condescending. Pools may feel more comfortable simply using a few extra getblocktemplate fields than having to interpret the BIP themselves and implementing it, even if you offer them personal assistance.

@melvincarvalho do you prefer to discuss on the mailing list or here? Cross-posting isn't necessary. I already replied on the list for now.

[darosior]

I didn't mean to be condescending, thanks for pointing it out. What i meant is that they already have all the tools in hand, and i don't think providing the fields in the GBT response is materially lowering the bar to make their software compatible. In fact having to upgrade to a later Bitcoin Core version that serves these (fairly redundant) fields may be a higher bar than just using the block height information they already get in their existing setup.

[murchandamus]

It’s been a few weeks since there was action here, is there an update on what should happen here next?

[sedited]

if these fields are given consensus meaning, then GBT "should"( ™️ ) provide these fields.

I think I agree with this. Maybe also link to the text here from BIP22's "See Also" section?

It’s been a few weeks since there was action here, is there an update on what should happen here next?

Not the author here, but ideally the PR bitcoin/bitcoin#34419 should get some review.

[Sjors]

Maybe also link to the text here from BIP22's "See Also" section?

Done

[darosior]

if these fields are given consensus meaning, then GBT "should"( ™️ ) provide these fields.

I think I agree with this. Maybe also link to the text here from BIP22's "See Also" section?

I don't see why, there are fields with consensus meaning that are not provided by GBT. Some even on the coinbase, introduced in soft forks. Why for instance should we provide a coinbase sequence but not a coinbase witness? Why should we specify GBT to have a coinbase sequence but no witness commitment¹? The coinbase locktime is the one that would make the most sense, because there is a single consensus-valid value, if it wasn't redundant. Then providing the coinbase version is just random, especially as this patch proposes adding it to an unrelated BIP. The only thing i would say is worth specifying for GBT is that the "!' prefix should be set for the bip54 rule.

As per my previous comment i think the given rationale that it would help pools to upgrade is invalid. But maybe a separate BIP, in the spirit of BIP145, that describes expected behaviour of GBT users, may be marginally valuable. But that doesn't require adding unnecessary fields, besides maybe specifying the "!" prefix for the bip54 rule.

In any case, i do not think those changes belong in BIP 54.

Footnotes

1. i am aware Core provides it, but since this is a PR to the bips repository, this is still inconsistent with how the BIP specifications handled previous soft forks. ↩

[Sjors]

I dropped the version field so the fields added here are limited to what BIP54 needs.

Why for instance should we provide a coinbase sequence but not a coinbase witness?

I think it was a mistake to not include the coinbase witness and the SegWit OP_RETURN commitment in BIP145 (I previously assumed it was specified). It was left to the implementers to do the latter and Bitcoin Core magically adds the former in submitblock:

/**
 * Update uncommitted block structures (currently: only the witness reserved
 * value). This is safe for submitted blocks as long as they honor
 * default_witness_commitment from the template.
 */
void UpdateUncommittedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev) const;

This is brittle, as the comment explains.

Ideally every coinbase field that's not static (e.g. the coinbase prevout) and not inherently custom (e.g. the payouts) should be provided by getblocktemplate. That's also exactly what the current IPC code does.

However I don't want to further expand the scope of this PR. Since that means completeness is out of reach here, I dropped the version field.

Completeness could be achieved in a separate BIP. Although existing pool software already works under current consensus rules, it might still be of use for newly developed pool software (although they should just use IPC imo).