grin-wallet icon indicating copy to clipboard operation
grin-wallet copied to clipboard

Ready for review: Fix for many wallet config errors including solution for -h and -t wrongly setting the node path

Open Anynomouss opened this issue 9 months ago • 7 comments

https://github.com/mimblewimble/grin-wallet/issues/478 [Partly Fixed/answered, cannot be solved easily since generate by toml crate ] https://github.com/mimblewimble/grin-wallet/issues/728 [fixed] https://github.com/mimblewimble/grin/issues/3803 [fixed] https://github.com/mimblewimble/grin/issues/3394 [fixed] https://github.com/mimblewimble/grin/pull/3420 [Abandoned Pull request, my pull replace this one] https://github.com/mimblewimble/grin/issues/3002 [Partly fixed, not default values part] https://github.com/mimblewimble/grin/issues/2300 [Could be closed, not dependent on my pull]

Note The wallet firsts checks a) current directory for config, b) top_dir for config, then c) assumes its in home/.grin. Option a) suppresses generation of a config file when running grin-wallet.exe init, not sure if that is intentional behavior or that the config should be crated but the one in current dir should be loaded.

Anynomouss avatar Mar 08 '25 20:03 Anynomouss

Update https://github.com/mimblewimble/grin-wallet/issues/581 https://github.com/mimblewimble/grin-wallet/issues/728 [fixed] https://github.com/mimblewimble/grin/issues/3803 [fixed] https://github.com/mimblewimble/grin/issues/3394 [fixed] https://github.com/mimblewimble/grin/pull/3420 [Pull, my own pull replaces this one] https://github.com/mimblewimble/grin/issues/3002 [Partly fixed, not default values part] https://github.com/mimblewimble/grin/issues/2300 [Could be closed, not dependent on my pull] • Fix/workaround for a “bug” in the fs::create_dir_alll create to not recognize backslashes as directory separator on Linux. Fixed by converting the top_dir to always use OS_SEPARATOR. This bug only occurs on Linux and was uncovered by @aglkm • Fix all Seed generation message, to format the path with OS specific separator in seed.rs and default.rs • Removing config exists check from wallet_args, all logic is now handled by the cofig::initial_wallet_setup • Added smart functionality: If run with a config in the current directory, the wallet will load the config file and if run with ‘init’ will use as a template for config generation, meaning port settings etc. get copied from the template config.

Possible things to improve

  1. I did not manage to change the top_dir arguments stored in the arguments. Therefore it is update at multiple places in the code. There is a lot of formatting going on in various message functions, but since you normally only get a few messages this should not slow things down much. Perhaps there are ways to reduce some of these formatting calls, I am open to suggestions. I used a helper function to format the path to use OS_SEPARATOR. This helper function is used in seed.rs, defaults.rs and wallet.rs, each with a new definition of the function. If there is such things as a crate where other helper functions are put, perhaps I could put it there and avoid redefining it.

  2. Config generation for wallet created using the command line are now handled by the wallet.rs. This means that if defaults.rs is called to generate a config, e.g. via the owner API, the behaviors is as before not distinguishing node and wallet directory. It needed I can also adjust the code in default.rs to use config::get_node_path and config::get_wallet_path , but I would have to convert the Config Errors to normal Errors. It here are reasons to make change there as well I could still do it, but if not needed I will move on to look at the node header syncing.

@aglkm If you have time, can you give it another test?

Anynomouss avatar Apr 18 '25 12:04 Anynomouss

Issues reported here: https://github.com/mimblewimble/grin-wallet/pull/731#issuecomment-2697447255 Fixed: 2 and 3. Not fixed: 1.

Steps to re-create:

  1. Compile the wallet with the PR
  2. Init a wallet: ./target/release/grin-wallet -t top-dir init
  3. Copy executable into the top dir: cp target/release/grin-wallet ./top-dir/.
  4. CD into the top dir: cd top-dir/
  5. Make sure there's no wallet seed in the default directory (/home/user/.grin/main/wallet_data/) and run the wallet: ./grin-wallet info You will see the following:
20250518 12:32:59.411 ERROR grin_wallet_impls::lifecycle::seed - wallet seed file /home/user/.grin/main/wallet_data/wallet.seed could not be opened (grin-wallet init). Run "grin-wallet init" to initialize a new wallet.
Wallet command failed: LibWallet Error: Lifecycle Error: Error opening wallet (is password correct?)

So the wallet is ignoring the "top-dir" directory (/home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir) and still is trying to use the default one.

In grin-wallet.toml file the path is correct:

#where to find wallet files (seed, data, etc)
data_file_dir = "/home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir/wallet_data"

Another issue: Make sure you are one level up from the "top-dir" directory, then run: ./top-dir/grin-wallet -t top-dir info You will get:

Unable to load wallet configuration: Error serializing configuration: /home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir/grin-wallet.toml already exists in the target directory (/home/user/grin/grin-wallet-sources/grin-wallet-pr732/grin-wallet/top-dir/wallet_data). Please remove it first (Run `grin-wallet init` to create a new wallet)

aglkm avatar May 18 '25 09:05 aglkm

@aglkm Thanks for explaining issue 1, I misunderstood you before.

Basically grin-wallet will always assume that if you do not provide the '-t' top-dir command that it should use the wallet in the default directory. What you are proposing is a smart auto-detect wallet_data directory function. This new smart function would mean grin-wallet would first test if the current directory contains a wallet_data folder, if present grin-wallet will assume this is the wallet you want to use, if not grin-wallet will default to the default wallet data directory. This would be new wallet behavior, I could implement it if all think this is desirable. The advantage I can think of is that it would make it much easier for beginning user to run for example grin-wallet -h in a directory and afterwards the user does not need to specify the directory, grin-wallet will always use the wallet in the current directory of his executable. The downside I can think of is that this smart behavior is implicit and not explicit, a user might not know it and even get confused why not his default wallet is loaded. I could implement this smart functionality but I think at least I should combine it with a warning warning to the users. Something along the line 'wallet_data' detected in current dir, defaulting to use this wallet. What do you think @yeastplume and @aglkm , would that be the desired behavior, or just keep it as is?

Another issue:
Make sure you are one level up from the "top-dir" directory, then run: ./top-dir/grin-wallet -t top-dir info
You will get:

This is actually desired behavior. What I mean is that all paths you provide to grin-wallet should be given relative to your current working directory, not relatively to the executable. For example, if you are in the top_dir, you would have to run ../grin-wallet.exe -t ../top-dir info. Additionally using paths relative to the working directory allows power users to do some cool tricks. For example:

  1. I run grin-wallet.exe -t top_dir init (with a template config file in the current directory of grin-wallet.exe) What will happen is that grin-wallet will create a new config file in the top-dir which uses the config file in the working directory as a template, for example keeping specific peers, ports etc. in the config file. Only the node path, wallet path and tor path will be updated.
  2. cd top_dir
  3. run ..grin-wallet.exe -t ../top_dir info Now there will be no template file in your current working directory since you are in directory top-dir, so the wallet will run using your new config file

I could change this behavior, but I think it is customary for executable to use paths relative to the current working directory and not paths relative to the directory of the executable. Therefore I would propose not to change this behavior and keep it as is. What do you think @yeastplume?

Anynomouss avatar May 22 '25 11:05 Anynomouss

It is documented in grin-wallet docs that the wallet will check the current directory first and then the default one.

When running any grin-wallet command, grin will check the working directory if these files exist. If not, it will use the default location ~/.grin.

This is how it works currently.

What is this PR proposing: if you wish to use local (non default) config files, you have to explicitly specifying current working directory every time you execute a wallet, while you are already locating there. I disagree with this.

./top-dir/grin-wallet -t top-dir info

Here ☝️ I've specified the path to grin-wallet executable relatively to the current working directory and provided the path to "top-dir" relatively to the current working directory as well, not an executable. The current wallet behaviour has no such issue.

You wrote:

What will happen is that grin-wallet will create a new config file in the top-dir which uses the config file in the working directory as a template, for example keeping specific peers, ports etc. in the config file. Only the node path, wallet path and tor path will be updated.

Wallet shouldn't try to think instead of a user, because a desired behaviour could be different. I could want a fresh config file, but instead wallet will create it from an old "template" - a config file which is placed in the current directory. It's more straightforward to let user to copy the old config file if he wishes to preserve the settings.

You wrote:

Now there will be no template file in your current working directory since you are in directory top-dir, so the wallet will run using your new config file

The difference between a template file and a config file: a config file becoming a template file if the user is trying to create a new wallet while also the config file exists in the current directory. If we want to have "template" files it could be better to introduce a new option, so the user can directly specify what he wants to do and do not let the wallet to decide.

As a user, it could be confusing, that the wallet is silently trying to use a config file as a template file.

There's another thing that need attention. When you are creating a config file from a "template" it will create a relative path in grin-wallet.toml file, like that:

#where to find wallet files (seed, data, etc)
data_file_dir = "top_dr"

What's the purpose of this change? This could potentially break the things, e.g. API.

aglkm avatar May 22 '25 13:05 aglkm

It is documented in grin-wallet docs that the wallet will check the current directory first and then the default one.

Interesting, this functionality is not present in the code. It does make sense to add it since many users might expect the wallet to be smart enough to detect it. I will add it to the code, it is similar smart behavior as I have implemented for detecting the path to the node.

Wallet shouldn't try to think instead of a user, because a desired behavior could be different. I could want a fresh config file, but instead wallet will create it from an old "template" - a config file which is placed in the current directory. It's more straightforward to let user to copy the old config file if he wishes to preserve the settings.

This sounds very contradicting when you are saying that you want smart behavior for the wallet to detect the path to the node and smart behavior to detect if a local wallet folder is present, yet you do not want smart behavior? The whole point of this pull request is to fix -t and -here and add smart behavior to grin-wallet to let it behave like the user expect it should.

Smart behavior:

  • Looking where to find the node, first in the current directory, if not there in the default directory
  • Looking which wallet to use, first try current directory if not there, use the default directory (smart behavior that should have been there already according to the documentation)
  • Detecting config file in the executable path and using its to open the right wallet, or as template when creating a new wallet (extension on existing behavior)

Using a config in the executable dir with -init as template is an extension of the behavior that when running without -init the config file will indicate which wallet to open and with what settings. Again, this is not me making real change, just extending on existing behavior. To give you an example, lets say you copy grin-wallet.ex inside the 'wallet_data' directory, it will exactly perform the smart behavior you are asking for, it will detect the config and as such load the wallet in which data directory the executable resides. So this is in fact the described behavior you are asking for. If we keep this behavior and add detection of the 'wallet_data' in the current working directory, this will mean that both if you put the executable in the data directory itself, or in its top directory, it will automatically detect and load the right wallet. I think that is indeed good user friendly behavior.

As a user, it could be confusing, that the wallet is silently trying to use a config file as a template file. I think this is the case for all smart behavior. Smart behavior is implicit since the user does not explicitly ask for it using an input argument. The solution is simple, I will ad Warning or Info level messages to the user to notify them each time any smart behavior is triggered. The alternative is not having any smart behavior at all which is very, very bad for the user experience and conflicts with the behavior described in the readme file. Also we have to be realistic, what is the chance of a user copying a config file in the working directory of without knowing about this smart behavior creating a new wallet? I think the chance of that happening is very small.

More importantly, lets make grin-wallet user friendly for those 99% of user that expect the grin-wallet to emulated some minimal level of intelligence. And also lets not forget that the current situation is that -t and -here are completely broken, which is the worst user experience you can offer. We can only go uphill from the current situation.

Anynomouss avatar May 23 '25 07:05 Anynomouss

I think the pull is ready for review. I made sure all changes from the PR732 were integrated and made my changes on top of them. So first merge PR732, then mine on top of it would be the way to go (after review).

Files that have changed (others can be discarded, only marked as changed) config/src/config.rs [on top of pull 732] controller/src/comments.rs [on top of pull 732] impls/src/lifecycle/default.rs impls/src/lifecycle/seed.rs src/bin/grin-wallet.rs src/cmd/wallet_args.rs impls/src/node_clients/http.rs tests/common/mod.rs

What this pull request fixes:

  • The main changes of this pull request are that running a) grin-wallet.exe -t top-dir init b) grin-wallet.exe init -h c) grin-wallet.exe -t top-dir info, will use (and if needed create) the proper directories.
  • Input paths provided as input using -t (top-dir) will be a) made absolute b) any backslash "\" is replaced with a "/" slash to avoid errors on Linux, Prefix added by Windows 10/11 ("\?") is removed to maintain platform compatibility of grin-wallet.toml In short both using relative or absolute paths as input for -t should work now.
  • The wallet path is selected as follows a) -t top directory if argument provided, b) grin-wallet.toml detected in working dir c) home.
  • Grin-wallet prints the wallet directory it is using to the console. Although this was mostly intended for debugging, it might be useful behavior, even if just to check if this pull request works as intended.
  • Previously this pull request introduced using a grin-wallet.toml in the current directory as template. After further consideration I removed this functionality since it might cause confusion as @aglkm pointed out and it actually lacks any clear use case. I did mostly testing on Windows, so in particular testing on Linux and Mac would be appreciated.

How to test this pull request:

First make a local copy:

Step 1: Clone the original repository git clone https://github.com/mimblewimble/grin-wallet.git cd grin-wallet

Step 2: Fetch the pull request into a new local branch git fetch origin pull/732/head:pr-732

Step 3: Checkout the new branch git checkout pr-732

Step 4: go into grin directory cd grin

TESTS

The examples below are for Windows. On Linux, replace ./grin-wallet.exe with ./grin-wallet. In the console for each wallet action, the wallet address will be printed e.g. wallet path: /your/dir/path. Use this to validate that the path shown for the opened wallet is indeed correct. Additionally you can use different passwords for each test wallet to validate the correct wallet is opened (e.g test1, test2, test3, test4 test)

  1. Run all test to see if this PR does not break anything cargo test

2A) Try to make a new wallet in your regular user directory, I chose a testnet wallet. Be sure that it does not already exist before running the command. If it does exist, a warning should appear asking you to delete it. ./grin-wallet.exe --testnet init

2B) Try opening your new testnet wallet with the password you specified ./grin-wallet.exe --testnet info

3A) Try to generate a new wallet at a specific top directory (top directory is just the name for a directory you pick) ./grin-wallet.exe --testnet -t dir1/dir2/dir3 init Check that the directory structure was created

3B) Try to open the wallet you just created ./grin-wallet.exe --testnet -t dir1/dir2/dir3 info

4A) Try the here (-h) flag, it should generate a wallet in your current working directory. Warning, grin will look in your working directory from the console, not the path relative to your grin executable. This is a conscious design decision and is the same behavior as the bitcoin daemon or any other crypto node or wallet executables ./grin-wallet.exe --testnet init -h

  1. A grin wallet should be created in your current working directory, including a config.toml file. Check if the wallet indeed detect this file when you run grin-wallet without specifying the top-dir.

./grin-wallet.exe --testnet info In the console wallet path: followed by the path to the wallet is printed to the console. Check if the path is indeed your working directory.

  1. Thank you for testing! Post your results here, do not forget to clean up since you have generated a lot of wallets and config files.

Additional tests:

-You can also test providing paths with backslash instead of front slash, it should work no-matter what you provide. Even mixed front and backslash as path should work. -You can try providing an absolute path instead of a relative path as top-dir, this also works at least on Windows it does.

Anynomouss avatar Sep 18 '25 11:09 Anynomouss

All other issues I reported earlier (https://github.com/mimblewimble/grin-wallet/pull/732#issuecomment-2888892027 and https://github.com/mimblewimble/grin-wallet/pull/732#issuecomment-2901218139) have been fixed.

aglkm avatar Oct 08 '25 12:10 aglkm