Cleaned up the response verification

.gitignore

@@ -8,4 +8,5 @@ tmp
 *.log
 
 # Artifacts that may be left over
-**/*-secret-manager-*
+**/*-secret-manager-*
+integration-tests/chain-signatures/.project

CHANGELOG.md

@@ -2,5 +2,6 @@
 
 ## 0.3.0
 
-- Payload hash scalars are now big endian. In general this means that clients of the contract should no longer reverse their hashed payload before sending it to the MPC contract.
+- Payload hash scalars are now big endian where previously they were little endian. In general this means that clients of the contract should no longer reverse their hashed payload before sending it to the MPC contract.
+- The sha256 scalar used to derive the epsilon value is now big endian where previously it was little endian. In general this means clients should no longer reverse the bytes generated when hashing the epsilon derivation path. These new derivation paths will result in all testnet keys being lost.
 

chain-signatures/crypto-shared/src/kdf.rs

@@ -22,10 +22,7 @@ pub fn derive_epsilon(predecessor_id: &AccountId, path: &str) -> Scalar {
     let derivation_path = format!("{EPSILON_DERIVATION_PREFIX}{},{}", predecessor_id, path);
     let mut hasher = Sha256::new();
     hasher.update(derivation_path);
-    let mut bytes = hasher.finalize();
-    // Due to a previous bug in our Scalar conversion code, this hash was reversed, we reverse it here to preserve compatibility, but will likely change this later.
-    bytes.reverse();
-    Scalar::from_bytes(&bytes)
+    Scalar::from_bytes(&hasher.finalize())
 }
 
 pub fn derive_key(public_key: PublicKey, epsilon: Scalar) -> PublicKey {
@@ -68,8 +65,8 @@ pub fn check_ec_signature(
     anyhow::bail!("cannot use either recovery id={recovery_id} to recover pubic key")
 }
 
-// #[cfg(not(target_arch = "wasm32"))]
-fn recover(
+#[cfg(not(target_arch = "wasm32"))]
+pub fn recover(
     prehash: &[u8],
     signature: &Signature,
     recovery_id: RecoveryId,
@@ -78,16 +75,21 @@ fn recover(
         .context("Unable to recover public key")
 }
 
-// TODO Migrate to native function
-// #[cfg(target_arch = "wasm32")]
-// fn recover(
-//     prehash: &[u8],
-//     signature: &Signature,
-//     recovery_id: RecoveryId,
-// ) -> anyhow::Result<VerifyingKey> {
-//     use near_sdk::env;
-//     let recovered_key =
-//         env::ecrecover(prehash, &signature.to_bytes(), recovery_id.to_byte(), false)
-//             .context("Unable to recover public key")?;
-//     VerifyingKey::try_from(&recovered_key[..]).context("Failed to parse returned key")
-// }
+#[cfg(target_arch = "wasm32")]
+pub fn recover(
+    prehash: &[u8],
+    signature: &Signature,
+    recovery_id: RecoveryId,
+) -> anyhow::Result<VerifyingKey> {
+    use k256::EncodedPoint;
+    use near_sdk::env;
+    // While this function also works on native code, it's a bit weird and unsafe.
+    // I'm more comfortable using an existing library instead.
+    let recovered_key_bytes =
+        env::ecrecover(prehash, &signature.to_bytes(), recovery_id.to_byte(), true)
+            .context("Unable to recover public key")?;
+    VerifyingKey::from_encoded_point(&EncodedPoint::from_untagged_bytes(
+        &recovered_key_bytes.into(),
+    ))
+    .context("Failed to parse returned key")
+}

chain-signatures/crypto-shared/src/types.rs

@@ -79,7 +79,6 @@ fn serializeable_scalar_roundtrip() {
             assert_eq!(input, output, "Failed on {:?}", scalar);
         }
 
-        dbg!(scalar);
         // Test Serde via JSON
         {
             let serialized = serde_json::to_vec(&input).unwrap();

chain-signatures/node/src/kdf.rs

@@ -1,11 +1,7 @@
 use anyhow::Context;
-use crypto_shared::{x_coordinate, ScalarExt, SignatureResponse};
+use crypto_shared::{kdf::recover, x_coordinate, ScalarExt, SignatureResponse};
 use hkdf::Hkdf;
-use k256::{
-    ecdsa::{RecoveryId, VerifyingKey},
-    elliptic_curve::sec1::ToEncodedPoint,
-    Scalar,
-};
+use k256::{ecdsa::RecoveryId, elliptic_curve::sec1::ToEncodedPoint, Scalar};
 use near_primitives::hash::CryptoHash;
 use sha2::Sha256;
 
@@ -34,7 +30,7 @@ pub fn into_eth_sig(
     let public_key = public_key.to_encoded_point(false);
     let signature = k256::ecdsa::Signature::from_scalars(x_coordinate(big_r), s)
         .context("cannot create signature from cait_sith signature")?;
-    let pk0 = VerifyingKey::recover_from_prehash(
+    let pk0 = recover(
         &msg_hash.to_bytes(),
         &signature,
         RecoveryId::try_from(0).context("cannot create recovery_id=0")?,
@@ -45,7 +41,7 @@ pub fn into_eth_sig(
         return Ok(SignatureResponse::new(*big_r, *s, 0));
     }
 
-    let pk1 = VerifyingKey::recover_from_prehash(
+    let pk1 = recover(
         &msg_hash.to_bytes(),
         &signature,
         RecoveryId::try_from(1).context("cannot create recovery_id=1")?,

chain-signatures/node/src/protocol/cryptography.rs

@@ -437,17 +437,14 @@ impl CryptographicProtocol for RunningState {
             let info = self.fetch_participant(&p)?;
             messages.push(info.clone(), MpcMessage::Signature(msg));
         }
-        if let Err(err) = signature_manager
+        signature_manager
             .publish(
                 ctx.rpc_client(),
                 ctx.signer(),
                 ctx.mpc_contract_id(),
                 &my_account_id,
             )
-            .await
-        {
-            tracing::warn!(?err, "running: failed to publish signatures");
-        }
+            .await;
         drop(signature_manager);
         let failures = messages
             .send_encrypted(

chain-signatures/node/src/protocol/signature.rs

@@ -182,17 +182,38 @@ pub struct SignatureManager {
     completed: HashMap<PresignatureId, Instant>,
     /// Generated signatures assigned to the current node that are yet to be published.
     /// Vec<(receipt_id, msg_hash, timestamp, output)>
-    signatures: Vec<(
-        CryptoHash,
-        SignatureRequest,
-        Instant,
-        FullSignature<Secp256k1>,
-    )>,
+    signatures: Vec<ToPublish>,
     me: Participant,
     public_key: PublicKey,
     epoch: u64,
 }
 
+pub const MAX_RETRY: u8 = 10;
+pub struct ToPublish {
+    receipt_id: CryptoHash,
+    request: SignatureRequest,
+    time_added: Instant,
+    signature: FullSignature<Secp256k1>,
+    retry_count: u8,
+}
+
+impl ToPublish {
+    pub fn new(
+        receipt_id: CryptoHash,
+        request: SignatureRequest,
+        time_added: Instant,
+        signature: FullSignature<Secp256k1>,
+    ) -> ToPublish {
+        ToPublish {
+            receipt_id,
+            request,
+            time_added,
+            signature,
+            retry_count: 0,
+        }
+    }
+}
+
 impl SignatureManager {
     pub fn new(me: Participant, public_key: PublicKey, epoch: u64) -> Self {
         Self {
@@ -437,7 +458,7 @@ impl SignatureManager {
                         };
                         if generator.proposer == self.me {
                             self.signatures
-                                .push((*receipt_id, request, generator.sign_request_timestamp, output));
+                                .push(ToPublish::new(*receipt_id, request, generator.sign_request_timestamp, output));
                         }
                         // Do not retain the protocol
                         return false;
@@ -531,18 +552,29 @@ impl SignatureManager {
         signer: &T,
         mpc_contract_id: &AccountId,
         my_account_id: &AccountId,
-    ) -> Result<(), near_fetch::Error> {
-        for (receipt_id, request, time_added, signature) in self.signatures.drain(..) {
+    ) {
+        let mut to_retry: Vec<ToPublish> = Vec::new();
+
+        for mut to_publish in self.signatures.drain(..) {
+            let ToPublish {
+                receipt_id,
+                request,
+                time_added,
+                signature,
+                ..
+            } = &to_publish;
             let expected_public_key = derive_key(self.public_key, request.epsilon.scalar);
             // We do this here, rather than on the client side, so we can use the ecrecover system function on NEAR to validate our signature
-            let signature = into_eth_sig(
+            let Ok(signature) = into_eth_sig(
                 &expected_public_key,
                 &signature.big_r,
                 &signature.s,
                 Scalar::from_bytes(&request.payload_hash),
-            )
-            .map_err(|_| near_fetch::Error::InvalidArgs("Failed to generate a recovery ID"))?;
-            let response = rpc_client
+            ) else {
+                tracing::error!(%receipt_id, "Failed to generate a recovery ID");
+                continue;
+            };
+            let response = match rpc_client
                 .call(signer, mpc_contract_id, "respond")
                 .args_json(serde_json::json!({
                     "request": request,
@@ -551,7 +583,30 @@ impl SignatureManager {
                 .max_gas()
                 .retry_exponential(10, 5)
                 .transact()
-                .await?;
+                .await
+            {
+                Ok(response) => response,
+                Err(err) => {
+                    tracing::error!(%receipt_id, error = ?err, "Failed to publish transaction");
+                    // Push the response to the back of the queue if it hasn't been retried the max number of times
+                    if to_publish.retry_count < MAX_RETRY {
+                        to_publish.retry_count += 1;
+                        to_retry.push(to_publish);
+                    }
+                    continue;
+                }
+            };
+
+            match response.json() {
+                Ok(()) => {
+                    tracing::info!(%receipt_id, bi_r = signature.big_r.affine_point.to_base58(), s = ?signature.s, "published signature sucessfully")
+                }
+                Err(err) => {
+                    tracing::error!(%receipt_id, bi_r = signature.big_r.affine_point.to_base58(), s = ?signature.s, error = ?err, "smart contract threw error");
+                    continue;
+                }
+            };
+
             crate::metrics::NUM_SIGN_SUCCESS
                 .with_label_values(&[my_account_id.as_str()])
                 .inc();
@@ -563,9 +618,9 @@ impl SignatureManager {
                     .with_label_values(&[my_account_id.as_str()])
                     .inc();
             }
-            tracing::info!(%receipt_id, big_r = signature.big_r.affine_point.to_base58(), s = ?signature.s, status = ?response.status(), "published signature response");
         }
-        Ok(())
+        // Put the failed requests at the back of the queue
+        self.signatures.extend(to_retry);
     }
 
     /// Check whether or not the signature has been completed with this presignature_id.

flake.nix

@@ -32,7 +32,6 @@
               # Native build dependencies
               protobuf
               curl
-              gmp
               openssl
 
               # Development

integration-tests/chain-signatures/tests/actions/mod.rs

@@ -95,7 +95,6 @@ pub async fn assert_signature(
     let mpc_pk = AffinePoint::from_encoded_point(&mpc_point).unwrap();
     let epsilon = derive_epsilon(account_id, "test");
     let user_pk = derive_key(mpc_pk, epsilon);
-
     assert!(signature.verify(&user_pk, &Scalar::from_bytes(payload),));
 }
 
@@ -348,28 +347,17 @@ pub async fn clear_toxics() -> anyhow::Result<()> {
     Ok(())
 }
 
-// This code was and still is a bit of a mess.
-// Previously converting a Scalar to bytes reversed the bytes and converted to a Scalar.
-// The big_r and s values were generated using chain signatures from an older commit, therefore the signature is generated against a reversed hash.
-// This shows that the old signatures will verify against a reversed payload
 #[tokio::test]
-async fn test_old_signatures_verify() {
-    use k256::sha2::{Digest, Sha256};
-    let big_r = "044bf886afee5a6844a25fa6831a01715e990d3d9e96b792a9da91cfbecbf8477cea57097a3db9fc1d4822afade3d1c4e6d66e99568147304ae34bcfa609d90a16";
-    let s = "1f871c67139f617409067ac8a7150481e3a5e2d8a9207ffdaad82098654e95cb";
-    let mpc_key = "02F2B55346FD5E4BFF1F06522561BDCD024CEA25D98A091197ACC04E22B3004DB2";
-    let account_id = "acc_mc.test.near";
-
-    let mut payload = [0u8; 32];
-    for (i, item) in payload.iter_mut().enumerate() {
-        *item = i as u8;
-    }
-
-    let mut hasher = Sha256::new();
-    hasher.update(payload);
-
-    let mut payload_hash: [u8; 32] = hasher.finalize().into();
-    payload_hash.reverse();
+async fn test_signatures_verify() {
+    let big_r = "03d6d674dae94517646708cfde6e2e46a2e666e06b92eba19290eb0ca11d5e45dc";
+    let s = "2a5f2bff1b8e7da4257d480c5610d0d2c35426ee12abb87ff9c3141fe448ab27";
+    let mpc_key = "04cc5ed2a876b6fc54176bcde0805e469ac7eca43a97bfff90acd5babbef3a33b10d14fed35065a06a67b9a243169f33ab20bf9dab49cf6c1466a15349c011ca2b";
+    let account_id = "dev-20240717130550-33209224232133.test.near";
+    let payload_hash: [u8; 32] =
+        hex::decode("7be9d96ac6895be4c59e59bb67c015f28cb94669657ddb00e8aa063f62e18031")
+            .unwrap()
+            .try_into()
+            .unwrap();
 
     let payload_hash_scalar = Scalar::from_bytes(&payload_hash);