Merge pull request fedimint#6603 from dpc/24-12-20-db-key-whole

chore(core): db should ensure whole key and value where consumed

Author: dpc

db/migrations/fedimint-server/000004.log

@@ -1 +1 @@
-ebf31ea0-f05c-4caa-9a0d-e24abaa18397
+1f1d7fc1-c6d7-4e18-b893-53f743eaa209

db/migrations/fedimint-server/IDENTITY

@@ -5,7 +5,7 @@
 #
 
 [Version]
-  rocksdb_version=8.10.0
+  rocksdb_version=8.11.4
   options_file_version=1.1
 
 [DBOptions]

db/migrations/fedimint-server/LOG

@@ -1947,11 +1947,8 @@ where
             return Err(DecodingError::wrong_prefix(Self::DB_PREFIX, data[0]));
         }
 
-        <Self as crate::encoding::Decodable>::consensus_decode(
-            &mut std::io::Cursor::new(&data[1..]),
-            modules,
-        )
-        .map_err(|decode_error| DecodingError::Other(decode_error.0))
+        <Self as crate::encoding::Decodable>::consensus_decode_whole(&data[1..], modules)
+            .map_err(|decode_error| DecodingError::Other(decode_error.0))
     }
 }
 
@@ -1960,8 +1957,7 @@ where
     T: Debug + Encodable + Decodable,
 {
     fn from_bytes(data: &[u8], modules: &ModuleDecoderRegistry) -> Result<Self, DecodingError> {
-        T::consensus_decode(&mut std::io::Cursor::new(data), modules)
-            .map_err(|e| DecodingError::Other(e.0))
+        T::consensus_decode_whole(data, modules).map_err(|e| DecodingError::Other(e.0))
     }
 
     fn to_bytes(&self) -> Vec<u8> {

db/migrations/fedimint-server/OPTIONS-000007

@@ -183,6 +183,35 @@ pub trait Decodable: Sized {
         Self::consensus_decode(r, modules)
     }
 
+    #[inline]
+    fn consensus_decode_whole(
+        slice: &[u8],
+        modules: &ModuleDecoderRegistry,
+    ) -> Result<Self, DecodeError> {
+        let total_len = slice.len() as u64;
+
+        let r = &mut &slice[..];
+        let mut r = Read::take(r, total_len);
+
+        // This method is always strictly less general than, `consensus_decode`, so it's
+        // safe and make sense to default to just calling it. This way most
+        // types, that don't care about protecting against resource exhaustion
+        // due to malicious input, can just ignore it.
+        let res = Self::consensus_decode_from_finite_reader(&mut r, modules)?;
+        let left = r.limit();
+
+        if left != 0 {
+            return Err(fedimint_core::encoding::DecodeError::new_custom(
+                anyhow::anyhow!(
+                    "Type did not consume all bytes during decoding; expected={}; left={}; type={}",
+                    total_len,
+                    left,
+                    std::any::type_name::<Self>(),
+                ),
+            ));
+        }
+        Ok(res)
+    }
     /// Decode an object with a well-defined format.
     ///
     /// This is the method that should be implemented for a typical, fixed sized

fedimint-core/src/db/mod.rs

@@ -85,6 +85,8 @@ impl RecoveryFromHistory for MintRecovery {
         dbtx: &mut DatabaseTransaction<'_>,
         args: &ClientModuleRecoverArgs<Self::Init>,
     ) -> anyhow::Result<Option<(Self, RecoveryFromHistoryCommon)>> {
+        dbtx.ensure_isolated()
+            .expect("Must be in prefixed database");
         Ok(dbtx
             .get_value(&RecoveryStateKey)
             .await
@@ -113,6 +115,8 @@ impl RecoveryFromHistory for MintRecovery {
         dbtx: &mut DatabaseTransaction<'_>,
         common: &RecoveryFromHistoryCommon,
     ) {
+        dbtx.ensure_isolated()
+            .expect("Must be in prefixed database");
         dbtx.insert_entry(
             &RecoveryStateKey,
             &(MintRecoveryState::V2(self.state.clone()), common.clone()),

fedimint-core/src/encoding/mod.rs

@@ -3,13 +3,15 @@ use std::io::Cursor;
 use fedimint_client::module::init::recovery::RecoveryFromHistoryCommon;
 use fedimint_client::module::{IdxRange, OutPointRange};
 use fedimint_core::core::OperationId;
-use fedimint_core::db::{DatabaseTransaction, IDatabaseTransactionOpsCoreTyped as _};
+use fedimint_core::db::{DatabaseRecord, DatabaseTransaction, IDatabaseTransactionOpsCore};
 use fedimint_core::encoding::{Decodable, Encodable};
 use fedimint_core::module::registry::ModuleDecoderRegistry;
 use fedimint_core::{impl_db_lookup, impl_db_record, Amount};
+use fedimint_logging::LOG_CLIENT_MODULE_MINT;
 use fedimint_mint_common::Nonce;
 use serde::Serialize;
 use strum_macros::EnumIter;
+use tracing::debug;
 
 use crate::backup::recovery::MintRecoveryState;
 use crate::input::{MintInputCommon, MintInputStateMachine, MintInputStateMachineV0};
@@ -127,10 +129,18 @@ impl_db_lookup!(
 pub async fn migrate_to_v1(
     dbtx: &mut DatabaseTransaction<'_>,
 ) -> anyhow::Result<Option<(Vec<(Vec<u8>, OperationId)>, Vec<(Vec<u8>, OperationId)>)>> {
+    dbtx.ensure_isolated().expect("Must be in our database");
     // between v0 and v1, we changed the format of `MintRecoveryState`, and instead
     // of migrating it, we can just delete it, so the recovery will just start
     // again, ignoring any existing state from before the migration
-    dbtx.remove_entry(&RecoveryStateKey).await;
+    if dbtx
+        .raw_remove_entry(&[RecoveryStateKey::DB_PREFIX])
+        .await
+        .expect("Raw operations only fail on low level errors")
+        .is_some()
+    {
+        debug!(target: LOG_CLIENT_MODULE_MINT, "Deleted previous recovery state");
+    }
 
     Ok(None)
 }