Remove the spending PIN encryption before backup #1133
Conversation
iSavicx
changed the title
Co-authored-by: Sebastian Nicol <sebastian.r.nicol@gmail.com> Co-authored-by: Oliver Aemmer <oliver.aemmer@protonmail.ch>
iSavicx
force-pushed
the
simplified-backup
branch
from
12cdca2
to
d3035a3
Compare
|
Thanks for picking this up! I took a quick look. I think there is a conceptual issue with PIN-decrypting/re-encrypting the wallet. Background: Ideally, the wallet in use would not be PIN-decrypted at all, only the wallet that is about to be backed up. However, for this we'd need the ability to clone a wallet in-memory, which I think we currently don't have any API for in bitcoinj. However what we could do as a workaround is serialize the wallet to protobuf, deserialize it to a different Wallet object (which at that point should be an independant clone), remove the PIN-encryption on that clone, then serialize to protobuf again for backup. No re-encryption necessary, the clone would be collected by the GC. This seems a bit laborious, but I think it's currently the safest way. What do you think about this approach? |
… then backup Co-authored-by: Sebastian Nicol <sebastian.r.nicol@gmail.com> Co-authored-by: Oliver Aemmer <oliver.aemmer@protonmail.ch>
|
@schildbach Thank you for taking a look at the proposed changes.
You're right. With your proposed solution, one would not have to worry about potential unwanted encryption state changes on the live wallet due to the backup failing for whatever reason. Furthermore, this approach requires fewer changes to the existing code and is also faster because it only needs to derive the key once and does not require re-encrypting the wallet at the end. We have added a new commit to this PR with an implementation of the suggested changes. The original commit is not really relevant anymore and could be squashed, but we left it for the time being for transparency reasons. |
| private Button positiveButton, negativeButton; | ||
|
|
||
| private Executor executor = Executors.newSingleThreadExecutor(); | ||
| private Wallet backupWallet; |
There was a problem hiding this comment.
I think the (decrypted) backupWallet should be kept in a MutableLiveData<Wallet> in BackupWalletViewModel, rather than here where it can be collected at any time (e.g. while you're picking the file).
And I'd use the name walletToBackUp, maybe even walletToBeBackedUp or decryptedWalletToBeBackedUp.
| @@ -111,6 +118,7 @@ public void onChanged(final Wallet wallet) { | |||
| final String targetProvider = WalletUtils.uriToProvider(uri); | |||
| final String password = passwordView.getText().toString().trim(); | |||
| checkState(!password.isEmpty()); | |||
| checkState(backupWallet != null); | |||
There was a problem hiding this comment.
In line 113 above, you'd observe that walletToBackUp which is then guaranteed to be non-null so you can remove this assert.
| setState(BackupWalletViewModel.State.EXPORTING); | ||
| backupWallet = walletActivityViewModel.wallet.getValue(); | ||
| backupWallet(); | ||
| backupWallet = null; |
There was a problem hiding this comment.
Not sure if the removal of the reference to the decrypted wallet is needed in general. We need to trust the garbage collector anyway. If you prefer to keep it, I'd move it towards the end of the CreateDocument callback (around line 171) because that's where we're done with it.
…ckup Change walletToBackup to MutableLiveData
This pull request modifies the backup process for the Bitcoin Wallet by introducing a prompt for the spending PIN if one is set. The backup file will now be encrypted only by the key derived from the backup password, avoiding double encryption. Additionally, users now only need to remember the backup password instead of both the password and spending PIN when importing and using the wallet later.
This change addresses feedback from another open pull request.
While the changes have been tested on Android 8.0 (API level 26) and Android 14 (API level 34), a thorough review is necessary due to the criticality of the encryption state.
Authors: Sebastian Nicol and Oliver Aemmer