Improve phrasing of digest_function added in #311

[EdSchouten]

• Unlike the other messages where a digest_function field was added, ExecuteActionMetadata is returned by the server to the client. This means that the words "client" and "server" need to be swapped around in some but not all places.

• To ensure backward compatibility, we permit digest functions that were defined at the time this field was added to remain unset. When the digest_function fields were added to the other messages, the last one to be added was MURMUR3. In the meantime we've added BLAKE3 and SHA256TREE, so for this specific field we must list those as well.

[EdSchouten]

@werkt @bergsieker I spotted a couple of small mistakes in the way the new digest_function was documented. PTAL!

[werkt]

The reason to list omittable digest functions is not just legacy - sha256 cannot be distinguished from blake3 by digest length, so you can't omit both.

[EdSchouten]

That also applies to existing cases, as MURMUR3 cannot be distinguished from MD5, due to both of them being 128 bits in size. We already discussed this issue during one of the monthly meetings when we originally added digest_function, and the outcome was that we preferred to remain backward compatible, even if it meant that ambiguity remained if a sender doesn't set this field. The way this field is documented right now, we are breaking compatibility.

[werkt]

If MURMUR3 is also 128 bits, then yes, it should be removed from the omittable list as well.

[EdSchouten]

No, it should not, because that would completely break backward incompatibility between existing clients and servers.

There are already servers out there that support BLAKE3. And those can already return ExecuteActionMetadata messages that have action_digest.hash set to a BLAKE3 hash, while having the newly added digest_function field left unset. Are those now suddenly in violation of the protocol? Should a client that fully complies to the latest protocol simply fail to parse these messages and terminate the build? And should the client also fail if the server supports nothing else than BLAKE3?

The ambiguity is only an issue if the server supports MURMUR3 AND MD5, or SHA256 AND BLAKE3. But it's completely well defined to use the protocol in setups that only support one of each. Even if ExecuteActionMetadata doesn't have digest_function set.

[werkt]

Not having written the original copy, I can't say for sure what the intent was. Based on the specification it seems to be a tenured, bit-count matching exclusion of later-numbered additions to DigestFunction.Value - newer sequence numbers without matching bit-counts for lower numbers MUST be omitted, but that is certainly not a formula expressed in docs or anything else I've seen.

Of note, the language actually doesn't require those things NOT in the omitted list to NOT be omitted (i.e. specified). It is within spec (as much as our comments are spec) for someone to omit DigestFunction.Value.BLAKE3 from a message with a relevant blake3 hash in it.

[EdSchouten]

Of note, the language actually doesn't require those things NOT in the omitted list to NOT be omitted (i.e. specified). It is within spec (as much as our comments are spec) for someone to omit DigestFunction.Value.BLAKE3 from a message with a relevant blake3 hash in it.

That is just being overly pedantic. The way the sentence is structured, the first "If" is obviously supposed to be interpreted as "if and only if". Otherwise the sentence would be a tautology, allowing it to be reduced to "This sentence is true.".

There are many fields in the protocol for which we don't explicitly document that they are required. I think we can all agree that calling Execute without an action_digest is a pilot error, yet we don't document it as being mandatory.

[EdSchouten]

Thanks for the review, @mostynb.