Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: repeated fields are packed in proto3 #2509

Closed
wants to merge 1 commit into from

Conversation

richard-ramos
Copy link
Member

@richard-ramos richard-ramos commented Mar 5, 2024

Description

As described here vacp2p/nim-libp2p#1035 proto3 repeated fields are packed by default, so with this PR we use writePacked and getPackedRepeatedField instead of write3 and getRepeatedField.
However, this would mean that once a release is done, nodes using the previous version would not be compatible with the newer release, which can be problematic.

@jm-clius, @alrevuelta, what would be the recommended approach here?

Some possibilities that come to mind are the following:

  1. In go-waku use the attribute [packed=false] so it behaves like nwaku, and then just close this PR. This would be the 'easy' solution, and would require some 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:
message WakuMetadataRequest {
  optional uint32 cluster_id = 1;
  repeated uint32 shards = 2 [packed=false];
}

  1. Merge this PR in nwaku and then, on new release, all users need to upgrade their nwaku version or they'll be disconnected.

Neither the first nor the second option seem ideal because the impact they have and also because they modify an existing field in a protobuffer, and this seems to not be the recommended approach?


  1. Add a new field for the shards and deprecate this field, 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;
}

Then in nwaku code we'd 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. I think this is the cleanest option

Copy link

github-actions bot commented Mar 5, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2509

Built from efe0d57

@chair28980 chair28980 added the status-waku-integ All issues relating to the Status Waku integration. label Mar 5, 2024
@Ivansete-status
Copy link
Collaborator

Thanks for this @richard-ramos ! I like the option 3. a lot :D

@richard-ramos
Copy link
Member Author

richard-ramos commented Mar 6, 2024

@Ivansete-status @jm-clius @alrevuelta
I created this PR: #2511 with the proposed solution # 3.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this @richard-ramos! Not approving, as indeed I think option 3 is the best way forward. It's sad to need the workaround, because this is in effect simply a bug in nwaku not correctly implementing the wire specification. However, with so many people currently experimenting with the Waku Network, I think it's best to maintain backwards compatibility as long as we remember to remove the deprecated field after two releases.

@richard-ramos
Copy link
Member Author

Closing as #2511 sounds like the best approach to solve this issue.

@jm-clius jm-clius mentioned this pull request Mar 26, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status-waku-integ All issues relating to the Status Waku integration.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants