search

rooch-network / rooch

VApp Container with Move Language
https://rooch.network
Apache License 2.0
128 stars 54 forks source link

[gh-457] add Schnorr signature support. #461

Closed feliciss closed 10 months ago

feliciss commented 10 months ago

This PR resolves #457

vercel[bot] commented 10 months ago

Someone is attempting to deploy a commit to the Rooch Team on Vercel.

A member of the Team first needs to authorize it.

feliciss commented 10 months ago

I would like to remove traits imported from fastcrypto and generalize trait RoochSignatureInner and use it for SchnorrRoochSignature, Ed25519RoochSignature and ECDSARoochSignature.

But I encountered some difficult issues:

  1. To satisfy AsRef<[u8]> of RoochAddress:
error[E0277]: the trait bound `secp256k1::XOnlyPublicKey: AsRef<[u8]>` is not satisfied
   --> crates/rooch-types/src/crypto.rs:812:48
    |
812 |         let received_addr = RoochAddress::from(&pk);
    |                             ------------------ ^^^ the trait `AsRef<[u8]>` is not implemented for `secp256k1::XOnlyPublicKey`
    |                             |
    |                             required by a bound introduced by this call
    |
    = help: the following other types implement trait `From<T>`:
              <address::RoochAddress as From<&T>>
              <address::RoochAddress as From<&crypto::PublicKey>>
              <address::RoochAddress as From<move_core_types::account_address::AccountAddress>>
note: required for `address::RoochAddress` to implement `From<&secp256k1::XOnlyPublicKey>`
  1. Duplicate implementation of Secp256k1KeyPair
// TODO generalize Secp256k1KeyPair
// impl Signer<Signature> for Secp256k1KeyPair {
//     fn sign(&self, msg: &[u8]) -> Signature {
//         ECDSARoochSignature::new(self, msg).into()
//     }
// }
impl Signer<Signature> for Secp256k1KeyPair {
    fn sign(&self, msg: &[u8]) -> Signature {
        SchnorrRoochSignature::new(self, msg).into()
    }
}

They are using same key pair base for ECDSA and Schnorr signatures.

Other wreaked issues:

error[E0277]: the trait bound `OsRng: rand_core::CryptoRng` is not satisfied
   --> crates/rooch-types/src/transaction/authenticator.rs:60:50
    |
60  |         let keypair: Keypair = Keypair::generate(&mut csprng);
    |                                ----------------- ^^^^^^^^^^^ the trait `rand_core::CryptoRng` is not implemented for `OsRng`
    |                                |
    |                                required by a bound introduced by this call
    |
    = help: the following other types implement trait `rand_core::CryptoRng`:
              &'a mut R
              Box<R>
              rand::rngs::adapter::reseeding::ReseedingRng<R, Rsdr>
              rand::rngs::entropy::EntropyRng
              rand::rngs::std::StdRng
              rand::rngs::thread::ThreadRng
              rand_chacha::chacha::ChaCha12Core
              rand_chacha::chacha::ChaCha12Rng
            and 6 others
note: required by a bound in `ed25519_dalek::Keypair::generate`
    |
129 |         R: CryptoRng + RngCore,
    |            ^^^^^^^^^ required by this bound in `Keypair::generate`
error[E0277]: the trait bound `StdRng: crypto::AllowedRng` is not satisfied
    --> crates/rooch-types/src/crypto.rs:1045:27
     |
1045 |     let kp = KP::generate(&mut StdRng::from_rng(csprng).unwrap());
     |              ------------ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `crypto::AllowedRng` is not implemented for `StdRng`
     |              |
     |              required by a bound introduced by this call
     |
note: required by a bound in `KeypairTraits::generate`
    --> crates/rooch-types/src/crypto.rs:440:20
     |
440  |     fn generate<R: AllowedRng>(rng: &mut R) -> Self;
     |                    ^^^^^^^^^^ required by this bound in `KeypairTraits::generate`

I'm still solving the above problems, but I might need some reviews and hints. @jolestar @wow-sven

There're conflicts between #466 and #461, mainly for renaming spec256k1 to ECDSA in crates/rooch-types/src/transaction/authenticator.rs.

wow-sven commented 10 months ago

I would like to remove traits imported from fastcrypto and generalize trait RoochSignatureInner and use it for SchnorrRoochSignature, Ed25519RoochSignature and ECDSARoochSignature.

But I encountered some difficult issues:

  1. To satisfy AsRef<[u8]> of RoochAddress:
error[E0277]: the trait bound `secp256k1::XOnlyPublicKey: AsRef<[u8]>` is not satisfied
   --> crates/rooch-types/src/crypto.rs:812:48
    |
812 |         let received_addr = RoochAddress::from(&pk);
    |                             ------------------ ^^^ the trait `AsRef<[u8]>` is not implemented for `secp256k1::XOnlyPublicKey`
    |                             |
    |                             required by a bound introduced by this call
    |
    = help: the following other types implement trait `From<T>`:
              <address::RoochAddress as From<&T>>
              <address::RoochAddress as From<&crypto::PublicKey>>
              <address::RoochAddress as From<move_core_types::account_address::AccountAddress>>
note: required for `address::RoochAddress` to implement `From<&secp256k1::XOnlyPublicKey>`
  1. Duplicate implementation of Secp256k1KeyPair
// TODO generalize Secp256k1KeyPair
// impl Signer<Signature> for Secp256k1KeyPair {
//     fn sign(&self, msg: &[u8]) -> Signature {
//         ECDSARoochSignature::new(self, msg).into()
//     }
// }
impl Signer<Signature> for Secp256k1KeyPair {
    fn sign(&self, msg: &[u8]) -> Signature {
        SchnorrRoochSignature::new(self, msg).into()
    }
}

They are using same key pair base for ECDSA and Schnorr signatures.

Other wreaked issues:

error[E0277]: the trait bound `OsRng: rand_core::CryptoRng` is not satisfied
   --> crates/rooch-types/src/transaction/authenticator.rs:60:50
    |
60  |         let keypair: Keypair = Keypair::generate(&mut csprng);
    |                                ----------------- ^^^^^^^^^^^ the trait `rand_core::CryptoRng` is not implemented for `OsRng`
    |                                |
    |                                required by a bound introduced by this call
    |
    = help: the following other types implement trait `rand_core::CryptoRng`:
              &'a mut R
              Box<R>
              rand::rngs::adapter::reseeding::ReseedingRng<R, Rsdr>
              rand::rngs::entropy::EntropyRng
              rand::rngs::std::StdRng
              rand::rngs::thread::ThreadRng
              rand_chacha::chacha::ChaCha12Core
              rand_chacha::chacha::ChaCha12Rng
            and 6 others
note: required by a bound in `ed25519_dalek::Keypair::generate`
    |
129 |         R: CryptoRng + RngCore,
    |            ^^^^^^^^^ required by this bound in `Keypair::generate`
error[E0277]: the trait bound `StdRng: crypto::AllowedRng` is not satisfied
    --> crates/rooch-types/src/crypto.rs:1045:27
     |
1045 |     let kp = KP::generate(&mut StdRng::from_rng(csprng).unwrap());
     |              ------------ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `crypto::AllowedRng` is not implemented for `StdRng`
     |              |
     |              required by a bound introduced by this call
     |
note: required by a bound in `KeypairTraits::generate`
    --> crates/rooch-types/src/crypto.rs:440:20
     |
440  |     fn generate<R: AllowedRng>(rng: &mut R) -> Self;
     |                    ^^^^^^^^^^ required by this bound in `KeypairTraits::generate`

I'm still solving the above problems, but I might need some reviews and hints. @jolestar @wow-sven

There're conflicts between #466 and #461, mainly for renaming spec256k1 to ECDSA in crates/rooch-types/src/transaction/authenticator.rs.

Duplicate implementation of Secp256k1KeyPair

Option1: Keep making changes based on this commit （865e9dce759a71a729f5e0f527e19e7dbc209a5e） without breaking the current Fastcrypto, can be pushed to Fastcrypto

Fork fastcrypto, Add schnorr to it.

ref：https://github.com/MystenLabs/fastcrypto/issues/51

Option2: Remove fastcrypto completely and reorganize instead of keeping parts. Avoid further incompatibilities later on

feliciss commented 10 months ago

I would like to remove traits imported from fastcrypto and generalize trait RoochSignatureInner and use it for SchnorrRoochSignature, Ed25519RoochSignature and ECDSARoochSignature. But I encountered some difficult issues:

  1. To satisfy AsRef<[u8]> of RoochAddress:
error[E0277]: the trait bound `secp256k1::XOnlyPublicKey: AsRef<[u8]>` is not satisfied
   --> crates/rooch-types/src/crypto.rs:812:48
    |
812 |         let received_addr = RoochAddress::from(&pk);
    |                             ------------------ ^^^ the trait `AsRef<[u8]>` is not implemented for `secp256k1::XOnlyPublicKey`
    |                             |
    |                             required by a bound introduced by this call
    |
    = help: the following other types implement trait `From<T>`:
              <address::RoochAddress as From<&T>>
              <address::RoochAddress as From<&crypto::PublicKey>>
              <address::RoochAddress as From<move_core_types::account_address::AccountAddress>>
note: required for `address::RoochAddress` to implement `From<&secp256k1::XOnlyPublicKey>`
  1. Duplicate implementation of Secp256k1KeyPair
// TODO generalize Secp256k1KeyPair
// impl Signer<Signature> for Secp256k1KeyPair {
//     fn sign(&self, msg: &[u8]) -> Signature {
//         ECDSARoochSignature::new(self, msg).into()
//     }
// }
impl Signer<Signature> for Secp256k1KeyPair {
   fn sign(&self, msg: &[u8]) -> Signature {
       SchnorrRoochSignature::new(self, msg).into()
   }
}

They are using same key pair base for ECDSA and Schnorr signatures. Other wreaked issues:

error[E0277]: the trait bound `OsRng: rand_core::CryptoRng` is not satisfied
   --> crates/rooch-types/src/transaction/authenticator.rs:60:50
    |
60  |         let keypair: Keypair = Keypair::generate(&mut csprng);
    |                                ----------------- ^^^^^^^^^^^ the trait `rand_core::CryptoRng` is not implemented for `OsRng`
    |                                |
    |                                required by a bound introduced by this call
    |
    = help: the following other types implement trait `rand_core::CryptoRng`:
              &'a mut R
              Box<R>
              rand::rngs::adapter::reseeding::ReseedingRng<R, Rsdr>
              rand::rngs::entropy::EntropyRng
              rand::rngs::std::StdRng
              rand::rngs::thread::ThreadRng
              rand_chacha::chacha::ChaCha12Core
              rand_chacha::chacha::ChaCha12Rng
            and 6 others
note: required by a bound in `ed25519_dalek::Keypair::generate`
    |
129 |         R: CryptoRng + RngCore,
    |            ^^^^^^^^^ required by this bound in `Keypair::generate`
error[E0277]: the trait bound `StdRng: crypto::AllowedRng` is not satisfied
   --> crates/rooch-types/src/crypto.rs:1045:27
    |
1045 |     let kp = KP::generate(&mut StdRng::from_rng(csprng).unwrap());
    |              ------------ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `crypto::AllowedRng` is not implemented for `StdRng`
    |              |
    |              required by a bound introduced by this call
    |
note: required by a bound in `KeypairTraits::generate`
   --> crates/rooch-types/src/crypto.rs:440:20
    |
440  |     fn generate<R: AllowedRng>(rng: &mut R) -> Self;
    |                    ^^^^^^^^^^ required by this bound in `KeypairTraits::generate`

I'm still solving the above problems, but I might need some reviews and hints. @jolestar @wow-sven There're conflicts between #466 and #461, mainly for renaming spec256k1 to ECDSA in crates/rooch-types/src/transaction/authenticator.rs.

Duplicate implementation of Secp256k1KeyPair

Option1: Keep making changes based on this commit （865e9dce759a71a729f5e0f527e19e7dbc209a5e） without breaking the current Fastcrypto, can be pushed to Fastcrypto

Fork fastcrypto, Add schnorr to it.

  • Implements the Authenticator trait for SchnorrSignature
  • Implements the VerifyingKey trait for XOnlyPublicKey
  • Implements the fastcrypto::KeyPair trait for KeyPair

ref：MystenLabs/fastcrypto#51

Option2: Remove fastcrypto completely and reorganize instead of keeping parts. Avoid further incompatibilities later on

Thanks. 🙂 I'll do Option 2 to decouple it from fastcrypto. I don't think it's a good idea to keep dependencies on commercial libraries.

jolestar commented 10 months ago

fastcrypto is in Apache2/MIT license. I think Option 1 is better.

feliciss commented 10 months ago

fastcrypto is in Apache2/MIT license. I think Option 1 is better.

I can add Option 1 temporarily, but in the long term view, it may need to decouple from fastcrypto if that repo isn't being maintained.

feliciss commented 10 months ago

fastcrypto is in Apache2/MIT license. I think Option 1 is better.

Can you @wow-sven @jolestar make a fork the repo fastcrypto? I do not have access to the team account rooch-network on GitHub. Or I can fork to my own and submit it later to the team repo.

jolestar commented 10 months ago

fastcrypto is in Apache2/MIT license. I think Option 1 is better.

Can you @wow-sven @jolestar make a fork the repo fastcrypto? I do not have access to the team account rooch-network on GitHub. Or I can fork to my own and submit it later to the team repo.

https://github.com/rooch-network/fastcrypto

vercel[bot] commented 10 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **rooch** | ⬜️ Ignored ([Inspect](https://vercel.com/rooch/rooch/ESzH9HnTFa4HPLyUk4JhkeLjaVXS)) | | | Jul 19, 2023 3:21am |
feliciss commented 10 months ago

@wow-sven @jolestar I can almost finish it, but, there's another duplicate implementation. Do you have any idea to resolve it?

Code:

#[derive(Debug, Clone, PartialEq, Eq, JsonSchema)]
// TODO From here
// #[derive(Debug, Clone, PartialEq, Eq, From, JsonSchema)]
pub enum PublicKey {
    Ed25519(Ed25519PublicKeyAsBytes),
    Ecdsa(Secp256k1PublicKeyAsBytes),
    Schnorr(SchnorrPublicKeyAsBytes),
}

// TODO resolve From conflicts
impl From<BytesRepresentation<32>> for PublicKey {
    fn from(pk: BytesRepresentation<32>) -> Self {
        PublicKey::Ed25519(pk)
    }
}

impl AsRef<[u8]> for PublicKey {
    fn as_ref(&self) -> &[u8] {
        match self {
            PublicKey::Ed25519(pk: &BytesRepresentation<32>) => &pk.0,
            PublicKey::Ecdsa(pk: &BytesRepresentation<33>) => &pk.0,
            PublicKey::Schnorr(pk: &BytesRepresentation<32>) => &pk.0,
        }
    }
}

There're two hidden implementations of &BytesRepresentation<32> here.

One solution I can think of is to abstract BytesRepresentation<32> with BytesRepresentation<ed25519>, BytesRepresentation<ecdsa>, BytesRepresentation<schnorr>, respectively.

I'll try implementing it later.

feliciss commented 10 months ago

It's ready for review now.