Announce chain-id inside ENR (and merge with pv key)
#408
Conversation
There was a problem hiding this comment.
Note that the Portal subnetworks do not really need this ENR field to find nodes.
At least not when they start from Portal bootstrap nodes. Further node discovery happens on the layer of the Portal subnetwork.
So this would give just some added resilience in case the node starts from 0 know Portal nodes and only general discv5 nodes (there are other ways for providing this resilience, such as storing previous nodes).
You could argue that it would also block a node from giving (maliciously or due to bug) nodes from another network, which it would. But a node would anyhow still have to verify the node (by pinging it) and cannot rely on only the ENR field.
And these invalid node responses would thus also just stop after 1 hop.
So all in all, I am not sure of how useful this will exactly be and whether it is worth a MUST and combining it with the already existing pv field. I will give it some more thought.
| - the network chain id that it operates on | ||
|
|
||
| ``` | ||
| enr['p'] = rlp([pv_min, pv_max, chain_id]) |
There was a problem hiding this comment.
I don't think we should keep using RLP for new definitions like this.
How would they stop after 1 hop / 1 ping? What if I'm doing a The scenario above can happen even without "bug" or "maliciously" intent. For example, user is running portal node on one network, but decides to switch to another network. For whatever reason (e.g. misconfiguration), user decides to use the same private key for generating NodeId. They will still receive messages from nodes on the old network, and create the problem above. |
A node should only "temporarily" add an unverified node/ENR to its routing table. Until this node is verified (ping/pong request/response or other type of req/reponse) it should not further forward this node/ENR to other nodes in FindContent or FindNodes requests. This is what I mean with that it would stop after 1 hop. Individual nodes can basically (by bug or by malicious intent) respond with invalid ENRs, but not the whole network would be propagating these.
It cannot happen in a "normal" scenario. What you describe would not occur on the Portal subnetwork level as the Portal nework requests will never arrive on the different subnetwork because of the different protocol ID. Of course it would still occur on discv5 level routing table, but that is not an issue, that is fine. Discv5 is base discovery layer for all type of networks. |
|
I misunderstood the full intent of this PR (thanks @morph-dev for clarifying). I did not realize that the idea was to use the same Protocol ID for different chains (chain ids or fork ids) and use the ENR field to separate between chain networks. My previous comments still apply but only when different Protocol ID is applied both for different subnetwork (history network) and different chain, as it was the idea in the past. With Protocol ID for portal subnetwork (in the new model only 1 will exist) and ENR field for the specific chain similar separation can be achieved, but it is slightly less separated at the Protocol level. I think the benefits are:
Down side:
Note that the devp2p/eth protocol also has |
| enr['p'] = rlp([pv_min, pv_max, chain_id]) | ||
| ``` | ||
|
|
||
| We use `rlp` encoded list as it allows us to add more items at the end in the future upgrades. The implementation should take that into consideration. |
There was a problem hiding this comment.
I understand how this might look appealing and how this is (not yet) possible with any of the SSZ serializations.
However, I think the cleaner solution here is just to have separate fields for these. I know that this suggestion is probably made to save some space in the ENR, but EL and especially CL clients already have more fields.
So we could just have the additional field which holds the forkid or so, similar like an EL has eth field.
When we eventually get fully integrated EL + Portal (using same base level discv5), it could also be just the same field that is used?
There was a problem hiding this comment.
I don't have a strong preference on this.
I think @lightclient and @fjl have preference for the current spec, but I don't know they feel about splitting it.
There was a problem hiding this comment.
I think the single ENR field p is fine for now. If we need to change this in the future we can absolutely revisit this.
I think this is a huge benefit going forward as we want to support various testnets / devnets / L2 networks (and maybe even L2 testnets).
I don't think this is an issue. I'm going to assume that client can only do network change between restarts. This implies that all existing sessions would become invalid and ENR sequence would increase. |
|
I'm going to merge this. If there is a need, we can always revisit and update. |
3 participants
The current approach of specifying the chain doesn't scale well (imagine supporting all testnets, devnets, layer-2 networks, etc).
Better approach is to specify supported chain-id as part of the ENR.
We already have
pventry in the ENR to indicate "Supported Portal wire-protocol versions". It seems like the waste to have multiple ENR entries that are Portal specific. This pr proposes following:pbut I'm open for better suggestions (preferably, not longer that 3 characters)pventry (no longer needed)rlplistConsidering that we are effectively in process of rebooting the networks, there is no need to go to transition from version 1 to 2. It's probably the easiest for clients to just support version 2+ by default.