fix: introduce new field for shards in metadata protocol

[richard-ramos]

Description

This is the solution #3 described in #2509

Add a new field for the shards and deprecate field 2, and for at least 2 releases support both fields i.e.:

message WakuMetadataRequest {
  optional uint32 cluster_id = 1;
  repeated uint32 shards = 2  [deprecated = true];
  repeated uint32 shards = 3;
}

We would use getPackedRepeatedField on field 3, and if it's not populated, use getPackedRepeatedField on field2, and if it's empty, use getRepeatedField. This would solve the problem of nwaku not understanding go-waku's packed field.

For writing, we'd still use write3 for field 2 and writePacked for field3.

Currently go-waku does not care about the format of the field for reading data since it seems to try decoding both packed and unpacked data. go-waku should also implement the same solution, but for the time being this would allow users of different nwaku and go-waku versions to connect with each other and not introduce breaking changes

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2511

Built from b3a2e29

[alrevuelta]

Is there a way to fix this on go-waku side? If go-waku is going to be deprecated I would prefer to add "this burden" on that side rather than in nwaku.

[richard-ramos]

@alrevuelta that would be the suggestion # 1:

In go-waku use the attribute [packed=false] so it behaves like nwaku

The resulting protobuffer would look like this, and would need to be also updated in both the RFC and in waku-proto repo as its current definition does not match the behavior nwaku has.

message WakuMetadataRequest {
  optional uint32 cluster_id = 1;
  repeated uint32 shards = 2 [packed=false];
}

Doing the change in go-waku is easy, but has a drawback: It would require coordination so when infra deploys a release with version >= 0.25.0, all contributors also update their desktop version to something that uses this protobuffer. At the end of the day, it would depend on how status would feel about introducing this breaking change.

[SionoiS]

Thanks!

[Ivansete-status]

LGTM! Thanks!

I wonder if we could add a new field, version, that will help to properly switch logic according to the version received/sent. That could help to better separate the per-version-logic.

Also, having a separate logic per each version will help to prune the code in the future when no longer supporting a particular version.

[arnetheduck]

I wonder if we could add a new field, version, that will help to properly switch logic according to the version received/sent. That could help to better separate the per-version-logic.

In general, when working with protobuf, it's good to assume a similar mindset as with working with JSON or any other unordered key-value mechanism: you should expect "unknown" fields in the stream and do as much as you can with the fields you do understand, degrading gracefully if there's something critical missing (for example because "future" versions stopped sending it) - it is "normal" to add and remove fields in protobuf and the application should generally degrade gracefully - a version field tends to complicate this "natural" be-lenient-in-what-you-accept approach (at that point, you might as well send a separate message entirely, on a new endpoint). In particular, the problem is that if you receive a version you don't recognise, you can do nothing with this information - generally, the outcome is better if instead you try to decode the fields you know in this situation.

Future versions can then exploit the graceful-degradation logic to create "overlapping-versions" support like in this PR. It's entirely fine and "natural" to add fields like this as the product evolves - one potential improvement is indeed to document the versions support in the protobuf and describe how that version handles the field, ie 0.24 will decode this field first as packed then as non-packed.

[jm-clius]

Thanks!