feat_: shards.staging fleet #5160
Conversation
richard-ramos
requested review from
cammellos,
jakubgs,
jrainville,
chaitanyaprem,
igor-sirotin and
Ivansete-status
Jenkins BuildsClick to see older builds (4)
|
| @@ -0,0 +1,28 @@ | |||
| { | |||
| "Rendezvous": false, | |||
| "NoDiscovery": true, | |||
There was a problem hiding this comment.
Does this mean, not to use this fleet for discovery? Because i already see my desktop app is connected to some of the nodes in shards.staging fleet.
There was a problem hiding this comment.
These two items are for WakuV1, rendezvous + discovery for V2 are setup in WakuV2Config attribute
| @@ -231,5 +231,41 @@ func DefaultMailservers() []Mailserver { | |||
| Fleet: params.FleetShardsTest, | |||
| Version: 2, | |||
| }, | |||
| Mailserver{ | |||
There was a problem hiding this comment.
Why do we need to configure all mail servers here?
Shouldn't they be discovered by discv5 anyways?
There was a problem hiding this comment.
While I agree that hardcoding the storenodes here is bad, currently for Status we use an specific set of trusted storenodes that have high availability and store at least 30d of data. Using discv5 would mean that it is possible to discover store nodes that do not have these guarantees. And it's also problematic because the way status works right now, it will not request history for a period of time that it has already requested the history before, so if we choose a random storenode and we request history for some time interval, and that storenode has no data for that interval, then, the client will no longer retrieve history for that period of time.
I wonder if storev3 sync solves this?
| params.FleetStatusTest, | ||
| params.FleetStatusProd, | ||
| params.FleetShardsTest, | ||
| params.FleetShardsStaging, |
There was a problem hiding this comment.
Maybe we should remove all old fleets that are no longer part of static sharding?
There was a problem hiding this comment.
Not sure, looking at fleets.status.im, those fleets are still active.
I think if we remove these fleets from this part of code, we'd likely need to make sure those fleets are gone too, otherwise a deployment of status-go to those fleets will likely fail.
cc: @jakubgs to confirm if this is correct
There was a problem hiding this comment.
The only fleets that should be removed are wakuv2.* fleets.
There was a problem hiding this comment.
Since the desktop and mobile apps are using only clusterID 16, I was wondering if there is a need for all the other Waku fleets that no longer use clusterID. Maybe better to check with Franck/Hanno though.
| "Enabled": true, | ||
| "Fleet": "shards.staging", | ||
| "BootNodes": [], | ||
| "TrustedMailServers": [], |
There was a problem hiding this comment.
Maybe add priority list of mailservers here rather than in code?
I think this file existed before we even had DNS Discovery, and updating the code to remove the hardcode has been kicked down the road for a while... I'll open an issue. However something interesting to take into account is that using only dns discovery is not a good solution and we'd likely need to have the list of storenodes somewhere, either by using multiaddresses with IP Addresses instead of domain names in the node config, or reliying in discv5, and compare the peerID of the discovered nodes with a list of trusted nodes to make sure we use specific store nodes... It has happened before that core contributors have had problems with their DNS for some reason, and then the core contributor couldn't retrieve the bootnodes and ended up with 0 peers. If the same situation happened with the storenodes dns discovery url, it would mean that no history nodes would be available for the user |
|
Fair enough. |
richard-ramos
force-pushed
the
staging-fleet
branch
from
3eb93eb
to
6b45d63
Compare
|
Issue for removing the hardcoded storenodes can be found here: #5171 |
Adds the staging fleet to the list of available fleets in status-go