Add WalletWrite::insert_address_with_diversifier_index function
#1151
Conversation
This enables use cases where the diversifier index is prescribed instead of sequentially assigned.
There was a problem hiding this comment.
This is looking good, but we should not hardcode Orchard receivers until Orchard is supported by the rest of zcash_client_sqlite and in general, I think we want to move in the direction of having explicit requests passed in (the current hardcoded default is temporary.)
| /// If the diversifier index cannot produce a valid Sapling address, no sapling receiver will | ||
| /// be included in the returned address. |
There was a problem hiding this comment.
I think that this method should take a UnifiedAddressRequest instead of having this default behavior: https://github.com/zcash/librustzcash/blob/main/zcash_keys/src/keys.rs#L421-L425
There was a problem hiding this comment.
The reason I left it out was that elsewhere, it was used to influence or check the diversifier index would work for the required receivers. In this case, the caller requires the exact index given to be inserted, so compatibility checks are moot. I didn't really need to return a UnifiedAddress from the function either. In my case I drop it on the floor.
My concern with taking UnifiedAddressRequest here is that it complicates the caller, as it now must describe what it expects to be the allowed receivers (and no more than that), when that isn't necessary.
There was a problem hiding this comment.
The caller may be someone like the Brave browser, which will not be supporting the Sapling pool at all. So it must be up to the caller what receivers are generated, and they must handle the error that can occur if the diversifier index does not correspond to an index that is valid for the request they provided.
| _account: AccountId, | ||
| _diversifier_index: DiversifierIndex, | ||
| ) -> Result<UnifiedAddress, Self::Error> { | ||
| todo!() |
There was a problem hiding this comment.
Please either implement this, or add an issue to track the TODO
There was a problem hiding this comment.
Would returning Err(()) count? It's difficult to imagine a better implementation given this mock doesn't store anything at all.
There was a problem hiding this comment.
I was thinking that the mock could store a seed, but just adding an issue for this is fine.
| .. | ||
| }, | ||
| _, | ||
| )) => Ok(addr), // conflicts are ignorable |
There was a problem hiding this comment.
Should we permit conflicts on insertion more generally, and just add an ON CONFLICT DO NOTHING clause to the SQL code in wallet::insert_address?
| .get(&account) | ||
| .ok_or(SqliteClientError::AccountUnknown(account))?; | ||
|
|
||
| let has_orchard = true; |
There was a problem hiding this comment.
This can't be merged as-is, because this will cause Orchard receivers to begin being generated before the wallet has orchard support, which is a "the user can't see their received funds" risk. Instead, either use https://github.com/zcash/librustzcash/blob/main/zcash_client_sqlite/src/lib.rs#L110 or have the request passed in by the caller.
There was a problem hiding this comment.
That potentially presents a problem: if orchard cannot be guaranteed, and sapling may fail at a given diversifier index, that leaves me with just transparent in some cases. I can't construct a UA with just a transparent receiver. Yet the sqlite table I'm adding a row to has an address column which (at least so far) seems to only take UAs. Can I insert a transparent address there, or will code fall over?
There was a problem hiding this comment.
Maybe I should just create a new table to track the transparent addresses. It's not in this PR, but I have a follow-up change in my fork that adds a column to the addresses table to record the last sync'd block for the transparent component of the address. If that isn't tolerable for this crate, then my creating a whole new table to store the data I need (and isn't wanted upstream) may be a better overall approach.
There was a problem hiding this comment.
If a Sapling receiver is requested and no valid diversifier exists at the specified index, that should result in an error being returned; we should never generate an address that does not have all of the requested components. This is another reason to pass in the request from the caller.
There was a problem hiding this comment.
That doesn't really answer my question. I need to be able to store addresses for which no valid sapling receiver exists. Since I can't store orchard receivers in this table without the caller's permission, I have to either store transparent addresses in the address column, or create another table to store this data. Do you have a preference?
There was a problem hiding this comment.
Is the use case you're after here the "transparent address zero" problem, wherein older wallets may have generated a transparent address at index zero but there is no corresponding Sapling address, and so you can't generate a UA to correspond?
That is a use case that we do need to handle, and I'd rather not special-case it, so we could potentially handle it by making the address column nullable, and using the cached_transparent_receiver_address for this purpose. However, for that, you'd need a different entrypoint, as at present you can't create UA requests that don't have a shielded component.
The alternative is that in ZIP 316 Revision 1, we could permit transparent-only UAs. Ultimately this might be what we do.
There was a problem hiding this comment.
No, this isn't directly related to the index 0 problem at all. This is simply about tracking which transparent addresses have been created/used so that sync knows which addresses to download transactions for.
There was a problem hiding this comment.
So up until now, you could obtain this from the transparent parts of UAs saved to the addresses table, plus the 0 address (which I had forgotten I added handling for in #685). Is the thing that is changing is that you intend to generate transparent addresses that are not correlated to any UA? Sorry if I'm being obtuse, but I'm still having a bit of trouble understanding the use case.
There was a problem hiding this comment.
Yes. All my transparent addresses are generated completely independently of any UA. So I need to be able to represent all of them, even if their sapling counterpart is invalid.
This enables use cases where the diversifier index is prescribed instead of sequentially assigned.