paritytech / json-rpc-interface-spec

29 stars 3 forks source link

Remove `chainHead_unstable_genesisHash` #61

Closed tomaka closed 1 year ago

tomaka commented 1 year ago

Since the objective is to stabilize the chainHead namespace in the nearby future, this pull request proposes to remove the chainHead_unstable_genesisHash JSON-RPC function out of caution.

The reason is that a client that simply follows the head of the chain might not necessarily be aware of what the genesis hash is. See https://github.com/polkadot-fellows/RFCs/pull/8 for an example. The genesis hash can be found in the chain at System::BlockHash[0], but since this is a runtime-specific thing it should be implemented by the JSON-RPC client on top of chainHead_storage, rather than by the server.

Note that archive_genesisHash and chainSpec_genesisHash still exist, as a node that implements archive is clearly supposed to know what the genesis hash is, and that a node that implements chainSpec can read the genesis hash from its chain spec.

josepot commented 1 year ago

Probably a silly question, but what is the point of having both archive_genesisHash and chainSpec_genesisHash? wouldn't it make sense to remove archive_genesisHash as well? Or this is perhaps because archive nodes are not meant to implement the chainSpec API?

tomaka commented 1 year ago

The point is that the JSON-RPC client doesn't know which API is available. Nodes might have archive but not chainSpec, or vice versa, or neither, or both.

tomaka commented 1 year ago

More generally, if we would stabilise the individual calls that we were happy about on a case by case basis, then the path to complete stabilisation may be quicker, and it would allow us to "experiment" with unstable methods that we weren't sure about in the meantime (or add unstable methods later etc that may or may not be stabilised).

Well, the design is that the JSON-RPC client calls rpc_methods, and if it finds any function that starts with chainSpec_v1, it can assume that all the chainSpec_v1 functions are supported.

This avoids the client having to check whether every single function it needs are supported, which is both tedious and error prone. Instead, a client developer just needs to keep track of which namespaces it needs.

Therefore if we stabilize a function later, we would also bump everything to v2.

jsdw commented 1 year ago

Ok, thanks for the clarification, that makes sense! So if we want to add new methods, they can take this sort of path I guess?:

tomaka commented 1 year ago

Ok, thanks for the clarification, that makes sense! So if we want to add new methods, they can take this sort of path I guess?:

* `chainHead_v1` methods are stable

* We add a new `chainHead_unstable` method or methods that we think might be useful.

* At some point we want to stabilise those, but the `v1` interface is fixed, so we expose all of the `v1` methods now as `chainHead_v2` and add the unstable methods to the `v2` namespace at the same time.

* We can keep the "original" `v1` methods also exposed in the `v1` namespace for backward compat (seems sensible to do this for as long as possible)

Yes! I'm making the bet that this shouldn't happen too often, because missing a function indicates either a big fuck up (which normally shouldn't happen) or that some design aspect of Substrate/Polkadot has changed (which shouldn't happen very often).

jsdw commented 1 year ago

Thanks, that makes sense!

@josepot FYI I'm happy for this to merge when you are!

(also FYI, the chainSpec_v1_ stable methods are already implemented and exposed in Substrate at least in case that helps, so we have a stable way to obtain genesis hash from that namespace)

josepot commented 1 year ago

Thanks, that makes sense!

@josepot FYI I'm happy for this to merge when you are!

(also FYI, the chainSpec_v1_ stable methods are already implemented and exposed in Substrate at least in case that helps, so we have a stable way to obtain genesis hash from that namespace)

Sorry, I thought that I had already approve this 😬