GRANDPA: optimize votes_ancestries when needed (#2262) (#2264)

* GRANDPA: optimize votes_ancestries when needed

* Address review comments

bridges/primitives/header-chain/src/justification.rs

@@ -21,7 +21,7 @@
 
 use crate::ChainWithGrandpa;
 
-use bp_runtime::{BlockNumberOf, Chain, HashOf};
+use bp_runtime::{BlockNumberOf, Chain, HashOf, HeaderId};
 use codec::{Decode, Encode, MaxEncodedLen};
 use finality_grandpa::voter_set::VoterSet;
 use frame_support::{RuntimeDebug, RuntimeDebugNoBound};
@@ -84,6 +84,10 @@ impl<H: HeaderT> GrandpaJustification<H> {
 		8u32.saturating_add(max_expected_signed_commit_size)
 			.saturating_add(max_expected_votes_ancestries_size)
 	}
+
+	pub fn commit_target_id(&self) -> HeaderId<H::Hash, H::Number> {
+		HeaderId(self.commit.target_number, self.commit.target_hash)
+	}
 }
 
 impl<H: HeaderT> crate::FinalityProof<H::Number> for GrandpaJustification<H> {
@@ -109,12 +113,12 @@ pub enum Error {
 	InvalidAuthoritySignature,
 	/// The justification contains precommit for header that is not a descendant of the commit
 	/// header.
-	PrecommitIsNotCommitDescendant,
+	UnrelatedAncestryVote,
 	/// The cumulative weight of all votes in the justification is not enough to justify commit
 	/// header finalization.
 	TooLowCumulativeWeight,
 	/// The justification contains extra (unused) headers in its `votes_ancestries` field.
-	ExtraHeadersInVotesAncestries,
+	RedundantVotesAncestries,
 }
 
 /// Given GRANDPA authorities set size, return number of valid authorities votes that the
@@ -139,20 +143,25 @@ pub fn verify_and_optimize_justification<Header: HeaderT>(
 	finalized_target: (Header::Hash, Header::Number),
 	authorities_set_id: SetId,
 	authorities_set: &VoterSet<AuthorityId>,
-	justification: GrandpaJustification<Header>,
+	justification: &mut GrandpaJustification<Header>,
-) -> Result<GrandpaJustification<Header>, Error>
+) -> Result<(), Error>
 where
 	Header::Number: finality_grandpa::BlockNumberOps,
 {
-	let mut optimizer = OptimizationCallbacks(Vec::new());
+	let mut optimizer = OptimizationCallbacks {
+		extra_precommits: vec![],
+		redundant_votes_ancestries: Default::default(),
+	};
 	verify_justification_with_callbacks(
 		finalized_target,
 		authorities_set_id,
 		authorities_set,
-		&justification,
+		justification,
 		&mut optimizer,
 	)?;
-	Ok(optimizer.optimize(justification))
+	optimizer.optimize(justification);
+
+	Ok(())
 }
 
 /// Verify that justification, that is generated by given authority set, finalizes given header.
@@ -175,19 +184,28 @@ where
 }
 
 /// Verification callbacks.
-trait VerificationCallbacks {
+trait VerificationCallbacks<Header: HeaderT> {
 	/// Called when we see a precommit from unknown authority.
 	fn on_unkown_authority(&mut self, precommit_idx: usize) -> Result<(), Error>;
 	/// Called when we see a precommit with duplicate vote from known authority.
 	fn on_duplicate_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error>;
+	/// Called when we see a precommit with an invalid signature.
+	fn on_invalid_authority_signature(&mut self, precommit_idx: usize) -> Result<(), Error>;
 	/// Called when we see a precommit after we've collected enough votes from authorities.
 	fn on_redundant_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error>;
+	/// Called when we see a precommit that is not a descendant of the commit target.
+	fn on_unrelated_ancestry_vote(&mut self, precommit_idx: usize) -> Result<(), Error>;
+	/// Called when there are redundant headers in the votes ancestries.
+	fn on_redundant_votes_ancestries(
+		&mut self,
+		redundant_votes_ancestries: BTreeSet<Header::Hash>,
+	) -> Result<(), Error>;
 }
 
 /// Verification callbacks that reject all unknown, duplicate or redundant votes.
 struct StrictVerificationCallbacks;
 
-impl VerificationCallbacks for StrictVerificationCallbacks {
+impl<Header: HeaderT> VerificationCallbacks<Header> for StrictVerificationCallbacks {
 	fn on_unkown_authority(&mut self, _precommit_idx: usize) -> Result<(), Error> {
 		Err(Error::UnknownAuthorityVote)
 	}
@@ -196,45 +214,82 @@ impl VerificationCallbacks for StrictVerificationCallbacks {
 		Err(Error::DuplicateAuthorityVote)
 	}
 
+	fn on_invalid_authority_signature(&mut self, _precommit_idx: usize) -> Result<(), Error> {
+		Err(Error::InvalidAuthoritySignature)
+	}
+
 	fn on_redundant_authority_vote(&mut self, _precommit_idx: usize) -> Result<(), Error> {
 		Err(Error::RedundantVotesInJustification)
 	}
+
+	fn on_unrelated_ancestry_vote(&mut self, _precommit_idx: usize) -> Result<(), Error> {
+		Err(Error::UnrelatedAncestryVote)
+	}
+
+	fn on_redundant_votes_ancestries(
+		&mut self,
+		_redundant_votes_ancestries: BTreeSet<Header::Hash>,
+	) -> Result<(), Error> {
+		Err(Error::RedundantVotesAncestries)
+	}
 }
 
 /// Verification callbacks for justification optimization.
-struct OptimizationCallbacks(Vec<usize>);
+struct OptimizationCallbacks<Header: HeaderT> {
+	extra_precommits: Vec<usize>,
+	redundant_votes_ancestries: BTreeSet<Header::Hash>,
+}
 
-impl OptimizationCallbacks {
+impl<Header: HeaderT> OptimizationCallbacks<Header> {
-	fn optimize<Header: HeaderT>(
+	fn optimize(self, justification: &mut GrandpaJustification<Header>) {
-		self,
+		for invalid_precommit_idx in self.extra_precommits.into_iter().rev() {
-		mut justification: GrandpaJustification<Header>,
-	) -> GrandpaJustification<Header> {
-		for invalid_precommit_idx in self.0.into_iter().rev() {
 			justification.commit.precommits.remove(invalid_precommit_idx);
 		}
-		justification
+		if !self.redundant_votes_ancestries.is_empty() {
+			justification
+				.votes_ancestries
+				.retain(|header| !self.redundant_votes_ancestries.contains(&header.hash()))
+		}
 	}
 }
 
-impl VerificationCallbacks for OptimizationCallbacks {
+impl<Header: HeaderT> VerificationCallbacks<Header> for OptimizationCallbacks<Header> {
 	fn on_unkown_authority(&mut self, precommit_idx: usize) -> Result<(), Error> {
-		self.0.push(precommit_idx);
+		self.extra_precommits.push(precommit_idx);
 		Ok(())
 	}
 
 	fn on_duplicate_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
-		self.0.push(precommit_idx);
+		self.extra_precommits.push(precommit_idx);
+		Ok(())
+	}
+
+	fn on_invalid_authority_signature(&mut self, precommit_idx: usize) -> Result<(), Error> {
+		self.extra_precommits.push(precommit_idx);
 		Ok(())
 	}
 
 	fn on_redundant_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
-		self.0.push(precommit_idx);
+		self.extra_precommits.push(precommit_idx);
+		Ok(())
+	}
+
+	fn on_unrelated_ancestry_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
+		self.extra_precommits.push(precommit_idx);
+		Ok(())
+	}
+
+	fn on_redundant_votes_ancestries(
+		&mut self,
+		redundant_votes_ancestries: BTreeSet<Header::Hash>,
+	) -> Result<(), Error> {
+		self.redundant_votes_ancestries = redundant_votes_ancestries;
 		Ok(())
 	}
 }
 
 /// Verify that justification, that is generated by given authority set, finalizes given header.
-fn verify_justification_with_callbacks<Header: HeaderT, C: VerificationCallbacks>(
+fn verify_justification_with_callbacks<Header: HeaderT, C: VerificationCallbacks<Header>>(
 	finalized_target: (Header::Hash, Header::Number),
 	authorities_set_id: SetId,
 	authorities_set: &VoterSet<AuthorityId>,
@@ -249,8 +304,8 @@ where
 		return Err(Error::InvalidJustificationTarget)
 	}
 
-	let threshold = authorities_set.threshold().0.into();
+	let threshold = authorities_set.threshold().get();
-	let mut chain = AncestryChain::new(&justification.votes_ancestries);
+	let mut chain = AncestryChain::new(justification);
 	let mut signature_buffer = Vec::new();
 	let mut votes = BTreeSet::new();
 	let mut cumulative_weight = 0u64;
@@ -277,34 +332,20 @@ where
 		// there's a lot of code in `validate_commit` and `import_precommit` functions inside
 		// `finality-grandpa` crate (mostly related to reporting equivocations). But the only thing
 		// that we care about is that only first vote from the authority is accepted
-		if !votes.insert(signed.id.clone()) {
+		if votes.contains(&signed.id) {
 			callbacks.on_duplicate_authority_vote(precommit_idx)?;
 			continue
 		}
 
-		// everything below this line can't just `continue`, because state is already altered
+		// all precommits must be descendants of the target block
-
+		let route =
-		// precommits aren't allowed for block lower than the target
+			match chain.ancestry(&signed.precommit.target_hash, &signed.precommit.target_number) {
-		if signed.precommit.target_number < justification.commit.target_number {
+				Some(route) => route,
-			return Err(Error::PrecommitIsNotCommitDescendant)
+				None => {
-		}
+					callbacks.on_unrelated_ancestry_vote(precommit_idx)?;
-		// all precommits must be descendants of target block
+					continue
-		chain = chain
+				},
-			.ensure_descendant(&justification.commit.target_hash, &signed.precommit.target_hash)?;
+			};
-		// since we know now that the precommit target is the descendant of the justification
-		// target, we may increase 'weight' of the justification target
-		//
-		// there's a lot of code in the `VoteGraph::insert` method inside `finality-grandpa` crate,
-		// but in the end it is only used to find GHOST, which we don't care about. The only thing
-		// that we care about is that the justification target has enough weight
-		cumulative_weight = cumulative_weight.checked_add(authority_info.weight().0.into()).expect(
-			"sum of weights of ALL authorities is expected not to overflow - this is guaranteed by\
-				existence of VoterSet;\
-				the order of loop conditions guarantees that we can account vote from same authority\
-				multiple times;\
-				thus we'll never overflow the u64::MAX;\
-				qed",
-		);
 
 		// verify authority signature
 		if !sp_consensus_grandpa::check_message_signature_with_buffer(
@@ -315,76 +356,98 @@ where
 			authorities_set_id,
 			&mut signature_buffer,
 		) {
-			return Err(Error::InvalidAuthoritySignature)
+			callbacks.on_invalid_authority_signature(precommit_idx)?;
+			continue
 		}
+
+		// now we can count the vote since we know that it is valid
+		votes.insert(signed.id.clone());
+		chain.mark_route_as_visited(route);
+		cumulative_weight = cumulative_weight.saturating_add(authority_info.weight().get());
+	}
+
+	// check that the cumulative weight of validators that voted for the justification target (or
+	// one of its descendents) is larger than the required threshold.
+	if cumulative_weight < threshold {
+		return Err(Error::TooLowCumulativeWeight)
 	}
 
 	// check that there are no extra headers in the justification
-	if !chain.unvisited.is_empty() {
+	if !chain.is_fully_visited() {
-		return Err(Error::ExtraHeadersInVotesAncestries)
+		callbacks.on_redundant_votes_ancestries(chain.unvisited)?;
 	}
 
-	// check that the cumulative weight of validators voted for the justification target (or one
+	Ok(())
-	// of its descendents) is larger than required threshold.
-	if cumulative_weight >= threshold {
-		Ok(())
-	} else {
-		Err(Error::TooLowCumulativeWeight)
-	}
 }
 
 /// Votes ancestries with useful methods.
 #[derive(RuntimeDebug)]
 pub struct AncestryChain<Header: HeaderT> {
+	/// We expect all forks in the ancestry chain to be descendants of base.
+	base: HeaderId<Header::Hash, Header::Number>,
 	/// Header hash => parent header hash mapping.
 	pub parents: BTreeMap<Header::Hash, Header::Hash>,
-	/// Hashes of headers that were not visited by `is_ancestor` method.
+	/// Hashes of headers that were not visited by `ancestry()`.
 	pub unvisited: BTreeSet<Header::Hash>,
 }
 
 impl<Header: HeaderT> AncestryChain<Header> {
 	/// Create new ancestry chain.
-	pub fn new(ancestry: &[Header]) -> AncestryChain<Header> {
+	pub fn new(justification: &GrandpaJustification<Header>) -> AncestryChain<Header> {
 		let mut parents = BTreeMap::new();
 		let mut unvisited = BTreeSet::new();
-		for ancestor in ancestry {
+		for ancestor in &justification.votes_ancestries {
 			let hash = ancestor.hash();
 			let parent_hash = *ancestor.parent_hash();
 			parents.insert(hash, parent_hash);
 			unvisited.insert(hash);
 		}
-		AncestryChain { parents, unvisited }
+		AncestryChain { base: justification.commit_target_id(), parents, unvisited }
 	}
 
-	/// Returns `Ok(_)` if `precommit_target` is a descendant of the `commit_target` block and
+	/// Returns a route if the precommit target block is a descendant of the `base` block.
-	/// `Err(_)` otherwise.
+	pub fn ancestry(
-	pub fn ensure_descendant(
+		&self,
-		mut self,
+		precommit_target_hash: &Header::Hash,
-		commit_target: &Header::Hash,
+		precommit_target_number: &Header::Number,
-		precommit_target: &Header::Hash,
+	) -> Option<Vec<Header::Hash>> {
-	) -> Result<Self, Error> {
+		if precommit_target_number < &self.base.number() {
-		let mut current_hash = *precommit_target;
+			return None
+		}
+
+		let mut route = vec![];
+		let mut current_hash = *precommit_target_hash;
 		loop {
-			if current_hash == *commit_target {
+			if current_hash == self.base.hash() {
 				break
 			}
 
-			let is_visited_before = !self.unvisited.remove(&current_hash);
 			current_hash = match self.parents.get(&current_hash) {
 				Some(parent_hash) => {
+					let is_visited_before = self.unvisited.get(&current_hash).is_none();
 					if is_visited_before {
-						// `Some(parent_hash)` means that the `current_hash` is in the `parents`
+						// If the current header has been visited in a previous call, it is a
-						// container `is_visited_before` means that it has been visited before in
+						// descendent of `base` (we assume that the previous call was successful).
-						// some of previous calls => since we assume that previous call has finished
+						return Some(route)
-						// with `true`, this also will be finished with `true`
-						return Ok(self)
 					}
+					route.push(current_hash);
 
 					*parent_hash
 				},
-				None => return Err(Error::PrecommitIsNotCommitDescendant),
+				None => return None,
 			};
 		}
-		Ok(self)
+
+		Some(route)
+	}
+
+	fn mark_route_as_visited(&mut self, route: Vec<Header::Hash>) {
+		for hash in route {
+			self.unvisited.remove(&hash);
+		}
+	}
+
+	fn is_fully_visited(&self) -> bool {
+		self.unvisited.is_empty()
 	}
 }

bridges/primitives/header-chain/tests/implementation_match.rs

@@ -38,8 +38,8 @@ type TestNumber = <TestHeader as HeaderT>::Number;
 struct AncestryChain(bp_header_chain::justification::AncestryChain<TestHeader>);
 
 impl AncestryChain {
-	fn new(ancestry: &[TestHeader]) -> Self {
+	fn new(justification: &GrandpaJustification<TestHeader>) -> Self {
-		Self(bp_header_chain::justification::AncestryChain::new(ancestry))
+		Self(bp_header_chain::justification::AncestryChain::new(justification))
 	}
 }
 
@@ -55,9 +55,9 @@ impl finality_grandpa::Chain<TestHash, TestNumber> for AncestryChain {
 			if current_hash == base {
 				break
 			}
-			match self.0.parents.get(&current_hash).cloned() {
+			match self.0.parents.get(&current_hash) {
 				Some(parent_hash) => {
-					current_hash = parent_hash;
+					current_hash = *parent_hash;
 					route.push(current_hash);
 				},
 				_ => return Err(finality_grandpa::Error::NotDescendent),
@@ -124,7 +124,7 @@ fn same_result_when_precommit_target_has_lower_number_than_commit_target() {
 			&full_voter_set(),
 			&justification,
 		),
-		Err(Error::PrecommitIsNotCommitDescendant),
+		Err(Error::UnrelatedAncestryVote),
 	);
 
 	// original implementation returns `Ok(validation_result)`
@@ -132,7 +132,7 @@ fn same_result_when_precommit_target_has_lower_number_than_commit_target() {
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 
@@ -157,7 +157,7 @@ fn same_result_when_precommit_target_is_not_descendant_of_commit_target() {
 			&full_voter_set(),
 			&justification,
 		),
-		Err(Error::PrecommitIsNotCommitDescendant),
+		Err(Error::UnrelatedAncestryVote),
 	);
 
 	// original implementation returns `Ok(validation_result)`
@@ -165,7 +165,7 @@ fn same_result_when_precommit_target_is_not_descendant_of_commit_target() {
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 
@@ -198,7 +198,7 @@ fn same_result_when_there_are_not_enough_cumulative_weight_to_finalize_commit_ta
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 
@@ -236,7 +236,7 @@ fn different_result_when_justification_contains_duplicate_vote() {
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 
@@ -277,7 +277,7 @@ fn different_results_when_authority_equivocates_once_in_a_round() {
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 
@@ -330,7 +330,7 @@ fn different_results_when_authority_equivocates_twice_in_a_round() {
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 
@@ -369,7 +369,7 @@ fn different_results_when_there_are_more_than_enough_votes() {
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 
@@ -410,7 +410,7 @@ fn different_results_when_there_is_a_vote_of_unknown_authority() {
 	let result = finality_grandpa::validate_commit(
 		&justification.commit,
 		&full_voter_set(),
-		&AncestryChain::new(&justification.votes_ancestries),
+		&AncestryChain::new(&justification),
 	)
 	.unwrap();
 

bridges/primitives/header-chain/tests/justification.rs

@@ -21,6 +21,8 @@ use bp_header_chain::justification::{
 	Error,
 };
 use bp_test_utils::*;
+use finality_grandpa::SignedPrecommit;
+use sp_consensus_grandpa::AuthoritySignature;
 
 type TestHeader = sp_runtime::testing::Header;
 
@@ -133,7 +135,7 @@ fn justification_with_invalid_commit_rejected() {
 			&voter_set(),
 			&justification,
 		),
-		Err(Error::ExtraHeadersInVotesAncestries),
+		Err(Error::TooLowCumulativeWeight),
 	);
 }
 
@@ -166,7 +168,7 @@ fn justification_with_invalid_precommit_ancestry() {
 			&voter_set(),
 			&justification,
 		),
-		Err(Error::ExtraHeadersInVotesAncestries),
+		Err(Error::RedundantVotesAncestries),
 	);
 }
 
@@ -197,14 +199,14 @@ fn justification_is_invalid_if_we_dont_meet_threshold() {
 
 #[test]
 fn optimizer_does_noting_with_minimal_justification() {
-	let justification = make_default_justification::<TestHeader>(&test_header(1));
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
 
 	let num_precommits_before = justification.commit.precommits.len();
-	let justification = verify_and_optimize_justification::<TestHeader>(
+	verify_and_optimize_justification::<TestHeader>(
 		header_id::<TestHeader>(1),
 		TEST_GRANDPA_SET_ID,
 		&voter_set(),
-		justification,
+		&mut justification,
 	)
 	.unwrap();
 	let num_precommits_after = justification.commit.precommits.len();
@@ -223,11 +225,11 @@ fn unknown_authority_votes_are_removed_by_optimizer() {
 	));
 
 	let num_precommits_before = justification.commit.precommits.len();
-	let justification = verify_and_optimize_justification::<TestHeader>(
+	verify_and_optimize_justification::<TestHeader>(
 		header_id::<TestHeader>(1),
 		TEST_GRANDPA_SET_ID,
 		&voter_set(),
-		justification,
+		&mut justification,
 	)
 	.unwrap();
 	let num_precommits_after = justification.commit.precommits.len();
@@ -244,11 +246,42 @@ fn duplicate_authority_votes_are_removed_by_optimizer() {
 		.push(justification.commit.precommits.first().cloned().unwrap());
 
 	let num_precommits_before = justification.commit.precommits.len();
-	let justification = verify_and_optimize_justification::<TestHeader>(
+	verify_and_optimize_justification::<TestHeader>(
 		header_id::<TestHeader>(1),
 		TEST_GRANDPA_SET_ID,
 		&voter_set(),
-		justification,
+		&mut justification,
+	)
+	.unwrap();
+	let num_precommits_after = justification.commit.precommits.len();
+
+	assert_eq!(num_precommits_before - 1, num_precommits_after);
+}
+
+#[test]
+fn invalid_authority_signatures_are_removed_by_optimizer() {
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
+
+	let target = header_id::<TestHeader>(1);
+	let invalid_raw_signature: Vec<u8> = ALICE.sign(b"").to_bytes().into();
+	justification.commit.precommits.insert(
+		0,
+		SignedPrecommit {
+			precommit: finality_grandpa::Precommit {
+				target_hash: target.0,
+				target_number: target.1,
+			},
+			signature: AuthoritySignature::try_from(invalid_raw_signature).unwrap(),
+			id: ALICE.into(),
+		},
+	);
+
+	let num_precommits_before = justification.commit.precommits.len();
+	verify_and_optimize_justification::<TestHeader>(
+		header_id::<TestHeader>(1),
+		TEST_GRANDPA_SET_ID,
+		&voter_set(),
+		&mut justification,
 	)
 	.unwrap();
 	let num_precommits_after = justification.commit.precommits.len();
@@ -267,14 +300,58 @@ fn redundant_authority_votes_are_removed_by_optimizer() {
 	));
 
 	let num_precommits_before = justification.commit.precommits.len();
-	let justification = verify_and_optimize_justification::<TestHeader>(
+	verify_and_optimize_justification::<TestHeader>(
 		header_id::<TestHeader>(1),
 		TEST_GRANDPA_SET_ID,
 		&voter_set(),
-		justification,
+		&mut justification,
 	)
 	.unwrap();
 	let num_precommits_after = justification.commit.precommits.len();
 
 	assert_eq!(num_precommits_before - 1, num_precommits_after);
 }
+
+#[test]
+fn unrelated_ancestry_votes_are_removed_by_optimizer() {
+	let mut justification = make_default_justification::<TestHeader>(&test_header(2));
+	justification.commit.precommits.insert(
+		0,
+		signed_precommit::<TestHeader>(
+			&ALICE,
+			header_id::<TestHeader>(1),
+			justification.round,
+			TEST_GRANDPA_SET_ID,
+		),
+	);
+
+	let num_precommits_before = justification.commit.precommits.len();
+	verify_and_optimize_justification::<TestHeader>(
+		header_id::<TestHeader>(2),
+		TEST_GRANDPA_SET_ID,
+		&voter_set(),
+		&mut justification,
+	)
+	.unwrap();
+	let num_precommits_after = justification.commit.precommits.len();
+
+	assert_eq!(num_precommits_before - 1, num_precommits_after);
+}
+
+#[test]
+fn redundant_votes_ancestries_are_removed_by_optimizer() {
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
+	justification.votes_ancestries.push(test_header(100));
+
+	let num_votes_ancestries_before = justification.votes_ancestries.len();
+	verify_and_optimize_justification::<TestHeader>(
+		header_id::<TestHeader>(1),
+		TEST_GRANDPA_SET_ID,
+		&voter_set(),
+		&mut justification,
+	)
+	.unwrap();
+	let num_votes_ancestries_after = justification.votes_ancestries.len();
+
+	assert_eq!(num_votes_ancestries_before - 1, num_votes_ancestries_after);
+}

bridges/relays/lib-substrate-relay/src/finality/engine.rs

@@ -91,8 +91,8 @@ pub trait Engine<C: Chain>: Send {
 	async fn optimize_proof<TargetChain: Chain>(
 		target_client: &Client<TargetChain>,
 		header: &C::Header,
-		proof: Self::FinalityProof,
+		proof: &mut Self::FinalityProof,
-	) -> Result<Self::FinalityProof, SubstrateError>;
+	) -> Result<(), SubstrateError>;
 
 	/// Prepare initialization data for the finality bridge pallet.
 	async fn prepare_initialization_data(
@@ -149,8 +149,8 @@ impl<C: ChainWithGrandpa> Engine<C> for Grandpa<C> {
 	async fn optimize_proof<TargetChain: Chain>(
 		target_client: &Client<TargetChain>,
 		header: &C::Header,
-		proof: Self::FinalityProof,
+		proof: &mut Self::FinalityProof,
-	) -> Result<Self::FinalityProof, SubstrateError> {
+	) -> Result<(), SubstrateError> {
 		let current_authority_set_key = bp_header_chain::storage_keys::current_authority_set_key(
 			C::WITH_CHAIN_GRANDPA_PALLET_NAME,
 		);
@@ -275,7 +275,7 @@ impl<C: ChainWithGrandpa> Engine<C> for Grandpa<C> {
 				(initial_header_hash, initial_header_number),
 				initial_authorities_set_id,
 				&authorities_for_verification,
-				justification.clone(),
+				&mut justification.clone(),
 			)
 			.is_ok();
 

bridges/relays/lib-substrate-relay/src/finality/target.rs

@@ -109,10 +109,10 @@ where
 	async fn submit_finality_proof(
 		&self,
 		header: SyncHeader<HeaderOf<P::SourceChain>>,
-		proof: SubstrateFinalityProof<P>,
+		mut proof: SubstrateFinalityProof<P>,
 	) -> Result<Self::TransactionTracker, Error> {
 		// runtime module at target chain may require optimized finality proof
-		let proof = P::FinalityEngine::optimize_proof(&self.client, &header, proof).await?;
+		P::FinalityEngine::optimize_proof(&self.client, &header, &mut proof).await?;
 
 		// now we may submit optimized finality proof
 		let transaction_params = self.transaction_params.clone();

bridges/relays/lib-substrate-relay/src/on_demand/headers.rs

@@ -135,11 +135,11 @@ impl<P: SubstrateFinalitySyncPipeline> OnDemandRelay<P::SourceChain, P::TargetCh
 	) -> Result<(HeaderIdOf<P::SourceChain>, Vec<CallOf<P::TargetChain>>), SubstrateError> {
 		// first find proper header (either `required_header`) or its descendant
 		let finality_source = SubstrateFinalitySource::<P>::new(self.source_client.clone(), None);
-		let (header, proof) = finality_source.prove_block_finality(required_header).await?;
+		let (header, mut proof) = finality_source.prove_block_finality(required_header).await?;
 		let header_id = header.id();
 
 		// optimize justification before including it into the call
-		let proof = P::FinalityEngine::optimize_proof(&self.target_client, &header, proof).await?;
+		P::FinalityEngine::optimize_proof(&self.target_client, &header, &mut proof).await?;
 
 		log::debug!(
 			target: "bridge",