Fix bad handling of interval when computing range to compute block range root

Alenar · Alenar · commit e2aa66c12fb7 · 2024-04-26T13:53:34.000+02:00
* This computation forgot to handle the case when the last stored
  transaction is included at the end of the last stored block range root.
  When this case was seen it yielded an error instead of saying that there
  was no work to be done.

  record it derives from. This allow to write simpler tests.
diff --git a/mithril-aggregator/src/database/record/interval_without_block_range_root.rs b/mithril-aggregator/src/database/record/interval_without_block_range_root.rs
@@ -1,18 +1,45 @@
+use std::ops::Range;
+
+use anyhow::anyhow;
 use sqlite::Row;
 
 use mithril_common::entities::BlockNumber;
+use mithril_common::StdResult;
 use mithril_persistence::sqlite::{HydrationError, Projection, SqLiteEntity};
 
 use crate::database::record::hydrator::try_to_u64;
 
 /// Interval of block numbers (`[start, end[`) without block ranges root.
 pub struct IntervalWithoutBlockRangeRootRecord {
-    /// Start of the interval
+    /// Start of the interval, fetched from the last block range root end
     pub start: Option<BlockNumber>,
-    /// End of the interval
+    /// End of the interval, fetched from the last transaction block number
     pub end: Option<BlockNumber>,
 }
 
+impl IntervalWithoutBlockRangeRootRecord {
+    /// Deduce from self the range of blocks that are available to compute block range roots.
+    pub fn to_range(&self) -> StdResult<Option<Range<BlockNumber>>> {
+        match (self.start, self.end) {
+            // No transactions stored - nothing can be computed
+            (_, None) => Ok(None),
+            // Transactions stored but no block range root - everything can be computed
+            (None, Some(end)) => Ok(Some(0..(end + 1))),
+            // Last stored transactions is at the end of the last block range - nothing to compute
+            (Some(start), Some(end)) if (end + 1) == start => Ok(None),
+            (Some(start), Some(end)) if end < start => {
+                Err(anyhow!(
+                        "Inconsistent state: \
+                        Last block range root block number ({start}) is higher than the last transaction block number ({end}). \
+                        Did you forgot to prune obsolete `block_range_root` after a transaction rollback ?"
+                    ))
+            }
+            // Nominal case
+            (Some(start), Some(end)) => Ok(Some(start..(end + 1))),
+        }
+    }
+}
+
 impl SqLiteEntity for IntervalWithoutBlockRangeRootRecord {
     fn hydrate(row: Row) -> Result<Self, HydrationError>
     where
@@ -37,3 +64,80 @@ impl SqLiteEntity for IntervalWithoutBlockRangeRootRecord {
         ])
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn db_without_block_range_nor_transactions_yield_none() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: None,
+            end: None,
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(None, range);
+    }
+
+    #[test]
+    fn db_with_only_transactions_yield_a_range() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: None,
+            end: Some(10),
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(Some(0..11), range);
+    }
+
+    #[test]
+    fn db_with_only_block_range_and_no_transactions_yield_none() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: Some(10),
+            end: None,
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(None, range);
+    }
+
+    #[test]
+    fn db_with_both_block_range_and_transactions_yield_a_range() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: Some(2),
+            end: Some(10),
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(Some(2..11), range);
+    }
+
+    #[test]
+    fn db_with_last_transaction_block_number_right_at_the_end_of_the_last_block_range_yield_none() {
+        // Reminder: the end of the block range is exclusive
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: Some(10),
+            end: Some(9),
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(None, range);
+    }
+
+    #[test]
+    fn db_with_last_transaction_block_number_below_the_end_of_the_last_block_range_yield_an_error()
+    {
+        // Reminder: the end of the block range is exclusive
+        let res = IntervalWithoutBlockRangeRootRecord {
+            start: Some(10),
+            end: Some(8),
+        }
+        .to_range()
+        .unwrap_err();
+        assert!(
+            format!("{res:?}").contains("Inconsistent state"),
+            "Expected 'Inconsistent state' error, got {res:?}",
+        );
+    }
+}
diff --git a/mithril-aggregator/src/database/repository/cardano_transaction_repository.rs b/mithril-aggregator/src/database/repository/cardano_transaction_repository.rs
@@ -1,7 +1,7 @@
 use std::ops::Range;
 use std::sync::Arc;
 
-use anyhow::{anyhow, Context};
+use anyhow::Context;
 use async_trait::async_trait;
 
 use mithril_common::crypto_helper::MKTreeNode;
@@ -208,18 +208,7 @@ impl TransactionStore for CardanoTransactionRepository {
         match row {
             // Should be impossible - the request as written in the provider always returns a single row
             None => panic!("IntervalWithoutBlockRangeProvider should always return a single row"),
-            Some(interval) => match (interval.start, interval.end) {
-                (_, None) => Ok(None),
-                (None, Some(end)) => Ok(Some(0..(end + 1))),
-                (Some(start), Some(end)) if end < start => {
-                    Err(anyhow!(
-                        "Inconsistent state: \
-                        Last block range root block number ({start}) is higher than the last transaction block number ({end}). \
-                        Did you forgot to prune obsolete `block_range_root` after a transaction rollback ?"
-                    ))
-                }
-                (Some(start), Some(end)) => Ok(Some(start..(end + 1))),
-            },
+            Some(interval) => interval.to_range(),
         }
     }
 
@@ -573,14 +562,15 @@ mod tests {
         let connection = Arc::new(cardano_tx_db_connection().unwrap());
         let repository = CardanoTransactionRepository::new(connection);
 
-        {
-            let interval = repository
-                .get_block_interval_without_block_range_root()
-                .await
-                .unwrap();
-
-            assert_eq!(None, interval);
-        }
+        // The last block range give the lower bound
+        let last_block_range = BlockRange::from_block_number(0);
+        repository
+            .store_block_range_roots(vec![(
+                last_block_range.clone(),
+                MKTreeNode::from_hex("AAAA").unwrap(),
+            )])
+            .await
+            .unwrap();
 
         // The last transaction block number give the upper bound
         let last_transaction_block_number = BlockRange::LENGTH * 4;
@@ -589,78 +579,15 @@ mod tests {
             .await
             .unwrap();
 
-        {
-            let interval = repository
-                .get_block_interval_without_block_range_root()
-                .await
-                .unwrap();
-
-            assert_eq!(Some(0..(last_transaction_block_number + 1)), interval);
-        }
-        {
-            // The last block range give the lower bound
-            let last_block_range = BlockRange::from_block_number(0);
-            repository
-                .store_block_range_roots(vec![(
-                    last_block_range.clone(),
-                    MKTreeNode::from_hex("AAAA").unwrap(),
-                )])
-                .await
-                .unwrap();
-
-            let interval = repository
-                .get_block_interval_without_block_range_root()
-                .await
-                .unwrap();
-
-            assert_eq!(
-                Some(last_block_range.end..(last_transaction_block_number + 1)),
-                interval
-            );
-        }
-    }
-
-    #[tokio::test]
-    async fn repository_get_block_interval_without_block_range_root_when_last_block_range_higher_than_stored_transactions(
-    ) {
-        let connection = Arc::new(cardano_tx_db_connection().unwrap());
-        let repository = CardanoTransactionRepository::new(connection);
-
-        let last_block_range = BlockRange::from_block_number(BlockRange::LENGTH * 10);
-        repository
-            .store_block_range_roots(vec![(
-                last_block_range.clone(),
-                MKTreeNode::from_hex("AAAA").unwrap(),
-            )])
+        let interval = repository
+            .get_block_interval_without_block_range_root()
             .await
             .unwrap();
 
-        // Only block range in db
-        {
-            let interval = repository
-                .get_block_interval_without_block_range_root()
-                .await
-                .unwrap();
-
-            assert_eq!(None, interval);
-        }
-        // Inconsistent state: Highest transaction block number is below the last computed block range
-        {
-            let last_transaction_block_number = last_block_range.end - 10;
-            repository
-                .create_transaction("tx-1", last_transaction_block_number, 50, "block-1", 99)
-                .await
-                .unwrap();
-
-            let res = repository
-                .get_block_interval_without_block_range_root()
-                .await;
-
-            assert!(
-                res.is_err(),
-                "Inconsistent state should raise an error, found:\n{res:?}"
-            );
-        }
+        assert_eq!(
+            Some(last_block_range.end..(last_transaction_block_number + 1)),
+            interval
+        );
     }
 
     #[tokio::test]
diff --git a/mithril-signer/src/database/record/interval_without_block_range_root.rs b/mithril-signer/src/database/record/interval_without_block_range_root.rs
@@ -1,18 +1,45 @@
+use std::ops::Range;
+
+use anyhow::anyhow;
 use sqlite::Row;
 
 use mithril_common::entities::BlockNumber;
+use mithril_common::StdResult;
 use mithril_persistence::sqlite::{HydrationError, Projection, SqLiteEntity};
 
 use crate::database::record::hydrator::try_to_u64;
 
 /// Interval of block numbers (`[start, end[`) without block ranges root.
 pub struct IntervalWithoutBlockRangeRootRecord {
-    /// Start of the interval
+    /// Start of the interval, fetched from the last block range root end
     pub start: Option<BlockNumber>,
-    /// End of the interval
+    /// End of the interval, fetched from the last transaction block number
     pub end: Option<BlockNumber>,
 }
 
+impl IntervalWithoutBlockRangeRootRecord {
+    /// Deduce from self the range of blocks that are available to compute block range roots.
+    pub fn to_range(&self) -> StdResult<Option<Range<BlockNumber>>> {
+        match (self.start, self.end) {
+            // No transactions stored - nothing can be computed
+            (_, None) => Ok(None),
+            // Transactions stored but no block range root - everything can be computed
+            (None, Some(end)) => Ok(Some(0..(end + 1))),
+            // Last stored transactions is at the end of the last block range - nothing to compute
+            (Some(start), Some(end)) if (end + 1) == start => Ok(None),
+            (Some(start), Some(end)) if end < start => {
+                Err(anyhow!(
+                        "Inconsistent state: \
+                        Last block range root block number ({start}) is higher than the last transaction block number ({end}). \
+                        Did you forgot to prune obsolete `block_range_root` after a transaction rollback ?"
+                    ))
+            }
+            // Nominal case
+            (Some(start), Some(end)) => Ok(Some(start..(end + 1))),
+        }
+    }
+}
+
 impl SqLiteEntity for IntervalWithoutBlockRangeRootRecord {
     fn hydrate(row: Row) -> Result<Self, HydrationError>
     where
@@ -37,3 +64,80 @@ impl SqLiteEntity for IntervalWithoutBlockRangeRootRecord {
         ])
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn db_without_block_range_nor_transactions_yield_none() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: None,
+            end: None,
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(None, range);
+    }
+
+    #[test]
+    fn db_with_only_transactions_yield_a_range() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: None,
+            end: Some(10),
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(Some(0..11), range);
+    }
+
+    #[test]
+    fn db_with_only_block_range_and_no_transactions_yield_none() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: Some(10),
+            end: None,
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(None, range);
+    }
+
+    #[test]
+    fn db_with_both_block_range_and_transactions_yield_a_range() {
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: Some(2),
+            end: Some(10),
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(Some(2..11), range);
+    }
+
+    #[test]
+    fn db_with_last_transaction_block_number_right_at_the_end_of_the_last_block_range_yield_none() {
+        // Reminder: the end of the block range is exclusive
+        let range = IntervalWithoutBlockRangeRootRecord {
+            start: Some(10),
+            end: Some(9),
+        }
+        .to_range()
+        .unwrap();
+        assert_eq!(None, range);
+    }
+
+    #[test]
+    fn db_with_last_transaction_block_number_below_the_end_of_the_last_block_range_yield_an_error()
+    {
+        // Reminder: the end of the block range is exclusive
+        let res = IntervalWithoutBlockRangeRootRecord {
+            start: Some(10),
+            end: Some(8),
+        }
+        .to_range()
+        .unwrap_err();
+        assert!(
+            format!("{res:?}").contains("Inconsistent state"),
+            "Expected 'Inconsistent state' error, got {res:?}",
+        );
+    }
+}
diff --git a/mithril-signer/src/database/repository/cardano_transaction_repository.rs b/mithril-signer/src/database/repository/cardano_transaction_repository.rs
