Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new crypto features #852

Merged
merged 19 commits into from
Apr 30, 2021
Merged

Add new crypto features #852

merged 19 commits into from
Apr 30, 2021

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Apr 28, 2021

Closes #582

Description

This PR introduces the Cadence changes needed for the new crypto features. Includes the following changes:

Cadence Changes

Hash Algorithms

  • New KMAC128_BLS_BLS12_381 hashing algorithm
    var algo: HashAlgorithm = HashAlgorithm.KMAC128_BLS_BLS12_381

Signature Algorithms

  • New BLS_BLS12_381 signature algorithm
    var algo: SignatureAlgorithm = SignatureAlgorithm.BLS_BLS12_381
  • Rename ECDSA_Secp256k1 signature algorithm to ECDSA_secp256k1

PublicKey Native Type

  • New isValid field
    • eg:
      let publicKey = PublicKey(publicKey: [], signatureAlgorithm: SignatureAlgorithm.ECDSA_P256)
      
      let valid = publicKey.isValid
    • Get initialized during the construction of a public key, to the value returned by the ValidatePublicKey method on the host env.
  • New verify method to validate a signature
    • signature: pub fun verify(signature: [UInt8], signedData: [UInt8], domainSeparationTag: String, hashAlgorithm: HashAlgorithm): Bool
    • e.g:
      let valid = publicKey.verify(
          signature: [],
          signedData: [],
          domainSeparationTag: "something",
          hashAlgorithm: HashAlgorithm.SHA2_256
      )

Crypto Contract

  • New hashWithTag function
    • Signature: pub fun hashWithTag(_ data: [UInt8], tag: string, algorithm: HashAlgorithm): [UInt8]
    • e.g:
      Crypto.hashWithTag(
          "01020304".decodeHex(),
          tag: "some-tag",
          algorithm: HashAlgorithm.SHA3_256
       )
      

Changes to the FVM Interface

  • New ValidatePublicKey(key *PublicKey) (bool, error) method
  • Additional parameter tag string in the Hash method.
    • New signature: Hash(data []byte, tag string, hashAlgorithm HashAlgorithm) ([]byte, error)
  • PublicKey has two new fields: IsValid and Validated

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@SupunS SupunS changed the title Supun/crypto changes Add new crypto features Apr 28, 2021
@SupunS SupunS added the Feature label Apr 28, 2021
@SupunS SupunS marked this pull request as ready for review April 28, 2021 07:33
@turbolent turbolent added the Language Breaking Change Breaks Cadence contracts deployed on Mainnet label Apr 28, 2021
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, nice work!

I added a breaking changes label because of the naming change from ECDSA_Secp256k1 to ECDSA_secp256k1, which we need to communicate

@SupunS SupunS merged commit 13ef7aa into master Apr 30, 2021
@SupunS SupunS deleted the supun/crypto-changes branch April 30, 2021 05:24
let isValid: Bool

/// Verifies a signature. Checks whether the signature was produced by signing
/// the given tag and data, using this public key and the given hash algorithm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite what the verification checks, I suggest updating the text to be more precise:

    ///  Verifies a signature under the given tag, data and public key. It uses the given hash algorithm to hash the tag and data. 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another point, I'm not sure if this is right place to mention that all verifications with an invalid public key will fail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine to add it here, or we can also add it as a note outside of the code block.

// Hash the data using the given hashing algorithm and returns the hashed data.
pub fun hash(_ data: [UInt8], algorithm: HashAlgorithm): [UInt8]

// Hash the data using the given hashing algorithm and the tag. Returns the hashed data.
pub fun hashWithTag(_ data: [UInt8], tag: string, algorithm: HashAlgorithm): [UInt8]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave this comment here, it's about the function at this line:

Shouldn't we also update isValid to Verify here too? or is that a big breaking change?

// Hash the data using the given hashing algorithm and returns the hashed data.
pub fun hash(_ data: [UInt8], algorithm: HashAlgorithm): [UInt8]

// Hash the data using the given hashing algorithm and the tag. Returns the hashed data.
pub fun hashWithTag(_ data: [UInt8], tag: string, algorithm: HashAlgorithm): [UInt8]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have suggested hashWithTag as I wasn't sure if PublicKey.Verify would be implemented entirely by the fvm or would be partly implemented in Cadence code. In the case of the former, I don't think hashWithTag is needed in Cadence, we would only need it as an fvm function.

@@ -167,6 +167,16 @@ struct AccountKey {
struct PublicKey {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can PublicKey fields (data, signature algo, isValid..) be updated after the creation of the object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they are defined as constant fields (i.e. read-only).


/// Verifies a signature. Checks whether the signature was produced by signing
/// the given tag and data, using this public key and the given hash algorithm
pub fun verify(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @turbolent mentioned about breaking changes, I think the change from isValid to verify is a breaking one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, both were added, so this isn't a breaking change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah my bad, I somehow though isValid already existed.

@turbolent turbolent mentioned this pull request May 4, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Breaking Change Breaks Cadence contracts deployed on Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New crypto features support
3 participants