From 8f1bb6d325c8f364b35e57e54e68b10018ff1616 Mon Sep 17 00:00:00 2001 From: Sital Kedia Date: Fri, 13 May 2022 21:57:37 -0700 Subject: [PATCH] [Storage] Remove Accumulator consistency proof from state proof Closes: #996 --- consensus/src/persistent_liveness_storage.rs | 2 +- .../src/integration_test_impl.rs | 14 ++-- .../executor/tests/db_bootstrapper_test.rs | 33 ++------- .../tests/storage_integration_test.rs | 8 +-- storage/aptosdb/src/lib.rs | 29 +------- types/src/state_proof.rs | 37 ++-------- types/src/trusted_state.rs | 67 +------------------ types/src/unit_tests/trusted_state_test.rs | 59 +++++----------- 8 files changed, 33 insertions(+), 216 deletions(-) diff --git a/consensus/src/persistent_liveness_storage.rs b/consensus/src/persistent_liveness_storage.rs index d67da03d65dc8..5844b8cc7f2d7 100644 --- a/consensus/src/persistent_liveness_storage.rs +++ b/consensus/src/persistent_liveness_storage.rs @@ -483,7 +483,7 @@ impl PersistentLivenessStorage for StorageWriteProxy { } fn retrieve_epoch_change_proof(&self, version: u64) -> Result { - let (_, proofs, _) = self + let (_, proofs) = self .aptos_db .get_state_proof(version) .map_err(DbError::from)? diff --git a/execution/executor-test-helpers/src/integration_test_impl.rs b/execution/executor-test-helpers/src/integration_test_impl.rs index af26488fa5a7e..86ce79703259e 100644 --- a/execution/executor-test-helpers/src/integration_test_impl.rs +++ b/execution/executor-test-helpers/src/integration_test_impl.rs @@ -168,14 +168,12 @@ pub fn test_execution_with_storage_impl() -> Arc { .commit_blocks(vec![block1_id], ledger_info_with_sigs) .unwrap(); - let initial_accumulator = db.reader.get_accumulator_summary(0).unwrap(); let state_proof = db.reader.get_state_proof(0).unwrap(); let trusted_state = TrustedState::from_epoch_waypoint(waypoint); - let trusted_state = - match trusted_state.verify_and_ratchet(&state_proof, Some(&initial_accumulator)) { - Ok(TrustedStateChange::Epoch { new_state, .. }) => new_state, - _ => panic!("unexpected state change"), - }; + let trusted_state = match trusted_state.verify_and_ratchet(&state_proof) { + Ok(TrustedStateChange::Epoch { new_state, .. }) => new_state, + _ => panic!("unexpected state change"), + }; let current_version = state_proof.latest_ledger_info().version(); assert_eq!(trusted_state.version(), 9); @@ -354,9 +352,7 @@ pub fn test_execution_with_storage_impl() -> Arc { .unwrap(); let state_proof = db.reader.get_state_proof(trusted_state.version()).unwrap(); - let trusted_state_change = trusted_state - .verify_and_ratchet(&state_proof, None) - .unwrap(); + let trusted_state_change = trusted_state.verify_and_ratchet(&state_proof).unwrap(); assert!(matches!( trusted_state_change, TrustedStateChange::Version { .. } diff --git a/execution/executor/tests/db_bootstrapper_test.rs b/execution/executor/tests/db_bootstrapper_test.rs index 05f9145d50de6..38bc2cd56e040 100644 --- a/execution/executor/tests/db_bootstrapper_test.rs +++ b/execution/executor/tests/db_bootstrapper_test.rs @@ -73,18 +73,12 @@ fn test_empty_db() { waypoint ); - let initial_accumulator = db_rw - .reader - .get_accumulator_summary(waypoint.version()) - .unwrap(); let trusted_state = TrustedState::from_epoch_waypoint(waypoint); let state_proof = db_rw .reader .get_state_proof(trusted_state.version()) .unwrap(); - let trusted_state_change = trusted_state - .verify_and_ratchet(&state_proof, Some(&initial_accumulator)) - .unwrap(); + let trusted_state_change = trusted_state.verify_and_ratchet(&state_proof).unwrap(); assert!(trusted_state_change.is_epoch_change()); // `maybe_bootstrap()` does nothing on non-empty DB. @@ -310,17 +304,11 @@ fn test_pre_genesis() { assert!(maybe_bootstrap::(&db_rw, &genesis_txn, waypoint).unwrap()); let trusted_state = TrustedState::from_epoch_waypoint(waypoint); - let initial_accumulator = db_rw - .reader - .get_accumulator_summary(trusted_state.version()) - .unwrap(); let state_proof = db_rw .reader .get_state_proof(trusted_state.version()) .unwrap(); - let trusted_state_change = trusted_state - .verify_and_ratchet(&state_proof, Some(&initial_accumulator)) - .unwrap(); + let trusted_state_change = trusted_state.verify_and_ratchet(&state_proof).unwrap(); assert!(trusted_state_change.is_epoch_change()); // Effect of bootstrapping reflected. @@ -351,14 +339,8 @@ fn test_new_genesis() { assert_eq!(get_balance(&account2, &db), 2_000_000); let trusted_state = TrustedState::from_epoch_waypoint(waypoint); - let initial_accumulator = db - .reader - .get_accumulator_summary(trusted_state.version()) - .unwrap(); let state_proof = db.reader.get_state_proof(trusted_state.version()).unwrap(); - let trusted_state_change = trusted_state - .verify_and_ratchet(&state_proof, Some(&initial_accumulator)) - .unwrap(); + let trusted_state_change = trusted_state.verify_and_ratchet(&state_proof).unwrap(); assert!(trusted_state_change.is_epoch_change()); // New genesis transaction: set validator set, bump epoch and overwrite account1 balance. @@ -398,18 +380,11 @@ fn test_new_genesis() { // Client bootable from waypoint. let trusted_state = TrustedState::from_epoch_waypoint(waypoint); - let initial_accumulator = db - .reader - .get_accumulator_summary(trusted_state.version()) - .unwrap(); let state_proof = db.reader.get_state_proof(trusted_state.version()).unwrap(); - let trusted_state_change = trusted_state - .verify_and_ratchet(&state_proof, Some(&initial_accumulator)) - .unwrap(); + let trusted_state_change = trusted_state.verify_and_ratchet(&state_proof).unwrap(); assert!(trusted_state_change.is_epoch_change()); let trusted_state = trusted_state_change.new_state().unwrap(); assert_eq!(trusted_state.version(), 5); - assert!(state_proof.consistency_proof().is_empty()); // Effect of bootstrapping reflected. assert_eq!(get_balance(&account1, &db), 1_000_000); diff --git a/execution/executor/tests/storage_integration_test.rs b/execution/executor/tests/storage_integration_test.rs index 67b0e6b22e244..4687bb9d8c7f4 100644 --- a/execution/executor/tests/storage_integration_test.rs +++ b/execution/executor/tests/storage_integration_test.rs @@ -32,15 +32,9 @@ fn test_genesis() { let (_, db, _executor, waypoint) = create_db_and_executor(path.path(), &genesis); let trusted_state = TrustedState::from_epoch_waypoint(waypoint); - let initial_accumulator = db - .reader - .get_accumulator_summary(trusted_state.version()) - .unwrap(); let state_proof = db.reader.get_state_proof(trusted_state.version()).unwrap(); - trusted_state - .verify_and_ratchet(&state_proof, Some(&initial_accumulator)) - .unwrap(); + trusted_state.verify_and_ratchet(&state_proof).unwrap(); let li = state_proof.latest_ledger_info(); assert_eq!(li.version(), 0); diff --git a/storage/aptosdb/src/lib.rs b/storage/aptosdb/src/lib.rs index f1f5bd0a01cde..8f52383f24e7d 100644 --- a/storage/aptosdb/src/lib.rs +++ b/storage/aptosdb/src/lib.rs @@ -47,7 +47,7 @@ use crate::{ system_store::SystemStore, transaction_store::TransactionStore, }; -use anyhow::{ensure, format_err, Result}; +use anyhow::{ensure, Result}; use aptos_config::config::{RocksdbConfig, StoragePrunerConfig, NO_OP_STORAGE_PRUNER_CONFIG}; use aptos_crypto::hash::{HashValue, SPARSE_MERKLE_PLACEHOLDER_HASH}; use aptos_infallible::Mutex; @@ -955,32 +955,7 @@ impl DbReader for AptosDB { EpochChangeProof::new(vec![], /* more = */ false) }; - // Only return a consistency proof up to the verifiable end LI. If a - // client still needs to sync more epoch change LI's, then they cannot - // verify the latest LI nor verify a consistency proof up to the latest - // LI. If the client needs more epochs, we just return the consistency - // proof up to the last epoch change LI. - let verifiable_li = if epoch_change_proof.more { - epoch_change_proof - .ledger_info_with_sigs - .last() - .ok_or_else(|| format_err!( - "No epoch changes despite claiming the client needs to sync more epochs: known_epoch={}, end_epoch={}", - known_epoch, end_epoch, - ))? - .ledger_info() - } else { - ledger_info - }; - - let consistency_proof = self - .ledger_store - .get_consistency_proof(Some(known_version), verifiable_li.version())?; - Ok(StateProof::new( - ledger_info_with_sigs, - epoch_change_proof, - consistency_proof, - )) + Ok(StateProof::new(ledger_info_with_sigs, epoch_change_proof)) }) } diff --git a/types/src/state_proof.rs b/types/src/state_proof.rs index 70c9a1e58d68b..3e30071d3840b 100644 --- a/types/src/state_proof.rs +++ b/types/src/state_proof.rs @@ -4,7 +4,6 @@ use crate::{ epoch_change::EpochChangeProof, ledger_info::{LedgerInfo, LedgerInfoWithSignatures}, - proof::AccumulatorConsistencyProof, }; #[cfg(any(test, feature = "fuzzing"))] use proptest_derive::Arbitrary; @@ -24,48 +23,25 @@ use serde::{Deserialize, Serialize}; pub struct StateProof { latest_li_w_sigs: LedgerInfoWithSignatures, epoch_changes: EpochChangeProof, - consistency_proof: AccumulatorConsistencyProof, } impl StateProof { pub fn new( latest_li_w_sigs: LedgerInfoWithSignatures, epoch_changes: EpochChangeProof, - consistency_proof: AccumulatorConsistencyProof, ) -> Self { Self { latest_li_w_sigs, epoch_changes, - consistency_proof, } } - pub fn into_inner( - self, - ) -> ( - LedgerInfoWithSignatures, - EpochChangeProof, - AccumulatorConsistencyProof, - ) { - ( - self.latest_li_w_sigs, - self.epoch_changes, - self.consistency_proof, - ) + pub fn into_inner(self) -> (LedgerInfoWithSignatures, EpochChangeProof) { + (self.latest_li_w_sigs, self.epoch_changes) } - pub fn as_inner( - &self, - ) -> ( - &LedgerInfoWithSignatures, - &EpochChangeProof, - &AccumulatorConsistencyProof, - ) { - ( - &self.latest_li_w_sigs, - &self.epoch_changes, - &self.consistency_proof, - ) + pub fn as_inner(&self) -> (&LedgerInfoWithSignatures, &EpochChangeProof) { + (&self.latest_li_w_sigs, &self.epoch_changes) } #[inline] @@ -82,11 +58,6 @@ impl StateProof { pub fn epoch_changes(&self) -> &EpochChangeProof { &self.epoch_changes } - - #[inline] - pub fn consistency_proof(&self) -> &AccumulatorConsistencyProof { - &self.consistency_proof - } } #[cfg(test)] diff --git a/types/src/trusted_state.rs b/types/src/trusted_state.rs index b3aa1d8052fb5..1a0a6073cf663 100644 --- a/types/src/trusted_state.rs +++ b/types/src/trusted_state.rs @@ -5,13 +5,12 @@ use crate::{ epoch_change::{EpochChangeProof, Verifier}, epoch_state::EpochState, ledger_info::{LedgerInfo, LedgerInfoWithSignatures}, - proof::{AccumulatorConsistencyProof, TransactionAccumulatorSummary}, + proof::TransactionAccumulatorSummary, state_proof::StateProof, transaction::Version, waypoint::Waypoint, }; use anyhow::{bail, ensure, format_err, Result}; -use aptos_crypto::HashValue; use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; #[cfg(any(test, feature = "fuzzing"))] use proptest_derive::Arbitrary; @@ -35,12 +34,6 @@ pub enum TrustedState { waypoint: Waypoint, /// The current epoch and validator set inside that epoch. epoch_state: EpochState, - /// The current verified view of the transaction accumulator. Note that this - /// is not the complete accumulator; rather, it is a summary containing only - /// the frozen subtrees at the currently verified state version. We use the - /// accumulator summary to verify accumulator consistency proofs when - /// applying state proofs. - accumulator: TransactionAccumulatorSummary, }, } @@ -91,7 +84,6 @@ impl TrustedState { Ok(Self::EpochState { waypoint: Waypoint::new_epoch_boundary(epoch_change_li)?, epoch_state, - accumulator, }) } @@ -110,24 +102,6 @@ impl TrustedState { } } - pub fn accumulator_root_hash(&self) -> Option { - match self { - Self::EpochWaypoint(_) => None, - Self::EpochState { accumulator, .. } => Some(accumulator.root_hash()), - } - } - - pub fn accumulator_summary(&self) -> Option<&TransactionAccumulatorSummary> { - match self { - Self::EpochWaypoint(_) => None, - Self::EpochState { accumulator, .. } => Some(accumulator), - } - } - - pub fn need_accumulator(&self) -> bool { - self.accumulator_summary().is_none() - } - /// Verify and ratchet forward our trusted state using an [`EpochChangeProof`] /// (that moves us into the latest epoch), a [`LedgerInfoWithSignatures`] /// inside that epoch, and an [`AccumulatorConsistencyProof`] from our current @@ -161,13 +135,10 @@ impl TrustedState { pub fn verify_and_ratchet<'a>( &self, state_proof: &'a StateProof, - initial_accumulator: Option<&'a TransactionAccumulatorSummary>, ) -> Result> { self.verify_and_ratchet_inner( state_proof.latest_ledger_info_w_sigs(), state_proof.epoch_changes(), - state_proof.consistency_proof(), - initial_accumulator, ) } @@ -175,8 +146,6 @@ impl TrustedState { &self, latest_li: &'a LedgerInfoWithSignatures, epoch_change_proof: &'a EpochChangeProof, - consistency_proof: &'a AccumulatorConsistencyProof, - initial_accumulator: Option<&'a TransactionAccumulatorSummary>, ) -> Result> { // Abort early if the response is stale. let curr_version = self.version(); @@ -187,28 +156,6 @@ impl TrustedState { target_version, curr_version, ); - let curr_accumulator = match self { - // If we're verifying from an epoch waypoint, the user needs to provide - // an initial accumulator. Note that we assume this accumulator is - // untrusted. - Self::EpochWaypoint(_) => { - // When verifying from a waypoint, we need to check that the initial - // untrusted accumulator is consistent with the received waypoint - // ledger info. - let curr_accumulator = initial_accumulator - .expect("Client must provide an initial untrusted accumulator when verifying from a waypoint"); - let waypoint_li = epoch_change_proof - .ledger_info_with_sigs - .first() - .ok_or_else(|| format_err!("Empty epoch change proof"))? - .ledger_info(); - curr_accumulator.verify_consistency(waypoint_li)?; - curr_accumulator - } - // Otherwise, we should already have a _trusted_ accumulator. - Self::EpochState { accumulator, .. } => accumulator, - }; - if self.epoch_change_verification_required(latest_li.ledger_info().next_block_epoch()) { // Verify the EpochChangeProof to move us into the latest epoch. let epoch_change_li = epoch_change_proof.verify(self)?; @@ -238,15 +185,9 @@ impl TrustedState { }; let new_waypoint = Waypoint::new_any(verified_ledger_info.ledger_info()); - // Try to extend our accumulator summary and check that it's consistent - // with the target ledger info. - let new_accumulator = curr_accumulator - .try_extend_with_proof(consistency_proof, verified_ledger_info.ledger_info())?; - let new_state = TrustedState::EpochState { waypoint: new_waypoint, epoch_state: new_epoch_state, - accumulator: new_accumulator, }; Ok(TrustedStateChange::Epoch { @@ -279,15 +220,9 @@ impl TrustedState { // Verify the target ledger info, which should be inside the current epoch. curr_epoch_state.verify(latest_li)?; - // Try to extend our accumulator summary and check that it's consistent - // with the target ledger info. - let new_accumulator = curr_accumulator - .try_extend_with_proof(consistency_proof, latest_li.ledger_info())?; - let new_state = Self::EpochState { waypoint: new_waypoint, epoch_state: curr_epoch_state.clone(), - accumulator: new_accumulator, }; Ok(TrustedStateChange::Version { new_state }) diff --git a/types/src/unit_tests/trusted_state_test.rs b/types/src/unit_tests/trusted_state_test.rs index f9244cd5d8a97..faca3dc4f122b 100644 --- a/types/src/unit_tests/trusted_state_test.rs +++ b/types/src/unit_tests/trusted_state_test.rs @@ -251,7 +251,7 @@ proptest! { #[test] fn test_ratchet_from( - (_vsets, lis_with_sigs, latest_li, accumulator) in arb_update_proof( + (_vsets, lis_with_sigs, latest_li, _) in arb_update_proof( 10, /* start epoch */ 123, /* start version */ 1..3, /* version delta */ @@ -270,14 +270,9 @@ proptest! { .as_ref() .and_then(|li_with_sigs| li_with_sigs.ledger_info().next_epoch_state()); - let initial_accumulator = accumulator.get_accumulator_summary(trusted_state.version()); let change_proof = EpochChangeProof::new(lis_with_sigs, false /* more */); - let consistency_proof = accumulator.get_consistency_proof( - Some(trusted_state.version()), - expected_latest_version, - ); let trusted_state_change = trusted_state - .verify_and_ratchet_inner(&latest_li, &change_proof, &consistency_proof, Some(&initial_accumulator)) + .verify_and_ratchet_inner(&latest_li, &change_proof) .expect("Should never error or be stale when ratcheting from waypoint with valid proofs"); match trusted_state_change { @@ -315,12 +310,8 @@ proptest! { // Use an empty epoch change proof let change_proof = EpochChangeProof::new(vec![], false /* more */); - let consistency_proof = accumulator.get_consistency_proof( - Some(trusted_state.version()), - expected_latest_version, - ); let trusted_state_change = trusted_state - .verify_and_ratchet_inner(&latest_li, &change_proof, &consistency_proof, None) + .verify_and_ratchet_inner(&latest_li, &change_proof) .expect("Should never error or be stale when ratcheting from waypoint with valid proofs"); match trusted_state_change { @@ -357,13 +348,9 @@ proptest! { lis_with_sigs.remove(li_gap_idx); let change_proof = EpochChangeProof::new(lis_with_sigs, false /* more */); - let consistency_proof = accumulator.get_consistency_proof( - Some(trusted_state.version()), - latest_li.ledger_info().version(), - ); // should fail since there's a missing epoch change li in the change proof. trusted_state - .verify_and_ratchet_inner(&latest_li, &change_proof, &consistency_proof, None) + .verify_and_ratchet_inner(&latest_li, &change_proof) .expect_err("Should always return Err with an invalid change proof"); } @@ -396,18 +383,14 @@ proptest! { // we're done syncing epoch changes but doesn't get us all the way to the // latest ledger info let mut change_proof = EpochChangeProof::new(lis_with_sigs, false /* more */); - let consistency_proof = accumulator.get_consistency_proof( - Some(trusted_state.version()), - expected_latest_version, - ); trusted_state - .verify_and_ratchet_inner(&latest_li, &change_proof, &consistency_proof, None) + .verify_and_ratchet_inner(&latest_li, &change_proof) .expect_err("Should return Err when more is false and there's a gap"); // ratcheting with more = true is fine change_proof.more = true; let trusted_state_change = trusted_state - .verify_and_ratchet_inner(&latest_li, &change_proof, &consistency_proof, None) + .verify_and_ratchet_inner(&latest_li, &change_proof) .expect("Should succeed with more in EpochChangeProof"); match trusted_state_change { @@ -440,11 +423,6 @@ proptest! { accumulator.get_accumulator_summary(initial_li.version()), ).unwrap(); - let consistency_proof = accumulator.get_consistency_proof( - Some(trusted_state.version()), - latest_li.ledger_info().version(), - ); - // Swap in a bad ledger info without signatures let li_with_sigs = bad_li_idx.get(&lis_with_sigs); let bad_li_with_sigs = LedgerInfoWithSignatures::new( @@ -455,7 +433,7 @@ proptest! { let change_proof = EpochChangeProof::new(lis_with_sigs, false /* more */); trusted_state - .verify_and_ratchet_inner(&latest_li, &change_proof, &consistency_proof, None) + .verify_and_ratchet_inner(&latest_li, &change_proof) .expect_err("Should always return Err with an invalid change proof"); } @@ -478,10 +456,6 @@ proptest! { let good_li = latest_li.ledger_info(); let change_proof = EpochChangeProof::new(lis_with_sigs, false /* more */); - let consistency_proof = accumulator.get_consistency_proof( - Some(trusted_state.version()), - good_li.version(), - ); let sigs = latest_li.signatures(); // Verifying latest ledger infos with mismatched data and signatures should fail @@ -518,12 +492,12 @@ proptest! { *bad_sigs.values_mut().next().unwrap() = Ed25519Signature::dummy_signature(); let bad_li_6 = LedgerInfoWithSignatures::new(good_li.clone(), bad_sigs); - trusted_state.verify_and_ratchet_inner(&bad_li_1, &change_proof, &consistency_proof, None).unwrap_err(); - trusted_state.verify_and_ratchet_inner(&bad_li_2, &change_proof, &consistency_proof, None).unwrap_err(); - trusted_state.verify_and_ratchet_inner(&bad_li_3, &change_proof, &consistency_proof, None).unwrap_err(); - trusted_state.verify_and_ratchet_inner(&bad_li_4, &change_proof, &consistency_proof, None).unwrap_err(); - trusted_state.verify_and_ratchet_inner(&bad_li_5, &change_proof, &consistency_proof, None).unwrap_err(); - trusted_state.verify_and_ratchet_inner(&bad_li_6, &change_proof, &consistency_proof, None).unwrap_err(); + trusted_state.verify_and_ratchet_inner(&bad_li_1, &change_proof).unwrap_err(); + trusted_state.verify_and_ratchet_inner(&bad_li_2, &change_proof).unwrap_err(); + trusted_state.verify_and_ratchet_inner(&bad_li_3, &change_proof).unwrap_err(); + trusted_state.verify_and_ratchet_inner(&bad_li_4, &change_proof).unwrap_err(); + trusted_state.verify_and_ratchet_inner(&bad_li_5, &change_proof).unwrap_err(); + trusted_state.verify_and_ratchet_inner(&bad_li_6, &change_proof).unwrap_err(); } } @@ -532,7 +506,7 @@ proptest! { #[test] fn test_stale_ratchet( - (_vsets, lis_with_sigs, latest_li, accumulator) in arb_update_proof( + (_vsets, lis_with_sigs, latest_li, _) in arb_update_proof( 1, /* start epoch */ 1, /* start version */ 1..3, /* version delta */ @@ -551,12 +525,9 @@ proptest! { future_accumulator.get_accumulator_summary(future_version), ).unwrap(); - let start_version = lis_with_sigs.first().unwrap().ledger_info().version(); - let end_version = latest_li.ledger_info().version(); let change_proof = EpochChangeProof::new(lis_with_sigs, false /* more */); - let consistency_proof = accumulator.get_consistency_proof(Some(start_version), end_version); trusted_state - .verify_and_ratchet_inner(&latest_li, &change_proof, &consistency_proof, None) + .verify_and_ratchet_inner(&latest_li, &change_proof) .expect_err("Expected stale change, got valid change"); } }