Add `zallet migrate-zcash-conf` command #13

str4d · 2025-01-27T06:45:12Z

No description provided.

str4d · 2025-01-27T10:21:51Z

zallet/src/commands/migrate_zcash_conf.rs

+        let config_toml =
+            toml::Value::try_from(config).map_err(|e| ErrorKind::Generic.context(e))?;
+        let output =
+            toml::to_string_pretty(&config_toml).map_err(|e| ErrorKind::Generic.context(e))?;


Due to how serde + toml serialization works, any config option that is Option<T> is omitted when None, but any config option that is just T is not omitted when set to its default value. This is how zebrad works as well, but I'm not very happy with it, because what I really want from this command is the config file that a user would have created if they were trying to override the same defaults.

I'm half inclined to make every config option Option<T> so that it can correctly represent "user-set / not-user-set" in memory, and then have getters that map None to the intended default value.

I guess that seems reasonable. In some respects though I kind of like the idea that the defaults are explicitly set in the config file, because then the user doesn't have to go hunting in various places to find out what options are available and what their defaults are; they just get to see all the options up front when looking at the config file.

I know that I value when default config files are "complete" and self-documenting, such that I don't have to go looking for separate documentation.

The problem with defaults set in the config file, is that you can't change them later. E.g. the default txexpirydelta changed from 20 to 40 across the Blossom NU, but if users had an explicit value set then we would not override it. I think there is value in distinguishing "user is using our defaults" from "user has specific choices they wish to use", and that it's reasonable for users to trust us to use and maintain reasonable defaults over time.

I think what I'd prefer is to have a separate command (similar to zebrad generate) that generates a config file containing all of the defaults (unlike zebrad which doesn't fill in defaults for some of its values like Option<T>s). Then users can run it to get a readable form of whatever the current defaults are, and adapt / use them at will. (The downside of offering this command is that most users might end up configuring the defaults explicitly, preventing us from ever updating them. So maybe the command should actually generate an "empty" config file with every default commented out.)

For this command, I prefer that it only generate a file containing settings that were explicitly configured by the user in their zcash.conf.

That makes sense. Having the defaults commented out is a pretty standard way of approaching the problem, and in the associated comments we can also describe why those particular defaults were chosen, and what the valid ranges etc. are.

nuttycom

Generally looks good, though I think that some of the options that you've set up migrations for should not be migrated.

zallet/src/config.rs

zallet/src/commands/migrate_zcash_conf.rs

nuttycom · 2025-01-27T18:01:03Z

zallet/src/commands/migrate_zcash_conf.rs

+        let config_toml =
+            toml::Value::try_from(config).map_err(|e| ErrorKind::Generic.context(e))?;
+        let output =
+            toml::to_string_pretty(&config_toml).map_err(|e| ErrorKind::Generic.context(e))?;


I guess that seems reasonable. In some respects though I kind of like the idea that the defaults are explicitly set in the config file, because then the user doesn't have to go hunting in various places to find out what options are available and what their defaults are; they just get to see all the options up front when looking at the config file.

I know that I value when default config files are "complete" and self-documenting, such that I don't have to go looking for separate documentation.

zallet/src/commands/migrate_zcash_conf.rs

str4d · 2025-01-28T02:27:52Z

Force-pushed to address review comments and implement some TODOs.

nuttycom

utACK e494b89

daira · 2025-01-29T14:04:49Z

zallet/src/commands/migrate_zcash_conf.rs

+        // TODO: Decide whether we want to allow renaming the `WalletDb` backing file.
+        .chain(Action::ignore("wallet"))


This should probably be a warning if it is not the default.

daira · 2025-01-29T14:08:50Z

zallet/src/commands/migrate_zcash_conf.rs

+        // TODO: Figure out where this was used, and if we want to keep it.
+        .chain(Action::ignore("maxtxfee"))


It was used to prevent accidentally specifying an excessively large fee. Given that we will only support ZIP 317 fees, it should be ignored I think. It would be ok to warn.

daira · 2025-01-29T14:11:49Z

zallet/src/commands/migrate_zcash_conf.rs

+            "preferredtxversion",
+            Action::warn(|value| {
+                Some(fl!(
+                    "migrate-warn-unsupported",
+                    option = "preferredtxversion",
+                    value = value,
+                ))
+            }),


There is no need to warn if the preferred version is 5, since that's the default for zallet (and will continue to be the default immediately after NU7 I think?)

daira · 2025-01-29T14:12:46Z

zallet/src/commands/migrate_zcash_conf.rs

+        // The logging systems of `zcashd` and Zallet differ sufficiently that we don't
+        // try to map existing log targets across.
+        .chain(Action::ignore("debug"))


This could be a warning with a link to documentation for Zallet's logging.

daira · 2025-01-29T14:13:44Z

zallet/src/commands/migrate_zcash_conf.rs

+        // We aren't going to migrate over the `zcashd` wallet's experimental features
+        // compatibly. If we add a similar framework to Zallet it will be for from-scratch
+        // features.
+        .chain(Action::ignore("experimentalfeatures"))


This could be a warning saying that all experimental feature flags are ignored.

daira · 2025-01-29T14:14:26Z

zallet/src/commands/migrate_zcash_conf.rs

+        ))
+        // TODO
+        .chain(Action::ignore("regtest"))
+        // TODO: Support mapping multi-arg options.


I thought there is support for that in this PR?

daira · 2025-01-29T14:17:11Z

zallet/src/commands/migrate_zcash_conf.rs

+        // Unsupported in `zcashd` since 1.0.0-beta1.
+        .chain(Action::ignore("rpcasyncthreads"))


Oh, that's part of the cause of the excessive complexity in the zcashd RPC code. (It was a good decision to disable this, mind.)

daira · 2025-01-29T14:18:17Z

zallet/src/commands/migrate_zcash_conf.rs

+        // TODO: Need to check how this interacts with `rpcport`; we may instead need a
+        // warning here and no migration.


A migration with a warning about the change in semantics seems like the right thing here to me.

daira · 2025-01-29T14:19:46Z

zallet/src/commands/migrate_zcash_conf.rs

+        // TODO
+        .chain(Action::ignore("testnet"));


This should be a warning for now I think.

daira · 2025-01-29T14:20:39Z

zallet/src/commands/migrate_zcash_conf.rs

+        "addnode",
+        "alertnotify",
+        "alerts",
+        "allowdeprecated",


Technically I think allowdeprecated is used to allow some wallet-related RPCs, but can be ignored.

daira · 2025-01-29T14:22:41Z

zallet/src/commands/migrate_zcash_conf.rs

+        "forcednsseed",
+        "fundingstream",
+        "fuzzmessagestest",
+        "gen",


Why is "gen" categorized differently to "genproclimit" or "mineraddress"?

daira · 2025-01-29T14:23:11Z

zallet/src/commands/migrate_zcash_conf.rs

+        "help",
+        "help-debug",
+        "ibdskiptxverification",
+        "insightexplorer",


This probably deserves a warning.

daira

Post-hoc ACK with comments.

Render error UX message in the correct place

df1b3b3

str4d force-pushed the cmd-migrate-zcash-conf branch from 1e23a82 to fa126fd Compare January 27, 2025 10:15

str4d commented Jan 27, 2025

View reviewed changes

nuttycom requested changes Jan 27, 2025

View reviewed changes

str4d added 2 commits January 28, 2025 02:27

Add zallet migrate-zcash-conf command

b171ae0

Minor cleanups

e494b89

str4d force-pushed the cmd-migrate-zcash-conf branch from fa126fd to e494b89 Compare January 28, 2025 02:27

nuttycom approved these changes Jan 29, 2025

View reviewed changes

str4d marked this pull request as ready for review January 29, 2025 03:01

nuttycom merged commit a60e0a1 into main Jan 29, 2025

nuttycom deleted the cmd-migrate-zcash-conf branch January 29, 2025 03:09

daira reviewed Jan 29, 2025

View reviewed changes

str4d added the A-config Area: Configuration label Jun 5, 2025

		// TODO: Decide whether we want to allow renaming the `WalletDb` backing file.
		.chain(Action::ignore("wallet"))

		// TODO: Figure out where this was used, and if we want to keep it.
		.chain(Action::ignore("maxtxfee"))

		// Unsupported in `zcashd` since 1.0.0-beta1.
		.chain(Action::ignore("rpcasyncthreads"))

		// TODO: Need to check how this interacts with `rpcport`; we may instead need a
		// warning here and no migration.

Add zallet migrate-zcash-conf command #13

Add zallet migrate-zcash-conf command #13

Uh oh!

Conversation

str4d commented Jan 27, 2025

Uh oh!

str4d Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

str4d commented Jan 28, 2025

Uh oh!

nuttycom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add `zallet migrate-zcash-conf` command #13

Add `zallet migrate-zcash-conf` command #13

str4d Jan 27, 2025 •

edited

Loading

str4d Jan 28, 2025 •

edited

Loading

daira Jan 29, 2025 •

edited

Loading

daira Jan 29, 2025 •

edited

Loading

daira Jan 29, 2025 •

edited

Loading