From 8925cfd4b9a30db1aa5085fdafd44b48376c7dad Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Sat, 16 Nov 2024 14:29:54 +0100
Subject: [PATCH] fix #262

---
 client/distance/src/lib.rs           |  14 +++----
 distance-oracle/src/api.rs           |   4 +-
 distance-oracle/src/lib.rs           |  54 ++++++++++++++-------------
 distance-oracle/src/mock.rs          |   2 +-
 pallets/distance/src/benchmarking.rs |   8 ++--
 pallets/distance/src/lib.rs          |  18 ++++-----
 resources/metadata.scale             | Bin 149644 -> 149648 bytes
 7 files changed, 52 insertions(+), 48 deletions(-)

diff --git a/client/distance/src/lib.rs b/client/distance/src/lib.rs
index 85885d339..4b387b5f7 100644
--- a/client/distance/src/lib.rs
+++ b/client/distance/src/lib.rs
@@ -40,19 +40,19 @@ where
     Backend: sc_client_api::Backend<B>,
     IdtyIndex: Decode + Encode + PartialEq + TypeInfo,
 {
-    // Retrieve the pool_index from storage. If storage is inaccessible or the data is corrupted,
+    // Retrieve the period_index from storage. If storage is inaccessible or the data is corrupted,
     // return the appropriate error.
-    let pool_index = client
+    let period_index = client
         .storage(
             parent,
             &StorageKey(
-                frame_support::storage::storage_prefix(b"Distance", b"CurrentPoolIndex").to_vec(),
+                frame_support::storage::storage_prefix(b"Distance", b"CurrentPeriodIndex").to_vec(),
             ),
         )?
         .map_or_else(
             || {
                 Err(sc_client_api::blockchain::Error::Storage(
-                    "CurrentPoolIndex value not found".to_string(),
+                    "CurrentPeriodIndex value not found".to_string(),
                 ))
             },
             |raw| {
@@ -70,7 +70,7 @@ where
             &StorageKey(
                 frame_support::storage::storage_prefix(
                     b"Distance",
-                    match pool_index {
+                    match period_index % 3 {
                         0 => b"EvaluationPool0",
                         1 => b"EvaluationPool1",
                         2 => b"EvaluationPool2",
@@ -100,9 +100,9 @@ where
     // Read evaluation result from file, if it exists
     log::debug!(
         "🧙 [distance oracle] Reading evaluation result from file {:?}",
-        distance_dir.clone().join(pool_index.to_string())
+        distance_dir.clone().join(period_index.to_string())
     );
-    let evaluation_result = match std::fs::read(distance_dir.join(pool_index.to_string())) {
+    let evaluation_result = match std::fs::read(distance_dir.join(period_index.to_string())) {
         Ok(data) => data,
         Err(e) => {
             match e.kind() {
diff --git a/distance-oracle/src/api.rs b/distance-oracle/src/api.rs
index 16f98a61b..4144648aa 100644
--- a/distance-oracle/src/api.rs
+++ b/distance-oracle/src/api.rs
@@ -40,11 +40,11 @@ pub async fn parent_hash(client: &Client) -> H256 {
         .hash()
 }
 
-pub async fn current_pool_index(client: &Client, parent_hash: H256) -> u32 {
+pub async fn current_period_index(client: &Client, parent_hash: H256) -> u32 {
     client
         .storage()
         .at(parent_hash)
-        .fetch(&runtime::storage().distance().current_pool_index())
+        .fetch(&runtime::storage().distance().current_period_index())
         .await
         .expect("Cannot fetch current pool index")
         .unwrap_or_default()
diff --git a/distance-oracle/src/lib.rs b/distance-oracle/src/lib.rs
index b2b25f1c5..668995d8e 100644
--- a/distance-oracle/src/lib.rs
+++ b/distance-oracle/src/lib.rs
@@ -85,7 +85,7 @@ impl Default for Settings {
 
 /// Asynchronously runs a computation using the provided client and saves the result to a file.
 pub async fn run_and_save(client: &api::Client, settings: &Settings) {
-    let Some((evaluation, current_pool_index, evaluation_result_path)) =
+    let Some((evaluation, current_period_index, evaluation_result_path)) =
         run(client, settings, true).await
     else {
         return;
@@ -114,35 +114,38 @@ pub async fn run_and_save(client: &api::Client, settings: &Settings) {
             )
         });
 
-    // Remove old results
-    let mut files_to_remove = Vec::new();
-    for entry in settings
+    // When a new result is written, remove old results except for the current period used by the inherent logic and the next period that was just generated.
+    settings
         .evaluation_result_dir
         .read_dir()
         .unwrap_or_else(|e| {
             panic!(
-                "Cannot read distance evaluation result directory `{0:?}`: {e:?}",
-                settings.evaluation_result_dir
+                "Cannot read distance evaluation result directory `{:?}`: {:?}",
+                settings.evaluation_result_dir, e
             )
         })
         .flatten()
-    {
-        if let Ok(entry_name) = entry.file_name().into_string() {
-            if let Ok(entry_pool) = entry_name.parse::<isize>() {
-                if current_pool_index as isize - entry_pool > 3 {
-                    files_to_remove.push(entry.path());
-                }
-            }
-        }
-    }
-    files_to_remove.into_iter().for_each(|f| {
-        std::fs::remove_file(&f)
-            .unwrap_or_else(move |e| warn!("Cannot remove old result file `{f:?}`: {e:?}"));
-    });
+        .filter_map(|entry| {
+            entry
+                .file_name()
+                .into_string()
+                .ok()
+                .and_then(|name| {
+                    name.parse::<isize>().ok().filter(|&pool| {
+                        pool != current_period_index as isize
+                            && pool != (current_period_index + 1) as isize
+                    })
+                })
+                .map(|_| entry.path())
+        })
+        .for_each(|path| {
+            std::fs::remove_file(&path)
+                .unwrap_or_else(|e| warn!("Cannot remove file `{:?}`: {:?}", path, e));
+        });
 }
 
 /// Asynchronously runs a computation based on the provided client and settings.
-/// Returns `Option<(evaluation, current_pool_index, evaluation_result_path)>`.
+/// Returns `Option<(evaluation, current_period_index, evaluation_result_path)>`.
 pub async fn run(
     client: &api::Client,
     settings: &Settings,
@@ -152,10 +155,11 @@ pub async fn run(
 
     let max_depth = api::max_referee_distance(client).await;
 
-    let current_pool_index = api::current_pool_index(client, parent_hash).await;
+    let current_period_index = api::current_period_index(client, parent_hash).await;
 
     // Fetch the pending identities
-    let Some(evaluation_pool) = api::current_pool(client, parent_hash, current_pool_index).await
+    let Some(evaluation_pool) =
+        api::current_pool(client, parent_hash, current_period_index % 3).await
     else {
         info!("Nothing to do: Pool does not exist");
         return None;
@@ -169,7 +173,7 @@ pub async fn run(
 
     let evaluation_result_path = settings
         .evaluation_result_dir
-        .join((current_pool_index + 1).to_string());
+        .join((current_period_index + 1).to_string());
 
     if handle_fs {
         // Stop if already evaluated
@@ -189,7 +193,7 @@ pub async fn run(
         });
     }
 
-    info!("Evaluating distance for pool {}", current_pool_index);
+    info!("Evaluating distance for period {}", current_period_index);
     let evaluation_block = api::evaluation_block(client, parent_hash).await;
 
     // member idty -> issued certs
@@ -245,7 +249,7 @@ pub async fn run(
         .map(|(idty, _)| distance_rule(&received_certs, &referees, max_depth, *idty))
         .collect();
 
-    Some((evaluation, current_pool_index, evaluation_result_path))
+    Some((evaluation, current_period_index, evaluation_result_path))
 }
 
 fn distance_rule_recursive(
diff --git a/distance-oracle/src/mock.rs b/distance-oracle/src/mock.rs
index f5de146c6..103c940d2 100644
--- a/distance-oracle/src/mock.rs
+++ b/distance-oracle/src/mock.rs
@@ -46,7 +46,7 @@ pub async fn parent_hash(_client: &Client) -> H256 {
     Default::default()
 }
 
-pub async fn current_pool_index(_client: &Client, _parent_hash: H256) -> u32 {
+pub async fn current_period_index(_client: &Client, _parent_hash: H256) -> u32 {
     0
 }
 
diff --git a/pallets/distance/src/benchmarking.rs b/pallets/distance/src/benchmarking.rs
index 8b3c3e00b..e88e8ec92 100644
--- a/pallets/distance/src/benchmarking.rs
+++ b/pallets/distance/src/benchmarking.rs
@@ -186,7 +186,7 @@ mod benchmarks {
     #[benchmark]
     fn do_evaluation_success() -> Result<(), BenchmarkError> {
         // Benchmarking do_evaluation in case of a single success.
-        CurrentPoolIndex::<T>::put(0);
+        CurrentPeriodIndex::<T>::put(0);
         // More than membership renewal to avoid antispam
         frame_system::pallet::Pallet::<T>::set_block_number(500_000_000u32.into());
         let idty = T::IdtyIndex::one();
@@ -203,7 +203,7 @@ mod benchmarks {
             .into(),
         );
 
-        CurrentPoolIndex::<T>::put(2);
+        CurrentPeriodIndex::<T>::put(2);
         Pallet::<T>::force_update_evaluation(
             RawOrigin::Root.into(),
             caller,
@@ -230,7 +230,7 @@ mod benchmarks {
     #[benchmark]
     fn do_evaluation_failure() -> Result<(), BenchmarkError> {
         // Benchmarking do_evaluation in case of a single failure.
-        CurrentPoolIndex::<T>::put(0);
+        CurrentPeriodIndex::<T>::put(0);
         // More than membership renewal to avoid antispam
         frame_system::pallet::Pallet::<T>::set_block_number(500_000_000u32.into());
         let idty = T::IdtyIndex::one();
@@ -247,7 +247,7 @@ mod benchmarks {
             .into(),
         );
 
-        CurrentPoolIndex::<T>::put(2);
+        CurrentPeriodIndex::<T>::put(2);
         Pallet::<T>::force_update_evaluation(
             RawOrigin::Root.into(),
             caller,
diff --git a/pallets/distance/src/lib.rs b/pallets/distance/src/lib.rs
index f1866bf17..738ee5909 100644
--- a/pallets/distance/src/lib.rs
+++ b/pallets/distance/src/lib.rs
@@ -234,10 +234,10 @@ pub mod pallet {
     #[pallet::storage]
     pub(super) type DidUpdate<T: Config> = StorageValue<_, bool, ValueQuery>;
 
-    /// The current evaluation pool index.
+    /// The current evaluation period index.
     #[pallet::storage]
-    #[pallet::getter(fn current_pool_index)]
-    pub(super) type CurrentPoolIndex<T: Config> = StorageValue<_, u32, ValueQuery>;
+    #[pallet::getter(fn current_period_index)]
+    pub(super) type CurrentPeriodIndex<T: Config> = StorageValue<_, u32, ValueQuery>;
 
     #[pallet::event]
     #[pallet::generate_deposit(pub(super) fn deposit_event)]
@@ -300,7 +300,7 @@ pub mod pallet {
     #[pallet::genesis_build]
     impl<T: Config> BuildGenesisConfig for GenesisConfig<T> {
         fn build(&self) {
-            CurrentPoolIndex::<T>::put(0u32);
+            CurrentPeriodIndex::<T>::put(0u32);
         }
     }
 
@@ -314,10 +314,10 @@ pub mod pallet {
             if block % BlockNumberFor::<T>::one().saturating_mul(T::EvaluationPeriod::get().into())
                 == BlockNumberFor::<T>::zero()
             {
-                let index = (CurrentPoolIndex::<T>::get() + 1) % 3;
-                CurrentPoolIndex::<T>::put(index);
+                let index = CurrentPeriodIndex::<T>::get() + 1;
+                CurrentPeriodIndex::<T>::put(index);
                 weight = weight
-                    .saturating_add(Self::do_evaluation(index))
+                    .saturating_add(Self::do_evaluation(index % 3))
                     .saturating_add(T::DbWeight::get().reads_writes(1, 1));
             }
             weight.saturating_add(<T as pallet::Config>::WeightInfo::on_finalize())
@@ -557,7 +557,7 @@ pub mod pallet {
             who: &T::AccountId,
             idty_index: <T as pallet_identity::Config>::IdtyIndex,
         ) -> Result<(), DispatchError> {
-            Pallet::<T>::mutate_current_pool(CurrentPoolIndex::<T>::get(), |current_pool| {
+            Pallet::<T>::mutate_current_pool(CurrentPeriodIndex::<T>::get() % 3, |current_pool| {
                 // extrinsics are transactional by default, this check might not be needed
                 ensure!(
                     current_pool.evaluations.len() < (MAX_EVALUATIONS_PER_SESSION as usize),
@@ -590,7 +590,7 @@ pub mod pallet {
             evaluator: <T as frame_system::Config>::AccountId,
             computation_result: ComputationResult,
         ) -> DispatchResult {
-            Pallet::<T>::mutate_next_pool(CurrentPoolIndex::<T>::get(), |result_pool| {
+            Pallet::<T>::mutate_next_pool(CurrentPeriodIndex::<T>::get() % 3, |result_pool| {
                 // evaluation must be provided for all identities (no more, no less)
                 ensure!(
                     computation_result.distances.len() == result_pool.evaluations.len(),
diff --git a/resources/metadata.scale b/resources/metadata.scale
index 74a7f95ffee5af127794fdb6bb9346fc4dee3cbe..db221c7f4528732b3c91dd29a07b5d5d748e2599 100644
GIT binary patch
delta 66
zcmeB~$T?vmXG06)7A7kpO%LbNqN3Eil7Q5r%={G3yp+@mMg{=^1|VRWq7agis*ns(
QF#Wy|lj!ymAtnhk0LUN|Pyhe`

delta 54
zcmbO*k+WwaXG06)7A7kpSqJCRqN3Eil7RgD9M8Oz)Cxuh0RaXeVChi^nZB!$QEGds
I5R<eS0NI!i%>V!Z

-- 
GitLab