Changing the Issuance Authorization Signature to the BIP 340 Schnorr scheme by vivek-arte · Pull Request #93 · QED-it/orchard

vivek-arte · 2023-11-13T07:06:27Z

This PR switches the issuance authorization signature from the redpallas signature scheme to the Schnorr signature scheme, as detailed in ZIP 227.

…norr random key generation algorithm

… trait

src/bundle/burn_validation.rs

src/issuance.rs

…sting part of the code

ConstanceBeguier

Good work !
Some suggestions to improve the implementation

src/bundle/burn_validation.rs

src/issuance.rs

src/keys.rs

src/note/asset_base.rs

… the keys are picked randomly

… since some entries are picked randomly

…the remaining asset_id are the ones that use the spec defined term for asset_id)

PaulLaux

Good overall, some more work is required.

In addition we want to test against the same test vectors like in k256/src/schnorr.rs (at least 2 first test values). Just manually copy the values to our repo as tests and add the same comment as in the following code from k256/src/schnorr.rs.

// Test vectors from:
// https://github.com/bitcoin/bips/blob/master/bip-0340/test-vectors.csv
#[cfg(test)]
mod tests {
    use super::{Signature, SigningKey, VerifyingKey};
    use hex_literal::hex;
    use signature::hazmat::PrehashVerifier;

    /// Signing test vector
    struct SignVector {... }

    /// BIP340 signing test vectors: index 0-3
    const BIP340_SIGN_VECTORS: &[SignVector] = &[
        SignVector {
            index: 0,
            secret_key: hex!("0000000000000000000000000000000000000000000000000000000000000003"),
            public_key: hex!("F9308A019258C31049344F85F89D5229B531C845836F99B08601F113BCE036F9"),
            aux_rand: hex!("0000000000000000000000000000000000000000000000000000000000000000"),
            message: hex!("0000000000000000000000000000000000000000000000000000000000000000"),
            signature: hex!(
                "E907831F80848D1069A5371B402410364BDF1C5F8307B0084C55F1CE2DCA8215
                 25F66A4A85EA8B71E482A74F382D2CE5EBEEE8FDB2172F477DF4900D310536C0"
            ),
        },

src/action.rs

src/issuance.rs

src/circuit.rs

src/bundle/burn_validation.rs

src/issuance.rs

src/keys.rs

src/issuance.rs

src/keys.rs

src/note/asset_base.rs

PaulLaux

Let's move to NonZeroScalar for pub struct IssuanceAuthorizingKey([u8; 32]);

/// Non-zero secp256k1 (K-256) scalar field element.
#[cfg(feature = "arithmetic")]
pub type NonZeroScalar = elliptic_curve::NonZeroScalar<Secp256k1>;

/// secp256k1 (K-256) public key.
#[cfg(feature = "arithmetic")]
pub type PublicKey = elliptic_curve::PublicKey<Secp256k1>;

…nstantiating the Rng inside the random function to avoid propagating the CryptoRngCore trait requirement

…eValidatingKey to contain PublicKey

src/keys.rs

…ance authorization signature function

PaulLaux

Good overall, added comments and questions.

src/keys.rs

PaulLaux · 2024-01-25T08:49:00Z

src/keys.rs

-impl ConstantTimeEq for IssuanceAuthorizingKey {
-    fn ct_eq(&self, other: &Self) -> Choice {
-        self.to_bytes().ct_eq(other.to_bytes())
+        IssuanceAuthorizingKey::from_bytes(*sk.to_bytes()).unwrap()


this can fail. Convert from to try_from to make sure we are not ignoring potential errors.

what if IssuanceAuthorizingKey::from_bytes(*sk.to_bytes()) is None
I don't want to ignore this case so I suggested to move to try_from

Oh I see, missed that. I'll update that

Updated this with some changes, that I will summarize here:

We don't actually need to allow for the generic conversion from SpendingKey to IssuanceAuthorizationKey anymore, since we are deriving it directly from ZIP 32 seeds.

So I removed the From<SpendingKey> implementation altogether, and I instead took the relevant part of logic and moved it to the only place we were using it, which is in the from_zip32_seed function.

In there, I allowed for the generation of an error if the IssuanceAuthorizingKey::from_bytes function fails, as pointed out in the original comment in this thread.

src/keys.rs

src/test_vectors/bip340_vector.rs

…he more low level sign_prehash_with_aux_rand

PaulLaux

Good overall, see last comments to finalize.

PaulLaux · 2024-01-31T08:59:00Z

src/keys.rs

-impl ConstantTimeEq for IssuanceAuthorizingKey {
-    fn ct_eq(&self, other: &Self) -> Choice {
-        self.to_bytes().ct_eq(other.to_bytes())
+        IssuanceAuthorizingKey::from_bytes(*sk.to_bytes()).unwrap()


what if IssuanceAuthorizingKey::from_bytes(*sk.to_bytes()) is None
I don't want to ignore this case so I suggested to move to try_from

src/keys.rs

src/test_vectors/zsa_issuance_auth_sig.rs

src/keys.rs

…t logic to the from_zip32_seed function

PaulLaux

approved with minor fixes

src/keys.rs

vivek-arte added 5 commits November 7, 2023 17:12

edit the IssuanceAuthorizingKey::random function to work with the sch…

619250d

…norr random key generation algorithm

changing sign function to take schnorr signature

c267adb

making most of the other schnorr signature changes across the code

f7aacfb

some further changes, start on the tests, attempts with CryptoRngCore…

6905340

… trait

fix the tests and the fmt/clippy issues

9ec7524