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 migration to config_file_version = 2 #3634

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

trevyn
Copy link
Contributor

@trevyn trevyn commented Apr 15, 2021

This adds an automatic migration of grin-server.toml to config_file_version = 2, performed as such:

  • Add config_file_version = 2 line after header comments
  • If server.pool_config.accept_fee_base is 1000000, change it to 500000
  • Remove the comment #a setting to 1000000 will be overridden to 500000 to respect the fixfees RFC, since it no longer will once config_file_version >= 2
  • Verify that the updated toml parses to the expected result
  • Write the updated grin-server.toml file to disk

This migration is only applied when the config_file_version key is not present in the existing grin-server.toml file.

For a fresh install, the migration is performed automatically when reading the initial default config.

Error handling can be improved upon request if this general approach is OK.

Closes #3540.

@phyro
Copy link
Member

phyro commented Apr 15, 2021

Awesome work, I have a couple of questions around the changes and what they mean

  1. Should we define a fresh install as config version 2 with 500000 as default since this would allow us to remove this migration later on? or did I understand it wrong and we do that already?
  2. If we are adding versioning to the node configuration, should we consider doing similar on the wallet side?

@trevyn
Copy link
Contributor Author

trevyn commented Apr 15, 2021

Should we define a fresh install as config version 2 with 500000 as default

Functionally, it doesn't really matter. Leaving the version 1 defaults provides some implicit testing of the migration mechanism, which should probably get an explicit test if the default is changed. I see your point though, I'll think on it.

since this would allow us to remove this migration later on?

I don't think it'd be a good idea to ever remove the migration — users could have arbitrarily old configs, so the code should always be able to migrate from a day-1 config file all the way up to whatever the current version is.

If we are adding versioning to the node configuration, should we consider doing similar on the wallet side?

I took a quick look, and didn't see anything that would currently benefit from a new config version. If I missed something, happy to add that.

I'm also happy to spend the time on a more generalized approach (dreams of a generic toml_migration crate), but I thought that might be falling into the over-engineering trap, and would make for a more difficult code review. :)

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

This looks like a good approach here. 👍
Planning to do some local testing of this over the next day or so.

@antiochp
Copy link
Member

We still include the offending comment via comments.rs when auto generating the config file -

grin/config/src/comments.rs

Lines 322 to 329 in 1b8acee

retval.insert(
"accept_fee_base".to_string(),
"
#base fee that's accepted into the pool
#a setting to 1000000 will be overridden to 500000 to respect the fixfees RFC
"
.to_string(),
);

@antiochp
Copy link
Member

So I guess we should also update the generated file (if we generate one) to include the latest version number.

@antiochp
Copy link
Member

Tested locally and looks good to me. 👍

@antiochp antiochp merged commit 9e27e6f into mimblewimble:master Apr 27, 2021
@antiochp antiochp added this to the 5.1.0 milestone Apr 29, 2021
@antiochp antiochp mentioned this pull request May 6, 2021
bayk added a commit to mwcproject/mwc-node that referenced this pull request Jun 17, 2024
…mble#3634) For MWC, no migration is made, keep that code in order need to migrate in the future.
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.

Need support for rewriting grin-server.toml file
3 participants