From bd2d1beb9f738e29cac1ac6396946a9c946d6dcd Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Thu, 20 Jun 2024 15:38:14 +0300 Subject: [PATCH 1/3] aptos-db: revert_commit improvements Pass LedgerInfoWithSignatures by reference like in other methods. Do not try to set epoch and version on the LedgerInfo in a way that does not work. Instead, it's expected to be correct for the latest pre-revert version. --- storage/aptosdb/src/db/aptosdb_test.rs | 26 ++++++------ .../aptosdb/src/db/include/aptosdb_writer.rs | 42 ++++++++----------- storage/storage-interface/src/lib.rs | 2 +- 3 files changed, 30 insertions(+), 40 deletions(-) diff --git a/storage/aptosdb/src/db/aptosdb_test.rs b/storage/aptosdb/src/db/aptosdb_test.rs index 669dcde57ef12..11ca3a552eea2 100644 --- a/storage/aptosdb/src/db/aptosdb_test.rs +++ b/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,23 @@ 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() + let latest_ledger_info_before_revert = blocks[1].1.clone(); + let root_hash = latest_ledger_info_before_revert .ledger_info() .commit_info() .executed_state_id(); + let version_to_revert = latest_ledger_info_before_revert.ledger_info().version() + 1; // Revert the last commit db.revert_commit( - db.get_latest_version().unwrap(), + version_to_revert, 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, + &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 - 1); } #[test] @@ -323,7 +321,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( @@ -356,15 +353,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(), + pre_revert_ledger_info.commit_info().executed_state_id(), + &pre_revert_ledger_info, ) .unwrap(); @@ -406,7 +404,7 @@ fn test_revert_commit_should_fail_with_wrong_hash() { 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, + &latest_ledger_info_before_revert, ); assert!(result.is_err()); } diff --git a/storage/aptosdb/src/db/include/aptosdb_writer.rs b/storage/aptosdb/src/db/include/aptosdb_writer.rs index 80d8b94f0744a..029cda0d04cc4 100644 --- a/storage/aptosdb/src/db/include/aptosdb_writer.rs +++ b/storage/aptosdb/src/db/include/aptosdb_writer.rs @@ -206,16 +206,24 @@ impl DbWriter for AptosDB { 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(); + + let target_version = version_to_revert.checked_sub(1).expect("cannot revert genesis"); + + ensure!( + ledger_info_with_sigs.ledger_info().version() == target_version, + "LedgerInfo is not for version {}", target_version, + ); + // Revert the ledger commit progress let ledger_batch = SchemaBatch::new(); ledger_batch.put::( &DbMetadataKey::LedgerCommitProgress, - &DbMetadataValue::Version(version_to_revert - 1), + &DbMetadataValue::Version(target_version), )?; self.ledger_db.metadata_db().write_schemas(ledger_batch)?; @@ -223,7 +231,7 @@ impl DbWriter for AptosDB { let ledger_batch = SchemaBatch::new(); ledger_batch.put::( &DbMetadataKey::OverallCommitProgress, - &DbMetadataValue::Version(version_to_revert - 1), + &DbMetadataValue::Version(target_version), )?; self.ledger_db.metadata_db().write_schemas(ledger_batch)?; @@ -233,7 +241,7 @@ 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)?; @@ -255,7 +263,7 @@ impl DbWriter for AptosDB { // 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() @@ -263,9 +271,9 @@ impl DbWriter for AptosDB { // 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, )?; @@ -275,25 +283,9 @@ impl DbWriter for AptosDB { .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); - - // Update the latest ledger info if provided + // Update the provided ledger info self.commit_ledger_info( - version_to_revert - 1, + target_version, new_root_hash, Some(&ledger_info_with_sigs), )?; diff --git a/storage/storage-interface/src/lib.rs b/storage/storage-interface/src/lib.rs index ee80b269eff46..c30cc4ac7d784 100644 --- a/storage/storage-interface/src/lib.rs +++ b/storage/storage-interface/src/lib.rs @@ -586,7 +586,7 @@ pub trait DbWriter: Send + Sync { version_to_revert: Version, latest_version: Version, new_root_hash: HashValue, - ledger_info_with_sigs: LedgerInfoWithSignatures, + ledger_info_with_sigs: &LedgerInfoWithSignatures, ) -> Result<()> { unimplemented!() } From c7c987cb1fccadac7baad4e5edf55240aa3004ca Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Thu, 20 Jun 2024 15:58:28 +0300 Subject: [PATCH 2/3] aptos-db: simplify signature of revert_commit All necessary information can be obtained from either the LedgerInfoWithSignatures, or the current database state. --- storage/aptosdb/src/db/aptosdb_test.rs | 66 ++----------------- .../aptosdb/src/db/include/aptosdb_writer.rs | 20 +++--- storage/storage-interface/src/lib.rs | 5 +- 3 files changed, 13 insertions(+), 78 deletions(-) diff --git a/storage/aptosdb/src/db/aptosdb_test.rs b/storage/aptosdb/src/db/aptosdb_test.rs index 11ca3a552eea2..f5846ecba677e 100644 --- a/storage/aptosdb/src/db/aptosdb_test.rs +++ b/storage/aptosdb/src/db/aptosdb_test.rs @@ -277,22 +277,12 @@ fn test_revert_single_commit() { // Get the latest ledger info before revert let latest_ledger_info_before_revert = blocks[1].1.clone(); - let root_hash = latest_ledger_info_before_revert - .ledger_info() - .commit_info() - .executed_state_id(); - let version_to_revert = latest_ledger_info_before_revert.ledger_info().version() + 1; + let version_to_revert_to = latest_ledger_info_before_revert.commit_info().version(); // Revert the last commit - db.revert_commit( - version_to_revert, - 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(); - assert_eq!(db.get_latest_version().unwrap(), version_to_revert - 1); + assert_eq!(db.get_latest_version().unwrap(), version_to_revert_to); } #[test] @@ -311,7 +301,6 @@ fn test_revert_nth_commit() { #[derive(Debug)] struct Commit { - hash: HashValue, info: LedgerInfoWithSignatures, first_version: Version, } @@ -336,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, }, @@ -358,57 +346,11 @@ fn test_revert_nth_commit() { // Get the version to revert to let version_to_revert = revert.first_version; - db.revert_commit( - version_to_revert, - db.get_latest_version().unwrap(), - pre_revert_ledger_info.commit_info().executed_state_id(), - &pre_revert_ledger_info, - ) - .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, LedgerInfoWithSignatures)>, ) { diff --git a/storage/aptosdb/src/db/include/aptosdb_writer.rs b/storage/aptosdb/src/db/include/aptosdb_writer.rs index 029cda0d04cc4..d7fd163af8cdc 100644 --- a/storage/aptosdb/src/db/include/aptosdb_writer.rs +++ b/storage/aptosdb/src/db/include/aptosdb_writer.rs @@ -203,21 +203,14 @@ 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, ) -> Result<()> { let _timer = OTHER_TIMERS_SECONDS .with_label_values(&["revert_commit"]) .start_timer(); - let target_version = version_to_revert.checked_sub(1).expect("cannot revert genesis"); - - ensure!( - ledger_info_with_sigs.ledger_info().version() == target_version, - "LedgerInfo is not for version {}", target_version, - ); + let latest_version = self.get_latest_version()?; + let target_version = ledger_info_with_sigs.ledger_info().version(); // Revert the ledger commit progress let ledger_batch = SchemaBatch::new(); @@ -247,18 +240,20 @@ impl DbWriter for AptosDB { .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 @@ -281,9 +276,10 @@ impl DbWriter for AptosDB { // Revert the state kv and ledger metadata self.state_store .state_kv_db - .revert_state_kv_and_ledger_metadata(version_to_revert)?; + .revert_state_kv_and_ledger_metadata(target_version)?; // Update the provided ledger info + let new_root_hash = ledger_info_with_sigs.commit_info().executed_state_id(); self.commit_ledger_info( target_version, new_root_hash, diff --git a/storage/storage-interface/src/lib.rs b/storage/storage-interface/src/lib.rs index c30cc4ac7d784..208ec1d3a7c82 100644 --- a/storage/storage-interface/src/lib.rs +++ b/storage/storage-interface/src/lib.rs @@ -580,12 +580,9 @@ 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, ) -> Result<()> { unimplemented!() From 6cb064032de8166c2a9d440cd608cd86432b8c80 Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Thu, 20 Jun 2024 16:01:32 +0300 Subject: [PATCH 3/3] aptos-db: fix excessive batching Update both progress values in the ledger metadata schema in one batch. --- storage/aptosdb/src/db/include/aptosdb_writer.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/storage/aptosdb/src/db/include/aptosdb_writer.rs b/storage/aptosdb/src/db/include/aptosdb_writer.rs index d7fd163af8cdc..40a59e4077d98 100644 --- a/storage/aptosdb/src/db/include/aptosdb_writer.rs +++ b/storage/aptosdb/src/db/include/aptosdb_writer.rs @@ -212,20 +212,21 @@ impl DbWriter for AptosDB { let latest_version = self.get_latest_version()?; let target_version = ledger_info_with_sigs.ledger_info().version(); - // Revert the ledger commit progress let ledger_batch = SchemaBatch::new(); + + // Revert the ledger commit progress ledger_batch.put::( &DbMetadataKey::LedgerCommitProgress, &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::( &DbMetadataKey::OverallCommitProgress, &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)?;