Merge pull request #22 from movementlabsxyz/mikhail/ledger-info-on-re…

…vert

Make `LedgerInfo` the source of truth on revert

storage/aptosdb/src/db/aptosdb_test.rs

@@ -246,7 +246,7 @@ fn create_signed_transaction(gas_unit_price: u64) -> SignedTransaction {
 }
 
 #[test]
-fn test_revert_last_commit() {
+fn test_revert_single_commit() {
     aptos_logger::Logger::new().init();
 
     let tmp_dir = TempPath::new();
@@ -276,25 +276,13 @@ fn test_revert_last_commit() {
     assert_eq!(db.get_latest_version().unwrap(), expected_version);
 
     // Get the latest ledger info before revert
-    let latest_ledger_info_before_revert = db.get_latest_ledger_info().unwrap();
-    let root_hash = db
-        .get_latest_ledger_info()
-        .unwrap()
-        .ledger_info()
-        .commit_info()
-        .executed_state_id();
+    let latest_ledger_info_before_revert = blocks[1].1.clone();
+    let version_to_revert_to = latest_ledger_info_before_revert.commit_info().version();
 
     // Revert the last commit
-    db.revert_commit(
-        db.get_latest_version().unwrap(),
-        db.get_latest_version().unwrap(), // In this case the last commit and version to commit are same
-        root_hash,                        // the hash is also the lastest
-        latest_ledger_info_before_revert,
-    )
-    .unwrap();
+    db.revert_commit(&latest_ledger_info_before_revert).unwrap();
 
-    let exepcted_version = cur_ver - 2;
-    assert_eq!(db.get_latest_version().unwrap(), exepcted_version);
+    assert_eq!(db.get_latest_version().unwrap(), version_to_revert_to);
 }
 
 #[test]
@@ -313,7 +301,6 @@ fn test_revert_nth_commit() {
 
     #[derive(Debug)]
     struct Commit {
-        hash: HashValue,
         info: LedgerInfoWithSignatures,
         first_version: Version,
     }
@@ -323,7 +310,6 @@ fn test_revert_nth_commit() {
     let mut blockheight = 0;
 
     for (txns_to_commit, ledger_info_with_sigs) in &blocks {
-        println!("Blockheight: {}", blockheight);
         let first_version = cur_ver;
         update_in_memory_state(&mut in_memory_state, txns_to_commit.as_slice());
         db.save_transactions_for_test(
@@ -339,7 +325,6 @@ fn test_revert_nth_commit() {
         committed_blocks.insert(
             blockheight,
             Commit {
-                hash: ledger_info_with_sigs.commit_info().executed_state_id(),
                 info: ledger_info_with_sigs.clone(),
                 first_version,
             },
@@ -356,61 +341,16 @@ fn test_revert_nth_commit() {
     // Get the 3rd block back from the latest block
     let revert_block_num = blockheight - 3;
     let revert = committed_blocks.get(&revert_block_num).unwrap();
+    let pre_revert_ledger_info = committed_blocks[&(revert_block_num - 1)].info.clone();
 
     // Get the version to revert to
-    let version_to_revert = revert.first_version - 1;
+    let version_to_revert = revert.first_version;
 
-    db.revert_commit(
-        version_to_revert,
-        db.get_latest_version().unwrap(),
-        revert.hash.clone(),
-        revert.info.clone(),
-    )
-    .unwrap();
+    db.revert_commit(&pre_revert_ledger_info).unwrap();
 
     assert_eq!(db.get_latest_version().unwrap(), version_to_revert - 1);
 }
 
-#[test]
-fn test_revert_commit_should_fail_with_wrong_hash() {
-    aptos_logger::Logger::new().init();
-
-    let tmp_dir = TempPath::new();
-    let db = AptosDB::new_for_test(&tmp_dir);
-
-    let mut cur_ver: Version = 0;
-    let mut in_memory_state = db.buffered_state().lock().current_state().clone();
-    let _ancestor = in_memory_state.base.clone();
-    let mut val_generator = ValueGenerator::new();
-    let blocks = val_generator.generate(arb_blocks_to_commit());
-    for (txns_to_commit, ledger_info_with_sigs) in &blocks {
-        update_in_memory_state(&mut in_memory_state, txns_to_commit.as_slice());
-        db.save_transactions_for_test(
-            txns_to_commit,
-            cur_ver, /* first_version */
-            cur_ver.checked_sub(1),
-            Some(ledger_info_with_sigs),
-            true, /* sync_commit */
-            in_memory_state.clone(),
-        )
-        .unwrap();
-        cur_ver += txns_to_commit.len() as u64;
-    }
-
-    // Get the latest ledger info before revert
-    let latest_ledger_info_before_revert = db.get_latest_ledger_info().unwrap();
-    let last_committed_version = latest_ledger_info_before_revert.ledger_info().version();
-
-    // Revert the last commit
-    let result = db.revert_commit(
-        last_committed_version,
-        last_committed_version.clone(), // In this case the last commit and version to commit are the same
-        HashValue::random(),            // A wrong hash
-        latest_ledger_info_before_revert,
-    );
-    assert!(result.is_err());
-}
-
 pub fn test_state_merkle_pruning_impl(
     input: Vec<(Vec<TransactionToCommit>, LedgerInfoWithSignatures)>,
 ) {

storage/aptosdb/src/db/include/aptosdb_writer.rs

@@ -203,28 +203,30 @@ impl DbWriter for AptosDB {
     /// Revert a commit.
     fn revert_commit(
         &self,
-        version_to_revert: Version,
-        latest_version: Version,
-        new_root_hash: HashValue,
-        ledger_info_with_sigs: LedgerInfoWithSignatures,
+        ledger_info_with_sigs: &LedgerInfoWithSignatures,
     ) -> Result<()> {
         let _timer = OTHER_TIMERS_SECONDS
             .with_label_values(&["revert_commit"])
             .start_timer();
-        // Revert the ledger commit progress
+
+        let latest_version = self.get_latest_version()?;
+        let target_version = ledger_info_with_sigs.ledger_info().version();
+
         let ledger_batch = SchemaBatch::new();
+
+        // Revert the ledger commit progress
         ledger_batch.put::<DbMetadataSchema>(
             &DbMetadataKey::LedgerCommitProgress,
-            &DbMetadataValue::Version(version_to_revert - 1),
+            &DbMetadataValue::Version(target_version),
         )?;
-        self.ledger_db.metadata_db().write_schemas(ledger_batch)?;
 
         // Revert the overall commit progress
-        let ledger_batch = SchemaBatch::new();
         ledger_batch.put::<DbMetadataSchema>(
             &DbMetadataKey::OverallCommitProgress,
-            &DbMetadataValue::Version(version_to_revert - 1),
+            &DbMetadataValue::Version(target_version),
         )?;
+
+        // Write ledger metadata db changes
         self.ledger_db.metadata_db().write_schemas(ledger_batch)?;
 
         let temp_position = Position::from_postorder_index(latest_version)?;
@@ -233,67 +235,54 @@ impl DbWriter for AptosDB {
         let batch = SchemaBatch::new();
         self.ledger_db
             .transaction_accumulator_db()
-            .revert_transaction_accumulator(version_to_revert - 1, &batch, temp_position)?;
+            .revert_transaction_accumulator(target_version, &batch, temp_position)?;
         self.ledger_db
             .transaction_accumulator_db()
             .write_schemas(batch)?;
 
         // Revert the transaction info
+        // FIXME: need to delete the range to current latest?
         let batch = SchemaBatch::new();
         self.ledger_db
             .transaction_info_db()
-            .delete_transaction_info(version_to_revert, &batch)?;
+            .delete_transaction_info(target_version + 1, &batch)?;
         let batch = SchemaBatch::new();
         self.ledger_db.transaction_info_db().write_schemas(batch)?;
 
         // Revert the events
+        // FIXME: need to delete the range to current latest?
         let batch = SchemaBatch::new();
         self.ledger_db
             .event_db()
-            .delete_events(version_to_revert, &batch)?;
+            .delete_events(target_version + 1, &batch)?;
         self.ledger_db.event_db().write_schemas(batch)?;
 
         // Revert the transaction auxiliary data
         let batch = SchemaBatch::new();
-        TransactionAuxiliaryDataDb::prune(version_to_revert - 1, latest_version, &batch)?;
+        TransactionAuxiliaryDataDb::prune(target_version, latest_version, &batch)?;
         let batch = SchemaBatch::new();
         self.ledger_db
             .transaction_auxiliary_data_db()
             .write_schemas(batch)?;
 
         // Revert the write set
         let batch = SchemaBatch::new();
-        WriteSetDb::prune(version_to_revert - 1, latest_version, &batch)?;
+        WriteSetDb::prune(target_version, latest_version, &batch)?;
         self.ledger_db.transaction_db().prune_transactions(
-            version_to_revert - 1,
+            target_version,
             latest_version,
             &batch,
         )?;
 
         // Revert the state kv and ledger metadata
         self.state_store
             .state_kv_db
-            .revert_state_kv_and_ledger_metadata(version_to_revert)?;
-
-        // Get the epoch of the version_to_revert
-        let target_epoch = self.ledger_db.metadata_db().get_epoch(version_to_revert)?;
-
-        // Set the epoch to the target epoch
-        ledger_info_with_sigs
-            .ledger_info()
-            .commit_info()
-            .to_owned()
-            .set_epoch(target_epoch);
-
-        //Set the Version
-        ledger_info_with_sigs
-            .ledger_info()
-            .to_owned()
-            .set_version(version_to_revert - 1);
+            .revert_state_kv_and_ledger_metadata(target_version)?;
 
-        // Update the latest ledger info if provided
+        // Update the provided ledger info
+        let new_root_hash = ledger_info_with_sigs.commit_info().executed_state_id();
         self.commit_ledger_info(
-            version_to_revert - 1,
+            target_version,
             new_root_hash,
             Some(&ledger_info_with_sigs),
         )?;

storage/storage-interface/src/lib.rs

@@ -580,13 +580,10 @@ pub trait DbWriter: Send + Sync {
         unimplemented!()
     }
 
-    /// Revert a commit.
+    /// Revert to committed state referred to with the ledger information.
     fn revert_commit(
         &self,
-        version_to_revert: Version,
-        latest_version: Version,
-        new_root_hash: HashValue,
-        ledger_info_with_sigs: LedgerInfoWithSignatures,
+        ledger_info_with_sigs: &LedgerInfoWithSignatures,
     ) -> Result<()> {
         unimplemented!()
     }