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

feat(enr): added support for relay shards field #1676

Merged
merged 1 commit into from
Apr 17, 2023
Merged

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Apr 17, 2023

This PR introduces the Static Sharding ENR extension.

  • Split the waku_enr module into three submodules: capabilities, multiaddr, and sharding.
  • Added the rs and rsv extension implementation.
  • Added some tests covering the sharding extension.

@LNSD LNSD self-assigned this Apr 17, 2023
@LNSD LNSD force-pushed the feat-enr-relay-sharding branch from b683fe6 to 58ecf8e Compare April 17, 2023 08:41
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

left some comments.

@kaiserd Remember we talked last week about sharding. Do we consider the spec ready and moving to implementation?

I raised my concerns here and here but as a tldr:

  • I think waku should handle discovery only on waku shards.
  • I dont think we need this hierarchy of cluster + index. just shard index.
  • I think we should cover way less shards. quality and not quantity.
  • I dont think we need static sharding, just automatic. I wouldn't justify the need of static wharding with the MVP.
  • imho static sharding would be waku's dystopia. companies pushing their users to shards they control with no guarantees on decentralization, privacy, etc. all shards should be equal = managed by the protocol = auto sharding.
  • if an app wants static shards, fine, fork the network (like bsc, gnosis, ethclassic or even eth networks). wouldn't be difficult with a network-id in the ENR (which acts as kind of the shard-cluster but more generic)

These discussions, belong to here, so lets reply in there.
vacp2p/research#174

But raising it again here since we started with the implementation.

waku/v2/protocol/waku_enr/sharding.nim Outdated Show resolved Hide resolved

let shards = RelayShards.init(shardCluster, shardIndices)

## When
Copy link
Contributor

Choose a reason for hiding this comment

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

can we perhaps merge the following into just.

var builder = EnrBuilder.init(enrPrivKey, seqNum = enrSeqNum)
builder.withWakuRelaySharding(shardCluster, shardIndices)

so we avoid having to init RelayShards first? and then passing it?

perhaps a nitpick but withSharding instead withWakuRelaySharding?
so .withSharding().withCapabilities().withMultiAddress() 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.

perhaps a nitpick but withSharding instead withWakuRelaySharding?
so .withSharding().withCapabilities().withMultiAddress() etc.

If you add this chained builder pattern, you lose the possibility of one of those methods to return a Result, which is very convenient.

Copy link
Contributor Author

@LNSD LNSD Apr 17, 2023

Choose a reason for hiding this comment

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

so we avoid having to init RelayShards first? and then passing it?

I added the needed methods for the static sharding discv5 implementation. I am reluctant to add more convenience methods because they "might be handy".

We can always extend the implementation once this part is used.

@kaiserd
Copy link
Contributor

kaiserd commented Apr 17, 2023

@alrevuelta

Do we consider the spec ready and moving to implementation?

Given the tight MVP time-line: yes. @jm-clius wdyt?
As you suggested, I will post here: vacp2p/research#174

@LNSD LNSD force-pushed the feat-enr-relay-sharding branch from 58ecf8e to 3b0359b Compare April 17, 2023 10:41
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

The PR looks great!
However, I think we could have some more details in error handling that maybe worth adding.

waku/v2/protocol/waku_enr/sharding.nim Outdated Show resolved Hide resolved
waku/v2/protocol/waku_enr/capabilities.nim Show resolved Hide resolved

let indexList = fromIndicesList(field.get())
if indexList.isErr():
return none(RelayShards)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case we are losing the possible error given by fromIndicesList.
I'd try to chain the possible errors back to the caller as much as possible by returning a Result[Option[..], string]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some cases, for convenience reasons, we don't want to bubble up the error and the probability of this happening (in normal operation conditions), I added a debug log so we can identify when this is occurring.

waku/v2/protocol/waku_enr/sharding.nim Outdated Show resolved Hide resolved
@LNSD LNSD force-pushed the feat-enr-relay-sharding branch from 3b0359b to 9293f7a Compare April 17, 2023 12:42
@jm-clius
Copy link
Contributor

Do we consider the spec ready and moving to implementation?
Given the tight MVP time-line: yes. @jm-clius wdyt?

@alrevuelta @kaiserd I think so too, given that this was also discussed in some detail with Status and addresses a large number of concerns (granted we're still waiting for more detailed feedback from them on the RFC). Will provide more comments on the related issue to keep this PR cleaner.

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.

LGTM! Thanks for making progress on this.

@LNSD LNSD merged commit 9616253 into master Apr 17, 2023
@LNSD LNSD deleted the feat-enr-relay-sharding branch April 17, 2023 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants