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

Remove chainHead_unstable_genesisHash #61

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jul 17, 2023

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 polkadot-fellows/RFCs#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
Copy link
Contributor

josepot commented Jul 17, 2023

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
Copy link
Contributor Author

tomaka commented Jul 17, 2023

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.

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Sounds fair to me!

Side question: can we not keep eg chainHead_genesisHash unstable even if we stabilise the rest of the interface?

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).

@tomaka
Copy link
Contributor Author

tomaka commented Aug 9, 2023

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
Copy link
Collaborator

jsdw commented Aug 10, 2023

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)

@tomaka
Copy link
Contributor Author

tomaka commented Aug 10, 2023

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
Copy link
Collaborator

jsdw commented Aug 10, 2023

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 josepot self-requested a review August 10, 2023 10:13
Copy link
Contributor

@josepot josepot left a comment

Choose a reason for hiding this comment

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

Thanks!

@josepot
Copy link
Contributor

josepot commented Aug 10, 2023

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 😬

@josepot josepot merged commit c75b3a1 into paritytech:main Aug 10, 2023
2 checks passed
@tomaka tomaka deleted the rm-chain-head-genesis branch August 10, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants