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

@@ -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",
 		}
 	}
 

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<Submitter> {
+	/// 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<Submitter>, Header)>,
@@ -88,10 +92,15 @@ pub fn finalize_blocks<S: Storage>(
 	// 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<Submitter>(
 	mut cached_votes: CachedFinalityVotes<Submitter>,
-	best_finalized_number: u64,
+	best_finalized: HeaderId,
 	validators: &BTreeSet<&Address>,
 	id: HeaderId,
 	header: &Header,
 	submitter: Option<Submitter>,
 ) -> Result<FinalityVotes<Submitter>, 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<Submitter>(
 
 	// 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<Submitter> Default for CachedFinalityVotes<Submitter> {
+	fn default() -> Self {
+		CachedFinalityVotes {
+			stopped_at_finalized_sibling: false,
+			unaccounted_ancestry: VecDeque::new(),
+			votes: None,
+		}
+	}
+}
+
 impl<Submitter> Default for FinalityVotes<Submitter> {
 	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),
+		);
+	}
 }

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<S: Storage>(
+		storage: &mut S,
+		validators: &[KeyPair],
+		number: u64,
+		step: u64,
+		customize: impl FnOnce(&mut Header),
+	) -> Result<HeaderId, Error> {
+		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::<Vec<_>>(), 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::<TestRuntime>::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();
-				if i == 1 {
-					expected_finalized_block = header.compute_id();
-				}
-
-				import_header(
-					&mut storage,
-					&mut pruning_strategy,
-					&test_aura_config(),
-					&validators_config,
-					None,
-					header,
-					None,
-				)
+				})
 				.unwrap();
+				parent_hash = id.hash;
+				if i == 1 {
+					expected_finalized_block = id;
+				}
 			}
 			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::<TestRuntime>::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),
+			);
+		});
+	}
 }

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<Self::Submitter>)>;
 	/// 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<Self::Submitter>;
 	/// 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<T: Trait> Storage for BridgeStorage<T> {
 
 	fn cached_finality_votes(
 		&self,
-		parent_hash: &H256,
+		parent: &HeaderId,
+		best_finalized: &HeaderId,
 		stop_at: impl Fn(&H256) -> bool,
 	) -> CachedFinalityVotes<Self::Submitter> {
 		let mut votes = CachedFinalityVotes::default();
-		let mut current_hash = *parent_hash;
+		let mut current_id = *parent;
 		loop {
-			if stop_at(&current_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(&current_id.hash) {
 				return votes;
 			}
 
-			let cached_votes = FinalityCache::<T>::get(&current_hash);
+			// if we have found cached votes => stop
+			let cached_votes = FinalityCache::<T>::get(&current_id.hash);
 			if let Some(cached_votes) = cached_votes {
 				votes.votes = Some(cached_votes);
 				return votes;
 			}
 
-			let header = match Headers::<T>::get(&current_hash) {
+			// read next parent header id
+			let header = match Headers::<T>::get(&current_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::<TestRuntime>::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(|| {

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<HeaderId> {
+		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()))