fix!(blockstore): remove an error if cid already exists (#224)

* fix(blockstore): remove an error if cid already exists In bitswap we can get blocks from peers simultanously. If some of the blocks are the same then 'put_many' could fail when inserting blocks already inserted from other peers. Since blockstore holds data associated by it's hash, it's fine to not fail an operation if data is already present. This way we won't loose blocks from other peers which were newly added. * don't update the data if cid exists in blockstore * Update blockstore/src/lru_blockstore.rs * apply in memory blockstore suggestion --------- Signed-off-by: Maciej Zwoliński <[email protected]> Co-authored-by: Yiannis Marangos <[email protected]>
eigerco · Feb 26, 2024 · a0b50ad · a0b50ad
1 parent ba94290
commit a0b50ad
Show file tree

Hide file tree

Showing 5 changed files with 12 additions and 25 deletions.
diff --git a/blockstore/src/in_memory_blockstore.rs b/blockstore/src/in_memory_blockstore.rs
@@ -1,8 +1,7 @@
 use cid::CidGeneric;
-use dashmap::mapref::entry::Entry;
 use dashmap::DashMap;
 
-use crate::{convert_cid, Blockstore, BlockstoreError, Result};
+use crate::{convert_cid, Blockstore, Result};
 
 /// Simple in-memory blockstore implementation.
 pub struct InMemoryBlockstore<const MAX_MULTIHASH_SIZE: usize> {
@@ -22,12 +21,7 @@ impl<const MAX_MULTIHASH_SIZE: usize> InMemoryBlockstore<MAX_MULTIHASH_SIZE> {
     }
 
     fn insert_cid(&self, cid: CidGeneric<MAX_MULTIHASH_SIZE>, data: &[u8]) -> Result<()> {
-        let cid_entry = self.map.entry(cid);
-        if matches!(cid_entry, Entry::Occupied(_)) {
-            return Err(BlockstoreError::CidExists);
-        }
-
-        cid_entry.insert(data.to_vec());
+        self.map.entry(cid).or_insert_with(|| data.to_vec());
 
         Ok(())
     }

diff --git a/blockstore/src/indexed_db_blockstore.rs b/blockstore/src/indexed_db_blockstore.rs
@@ -78,10 +78,8 @@ impl IndexedDbBlockstore {
 
         if !has_key(&blocks, &cid).await? {
             blocks.add(&data, Some(&cid)).await?;
-            Ok(())
-        } else {
-            Err(BlockstoreError::CidExists)
         }
+        Ok(())
     }
 
     async fn has<const S: usize>(&self, cid: &CidGeneric<S>) -> Result<bool> {

diff --git a/blockstore/src/lib.rs b/blockstore/src/lib.rs
@@ -31,10 +31,6 @@ pub use crate::sled_blockstore::SledBlockstore;
 /// Error returned when performing operations on [`Blockstore`]
 #[derive(Debug, Error)]
 pub enum BlockstoreError {
-    /// Provided CID already exists in blockstore when trying to insert it
-    #[error("CID already exists in the store")]
-    CidExists,
-
     /// Provided CID is longer than max length supported by the blockstore
     #[error("CID length longer that max allowed by the store")]
     CidTooLong,
@@ -195,8 +191,7 @@ pub(crate) mod tests {
         store.put_keyed(&cid0, b"1").await.unwrap();
         store.put_keyed(&cid1, b"2").await.unwrap();
 
-        let insert_err = store.put_keyed(&cid1, b"3").await.unwrap_err();
-        assert!(matches!(insert_err, BlockstoreError::CidExists));
+        assert!(store.put_keyed(&cid1, b"3").await.is_ok());
     }
 
     #[rstest]

diff --git a/blockstore/src/lru_blockstore.rs b/blockstore/src/lru_blockstore.rs
@@ -3,7 +3,7 @@ use std::{num::NonZeroUsize, sync::Mutex};
 use cid::CidGeneric;
 use lru::LruCache;
 
-use crate::{convert_cid, Blockstore, BlockstoreError, Result};
+use crate::{convert_cid, Blockstore, Result};
 
 /// An LRU cached [`Blockstore`].
 pub struct LruBlockstore<const MAX_MULTIHASH_SIZE: usize> {
@@ -30,12 +30,12 @@ impl<const MAX_MULTIHASH_SIZE: usize> Blockstore for LruBlockstore<MAX_MULTIHASH
     async fn put_keyed<const S: usize>(&self, cid: &CidGeneric<S>, data: &[u8]) -> Result<()> {
         let cid = convert_cid(cid)?;
         let mut cache = self.cache.lock().expect("lock failed");
-        if !cache.contains(&cid) {
-            cache.put(cid, data.to_vec());
-            Ok(())
+        if cache.contains(&cid) {
+            cache.promote(&cid);
         } else {
-            Err(BlockstoreError::CidExists)
+            cache.put(cid, data.to_vec());
         }
+        Ok(())
     }
 
     async fn has<const S: usize>(&self, cid: &CidGeneric<S>) -> Result<bool> {

diff --git a/blockstore/src/sled_blockstore.rs b/blockstore/src/sled_blockstore.rs
@@ -60,10 +60,10 @@ impl SledBlockstore {
         let data = data.to_vec();
 
         spawn_blocking(move || {
-            inner
+            let _ = inner
                 .blocks
-                .compare_and_swap(cid, None as Option<&[u8]>, Some(data))?
-                .or(Err(BlockstoreError::CidExists))
+                .compare_and_swap(cid, None as Option<&[u8]>, Some(data))?;
+            Ok(())
         })
         .await?
     }