rotki / rotki

A portfolio tracking, analytics, accounting and management application that protects your privacy
https://rotki.com
GNU Affero General Public License v3.0
2.83k stars 522 forks source link

Cannot delete an XPUB key that has no coins #1558

Closed coblee closed 3 years ago

coblee commented 4 years ago

Problem Definition

After adding an XPUB key that has balance on its derived address, I can not delete that XPUB from my database anymore. I assume this is because the XPUB address does not show up in the list BTC per account balances since there's no balance.

System Description

Here add a detailed description of your system, e.g. output of the following script:

uname -a
command -v geth && geth version
command -v parity && parity --version
command -v rotki && rotki version
[ -d .git ] && git rev-parse HEAD

Darwin ChocoBookPro.local 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64

rotki is version 18.1

LefterisJP commented 4 years ago

Hey @coblee let me see if I understand. You added an account xpub. Did you see any addresses or not? If not then are you sure you added an account xpub? This is the only xpubs we can derive from at the moment. For example if you take the xpub of an electrum wallet the addresses will be properly derived.

And your problem is that with an xpub that has never had any derived addresses, it does not show in the UI but is still in the DB and you can't delete it from there. Right?

That last part is indeed a bug. We should handle it.

coblee commented 4 years ago

It is indeed an account xpub. The issue is that my derivation path is m/49'/0'/0', but I added it with no derivation path specified. I assume because of that it probably defaulted to m/44 or something. So it found no coins.

Afterwards, I tried adding the same xpub with the m/49'/0'/0' derivation path, but that failed also. I think that's another bug, but I can't be sure until I can delete this entry.

LefterisJP commented 4 years ago

This is our first attempt at derivation of addresses from xpubs so it's still experimental but in its basic form it should work. We tried it with 2 electrum wallets and the test xpub from ledger for its accounts: xpub6DCi5iJ57ZPd5qPzvTm5hUt6X23TJdh9H4NjNsNbt7t7UuTMJfawQWsdWRFhfLwkiMkB1rQ4ZJWLB9YBnzR7kbs9N8b2PsKZgKUHQm1X4or https://github.com/LedgerHQ/bitcoin-keychain-svc/blob/744736af1819cdab0a46ea7faf834008aeade6b1/integration/p2pkh_keychain_test.go#L40-L95

The intended function of the xpub derivation for now is for simple usage. Just a simple account xpub. You can also specify a derivation path that will lead from the given xpub to the final account xpub. That's why we have the extra derivation path now.

Once we get the account xpub we start the derivation in the way most BTC wallets do it.

So we do (pseudocode):

receiving_xpub = account_xpub.derive_child(0)
addresses.extend(derived_used_addresses(receiving_xpub))
change_xpub = account_xpub.derive_child(1)
addresses.extend(derived_used_addresses(change_xpub))
LefterisJP commented 4 years ago

@coblee I suspect what you are experiencing is due to our current implementation. It has some limitations which are explained here: https://github.com/rotki/rotki/issues/1562. We only support legacy xpubs and native segwit ones starting with zpub at the moment.

(Bitcoin side of things is still new to me)

Next release by implementing what #1562 says I hope to provide a more complete functionality. As a more seasoned dev in the bitcoin side of things can you check if that would make sense?

coblee commented 4 years ago

Yup, that must be it. I don't know BIP44/BIP49 well, but yes, my addresses are P2SH-P2WPKH and derivation path is m/49'/0'/0'.

Anyways, this bug is about deleting an existing XPUB account. I will track the other bug separately.