8000 refactor(config): Remove genesis hash from config · Issue #3547 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor(config): Remove genesis hash from config #3547

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

Closed
jmalicevic opened this issue Jul 23, 2024 · 1 comment · Fixed by #3595
Closed

refactor(config): Remove genesis hash from config #3547

jmalicevic opened this issue Jul 23, 2024 · 1 comment · Fixed by #3595
Assignees
Labels
backport-to-v1.x Tell Mergify to backport the PR to v1.x bug Something isn't working
Milestone

Comments

@jmalicevic
Copy link
Contributor

Genesis hash saved in config file but read-only

#1324 saves the genesis hash in the config file to check it against the has from the DB. However, its sole purpose is to forward this config flag to be checked on node boot-up against the hash of the genesis file if it is the first boot or the hash stored in the DB if the node has already read in a genesis file before. The user is NOT supposed to put the hash into the config file manually. We decided at the time to not break any APIs and apply this approach.

Upon further discussion, we chose this to be a suboptimal way of doing things and bad UX. The flag should be passed as an argument, even if APIs are being broken.

Once a PR is open, we should however check whether we are causing major breaking changes for some users and let them know that this update is coming.

@jmalicevic jmalicevic added bug Something isn't working backport-to-v1.x Tell Mergify to backport the PR to v1.x labels Jul 23, 2024
@jmalicevic jmalicevic added this to the 2024-Q3 milestone Jul 23, 2024
@jmalicevic jmalicevic self-assigned this Jul 23, 2024
@jmalicevic jmalicevic added this to CometBFT Jul 23, 2024 8000
@github-project-automation github-project-automation bot moved this to Todo in CometBFT Jul 23, 2024
@sergio-mena
Copy link
Contributor
sergio-mena commented Jul 25, 2024

I would be best if this could make it into v1.x before cutting v1.0.0

@jmalicevic jmalicevic linked a pull request Jul 30, 2024 that will close this issue
@jmalicevic jmalicevic changed the title chore(config): Remove genesis hash from config refactor(config): Remove genesis hash from config Jul 30, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in CometBFT Jul 31, 2024
mergify bot pushed a commit that referenced this issue Jul 31, 2024
Closes #3547 .

This PR removes the `GenesisHash` from the config file because this
field is not meant to be updated by the operator. It was only introduced
to avoid breaking an API and used to pass the genesis hash provided via
cli to the node constructor.

As this feature is going into an unreleased version, we decided to break
the API of `DefaultNewNode`. We maintain the signature of `NewNode` as
this is probably used by the community more and introduce
`NewNodeWithCliParams`.

`NewNodeWithCliParams` allows for passing different parameters from the
cli to the node.

The PR also introduces a new type `CliParams`. At the moment this struct
contains just the `GenesisHash` but it was done this way so that we can
in the future extend it with new parameters without breaking APIs.

---------

Co-authored-by: Daniel <daniel.cason@informal.systems>
(cherry picked from commit 11c5b25)

# Conflicts:
#	node/node.go
#	node/node_test.go
alesforz pushed a commit that referenced this issue Jul 31, 2024
Closes #3547 .

This PR removes the `GenesisHash` from the config file because this
field is not meant to be updated by the operator. It was only introduced
to avoid breaking an API and used to pass the genesis hash provided via
cli to the node constructor.


As this feature is going into an unreleased version, we decided to break
the API of `DefaultNewNode`. We maintain the signature of `NewNode` as
this is probably used by the community more and introduce
`NewNodeWithCliParams`.

`NewNodeWithCliParams` allows for passing different parameters from the
cli to the node.

The PR also introduces a new type `CliParams`. At the moment this struct
contains just the `GenesisHash` but it was done this way so that we can
in the future extend it with new parameters without breaking APIs.

---------

Co-authored-by: Daniel <daniel.cason@informal.systems>
melekes added a commit that referenced this issue Aug 1, 2024
… (#3600)

Closes #3547 .

This PR removes the `GenesisHash` from the config file because this
field is not meant to be updated by the operator. It was only introduced
to avoid breaking an API and used to pass the genesis hash provided via
cli to the node constructor.


As this feature is going into an unreleased version, we decided to break
the API of `DefaultNewNode`. We maintain the signature of `NewNode` as
this is probably used by the community more and introduce
`NewNodeWithCliParams`.

`NewNodeWithCliParams` allows for passing different parameters from the
cli to the node.

The PR also introduces a new type `CliParams`. At the moment this struct
contains just the `GenesisHash` but it was done this way so that we can
in the future extend it with new parameters without breaking APIs.

<hr>This is an automatic backport of pull request #3595 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v1.x Tell Mergify to backport the PR to v1.x bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants
0