feat: Initial implementation of post quantum crypto + account_manager changes by ch4r10t33r · Pull Request #668 · ReamLabs/ream

ch4r10t33r · 2025-07-28T20:59:30Z

Initial implementation of post quantum crypto + account_manager changes

I have read CONTRIBUTING.md.
This PR uses conventional commits.

… to account_manager

syjn99

Few cosmetics reviews. I will dive deeper into the actual implementation. I think I need to spend a couple of hours of hash-sig library to fully understand what this PR deals with. (and luckily the documentation of hash-sig is good!, I guess they recently took an effort regarding it)

Also could you elaborate more about this PR in the description, so that others can quickly grasp the content?

syjn99 · 2025-08-21T01:23:46Z

crates/networking/p2p/src/network/misc.rs

    }
 }
-
+#[allow(deprecated)]


It seems those kind of changes is not needed.

Cargo.toml

syjn99 · 2025-08-21T01:39:49Z

Cargo.lock

It seems like there are bunch of minor version updates which are not that related with this PR. I couldn't find any issues rn. But personally if we want to bump the dependency versions, I think this PR is not a right place to do so as it brings lots of dependency changes itself.

syjn99 · 2025-08-21T01:41:30Z

crates/common/consensus/lean/src/block.rs

 pub struct SignedBlock {
    pub data: Block,
-    pub signature: PQSignature,
+    // pub signature: PQSignature,


actually PQSignature is a stub struct that should be fixed after we finalized how we put signature in SignedBlock. I would suggest not changing consensus/lean crate, and remain the stub struct as well.

I had to comment it because I ran into issues while testing. I no longer see the issue, I'll revert my changes.

use pub _signature:PQSignature then or remove it, but commenting this out doesn't make sense

syjn99 · 2025-08-21T01:42:05Z

crates/common/consensus/lean/src/validator.rs

This file isn't necessary anymore, seems like you have been working on the outdated codebase. So please remove it.

This comment was marked as resolved, but the problem wasn't resolved

syjn99

Only checked the pqc crate! Left lots of discussion points.

bin/ream/src/main.rs

syjn99 · 2025-08-21T05:31:45Z

crates/account_manager/Cargo.toml

+aes-gcm.workspace = true
+anyhow.workspace = true
+argon2.workspace = true
+base64.workspace = true
 hashsig.workspace = true
+hex.workspace = true
+hmac.workspace = true
 rand.workspace = true
-rand_chacha.workspace = true
+ream-pqc = { path = "../crypto/pqc" }
+serde = { version = "1.0", features = ["derive"] }
+serde_json.workspace = true
 sha2.workspace = true
+thiserror = "2.0"
 tracing.workspace = true


[dependencies] aes-gcm.workspace = true anyhow.workspace = true argon2.workspace = true base64.workspace = true hashsig.workspace = true hex.workspace = true hmac.workspace = true rand.workspace = true serde.workspace = true serde_json.workspace = true sha2.workspace = true thiserror.workspace = true tracing.workspace = true # ream dependencies ream-pqc.workspace = true

I think above code is more consistent with our current codebase.

syjn99 · 2025-08-21T05:42:14Z

crates/crypto/pqc/src/keystore.rs

It seems the filename keystore.rs isn't intuitive, as it mostly handles the signature scheme itself.

crates/crypto/pqc/src/keystore.rs

syjn99 · 2025-08-21T05:44:04Z

crates/crypto/pqc/src/keystore.rs

+#[derive(Serialize, Deserialize, Clone)]
+pub struct KeyPair {
+    pub public_key: Vec<u8>,
+    pub secret_key: Vec<u8>,
+    pub lifetime: u32,
+}


What's the benefit to define as a byte array for both public_key and secret_key, provided GeneralizedXMSSPublicKey and GeneralizedXMSSSecretKey are already serializable? Same comment for XmssSignature.signature.

We don't have multiple backends for the hash-based signature (at this point), so I don't think this is justified.

syjn99 · 2025-08-21T06:00:56Z

crates/crypto/pqc/src/keystore.rs

+    pub signature_index: u32, // TODO: Implement proper signature index tracking
+}
+
+pub struct XmssWrapper;


I think this struct can be improved much better. Here's what I experimented with my local environment:

Matching by height isn't scalable, and also can't handle different signature schemes. There are so many schemes in hash-sig with different 1) incomparable encodings (e.g., Winternitz, Target Sum, Top-level), 2) chunk size and 3) lifetime.

Fortunately, I found SignatureScheme trait in hash-sig crate, which will make our lives easier. Let me call the new struct XmssWrapper2:

pub struct XmssWrapper2<T: SignatureScheme> { _phantom: std::marker::PhantomData<T>, }

Then we can implement generate_keys with this trait, and note that if we don't use a byte array for PublicKey, most of the code below can also be removed. (Please refer to the previous comment that I left)

impl<T> XmssWrapper2<T> where T: SignatureScheme, T::PublicKey: Serialize + for<'de> Deserialize<'de>, T::SecretKey: Serialize + for<'de> Deserialize<'de>, T::Signature: Serialize + for<'de> Deserialize<'de>, { /// Generate XMSS key pair (serialized) pub fn generate_keys<R: Rng>(rng: &mut R) -> Result<KeyPair, XmssWrapperError> { let (public_key, secret_key) = T::key_gen(rng, 0, T::LIFETIME as usize); let public_key_bytes = serde_json::to_vec(&public_key).map_err(|e| { XmssWrapperError::Serialization(format!("Failed to serialize public key: {:?}", e)) })?; let secret_key_bytes = serde_json::to_vec(&secret_key).map_err(|e| { XmssWrapperError::Serialization(format!("Failed to serialize secret key: {:?}", e)) })?; Ok(KeyPair { public_key: public_key_bytes, secret_key: secret_key_bytes, lifetime: (T::LIFETIME as f64).log2() as u32, }) } }

And this can be tested like:

#[test] fn test_key_generation2() { let mut rng = rng(); type XmssWrapper8 = XmssWrapper2<custom_lifetime_8::CustomSIGWinternitzLifetime8W8>; let result = XmssWrapper8::generate_keys(&mut rng); assert!(result.is_ok()); let key_pair = result.unwrap(); assert_eq!(key_pair.lifetime, 8); assert!(!key_pair.public_key.is_empty()); assert!(!key_pair.secret_key.is_empty()); }

From this approach, I expect we can shrink the LOC of this file upto 40%.

Also in my knowledge, we won't use Winternitz as our primary incomparable encoding scheme.

Reference: https://eprint.iacr.org/2025/1332.pdf

syjn99 · 2025-08-21T06:03:11Z

crates/crypto/pqc/src/keystore.rs

+                // Convert message to 32-byte array as required by SignatureScheme
+                let mut message_array = [0u8; 32];
+                let message_len = message.len().min(32);
+                message_array[..message_len].copy_from_slice(&message[..message_len]);


We shouldn't sign only part of the given message. Is this code intended to get hash value of the message, as an argument?

message: &[u8; MESSAGE_LENGTH]

^ This can be much better for parameter definition.

syjn99 · 2025-08-21T06:04:28Z

crates/crypto/pqc/src/keystore.rs

+
+                #[allow(clippy::unnecessary_mut_passed)]
+                let signature =
+                    SIGWinternitzLifetime18W8::sign(rng, &mut secret_key, epoch, &message_array)


Suggested change

SIGWinternitzLifetime18W8::sign(rng, &mut secret_key, epoch, &message_array)

SIGWinternitzLifetime18W8::sign(rng, &secret_key, epoch, &message_array)

It is enough to pass only reference, not mutable reference. It also means that secret_key_bytes is never updated.

syjn99 · 2025-08-21T06:05:22Z

crates/crypto/pqc/src/keystore.rs

+        assert!(!key_pair.public_key.is_empty());
+        assert!(!key_pair.secret_key.is_empty());
+
+        // Test with tree height 8 (256 signatures)


Why do we have copy-pasted assertion for each tests?

unnawut · 2025-08-21T08:26:35Z

Cargo.toml

 ssz_types = { git = "https://github.com/ReamLabs/ssz_types", branch = "removable-variable-list" }
 tempdir = "0.3.7"
-tempfile = "3.19"
+tempfile = "=3.19"


just for my own learning, what required this pinning?

The latest version of tempfile has deprecated several functions, which is causing our make pr command to fail.

unnawut · 2025-08-21T08:32:29Z

crates/account_manager/src/lib.rs

+    // Save keys
+    save_secret_key(&key_pair, password)?;
+
+    println!("Poseidon2-XMSS key pair generated and saved successfully!");


use tracing's info! here

unnawut · 2025-08-21T08:51:29Z

crates/account_manager/src/lib.rs

+    let encrypted_json = serde_json::to_string_pretty(&encrypted_secret)
+        .map_err(|e| AccountManagerError::Serialization(e.to_string()))?;
+
+    let mut secret_file = File::create("xmss_secret_key.json")?;


Can we pass in the path as a function argument rather than hardcoding the file name here? Also applies to public key

Yes, this entire crate has to be refactored. I'll be addressing all of them in the new PR. The recent discussions in post-quantum group will require further changes in our code base.

unnawut · 2025-08-21T08:52:45Z

crates/account_manager/src/lib.rs

+fn save_secret_key(key_pair: &KeyPair, password: &str) -> Result<(), AccountManagerError> {
+    // Save public key file (unencrypted)
+    let public_key_data = serde_json::json!({
+        "public_key": hex::encode(&key_pair.public_key),
+        "tree_height": key_pair.lifetime,
+        "max_signatures": 1u64 << key_pair.lifetime,
+        "key_type": "XMSS_PUBLIC",
+        "hash_function": "Poseidon2"
+    });
+
+    let public_json_string = serde_json::to_string_pretty(&public_key_data)
+        .map_err(|e| AccountManagerError::Serialization(e.to_string()))?;
+
+    let mut public_file = File::create("xmss_public_key.json")?;
+    public_file.write_all(public_json_string.as_bytes())?;
+
+    // Encrypt and save secret key file
+    save_encrypted_secret_key(key_pair, password)?;
+
+    Ok(())
+}


Instead of having save_secret_key() that saves the public key and calls save_encrypted_secret_key(), can we instead put the public key saving into save_public_key().

Then call save_public_key() and save_encrypted_secret_key() from generate_and_save_keys()?

Yes, I had them as separate functions for me to test out the serialisation. I'll be moving this over to generate_and_save_keys() with a pre-agreed path.

unnawut · 2025-08-21T09:07:03Z

crates/account_manager/src/lib.rs

+    secret_key_path: &Path,
+    password: &str,


I think it'll be quite impractical to recover the secret key from file each time we want to sign a message.

Can we wrap this into, say, an AccountManager struct that can load a secret key from file and keep the decrypted key in memory in the struct? then we call account_manager.sign_message(...) to sign a message?

Overall this code feels unstructured. I agree most of these functions should be in structures organized by what makes sense.

unnawut · 2025-08-21T09:10:09Z

crates/account_manager/src/lib.rs

+
+    // Sign the message
+    let (signature, updated_secret_key_bytes) =
+        XmssWrapper::sign_message(message, &secret_key_bytes, tree_height, &mut rng, 0)


Umm don't we need to keep the epoch argument (i.e. fixed to 0 here) inputtable?

Please ignore the acocunt_manager, this code is incomplete.

unnawut · 2025-08-21T09:24:14Z

crates/account_manager/src/lib.rs

+            .map_err(|e| AccountManagerError::Serialization(e.to_string()))?;
+
+    // Update the secret key file with the new state
+    let updated_keypair = KeyPair {


Does the keypair change after each signing? I thought the secret_key and lifetime doesn't change per signing.

Also by setting public_key: vec![] this is a destructive operation that removes public key from the keystore file.

unnawut · 2025-08-21T09:25:46Z

crates/account_manager/src/lib.rs

+    let public_key_bytes = load_public_key(public_key_path)?;
+
+    // Verify the signature
+    let is_valid = XmssWrapper::verify_signature(message, signature, &public_key_bytes, 0)


Similar comment to signing, doesn't the epoch need to be an input?

unnawut · 2025-08-21T09:27:07Z

crates/account_manager/src/lib.rs

+fn get_tree_height_from_file<P: AsRef<Path>>(path: P) -> Result<u32, AccountManagerError> {
+    let mut file = File::open(path)?;
+    let mut content = String::new();
+    file.read_to_string(&mut content)?;
+
+    let encrypted_secret: EncryptedSecretKey = serde_json::from_str(&content)
+        .map_err(|e| AccountManagerError::Serialization(e.to_string()))?;
+
+    Ok(encrypted_secret.tree_height)


Do we need to load the file separately to get the tree height? Can we do this as part of load_secret_key()?

unnawut · 2025-08-21T10:01:29Z

crates/account_manager/src/lib.rs

+}
+
+/// Generate and save XMSS key pair with encryption
+pub fn generate_and_save_keys(height: u32, password: &str) -> Result<(), AccountManagerError> {


Super nitpick and super unsure. I think we use anyhow::Result everywhere else instead of import it for disambiguity.

@KolbyML @syjn99 Can you help confirm?

@unnawut Yes, that would be better in terms of consistency

He isn't using anyhow here.

We can do 2 things for errors

anyhow

this_error

context decides which we opt for

KolbyML

I am concerned by the uses of #[allow and I think a lot of functions should in structures, unless they are general utility functions which doesn't seem to be the case for a lot of these.

I left a few comments which should apply to multiple parts of the PR

KolbyML · 2025-08-23T06:14:17Z

crates/account_manager/Cargo.toml

+hmac.workspace = true
 rand.workspace = true
-rand_chacha.workspace = true
+ream-pqc = { path = "../crypto/pqc" }


defining path's should only be defined in the main Cargo.toml

KolbyML · 2025-08-23T06:15:50Z

bin/ream/src/cli/account_manager.rs

-            let phrase = mnemonic.phrase().to_string();
+            // Generate a random 12-word mnemonic using the correct bip39 v2.0 API
+            let entropy = rand::random::<[u8; 16]>(); // 128 bits for 12 words
+            let mnemonic = Mnemonic::from_entropy_in(bip39::Language::English, &entropy).unwrap();


unwrap shouldn't be used in production code, can this fail? under what conditions? depending on the case we should pass up an error or use expect instead of unwrap, it is probably safer to just pass up an error

KolbyML · 2025-08-23T06:17:21Z

crates/account_manager/src/lib.rs

+
+    // Generate keys using the pqc crate
+    let key_pair = XmssWrapper::generate_keys(height, &mut rng)
+        .map_err(|e| AccountManagerError::Serialization(e.to_string()))?;


Suggested change

.map_err(|e| AccountManagerError::Serialization(e.to_string()))?;

.map_err(|err| AccountManagerError::Serialization(err.to_string()))?;

```}

please make this change in the 22 other spots in the PR

KolbyML · 2025-08-23T06:18:11Z

crates/account_manager/src/lib.rs

+}
+
+/// Generate and save XMSS key pair with encryption
+pub fn generate_and_save_keys(height: u32, password: &str) -> Result<(), AccountManagerError> {


He isn't using anyhow here.

We can do 2 things for errors

anyhow

this_error

context decides which we opt for

KolbyML · 2025-08-23T06:18:56Z

crates/account_manager/src/lib.rs

+        height,
+        height,


these can be inlined, and as @unnawut state println!() shouldn't be used in production code

KolbyML · 2025-08-23T06:32:23Z

crates/networking/p2p/src/network/misc.rs

    yamux,
 };
 use libp2p_identity::{Keypair, PeerId, secp256k1::PublicKey as Secp256k1PublicKey};
+#[allow(deprecated)]


We shouldn't be using #[allow(deprecated)]

Either we update the crate in a separate PR or fix it here in this PR

KolbyML · 2025-08-23T06:34:54Z

crates/account_manager/src/lib.rs

+    secret_key_path: &Path,
+    password: &str,
+) -> Result<XmssSignature, AccountManagerError> {
+    use ream_pqc::keystore::XmssWrapper;


KolbyML · 2025-08-23T06:35:55Z

crates/account_manager/src/lib.rs

+    secret_key_path: &Path,
+    password: &str,


Overall this code feels unstructured. I agree most of these functions should be in structures organized by what makes sense.

KolbyML · 2025-08-23T06:36:31Z

crates/crypto/pqc/src/keystore.rs

+#[allow(unused_imports)]
+use custom_lifetime_8::CustomSIGWinternitzLifetime8W8;


if this is unused, why is it here?

KolbyML · 2025-08-23T06:37:14Z

crates/crypto/pqc/src/keystore.rs

+    pub public_key: Vec<u8>,
+    pub secret_key: Vec<u8>,


aren't these defined lengths? we shouldn't use vector's if the lengths are defined (are they?)

ch4r10t33r · 2025-08-24T14:12:07Z

The work will be split into multiple tasks which will be shared by @shariqnaiyer and myself. Hence, closing this pull request.

feat: Initial implementation of post quantum crypto crate and changes…

019ec3f

… to account_manager

ch4r10t33r requested review from Kayden-ML, KolbyML, syjn99 and varun-doshi as code owners July 28, 2025 20:59

ch4r10t33r marked this pull request as draft July 28, 2025 20:59

ch4r10t33r added this to Ream Client Jul 28, 2025

ch4r10t33r linked an issue Jul 28, 2025 that may be closed by this pull request

Implement an interface crate for PQ cryptography #655

Closed

ch4r10t33r removed this from Ream Client Jul 28, 2025

ch4r10t33r self-assigned this Jul 28, 2025

This was referenced Aug 6, 2025

feat: initialize lean node architecture #679

Merged

PQ Devnet-0 Meta Tracking Issue #654

Closed

ch4r10t33r added 8 commits August 17, 2025 10:08

fix:Resolved merged conflicts

3d7869c

fix: Added wrapper function to the hash-sig crate

8615481

Merge branch 'master' into pq-devnet-0

c53dad8

fix: resolved linting issues

15fcb5a

fix: refactored code between keystore and account_manager

2e8010a

fix:Merge branch 'master' into pq-devnet-0

8ba81b0

fix: corrected tests

4c8b5ca

fix: Removed failing tests with stateful signing

f144d1a

ch4r10t33r marked this pull request as ready for review August 20, 2025 14:25

ch4r10t33r added 2 commits August 20, 2025 15:36

Fix: Addressed issues with CI

9560f49

Fix: addressed failing ci issues

a28f88d

syjn99 suggested changes Aug 21, 2025

View reviewed changes

unnawut reviewed Aug 21, 2025

View reviewed changes

syjn99 removed a link to an issue Aug 22, 2025

Implement an interface crate for PQ cryptography #655

Closed

KolbyML requested changes Aug 23, 2025

View reviewed changes

ch4r10t33r closed this Aug 24, 2025

syjn99 mentioned this pull request Sep 3, 2025

Make HashSigScheme configurable & Match with current leanSpec #771

Closed

	SIGWinternitzLifetime18W8::sign(rng, &mut secret_key, epoch, &message_array)
	SIGWinternitzLifetime18W8::sign(rng, &secret_key, epoch, &message_array)

-        .map_err(|e| AccountManagerError::Serialization(e.to_string()))?;
+        .map_err(|err| AccountManagerError::Serialization(err.to_string()))?;
+```}
+please make this change in the 22 other spots in the PR

		#[allow(unused_imports)]
		use custom_lifetime_8::CustomSIGWinternitzLifetime8W8;

Comments

Conversation

ch4r10t33r commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

syjn99 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syjn99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syjn99 Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syjn99 Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unnawut Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ch4r10t33r commented Jul 28, 2025 •

edited

Loading

syjn99 left a comment •

edited

Loading

syjn99 Aug 21, 2025 •

edited

Loading

syjn99 Aug 21, 2025 •

edited

Loading

unnawut Aug 21, 2025 •

edited

Loading

KolbyML left a comment •

edited

Loading