allow Group Channels to contain Extended Channels

[plebhash]

based on #157

this PR is modifying the Sv2 Spec so that Group Channels can also contain Extended Channels

most changes should be (relatively) backward compatible, with one exception:

OpenExtendedMiningChannel.Success now has a new field group_channel_id

[jakubtrnka]

I find this statement quite confusing. Although I see it was already present before

I personally would prefer having an uniform approach for all kind of channels:
First:
coinbase is concatenation of

1. coinbase prefix
2. extranonce prefix
3. extranonce
4. coinbase suffix

Second.

• standard channels have always empty extranonce

Third

• no other restrictions - extended channels should technically be allowed with empty extranonce prefix and zero-length extranonce at the same time. I know it's perhaps silly, but I don't think it should be outright forbidden here. The client implementation needs to handle the case anyway, regardless of whether it's allowed here in this document.

[jakubtrnka]

Also this

…the Merkle root is then defined deterministically…

is pretty strange think to say. Is it somewhere defined non-deterministically?

And generally,
I don't think the specification should talk about the "conversion of extended mining job into a set of distinct standard jobs" in this place. Everyone knows how to do that. It follows directly from the logic of hashing space established by extranonces and prefixes.

What would be great thing to know in this place - and what I don't see - is how exactly is the merkle root shall be calculated.
I mean step by step - starting with coinbase, going through the branch. How is that with endianity. And providing a test vector would also be awesome.
Something like:

let mut coinbase = vec![];
coinbase.extend_from_slice(cb_prefix);
coinbase.extend_from_slice(extranonce_prefix);
coinbase.extend_from_slice(extranonce);
coinbase.extend_from_slice(cb_suffix);
let coinbase_hash: [u8; 32] = sha256(sha256(coinbase));

// let merkle_path: Vec<Uint256Bytes>;
let raw_merkle_root = merkle_path
    .iter()
    .fold(coinbase_hash, |state, merkle_leaf| {
        let mut sha256_state = Sha256::context();
        sha256_state.update(&state);
        sha256_state.update(&merkle_leaf.to_le_bytes()); // little endian representation of U256 number
        sha256(sha256_state.finish())
    });
Uint256Bytes::from_le_bytes(raw_merkle_root) // Using the U256 number from its little endian representation byte array

[plebhash]

I'll have to read and digest this, I'll answer later

[coleFD]

it sounds like you are saying:

1. just say "The full coinbase is then constructed as follows: coinbase_tx_prefix + extranonce_prefix + extranonce + coinbase_tx_suffix where extranonce is zero-length bytes for standard jobs" so coinbase reconstruction is consistent across the spec
2. provide a clear example of logic or psuedo code to be extremely clear how to reconstruct the coinbase

Which i think both are good ideas

[plebhash]

I removed the sentences deemed redundant via 97afd01

but I should point out that a few lines below (which probably went unnoticed because they weren't in the PR diff), we already have a description of how to reconstruct the full coinbase tx that's pretty similar to what was suggested here... so I'm not sure we would get much value from adding it again?

sv2-spec/05-Mining-Protocol.md

Lines 425 to 428 in 2c21bcf

	\*The full coinbase is constructed by inserting one of the following:
	- For a **standard channel**: `extranonce_prefix`
	- For an **extended channel**: `extranonce_prefix + extranonce (=N bytes)`, where `N` is the negotiated extranonce space for the channel (`OpenMiningChannel.Success.extranonce_size`)

[plebhash]

added pseudocode for merkle root calculation via 547fa1b

[plebhash]

@Fi3 this should be almost ready for merging... ACK from you?

[Fi3]

I would add 2 messages instead of modifying this one.

OpenExtendedMiningChannelGrouped and OpenExtendedMiningChannelGrouped.Succes

so we do not break compatibility with old clients. Not an hard requirements.

[plebhash]

@jakubtrnka what do you think?

[plebhash]

I see the benefit in avoiding the compatibility chaos.

On the other hand, the spec will get more counter-intuitive to fresh eyes.

[jakubtrnka]

To be fair, I don't like the idea of having two separate messages for the same thing.

What about this?

• Using SetGroupChannel in conjunction with this Success message? ⇔ full compatibility, but slightly bigger overhead
• OR Since this is a trailing field in message payload, can that be somehow optional? ⇔ still kindof compatible
• Maybe there could be something like autogroup? using 0xffff_ffff unless specified otherwise
• Maybe quite broadly, we can introduce default values for everything in sv2. You can leave out last N fields in the payload, which will then be set to their default values. This would open the way to adding more fields in the future

In our case we could say default group id is 0xffff_ffff, and all message that don't have it in the Success msg payload in the end could assume this value.
Maybe also SetupConnection flag DEFAULT_VALUES should be used to indicate that the client does understand this feature.

[jakubtrnka]

How did the discussion about submit_shares_extended end, where additional information needed to be passed along the submit?
Can we do the same here?

[Fi3]

btw if we are really committed on making things simpler imo we should remove group channel standard channel and just have extended channels.

[jakubtrnka]

@Fi3 I guess this was a joke. But if you think about it a bit, There won't be much use cases for standard channels in the future in my opinion.
Standard mining jobs (even if delivered by group as an extended one), don't have enough enthropy to satisfy modern miners with 300+ TH/s (let's not forget that ASICs can't use all of the space in the header). So the theoretical 280 TH per mining job per nTime, is not enough for even 280 TH/s miner with nTime rolling.

[Fi3]

no I'm damn serious but I think is to late at that point

[plebhash]

so I propose the following:

we proceed this breaking change and coordinate across the ecosystem to make sure everyone is using the same updated OpenExtendedMiningChannel.Success (with the added group_channel_id field)

---

with regards to making Standard Channels obsolete, we can discuss this in the future... and IMO protocol versioning will definitely be required for that kind of change

[Fi3]

btw just to make my point more clear. I'm not opposing to what we have now. But I would support an be very happy in changing the spec to just use extended channels remove the concept of standard channels and header only mining, and integrate the group channel (or what we really need there) in the extended channel. I would rather have just one thing rather than (extended + standard (hom and non)) * group

[Fi3]

I don't understand why you need to say how to calculate the m root? It must be calculated how bitcoin do it otherwise nothing work. This section is confusing and redundant imo.

[plebhash]

this came as suggestions from @jakubtrnka and @coleFD #162 (comment)

I had resolved the thread, just unresolved it for visibility