bugfix: fix various bugs within nym-api making future re-DKG smoother

Cargo.toml

@@ -661,3 +661,15 @@ exit = "deny"
 panic = "deny"
 unimplemented = "deny"
 unreachable = "deny"
+
+# The contracts workspace declares its nym-* dependencies against crates.io and
+# redirects them to local paths via its own [patch.crates-io]. That patch does not
+# apply when a contract is built as a path dependency from this workspace, so
+# without mirroring it here cargo resolves a second, published copy of each shared
+# crate and the two sets of types no longer unify.
+[patch.crates-io]
+nym-coconut-dkg-common = { path = "common/cosmwasm-smart-contracts/coconut-dkg" }
+nym-contracts-common = { path = "common/cosmwasm-smart-contracts/contracts-common" }
+nym-contracts-common-testing = { path = "common/cosmwasm-smart-contracts/contracts-common-testing" }
+nym-group-contract-common = { path = "common/cosmwasm-smart-contracts/group-contract" }
+nym-multisig-contract-common = { path = "common/cosmwasm-smart-contracts/multisig-contract" }

common/client-libs/validator-client/Cargo.toml

@@ -89,6 +89,7 @@ features = ["json", "rustls"]
 anyhow = { workspace = true }
 bip39 = { workspace = true }
 cosmrs = { workspace = true, features = ["bip32"] }
+tokio = { workspace = true, features = ["rt-multi-thread", "macros"] }
 ts-rs = { workspace = true }
 
 [[example]]

common/client-libs/validator-client/src/coconut/mod.rs

@@ -9,6 +9,7 @@ use nym_compact_ecash::error::CompactEcashError;
 use nym_compact_ecash::{Base58, VerificationKeyAuth};
 use std::fmt::{Display, Formatter};
 use thiserror::Error;
+use tracing::warn;
 use url::Url;
 
 // TODO: it really doesn't feel like this should live in this crate.
@@ -108,26 +109,163 @@ pub enum EcashApiError {
     },
 }
 
+impl TryFrom<ContractVKShare> for EcashApiClient {
+    type Error = EcashApiError;
+
+    fn try_from(share: ContractVKShare) -> Result<Self, Self::Error> {
+        if !share.verified {
+            return Err(EcashApiError::UnverifiedShare);
+        }
+
+        let url_address = Url::parse(&share.announce_address)?;
+
+        // The NymApiClient constructed here uses the default (hickory DoT/DoH) resolver because
+        // this EcashApiClient is used by both client and non-client applications.
+        //
+        // In non-client applications this resolver can cause warning logs about H2 connection
+        // failure. This indicates that the long lived https connection was closed by the remote
+        // peer and the resolver will have to reconnect. It should not impact actual functionality
+        let api_client = nym_http_api_client::Client::builder(url_address)
+            .map_err(|e| EcashApiError::ClientError(e.to_string()))?
+            .build()
+            .map_err(|e| EcashApiError::ClientError(e.to_string()))?;
+
+        Ok(EcashApiClient {
+            api_client,
+            verification_key: VerificationKeyAuth::try_from_bs58(&share.share)?,
+            node_id: share.node_index,
+            cosmos_address: share.owner.as_str().parse()?,
+        })
+    }
+}
+
+/// Turn an epoch's key shares into usable API clients, skipping any share that can't be
+/// used and logging why.
+///
+/// A single bad share must not take the epoch down with it. Beyond the obvious case of a
+/// share that was never verified, the contract stores `announce_address` verbatim and
+/// share validation never looks at it, so a share can be marked verified on chain and
+/// still fail to convert here. Failing the whole batch would then deny every caller
+/// signer discovery for that epoch, recoverable only if the offending dealer updates its
+/// own announce address.
+///
+/// Callers must check the result still meets the epoch threshold - skipping is only safe
+/// because too few usable signers is a condition they detect themselves.
+pub fn usable_hickory_ecash_api_clients(shares: Vec<ContractVKShare>) -> Vec<EcashApiClient> {
+    let mut clients = Vec::with_capacity(shares.len());
+
+    for share in shares {
+        let owner = share.owner.clone();
+        let epoch_id = share.epoch_id;
+
+        match EcashApiClient::try_construct_from_share(share) {
+            Ok(client) => clients.push(client),
+            Err(err) => {
+                warn!("ignoring the key share of {owner} for epoch {epoch_id}: {err}")
+            }
+        }
+    }
+
+    clients
+}
+
 pub async fn all_ecash_api_clients<C>(
     client: &C,
     epoch_id: EpochId,
 ) -> Result<Vec<EcashApiClient>, EcashApiError>
 where
     C: DkgQueryClient,
 {
-    // TODO: this will error out if there's an invalid share out there. is that what we want?
-    client
-        .get_all_verification_key_shares(epoch_id)
-        .await?
-        .into_iter()
-        .map(EcashApiClient::try_construct_from_share)
-        .collect::<Result<Vec<_>, _>>()
-
-    // ... if not, let's switch to the below:
-    // client
-    //     .get_all_verification_key_shares(epoch_id)
-    //     .await?
-    //     .into_iter()
-    //     .filter_map(TryInto::try_into)
-    //     .collect::<Result<Vec<_>, _>>()
+    Ok(usable_hickory_ecash_api_clients(
+        client.get_all_verification_key_shares(epoch_id).await?,
+    ))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::nyxd::contract_traits::dkg_query_client::{DkgQueryMsg, PagedVKSharesResponse};
+    use async_trait::async_trait;
+    use cosmrs::AccountId;
+    use cosmwasm_std::Addr;
+    use nym_compact_ecash::ttp_keygen;
+    use serde::{Deserialize, Serialize};
+
+    /// Serves a fixed set of shares; every other query is out of scope for these tests.
+    struct StubDkgQueryClient {
+        shares: Vec<ContractVKShare>,
+    }
+
+    fn respond<S, T>(value: S) -> Result<T, NyxdError>
+    where
+        S: Serialize,
+        for<'a> T: Deserialize<'a>,
+    {
+        let raw = serde_json::to_vec(&value).expect("failed to serialise the stub response");
+        Ok(serde_json::from_slice(&raw).expect("the stub returned the wrong response type"))
+    }
+
+    #[async_trait]
+    impl DkgQueryClient for StubDkgQueryClient {
+        async fn query_dkg_contract<T>(&self, query: DkgQueryMsg) -> Result<T, NyxdError>
+        where
+            for<'a> T: Deserialize<'a>,
+        {
+            match query {
+                DkgQueryMsg::GetVerificationKeys { epoch_id, .. } => {
+                    respond(PagedVKSharesResponse {
+                        shares: self
+                            .shares
+                            .iter()
+                            .filter(|share| share.epoch_id == epoch_id)
+                            .cloned()
+                            .collect(),
+                        per_page: self.shares.len(),
+                        start_next_after: None,
+                    })
+                }
+                other => panic!("the stub does not serve {other:?}"),
+            }
+        }
+    }
+
+    fn share(index: NodeIndex, key: &VerificationKeyAuth, verified: bool) -> ContractVKShare {
+        let owner = AccountId::new("n", &[index as u8; 32]).unwrap();
+
+        ContractVKShare {
+            share: key.to_bs58(),
+            announce_address: format!("http://localhost:{}", 8080 + index),
+            node_index: index,
+            owner: Addr::unchecked(owner.to_string()),
+            epoch_id: 0,
+            verified,
+        }
+    }
+
+    #[tokio::test]
+    async fn unverified_shares_are_skipped_rather_than_failing_the_whole_epoch() {
+        let keys = ttp_keygen(2, 3).unwrap();
+
+        // one dealer never got its share verified on chain - it missed the finalisation
+        // window, say. the epoch still concluded, and the other two shares are usable.
+        let shares = vec![
+            share(1, &keys[0].verification_key(), true),
+            share(2, &keys[1].verification_key(), false),
+            share(3, &keys[2].verification_key(), true),
+        ];
+
+        let client = StubDkgQueryClient { shares };
+        let clients = all_ecash_api_clients(&client, 0)
+            .await
+            .expect("a single unverified share must not brick signer discovery for the epoch");
+
+        // the verified signers remain discoverable, so callers can still apply their own
+        // threshold check - today the unverified share aborts the conversion before any
+        // threshold is ever considered
+        assert_eq!(clients.len(), 2);
+        assert_eq!(
+            clients.iter().map(|c| c.node_id).collect::<Vec<_>>(),
+            vec![1, 3]
+        );
+    }
 }

common/client-libs/validator-client/src/nyxd/contract_traits/dkg_signing_client.rs

@@ -10,7 +10,7 @@ use async_trait::async_trait;
 use cosmrs::AccountId;
 use nym_coconut_dkg_common::dealing::{DealingChunkInfo, PartialContractDealing};
 use nym_coconut_dkg_common::msg::ExecuteMsg as DkgExecuteMsg;
-use nym_coconut_dkg_common::types::{DealingIndex, EncodedBTEPublicKeyWithProof};
+use nym_coconut_dkg_common::types::{DealingIndex, EncodedBTEPublicKeyWithProof, EpochId};
 use nym_coconut_dkg_common::verification_key::VerificationKeyShare;
 use nym_contracts_common::IdentityKey;
 
@@ -107,11 +107,13 @@ pub trait DkgSigningClient {
         &self,
         owner: &AccountId,
         resharing: bool,
+        epoch_id: EpochId,
         fee: Option<Fee>,
     ) -> Result<ExecuteResult, NyxdError> {
         let req = DkgExecuteMsg::VerifyVerificationKeyShare {
             owner: owner.to_string(),
             resharing,
+            epoch_id,
         };
 
         self.execute_dkg_contract(
@@ -226,8 +228,12 @@ mod tests {
             DkgExecuteMsg::CommitVerificationKeyShare { share, resharing } => client
                 .submit_verification_key_share(share, resharing, None)
                 .ignore(),
-            DkgExecuteMsg::VerifyVerificationKeyShare { owner, resharing } => client
-                .verify_verification_key_share(&owner.parse().unwrap(), resharing, None)
+            DkgExecuteMsg::VerifyVerificationKeyShare {
+                owner,
+                resharing,
+                epoch_id,
+            } => client
+                .verify_verification_key_share(&owner.parse().unwrap(), resharing, epoch_id, None)
                 .ignore(),
             DkgExecuteMsg::AdvanceEpochState {} => client.advance_dkg_epoch_state(None).ignore(),
             DkgExecuteMsg::TriggerReset {} => client.trigger_dkg_reset(None).ignore(),

common/cosmwasm-smart-contracts/coconut-dkg/src/msg.rs

@@ -66,6 +66,10 @@ pub enum ExecuteMsg {
     VerifyVerificationKeyShare {
         owner: String,
         resharing: bool,
+        /// The epoch whose share this order is about. Multisig proposals outlive the round
+        /// that created them, so without it an order that never got voted on could still be
+        /// executed against whatever share its owner has in a later epoch.
+        epoch_id: EpochId,
     },
 
     AdvanceEpochState {},