mimblewimble / grin

Minimal implementation of the Mimblewimble protocol.
https://grin.mw/
Apache License 2.0
5.04k stars 990 forks source link

Add migration to `config_file_version = 2` #3634

Closed trevyn closed 3 years ago

trevyn commented 3 years ago

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

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 commented 3 years ago

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 commented 3 years ago

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. :)

antiochp commented 3 years ago

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

https://github.com/mimblewimble/grin/blob/1b8acee72e7a4236cdf8561a7af5f894bfe11985/config/src/comments.rs#L322-L329

antiochp commented 3 years ago

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

antiochp commented 3 years ago

Tested locally and looks good to me. :+1: