Enhance wallet-tool.py to check correct directory and wallet name or file

[unsystemizer]

Currently wallet-tool.py works well or not well (in 2 different ways) depending where the command is executed (see [1] for three examples) and if it fails, it does so with different errors.

One possible improvement would be to check current directory to ensure it's scripts. Or maybe improve docs to suggest to add scripts to user's path?
For wallet filename, I suppose any valid paths (full, relative, or relative to scripts subdirectory) should ideally work if wallet name or file name (wallet name +.json) are valid.

Ref: [1] JoinMarket-Org/joinmarket#747

[AdamISZ]

Thanks for reopening here.

The overall issue (location of wallet):

The problem I have here is, first, the existing code follows the logic: "look for wallet name provided under wallets/ relative to current working directory" and I wanted to keep the functionality as identical as possible since most people running this code have been running the other repo previously.

Now, while this behaviour is replicated, it also replicates the disadvantages, including the biggest one: people naturally assume that the wallet argument is treated as a path, but it isn't, we had tons of people get confused about that in the past, and they will do here, too, if they're new.

My inclination is to do what I did in https://github.com/AdamISZ/CoinSwapCS which uses this as a dependency: to change the code to store user data in ~/.joinmarket or similar, i.e. wallets/, logs/, cmtdata/ and joinmarket.cfg all going in there. But the reason I haven't done that yet is the above (backwards compatibility).

Further, on to your more specific issue:

Re:
"From scripts with full path to wallet file"

Another thing that makes it even more confusing is specific to testnet: we allow seeds provided on the command line in that case; in the original repo these seeds could be any string at all, this lead to the amusing situation where some people ran python wallet-tool.py wallet.json, and there was no file wallet.json in wallets/, leading the code to create a wallet based on the sha256 of the string "wallet.json" :) Needless to say there is no such "functionality" on mainnet, it's there for convenience of testing.

As I rewrote some sections of the wallet code, I changed it so that that "seed on command line" feature for testnet still works, but only with hex strings (hence the hex exception). I know it's messy but I don't care much as it's only for testnet.

Re:
"From one level below scripts:"
(I'm pretty sure you meant above)

The error here is different but still based on the first mentioned point, that all user level data is currently read from the current working directory. Since you moved up a level, you created a new default joinmarket.cfg in that directory, which has "mainnet" specified in the network, meaning the RPC connection fails.

Hope it makes sense.

[unsystemizer]

Thanks for taking the time to explain.
That makes sense, and until the current approach changes I am tempted to insert some clarifications to certain Wiki pages here, but on the other hand I've seen people complain they have to read too much documentation in order to get started. Maybe a judiciously added paragraph would help beginners figure this out.
If you want to close this, that's okay.

[AdamISZ]

Sure sounds good. If you want to edit wiki pages, feel free, but do ping us say on #joinmarket freenode if you do so we can review the changes.

It's not easy to get started wading through the docs, I agree. A good idea might be to reproduce the walkthrough I did on the Joinmarket-Org/JMBinary repo with screenshots step by step, but that's more suitable for the GUI version. Anyway something along those lines, walkthrough type guide. It's a bit of work of course.

I think I won't close this one for now but mark 'enhancement' as it's something that should probably be fixed in future, just not right now perhaps.

[unsystemizer]

I have a Q: this repo has no wiki and joinmarket is a fork from upstream. Should I submit a change proposal to upstream joinmarket Wiki?

[AdamISZ]

This repo is not technically a fork. It's a complete refactoring from scratch (but still does copy and reuse a lot of code from the original repo in various places).

Joinmarket-Org is the github organisation that hosts joinmarket, owned by me, belcher, adlai. The main joinmarket repo is Joinmarket-Org/joinmarket. This repo is done by me but has not been reviewed by the others, I would like to add it to Joinmarket-Org at some point.

Referring to joinmarket-org/joinmarket's wiki makes sense to me as long as the instructions for running the scripts is the same, would make no sense to try to duplicate. A separate issue is whether instructions should be moved off of a wiki, to another format, I think they should. A wiki is good for general background/technical info, not for instructions I think.

[unsystemizer]

I downloaded and installed "the other" JoinMarket to see understand what it does.
Based on that I removed references to scripts directory and made some other, small changes.
Feel free to edit it directly if you want - I know this isn't the most effective way to to do it.

[AdamISZ]

Thanks, and sorry again for the confusion.

[kristapsk]

Thinking about this now, in a context of my JM guide for RaspiBolt.

RaspiBolt runs on Raspberry Pi where you have internal SD card with a root filesystem and then mounted external USB HDD or SSD with blockchain data and other stuff. There I really want to move wallet files and logs out of SD card to external drive, to have less writes on SD card due to wear out problems of raw NAND flash storage (it was few months, no JM, but one microSD card already died, but it probably was a problem with wrong manufacturer, which I will not mention, but it does other things very good, just not flash storage). This can be accomplished with two symlinks to directories on external drive (which is four commands, because also need to rm these two directories from the release), but it would be alot easier to just do one symlink for ~/.joinmarket/.

Another issue is upgrading, for which I haven't yet wrote tutorial. It will require user to copy joinmarket.cfg and contents wallets/ directory from old to new JM directory every time. Again, can get rid of this with having these things in ~/.joinmarket/.

Also, some time ago I lost some testnet bitcoins, because I removed some development copy of JM and forgot that only copy of that testnet wallet was under it (and who writes down seed phrases for testnet wallets? :) ). So, mixing code and data in the same directory is also probably not a good thing.

Only problem is backwards compatbility and there I see two ways forward: 1) force everything to be in ~/.joinmarket/ with some release and use some migration script, like there was with move from .json to .jmdat wallet files; 2) have some list of priorities, like first treat wallet parameter as an absolute/relative path from cwd, then look into ~/.joinmarket/wallets/, then os.path.join(os.path.dirname(os.path.realpath(__file__)), 'wallets'). And similarly to joinmarket.cfg (apart from absolute/relative path argument, of course). With (2) there could be some potential risks of wrong directory / file being choosen, but it's more easy for users on upgrade from the old to new way.

@AdamISZ , in # 42 #420 you said "I already wrote that code but didn't implement it here to avoid another point of confusion while people were moving to this repo". What is status of this? Is it something that could be used or needs to be re-implemented anyways?

And what are thoughs of others on this?

[AdamISZ]

@AdamISZ , in #42 you said "I already wrote that code but didn't implement it here to avoid another point of confusion while people were moving to this repo". What is status of this? Is it something that could be used or needs to be re-implemented anyways?

It should be done, I think, as per the above comment that mentioned CoinSwapCS, but why did you link to that issue? Looks unconnected?

[kristapsk]

Ohh, sorry, I meant #420, not 42.

[AdamISZ]

Closing after merge of #475