Refactor Rust-based chain specs #592

HCastano · 2024-01-16T00:45:43Z

This PR refactors the Rust-based chain spec code.

It removes some unused chain specs, and moves the remaining ones (dev,
integration-tests, and testnet) into their own modules. This makes it easier to make
changes to a specific chain spec without being tangled in a sea of other configuration
options and chain specs.

I've also pulled out a lot of accounts/public keys into consts (or equivalent) to make
the configurations easier to read.

One behavioral change here is that now if you run without specifying a --chain, then
it'll run a --dev chain. Before it used to run a local chain.

Follow-ups

There's a few follow-ups that I'd like to do after this is merged:

For the testnet genesis accounts, I'd like to use the accounts and maybe IPs that we actually
intend to launch testnet with. This will make it so that @vitropy doesn't need to
manually fill these fields out as part of the Ansible playbooks
- If we can't get the IPs that's fine, we'll just keep substituting them in the
  playbook
I'd like to review the configurations across each network and see if we need all the
validators/TSS servers listed, or really any other pallet configurations
I'd like to review the accounts used across all the networks and clearly document their
mnemonics and purpose
- For example, in the dev config it's unclear if we need three different accounts in the
  relayer GenesisConfig
- There's also some random byte-based accounts floating around that I've got no clue
  what the original mnemonic is
Look into merging dev and integration-tests networks #596
Use JSON + builder pattern in chain specs #595

We don't need this anymore. Functionality has been moved into the `chain_spec` module.

Let's keep this as simple as we can. If we need to add more chain configs later let's do that, but only if they're acutally being used.

This make `--dev` the default option if no chain is passed in. In the future we'll may want to change this to be our mainnet.

vercel · 2024-01-16T00:45:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
entropy-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 17, 2024 10:37pm

Ideally I'd want this to be the same as the `dev` config, but let's leave it this way for now.

HCastano · 2024-01-17T03:34:55Z

node/cli/src/chain_spec/dev.rs

+                    Some([
+                        28, 63, 144, 84, 78, 147, 195, 214, 190, 234, 111, 101, 117, 133, 9, 198,
+                        96, 96, 76, 140, 152, 251, 255, 28, 167, 38, 157, 185, 192, 42, 201, 82,
+                    ]),


These are some of the mystery accounts I mention in the PR description. If nobody knows the mnemonic I'll just re-create these with a documented one in a follow-up PR

Thats not a mystery account if ive understood you. This is a tuple, (account_id, keyvisibilty, Option<x25519_public_key>) where keyvisibility is a u8 representing public, private or permissioned. So //Eve's account is private and has a given x25519 public key. @JesseAbram did it like this because there was some difficulty using the KeyVisibilty type here, i don't remember exactly what it was.

This x25519 keypair was created by calling the derive_static_secret fn //Eve's private signing key. So we could maybe make that clearer by calling that fn here rather than giving it hard-coded.

ya it is sometimes wildly hard to get enums and structs to compile inside the runtime wasm and then also outside of it in the build spec

Thats not a mystery account if ive understood you. This is a tuple, (account_id, keyvisibilty, Option<x25519_public_key>) where keyvisibility is a u8 representing public, private or permissioned. So //Eve's account is private and has a given x25519 public key

Thanks for clarifying this. In the genesis config the last part of the tuple is just a [u8; 32], so it's unclear what it actually was supposed to be.

This x25519 keypair was created by calling the derive_static_secret fn //Eve's private signing key. So we could maybe make that clearer by calling that fn here rather than giving it hard-coded.

Thing is, we use two different sets of Alice, Bob, etc. accounts. Some come from Substrate's sp_keyring's Keyring, while others are accounts which we've designated as Alice, Bob, etc. and are used by the TSS. Without the mnemonic it's hard to tell which account is which. But yes, would definitely be nicer to derive it in place.

ya it is sometimes wildly hard to get enums and structs to compile inside the runtime wasm and then also outside of it in the build spec

We can improve this using typedefs, and things like this:

enum Enum { Foo = 1, Bar = 2, Baz = 3, }

We don't need to use complex types which are hard to serialize, and we don't need to work with raw byte arrays.

ameba23

Nice work! this makes stuff a lot clearer.

Its a little bit hard to see what exactly has changed as names of things have changed as well as what files they are in. Is the idea that docker compose would now use development_config, and testnet-local would be for if external people wanted to make their own testnet deployment similar to ours? I'm asking because the name 'testnet-local' kind of implies it runs locally, but it can't be used in the 'local' docker compose setup as it currently is because it has 4 validators rather than 2.

ameba23 · 2024-01-17T14:01:00Z

node/cli/src/chain_spec/dev.rs

+                    Some([
+                        28, 63, 144, 84, 78, 147, 195, 214, 190, 234, 111, 101, 117, 133, 9, 198,
+                        96, 96, 76, 140, 152, 251, 255, 28, 167, 38, 157, 185, 192, 42, 201, 82,
+                    ]),


Thats not a mystery account if ive understood you. This is a tuple, (account_id, keyvisibilty, Option<x25519_public_key>) where keyvisibility is a u8 representing public, private or permissioned. So //Eve's account is private and has a given x25519 public key. @JesseAbram did it like this because there was some difficulty using the KeyVisibilty type here, i don't remember exactly what it was.

This x25519 keypair was created by calling the derive_static_secret fn //Eve's private signing key. So we could maybe make that clearer by calling that fn here rather than giving it hard-coded.

node/cli/src/endowed_accounts.rs

ameba23 · 2024-01-17T14:28:46Z

node/cli/src/command.rs

    fn load_spec(&self, id: &str) -> Result<Box<dyn sc_service::ChainSpec>, String> {
        Ok(match id {
-            "dev" => Box::new(chain_spec::development_config()),
-            "test" => Box::new(chain_spec::testing_config()),
-            "local-devnet" => Box::new(chain_spec::local_devnet_config()),


The docker compose setup uses local-devnet so if removing it here i guess we need to change docker-compose.yaml.

nah I think you need to add it in specifically since it uses different urls to match the docker containers, this def needs to be kept in as an option

It would be nice to have this be configurable on the Docker side instead of having specific chainspecs for it. Future problem though

JesseAbram

We need one more chain type for docker other than that this looks good

JesseAbram · 2024-01-17T14:57:49Z

node/cli/src/chain_spec/dev.rs

+                    Some([
+                        28, 63, 144, 84, 78, 147, 195, 214, 190, 234, 111, 101, 117, 133, 9, 198,
+                        96, 96, 76, 140, 152, 251, 255, 28, 167, 38, 157, 185, 192, 42, 201, 82,
+                    ]),


ya it is sometimes wildly hard to get enums and structs to compile inside the runtime wasm and then also outside of it in the build spec

JesseAbram · 2024-01-17T15:01:02Z

node/cli/src/command.rs

    fn load_spec(&self, id: &str) -> Result<Box<dyn sc_service::ChainSpec>, String> {
        Ok(match id {
-            "dev" => Box::new(chain_spec::development_config()),
-            "test" => Box::new(chain_spec::testing_config()),
-            "local-devnet" => Box::new(chain_spec::local_devnet_config()),


nah I think you need to add it in specifically since it uses different urls to match the docker containers, this def needs to be kept in as an option

node/cli/src/chain_spec.rs

This is meant to be compatible with the hostname scheme expected by the local `docker-compose` setup.

HCastano · 2024-01-17T21:38:59Z

Its a little bit hard to see what exactly has changed as names of things have changed as well as what files they are in.

Yeah sorry. The best way to review is to open this up locally and look around the new chain_spec folder. The structure will be more clear that way.

Is the idea that docker compose would now use development_config, and testnet-local would be for if external people wanted to make their own testnet deployment similar to ours? I'm asking because the name 'testnet-local' kind of implies it runs locally, but it can't be used in the 'local' docker compose setup as it currently is because it has 4 validators rather than 2.

So tbh I forgot about the Docker config. Good catch on that!

Your comment got me thinking and I'm gonna say that the convention is that a *-local chain should be compatible with our Docker Compose setup. I've updated both the devnet and testnet configs to support this.

The testnet-local config only has two validators, Alice and Bob, not four. The testnet config does have four validators however.

node/cli/src/command.rs

HCastano added 9 commits January 15, 2024 15:15

Move some chain spec accounts to constants

ea7851e

Create new chain_spec module

e787a1b

Fill out dev module

15e2213

Add WIP load_spec changes

069c75b

Fill out testnet module

6e03f9f

Add testnet_local configuration

38396c5

Remove admin module

5c25b9d

We don't need this anymore. Functionality has been moved into the `chain_spec` module.

Remove uneccessary chain configs

c1efa38

Let's keep this as simple as we can. If we need to add more chain configs later let's do that, but only if they're acutally being used.

Clean up --chain options

1d08f61

This make `--dev` the default option if no chain is passed in. In the future we'll may want to change this to be our mainnet.

TaploFmt

7523d3a

vercel bot deployed to Preview January 16, 2024 01:36 View deployment

HCastano added 2 commits January 16, 2024 16:18

Move testing configuration to own module

e22791d

Ideally I'd want this to be the same as the `dev` config, but let's leave it this way for now.

Rename test chainspec to integration_tests

32a9648

vercel bot deployed to Preview January 16, 2024 23:54 View deployment

HCastano added 8 commits January 16, 2024 17:04

Move X25519 public keys into consts and own module

eb6ca1b

Move TSS AccountID into own module

0f867e3

Use new consts in integration_tests module

466ab20

Clean up some unused re-exports

ddb452a

Use full word for "properties"

1393f21

Use get_account_id_from_seed from import instead of full path

0f6f318

Add Charlie X25519 public key

856daf2

Update comments around what each network is made up of

1e80359

vercel bot deployed to Preview January 17, 2024 01:52 View deployment

HCastano added 4 commits January 16, 2024 19:58

It doesn't need to be a validator for tests to pass

ab3fc2c

Clean up unused re-export

fe40636

Update some comments

63c0706

Replace more hex accounts with named accounts

38cfa27

vercel bot deployed to Preview January 17, 2024 03:26 View deployment

HCastano marked this pull request as ready for review January 17, 2024 03:30

HCastano requested review from JesseAbram and ameba23 January 17, 2024 03:30

HCastano commented Jan 17, 2024

View reviewed changes

vercel bot deployed to Preview January 17, 2024 03:53 View deployment

ameba23 reviewed Jan 17, 2024

View reviewed changes

JesseAbram requested changes Jan 17, 2024

View reviewed changes

HCastano added 3 commits January 17, 2024 13:59

Pull out Eve X25519 public key into const

83bec8b

Add devnet-local configuration back

3be8e60

This is meant to be compatible with the hostname scheme expected by the local `docker-compose` setup.

Change testnet-local address to use Docker addresses

f0b5785

HCastano requested review from JesseAbram and ameba23 January 17, 2024 21:40

vercel bot deployed to Preview January 17, 2024 21:56 View deployment

JesseAbram requested changes Jan 17, 2024

View reviewed changes

node/cli/src/command.rs Show resolved Hide resolved

Make note about breaking changes in CHANGELOG

0e9eb27

JesseAbram approved these changes Jan 17, 2024

View reviewed changes

vercel bot deployed to Preview January 17, 2024 22:37 View deployment

HCastano merged commit 9181513 into master Jan 18, 2024
10 checks passed

HCastano deleted the hc/clean-up-chainspec branch January 18, 2024 02:35

HCastano added the Breaks API label Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Rust-based chain specs #592

Refactor Rust-based chain specs #592

HCastano commented Jan 16, 2024 •

edited

Loading

vercel bot commented Jan 16, 2024 •

edited

Loading

HCastano Jan 17, 2024

ameba23 Jan 17, 2024

JesseAbram Jan 17, 2024

HCastano Jan 17, 2024

HCastano Jan 17, 2024 •

edited

Loading

ameba23 left a comment

ameba23 Jan 17, 2024

ameba23 Jan 17, 2024

JesseAbram Jan 17, 2024

HCastano Jan 17, 2024

JesseAbram left a comment

JesseAbram Jan 17, 2024

JesseAbram Jan 17, 2024

HCastano commented Jan 17, 2024

Refactor Rust-based chain specs #592

Refactor Rust-based chain specs #592

Conversation

HCastano commented Jan 16, 2024 • edited Loading

Follow-ups

vercel bot commented Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

ameba23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano commented Jan 17, 2024

HCastano commented Jan 16, 2024 •

edited

Loading

vercel bot commented Jan 16, 2024 •

edited

Loading

HCastano Jan 17, 2024 •

edited

Loading