[token-2022] Add elgamal registry account #7341

samkim-crypto · 2024-10-08T21:24:58Z

Problem

Currently, it is not possible to use associated token accounts for confidential transfers because in order to initialize a confidential transfer account, the owner must initialize it with an ElGamal public key.

It would be nice to have a special program "registry" program where a user can store an ElGamal public key that it can use for any mint with confidential transfers enabled.

Summary of Changes

I created a simple ElGamal registry program where a user can store its ElGamal public key in a PDA account that is derived from the user's wallet address - ba60c3d. The program has two instructions. The CreateRegistry instruction derives the PDA, invokes the system program to create the account, and then initializes the registry account. The UpdateRegistry instruction checks the owner signature and updates the old ElGamal key with a new key. I believe the UpdateRegistry instruction does not need to check the PDA address, but please correct me if I am overlooking something 🙏 .
- In the process, I refactored the proof extraction logic that lived in program-2022 to confidential-transfer-proof-extraction module since this will be shared by both program-2022 and ElGamal registry program - eef0666
I added a new ConfigureAccountWithRegistry instruction in the confidential transfer extension which is identical to the ConfigureAccount instruction, but skips the signature and zk proof verification - 3104459. I believe the token program itself just need to check the provided registry account is owned by the registry program and does not need to necessarily check the validity of the PDA address, but please correct me if I am overlooking something 🙏 . Another thing to note is the fact that on ConfigureAccountWithRegistry instruction, the program initializes the confidential transfer account with default maximum pending credits counter and also AeCiphertext::default() (all zero ciphertext). Unlike ElGamal ciphertexts, the all-zero symmetric ciphertext is not really valid, but I added this logic to simplify the program at the cost of increasing the complexity slightly on the client side (client can just interpret all zero ciphertext as encrypting 0). An alternative would be to add a valid zero-ciphertext field to the ElGamal registry account, but I thought this would make things a bit more complicated.
Added support for ConfigureAccountWithRegistry instruction in the token-client - 1a8dc29
Added tests to program-2022-test - 4862464

joncinque

Looks pretty good overall! Mostly small things, let me know what you think

Cargo.lock

token/confidential-transfer/proof-extraction/Cargo.toml

token/confidential-transfer/elgamal-registry/Cargo.toml

token/confidential-transfer/elgamal-registry/src/instruction.rs

token/program-2022/src/extension/confidential_transfer/instruction.rs

token/confidential-transfer/elgamal-registry/src/processor.rs

token/confidential-transfer/proof-extraction/src/lib.rs

joncinque · 2024-10-18T21:45:01Z

token/program-2022-test/tests/confidential_transfer.rs

+    // configure account using ElGamal registry
+    let alice_account_keypair = Keypair::new();
+    let alice_token_account = alice_account_keypair.pubkey();
+    token
+        .create_auxiliary_token_account_with_extension_space(
+            &alice_account_keypair,
+            &alice_token_account,
+            vec![ExtensionType::ConfidentialTransferAccount],
+        )
+        .await
+        .unwrap();


Oh gosh, this makes me realize something we haven't thought about yet. Should we make reallocate on token-2022 permissionless? Otherwise, it's not possible to configure someone else's account.

If we want to go with the safer change, we can have configure_with_registry optionally reallocate the account.

Oh yeah completely missed that. It seems that if all accounts are to be rent-exempt, then there seems to be not much harm in making it permissionless? I'll think about it today and let's discuss tomorrow.

There is one slightly strange attack -- if we're competing in a trading environment, I could reallocate your token account to be as big as possible, which would count against the loaded data costs of your transaction, and try to make your transaction fail.

I'd probably go with special-casing this behavior and only reallocating for the confidential token account extension to be totally safe, but maybe I'm being overly cautious.

Oh yes I forgot that there was a MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES field. Yeah in this case, it seems safest to just make reallocation be permissionless for confidential extensions.

Okay, I added the option to reallocate when configuring confidential transfer accounts with registry.

token/program-2022/src/extension/confidential_transfer/processor.rs

joncinque · 2024-10-22T11:08:44Z

rust-toolchain.toml

@@ -1,2 +1,2 @@
 [toolchain]
-channel = "1.78.0"
+channel = "1.79.0"


Is this bumped intentionally?

Oops, that accidentally got in. I will revert.

token/program-2022/src/extension/confidential_transfer/processor.rs

joncinque · 2024-10-22T11:27:14Z

token/confidential-transfer/elgamal-registry/src/instruction.rs

+pub enum RegistryInstruction {
+    /// Initialize an ElGamal public key registry.
+    ///
+    /// 0. `[writable, signer]` The funding account (must be a system account)


I think the other part of my comment got lost under the typo suggestion -- could we remove this and have the client function automatically transfer the rent-exempt lamports into the account so that the user does not need to pass a mutable signer to the program?

Okay, I updated the program so that the registry program just allocates and assigns ownership of the PDA account. Please take a look!

joncinque

Looking really close! Sorry, I did one last pass over the whole thing and found a couple more bits

token/program-2022/src/extension/confidential_transfer/instruction.rs

joncinque · 2024-10-23T11:41:10Z

token/program-2022/src/extension/confidential_transfer/instruction.rs

+/// Data expected by
+/// `ConfidentialTransferInstruction::ConfigureAccountWithRegistry`
+#[cfg_attr(feature = "serde-traits", derive(Serialize, Deserialize))]
+#[derive(Clone, Copy, Debug, PartialEq, Pod, Zeroable)]
+#[repr(C)]
+pub struct ConfigureAccountWithRegistryInstructionData {
+    /// Reallocate token account if it is not large enough for the
+    /// `ConfidentialTransfer` extension.
+    pub reallocate_account: PodBool,
+}


We can probably use the presence of the payer and system program to signal that the caller is ok with doing a reallocation, making the instruction data here unnecessary. What do you think?

Yeah that is a good idea! I made the instruction constructor logic this way, but I missed having that logic in the program.

joncinque · 2024-10-23T11:45:54Z

token/confidential-transfer/elgamal-registry/src/processor.rs

+
+    let elgamal_registry_account_seeds: &[&[_]] = &[
+        REGISTRY_ADDRESS_SEED,
+        &wallet_account_info.key.to_bytes(),


nit: to_bytes() creates a new array of 32 bytes, so it's better to use as_ref()

Good point!

token/confidential-transfer/elgamal-registry/src/processor.rs

joncinque · 2024-10-23T11:53:43Z

token/program-2022/src/extension/confidential_transfer/processor.rs

+    current_extension_types.push(ExtensionType::ConfidentialTransferAccount);
+    let needed_account_len =
+        ExtensionType::try_calculate_account_len::<Account>(&current_extension_types)?;


nit: can you add a comment saying that ExtensionType::try_calculate_account_len dedupes extension types, making this correct?

joncinque · 2024-10-23T11:56:08Z

token/program-2022/src/extension/confidential_transfer/processor.rs

+
+    // reallocate
+    msg!(
+        "account needs realloc, +{:?} bytes",


nit: is there a reason to use the Debug format string of {:?} instead of the normal one of {}? If not, let's go with the normal formatting

I modeled the function over the reallocate processor logic and used debug mode for consistency with that function.

Btw, I considered refactoring the process_reallocate logic into sub-functions and recycling the logic for configuring confidential transfers, but I thought that would make things less readable. I can do the refactoring if you think that would be cleaner.

joncinque · 2024-10-23T12:02:33Z

token/program-2022/src/extension/confidential_transfer/processor.rs

+) -> ProgramResult {
+    let mut current_extension_types = {
+        let token_account = token_account_info.data.borrow();
+        let account = StateWithExtensions::<Account>::unpack(&token_account)?;


nit: can you use PodStateWithExtensions::<PodAccount> instead for better CU usage?

joncinque · 2024-10-23T12:04:15Z

token/program-2022/src/extension/confidential_transfer/processor.rs

+    if reallocate {
+        let payer_info = next_account_info(account_info_iter)?;
+        let system_program_info = next_account_info(account_info_iter)?;
+        reallocate_for_configure_account_with_registry(
+            token_account_info,
+            payer_info,
+            system_program_info,
+        )?;
+    }


We can maybe make this processing logic more clever. Rather than always trying pull out the payer / system program info and then deciding to reallocate, the instruction can always try to reallocate, and only if extra lamports are needed, try to extract the payer and system program. What do you think?

I thought about this as well since that logic will be cleaner. The reason why I went with the current implementation is that if the user does not want to reallocate the account, then we would be consuming unnecessary compute units by unpacking the account and calculating the needed account length. What do you think?

Hm, that's a good point on the CUs... could you do a little test to see how many CUs the extra logic takes up? I'm hoping it's not too many, but there is a vector allocation, so who knows

…er-proof-extraction`

Co-authored-by: Jon C <[email protected]>

joncinque

Looks good to me, thanks! We can do the CU calculation with a separate piece of work, this can go in first

samkim-crypto force-pushed the elgamal-key branch 5 times, most recently from 2049427 to d5890aa Compare October 15, 2024 09:49

samkim-crypto force-pushed the elgamal-key branch from d5890aa to 4862464 Compare October 17, 2024 09:05

samkim-crypto marked this pull request as ready for review October 17, 2024 09:43

samkim-crypto requested a review from joncinque October 17, 2024 09:43

joncinque reviewed Oct 18, 2024

View reviewed changes

joncinque reviewed Oct 22, 2024

View reviewed changes

samkim-crypto force-pushed the elgamal-key branch 3 times, most recently from b96645c to 8ea238e Compare October 23, 2024 09:32

joncinque reviewed Oct 23, 2024

View reviewed changes

samkim-crypto and others added 15 commits October 24, 2024 17:58

refactor proof extraction logic into a `spl-token-confidential-transf…

3d0cca2

…er-proof-extraction`

create ElGamal registry program

e41f90d

add support for ElGamal registry program in the token program

421cefa

add support for ConfigureAccountWithRegistry in the token-client

8a11d6e

add tests for configure account with registry

7ee89e2

use local spl-pod for dependency

84cf6b6

Apply suggestions from code review

69001ec

Co-authored-by: Jon C <[email protected]>

require owner signature when creating registry account

acedb89

derive registry address in UpdateRegistry instruction constructor

d2f04a1

make append_zk_elgamal_proof function more general

6d9cf4c

make check_elgamal_registry_program_account pub(crate)

5d9dc1b

refactor zk elgamal instruction data logic into a separate file

ca92028

fix registry and token account owner check

f9d886e

remove max(1) condition on pda creation

0a78c52

revert rust version bump

7afec1f

samkim-crypto and others added 7 commits October 24, 2024 17:59

remove mutable signer in CreateRegistry instruction

c5f78e1

add an option to realloc account when configuring account with registry

549b093

Apply suggestions from code review

2c626e2

Co-authored-by: Jon C <[email protected]>

remove ConfigureAccountWithRegistryInstructionData

05bd8a6

Replace to_bytes() of Pubkey to as_bytes()

1213b10

use PodStateWithExtensions instead of StateWithExtensions

1c3c19b

add a clarifying comment on try_calculate_account_len dedupe

7b77b83

samkim-crypto force-pushed the elgamal-key branch from 4b4f84a to 7b77b83 Compare October 24, 2024 08:59

joncinque approved these changes Oct 24, 2024

View reviewed changes

samkim-crypto merged commit c59e2f3 into solana-labs:master Oct 25, 2024
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[token-2022] Add elgamal registry account #7341

[token-2022] Add elgamal registry account #7341

samkim-crypto commented Oct 8, 2024 •

edited

Loading

joncinque left a comment

joncinque Oct 18, 2024

samkim-crypto Oct 22, 2024

joncinque Oct 22, 2024

samkim-crypto Oct 22, 2024

samkim-crypto Oct 23, 2024

joncinque Oct 22, 2024

samkim-crypto Oct 22, 2024

joncinque Oct 22, 2024

samkim-crypto Oct 23, 2024

joncinque left a comment

joncinque Oct 23, 2024

samkim-crypto Oct 24, 2024 •

edited

Loading

joncinque Oct 23, 2024

samkim-crypto Oct 24, 2024

joncinque Oct 23, 2024

joncinque Oct 23, 2024

samkim-crypto Oct 24, 2024

joncinque Oct 23, 2024

joncinque Oct 23, 2024

samkim-crypto Oct 24, 2024

joncinque Oct 24, 2024

joncinque left a comment

[token-2022] Add elgamal registry account #7341

[token-2022] Add elgamal registry account #7341

Conversation

samkim-crypto commented Oct 8, 2024 • edited Loading

Problem

Summary of Changes

joncinque 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

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samkim-crypto Oct 24, 2024 • edited Loading

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

joncinque left a comment

Choose a reason for hiding this comment

samkim-crypto commented Oct 8, 2024 •

edited

Loading

samkim-crypto Oct 24, 2024 •

edited

Loading