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

feat(core): load genesis file by network id #250

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

dan-da
Copy link

@dan-da dan-da commented Nov 24, 2022

Addresses #243

Support for loading a different genesis block for each network.

edit: see this summary of latest changes

The genesis block is loaded from a file whose path is the pattern: ~/.kindelia/genesis/<network-id>.kdl

Changes:

core:

  • add pub(crate) util::genesis_path() with associated error enum
  • add pub(crate) util::genesis_code() with associated error enum
  • modify hvm::test_statements*() to accept network_id and load genesis block from file instead of compiled string
  • modify node::new() to to accept network_id and load genesis block from file instead of compiled string

note: the error enums are based on the style/pattern recommended in this article, which is well worth a read. The pre-existing calling functions still call unwrap() on the result, but intent is to address that in #247.

cli:

  • add network_id to test command, required by hvm::test_statements()
  • fix parsing of hex values for --network_id
  • add clap_num dep for parsing hex values

I will note that this change might have security implications. In particular, when a genesis block is loaded from a static string in the executable then a checksum of the executable binary will also include the genesis block. By moving the block out into the filesystem we lose that guarantee. So both the binary and corresponding block file must be summed independently. Of course the benefit is that the system can cleanly handle an arbitrary number of networks. It could be that for mainnet, we use a hybrid approach, whereby mainnet and perhaps present testnet(s) genesis blocks are included in the binary, but others can be created by users at will.

@dan-da
Copy link
Author

dan-da commented Nov 24, 2022

I think tests are failing in CI because the genesis file does not exist yet. They pass on my machine, where it does exist. Probably I need to update the kindelia init command to create this file. I thought about this a couple times and then forgot about it. I will look at it tmw.

kindelia/src/cli.rs Outdated Show resolved Hide resolved
kindelia_core/src/hvm.rs Outdated Show resolved Hide resolved
kindelia_core/src/node.rs Outdated Show resolved Hide resolved
kindelia_core/src/hvm.rs Outdated Show resolved Hide resolved
kindelia/src/cli.rs Show resolved Hide resolved
@steinerkelvin
Copy link
Contributor

steinerkelvin commented Nov 24, 2022

update the kindelia init command to create this file

Totally. It should be done inside (or beside) init_config_file().

@dan-da
Copy link
Author

dan-da commented Nov 26, 2022

Ok, so I just pushed a commit that does the kindelia init genesis install and also passes &[Statement] to core api instead of keying off network_id.

Getting the init functionality working was a bit tricky. I had to think about how these genesis files will be stored in the git repo and installed. I figured it makes sense that they would have same filename in the repo as in user's homedir. I put them in kindelia crate since all the loading is taking place there now.

Previously the genesis code was loaded into the executable with include_str!("genesis.kdl") macro, which works fine. But it requires a static string for the file path. Now I wanted the code to dynamically include the most recent version, eg "0xCAFE0006.kdl". That should be possible with a macro, but I am not experienced writing rust macros yet. So I found include_dir!() crate/macro that includes all files in a given directory. With this, I am able to sort and get the latest version for installation, which works.

However that gets me thinking: with this include_dir!() functionality including multiple genesis files directly in the source code, do we really need/want to load genesis files from disk at runtime? For purposes of testnets and mainnet, would it not be sufficient (and less complicated) to have a few directly in the binary? (and no need to install + load)

The tradeoff is that users would not be able to define their own networks without recompiling. I am unsure that users would typically need to define their own networks though...?

I hope I am being clear. ;-) See also the kindelia/genesis/README.md

@dan-da dan-da force-pushed the issue_243 branch 2 times, most recently from f6435eb to 3911d3a Compare November 28, 2022 20:36
@dan-da
Copy link
Author

dan-da commented Nov 28, 2022

rebased to dev tip.

@dan-da dan-da requested a review from steinerkelvin December 1, 2022 16:48
@dan-da
Copy link
Author

dan-da commented Dec 1, 2022

I don't understand why the tests are failing in CI. The errors indicate that ~/.kindelia/genesis/0xCAFE0006.kdl is not found.

But it works fine for me, provided that I run kindelia init. Example:

$ cargo install --path .
...
$ mv ~/.kindelia/ ~/.kindelia.bak

$ kindelia init
Writing default configuration to '/home/danda/.kindelia/kindelia.toml'...

$ tree ~/.kindelia/
/home/danda/.kindelia/
├── genesis
│   └── 0xCAFE0006.kdl
└── kindelia.toml

1 directory, 2 files

$ cargo test
...
test result: ok. 37 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 8.44s

So either the CI is somehow not running kindelia init or there is something else different in the environment I do not understand.

@steinerkelvin
Copy link
Contributor

I am unsure that users would typically need to define their own networks though

I think this is useful for development / experimentation. But it's indeed not strictly necessary.

With this, I am able to sort and get the latest version for installation, which works.

I was thinking of keeping include_str!("genesis.kdl"), hardcoding the "default" besides it (in this version 0xCAFE0006) that would be filled in the default.toml template to be written on the user config, and would be the filename for genesis.kdl when installed.

It's important to keep genesis.kdl static - somehow - because it's used in some tests. These unit tests should not depend on external files at all.

So either the CI is somehow not running kindelia init or there is something else different in the environment I do not understand.

It's indeed not run, this should not be necessary to run tests.

@dan-da
Copy link
Author

dan-da commented Dec 3, 2022

It's important to keep genesis.kdl static - somehow - because it's used in some tests. These unit tests should not depend on external files at all.

It's indeed not run, this should not be necessary to run tests.

I thought I had accounted for this by creating genesis-tests.kdl which the tests include_str!().

What I missed is that some tests actually invoke the kindelia binary with a kindelia!() macro and then check its output. And since I had already run kindelia init locally, the test succeeded.

I think what I can do is change the test command to accept a file path as you suggest. Then the tests can pass that file to the binary.

Anyway, I will be sure and run the tests without the ~/.kindelia directory existing before pushing to this branch again.

@dan-da dan-da marked this pull request as draft December 5, 2022 02:40
@dan-da
Copy link
Author

dan-da commented Dec 7, 2022

I went back to the drawing board a little on this, thinking about what is useful and makes sense for the end user.

The general approach is to make it easy for user to create a new network and genesis block for testing and local development, but also make it difficult to inadvertently screw up the genesis block for an "official" network.

Overview:

  • same: kindelia/genesis/networks/*.kdl are compiled into binary and considered built-in networks. Any number of built-in networks can be supported simultaneously.
  • same: user can choose between built-in networks using network_id (env, conf, cli arg)
  • new: genesis statements for built-in networks are not installed into ~/.kindelia. They only exist in the executable. No action is needed in init.
  • new: user can specify a network_id that is not built-in and also specify a genesis file containing prelude statements (env, conf, cli arg)
  • new: if user attempts to specify a genesis file for built-in network, an error is printed and node does not start. The idea here is to prevent confusion/weirdness if a user mistakenly (or knowingly) attempts to run non standard prelude with a built-in network, eg mainnet.
  • fixed: test command accepts a genesis file from user, or else defaults to kindelia_core/genesis-tests.kdl
  • fixed: kindelia_core/genesis-tests.kdl is compiled into binary and used for test cases in both kindelia_core and kindelia.

@dan-da dan-da marked this pull request as ready for review December 7, 2022 03:41
@dan-da dan-da marked this pull request as draft December 7, 2022 03:57
@dan-da dan-da marked this pull request as ready for review December 7, 2022 03:58
@dan-da dan-da marked this pull request as draft December 7, 2022 22:40
@dan-da dan-da marked this pull request as ready for review December 7, 2022 22:40
@dan-da
Copy link
Author

dan-da commented Dec 7, 2022

rebased on latest dev.

@dan-da dan-da marked this pull request as draft December 13, 2022 20:19
@dan-da dan-da marked this pull request as ready for review December 13, 2022 20:19
@dan-da
Copy link
Author

dan-da commented Dec 13, 2022

rebased on latest dev to resolve conflict.

dan-da and others added 2 commits December 18, 2022 14:20
Addresses kindelia#163

Previously chain state was stored under a path such as:

    ~/.kindelia/state/{blocks,heaps}

With this change, the data is stored at:

    ~/.kindelia/state/<network_id>/{blocks,heaps}

This enables for example flipping back and forth between a testnet
and mainnet just by changing network_id in the config, or even via
cli arg.
Addresses kindelia#243

Support for loading a different genesis block for each network.

The genesis block is loaded from a file whose path is the pattern:
~/.kindelia/genesis/<network-id>.kdl

Changes:

 core:
  * add util::genesis_path() with associated error enum
  * add util::genesis_code() with associated error enum
  * modify hvm::test_statements*() to accept network_id and load genesis
    block from file instead of compiled string
  * modify node::new() to to accept network_id and load genesis block
    from file instead of compiled string

 cli:
  * add network_id to test command, required by hvm::test_statements()
  * fix parsing of hex values for --network_id
  * add clap_num dep for parsing hex values
kindelia_core:
* genesis.kdl file removed. (moved into kindelia/genesis/networks)
* genesis Statements are passed to kindelia_core api, not network_id
* added kindelia_core/genesis-tests.kdl for core test cases
* updated test cases
* remove empty constants.rs
* remove genesis_path().  (moved into kindelia/src/genesis.rs)

kindelia:
* all genesis files get compiled into kindelia executable
* latest (by name) genesis file gets installed by kindelia init
* add kindelia/genesis/README.md
* add genesis.rs and move some util fn into it
* cargo add include_dir
* cargo fmt fixes
* parse genesis statements in 'node start' and 'test'
* update test(s)
This fixes some test cases that were failing when run before
`kindelia init`.  This failed because the tests invoke the
kindelia executable and depend on statements in the genesis block.

The fix is to:
1) load code from ../kindelia_core/genesis-tests.kdl by default
2) allow user to supply an alternate file with --genesis arg.

cli.rs
* add --genesis to test cmd

main.rs
* read genesis arg and load code from file if present
* load code from ../kindelia_core/genesis-tests.kdl if not present
* test_code() accepts genesis_code arg instead of network_id
Simplifies loading of the genesis block (prelude) because we simply
load the code from static string compiled into executable.

Previously, we wrote the code to a file on disk during the `init`
command and later read it in for other commands.

genesis.rs:
* remove genesis_path() and GenesisPathError
* remove init_genesis() and InitGenesisError
* modify genesis_code() to read from static string instead of a file

main.rs:
* remove call to init_genesis()

README.rs:
* remove note about init command installing genesis files.
Adds ability for user to specify genesis statements for a network.

To avoid confusion, it is not allowed to override the genesis
statements for a network_id that is built into the executable.

cli.rs:
* node command accepts optional genesis argument

main.rs:
* retrieve genesis path from:
   env: KINDELIA_GENESIS
   config: node.network.<network_id>.genesis
   cli arg: genesis
* load genesis statements from file if provided and
  verify the network_id is not compiled into executable.
Fixes a "property X not found in config" error when the missing
property has a default value.
@dan-da dan-da marked this pull request as draft December 18, 2022 23:27
@dan-da dan-da marked this pull request as ready for review December 18, 2022 23:27
@dan-da
Copy link
Author

dan-da commented Dec 18, 2022

rebased on latest dev to resolve conflict.

@dan-da dan-da marked this pull request as draft December 22, 2022 21:08
@dan-da dan-da marked this pull request as ready for review December 22, 2022 21:08
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.

3 participants