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

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

@@ -4,6 +4,7 @@
 #![allow(clippy::derivable_impls)]
 // MAX: surpressing warning for the moment, will be dealt with in a different PR (TODO)
 use cosmwasm_schema::cw_serde;
+use std::cmp::Ordering;
 use std::fmt::{Display, Formatter};
 use std::str::FromStr;
 
@@ -200,6 +201,22 @@ impl Epoch {
         )
     }
 
+    /// Whether the DKG ceremony that establishes `epoch_id`'s keys has finished, judged against
+    /// `self` as the current epoch. Note this says nothing about whether that epoch is *over* -
+    /// the current epoch's own ceremony is concluded for all of the time it is in use.
+    ///
+    /// The ceremony of any epoch before the current one has necessarily finished, and one that
+    /// has not been reached yet certainly has not started. Callers working from a cached copy of
+    /// the current epoch can only get a pessimistic answer out of a stale one, never a premature
+    /// "yes".
+    pub fn is_ceremony_concluded(&self, epoch_id: EpochId) -> bool {
+        match epoch_id.cmp(&self.epoch_id) {
+            Ordering::Less => true,
+            Ordering::Greater => false,
+            Ordering::Equal => self.state.is_in_progress(),
+        }
+    }
+
     pub fn final_timestamp_secs(&self) -> Option<u64> {
         let mut finish = self.deadline?.seconds();
         let time_configuration = self.time_configuration;
@@ -340,3 +357,45 @@ impl EpochState {
         matches!(self, EpochState::WaitingInitialisation)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn epoch_at(epoch_id: EpochId, state: EpochState) -> Epoch {
+        Epoch::new(
+            state,
+            epoch_id,
+            TimeConfiguration::default(),
+            Timestamp::from_seconds(0),
+        )
+    }
+
+    /// Signer sets are only settled once a ceremony finishes, and callers cache them per epoch.
+    /// Answering "concluded" for an epoch still mid-ceremony would have them remember a set
+    /// that is empty or partial.
+    #[test]
+    fn a_ceremony_is_concluded_only_once_its_epoch_is_in_progress() {
+        let current = epoch_at(5, EpochState::InProgress);
+
+        // earlier ceremonies are finished by definition, and later ones cannot have started
+        assert!(current.is_ceremony_concluded(4));
+        assert!(!current.is_ceremony_concluded(6));
+
+        // the current epoch's own ceremony is concluded for all the time it is in use
+        assert!(current.is_ceremony_concluded(5));
+        for state in [
+            EpochState::WaitingInitialisation,
+            EpochState::PublicKeySubmission { resharing: false },
+            EpochState::DealingExchange { resharing: false },
+            EpochState::VerificationKeySubmission { resharing: true },
+            EpochState::VerificationKeyValidation { resharing: false },
+            EpochState::VerificationKeyFinalization { resharing: false },
+        ] {
+            assert!(
+                !epoch_at(5, state).is_ceremony_concluded(5),
+                "{state} was treated as a concluded ceremony"
+            );
+        }
+    }
+}

common/credential-proxy/src/shared_state/ecash_state.rs

@@ -33,7 +33,7 @@ use nym_validator_client::nyxd::contract_traits::{DkgQueryClient, PagedDkgQueryC
 use std::time::Duration;
 use time::{Date, OffsetDateTime};
 use tokio::sync::{RwLock, RwLockReadGuard};
-use tracing::info;
+use tracing::{info, warn};
 use url::Url;
 
 pub struct EcashState {
@@ -55,6 +55,24 @@ pub struct EcashState {
         CachedImmutableItems<(EpochId, Date), AggregatedExpirationDateSignatures>,
 }
 
+fn construct_usable_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 construct_ecash_api_client(share) {
+            Ok(client) => clients.push(client),
+            Err(err) => {
+                warn!("ignoring the key share of {owner} for epoch {epoch_id}: {err}")
+            }
+        }
+    }
+
+    clients
+}
+
 fn construct_ecash_api_client(share: ContractVKShare) -> Result<EcashApiClient, EcashApiError> {
     if !share.verified {
         return Err(EcashApiError::UnverifiedShare);
@@ -123,23 +141,55 @@ impl EcashState {
         self.required_deposit_cache.get_or_update(client).await
     }
 
+    /// Whether the ceremony for `epoch_id` has concluded, so its set of signers is settled.
+    async fn ceremony_concluded(
+        &self,
+        client: &ChainClient,
+        epoch_id: EpochId,
+    ) -> Result<bool, CredentialProxyError> {
+        Ok(self
+            .current_epoch(client)
+            .await?
+            .is_ceremony_concluded(epoch_id))
+    }
+
+    /// The signers registered for `epoch_id`, skipping any whose share cannot be used.
+    ///
+    /// A single share that was never verified - or that carries an announce address the DKG
+    /// contract never validated - must not deny the caller every *other* signer of that epoch.
+    async fn registered_ecash_clients(
+        &self,
+        client: &ChainClient,
+        epoch_id: EpochId,
+    ) -> Result<Vec<EcashApiClient>, CredentialProxyError> {
+        Ok(construct_usable_ecash_api_clients(
+            client
+                .query_chain()
+                .await
+                .get_all_verification_key_shares(epoch_id)
+                .await?,
+        ))
+    }
+
     pub async fn ecash_clients(
         &self,
         client: &ChainClient,
         epoch_id: EpochId,
-    ) -> Result<RwLockReadGuard<'_, Vec<EcashApiClient>>, CredentialProxyError> {
+    ) -> Result<Vec<EcashApiClient>, CredentialProxyError> {
+        // the moment a ceremony starts, the epoch id increments and the new epoch has no
+        // verified shares yet. this cache has no expiry, so answering from it then would
+        // remember an empty signer set for the life of the process - and this proxy would
+        // keep failing to fan out long after the ceremony finished.
+        if !self.ceremony_concluded(client, epoch_id).await? {
+            return self.registered_ecash_clients(client, epoch_id).await;
+        }
+
         self.epoch_clients
             .get_or_init(epoch_id, || async {
-                Ok(client
-                    .query_chain()
-                    .await
-                    .get_all_verification_key_shares(epoch_id)
-                    .await?
-                    .into_iter()
-                    .map(construct_ecash_api_client)
-                    .collect::<anyhow::Result<Vec<_>, EcashApiError>>()?)
+                self.registered_ecash_clients(client, epoch_id).await
             })
             .await
+            .map(|guard| guard.clone())
     }
 
     pub async fn current_epoch(&self, client: &ChainClient) -> Result<Epoch, CredentialProxyError> {
@@ -277,8 +327,7 @@ impl EcashState {
                 };
 
                 let shares =
-                    query_all_threshold_apis(all_apis.clone(), threshold, get_partial_signatures)
-                        .await?;
+                    query_all_threshold_apis(all_apis, threshold, get_partial_signatures).await?;
 
                 let aggregated = aggregate_annotated_indices_signatures(
                     nym_credentials_interface::ecash_parameters(),
@@ -352,7 +401,7 @@ impl EcashState {
                 };
 
                 let shares =
-                    query_all_threshold_apis(all_apis.clone(), threshold, get_partial_signatures)
+                    query_all_threshold_apis(all_apis, threshold, get_partial_signatures)
                         .await?;
 
                 let aggregated = aggregate_annotated_expiration_signatures(

common/credential-proxy/src/shared_state/mod.rs

@@ -222,7 +222,7 @@ impl CredentialProxyState {
     pub async fn ecash_clients(
         &self,
         epoch_id: EpochId,
-    ) -> Result<RwLockReadGuard<'_, Vec<EcashApiClient>>, CredentialProxyError> {
+    ) -> Result<Vec<EcashApiClient>, CredentialProxyError> {
         self.ecash_state()
             .ecash_clients(self.client(), epoch_id)
             .await

common/credential-proxy/src/ticketbook_manager/wallet_shares.rs

@@ -48,7 +48,7 @@ impl TicketbookManager {
         // before we commit to making the deposit, ensure we have required signatures cached and stored
         self.ensure_global_data_cached(epoch, expiration_date)
             .await?;
-        let ecash_api_clients = self.state.ecash_clients(epoch).await?.clone();
+        let ecash_api_clients = self.state.ecash_clients(epoch).await?;
 
         let deposit_data = self
             .state

nym-api/src/ecash/api_routes/aggregation.rs

@@ -51,7 +51,8 @@ pub(crate) fn aggregation_routes() -> Router<AppState> {
             (VerificationKeyResponse = "application/json"),
             (VerificationKeyResponse = "application/yaml"),
             (VerificationKeyResponse = "application/bincode")
-        ))
+        )),
+        (status = 400, body = String, description = "the requested epoch's DKG ceremony has not concluded, so it has no key yet"),
     ),
 )]
 async fn master_verification_key(
@@ -61,10 +62,16 @@ async fn master_verification_key(
     trace!("aggregated_verification_key request");
     let output = output.unwrap_or_default();
 
-    // see if we're not in the middle of new dkg
-    state.ensure_dkg_not_in_progress().await?;
+    let epoch_id = match epoch_id {
+        Some(epoch_id) => epoch_id,
+        None => state.current_dkg_epoch().await?,
+    };
 
-    let key = state.master_verification_key(epoch_id).await?;
+    // a concluded epoch's key is fixed, so a ceremony running for some *other* epoch is no
+    // reason to withhold it
+    state.ensure_ceremony_concluded(epoch_id).await?;
+
+    let key = state.master_verification_key(Some(epoch_id)).await?;
 
     Ok(output.to_response(VerificationKeyResponse::new(key.clone())))
 }
@@ -88,7 +95,8 @@ struct ExpirationDateParam {
             (AggregatedExpirationDateSignatureResponse = "application/json"),
             (AggregatedExpirationDateSignatureResponse = "application/yaml"),
             (AggregatedExpirationDateSignatureResponse = "application/bincode")
-        ))
+        )),
+        (status = 400, body = String, description = "the requested epoch's DKG ceremony has not concluded, so it has no signatures yet"),
     ),
 )]
 async fn expiration_date_signatures(
@@ -108,14 +116,15 @@ async fn expiration_date_signatures(
             .map_err(|_| EcashError::MalformedExpirationDate { raw })?,
     };
 
-    // see if we're not in the middle of new dkg
-    state.ensure_dkg_not_in_progress().await?;
-
     let epoch_id = match epoch_id {
         Some(epoch_id) => epoch_id,
         None => state.current_dkg_epoch().await?,
     };
 
+    // these signatures are an input to spending a ticketbook from that epoch, and they cannot
+    // change once its ceremony is done - so a later ceremony must not withhold them
+    state.ensure_ceremony_concluded(epoch_id).await?;
+
     let expiration_date_signatures = state
         .master_expiration_date_signatures(expiration_date, epoch_id)
         .await?;
@@ -141,7 +150,8 @@ async fn expiration_date_signatures(
             (AggregatedCoinIndicesSignatureResponse = "application/json"),
             (AggregatedCoinIndicesSignatureResponse = "application/yaml"),
             (AggregatedCoinIndicesSignatureResponse = "application/bincode")
-        ))
+        )),
+        (status = 400, body = String, description = "the requested epoch's DKG ceremony has not concluded, so it has no signatures yet"),
     ),
 )]
 async fn coin_indices_signatures(
@@ -151,10 +161,16 @@ async fn coin_indices_signatures(
     trace!("aggregated_coin_indices_signatures request");
 
     let output = output.unwrap_or_default();
-    // see if we're not in the middle of new dkg
-    state.ensure_dkg_not_in_progress().await?;
 
-    let coin_indices_signatures = state.master_coin_index_signatures(epoch_id).await?;
+    let epoch_id = match epoch_id {
+        Some(epoch_id) => epoch_id,
+        None => state.current_dkg_epoch().await?,
+    };
+
+    // as above: an input to spending, fixed once that epoch's ceremony concluded
+    state.ensure_ceremony_concluded(epoch_id).await?;
+
+    let coin_indices_signatures = state.master_coin_index_signatures(Some(epoch_id)).await?;
 
     Ok(output.to_response(AggregatedCoinIndicesSignatureResponse {
         epoch_id: coin_indices_signatures.epoch_id,

nym-api/src/ecash/api_routes/partial_signing.rs

@@ -132,7 +132,7 @@ struct ExpirationDateParam {
             (PartialExpirationDateSignatureResponse = "application/yaml"),
             (PartialExpirationDateSignatureResponse = "application/bincode")
         )),
-        (status = 400, body = String, description = "this nym-api is not an ecash signer in the current epoch"),
+        (status = 400, body = String, description = "this nym-api is not an ecash signer in the requested epoch, or that epoch's DKG ceremony has not concluded"),
     )
 )]
 async fn partial_expiration_date_signatures(
@@ -143,7 +143,6 @@ async fn partial_expiration_date_signatures(
         output,
     }): Query<ExpirationDateParam>,
 ) -> AxumResult<FormattedResponse<PartialExpirationDateSignatureResponse>> {
-    state.ensure_signer().await?;
     let output = output.unwrap_or_default();
 
     let expiration_date = match expiration_date {
@@ -152,14 +151,18 @@ async fn partial_expiration_date_signatures(
             .map_err(|_| EcashError::MalformedExpirationDate { raw })?,
     };
 
-    // see if we're not in the middle of new dkg
-    state.ensure_dkg_not_in_progress().await?;
-
     let epoch_id = match epoch_id {
         Some(epoch_id) => epoch_id,
         None => state.current_dkg_epoch().await?,
     };
 
+    // the caller wants this epoch's material, so it's this epoch's signers that have to answer
+    state.ensure_signer_for_epoch(epoch_id).await?;
+
+    // an aggregator collecting a past epoch's signatures depends on us answering this while a
+    // later ceremony runs, so only that ceremony's own epoch is refused
+    state.ensure_ceremony_concluded(epoch_id).await?;
+
     let expiration_date_signatures = state
         .partial_expiration_date_signatures(expiration_date, epoch_id)
         .await?;
@@ -184,19 +187,25 @@ async fn partial_expiration_date_signatures(
             (PartialCoinIndicesSignatureResponse = "application/yaml"),
             (PartialCoinIndicesSignatureResponse = "application/bincode")
         )),
-        (status = 400, body = String, description = "this nym-api is not an ecash signer in the current epoch"),
+        (status = 400, body = String, description = "this nym-api is not an ecash signer in the requested epoch, or that epoch's DKG ceremony has not concluded"),
     )
 )]
 async fn partial_coin_indices_signatures(
     State(state): State<Arc<EcashState>>,
     Query(EpochIdParam { epoch_id, output }): Query<EpochIdParam>,
 ) -> AxumResult<FormattedResponse<PartialCoinIndicesSignatureResponse>> {
-    state.ensure_signer().await?;
+    let epoch_id = match epoch_id {
+        Some(epoch_id) => epoch_id,
+        None => state.current_dkg_epoch().await?,
+    };
 
-    // see if we're not in the middle of new dkg
-    state.ensure_dkg_not_in_progress().await?;
+    // the caller wants this epoch's material, so it's this epoch's signers that have to answer
+    state.ensure_signer_for_epoch(epoch_id).await?;
+
+    // as above: refuse only the epoch whose ceremony is still running
+    state.ensure_ceremony_concluded(epoch_id).await?;
 
-    let coin_indices_signatures = state.partial_coin_index_signatures(epoch_id).await?;
+    let coin_indices_signatures = state.partial_coin_index_signatures(Some(epoch_id)).await?;
 
     Ok(output
         .unwrap_or_default()