Support additional field in messages

[Fi3]

Right now the specification do not explicitly allow for having additional data in Sv2 messages, so the approach should be to define a new message in a new extension.

@TheBlueMatt said that this approach do not scale well, and suggested to use what lightning have been used for several years (not anymore btw). When we need an additional field we add it at the end of the message, the message len is encoded in the message header. If an old decoder receive a new message it see that the message have additional data (since header message len is different from what decoder expect) and just ignore the new data. When a new decoder receive a message it should parse before with the new message format and if it fail with the old message format.

@jakubtrnka think that the above approach is too messy. He prefer to add a suffix at the end of the message with a varint indicating the message version. Is similar to the above but additional data can only be a varint, then you have to use the varint to match the exact message kind. (@jakubtrnka said that this should be done carefully and that will elaborate on it later)

[jakubtrnka]

Currently the specification doesn't say anything about trailing data in a Frame payload, after a message has been deserialized.

The least we can do is to specify that decoders shouldn't reject those data with decoding error and instead just ignore them (of course assuming the decryption didn't fail on integrity check).

I think this discussion is bit too generic. I'm not really sure what goal are we trying to achieve with the "additional fields support".

case 1 is: We forgot to add some very important field into a message definition. We can see that this may happen from time to time. So we are seeking for a way to generally adding more fields into stratum messages

case 2: It's not that much about "we forgot to define some important field". We only noticed that there is a logical hole in the protocol framing and we can add arbitrary trailing data to the serialized payload. And we want to give it some structure. We don't plan changing the message definitions right now.

I'd suggest to call this just trailing-data. And I'd define that every decoder and stratum role must be able to accept any trailing data transparently. That means generic client can use a frames with or without it. It can append random data and protocol must keep working the same.
That doesn't say anything about a "special data". data prefixed for example with a magic constant. And followed by anything you like. Perhaps custom binary data, json string, serialized protobuf.
Perhaps there can be some semi-official list of SV2-anexes where the details are explained. That way you might amend your message with additional fields. The versioning wouldn't be as strict because it wouldn't be an official sv2. just a dialect thereof hidden in serialized payload which is generic sv2-payload trailing data. But this would have to be done carefully. I'm not entirely sure if it's a good idea to encourage this.

[plebhash]

I'm curious to know how this would affect SRI protocols crates.

I assume it has implications into codec_sv2 and framing_sv2?

[Fi3]

I'm curious to know how this would affect SRI protocols crates.

I assume it has implications into codec_sv2 and framing_sv2?

that will work out of the box with SRI, no need to change anyhitng.

[GitGab19]

I think this discussion is bit too generic. I'm not really sure what goal are we trying to achieve with the "additional fields support".

I fully agree with this point. What do we want to achieve? In the case of PR #110, it's a specific field to extend the SubmitSharesExtended message, for reasons explained and discussed in the google doc. Do we think we need some other necessary fields? If so, which fields? For which messages?

I'm not sold at all on adding general complexity for things which are not even specified, just thinking about possible needs in the future. If we think we need to add fields to messages, let's discuss about them, and if we can't manage them through flags or extensions, it probably means something is wrong with the specs.

[Fi3]

@GitGab19 I would say that between (1) and (2) we want to achieve (1):

case 1 is: We forgot to add some very important field into a message definition. We can see that this may happen from time to time. So we are seeking for a way to generally adding more fields into stratum messages.

Another thing that we need is adding 2 field to SubmitShare.Success for the auditing process. Additional data and signature.

So now we have 3 possibilities:

1. extensions
2. additional data with flags
3. additional data with magic constant

I can't stress enough how much is important the we all agree with one method, otherwise we will end up with an unmaintainable mess.

[GitGab19]

Auditing process as a whole sounds like something to be done as an extensions to me, otherwise what did we specified extensions for?

[Fi3]

The idea is to have this improved SubmitShare.Success that is part of the mining protocol and that will be saved by the SRI client, than the auditor, that can use whatever extension or somthing that have nothing to do with Sv2 will use the saved shares to do what want to do. Each pool will have different auditor. In that case do not make too much sense to implement it as a separate extension.

Is more then ok for a client to use a pool that send improved SubmitShare.Success or normal ones.

[GitGab19]

I'm missing some pieces. If a client wants to audit its hashrate, it needs this improved SubmitShare.Success, right?
If so, it means the client needs to be sure that pool is going to use the improved messages, right? In this case, it means that client is going to request the extension to be in place, isn't it?

[Fi3]

we risk to go off topic, to summarize what we discussed on discord it seems that we want an improved share success that can be used by the auditor, and we want the SRI client to support it. Then if the pool do not offer auditing and send down normal share should be supported as well. Making an extension for this case is not the best option since is a lot easier to just use additional field. Having additional data is a lot more easy to handle. Same thing for the SubmitSharesExtended message imo.

Now the question IMO is, the additional data should be specified in the spec: so there is only one valid set of additional data. Or should be something with no meaning as suggested by @jakubtrnka ?

[GitGab19]

I don't think we're going off topic, I think that we need to discuss tangent needs to get a proper consensus about how to move on.

Now the question IMO is, the additional data should be specified in the spec: so there is only one valid set of additional data. Or should be something with no meaning as suggested by @jakubtrnka ?

Regarding this, I don't like the idea about having something with no meaning, since it could lead to more fragmented implementations, opposed to a well specified extension.

[Fi3]

Regarding this, I don't like the idea about having something with no meaning, since it could lead to more fragmented implementations, opposed to a well specified extension.

yep this was the main reason to not using extensions in the first place.
Well specfied extension or additonal data with no meaning -> fragmented implementations
Additional with meaning -> only one possible superset of messages

[Fi3]

It think that extension can make sense for very specific things like the auditor role and a metric collector, or something to manage workers from upstream. Where for things that we expect to be shared by a lot of implementation like the improved submit share and submit share success we can use well specified additional data. With or without flag depend on the specific use case?

[GitGab19]

Well specfied extension or additonal data with no meaning -> fragmented implementations

How could a well specified extension lead to fragmented implementations? If you really think this, it means that same applies to current specs and to any eventual additional data with meaning.

Where for things that we expect to be shared by a lot of implementation like the improved submit share and submit share success we can use well specified additional data. With or without flag depend on the specific use case?

Agree with the flag here. But regarding additional data, if the reason is that they are related to use cases shared by a lot of implementations, it probably means that we are missing those data from specs. Otherwise, I see it better suited for an extension. Again, talking about auditor stuff to have a concrete example, it seems to me that it's going to be optional by definition (miners would opt-in this feature), so imo it should be a well defined extension.

[Fi3]

How could a well specified extension lead to fragmented implementations? If you really think this, it means that same applies to current specs and to any eventual additional data with meaning.

cause you end up with a lot of different extensions. If for every field that you want to add you do a new extension you end up in a world with a lot of extension, and no implementation will implement every extension so you end up with fragmented implementation.
On the contrary if you use the Sv2 spec to add field to messages in a non breaking way you still end up with several possible implementation but every additional field is part of the Sv2 spec, not of some obscure extension.

Agree with the flag here. But regarding additional data, if the reason is that they are related to use cases shared by a lot of implementations, it probably means that we are missing those data from specs. Otherwise, I see it better suited for an extension. Again, talking about auditor stuff to have a concrete example, it seems to me that it's going to be optional by definition (miners would opt-in this feature), so imo it should be a well defined extension.

the main point here is that is very very likely that in the future we will find other fields that should be part of the spec and are not, we can not know them now. So we need a way to add these fields. We can do a non compatible Sv2.2 and Sv2.3 ecc ecc or we can add the field as additional data at the end of the messages. Sometimes will make sense go with the former and other times with the latter.

it seems to me that it's going to be optional by definition (miners would opt-in this feature), so imo it should be a well defined extension.

why? having optional field at the end of a message is a lot better for this particular use case then having an extension. If you have an extension you need to ask the remote node if it support it and connection can have one more possible state (that will be added to all the possible state that an sv2 connection can already have), with optional field at the end you just know that when you have the field it means that you have to save the message somewhere so the auditior can use them later, super easy.

[GitGab19]

On the contrary if you use the Sv2 spec to add field to messages in a non breaking way you still end up with several possible implementation but every additional field is part of the Sv2 spec, not of some obscure extension.

This point makes no difference with regards to fragmentation from that perspective. If you want to implement a feature, and it's not in the spec, you end up looking at the extensions.

why? having optional field at the end of a message is a lot better for this particular use case then having an extension. If you have an extension you need to ask the remote node if it support it and connection can have one more possible state (that will be added to all the possible state that an sv2 connection can already have), with optional field at the end you just know that when you have the field it means that you have to save the message somewhere so the auditior can use them later, super easy.

Let's details things here: what happens when a client adds additional data, but server doesn't support the auditor stuff? Wouldn't it be better for the client to know it in advance? So that if a server doesn't support it, client can save work and just ask to another server. Does this make sense to you?

[Fi3]

This point makes no difference with regards to fragmentation from that perspective. If you want to implement a feature, and it's not in the spec, you end up looking at the extensions.

it make a lot of difference. Additional data are part of the spec extensions not.

Let's details things here: what happens when a client adds additional data, but server doesn't support the auditor stuff? Wouldn't it be better for the client to know it in advance? So that if a server doesn't support it, client can save work and just ask to another server. Does this make sense to you?

Server put the data not client. So what happen is that, we have the SRI client that of course should be able to support pool that do auditing and pool that dont't.

1. The client connect with the pool with the preferred method extended, standard with group, extend with custom
2. The client start mine
3. It receive success, if the message have additional data it save them if not it do not save them

With an extension, before start mining you need to negotiate with the pool the extension.