Define ping/pong payload extensions #348
Conversation
|
Considering the goal of "prevent future protocol breaking changes, more flexible, easier less risk upgrades regarding", imagine the scenario where a ping receive a custom payload with Also, for privacy reasons, some client may decide to do not reveal its implementation/version/system, thys payload may be optional, or possibly a free string with defaut value |
I don't think the PR was read because some of the questions you asked were answered. Second EL clients have a similar type of message no? |
Sorry, it is in ping-payload-extensions/ping-custom-payload-extensions.md, yes, it is answered. |
Yes, they have node identifier (name+userIdentity+version+os+arch+compiler), and at least Geth do not allow to replace this identifier using flags, only This info is available through geth RPC, just could not find wich message in devP2P protocol return this info, anyways it is public. |
KolbyML
force-pushed
the
ping-pong-extensions
branch
3 times, most recently
from
7cda182
to
a9e920d
Compare
I think you can request it over devp2p or maybe discv4 I am exactly sure which one it is, but that is how https://ethernodes.org/ this website works to my knowledge. |
KolbyML
force-pushed
the
ping-pong-extensions
branch
from
a9e920d
to
a603389
Compare
| ## Custom Payload Extensions Format | ||
|
|
||
| - **type**: what payload type is it | ||
| - **verison**: what version of the type it is |
There was a problem hiding this comment.
| - **verison**: what version of the type it is | |
| - **version**: what version of the type it is |
| CustomPayloadExtensionsFormat = Container( | ||
| type: Bytes4, | ||
| version: Bytes4, | ||
| payload: Container(inner payload is defined by type and version) | ||
| ) |
There was a problem hiding this comment.
The way this currently is defined, it means that one has to write some custom code to only decode the encoded type and version, at which point it can then know how to decode the other Container.
Unless you meant to have the payload to be a ByteList which then could be decoded in a second step based on the type/version information after decoding the CustomPayloadExtensionsFormat Container. That would be a possible solution for this.
Or as Ping.payload is already a ByteList, another way might be to just prefix it with those bytes that indicated the type & version. Either way would work.
| type: [Type Number], | ||
| version: [Type Version], |
There was a problem hiding this comment.
These feel potentially redundant. Seems like we could simply have a type and if we end up needing to upgrade one of these types, we can use a new type byte.
| type: [Type Number], | ||
| version: [Type Version], | ||
| payload: Container([Payload Description]) |
There was a problem hiding this comment.
Ok, so using Union is a potentially loaded topic but... It seems like we could do this whole thing in SSZ cleanly with a Union.
BasicRadiusPayload = Container(data_radius: U256)
HistoryRadiusPayload = Container(data_radius: U256, ephemeral_header_count=U16)
ClientVersion = Container(version_string: ByteList[max_length=XXX])
StateNetworkDataPayload = Union[BasicRadiusPayload, ClientVersion]
HistoryNetworkCustomDataPayload = Union[HistoryRadiusPayload, Clientversion]What am I missing?
There was a problem hiding this comment.
I am assuming this would remove the type field I added?
I am against unions because
- they only support 127 selectors (if we have to upgrade stuff that might go quick).
- I don't think it is easy to deprecate old Payloads, as won't they then be enshrine in the format?
- Most teams are against unions, and most Portal clients don't handle Unions properly they manage them out of the library I think
I don't want to need to make an awkward upgrade if we run out of selectors. Also using a union would make it so parties couldn't make custom extensions which I think it important.
There was a problem hiding this comment.
I think the only one of those reasons that I'm compelled by is that unions are akward to implement any kind of rolling upgrade with. Given that, I think I'd advocate for a single type field that is something like u16 and a ByteList payload that is the encoded payload.
There was a problem hiding this comment.
Said explicitely, it would be
Container(type: u16, payload: ByteList[max_length=XXXX])where clients implement application specific logic for decoding the payload value into some number of known SSZ types that are supported in that network.
There was a problem hiding this comment.
Yes, this is also what I mentioned as 1 of the two options here: #348 (comment)
And I agree that type + version is quite overkill, especially with the amount of bytes reserved for each.
There was a problem hiding this comment.
This sounds like the play and is aligned with what Kim said. I think a u32 would still be very nice. I might be being a little hyperbolic, but I wouldn't want to deal with the possibility of running out of types. So it will probably be fine with a u16, but if we did run out that would be a dreadful experience to deal with. But it is probably fine
There was a problem hiding this comment.
For errors I think a u16 is more then enough to be clear*
| CustomPayloadExtensionsFormat = Container( | ||
| type: 4_294_967_295, | ||
| version: 1, | ||
| payload: Container(error_code: Bytes4, message: ByteList[MAX_ERROR_BYTE_LENGTH]) |
There was a problem hiding this comment.
| payload: Container(error_code: Bytes4, message: ByteList[MAX_ERROR_BYTE_LENGTH]) | |
| payload: Container(error_code: uint16, message: ByteList[MAX_ERROR_BYTE_LENGTH]) |
There was a problem hiding this comment.
Should have given some context here. I would think u16 is enough.
- Identical payload definitions can be re-used across multiple networks.
- We are in a byte-constrained packet so I'm inclined to be a little stingy with our bytes.
- It's ok if completely disparate use cases end up colliding on the same
typenumbers since the data won't be mixed across network boundaries, which suggest that 65k is a plenty big-enough namespace for us.
| - 0: Extension not supported | ||
| - 1: Requested data not found | ||
| - 2: System error |
There was a problem hiding this comment.
If we're going to provide suggested error codes, I would suggest we expand this a bit and include clear language around how we suggest they be used that includes examples.
|
@kdeme @pipermerriam I implemented the suggestions. I'm ready for another look |
|
I might have missed it, but how is upgrade supposed to work? Let's say we want to introduce new
Am I missing something? Maybe this should be documented as well? |
Yes, I have the same remark as @morph-dev basically. There is currently nothing defined of how nodes would share their "capabilities" (= what they support), thus nodes cannot negotiate over which actual types/versions to use, and it would work rather on a trial&error basis, which is far from ideal. And with the current 1,2,3,4 steps of above, I do not see much difference compared to not having this type/version system and just deciding on a specific moment in time where the payload changes. Because here you still need to do same currently (step 3). |
|
Ok I added
A Portal implementations can add support for or use non- So before this I think there is value in this extension system as we can use it instead of extending or changing the |
KolbyML
force-pushed
the
ping-pong-extensions
branch
from
c0a8987
to
3a68292
Compare
| | 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 types | Yes | | ||
| | [1](extensions/type-1.md) | Basic Radius Payload | State, Beacon, Canonical Transaction Index, Transaction Gossip | Provides the nodes Radius | Yes | |
There was a problem hiding this comment.
| | [1](extensions/type-1.md) | Basic Radius Payload | State, Beacon, Canonical Transaction Index, Transaction Gossip | Provides the nodes Radius | Yes | | |
| | [1](extensions/type-1.md) | Basic Radius Payload | State, Beacon | Provides the nodes Radius | Yes | |
Since these other networks don't exist yet, I don't think we should reference them.
| ## The problem | ||
|
|
||
| Over time we will need to change what is sent over ping/pong `custom_payload`'s to adjust for what is needed in the future. | ||
| Currently the only way to update a `custom_payload` is through breaking changes which break protocol compatible between Portal implementations. | ||
| As we get users, deploying breaking changes to mainnet will no longer be an option as we are aiming for a 100% uptime. | ||
|
|
||
| ## The Solution | ||
|
|
||
| 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) |
There was a problem hiding this comment.
I'm not sure that the "problem/solution" belongs in the spec. Once this is merged, the "problem" part is no longer applicable and I think feels awkward.
I would remove the "problem" part and change "the solution" part to be "motivation" and include the parts from the problem statement about needing a way to communicate meta data without modifying the base protocol.
|
|
||
| ## Custom Payload Extensions Format | ||
|
|
||
| - **type**: what payload type is it |
There was a problem hiding this comment.
| - **type**: what payload type is it | |
| - **type**: numeric identifier which tells clients how the `payload` field should be decoded. |
| ## Custom Payload Extensions Format | ||
|
|
||
| - **type**: what payload type is it | ||
| - **payload**: a ssz ByteList of max length 1100 which contents are specified the type field |
There was a problem hiding this comment.
| - **payload**: a ssz ByteList of max length 1100 which contents are specified the type field | |
| - **payload**: the SSZ encoded extension payload |
| ## 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 mirrored formats, but there is room for a Ping `custom_payload` to specify details about what it wants to request, then pong handles it. |
There was a problem hiding this comment.
| Currently type 1,2,3 are mirrored formats, but there is room for a Ping `custom_payload` to specify details about what it wants to request, then pong handles it. | |
| > 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. |
| - The extension isn't a required extension for specified Portal Network. | ||
|
|
||
| #### 1: Requested data not found | ||
| This error code is for if an extension is asking for something and it doesn't exist. |
There was a problem hiding this comment.
| This error code is for if an extension is asking for something and it doesn't exist. | |
| This error code should be used when a client is unable to provide the necessary data for the response. |
|
Looks good from my point of view. |
|
I'm fine doing my cleanup post merge. It's all tweaks to the language and it'll be easier as a pull request than as PR feedback. |
This is a WIP and I am looking to get early feedback
I propose ping/pong
custom_payloadextensions. A typed versioned format with the goal of making Portal Ping/Pong a flexible extension and preventing protocol changes from breaking other clients when updated versions are deployed. This will also give us to time rollout updated ping/pong endpoints before deprecating older versions.