add BIP141 conventions for serialized coinbase

[plebhash]

as discussed in #149

[GitGab19]

ACK

[Sjors]

My SegWit knowledge is a bit rusty, but IIRC the coinbase transaction doesn't need to have a witness, and typically doesn't. It doesn't matter what the outputs are, i.e. paying to a segwit address doesn't make it a segwit transaction.

But if a pool wants to add a witness for some reason, then it becomes a witness transaction and they need to correctly serialize it.

Could be worth elaborating a bit on this in the text.

[plebhash]

the coinbase transaction doesn't need to have a witness, and typically doesn't.

hmm, I'd say that's not correct.

If there are SegWit transactions in the block, the Coinbase transaction does need to have a witness (or the so called "witness reserved value").

BIP141 has a section called Commitment structure that says the following:

The commitment is recorded in a scriptPubKey of the coinbase transaction. It must be at least 38 bytes, with the first 6-byte of 0x6a24aa21a9ed, that is:

  1-byte - OP_RETURN (0x6a)
  1-byte - Push the following 36 bytes (0x24)
  4-byte - Commitment header (0xaa21a9ed)
 32-byte - Commitment hash: Double-SHA256(witness root hash|witness reserved value)

39th byte onwards: Optional data with no consensus meaning

and the coinbase's input's witness must consist of a single 32-byte array for the witness reserved value.

If there are more than one scriptPubKey matching the pattern, the one with highest output index is assumed to be the commitment.

If all transactions in a block do not have witness data, the commitment is optional.

The commitment hash takes the Coinbase's witness reserved value as part of its pre-image.

Sure, you could argue this is TIPICALLY 0x000...000, but that's different than saying it doesn't exist, especially given that we're talking about the pre-image of a consensus-critical hash.

BIP141 also has a section called Extensible commitment structure that says:

The new commitment in coinbase transaction is a hash of the witness root hash and a witness reserved value. The witness reserved value currently has no consensus meaning, but in the future allows new commitment values for future softforks. For example, if a new consensus-critical commitment is required in the future, the commitment in coinbase becomes:

 Double-SHA256(Witness root hash|Hash(new commitment|witness reserved value))

For backward compatibility, the Hash(new commitment|witness reserved value) will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.

which means this field could eventually carry something different than 0x000...000

---

It doesn't matter what the outputs are, i.e. paying to a segwit address doesn't make it a segwit transaction.

correct.

what forces the Coinbase to be a SegWit transaction (with marker and flag, which are also relevant on this discussion, aside from a potentially-zero'ed witness) is the fact that there are SegWit transactions in the block, which forces the Coinbase to carry a witness commitment, which in turn forces the Coinbase to have a witness (even if it's zero'ed).

we can only get away with a non-SegWit Coinbase if all transactions in the block do not have witness data (or the block is empty), in which case the commitment is optional.

[Sjors]

I see where my confusion came from. I didn't realize that setting the coinbase witness (to some 32 byte value) was already mandatory (for SegWit blocks). I thought it was just a proposed hack to deal with BIP30 / BIP34.

Found the relevant code in Bitcoin Core:

In validation.h:

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

In validation.cpp:

void ChainstateManager::UpdateUncommittedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev) const
{
    int commitpos = GetWitnessCommitmentIndex(block);
    static const std::vector<unsigned char> nonce(32, 0x00);
    if (commitpos != NO_WITNESS_COMMITMENT && DeploymentActiveAfter(pindexPrev, *this, Consensus::DEPLOYMENT_SEGWIT) && !block.vtx[0]->HasWitness()) {
        CMutableTransaction tx(*block.vtx[0]);
        tx.vin[0].scriptWitness.stack.resize(1);
        tx.vin[0].scriptWitness.stack[0] = nonce;
        block.vtx[0] = MakeTransactionRef(std::move(tx));
    }
}

This is what sets the coinbase witness to 0x00...00 if there is at least one SegWit transaction in the block. It's called at the end of GenerateCoinbaseCommitment, which sets the more widely known OP_RETURN, and this indirection is probably why I hadn't noticed it.

My confusion came from an earlier discussion about using the witness commitment to fix BIP30 / BIP34: https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710:

make the witness commitment mandatory in all coinbase transactions (h/t Greg Sanders). I have yet to check if there is any violation of this but i’d be surprised if there is a pre-Segwit coinbase transaction with an output pushing exactly the witness commitment header followed by 32 0x00 bytes

The new consensus rule would not require 0x00...00 specifically, there just has to be a commitment.

One practical way to achieve that is for Bitcoin Core to always set 0x00...00 as the coinbase witness, even if there's no segwit transactions in the block. Until such time that a new consensus rule comes along that requires a different value.

Eventually BIP54 picked a different approach to deal with BIP30 / BIP34, so I didn't think about this anymore.

Interestingly there was an earlier discussion on the Sv2 spec to add this value to the NewTemplate message, but this was decided against: #15

[Sjors]

So maybe add:

For a block that contains any SegWit transactions (in practice almost any non-empty block), the coinbase transaction MUST have a witness as well as an OP_RETURN output.

The OP_RETURN output is provided by Template Provider in the coinbase_tx_outputs field.

The coinbase witness is not provided by the Template Provider. Although today it's typically 0x00..00, no assumption should be made about this value. The only consensus requirement is that it is a 32 byte push, but future consensus rules may require a different value.

For a block that does not contain any SegWit transactions, the coinbase transaction generated by current versions of Bitcoin Core won't have a witness. But that's not a consensus rule. Future versions and other node implements MAY set a witness anyway.

In other words, if the Template Provider provides an empty block, it is not safe to assume its coinbase transaction doesn't have a witness.

[plebhash]

The OP_RETURN output is provided by Template Provider in the coinbase_tx_outputs field.

Although today it's typically 0x00..00, no assumption should be made about this value.

These two sentences are conflicting.

TP provides an OP_RETURN with a commitment hash that's calculated with a Coinbase witness as part of its pre-image. So whoever is downstream to TP is pretty much forced to abide by the Coinbase witness that TP chose, despite the fact that this is not really communicated anywhere on Sv2 TDP.

Unfortunately, there is a NewTemplate.coinbase_tx_outputs (where OP_RETURN with witness commitments is sent), but there's no NewTemplate.coinbase_witness or alike.

As expected, our current TP implementation does it with a 0x00..00 witness.

I validated this empirically, by trying to propagate a block with a witness commitment sent by our current TP and a Coinbase witness different than 0x00..00. The block was rejected by its peers.

So either:

• A: TP should stop providing the OP_RETURN with the witness commitment, leaving it for the receiving end to decide which Coinbase witness they want to use and calculate a witness commitment accordingly
  OR
• B: we establish some convention that says it's always 0x00..00 until some soft-fork changes it into consensus-critical

Option A would pretty much render NewTemplate.coinbase_tx_outputs useless. Or at least, AFAIK, sending Witness commitment is the only meaningful purpose for this field.

Option B would force a spec upgrade if-and-when a soft-fork changes things with regards to Coinbase's witness reserved value.

[Sjors]

Option B would force a spec upgrade if-and-when a soft-fork changes things with regards to Coinbase's witness reserved value.

This would be a disaster, because it's hard enough to get miners to upgrade their node software, let alone their entire sv2 stack.

It sounds like closing #15 was a mistake?

[Sjors]

As expected, our current TP implementation does it with a 0x00..00 witness.

I'm a bit confused about what you mean here. You're referring to the TP I implemented (sv2-tp and its bitcoind -sv2 predecessor), right? This lets Bitcoin Core generate a block template. The coinbase it uses for that is never communicated, but it indeed uses 0x00..00 and the OP_RETURN value reflects that.

I validated this empirically, by trying to propagate a block with a witness commitment sent by our current TP and a Coinbase witness different than 0x00..00. The block was rejected by its peers.

For SubmitSolution the coinbase has to be reconstructed by other SRI roles. The Template Provider just passes it along. Are you saying that if these other roles pick a different value than 0x00..00 then the result is broken? That would make sense.

[plebhash]

It sounds like closing #15 was a mistake?

According to my understanding, yes. Or at least as far as we expect:

• the Template Provider to be the one who calculates the Witness Commitment
• Sv2 to be reasonably future-proof with regards to soft-forks

When it comes to who's responsible for calculating the Witness Commitment, an alternative option would be to shift this responsibility to the Sv2 role who receives NewTemplate. The tradeoffs would be:

• this would delay creation of jobs, since:
  • a RequestTransactionData/.Success round-trip would be necessary
  • we'd have to wait for the calculation of the Witness Root
• we don't know what kind of consensus-critical data would have to be committed into witness reserved value on this hypothetical soft fork... does the Sv2 role have all the necessary data to make that decision, or is that something that only a Bitcoin Full Node could do?

So having TP be responsible for calculating the Witness Commitment feels like a sensible design decision.

But as long as NewTemplate carries no information about Coinbase witness (which was proposed and rejected on #15), the Sv2 role that receives this message and wants to propagate a block later has no option but to ASSUME a value for Coinbase witness. And today we live with this "implied convention" of using 0x000...000, which is an intuitively sensible choice (but really could have been anything, given that this field is not consensus-critical).

It's kinda sad to realize that the protocol is not future-proof on this regard, especially given that this could have been solved with just one extra field on NewTemplate, as #15 proposed.

However, I must also point out that the purpose of this current PR is not to advocate for spec changes, but rather to simply establish conventions that avoids confusion and enables smooth interoperability with what the Bitcoin protocol consensus looks like today. So I'm pretty ok with saying "just use 0x000...000 for witness reserved value" as an official convention and leaving future/hypothetical problems for the future.

In case protocol stakeholders (@jakubtrnka @Fi3 @TheBlueMatt) feel this deserves a discussion for spec change, we can do it on a separate issue/PR.

[plebhash]

I added a few words suggested by @Sjors #153 (comment)

but not all

that's because I feel this topic about future implications of Coinbase witness is derailing into a discussion on its own, and as stated above I'd like to keep the scope of this PR constrained to conventions around serialization without diving into potential spec changes

so I crated #156 as a place where we can discuss about it more freely

[Sjors]

I do think it's worth mentioning that the witness commitment is (currently) assumed to be zero.

[plebhash]

you mean the Coinbase witness (currently aka witness reserved value) is assumed to be zero, right?

the witness commitment is 0x6a24aa21a9ed followed by the hash of the concatenation of witness root and witness reserved value, so we don't really expect it to be zero.

anyways, just added a commit for this:

7251e64

[Sjors]

you mean the Coinbase witness (currently aka witness reserved value)

Yes

[Sjors]

So IIUC NewExtendedMiningJob is the only place that needs to strip witness data. Maybe that suggests a defect in the spec? I'm not familiar with how extended mining jobs work, but isn't there a way to avoid stripping witness data still give clients the data they need to get the correct merkle_root?

[plebhash]

Ok, first some recap on Sv2 Standard vs Extended Jobs:

Standard Jobs have a NewMiningJob.merkle_root field. That's meant to be an innovation that makes messages smaller and puts the burden of Merkle Root calculation on the Server. There's no extranonce rolling.

Extended Jobs are closer to Sv1, in the sense that there's a coinbase_tx_prefix (akin to Sv1 Coinbase1) and coinbase_tx_suffix (akin to Sv1 Coinbase2). That's meant to allow either:

• broadcasting a single job to multiple channels
• allowing extranonce rolling

---

AFAIK most Sv1 servers send their mining.notify with Coinbase1 + Coinbase2 that's already stripped of BIP141 fields. Maybe some Sv1 firmware is able to detect whether that's not the case and strip it afterwards, but according to @Fi3 that's not ubiquous, so not safe to assume.

Until recently, SRI was sending and expecting NewExtendedMiningJob messages that contained BIP141 fields serialized into coinbase_tx_prefix and coinbase_tx_suffix. Until stratum-mining/stratum#1399 led us to realize that Braiins was doing Extended Jobs that follow the Sv1 pattern of stripping those.

isn't there a way to avoid stripping witness data still give clients the data they need to get the correct merkle_root

We could leave it up for the client to figure out whether NewExtendedMiningJob already stripped BIP141 or not, and stripping it before hashing. But @TheBlueMatt opposed this idea here and here (with some mixup in terminology).

I agree with you that it's a bit confusing to follow one pattern for one message and a different pattern in other messages. @Fi3 also pointed this out here, which motivated the addition of the new BIP141 section into 03-Protocol-Overview.md on this PR.

Overall, when it comes to NewExtendedMiningJob I'm not really opinionated towards doing one way or the other.

But I'm strongly opinionated that it's of upmost importance to have clear (even if somewhat counter-intuitive) conventions on the spec, which is why I'm pushing for these discussions. Not having anything is a recipe for interoperability hell, as we already started to see with stratum-mining/stratum#1399

[Sjors]

I think we should have a clear distinction between Stratum v2 and Stratum v1, with the Translator role taking care of serialisation changes.

[plebhash]

in fairness, the receiving end of NewExtendedMiningJob (be it Sv2 firmware or tProxy) has no purpose for BIP141 fields, given that coinbase_tx_prefix + coinbase_tx_suffix are only meant to enable calculation of the Coinbase's txid then merkle_root

so one could argue that forcing NewExtendedMiningJob to carry BIP141 would be bloating the message size unnecessarily, and we'd be sacrificing bandwidth efficiency with no real purpose other than making the spec slightly less confusing for the reader

as much as I value clarity and cleanness in the spec, this feels like a reasonable tradeoff?

[Sjors]

I generally think that a tiny increase in message size is worth the reduction in complexity, unless it you're at the edge of some network packet size and want to prevent roundtrips.

But for the purpose of this PR it may be better to just document things as they are now.

[Sjors]

I opened https://github.com/Sjors/sv2-tp/pull/10 which adds coinbase_witness_reserved_value on the TP side. It's a U256 unlike in #15, because the consensus rule require exactly one stack element of exactly that size. I'd be curious to see what the matching SRI changes would look like.

There's absolutely no rush to do this, since there are currently no soft fork proposals that change the witness reserve value. And I can trivially keep the PR rebased. However, the switch to IPC may be a good opportunity to make a breaking change.

[Sjors]

Another way you can test the impact of the assumed witness reserved value is by patching Bitcoin Core:

diff --git a/src/validation.cpp b/src/validation.cpp
index 8fcc719a68..c231899fd3 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4079,7 +4079,7 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
 void ChainstateManager::UpdateUncommittedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev) const
 {
     int commitpos = GetWitnessCommitmentIndex(block);
-    static const std::vector<unsigned char> nonce(32, 0x00);
+    static const std::vector<unsigned char> nonce(32, 0x01);
     if (commitpos != NO_WITNESS_COMMITMENT && DeploymentActiveAfter(pindexPrev, *this, Consensus::DEPLOYMENT_SEGWIT) && !block.vtx[0]->HasWitness()) {
         CMutableTransaction tx(*block.vtx[0]);
         tx.vin[0].scriptWitness.stack.resize(1);
@@ -4092,7 +4092,7 @@ std::vector<unsigned char> ChainstateManager::GenerateCoinbaseCommitment(CBlock&
 {
     std::vector<unsigned char> commitment;
     int commitpos = GetWitnessCommitmentIndex(block);
-    std::vector<unsigned char> ret(32, 0x00);
+    std::vector<unsigned char> ret(32, 0x01);
     if (commitpos == NO_WITNESS_COMMITMENT) {
         uint256 witnessroot = BlockWitnessMerkleRoot(block, nullptr);
         CHash256().Write(witnessroot).Write(ret).Finalize(witnessroot);

This code is only used for mining, both for tests (some Bitcoin Core tests will fail because they use hardcoded block and transaction hashes on regtest) and the Mining interface that's used by the Template Provider.

This should impact the block templates generated by the Template Provider. Once combined with https://github.com/Sjors/sv2-tp/pull/10 such a change should not break anything.

[Sjors]

ACK 93ca151

But I didn't study the NewExtendedMiningJob and DeclareMiningJob sections, since I'm mainly focussed on the Template Provider.