Merge pull request fedimint#4689 from dpc/24-03-22-better-debug-no-dbx

chore: dyntypes decoding improvements

fedimint-cli/src/lib.rs

@@ -568,7 +568,7 @@ impl FedimintCli {
 
         let client_builder = self.make_client_builder(cli).await?;
 
-        let mnemonic = load_or_generate_mnemonic(client_builder.db_no_encoders()).await?;
+        let mnemonic = load_or_generate_mnemonic(client_builder.db_no_decoders()).await?;
 
         client_builder
             .join(
@@ -594,7 +594,7 @@ impl FedimintCli {
         }
 
         let mnemonic = Mnemonic::from_entropy(
-            &Client::load_decodable_client_secret::<Vec<u8>>(client_builder.db_no_encoders())
+            &Client::load_decodable_client_secret::<Vec<u8>>(client_builder.db_no_decoders())
                 .await
                 .map_err_cli()?,
         )
@@ -626,7 +626,7 @@ impl FedimintCli {
             .await
             .map_err_cli()?;
 
-        match Client::load_decodable_client_secret_opt::<Vec<u8>>(builder.db_no_encoders())
+        match Client::load_decodable_client_secret_opt::<Vec<u8>>(builder.db_no_decoders())
             .await
             .map_err_cli()?
         {
@@ -637,7 +637,7 @@ impl FedimintCli {
             }
             None => {
                 Client::store_encodable_client_secret(
-                    builder.db_no_encoders(),
+                    builder.db_no_decoders(),
                     mnemonic.to_entropy(),
                 )
                 .await

fedimint-client/src/lib.rs

@@ -1756,7 +1756,7 @@ pub struct ClientBuilder {
     module_inits: ClientModuleInitRegistry,
     primary_module_instance: Option<ModuleInstanceId>,
     admin_creds: Option<AdminCreds>,
-    db_no_encoders: Database,
+    db_no_decoders: Database,
     stopped: bool,
 }
 
@@ -1766,7 +1766,7 @@ impl ClientBuilder {
             module_inits: Default::default(),
             primary_module_instance: Default::default(),
             admin_creds: None,
-            db_no_encoders: db,
+            db_no_decoders: db,
             stopped: false,
         }
     }
@@ -1776,7 +1776,7 @@ impl ClientBuilder {
             module_inits: client.module_inits.clone(),
             primary_module_instance: Some(client.primary_module_instance),
             admin_creds: None,
-            db_no_encoders: client.db.with_decoders(Default::default()),
+            db_no_decoders: client.db.with_decoders(Default::default()),
             stopped: false,
         }
     }
@@ -1840,12 +1840,12 @@ impl ClientBuilder {
         Ok(())
     }
 
-    pub fn db_no_encoders(&self) -> &Database {
-        &self.db_no_encoders
+    pub fn db_no_decoders(&self) -> &Database {
+        &self.db_no_decoders
     }
 
     pub async fn load_existing_config(&self) -> anyhow::Result<ClientConfig> {
-        let Some(config) = Client::get_config_from_db(&self.db_no_encoders).await else {
+        let Some(config) = Client::get_config_from_db(&self.db_no_decoders).await else {
             bail!("Client database not initialized")
         };
 
@@ -1862,15 +1862,15 @@ impl ClientBuilder {
         config: ClientConfig,
         init_mode: InitMode,
     ) -> anyhow::Result<ClientHandle> {
-        if Client::is_initialized(&self.db_no_encoders).await {
+        if Client::is_initialized(&self.db_no_decoders).await {
             bail!("Client database already initialized")
         }
 
         // Note: It's important all client initialization is performed as one big
         // transaction to avoid half-initialized client state.
         {
             debug!(target: LOG_CLIENT, "Initializing client database");
-            let mut dbtx = self.db_no_encoders.begin_transaction().await;
+            let mut dbtx = self.db_no_decoders.begin_transaction().await;
             // Save config to DB
             dbtx.insert_new_entry(
                 &ClientConfigKey {
@@ -2024,7 +2024,7 @@ impl ClientBuilder {
     }
 
     pub async fn open(self, root_secret: DerivableSecret) -> anyhow::Result<ClientHandle> {
-        let Some(config) = Client::get_config_from_db(&self.db_no_encoders).await else {
+        let Some(config) = Client::get_config_from_db(&self.db_no_decoders).await else {
             bail!("Client database not initialized")
         };
         let stopped = self.stopped;
@@ -2071,7 +2071,7 @@ impl ClientBuilder {
         let decoders = self.decoders(&config);
         let config = Self::config_decoded(config, &decoders)?;
         let fed_id = config.calculate_federation_id();
-        let db = self.db_no_encoders.with_decoders(decoders.clone());
+        let db = self.db_no_decoders.with_decoders(decoders.clone());
         let api = if let Some(admin_creds) = self.admin_creds.as_ref() {
             Self::admin_api_from_id(admin_creds.peer_id, &config)?
         } else {

fedimint-client/src/sm/state.rs

@@ -319,10 +319,12 @@ impl Encodable for DynState {
 impl Decodable for DynState {
     fn consensus_decode<R: std::io::Read>(
         reader: &mut R,
-        modules: &::fedimint_core::module::registry::ModuleDecoderRegistry,
+        decoders: &::fedimint_core::module::registry::ModuleDecoderRegistry,
     ) -> Result<Self, fedimint_core::encoding::DecodeError> {
-        let key = fedimint_core::core::ModuleInstanceId::consensus_decode(reader, modules)?;
-        modules.get_expect(key).decode(reader, key, modules)
+        let module_id = fedimint_core::core::ModuleInstanceId::consensus_decode(reader, decoders)?;
+        decoders
+            .get_expect(module_id)
+            .decode_partial(reader, module_id, decoders)
     }
 }
 

fedimint-client/src/transaction/sm.rs

@@ -148,7 +148,7 @@ impl IntoDynInstance for TxSubmissionStates {
 }
 
 pub fn tx_submission_sm_decoder() -> Decoder {
-    let mut decoder_builder = Decoder::builder();
+    let mut decoder_builder = Decoder::builder_system();
     decoder_builder.with_decodable_type::<OperationState<TxSubmissionStates>>();
     decoder_builder.build()
 }

fedimint-core/src/core.rs

@@ -258,15 +258,20 @@ where
     }
 }
 
-type DecodeFn = for<'a> fn(
-    Box<dyn Read + 'a>,
-    ModuleInstanceId,
-    &ModuleDecoderRegistry,
-) -> Result<Box<dyn Any>, DecodeError>;
+type DecodeFn = Box<
+    dyn for<'a> Fn(
+            Box<dyn Read + 'a>,
+            ModuleInstanceId,
+            &ModuleDecoderRegistry,
+        ) -> Result<Box<dyn Any>, DecodeError>
+        + Send
+        + Sync,
+>;
 
 #[derive(Default)]
 pub struct DecoderBuilder {
     decode_fns: BTreeMap<TypeId, DecodeFn>,
+    transparent: bool,
 }
 
 impl DecoderBuilder {
@@ -291,14 +296,30 @@ impl DecoderBuilder {
     where
         Type: IntoDynInstance + Decodable,
     {
+        let is_transparent_decoder = self.transparent;
         // TODO: enforce that all decoders are for the same module kind (+fix docs
         // after)
-        let decode_fn: DecodeFn = |mut reader, instance, modules| {
-            let typed_val = Type::consensus_decode(&mut reader, modules)?;
-            let dyn_val = typed_val.into_dyn(instance);
-            let any_val: Box<dyn Any> = Box::new(dyn_val);
-            Ok(any_val)
-        };
+        let decode_fn: DecodeFn = Box::new(
+            move |mut reader, instance, decoders: &ModuleDecoderRegistry| {
+                // TODO: Ideally `DynTypes` decoding couldn't ever be nested, so we could just
+                // pass empty `decoders`. But the client context uses nested `DynTypes` in
+                // `DynState`, so we special-case it with a flag.
+                let decoders = if is_transparent_decoder {
+                    Cow::Borrowed(decoders)
+                } else {
+                    Cow::Owned(Default::default())
+                };
+                let typed_val = Type::consensus_decode(&mut reader, &decoders).map_err(|err| {
+                    let err: anyhow::Error = err.into();
+                    DecodeError::new_custom(
+                        err.context(format!("while decoding Dyn type module_id={instance}")),
+                    )
+                })?;
+                let dyn_val = typed_val.into_dyn(instance);
+                let any_val: Box<dyn Any> = Box::new(dyn_val);
+                Ok(any_val)
+            },
+        );
         if self
             .decode_fns
             .insert(TypeId::of::<Type::DynType>(), decode_fn)
@@ -322,15 +343,53 @@ impl Decoder {
         DecoderBuilder::default()
     }
 
+    /// System Dyn-type, don't use.
+    #[doc(hidden)]
+    pub fn builder_system() -> DecoderBuilder {
+        DecoderBuilder {
+            transparent: true,
+            ..DecoderBuilder::default()
+        }
+    }
+
     /// Decodes a specific `DynType` from the `reader` byte stream.
     ///
     /// # Panics
     /// * If no decoder is registered for the `DynType`
-    pub fn decode<DynType: Any>(
+    pub fn decode_complete<DynType: Any>(
+        &self,
+        reader: &mut dyn Read,
+        total_len: u64,
+        module_id: ModuleInstanceId,
+        decoders: &ModuleDecoderRegistry,
+    ) -> Result<DynType, DecodeError> {
+        let mut reader = reader.take(total_len);
+
+        let val = self.decode_partial(&mut reader, module_id, decoders)?;
+        let left = reader.limit();
+
+        if left != 0 {
+            return Err(fedimint_core::encoding::DecodeError::new_custom(
+                anyhow::anyhow!(
+                    "Dyn type did not consume all bytes during decoding; module_id={}; expected={}; left={}; type={}",
+                    module_id,
+                    total_len,
+                    left,
+                    std::any::type_name::<DynType>(),
+                ),
+            ));
+        }
+
+        Ok(val)
+    }
+
+    /// Like [`Self::decode_complete`] but does not verify that all bytes were
+    /// consumed
+    pub fn decode_partial<DynType: Any>(
         &self,
         reader: &mut dyn Read,
-        instance_id: ModuleInstanceId,
-        modules: &ModuleDecoderRegistry,
+        module_id: ModuleInstanceId,
+        decoders: &ModuleDecoderRegistry,
     ) -> Result<DynType, DecodeError> {
         let decode_fn = self
             .decode_fns
@@ -343,7 +402,7 @@ impl Decoder {
                 )
             })
             .expect("Types being decoded must be registered");
-        Ok(*decode_fn(Box::new(reader), instance_id, modules)?
+        Ok(*decode_fn(Box::new(reader), module_id, decoders)?
             .downcast::<DynType>()
             .expect("Decode fn returned wrong type, can't happen due to with_decodable_type"))
     }

fedimint-core/src/encoding/mod.rs

@@ -1042,8 +1042,9 @@ where
                 module_instance_id,
                 raw,
             } => match decoders.get(module_instance_id) {
-                Some(decoder) => DynRawFallback::Decoded(decoder.decode(
+                Some(decoder) => DynRawFallback::Decoded(decoder.decode_complete(
                     &mut &raw[..],
+                    raw.len() as u64,
                     module_instance_id,
                     decoders,
                 )?),
@@ -1078,16 +1079,12 @@ where
         Ok(match decoders.get(module_instance_id) {
             Some(decoder) => {
                 let total_len_u64 = u64::consensus_decode_from_finite_reader(reader, decoders)?;
-                let mut reader = reader.take(total_len_u64);
-                let v: T = decoder.decode(&mut reader, module_instance_id, decoders)?;
-
-                if reader.limit() != 0 {
-                    return Err(fedimint_core::encoding::DecodeError::new_custom(
-                        anyhow::anyhow!("Dyn type did not consume all bytes during decoding"),
-                    ));
-                }
-
-                DynRawFallback::Decoded(v)
+                DynRawFallback::Decoded(decoder.decode_complete(
+                    reader,
+                    total_len_u64,
+                    module_instance_id,
+                    decoders,
+                )?)
             }
             None => {
                 // since the decoder is not available, just read the raw data

fedimint-core/src/macros.rs

@@ -313,34 +313,28 @@ macro_rules! module_plugin_dyn_newtype_encode_decode {
         impl Decodable for $name {
             fn consensus_decode_from_finite_reader<R: std::io::Read>(
                 reader: &mut R,
-                modules: &$crate::module::registry::ModuleDecoderRegistry,
+                decoders: &$crate::module::registry::ModuleDecoderRegistry,
             ) -> Result<Self, fedimint_core::encoding::DecodeError> {
                 let module_instance_id =
                     fedimint_core::core::ModuleInstanceId::consensus_decode_from_finite_reader(
-                        reader, modules,
+                        reader, decoders,
                     )?;
-                let val = match modules.get(module_instance_id) {
+                let val = match decoders.get(module_instance_id) {
                     Some(decoder) => {
                         let total_len_u64 =
-                            u64::consensus_decode_from_finite_reader(reader, modules)?;
-                        let mut reader = std::io::Read::take(reader, total_len_u64);
-                        let v = decoder.decode(&mut reader, module_instance_id, modules)?;
-
-                        if reader.limit() != 0 {
-                            return Err(fedimint_core::encoding::DecodeError::new_custom(
-                                anyhow::anyhow!(
-                                    "Dyn type did not consume all bytes during decoding"
-                                ),
-                            ));
-                        }
-
-                        v
+                            u64::consensus_decode_from_finite_reader(reader, decoders)?;
+                        decoder.decode_complete(
+                            reader,
+                            total_len_u64,
+                            module_instance_id,
+                            decoders,
+                        )?
                     }
-                    None => match modules.decoding_mode() {
+                    None => match decoders.decoding_mode() {
                         $crate::module::registry::DecodingMode::Reject => {
                             return Err(fedimint_core::encoding::DecodeError::new_custom(
                                 anyhow::anyhow!(
-                                    "Module decoder not available: {module_instance_id}"
+                                    "Module decoder not available: {module_instance_id} when decoding {}", std::any::type_name::<Self>()
                                 ),
                             ));
                         }

fedimint-core/src/module/mod.rs

@@ -16,7 +16,6 @@ pub mod registry;
 
 use std::collections::BTreeMap;
 use std::fmt::{self, Debug, Formatter};
-use std::io::Read;
 use std::marker::{self, PhantomData};
 use std::pin::Pin;
 use std::sync::Arc;
@@ -926,23 +925,9 @@ impl<T: Encodable + Decodable + 'static> SerdeModuleEncoding<T> {
 
         let total_len = u64::consensus_decode(&mut reader, &ModuleDecoderRegistry::default())?;
 
-        let mut reader = reader.take(total_len);
-
         // No recursive module decoding is supported since we give an empty decoder
         // registry to the decode function
-        let val = decoder.decode(
-            &mut reader,
-            module_instance,
-            &ModuleDecoderRegistry::default(),
-        )?;
-
-        if reader.limit() != 0 {
-            return Err(fedimint_core::encoding::DecodeError::new_custom(
-                anyhow::anyhow!("Dyn type did not consume all bytes during decoding"),
-            ));
-        }
-
-        Ok(val)
+        decoder.decode_complete(&mut reader, total_len, module_instance, &Default::default())
     }
 }
 

fedimint-load-test-tool/src/common.rs

@@ -149,10 +149,10 @@ pub async fn build_client(
     client_builder.with_module(WalletClientInit::default());
     client_builder.with_primary_module(1);
     let client_secret =
-        Client::load_or_generate_client_secret(client_builder.db_no_encoders()).await?;
+        Client::load_or_generate_client_secret(client_builder.db_no_decoders()).await?;
     let root_secret = PlainRootSecretStrategy::to_root_secret(&client_secret);
 
-    let client = if !Client::is_initialized(client_builder.db_no_encoders()).await {
+    let client = if !Client::is_initialized(client_builder.db_no_decoders()).await {
         if let Some(invite_code) = &invite_code {
             let client_config = ClientConfig::download_from_invite_code(invite_code).await?;
             client_builder