bitcoindevkit
Reintroduce descriptor data to `bdk::Wallet` persistence
Describe the enhancement
The current stable version of BDK stores the descriptor checksum in the database. This ensures the caller does not use the wrong descriptor with the database. We should reintroduce this.
Implementation proposal
Add verify_checksum method to the Persist trait.
This is a good point. I seem to recall that things turned out this way because I didn't even plan on having any keychain data in the database in general and would leave that up to the application. This changed in order to simplify the lives of Wallet users.
Why don't we just persist the public descriptor in the database? We could even have this as part of the changeset for KeychainTxOutIndex. This would make sense since inserting a descriptor is a change you are making to the thing. This would remove the error case where you pass in a descriptor that doesn't match something in the database. Instead you just load from the database. New keychain descriptors can naturally be added later.
I seem to remember @afilini mentioning that it was to maintain some kind of forward privacy against an attacker who can read your database at some point (and then loses access). This would prevent them from learning the full descriptor at that point. I think this is a very weak guarantee:
- You get to see all the spks at one point which lets you follow those coins forward. Using common input ownership heuristic and change heuristic you get a good idea about all future coins ownership.
- The material that produces the descriptor is usually on disk anyway. If you have access to the database you probably got access to that as well.
If you want strong privacy against a database compromiser then there are two better choices:
1. Don't store the data at all and just sync each time i.e. not persistence
2. Encrypt the data using some secret. If you have a private descriptor (with secrets) then this is an obvious choice to derive a key from it and encrypt the database. A key derived from an application password would be good to. We could make this quite easy without much effort for bdk_file_persist.
Had a little discussion about this with @evanlinjin and rather than persist the full descriptors I like the idea of offering a different trait for storing the descriptors, in case the wallet user wants to store those in a different more secure way. I still think we need the verify_checksum or should it be verify_descriptor_checksums? since we're confirming the persisted wallet data is for the right set of descriptors, currently 1 or 2, could be more in the future.
- What's the need for a trait for this? Aren't you suggesting not storing descriptors and just letting the application provide them when creating the keychain txout index.
- I wouldn't use the checksum unless there is some other concern I'm not aware of -- checksums are for error catching not for equality checking which is what we are doing here. Just sha256 hash the descriptor.
- I don't think there's a need for the user to call a method to check this. Just add the descriptor hashes to the changesets of
KeychainTxOutIndexand index the last revealed by the hash of the descriptor. This removes any error cases.
I would still just store the public descriptor in there. You mentioned storing them in a different "more secure" way but did not make the case for doing that if the entire wallet history is stored in the persistence anyway. Is there really context where users don't care about an attacker knowing their wallet history up until the point the attacker compromised it but care deeply about the future of it after that event? That doesn't make sense to me. I think we should guide application developers in the right direction here which is to encrypt history and descriptors together.
I've assigned myself to this issue as it's one of the priority tickets for the next release.
I agree with @LLFourn's points, and I think the best way to implement it is what @LLFourn suggested:
Why don't we just persist the public descriptor in the database? We could even have this as part of the changeset for KeychainTxOutIndex. This would make sense since inserting a descriptor is a change you are making to the thing. This would remove the error case where you pass in a descriptor that doesn't match something in the database. Instead you just load from the database. New keychain descriptors can naturally be added later.
@notmandatory Would you be okay with this, or is there something we haven't considered?
@notmandatory Would you be okay with this, or is there something we haven't considered?
~~The reason I don't want to store the full descriptors in the wallet database is that they are privacy sensitive information that the wallet developer should have the option to persist more securely than the rest of the wallet historical chain data. I don't care if we persist the hashes of the descriptors (instead of their checksums) to validate the wallet chain data matches the given wallet descriptors. I even think it's fine to give the users the option to persist also the full descriptors with the chain data as long as they also have the option to store the descriptors separately.~~
I re-read above comments and I'm on-board with storing the descriptors in the wallet data and if the users is concerned about forward privacy then we should also have some example for how to encrypt all persisted wallet data.
@thunderbiscuit and @reez you guys should think about the best way to use android/ios features to encrypt persisted wallet data.
I agree with this. In most cases for production software you'd want to encrypt all persisted wallet-related data IMO, so having the public descriptors in there is totally fine.
