feat: symmetric key notifications #169

dan-da · 2024-07-24T17:39:05Z

closes #161.

Implements public announcements encrypted via symmetric keys

This is most useful for utxos that transfer funds within a single wallet such as change addresses.

Compared to on-chain pub-key public announcements:

Data encrypted with symmetric keys is smaller than data encrypted with asymmetric keys so there is a blockchain space (and thus fee) savings.

Compared to off-chain expected utxos:

symmetric-key announcements exist on the blockchain and thus are immune to local data-loss situations. off-chain expected utxos do not use any blockchain space but require that the wallet holder make ongoing backups of wallet state and never lose them.

Design choices, please review:

SymmetricKey presently uses Aes256Gcm. Is this our final choice?
Symmetric keys are derived from a seed and are unique for each utxo. (rather than using a single sym-key per wallet shared for all utxo.) This is because we want the receiver_id to be unique to avoid linking utxos.
Symmetric keys derive from the same root seed WalletSecret::secret_seed as generation addresses but the derivation in ::nth_symmetric_key() uses a SYMMETRIC_KEY_FLAG. This means that end-user should only require a single mnemonic phrase for the root seed. A symmetric-key derivation counter must be stored in wallet state.
efficiency: SymmetricKey presently derives all fields from seed as needed, rather than pre-calc and store when created. see doc-comment for the struct.
change notify method now defaults to on-chain symmetric key, rather than offchain (expected utxo) in dashboard and neptune-cli. (note: this could easily be made an end-user option)

Summary of changes:

Cargo.toml:

aead dep now requires feature std so that the aead::Error impls std::error::Error trait.

New code:

add struct AnnouncedUtxo to represent a found/claimed utxo and its secrets.
add enum UtxoNotifyMethodSpecifier that further simplifies create_transaction() params and impl.
add struct SymmetricKey
add WalletSecret::next_unused_symmetric_key() and ::nth_symmetric_key()
add WalletState::get_known_symmetric_keys()

Modified code:

added SymmetricKey to SpendingKeyType and ReceivingAddressType.
added some missing doc-comments
minor refactors to generation_address to match and share code with symmetric_key.
update UtxoNotificationPool::scan_for_expected_utxos()
update WalletState::scan_for_announced_utxos() to include symmetric keys.
update WalletState::find_spending_key_for_utxo() to include symmetric keys.
update WalletState::update_wallet_state_with_new_block()
update GlobalState::create_transaction(). simplify args and impl.
update rpc_server::send() and send_to_many() to accept change_utxo_notify_method param.
dashboard and neptune-cli now pass OffChainSymmetricKey flag for send() parameter change_notify_method.

New Tests:

symmetric_key::test::test_encrypt_decrypt()
symmetric_key::test::scan_for_announced_utxos_test()

Implements public announcements encrypted via symmetric keys This is most useful for utxos that transfer funds within a single wallet such as change addresses. compared to on-chain pub-key public announcements: Data encrypted with symmetric keys is smaller than data encrypted with asymmetric keys so there is a blockchain space (and thus fee) savings. compared to off-chain expected utxos: symmetric-key announcements exist on the blockchain and thus are immune to local data-loss situations. off-chain expected utxos do not use any blockchain space but require that the wallet holder make ongoing backups of wallet state and never lose them.

aszepieniec

By and large: good stuff, kudos!

Some requests:

Please add a page to the documentation covering symmetric keys for change UTXOs.
Avoid importing tools from generation_address in symmetric_key. Instead, factor them out to a helper module.
UtxoNotifierMethodSpecifier and UtxoNotifierMethod seem similar and I wonder if they cannot be merged.
Consider renaming SpendingKeyType to SpendingKey and generation_address::SpendingKey to generation_address::GenerationSpendingKey.
Please convert the randomized tests in symmetric_key.rs into proptests. Have a look in primitive_witness.rs if you need something to pattern match on. (I realize there are plenty of tests out there that are randomized with thread_rng but I should like to convert those to proptests at some point. Let's not add to that conversion workload.)
Please add an integration test (not sure that's the right word) whereby you spawn two global states, one of which is a premine recipient whose sends some money to the other node and gets some change, and verify that the change is being observed and recovered correctly, even if the node in question reboots and loses everything but the seed phrase.

aszepieniec · 2024-07-25T12:58:21Z

src/models/state/wallet/address/address_type.rs

        }
    }

    /// encodes this address as bech32m
    pub fn to_bech32m(&self, network: Network) -> Result<String> {
        match self {
            Self::Generation(k) => k.to_bech32m(network),
+            Self::Symmetric(_k) => unimplemented!(),


How about adding a text string explaining why it doesn't make any sense?

The reason why we need bech32m encoding (or something similar) for non-change payment addresses is because those addresses are transmitted off-band and need to be encoded for that purpose, preferably in a human-verifiable way. It doesn't make any sense for change "addresses" because there is no need for transmission from one party to another.

yep yep. I will add a comment.

aszepieniec · 2024-07-25T13:04:51Z

src/models/state/wallet/address/generation_address.rs

-                    Err(_) => false,
-                }
+
+            // ... that are marked as symmetric key encrypted


Suggested change

// ... that are marked as symmetric key encrypted

// ... that are marked as generation address ciphertext

aszepieniec · 2024-07-25T13:10:07Z

src/models/state/wallet/address/symmetric_key.rs

+        generation_address::shake256::<32>(
+            &bincode::serialize(&self.seed).expect("serialization should always succeed"),
+        )


If the shake256 convenience wrapper is useful, it should probably be defined elsewhere. Probably in twenty-first. Importing it from generation_address is bad form.

it does exist in twenty-first, but is private for some reason. There is actually a comment about that in generation_address::shake256, that it was copied from twenty-first for that reason. I can make it pub in twenty-first, but that change might take a while to make it into neptune-core, so for now I guess I will put it into a shared module, with a comment to use twenty-first version when available.

aszepieniec · 2024-07-25T13:12:04Z

src/models/state/wallet/address/symmetric_key.rs

+
+    /// returns the receiver_identifier, a public fingerprint
+    pub fn receiver_identifier(&self) -> BFieldElement {
+        generation_address::derive_receiver_id(self.seed)


I don't think it's a good idea to import things from generation_address. What if we should decide to get rid of that? IMO, symmetric_key should stand on its own.

ok, I can refactor common code out into a shared module.

aszepieniec · 2024-07-25T13:23:25Z

src/models/state/wallet/address/symmetric_key.rs

+    #[test]
+    fn test_encrypt_decrypt() {
+        let mut rng = thread_rng();
+
+        // 1. generate key from random seed.
+        let symmetric_key = SymmetricKey::from_seed(rng.gen());
+
+        // 2. create utxo with random amount
+        let amount = NeptuneCoins::new(rng.gen_range(0..42000000));
+        let utxo = Utxo::new_native_coin(symmetric_key.lock_script(), amount);
+
+        // 3. generate sender randomness
+        let sender_randomness: Digest = rng.gen();
+
+        // 4. encrypt secrets (utxo, sender_randomness)
+        let ciphertext = symmetric_key.encrypt(&utxo, sender_randomness).unwrap();
+        println!("ciphertext.get_size() = {}", ciphertext.len() * 8);
+
+        // 5. decrypt secrets
+        let (utxo_again, sender_randomness_again) = symmetric_key.decrypt(&ciphertext).unwrap();
+
+        // 6. verify that decrypted secrets match original secrets
+        assert_eq!(utxo, utxo_again);
+        assert_eq!(sender_randomness, sender_randomness_again);
+    }


Consider converting this test to a proptest.

ok. It seems it would mainly test the (3rd party) implementation of Aes256. Is that the intent?

dan-da · 2024-07-30T19:39:05Z

closing in favor of #171.

dan-da requested review from aszepieniec and Sword-Smith July 24, 2024 17:39

aszepieniec requested changes Jul 25, 2024

View reviewed changes

dan-da mentioned this pull request Jul 29, 2024

161 symmetric key pr2 #171

Merged

dan-da closed this Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: symmetric key notifications #169

feat: symmetric key notifications #169

dan-da commented Jul 24, 2024 •

edited

Loading

aszepieniec left a comment

aszepieniec Jul 25, 2024

aszepieniec Jul 25, 2024

dan-da Jul 25, 2024

aszepieniec Jul 25, 2024

aszepieniec Jul 25, 2024

dan-da Jul 25, 2024

aszepieniec Jul 25, 2024

dan-da Jul 25, 2024

aszepieniec Jul 25, 2024

dan-da Jul 25, 2024

dan-da commented Jul 30, 2024

	// ... that are marked as symmetric key encrypted
	// ... that are marked as generation address ciphertext

feat: symmetric key notifications #169

feat: symmetric key notifications #169

Conversation

dan-da commented Jul 24, 2024 • edited Loading

Compared to on-chain pub-key public announcements:

Compared to off-chain expected utxos:

Design choices, please review:

Summary of changes:

aszepieniec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-da commented Jul 30, 2024

dan-da commented Jul 24, 2024 •

edited

Loading