Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

new RFC: 57/STATUS-Simple-Scaling #580

Merged
merged 9 commits into from
Mar 17, 2023

Conversation

kaiserd
Copy link
Contributor

@kaiserd kaiserd commented Mar 10, 2023

This PR addresses vacp2p/research#160

> *Note*: This is not planned for the MVP.


# Light Clients
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if necessary, but we previously tried to move away from the binary sounding "light" vs "full" nodes in terminology (though people seem to fall back to these familiar terms). Reasoning then was that we really provide a spectrum of options for nodes, who can choose based on either resource restrictions or own motivations what level of service they want to provide/consume. I'm OK with using "light" here, as long as we don't set this in stone as a special type of client - i.e. nodes can choose to fully relay for one community, but only use the light protocols for another, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rename this section to light protocols, and adjust accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done: 1f552ad

Nodes can request history messages via the [13/WAKU2-STORE](/spec/13/).

The store service is not limited to a Status fleet.
Anybody can run a Waku Archive node in the Status shards.
Copy link
Contributor

Choose a reason for hiding this comment

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

Although we don't have proper capability discovery defined here yet - in other words, nodes would simply assume such Archive nodes "store everything".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a note :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done: a960388


# Infrastructure Shards

## Control Message Shards
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, general control messages will still be relayed by default in the main Community shard? This section just discusses "special" control message sharding, for large messages. For clarity it may be worth being explicit here (or earlier) about which traffic goes to the static community shard ("all" by default), and that this separate sharding is not a requirement for all control messages? E.g. renaming this section to Sharding for large Control Messages. Hoping this makes sense. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Wdyt about keeping the name control message shard,
but making explicit that control messages CAN be routed via control message shards, but don't have to be?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! In other words, the way I read this RFC is that it starts with general description of static shard allocation per community and moves to specific further shard separation techniques which may or may not be required based on community size/need.

Copy link
Contributor Author

@kaiserd kaiserd Mar 13, 2023

Choose a reason for hiding this comment

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

I added more context in dd23486
Wdyt? Imo, with this it should be obvious that other messages are still relayed as per normal operation.
I can rephrase if it is not clear yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! Looks good and is clear. Thanks.

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.

Looks good directionally!

@kaiserd kaiserd requested a review from rymnc March 13, 2023 14:01
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM as a raw rfc!

Comment on lines +103 to +131
```protobuf

syntax = "proto3";

message CommunityDescription {
// The Lamport timestamp of the message
uint64 clock = 1;
// A mapping of members in the community to their roles
map<string,CommunityMember> members = 2;
// The permissions of the Community
CommunityPermissions permissions = 3;
// The metadata of the Community
ChatIdentity identity = 5;
// A mapping of chats to their details
map<string,CommunityChat> chats = 6;
// A list of banned members
repeated string ban_list = 7;
// A mapping of categories to their details
map<string,CommunityCategory> categories = 8;
// The admin settings of the Community
CommunityAdminSettings admin_settings = 10;
// If the community is encrypted
bool encrypted = 13;
// The list of tags
repeated string tags = 14;
// index of the community's shard within the Status shard cluster
uint16 relay_shard_index = 15
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe show the diff here so it is clear that the relay_shard_index is being added?

Copy link
Contributor Author

@kaiserd kaiserd Mar 17, 2023

Choose a reason for hiding this comment

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

Agreed, the change should be explicit. Added a clarification in 00dcaab
wdyt?
(Imo, the protobuf should be copyable and not a diff.)

Comment on lines +103 to +131
```protobuf

syntax = "proto3";

message CommunityDescription {
// The Lamport timestamp of the message
uint64 clock = 1;
// A mapping of members in the community to their roles
map<string,CommunityMember> members = 2;
// The permissions of the Community
CommunityPermissions permissions = 3;
// The metadata of the Community
ChatIdentity identity = 5;
// A mapping of chats to their details
map<string,CommunityChat> chats = 6;
// A list of banned members
repeated string ban_list = 7;
// A mapping of categories to their details
map<string,CommunityCategory> categories = 8;
// The admin settings of the Community
CommunityAdminSettings admin_settings = 10;
// If the community is encrypted
bool encrypted = 13;
// The list of tags
repeated string tags = 14;
// index of the community's shard within the Status shard cluster
uint16 relay_shard_index = 15
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax = "proto3";

message CommunityDescription {
  // The Lamport timestamp of the message
  uint64 clock = 1;
  // A mapping of members in the community to their roles
  map<string,CommunityMember> members = 2;
  // The permissions of the Community
  CommunityPermissions permissions = 3;
  // The metadata of the Community
  ChatIdentity identity = 5;
  // A mapping of chats to their details
  map<string,CommunityChat> chats = 6;
  // A list of banned members
  repeated string ban_list = 7;
  // A mapping of categories to their details
  map<string,CommunityCategory> categories = 8;
  // The admin settings of the Community
  CommunityAdminSettings admin_settings = 10;
  // If the community is encrypted
  bool encrypted = 13;
  // The list of tags
  repeated string tags = 14;
  // index of the community's shard within the Status shard cluster
+  uint16 relay_shard_index = 15
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

Copy link
Contributor

Choose a reason for hiding this comment

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

You could add an empty line before line 129 to separate the new field from other fields.

@rymnc rymnc self-requested a review March 17, 2023 07:17
@kaiserd kaiserd merged commit a32e202 into master Mar 17, 2023
@kaiserd kaiserd deleted the add/rfc57-status-communities-simple-scaling branch March 17, 2023 07:49
Comment on lines +103 to +131
```protobuf

syntax = "proto3";

message CommunityDescription {
// The Lamport timestamp of the message
uint64 clock = 1;
// A mapping of members in the community to their roles
map<string,CommunityMember> members = 2;
// The permissions of the Community
CommunityPermissions permissions = 3;
// The metadata of the Community
ChatIdentity identity = 5;
// A mapping of chats to their details
map<string,CommunityChat> chats = 6;
// A list of banned members
repeated string ban_list = 7;
// A mapping of categories to their details
map<string,CommunityCategory> categories = 8;
// The admin settings of the Community
CommunityAdminSettings admin_settings = 10;
// If the community is encrypted
bool encrypted = 13;
// The list of tags
repeated string tags = 14;
// index of the community's shard within the Status shard cluster
uint16 relay_shard_index = 15
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add an empty line before line 129 to separate the new field from other fields.

Infrastructure nodes are especially important for providing connectivity in the roll-out phase.

Infrastructure nodes are not limited to Status fleets, or nodes run by community owners.
Anybody can run infrastructure nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the context of Status MVP, to ensure good UX, one need will need to run a node with postgres and some minimal requirements. Should we mention this in the RFC?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants