Merge pull request #193 from nervosnetwork/quake/fix-set-scripts-dele…

…te-bug fix: resolve set_scripts delete bug
nervosnetwork · Apr 12, 2024 · dd8507f · dd8507f
2 parents 8eaf4c3 + efd54dc
commit dd8507f
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 17 deletions.
diff --git a/src/storage.rs b/src/storage.rs
@@ -271,7 +271,6 @@ impl Storage {
 
     pub fn update_filter_scripts(&self, scripts: Vec<ScriptStatus>, command: SetScriptsCommand) {
         let mut should_filter_genesis_block = false;
-        let mut min_block_number = None;
         let mut batch = self.batch();
         let key_prefix = Key::Meta(FILTER_SCRIPTS_KEY).into_vec();
 
@@ -288,13 +287,6 @@ impl Storage {
                     });
 
                 for ss in scripts {
-                    if min_block_number
-                        .as_ref()
-                        .map(|n| *n > ss.block_number)
-                        .unwrap_or(true)
-                    {
-                        min_block_number = Some(ss.block_number);
-                    }
                     let key = [
                         key_prefix.as_ref(),
                         ss.script.as_slice(),
@@ -315,12 +307,6 @@ impl Storage {
                 }
                 let min_script_block_number = scripts.iter().map(|ss| ss.block_number).min();
                 should_filter_genesis_block = min_script_block_number == Some(0);
-                // min_block_number should be the min of all scripts' block_number when storage's filter_scripts is empty
-                min_block_number = if self.is_filter_scripts_empty() {
-                    min_script_block_number
-                } else {
-                    min_script_block_number.map(|n| n.min(self.get_min_filtered_block_number()))
-                };
 
                 for ss in scripts {
                     let key = [
@@ -341,6 +327,7 @@ impl Storage {
                 if scripts.is_empty() {
                     return;
                 }
+
                 for ss in scripts {
                     let key = [
                         key_prefix.as_ref(),
@@ -358,9 +345,7 @@ impl Storage {
 
         batch.commit().expect("batch commit should be ok");
 
-        if let Some(min_number) = min_block_number {
-            self.update_min_filtered_block_number(min_number);
-        }
+        self.update_min_filtered_block_number_by_scripts();
         self.clear_matched_blocks();
 
         if should_filter_genesis_block {
@@ -369,6 +354,24 @@ impl Storage {
         }
     }
 
+    fn update_min_filtered_block_number_by_scripts(&self) {
+        let key_prefix = Key::Meta(FILTER_SCRIPTS_KEY).into_vec();
+        let mode = IteratorMode::From(key_prefix.as_ref(), Direction::Forward);
+
+        let min_block_number = self
+            .db
+            .iterator(mode)
+            .take_while(|(key, _value)| key.starts_with(&key_prefix))
+            .map(|(_key, value)| {
+                BlockNumber::from_be_bytes(value.as_ref().try_into().expect("stored BlockNumber"))
+            })
+            .min();
+
+        if let Some(n) = min_block_number {
+            self.update_min_filtered_block_number(n);
+        }
+    }
+
     // get scripts hash that should be filtered below the given block number
     pub fn get_scripts_hash(&self, block_number: BlockNumber) -> Vec<Byte32> {
         let key_prefix = Key::Meta(FILTER_SCRIPTS_KEY).into_vec();

diff --git a/src/tests/service.rs b/src/tests/service.rs
@@ -1542,6 +1542,53 @@ fn test_set_scripts_partial_min_filtered_block_number_bug() {
     assert_eq!(storage.get_min_filtered_block_number(), 1234,);
 }
 
+#[test]
+fn test_set_scripts_delete_min_filtered_block_number_bug() {
+    let storage = new_storage("set_scripts_delete_min_filtered_block_number_bug");
+    let peers = create_peers();
+    let swc = StorageWithChainData::new(storage.clone(), Arc::clone(&peers), Default::default());
+    let rpc = BlockFilterRpcImpl { swc };
+
+    storage.update_min_filtered_block_number(42);
+    rpc.set_scripts(
+        vec![
+            ScriptStatus {
+                script: Script::new_builder()
+                    .args(Bytes::from("abc").pack())
+                    .build()
+                    .into(),
+                script_type: ScriptType::Lock,
+                block_number: 1234.into(),
+            },
+            ScriptStatus {
+                script: Script::new_builder()
+                    .args(Bytes::from("xyz").pack())
+                    .build()
+                    .into(),
+                script_type: ScriptType::Type,
+                block_number: 5678.into(),
+            },
+        ],
+        Some(SetScriptsCommand::All),
+    )
+    .unwrap();
+    assert_eq!(storage.get_min_filtered_block_number(), 1234,);
+
+    rpc.set_scripts(
+        vec![ScriptStatus {
+            script: Script::new_builder()
+                .args(Bytes::from("abc").pack())
+                .build()
+                .into(),
+            script_type: ScriptType::Lock,
+            block_number: 1234.into(),
+        }],
+        Some(SetScriptsCommand::Delete),
+    )
+    .unwrap();
+    assert_eq!(storage.get_min_filtered_block_number(), 5678,);
+}
+
 #[test]
 fn test_chain_txs_in_same_block_bug() {
     let storage = new_storage("chain_txs_in_same_block_bug");