fix standalone private keys encryption and usage #1959
Conversation
OBorce
commented
Aug 19, 2025
OBorce
commented
- fix encryption of the standalone private keys when a new password is created or changed
- fix watching/scanning for relevant public key destinations belonging to standalone keys (only public key has destinations used to work before)
- fix wallet balance to show spendable UTXOs belonging to standalone private keys.
wallet/src/wallet/tests.rs
Outdated
| @@ -298,18 +298,19 @@ fn create_wallet(chain_config: Arc<ChainConfig>) -> DefaultWallet { | |||
| } | |||
|
|
|||
| #[track_caller] | |||
| fn create_block<B, P>( | |||
| fn create_block_with_address_reward<B, P>( | |||
There was a problem hiding this comment.
-
The name looks strange.
create_block_with_reward_addressorcreate_block_with_reward_destinationwould be better. -
It doesn't make sense to return an
Addressfrom this function. Or even constructing it, because the function only needsDestination.
wallet/src/wallet/tests.rs
Outdated
|
|
||
| // success after unlock | ||
| wallet.unlock_wallet(&password.unwrap()).unwrap(); | ||
| if rng.gen::<bool>() { |
There was a problem hiding this comment.
In such cases it's better to add a parameter to the test instead of randomly deciding what to do (because the code paths are quite different)
wallet/src/wallet/tests.rs
Outdated
| wallet | ||
| .add_standalone_private_key(DEFAULT_ACCOUNT_INDEX, standalone_sk, None) | ||
| .unwrap(); |
There was a problem hiding this comment.
For completeness, it's better to check both cases - i.e. both when the key is added before the wallet is encrypted and after that.
(via a test parameter or at least a random bool)
| ) | ||
| .unwrap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Another scenario worth checking is:
- encrypt the wallet.
- add a standalone private key.
- while the wallet is encrypted, encrypt it again with a different password.
- check that that the key works (i.e. it was re-encrypted with the new password).
Probably it's better to make it a separate test though.
wallet/src/wallet/tests.rs
Outdated
| @@ -5363,7 +5493,7 @@ fn test_add_standalone_private_key(#[case] seed: Seed) { | |||
|
|
|||
| // Check amount is still zero | |||
| let coin_balance = get_coin_balance(&wallet); | |||
| assert_eq!(coin_balance, Amount::ZERO); | |||
| assert_eq!(coin_balance, block1_amount); | |||
There was a problem hiding this comment.
The comment "Check amount is still zero" is now wrong.
P.S. the later comment "but the transaction has been added to the wallet" will also have to be changed.
wallet/src/wallet/tests.rs
Outdated
| vec![], | ||
| block1_amount, | ||
| 0, | ||
| Destination::PublicKey(standalone_pk), |
There was a problem hiding this comment.
Perhaps we should try both PublicKey and PublicKeyHash here (via a test parameter or a random bool).
| @@ -1847,7 +1842,7 @@ impl<K: AccountKeyChains> Account<K> { | |||
| Destination::PublicKeyHash(pkh) => { | |||
| self.key_chain.is_public_key_hash_mine_or_watched(*pkh) | |||
| } | |||
| Destination::PublicKey(pk) => self.key_chain.is_public_key_mine(pk), | |||
| Destination::PublicKey(pk) => self.key_chain.is_public_key_mine_or_watched(pk.clone()), | |||
There was a problem hiding this comment.
Btw, I guess there is no test that covers this fix. Can you add one?
wallet/src/wallet/tests.rs
Outdated
| #[rstest] | ||
| #[trace] | ||
| #[case(Seed::from_entropy())] | ||
| fn locked_wallet_standalone_keys(#[case] seed: Seed) { |
There was a problem hiding this comment.
- I'd say the emphasis here is on "standalone_keys" and not "locked_wallet".
- Do we still need
test_add_standalone_private_key? It looks like it does only a subset of what this test does. (I guess the only thing this test doesn't do is check the result ofwallet.get_transaction, but we could add the check). - If it's still needed, let's put the two tests near each other.
