-
Notifications
You must be signed in to change notification settings - Fork 85
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
Define ping/pong payload extensions #348
base: master
Are you sure you want to change the base?
Changes from 9 commits
a603389
cedecda
3d76802
3a68292
56fa823
a0aa9ba
3ab5275
b01d69a
014dd45
c06e6ae
313c977
3801b7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,12 +56,7 @@ The canonical transaction index network supports the following protocol messages | |
|
||
#### `Ping.custom_data` & `Pong.custom_data` | ||
|
||
In the canonical transaction index network the `custom_payload` field of the `Ping` and `Pong` messages is the serialization of an SSZ Container specified as `custom_data`: | ||
|
||
``` | ||
custom_data = Container(data_radius: uint256) | ||
custom_payload = SSZ.serialize(custom_data) | ||
``` | ||
In the canonical transaction index network the `custom_payload` field of the `Ping` and `Pong` messages is the serialization of an SSZ Container specified as [Type 1 Basic Radius Payload](../ping-payload-extensions/extensions/type-1.md) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not link to type 0 and type 1? Both are ok to be used? Or just link to type 0 and mention that capabilities can be checked for further extensions. That text would be more future proof. |
||
|
||
|
||
### Routing Table | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,12 +61,8 @@ The history network supports the following protocol messages: | |
|
||
#### `Ping.custom_data` & `Pong.custom_data` | ||
|
||
In the history network the `custom_payload` field of the `Ping` and `Pong` messages is the serialization of an SSZ Container specified as `custom_data`: | ||
In the history network the `custom_payload` field of the `Ping` and `Pong` messages is the serialization of an SSZ Container specified as [Type 2 History Radius Payload](../ping-payload-extensions/extensions/type-2.md) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not link to type 0 and type 2? Both are ok to be used (if the other side supports it). Or just link to type 0 and mention that capabilities can be checked for further extensions. That text would be more future proof. |
||
|
||
```python | ||
custom_data = Container(data_radius: uint256) | ||
custom_payload = SSZ.serialize(custom_data) | ||
``` | ||
|
||
### Routing Table | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# This document lists defined extensions | ||
Some extensions are network specific some are generic | ||
|
||
This is a list and short description of all the extensions | ||
|
||
|
||
| Type number | Name | Supported sub-networks | Short Description | Is this call Required to Implement | | ||
|---|---|---|---|---| | ||
| [0](extensions/type-0.md) | Capabilities | All | Provides a list of enabled extensions | Yes | | ||
| [1](extensions/type-1.md) | Basic Radius Payload | State, Beacon | Provides the nodes Radius | Yes | | ||
| [2](extensions/type-2.md) | History Radius Payload | History | Provides the nodes radius and ephemeral header count | Yes | | ||
Comment on lines
+10
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But these would not be required to implement as they are not part of the standard extensions? |
||
| [3](extensions/type-3.md) | Client Info | All | It will return you something like `trin/0.1.1-2b00d730/linux-x86_64/rustc1.81.0` | Yes | |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# [Title] | ||
|
||
[Description] | ||
|
||
Ping payload | ||
```python | ||
|
||
[Payload] = SSZ.serialize(Container([Key Value Pairs])) | ||
|
||
|
||
[Container Name] = Container( | ||
type: [Type Number], | ||
payload: [Payload] | ||
) | ||
``` | ||
|
||
Pong payload | ||
```python | ||
|
||
[Payload] = SSZ.serialize(Container([Key Value Pairs])) | ||
|
||
[Container Name] = Container( | ||
type: [Type Number], | ||
payload: [Payload] | ||
) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Capabilities Payload | ||
|
||
Portal clients can only have max 500 extensions enabled per sub-network. | ||
|
||
This payload provides a list of u16's each u16 provide in the list corresponds to an enabled extension type. | ||
|
||
Ping and Pong Payload | ||
```python | ||
|
||
MAX_CAPABILITIES_LENGTH = 500 | ||
|
||
capabilities = SSZ.serialize(List[u16, MAX_CAPABILITIES_LENGTH]) | ||
|
||
BasicRadiusPayload = Container( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be named CapabilitiesPayload? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch |
||
type: 0, | ||
payload: capabilities | ||
) | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# Basic Radius Payload | ||
|
||
A basic Ping/Pong payload which only contains the nodes radius | ||
|
||
Ping and Pong Payload | ||
```python | ||
|
||
basic_radius = SSZ.serialize(Container(data_radius: U256)) | ||
|
||
BasicRadiusPayload = Container( | ||
type: 1, | ||
payload: basic_radius | ||
) | ||
``` | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,14 @@ | ||||||
# History Radius Payload | ||||||
|
||||||
A specialized radius payload for the history network which contains field for how many ephemeral headers the node holds. | ||||||
|
||||||
Ping and Pong Payload | ||||||
```python | ||||||
|
||||||
history_radius = SSZ.serialize(Container(data_radius: U256, ephemeral_header_count=U16)) | ||||||
|
||||||
HistoryRadiusPayload = Container( | ||||||
type: 1, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
payload: history_radius | ||||||
) | ||||||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# Get Client info | ||
|
||
This payload is only supposed to be usable from all Networks. This payload allows you to figure out peer's client info. Which will be useful for censuses. | ||
|
||
|
||
### Type Specifications | ||
|
||
Client info are ASCII hex encoded strings. | ||
|
||
Client info strings consist of 4 parts | ||
- client name (e.x. `trin`,`fluffy`) | ||
- client version + short commit (e.x. `0.1.1-2b00d730`) | ||
- operating system + cpu archtecture (e.x. `linux-x86_64`) | ||
- programming language + language version (e.x. `rustc1.81.0`) | ||
|
||
Example | ||
- String: `trin/0.1.1-2b00d730/linux-x86_64/rustc1.81.0` | ||
- Hex encoding: `0x7472696E2F302E312E312D32623030643733302F6C696E75782D7838365F36342F7275737463312E38312E30` | ||
|
||
|
||
Max length of a client info we should accept | ||
- MAX_CLIENT_INFO_BYTE_LENGTH = 200 | ||
|
||
|
||
Ping and Pong payload | ||
```python | ||
|
||
client_info = SSZ.serialize(Container(client_info: ByteList[MAX_CLIENT_INFO_BYTE_LENGTH])) | ||
|
||
ClientInfoPayload = Container( | ||
type: 3, | ||
payload: client_info | ||
) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
# Ping Custom Payload Extensions | ||
|
||
## Motivation | ||
|
||
Ping Payload Extensions. There is a minimal set of `standard extensions` which require a fork hard fork to change. This framework allows Portal clients to implement `non standard extensions` which don't require a hard fork to deploy to the network. A more flexible way to extend the Protocol without bloating the [Portal-Wire-Protocol](../portal-wire-protocol.md) | ||
|
||
# Type's | ||
|
||
There are 65536 unique type ids. | ||
|
||
Types 0-10_000 and 65436-65535 are reserved for for future upgrades. | ||
|
||
The rest are first come first serve, but they should still be defined in this repo to avoid overlaps. | ||
|
||
|
||
## Requirements | ||
|
||
All payloads used in the Ping `custom_payload` MUST follow the `Ping Custom Payload Extensions` format. | ||
|
||
## Custom Payload Extensions Format | ||
|
||
- **type**: numeric identifier which tells clients how the `payload` field should be decoded. | ||
- **payload**: the SSZ encoded extension payload | ||
|
||
|
||
```python | ||
CustomPayloadExtensionsFormat = Container( | ||
type: u16, | ||
payload: ByteList[max_length=1100] | ||
) | ||
``` | ||
|
||
## Ping vs Pong | ||
The relationship between Ping and Pong message will be determined by the requirements of the type. | ||
|
||
Currently type 1,2,3 are symmetrical, having the same payload for both request and response. This symmetry is not required. Extensions may define separate payloads for Ping and Pong within the same type. | ||
|
||
|
||
### Error responses | ||
If the ping receiver can't handle the ping for any reason the pong should return an error payload | ||
|
||
Pong payload | ||
```python | ||
|
||
# Max ASCII hex encoded strings length | ||
MAX_ERROR_BYTE_LENGTH = 300 | ||
|
||
error_payload = SSZ.serialize(Container(error_code: u16, message: ByteList[MAX_ERROR_BYTE_LENGTH])) | ||
|
||
ErrorPayload = Container( | ||
type: 65535, | ||
payload: error_payload | ||
) | ||
``` | ||
|
||
### Error Code's | ||
|
||
#### 0: Extension not supported | ||
This code should be returned if the extension isn't supported. This error should only be returned if | ||
- The extension isn't supported | ||
- The extension isn't a required extension for specified Portal Network. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not add type |
||
#### 1: Requested data not found | ||
This error code should be used when a client is unable to provide the necessary data for the response. | ||
|
||
#### 2: Failed to decode payload | ||
Wasn't able to decode the payload | ||
|
||
#### 3: System error | ||
A critical error happened and the ping can't be processed | ||
|
||
## Standard extensions | ||
|
||
Every Portal Network Client is required to support 3 extensions | ||
- [Type 0: Capabilities Payload](extensions/type-0.md): useful for finding what ping extensions a client supports | ||
- Sub-network standard extension. For history this is currently [Type 2: History Radius Payload](extensions/type-2.md). For State and Beacon the base extension is [Type 1: Basic Radius Payload](extensions/type-1.md). These are extensions what are crucial. This standard extensions are specified the respective sub-networks specification | ||
- Type 3: [Get Client info Payload](extensions/type-3.md) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest we remove this from the standard set. Clients should support it but I don't think it makes sense to make it be required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is fine to make it so a valid option is an empty string for privacy inclined users, but I don't think we should remove it from being a standard extension, as 99.9% of honest Portal nodes would support it.
If this isn't a standard extension it would require
If 99.9% of honest clients are going to support this call, I think it should be a standard extension, with an opt out to return an empty string by the 0.001% of users which have a privacy concern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I agree with this. Will set the default path towards clients supporting this which I think is positive for network monitoring/health. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it is fine to just be an empty string, then it is a bit silly to send this around as another message req/rep for those that want to keep that private. Perhaps just add it as a field in the capabilities extension? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I actually like this. Removes an additional message pair and makes it so that this data gets transmitted implicitly with the other message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will update the PR to make it so client info is a part of the capabilities extension, this will limit the max amount of ping extensions a client can have enabled at a time to 450 or 400 instead of 500 to reserve byte space for the client enough, but 400 extensions should be more then enough so it isn't a deal breaker at the end of the day. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I don't think anybody would want to be writing Portal implementations if they have to deal with 400 (or even 40 for that matter) ping extensions :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The whole point is they are optional, so Portal implementation that didn't want to implement something wouldn't have to. I don't know what will happen in the future or how extensions we would be adopted. So who knows how many would end up being used. I don't see an issue in having some head room |
||
|
||
## What is the [Type 0: Capabilities Payload](extensions/type-0.md) for | ||
It is for Portal implementations which want to see what extensions a peer supports. Not all extensions need to be implemented by all parties. So in order for a peer to find if an extension is implemented a [Type 0: Capabilities Payload](extensions/type-0.md) should be exchanged. | ||
|
||
Non-required extension offer a way for Portal implementations to offer extended functionality to its users without requiring every Portal implementing party to agree to a new feature. This allows for a diverse set of use cases to be fulfilled without requiring every implementer implement every extension, or requiring the need to bloat the minimal [Portal-Wire-Protocol](../portal-wire-protocol.md) with new `Message Types`. | ||
|
||
## Does implementing non-standard-extensions require a hardfork? | ||
No only changing a sub-networks standard extension requires a hard fork. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the use of the term "hard fork" here. It collides too much with the core ethereum protocol use and in my opinion makes it sound more severe than it actually is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A hard fork is just a rule change that occurs on all clients. In this case it would just be a timestamp which when reached all Portal clients would change X functionality for the base Portal Protocol. Hardforks will be required regardless I expect in the future, there will be networking changes we make to calls other then ping/pong. To ensure a smooth not breaking change hardforks, might be a good way to do this. Another way to do this could potentially be adding some kind of Portal Version message, where clients would always speak in the oldest version or something along those lines, but I feel that solution would be a lot more complex than hard forks. Ping/Pong is only a small part of the
to name a few If we want to use a different term then "hard fork", I am fine with that and I am open to suggestions. I struggle to think of a better understood word to describe what is happening though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my concern is not so much about semantics, but rather about the fact that you still require to have a client aligned release mechanism, see also comment at #348 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that using the term 'hard fork' doesn't sound right. Portal doesn't take part in consensus and doesn't really fork because it's not a blockchain in itself. Of course we can have breaking protocol changes that require all clients to be updated at the same time so perhaps a better term would be 'breaking protocol change' or 'non backwards compatible protocol upgrade'. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Portal isn't exactly a blockchain protocol, it's a p2p network that contains blockchain data which follows a blockchain protocol. The term fork comes from the fact that the block history forks. If bittorrent had a breaking change, it wouldn't be called a hard fork. Anyway, its just semantics I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think this is very fair and it is a symantic issue, ideally their is a single word to refer to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm 👎 on using the term hard fork for this. It name collides with the core protocol terminology and will be confusing. I recognize that the term may be correct in definition but the name collision is a deal breaker for me. I also will re-iterate that I don't think we should try to define how this process will work because I don't think we know and trying to define it corners us into future behavior that may not be necessary or optimal. We should leave this process undefined. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, I updated the PR to remove the term |
||
|
||
|
||
## How do sub-network standard extension's work | ||
sub-network standard extension's are fundamental extensions that are required for a Portal sub-network to function. They must be supported by the sub-networks implementations. | ||
|
||
### What is the process for changing a sub-network standard extension | ||
|
||
The process of changing a Portal sub-networks standard extension is through a hard fork set by a timestamp. When the timestamp is reached usage of the old standard extension is replaced by the new standard extension. Details on what standard extensions for what networks are being replace are specified in the fork's document. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me the usefulness of these ping/pong payload extensions is to avoid having to do something like this in the first place. If we don't avoid this hard-forking, then I don't really understand the point of adding this complexity. But imo it is possible to do it without hard-forking, as is also explained in the chapter below this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest that we remove this section and leave the process undefined. I think we are getting into the realm of over-specifying this piece of the protocol. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A client would never need to call The usefulness of ping extensions is not requiring all parties to agree to make a change to the protocol or add a feature. Of course supporting the base protocol is required i.e. Ping extensions don't change the base protocol much, new clients can just return empty for If a client wants a feature what is not popular or doesn't make sense for all clients to implement it they can, which I think is important long term. Especially as not only L1 will be using Portal. The base Portal P2P protocol should be simple, and Ping extensions allow us to keep it simple. Well still allowing for clients to experiment or add new features, which maybe one day become standard extensions if they become popular, but there is no requirement for that, which is one of the important parts of this proposal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My comment is not about the usefulness of the extensions. It is about this specific section which talks about the requirement of a hard fork. I think this should be designed in such way that a hard fork is not required at all, else I don't see the point of it, and with the capabilities extension this should be feasible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we assuming
or
? If 1. is true I think a forkless mechanism makes sense as the latancy cost of calling if 2. is true I think the
but if you were a I need to put more thought into this as these are only my additional thoughts. I didn't conceive 1. could be an option till just now so I still need to spend time thinking which assumption is actually true 1. or 2. A forkless method makes a lot of sense for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting to think about it from short vs long lived connections. However to avoid confusion I'd suggest to call it something else, because in Portal all connections are short lived. But from reading your comment I think you mean to differentiate between:
I think that the second group is rarely going to receive a ping from a "regular" node. When running a recursive find nodes or find content it would not require a ping. I can see crawlers and nodes that run bridges perhaps hitting those nodes also, as they want to see the full view. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess there is more thinking to be done, some of this we would only need to worry about when we propose the first breaking change so I don't think we need to pick a path immediately The 2 paths I see are
Both have trade offs, to ask bluntly which path do you prefer? |
||
### Can a node use the next standard extension for a network before the hardfork | ||
Yes there is no limitation for a Portal Client implementation using the next standard extension or a 3rd party extension before the hard fork has taken place. A requirement of this flexibility is that the new extension being used in-place of the standard extension must still fulfill the duties of the current standard extension. One downside of using new Standard Extension's before the fork is you won't know if a peer supported it yet unless you did either a | ||
- trial and error to see if the peer supports the call | ||
- Sent a [Type 0: Capabilities Payload](extensions/type-0.md) | ||
|
||
This can be useful for | ||
- accessing functionality before the hard fork has taken place in a none breaking way | ||
- Extending the protocol with implementation specific heuristics. | ||
|
||
It is only recommended to do this for peers in an implementations routing table as most connections are too short lived to make the addition call worth it. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,12 +56,7 @@ The execution state network supports the following protocol messages: | |
|
||
#### `Ping.custom_data` & `Pong.custom_data` | ||
|
||
In the execution state network the `custom_payload` field of the `Ping` and `Pong` messages is the serialization of an SSZ Container specified as `custom_data`: | ||
|
||
``` | ||
custom_data = Container(data_radius: uint256) | ||
custom_payload = SSZ.serialize(custom_data) | ||
``` | ||
In the execution state network the `custom_payload` field of the `Ping` and `Pong` messages is the serialization of an SSZ Container specified as [Type 1 Basic Radius Payload](../ping-payload-extensions/extensions/type-1.md) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not link to type 0 and type 1? Both are ok to be used? Or just link to type 0 and mention that capabilities can be checked for further extensions. That text would be more future proof. |
||
|
||
#### POKE Mechanism | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,19 +26,7 @@ The network uses the PING/PONG/FINDNODES/FOUNDNODES/OFFER/ACCEPT messages from t | |
|
||
The network uses the standard XOR distance function. | ||
|
||
### PING payload | ||
|
||
``` | ||
Ping.custom_payload := ssz_serialize(custom_data) | ||
custom_data := Container(transaction_radius: uint256) | ||
``` | ||
|
||
### PONG payload | ||
|
||
``` | ||
Pong.custom_payload := ssz_serialize(custom_data) | ||
custom_data := Container(transaction_radius: uint256) | ||
``` | ||
In the execution state network the `custom_payload` field of the `Ping` and `Pong` messages is the serialization of an SSZ Container specified as [Type 1 Basic Radius Payload](../ping-payload-extensions/extensions/type-1.md) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not link to type 0 and type 1? Both are ok to be used? Or just link to type 0 and mention that capabilities can be checked for further extensions. That text would be more future proof. |
||
|
||
## Content Keys | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not link to type 0 and type 1? Both are ok to be used? Or just link to type 0 and mention that capabilities can be checked for further extensions. That text would be more future proof.