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: standardize node directory structure #1944

Merged
merged 14 commits into from
Apr 25, 2024

Conversation

zivkovicmilos
Copy link
Member

@zivkovicmilos zivkovicmilos commented Apr 17, 2024

Description

Closes #1882

This PR standardizes the node's directory structure, and does not assume that the genesis.json is contained within the node directory (further tackled in #1883).

The biggest change introduced in this PR is that the node's directory path is treated as absolute, so there is no funny business with path concatenation when the node is running.

This is best described with the desired structure:

.
├── genesis.json
└── my-node-dir/
    ├── db/
    │   ├── blockstore.db (folder)
    │   ├── gnolang.db (folder)
    │   ├── state.db (folder)
    │   └── cs.wal
    ├── secrets/
    │   ├── priv_validator_state.json
    │   ├── node_key.json
    │   └── priv_validator_key.json
    └── config/
       └── config.toml

BREAKING CHANGES:

  • the genesis.json file is no longer contained inside the node's working directory, but outside it by default (this is dropped entirely in [chain] Add a genesis.json path flag to gnoland start #1883)
  • the configuration is moved top-level to the node's directroy
  • the secrets are no longer contained in <node-dir>/config, but in <node-dir>/secrets
  • the priv_validator_state.json is moved to <node-dir>/secrets by default

If you have an old Gno chain, and what to keep backwards compatibility, move the genesis.json to just outside the node directory (to prevent it from being regenerated), and update the relevant config.toml path values (genesis_file and home for the BaseConfig). Additionally, update all home paths in the config.toml to be absolute paths (this is dropped in #1884).

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@zivkovicmilos zivkovicmilos added the breaking change Functionality that contains breaking changes label Apr 17, 2024
@zivkovicmilos zivkovicmilos self-assigned this Apr 17, 2024
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Apr 17, 2024
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 47.05882% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 45.59%. Comparing base (2f72f84) to head (9cb751d).
Report is 5 commits behind head on master.

Files Patch % Lines
tm2/pkg/bft/config/config.go 25.00% 7 Missing and 2 partials ⚠️
tm2/pkg/bft/config/toml.go 50.00% 2 Missing and 3 partials ⚠️
gno.land/cmd/gnoland/start.go 75.00% 1 Missing and 1 partial ⚠️
gno.land/pkg/gnoland/app.go 0.00% 1 Missing ⚠️
tm2/pkg/crypto/keys/utils.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1944      +/-   ##
==========================================
- Coverage   45.63%   45.59%   -0.05%     
==========================================
  Files         480      482       +2     
  Lines       68817    68860      +43     
==========================================
- Hits        31408    31394      -14     
- Misses      34795    34849      +54     
- Partials     2614     2617       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zivkovicmilos zivkovicmilos changed the title feat standardize node directory structure feat: standardize node directory structure Apr 18, 2024
tm2/pkg/bft/config/config.go Outdated Show resolved Hide resolved
tm2/pkg/bft/config/utils.go Show resolved Hide resolved
.gitignore Show resolved Hide resolved
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

🙏

pre-approving, good after changes

@thehowl
Copy link
Member

thehowl commented Apr 18, 2024

From the review meeting:

  • Agreed on the directory structure, and on renaming the sub-directory data to db (avoiding ambiguity in potential documentation in the phrase "delete the data directory")
  • Agreed on the default name for --data-dir to be gnoland-data

@zivkovicmilos zivkovicmilos requested a review from a team as a code owner April 19, 2024 14:17
@albttx
Copy link
Member

albttx commented Apr 22, 2024

Do you mind keeping the config.toml inside a config/ directory ?

This could be useful for multiple reasons.

  • it's more compliant with cosmos-sdk
  • If in the future we have a sdk on top (gnosdk), or anything that require configurations, we'll have more files in root

I don't really understand why you put the genesis.json outside of the directory structure... Could you let in into config/genesis.json. And if some (i don't see why) people need a genesis elsewhere, allow a flag --genesis-file ?

And YES, this as it is will break the portal-loop

@zivkovicmilos
Copy link
Member Author

Do you mind keeping the config.toml inside a config/ directory ?

This could be useful for multiple reasons.

  • it's more compliant with cosmos-sdk
  • If in the future we have a sdk on top (gnosdk), or anything that require configurations, we'll have more files in root

I don't really understand why you put the genesis.json outside of the directory structure... Could you let in into config/genesis.json. And if some (i don't see why) people need a genesis elsewhere, allow a flag --genesis-file ?

And YES, this as it is will break the portal-loop

We are completely dropping the config directory, as the config is becoming optional, and not required to run the node (as it is with the SDK, and old Gno versions). Once we've separated the concept of node secrets, the config directory loses its meaning. Please check #1836 and child issues.
I also don't see a reason to keep SDK tooling compatibility, since we are fundamentally moving away from how the SDK works -- if that means we have to write new tools to manage nodes, that's completely fine, it's what we've been doing anyways for the last 2 years

The reasoning for genesis.json being moved out of the node directory is:

  • it's required, you need a genesis.json to run a node
  • you're gonna fetch this genesis.json from another location if you're joining an existing network (GitHub, wget...)
  • you need to generate and share this genesis.json if you're starting a new network

The node directory should contain wipe-able data, (if we wipe the node directory, we can catch up easily again), so keeping the genesis there doesn't make too much sense with this premise

@zivkovicmilos
Copy link
Member Author

And YES, this as it is will break the portal-loop

What changes need to be done to the portal loop to accommodate this PR?
What's the breaking part of it?

@albttx
Copy link
Member

albttx commented Apr 22, 2024

Changes require to be done:

@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Apr 22, 2024

Changes require to be done:

@albttx

I've updated the sed values, good catch 💯

533cae2

Can you please check if everything for the Portal Loop is working fine with this commit locally?

The gnoland start command is updated for the new directory structure in this PR. Please also check for any kind of genesis manipulation done by the Portal Loop, as by default this PR generates the genesis.json outside the node directory

tm2/pkg/bft/config/toml.go Outdated Show resolved Hide resolved
@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Apr 24, 2024

@thehowl
After discussing with @albttx, and having a lot of back and forth, we decided it was best to keep the config.toml by default in the config directory of the node folder:

87173b8

The reason being we will have more fine configurations in the future (gnoVM, different modules), and it makes sense to keep them grouped like this

It's also worth noting it's completely irrelevant where the config is located, since the secret paths are no longer relative (as of this PR), and will get dropped entirely in a PR that resolves #1884

@zivkovicmilos zivkovicmilos merged commit 0fde5b8 into gnolang:master Apr 25, 2024
123 of 128 checks passed
@zivkovicmilos zivkovicmilos deleted the feat/standardize-node-dir branch April 25, 2024 09:38
thehowl added a commit that referenced this pull request Apr 25, 2024
- Depends on #1944 (git history is based on that one)
- Closes #1883 



<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Co-authored-by: Milos Zivkovic <milos.zivkovic@tendermint.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

[chain] Standardize node directory structure
4 participants