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

Add chainSpec_unstable_spec method #124

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Add chainSpec_unstable_spec method #124

wants to merge 26 commits into from

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jan 18, 2024

This PR adds the chainSpec_unstable_spec method to fetch the chainSpec from a running node.

Substrate nodes expose currently sync_state_genSyncSpec. The new method unifies the format of substrate chain-spec needed for the light-clients to sync to the head of the chain.

There are several differences from the traditional chain-spec:

  • the Genesis entry contains either a Raw entry or a stateRootHash entry
    • the stateRootHash represents the merkle value of the genesis. This is an optimisation used by smoldot to store smaller chainSpec files, and to allow light-clients to sync up faster (since a raw entry must be first converted to a stateRootHash entry).
  • protocolID and forkId are placed under a common networkProperties object
  • para_id and relay_chain are placed under a common parachain object; while using camelCase naming
  • telemetryEndpoints contains objects instead of tuples to explicitly describe the address of the telemetry server and the verbosity_level
  • a new checkpoint object is added
    • the forkedBlocks entry is renamed to trustedBlocks and moved under this object
      • forkedBlocks are now identified by a block number and block hash
    • the badBlocks are moved under this object

The checkpoint object could then be extended to include the syncState needed by lightclients: https://github.com/paritytech/substrate/issues/11184

Block numbers are represented as string unsigned integers, similar to the current chainSpec. I believe this may be an artifact of javaScript users.

Would love to get your thoughts on this 🙏
// cc @jsdw @tomaka @bkchr @skunert

Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@tomaka
Copy link
Contributor

tomaka commented Jan 18, 2024

While I like the idea of defining the format of the chain spec in the JSON-RPC specification, it is unfortunately a bit complicated to define a proper format, due to a lot of details.

In smoldot, I intended to indeed add a JSON-RPC function that returns the chain spec, but I wanted to leave this JSON-RPC function unstable forever and make it smoldot-specific.

Apart from which name to give to things, here's a list of remarks:

  • protocolId is completely deprecated. Get rid of it.

  • What happens if you pass true as parameter, but the chain spec that the node knows only contains stateRootHash? Should the node download the genesis storage?

  • One of the biggest problems is the relay_chain field, because its value doesn't have any meaning, and is just a "reference" to something external. You can see it as something akin to a path to a file, in the sense that it doesn't make sense to send this value over the network, unless we also add a way to retrieve the relay chain chain spec itself.

  • The properties field is really a plain hack that nobody has ever bothered fixing, and it's starting to be annoying to carry this hack everywhere. The efforts that we're making to maintain this field are disproportionally larger than the efforts that it would take to get rid of it entirely. Similarly, chainType and name feel completely useless. The only point of the name field is to return it from the JSON-RPC server so that it is then displayed on PolkadotJS. It would be way more appropriate for PolkadotJS to just load this name from the chain.

  • A checkpoint containing just trustedBlocks is useless. There's no point in having this field before Light sync state needs a proper format polkadot-sdk#60.

  • There's no such thing as a "trusted bootnode" in this context. When the user of smoldot or Substrate pass a chain spec, we say that the bootnodes are trusted in the sense that smoldot/Substrate trusts the user. If the user passes wrong bootnodes, the only person who suffers consequences is the user, so it's fair game. If you return a chain spec from the JSON-RPC API, the word "trusted" doesn't really mean much.

@lexnv
Copy link
Contributor Author

lexnv commented Aug 1, 2024

In this revision I've added the consensus and finality bits we need to sync with the chain.
Its a bit less elegant than the agnostic checkpoint, but this way smoldot can provide the chainSpec without making queries to the full nodes.

I suggest adding this as unstable and implementing it in substrate to have a starting point.
We can always go back to the agnostic checkpoint if you think it is not too error prone

Would love your feedback on this
@tomaka @jsdw 🙏

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.

2 participants