Currently we store unencrypted private keys in a key record, and encrypted private keys in a ckey record. During encryption, all key records are deleted and the keys encrypted and written into new ckey records. However database systems and filesystems do not always remove data when it is deleted, many simply mark the data as deleted for it to be overwritten later. In databases like BDB, some deleted records are kept around to help keep the structure of the b-tree. This fact has resulted in a notable issue in the original wallet encryption implementation where the unencrypted private keys were kept in the wallet.dat file and in the database transaction logs after the encryption was done. If/when we move to a new storage method, this same issue will likely crop up again and another workaround developed for that specific storage method. However modifying records is usually done in place and overwrites existing records.
I would like to suggest a different way that this could be handled that is independent of the storage method. Instead of storing private keys unencrypted, we can always encrypt the private keys using either a default passphrase or a passphrase that is stored in plaintext in the wallet file. So all private keys would be stored in ckey records. "Unencrypted" then means that the passphrase is either publicly known (a default passphrase) or easily found within the wallet.dat file itself. Encrypting the wallet would then be our typical passphrase changing mechanism.
Specifically, each ckey is encrypted with a randomly generated CMasterKey. The CMasterKey itself is encrypted with a passphrase. An "unencrypted" wallet encrypts the CMasterKey with some default passphrase. During encryption, as we do now for passphrase changing, the CMasterKey is decrypted in memory and encrypted with the new passphrase. The original CMasterKey record is overwritten. Of course we would still regenerate the keypool and set a new HD seed during this process.
Alternatively, instead of using a default passphrase, a randomly generated passphrase can be used to encrypt the CMasterKey. This randomly generated passphrase can be stored on disk in a new record. When the wallet is actually encrypted by the user, a new CMasterKey can be generated and each key re-encrypted with the new CMasterKey along with the CMasterKey itself being encrypted with the new passphrase. Then the passphrase record can be deleted. Since all of these modifications are updates to existing records, there shouldn't be anything that could remain in slack space that reveals the private keys. By rotating the CMasterKey, the old plaintext passphrase and its CMasterKey are not useful even if they are left behind due to deletion not always deleting. This approach could also be used with a default passphrase too.
There are a few benefits from doing this: we avoid having to ensure that unencrypted private keys never get left behind when encrypting, and ckey records are way smaller than key records so we get the space savings too. Some downsides are that it is possible that this could make wallet recovery much more difficult. If a wallet becomes corrupted and the CMasterKey encrypting the keys is lost, the keys would be lost too. But perhaps that could be resolved by using a default CMasterKey instead of a default passphrase.
Thoughts?
