From f5a00140cbd6fe2bebc5394aa854457ad44e965b Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 18 Jun 2020 10:45:16 +0300 Subject: [PATCH] Forbid appending blocks to forks that are competing with finalized chain (#138) * Error::TryingToFinalizeSibling * cargo fmt --all * updated PruningStrategy comment --- bridges/modules/ethereum/src/error.rs | 3 + bridges/modules/ethereum/src/finality.rs | 90 ++++++++--- bridges/modules/ethereum/src/import.rs | 170 ++++++++++++++++----- bridges/modules/ethereum/src/lib.rs | 93 ++++++++--- bridges/primitives/ethereum-poa/src/lib.rs | 8 + 5 files changed, 296 insertions(+), 68 deletions(-) diff --git a/bridges/modules/ethereum/src/error.rs b/bridges/modules/ethereum/src/error.rs index 4c887a4a493e..0a9ca57f8c13 100644 --- a/bridges/modules/ethereum/src/error.rs +++ b/bridges/modules/ethereum/src/error.rs @@ -60,6 +60,8 @@ pub enum Error { TransactionsReceiptsMismatch = 18, /// Can't accept unsigned header from the far future. UnsignedTooFarInTheFuture = 19, + /// Trying to finalize sibling of finalized block. + TryingToFinalizeSibling = 20, } impl Error { @@ -85,6 +87,7 @@ impl Error { Error::RedundantTransactionsReceipts => "Redundant transactions receipts are provided", Error::TransactionsReceiptsMismatch => "Invalid transactions receipts provided", Error::UnsignedTooFarInTheFuture => "The unsigned header is too far in future", + Error::TryingToFinalizeSibling => "Trying to finalize sibling of finalized block", } } diff --git a/bridges/modules/ethereum/src/finality.rs b/bridges/modules/ethereum/src/finality.rs index 0fb5f1f77741..c7bfe67d640b 100644 --- a/bridges/modules/ethereum/src/finality.rs +++ b/bridges/modules/ethereum/src/finality.rs @@ -28,9 +28,13 @@ use sp_std::collections::{ use sp_std::prelude::*; /// Cached finality votes for given block. -#[derive(RuntimeDebug, Default)] +#[derive(RuntimeDebug)] #[cfg_attr(test, derive(PartialEq))] pub struct CachedFinalityVotes { + /// True if we have stopped at best finalized block' sibling. This means + /// that we are trying to finalize block from fork that has forked before + /// best finalized. + pub stopped_at_finalized_sibling: bool, /// Header ancestors that were read while we have been searching for /// cached votes entry. Newest header has index 0. pub unaccounted_ancestry: VecDeque<(HeaderId, Option, Header)>, @@ -88,10 +92,15 @@ pub fn finalize_blocks( // compute count of voters for every unfinalized block in ancestry let validators = header_validators.1.iter().collect(); let votes = prepare_votes( - storage.cached_finality_votes(&header.parent_hash, |hash| { - *hash == header_validators.0.hash || *hash == best_finalized.hash - }), - best_finalized.number, + header + .parent_id() + .map(|parent_id| { + storage.cached_finality_votes(&parent_id, &best_finalized, |hash| { + *hash == header_validators.0.hash || *hash == best_finalized.hash + }) + }) + .unwrap_or_else(|| CachedFinalityVotes::default()), + best_finalized, &validators, id, header, @@ -133,12 +142,18 @@ fn is_finalized( /// Prepare 'votes' of header and its ancestors' signers. fn prepare_votes( mut cached_votes: CachedFinalityVotes, - best_finalized_number: u64, + best_finalized: HeaderId, validators: &BTreeSet<&Address>, id: HeaderId, header: &Header, submitter: Option, ) -> Result, Error> { + // if we have reached finalized block sibling, then we're trying + // to switch finalized blocks + if cached_votes.stopped_at_finalized_sibling { + return Err(Error::TryingToFinalizeSibling); + } + // this fn can only work with single validators set if !validators.contains(&header.author) { return Err(Error::NotValidator); @@ -154,7 +169,7 @@ fn prepare_votes( // remove votes from finalized blocks while let Some(old_ancestor) = votes.ancestry.pop_front() { - if old_ancestor.id.number > best_finalized_number { + if old_ancestor.id.number > best_finalized.number { votes.ancestry.push_front(old_ancestor); break; } @@ -245,6 +260,16 @@ fn empty_step_signer(empty_step: &SealedEmptyStep, parent_hash: &H256) -> Option .map(|public| public_to_address(&public)) } +impl Default for CachedFinalityVotes { + fn default() -> Self { + CachedFinalityVotes { + stopped_at_finalized_sibling: false, + unaccounted_ancestry: VecDeque::new(), + votes: None, + } + } +} + impl Default for FinalityVotes { fn default() -> Self { FinalityVotes { @@ -307,7 +332,7 @@ mod tests { assert_eq!( finalize_blocks( &storage, - Default::default(), + genesis().compute_id(), (Default::default(), &validators_addresses(5)), id1, None, @@ -331,7 +356,7 @@ mod tests { assert_eq!( finalize_blocks( &storage, - Default::default(), + genesis().compute_id(), (Default::default(), &validators_addresses(5)), id2, None, @@ -355,7 +380,7 @@ mod tests { assert_eq!( finalize_blocks( &storage, - Default::default(), + genesis().compute_id(), (Default::default(), &validators_addresses(5)), id3, None, @@ -397,6 +422,7 @@ mod tests { assert_eq!( prepare_votes::<()>( CachedFinalityVotes { + stopped_at_finalized_sibling: false, unaccounted_ancestry: vec![(headers[3].compute_id(), None, headers[3].clone()),] .into_iter() .collect(), @@ -407,7 +433,7 @@ mod tests { ancestry: ancestry[..3].iter().cloned().collect(), }), }, - 2, + headers[1].compute_id(), &validators.iter().collect(), header5.compute_id(), &header5, @@ -471,8 +497,12 @@ mod tests { let id7 = headers[6].compute_id(); assert_eq!( prepare_votes( - storage.cached_finality_votes(&hashes.get(5).unwrap(), |_| false,), - 0, + storage.cached_finality_votes( + &headers.get(5).unwrap().compute_id(), + &genesis().compute_id(), + |_| false, + ), + Default::default(), &validators_addresses.iter().collect(), id7, headers.get(6).unwrap(), @@ -498,8 +528,12 @@ mod tests { // check that votes at #7 are computed correctly with cache assert_eq!( prepare_votes( - storage.cached_finality_votes(&hashes.get(5).unwrap(), |_| false,), - 0, + storage.cached_finality_votes( + &headers.get(5).unwrap().compute_id(), + &genesis().compute_id(), + |_| false, + ), + Default::default(), &validators_addresses.iter().collect(), id7, headers.get(6).unwrap(), @@ -522,8 +556,12 @@ mod tests { }; assert_eq!( prepare_votes( - storage.cached_finality_votes(&hashes.get(5).unwrap(), |hash| *hash == hashes[2],), - 3, + storage.cached_finality_votes( + &headers.get(5).unwrap().compute_id(), + &headers.get(2).unwrap().compute_id(), + |hash| *hash == hashes[2], + ), + headers[2].compute_id(), &validators_addresses.iter().collect(), id7, headers.get(6).unwrap(), @@ -534,4 +572,22 @@ mod tests { ); }); } + + #[test] + fn prepare_votes_fails_when_finalized_sibling_is_in_ancestry() { + assert_eq!( + prepare_votes::<()>( + CachedFinalityVotes { + stopped_at_finalized_sibling: true, + ..Default::default() + }, + Default::default(), + &validators_addresses(3).iter().collect(), + Default::default(), + &Default::default(), + None, + ), + Err(Error::TryingToFinalizeSibling), + ); + } } diff --git a/bridges/modules/ethereum/src/import.rs b/bridges/modules/ethereum/src/import.rs index 9f8d146f881b..4ae91bb70c70 100644 --- a/bridges/modules/ethereum/src/import.rs +++ b/bridges/modules/ethereum/src/import.rs @@ -168,6 +168,7 @@ mod tests { use crate::validators::ValidatorsSource; use crate::{BlocksToPrune, BridgeStorage, Headers, PruningRange}; use frame_support::{StorageMap, StorageValue}; + use parity_crypto::publickey::KeyPair; #[test] fn rejects_finalized_block_competitors() { @@ -400,39 +401,53 @@ mod tests { }); } + fn import_custom_block( + storage: &mut S, + validators: &[KeyPair], + number: u64, + step: u64, + customize: impl FnOnce(&mut Header), + ) -> Result { + let header = custom_block_i(number, validators, |header| { + header.seal[0][0] = step as _; + header.author = + crate::validators::step_validator(&validators.iter().map(|kp| kp.address()).collect::>(), step); + customize(header); + }); + let header = signed_header(validators, header, step); + let id = header.compute_id(); + import_header( + storage, + &mut KeepSomeHeadersBehindBest::default(), + &test_aura_config(), + &ValidatorsConfiguration::Single(ValidatorsSource::Contract( + [0; 20].into(), + validators.iter().map(|kp| kp.address()).collect(), + )), + None, + header, + None, + ) + .map(|_| id) + } + #[test] fn import_of_non_best_block_may_finalize_blocks() { const TOTAL_VALIDATORS: u8 = 3; let validators_addresses = validators_addresses(TOTAL_VALIDATORS); custom_test_ext(genesis(), validators_addresses.clone()).execute_with(move || { let validators = validators(TOTAL_VALIDATORS); - let validators_config = ValidatorsConfiguration::Single(ValidatorsSource::Contract( - [0; 20].into(), - validators_addresses.clone(), - )); let mut storage = BridgeStorage::::new(); - let mut pruning_strategy = KeepSomeHeadersBehindBest::default(); // insert headers (H1, validator1), (H2, validator1), (H3, validator1) // making H3 the best header, without finalizing anything (we need 2 signatures) let mut expected_best_block = Default::default(); for i in 1..4 { let step = GENESIS_STEP + i * TOTAL_VALIDATORS as u64; - let header = custom_block_i(i, &validators, |header| { + expected_best_block = import_custom_block(&mut storage, &validators, i, step, |header| { header.author = validators_addresses[0]; header.seal[0][0] = step as u8; - }); - let header = signed_header(&validators, header, step); - expected_best_block = header.compute_id(); - import_header( - &mut storage, - &mut pruning_strategy, - &test_aura_config(), - &validators_config, - None, - header, - None, - ) + }) .unwrap(); } let (best_block, best_difficulty) = storage.best_block(); @@ -444,26 +459,16 @@ mod tests { let mut expected_finalized_block = Default::default(); let mut parent_hash = genesis().compute_hash(); for i in 1..3 { - let header = custom_block_i(i, &validators, |header| { + let step = GENESIS_STEP + i; + let id = import_custom_block(&mut storage, &validators, i, step, |header| { header.gas_limit += 1.into(); header.parent_hash = parent_hash; - }); - let header = signed_header(&validators, header, GENESIS_STEP + i); - parent_hash = header.compute_hash(); + }) + .unwrap(); + parent_hash = id.hash; if i == 1 { - expected_finalized_block = header.compute_id(); + expected_finalized_block = id; } - - import_header( - &mut storage, - &mut pruning_strategy, - &test_aura_config(), - &validators_config, - None, - header, - None, - ) - .unwrap(); } let (new_best_block, new_best_difficulty) = storage.best_block(); assert_eq!(new_best_block, expected_best_block); @@ -471,4 +476,101 @@ mod tests { assert_eq!(storage.finalized_block(), expected_finalized_block); }); } + + #[test] + fn append_to_unfinalized_fork_fails() { + const TOTAL_VALIDATORS: u64 = 5; + let validators_addresses = validators_addresses(TOTAL_VALIDATORS as _); + custom_test_ext(genesis(), validators_addresses.clone()).execute_with(move || { + let validators = validators(TOTAL_VALIDATORS as _); + let mut storage = BridgeStorage::::new(); + + // header1, authored by validator[2] is best common block between two competing forks + let header1 = import_custom_block(&mut storage, &validators, 1, GENESIS_STEP + 1, |_| ()).unwrap(); + assert_eq!(storage.best_block().0, header1); + assert_eq!(storage.finalized_block().number, 0); + + // validator[3] has authored header2 (nothing is finalized yet) + let header2 = import_custom_block(&mut storage, &validators, 2, GENESIS_STEP + 2, |_| ()).unwrap(); + assert_eq!(storage.best_block().0, header2); + assert_eq!(storage.finalized_block().number, 0); + + // validator[4] has authored header3 (header1 is finalized) + let header3 = import_custom_block(&mut storage, &validators, 3, GENESIS_STEP + 3, |_| ()).unwrap(); + assert_eq!(storage.best_block().0, header3); + assert_eq!(storage.finalized_block(), header1); + + // validator[4] has authored 4 blocks: header2'...header5' (header1 is still finalized) + let header2_1 = import_custom_block(&mut storage, &validators, 2, GENESIS_STEP + 3, |header| { + header.gas_limit += 1.into(); + }) + .unwrap(); + let header3_1 = import_custom_block( + &mut storage, + &validators, + 3, + GENESIS_STEP + 3 + TOTAL_VALIDATORS, + |header| { + header.parent_hash = header2_1.hash; + }, + ) + .unwrap(); + let header4_1 = import_custom_block( + &mut storage, + &validators, + 4, + GENESIS_STEP + 3 + TOTAL_VALIDATORS * 2, + |header| { + header.parent_hash = header3_1.hash; + }, + ) + .unwrap(); + let header5_1 = import_custom_block( + &mut storage, + &validators, + 5, + GENESIS_STEP + 3 + TOTAL_VALIDATORS * 3, + |header| { + header.parent_hash = header4_1.hash; + }, + ) + .unwrap(); + assert_eq!(storage.best_block().0, header5_1); + assert_eq!(storage.finalized_block(), header1); + + // when we import header4 { parent = header3 }, authored by validator[0], header2 is finalized + let header4 = import_custom_block(&mut storage, &validators, 4, GENESIS_STEP + 4, |_| ()).unwrap(); + assert_eq!(storage.best_block().0, header5_1); + assert_eq!(storage.finalized_block(), header2); + + // when we import header5 { parent = header4 }, authored by validator[1], header3 is finalized + let _ = import_custom_block(&mut storage, &validators, 5, GENESIS_STEP + 5, |header| { + header.parent_hash = header4.hash; + }) + .unwrap(); + assert_eq!(storage.best_block().0, header5_1); + assert_eq!(storage.finalized_block(), header3); + + // import of header2'' { parent = header1 } fails, because it has number < best_finalized + assert_eq!( + import_custom_block(&mut storage, &validators, 2, GENESIS_STEP + 3, |header| { + header.gas_limit += 2.into(); + }), + Err(Error::AncientHeader), + ); + + // import of header6' should also fail because we're trying to append to fork thas + // has forked before finalized block + assert_eq!( + import_custom_block( + &mut storage, + &validators, + 6, + GENESIS_STEP + 3 + TOTAL_VALIDATORS * 4, + |_| () + ), + Err(Error::TryingToFinalizeSibling), + ); + }); + } } diff --git a/bridges/modules/ethereum/src/lib.rs b/bridges/modules/ethereum/src/lib.rs index ef7f7b95d514..4c495a8c1489 100644 --- a/bridges/modules/ethereum/src/lib.rs +++ b/bridges/modules/ethereum/src/lib.rs @@ -268,11 +268,12 @@ pub trait Storage { /// Returns header and its submitter (if known). fn header(&self, hash: &H256) -> Option<(Header, Option)>; /// Returns latest cached finality votes (if any) for block ancestors, starting - /// from `parent_hash` block and stopping at genesis block, or block where `stop_at` - /// returns true. + /// from `parent_hash` block and stopping at genesis block, best finalized block + /// or block where `stop_at` returns true. fn cached_finality_votes( &self, - parent_hash: &H256, + parent: &HeaderId, + best_finalized: &HeaderId, stop_at: impl Fn(&H256) -> bool, ) -> CachedFinalityVotes; /// Get header import context by parent header hash. @@ -307,6 +308,12 @@ pub trait PruningStrategy: Default { /// guarantees on when it will happen. Example: if some unfinalized block at height N /// has scheduled validators set change, then the module won't prune any blocks with /// number >= N even if strategy allows that. + /// + /// If your strategy allows pruning unfinalized blocks, this could lead to switch + /// between finalized forks (only if authorities are misbehaving). But since 50%+1 (or 2/3) + /// authorities are able to do whatever they want with the chain, this isn't considered + /// fatal. If your strategy only prunes finalized blocks, we'll never be able to finalize + /// header that isn't descendant of current best finalized block. fn pruning_upper_bound(&mut self, best_number: u64, best_finalized_number: u64) -> u64; } @@ -660,36 +667,49 @@ impl Storage for BridgeStorage { fn cached_finality_votes( &self, - parent_hash: &H256, + parent: &HeaderId, + best_finalized: &HeaderId, stop_at: impl Fn(&H256) -> bool, ) -> CachedFinalityVotes { let mut votes = CachedFinalityVotes::default(); - let mut current_hash = *parent_hash; + let mut current_id = *parent; loop { - if stop_at(¤t_hash) { + // if we have reached finalized block' sibling => stop with special signal + if current_id.number == best_finalized.number { + if current_id.hash != best_finalized.hash { + votes.stopped_at_finalized_sibling = true; + return votes; + } + } + + // if we have reached target header => stop + if stop_at(¤t_id.hash) { return votes; } - let cached_votes = FinalityCache::::get(¤t_hash); + // if we have found cached votes => stop + let cached_votes = FinalityCache::::get(¤t_id.hash); if let Some(cached_votes) = cached_votes { votes.votes = Some(cached_votes); return votes; } - let header = match Headers::::get(¤t_hash) { + // read next parent header id + let header = match Headers::::get(¤t_id.hash) { Some(header) if header.header.number != 0 => header, _ => return votes, }; - let parent_hash = header.header.parent_hash; - let current_id = HeaderId { - number: header.header.number, - hash: current_hash, - }; + let parent_id = header.header.parent_id().expect( + "only returns None at genesis header;\ + the header is proved to have number > 0;\ + qed", + ); + votes .unaccounted_ancestry .push_back((current_id, header.submitter, header.header)); - current_hash = parent_hash; + current_id = parent_id; } } @@ -1108,10 +1128,11 @@ pub(crate) mod tests { } // when inserting header#6, entry isn't found - let hash5 = headers.last().unwrap().compute_hash(); + let id5 = headers.last().unwrap().compute_id(); assert_eq!( - storage.cached_finality_votes(&hash5, |_| false), + storage.cached_finality_votes(&id5, &genesis().compute_id(), |_| false), CachedFinalityVotes { + stopped_at_finalized_sibling: false, unaccounted_ancestry: headers .iter() .map(|header| (header.compute_id(), None, header.clone(),)) @@ -1139,8 +1160,9 @@ pub(crate) mod tests { // searching at #6 again => entry is found assert_eq!( - storage.cached_finality_votes(&hash5, |_| false), + storage.cached_finality_votes(&id5, &genesis().compute_id(), |_| false), CachedFinalityVotes { + stopped_at_finalized_sibling: false, unaccounted_ancestry: headers .iter() .skip(3) @@ -1153,6 +1175,43 @@ pub(crate) mod tests { }); } + #[test] + fn cached_finality_votes_stops_at_finalized_sibling() { + custom_test_ext(genesis(), validators_addresses(3)).execute_with(|| { + let validators = validators(3); + let mut storage = BridgeStorage::::new(); + + // insert header1 + let header1 = block_i(1, &validators); + let header1_id = header1.compute_id(); + insert_header(&mut storage, header1); + + // insert header1' - sibling of header1 + let header1s = custom_block_i(1, &validators, |header| { + header.gas_limit += 1.into(); + }); + let header1s_id = header1s.compute_id(); + insert_header(&mut storage, header1s); + + // header1 is finalized + FinalizedBlock::put(header1_id); + + // trying to get finality votes when importing header2 -> header1 succeeds + assert!( + !storage + .cached_finality_votes(&header1_id, &genesis().compute_id(), |_| false) + .stopped_at_finalized_sibling + ); + + // trying to get finality votes when importing header2s -> header1s fails + assert!( + storage + .cached_finality_votes(&header1s_id, &header1_id, |_| false) + .stopped_at_finalized_sibling + ); + }); + } + #[test] fn verify_transaction_finalized_works_for_best_finalized_header() { custom_test_ext(example_header(), validators_addresses(3)).execute_with(|| { diff --git a/bridges/primitives/ethereum-poa/src/lib.rs b/bridges/primitives/ethereum-poa/src/lib.rs index da04c13faa77..12ee57131535 100644 --- a/bridges/primitives/ethereum-poa/src/lib.rs +++ b/bridges/primitives/ethereum-poa/src/lib.rs @@ -178,6 +178,14 @@ impl Header { keccak_256(&self.rlp(true)).into() } + /// Get id of this header' parent. Returns None if this is genesis header. + pub fn parent_id(&self) -> Option { + self.number.checked_sub(1).map(|parent_number| HeaderId { + number: parent_number, + hash: self.parent_hash, + }) + } + /// Check if passed transactions receipts are matching receipts root in this header. pub fn verify_receipts_root(&self, receipts: &[Receipt]) -> bool { verify_merkle_proof(self.receipts_root, receipts.iter().map(|r| r.rlp()))