JoinMarket-Org / joinmarket-clientserver

Bitcoin CoinJoin implementation with incentive structure to convince people to take part
GNU General Public License v3.0
725 stars 177 forks source link

Effective max mixdepth must be at most 4! #465

Closed johnblanc closed 4 years ago

johnblanc commented 4 years ago

Hey any idea why "python wallet-tool -m 15 wallet" doesn't work any more? It throws out an error that any mixing depth higher than 4 is not allowed and in the docs that is the command recommended for recovering btc spread among depths

Traceback (most recent call last): File "wallet-tool.py", line 13, in jmprint(wallet_tool_main("wallets"), "success") File "/home/user/joinmarket-clientserver-master/jmvenv/lib/python3.7/site-packages/jmclient/wallet_utils.py", line 1240, in wallet_tool_main wallet_password_stdin=options.wallet_password_stdin, gap_limit=options.gaplimit) File "/home/user/joinmarket-clientserver-master/jmvenv/lib/python3.7/site-packages/jmclient/wallet_utils.py", line 1134, in open_test_wallet_maybe return open_wallet(path, mixdepth=max_mixdepth, kwargs) File "/home/user/joinmarket-clientserver-master/jmvenv/lib/python3.7/site-packages/jmclient/wallet_utils.py", line 1180, in open_wallet wallet = wallet_cls(storage, kwargs) File "/home/user/joinmarket-clientserver-master/jmvenv/lib/python3.7/site-packages/jmclient/wallet.py", line 1020, in init super(ImportWalletMixin, self).init(storage, kwargs) File "/home/user/joinmarket-clientserver-master/jmvenv/lib/python3.7/site-packages/jmclient/wallet.py", line 1271, in init super(BIP32Wallet, self).init(storage, kwargs) File "/home/user/joinmarket-clientserver-master/jmvenv/lib/python3.7/site-packages/jmclient/wallet.py", line 324, in init .format(self.max_mixdepth))Exception: Effective max mixdepth must be at most 4!

chris-belcher commented 4 years ago

This is definitely wrong, because if a tumbler user sets more than 3 destinations addresses then the number of mixdepths has to go up too.

For recovering your coins, perhaps try using an older version of joinmarket.

AdamISZ commented 4 years ago

@chris-belcher I wouldn't jump immediately to that conclusion.

Consider: the wallet design now refers to the size of the list of mixdepth indices (index_cache) and if it's being asked to operate in read-only mode, it refuses to display more than that number of mixdepths. The wallet is basically asserting that it will not have coins in mixdepths it hasn't yet touched.

If the tumbler was run beforehand, the list of mixdepths would have been extended and persisted at the time those addresses were first accessed. This would mean that running without specifying a -m value should automatically display the much larger list of mixdepths (if you went up to say 15). But it wouldn't let you choose a randomly large value like -m 20 for the above reason.

This is all as I understand and remember it working, today; but I'll need to review it again. @undeath should be involved in the conversation if he's around.

However if my understanding is correct, we do still have at least one problem in the general case: if someone recovers a wallet on a new machine (i.e. not having a wallet file, only a seedphrase), they lost the mixdepth information; how can they create the new wallet file and force it to have a larger number of mixdepths? Was this corner case missed? I don't know but again I'll investigate.

@johnblanc as I've just said on IRC, please tell us the circumstances regarding version of Joinmarket used, and especially if there was any recovery process involved or whether you were continually running with one wallet file on one Core instance. There is almost certainly a problem here, but we need to work to pin it down exactly.

johnblanc commented 4 years ago

if someone recovers a wallet on a new machine (i.e. not having a wallet file, only a seedphrase), they lost the mixdepth information; how can they create the new wallet file and force it to have a larger number of mixdepths? Was this corner case missed? I don't know but again I'll investigate.

Exactly the case here. Recovering from a seed phrase of few old wallets from various versions. Some of them might have used larger than default mixdepths, i don't remember for sure, but I remember experimenting with more than 3 output addresses.

So I have fresh install of JM now with full bitcoin core and need to go through all of them in a way that 100% ensures there are no BTC stuck among any depth. I remember having left quite few BTC in one of them that were stuck during the tumble and I couldn't be bothered to sweep them at the time, but with the price now it's well worth it.

AdamISZ commented 4 years ago

OK I will analyze that specific case now. If as expected it needs a fix, i will push an update and give you instructions. I'm not addressing the (very old now!) non-segwit wallet case, but I refer you to this specific issue comment for a non-intuitive aspect of how setting segwit=false in the config behaves. If as not expected, it doesn't need a fix and it's just a particular option or action we forgot about, then I'll let you know. More likely as per above, a patch is needed for this recovery case.

johnblanc commented 4 years ago

Regarding to the above, I had to set segwit=false in order to even be able to recover the non-segwit wallets. When importing non-segwit wallet with segwit=true it fails right after typing the mnemonic seed.

AdamISZ commented 4 years ago

Right. So on recovering a wallet with a larger (potential) number of mixdepths, it occurred to me while I was busy with other stuff that we have missed something simple (so "not as expected"!) - you can and should specify the number of mixdepths on wallet creation. I've just tested this out and it seems to work fine.

For example you have seed seed. You do python wallet-tool -m 15 recover then enter seed at prompt, then after creating a new file do python wallet-tool.py newfilename.jmdat (note: no need for - m). This works for me; it shows the full set of mixdepths.

It's in fact the same issue as with segwit=false; since the wallet stores and persists this info (is it segwit, how many mixdepths does it have), you have to specify it at creation, albeit the number of mixdepths can change (and then be persisted again) if you run e.g. tumbler and ask for that.

(Something I'm guessing you're well aware of: You still have the considerable issue that you always have in this very nasty corner case of Joinmarket usage - no wallet file so no index cache information and no imported addresses and very large scale usage (no issue for small wallets really) - can mean in worst case multiple rescans and may need to use -g N where N is large.)

AdamISZ commented 4 years ago

@johnblanc Also, thanks for pointing out the error here on IRC, it'll get fixed as part of closing this issue. More specifically it seems to be this subsection that needs a rewrite.

AdamISZ commented 4 years ago

@johnblanc please let me know if something is still not clear. from my perspective this problem is solved (although I will now update documentation), so I plan to close this.

AdamISZ commented 4 years ago

Doc updated in 936036d1d34be561e39ae83edca46fd86612e329 and closing, but please reopen if needed!

johnblanc commented 4 years ago

Hello Everything is clear and I've been testing in the last few days. The wallet are recovered just fine as you said with -m 15 recover and then I do python wallet-tool -g 1000 walletfile but so far all the wallets are empty.

One thing I noticed is that it does NOT always ask for running bitcoin-qt with -rescan option after recovering and accessing a wallet for first time. If I've just recovered another wallet, rescanned it, and then recovered another one it doesn't ask for -rescan again.

So i'm not sure If i should trust the output of the wallets that have not been rescanned, so right now I'm doing it for each of them just in case.

However, if that fails, what would be other bullet proof way to make 100% sure that everything that could have been generated from a seed (mixdepths and gap) have been checked and is indeed trully empty?

Can I use that seed to recover it in another wallet (like electrum) or the mixdepth architecture makes it impossible for other wallets to see everything?

AdamISZ commented 4 years ago

There are many wallets that support BIP49, which is what we've been using since mid 2017, e.g. Trezor, Electrum, Eclair and several others. Not always do they allow selection of individual accounts, but often they do (Trezor automatically, Electrum you must load each account (=mixdepth) separately).

However none of that will help you with wallets created before the switch to BIP49. They use a custom path that afaik no wallet supports.

You could attempt to create some kind of customised method to query some external blockchain source on a mass of addresses; you could python wallet-tool.py displayall with some huge gap limit (this script also could help if you wanted to generate and read all the private keys over a huge range, without attempting to read it from the blockchain, but I don't think that's needed here since you have access to Core).

But almost by definition it isn't possible to get an absolute proof that no further addresses have been used, if you ditched the wallet file and are just recovering from seed only; and that part is true whatever kind of seed or wallet you're using, including a standard like BIP49, including a more sophisticated wallet.

undeath commented 4 years ago

They use a custom path that afaik no wallet supports.

afaik electrum supports specifying any bip49 path. If not you can still take the xpriv keys from the joinmarket wallet and import those.

AdamISZ commented 4 years ago

Hmm, I think you're right, if memory serves, except you mean BIP32 not BIP49 :) Can't remember if I ever tried the old m/0/ branches on electrum.

johnblanc commented 4 years ago

Ok I'll play with that a little, in the meantime please tell me, should i restart bitcoin-qt with -rescan option each time i do "python walelt-tool.py -g 5000 newwalletfile.jmdat? for a new wallet? most of the time it asks me to, but not always, and im not sure if that might be a reason to not show something.

-rescan takes me several hours and makes experimenting much slower.

undeath commented 4 years ago

except you mean BIP32 not BIP49

yes, thanks :)

For wallet files that have never been used before (have a new seed) you don't need to use -rescan. If you try to recover a previously used wallet/seed you do need to use -rescan.

chris-belcher commented 4 years ago

You should recover and import all those seed phrases, and then run rescan. You'll only need to run rescan once that way.

AdamISZ commented 4 years ago

Very good point, also don't forget you can do bitcoin-cli rescanblockchain X where X is a blockheight to start from, which is quicker in cases you know the wallet was not used before X.