search

hhanh00 / zwallet

Zcash and Ycash light shielded wallet
https://ywallet.app
MIT License
42 stars 20 forks source link

Receiving address may contain a mix of receivers from multiple accounts #158

Closed AArnott closed 2 months ago

AArnott commented 2 months ago

Subject of the issue

YWallet shows a unified receiving address containing sapling and orchard receivers from one account and a transparent receiver from a different account.

Your environment

1.6.1+538/f2970c23 Android

Steps to reproduce

I expect this will repro the problem based on the steps I took. But I didn't try them on a fresh install myself.

  1. Create a new account with a new seed phrase. This will use account index 0.
  2. Create a new account, using the import feature and the same seed phrase used by the first account. Set the account index to 3.

With the new account selected, copy the default address shown by the home screen, which is a unified address including all receivers.

Expected behaviour

All receivers direct money into the new account (with account index 3).

Actual behaviour

The orchard and sapling receivers are derived from account index 3. The transparent receiver is derived from the original account (account index 0) using address index 3.

As a result, ZEC sent to the unified address will arrive in account index 3 if sent to the shielded receivers, or account index 0 if sent to the transparent receiver.

hhanh00 commented 2 months ago

We inherited this weird behavior from zec wallet lite. The extra transparent addresses increment address index, whereas extra shielded addresses increment account index.

AArnott commented 2 months ago

Does it only impact the displayed receiving address, or does sync download transactions from the wrong transparent addresses as well?

hhanh00 commented 2 months ago

It downloads from the address displayed. You call it wrong because it is not what you expect or because of some zip?

On Mon, Jun 24, 2024, 1:16 AM Andrew Arnott @.***> wrote:

Does it only impact the displayed receiving address, or does sync download transactions from the wrong transparent addresses as well?

— Reply to this email directly, view it on GitHub https://github.com/hhanh00/zwallet/issues/158#issuecomment-2185037058, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABWIH6OIOUNXI2LV4SENKTZI3RDJAVCNFSM6AAAAABJYM3MKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBVGAZTOMBVHA . You are receiving this because you commented.Message ID: @.***>

AArnott commented 2 months ago

Both my expectation and the ZIPs.

ZIP-32 states that it builds on top of BIP-44 which specifies the derivation path for transparent addresses to be m / purpose' / coin_type' / account' / change / address_index. ZIP-32 re-states account index as in the same derivation position that BIP-44 specifies. ZIP-32's Motivation section also calls out how the intent is to have logical "accounts" from which all keys are derived.

It is also the way Bitcoin wallets operate: one can create multiple accounts with a single master seed, and then within that account can create many addresses.

YWallet however is splintering the concept of account by creating and joining addresses from across accounts. It deviates from every other wallet I've seen that supports more than one account (ZECWallet excepted, apparently).

hhanh00 commented 2 months ago

Backward compatibility was deemed more important especially since at that time UA didn't even exist. Now we cannot change the derivation path without forcing users to move their funds.

It is just gonna be a known quirk.

On Mon, Jun 24, 2024, 3:43 PM Andrew Arnott @.***> wrote:

Both my expectation and the ZIPs.

ZIP-32 https://zips.z.cash/zip-0032 states that it builds on top of BIP-44 which specifies the derivation path for transparent addresses https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#user-content-Path_levels to be m / purpose' / coin_type' / account' / change / address_index. ZIP-32 re-states account index as in the same derivation position that BIP-44 specifies. ZIP-32's Motivation section also calls out how the intent is to have logical "accounts" from which all keys are derived.

It is also the way Bitcoin wallets operate: one can create multiple accounts with a single master seed, and then within that account can create many addresses.

YWallet however is splintering the concept of account by creating and joining addresses from across accounts. It deviates from every other wallet I've seen that supports more than one account (ZECWallet excepted, apparently).

— Reply to this email directly, view it on GitHub https://github.com/hhanh00/zwallet/issues/158#issuecomment-2186492086, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABWIHYJPAJPVEPEVI3ROF3ZJAH7BAVCNFSM6AAAAABJYM3MKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBWGQ4TEMBYGY . You are receiving this because you commented.Message ID: @.***>

AArnott commented 2 months ago

Yes, I can appreciate the difficult position you're in. You mentioned it as "a known quirk", but who knows it? It seems safe enough to be unknown if a user never uses a seed phrase across wallets, but we know users do actually do that. Can this quirk be documented prominently enough (perhaps on the Import Seed screen) so users are aware?

hhanh00 commented 2 months ago

My mistake was to call the field "account index", "address index" could have been preferable but I thought some may think it is the address diversifier index.

On Mon, Jun 24, 2024, 8:13 PM Andrew Arnott @.***> wrote:

Yes, I can appreciate the difficult position you're in. You mentioned it as "a known quirk", but who knows it? It seems safe enough to be unknown if a user never uses a seed phrase across wallets, but we know users do actually do that. Can this quirk be documented prominently enough (perhaps on the Import Seed screen) so users are aware?

— Reply to this email directly, view it on GitHub https://github.com/hhanh00/zwallet/issues/158#issuecomment-2187138768, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABWIH3P5ONRR7IPVATV7BDZJBOURAVCNFSM6AAAAABJYM3MKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXGEZTQNZWHA . You are receiving this because you commented.Message ID: @.***>

AArnott commented 2 months ago

Well, you are using the value for account index for the shielded pools, so "address index" would have been misleading there too.

hhanh00 commented 2 months ago

There is no address index in ZIP-32, so the account index could be considered an address index. Anyway, it is a legacy now.

On Tue, Jun 25, 2024 at 3:55 AM Andrew Arnott @.***> wrote:

Well, you are using the value for account index for the shielded pools, so "address index" would have been misleading there too.

— Reply to this email directly, view it on GitHub https://github.com/hhanh00/zwallet/issues/158#issuecomment-2187782380, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABWIH3W2NPIWSD2N2NDWK3ZJDE2HAVCNFSM6AAAAABJYM3MKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXG44DEMZYGA . You are receiving this because you commented.Message ID: @.***>

AArnott commented 2 months ago

"address index" may not be mentioned in ZIP-32, but ZIP-32 explicitly builds on BIP-44, where address indexes are a thing, so address index is still a term that applies in Zcash, if only to transparent addresses. And as "account index" is also a defined term, conflating the two could only be confusing and misleading IMO.