optimize justification before submit (#1887)

* optimize justification before submit

* fmt

* spelling

* clippy

* fmt again

* aaand compilation

* clippy

Author: svyatonik

modules/grandpa/src/lib.rs

@@ -1187,6 +1187,11 @@ mod tests {
 			bp_header_chain::storage_keys::pallet_operating_mode_key("Grandpa").0,
 		);
 
+		assert_eq!(
+			CurrentAuthoritySet::<TestRuntime>::storage_value_final_key().to_vec(),
+			bp_header_chain::storage_keys::current_authority_set_key("Grandpa").0,
+		);
+
 		assert_eq!(
 			BestFinalized::<TestRuntime>::storage_value_final_key().to_vec(),
 			bp_header_chain::storage_keys::best_finalized_key("Grandpa").0,

primitives/header-chain/src/justification.rs

@@ -71,6 +71,14 @@ pub enum Error {
 	ExtraHeadersInVotesAncestries,
 }
 
+/// Given GRANDPA authorities set size, return number of valid authorities votes that the
+/// justification must have to be valid.
+///
+/// This function assumes that all authorities have the same vote weight.
+pub fn required_justification_precommits(authorities_set_length: u32) -> u32 {
+	authorities_set_length - authorities_set_length.saturating_sub(1) / 3
+}
+
 /// Decode justification target.
 pub fn decode_justification_target<Header: HeaderT>(
 	raw_justification: &[u8],
@@ -80,13 +88,111 @@ pub fn decode_justification_target<Header: HeaderT>(
 		.map_err(|_| Error::JustificationDecode)
 }
 
+/// Verify and optimize given justification by removing unknown and duplicate votes.
+pub fn optimize_justification<Header: HeaderT>(
+	finalized_target: (Header::Hash, Header::Number),
+	authorities_set_id: SetId,
+	authorities_set: &VoterSet<AuthorityId>,
+	justification: GrandpaJustification<Header>,
+) -> Result<GrandpaJustification<Header>, Error>
+where
+	Header::Number: finality_grandpa::BlockNumberOps,
+{
+	let mut optimizer = OptimizationCallbacks(Vec::new());
+	verify_justification_with_callbacks(
+		finalized_target,
+		authorities_set_id,
+		authorities_set,
+		&justification,
+		&mut optimizer,
+	)?;
+	Ok(optimizer.optimize(justification))
+}
+
 /// Verify that justification, that is generated by given authority set, finalizes given header.
 pub fn verify_justification<Header: HeaderT>(
 	finalized_target: (Header::Hash, Header::Number),
 	authorities_set_id: SetId,
 	authorities_set: &VoterSet<AuthorityId>,
 	justification: &GrandpaJustification<Header>,
 ) -> Result<(), Error>
+where
+	Header::Number: finality_grandpa::BlockNumberOps,
+{
+	verify_justification_with_callbacks(
+		finalized_target,
+		authorities_set_id,
+		authorities_set,
+		justification,
+		&mut (),
+	)
+}
+
+/// Verification callbacks.
+trait VerificationCallbacks {
+	/// 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 after we've collected enough votes from authorities.
+	fn on_redundant_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error>;
+}
+
+/// Verification callbacks for justification optimization.
+struct OptimizationCallbacks(Vec<usize>);
+
+impl OptimizationCallbacks {
+	fn optimize<Header: HeaderT>(
+		self,
+		mut justification: GrandpaJustification<Header>,
+	) -> GrandpaJustification<Header> {
+		for invalid_precommit_idx in self.0.into_iter().rev() {
+			justification.commit.precommits.remove(invalid_precommit_idx);
+		}
+		justification
+	}
+}
+
+impl VerificationCallbacks for OptimizationCallbacks {
+	fn on_unkown_authority(&mut self, precommit_idx: usize) -> Result<(), Error> {
+		self.0.push(precommit_idx);
+		Ok(())
+	}
+
+	fn on_duplicate_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
+		self.0.push(precommit_idx);
+		Ok(())
+	}
+
+	fn on_redundant_authority_vote(&mut self, precommit_idx: usize) -> Result<(), Error> {
+		self.0.push(precommit_idx);
+		Ok(())
+	}
+}
+
+// this implementation will be removed in https://github.com/paritytech/parity-bridges-common/pull/1882
+impl VerificationCallbacks for () {
+	fn on_unkown_authority(&mut self, _precommit_idx: usize) -> Result<(), Error> {
+		Ok(())
+	}
+
+	fn on_duplicate_authority_vote(&mut self, _precommit_idx: usize) -> Result<(), Error> {
+		Ok(())
+	}
+
+	fn on_redundant_authority_vote(&mut self, _precommit_idx: usize) -> Result<(), Error> {
+		Ok(())
+	}
+}
+
+/// Verify that justification, that is generated by given authority set, finalizes given header.
+fn verify_justification_with_callbacks<Header: HeaderT, C: VerificationCallbacks>(
+	finalized_target: (Header::Hash, Header::Number),
+	authorities_set_id: SetId,
+	authorities_set: &VoterSet<AuthorityId>,
+	justification: &GrandpaJustification<Header>,
+	callbacks: &mut C,
+) -> Result<(), Error>
 where
 	Header::Number: finality_grandpa::BlockNumberOps,
 {
@@ -95,17 +201,23 @@ where
 		return Err(Error::InvalidJustificationTarget)
 	}
 
+	let threshold = authorities_set.threshold().0.into();
 	let mut chain = AncestryChain::new(&justification.votes_ancestries);
 	let mut signature_buffer = Vec::new();
 	let mut votes = BTreeSet::new();
 	let mut cumulative_weight = 0u64;
-	for signed in &justification.commit.precommits {
+	for (precommit_idx, signed) in justification.commit.precommits.iter().enumerate() {
+		// if we have collected enough precommits, we probabably want to fail/remove extra
+		// precommits
+		if cumulative_weight > threshold {
+			callbacks.on_redundant_authority_vote(precommit_idx)?;
+		}
+
 		// authority must be in the set
 		let authority_info = match authorities_set.get(&signed.id) {
 			Some(authority_info) => authority_info,
 			None => {
-				// just ignore precommit from unknown authority as
-				// `finality_grandpa::import_precommit` does
+				callbacks.on_unkown_authority(precommit_idx)?;
 				continue
 			},
 		};
@@ -116,6 +228,7 @@ where
 		// `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()) {
+			callbacks.on_duplicate_authority_vote(precommit_idx)?;
 			continue
 		}
 
@@ -142,6 +255,7 @@ where
 				thus we'll never overflow the u64::MAX;\
 				qed",
 		);
+
 		// verify authority signature
 		if !sp_finality_grandpa::check_message_signature_with_buffer(
 			&finality_grandpa::Message::Precommit(signed.precommit.clone()),
@@ -162,7 +276,6 @@ where
 
 	// check that the cumulative weight of validators voted for the justification target (or one
 	// of its descendents) is larger than required threshold.
-	let threshold = authorities_set.threshold().0.into();
 	if cumulative_weight >= threshold {
 		Ok(())
 	} else {

primitives/header-chain/src/storage_keys.rs

@@ -20,6 +20,8 @@
 pub const PALLET_OPERATING_MODE_VALUE_NAME: &str = "PalletOperatingMode";
 /// Name of the `BestFinalized` storage value.
 pub const BEST_FINALIZED_VALUE_NAME: &str = "BestFinalized";
+/// Name of the `CurrentAuthoritySet` storage value.
+pub const CURRENT_AUTHORITY_SET_VALUE_NAME: &str = "CurrentAuthoritySet";
 
 use sp_core::storage::StorageKey;
 
@@ -34,6 +36,17 @@ pub fn pallet_operating_mode_key(pallet_prefix: &str) -> StorageKey {
 	)
 }
 
+/// Storage key of the `CurrentAuthoritySet` variable in the runtime storage.
+pub fn current_authority_set_key(pallet_prefix: &str) -> StorageKey {
+	StorageKey(
+		bp_runtime::storage_value_final_key(
+			pallet_prefix.as_bytes(),
+			CURRENT_AUTHORITY_SET_VALUE_NAME.as_bytes(),
+		)
+		.to_vec(),
+	)
+}
+
 /// Storage key of the best finalized header number and hash value in the runtime storage.
 pub fn best_finalized_key(pallet_prefix: &str) -> StorageKey {
 	StorageKey(
@@ -63,6 +76,19 @@ mod tests {
 		);
 	}
 
+	#[test]
+	fn current_authority_set_key_computed_properly() {
+		// If this test fails, then something has been changed in module storage that is breaking
+		// compatibility with previous pallet.
+		let storage_key = current_authority_set_key("BridgeGrandpa").0;
+		assert_eq!(
+			storage_key,
+			hex!("0b06f475eddb98cf933a12262e0388de24a7b8b5717ea33346fa595a66ccbcb0").to_vec(),
+			"Unexpected storage key: {}",
+			hex::encode(&storage_key),
+		);
+	}
+
 	#[test]
 	fn best_finalized_key_computed_properly() {
 		// If this test fails, then something has been changed in module storage that is breaking

primitives/header-chain/tests/justification.rs

@@ -16,7 +16,7 @@
 
 //! Tests for Grandpa Justification code.
 
-use bp_header_chain::justification::{verify_justification, Error};
+use bp_header_chain::justification::{optimize_justification, verify_justification, Error};
 use bp_test_utils::*;
 
 type TestHeader = sp_runtime::testing::Header;
@@ -190,3 +190,87 @@ fn justification_is_invalid_if_we_dont_meet_threshold() {
 		Err(Error::TooLowCumulativeWeight),
 	);
 }
+
+#[test]
+fn optimizer_does_noting_with_minimal_justification() {
+	let justification = make_default_justification::<TestHeader>(&test_header(1));
+
+	let num_precommits_before = justification.commit.precommits.len();
+	let justification = optimize_justification::<TestHeader>(
+		header_id::<TestHeader>(1),
+		TEST_GRANDPA_SET_ID,
+		&voter_set(),
+		justification,
+	)
+	.unwrap();
+	let num_precommits_after = justification.commit.precommits.len();
+
+	assert_eq!(num_precommits_before, num_precommits_after);
+}
+
+#[test]
+fn unknown_authority_votes_are_removed_by_optimizer() {
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
+	justification.commit.precommits.push(signed_precommit::<TestHeader>(
+		&bp_test_utils::Account(42),
+		header_id::<TestHeader>(1),
+		justification.round,
+		TEST_GRANDPA_SET_ID,
+	));
+
+	let num_precommits_before = justification.commit.precommits.len();
+	let justification = optimize_justification::<TestHeader>(
+		header_id::<TestHeader>(1),
+		TEST_GRANDPA_SET_ID,
+		&voter_set(),
+		justification,
+	)
+	.unwrap();
+	let num_precommits_after = justification.commit.precommits.len();
+
+	assert_eq!(num_precommits_before - 1, num_precommits_after);
+}
+
+#[test]
+fn duplicate_authority_votes_are_removed_by_optimizer() {
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
+	justification
+		.commit
+		.precommits
+		.push(justification.commit.precommits.first().cloned().unwrap());
+
+	let num_precommits_before = justification.commit.precommits.len();
+	let justification = optimize_justification::<TestHeader>(
+		header_id::<TestHeader>(1),
+		TEST_GRANDPA_SET_ID,
+		&voter_set(),
+		justification,
+	)
+	.unwrap();
+	let num_precommits_after = justification.commit.precommits.len();
+
+	assert_eq!(num_precommits_before - 1, num_precommits_after);
+}
+
+#[test]
+fn redundant_authority_votes_are_removed_by_optimizer() {
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
+	justification.commit.precommits.push(signed_precommit::<TestHeader>(
+		&EVE,
+		header_id::<TestHeader>(1),
+		justification.round,
+		TEST_GRANDPA_SET_ID,
+	));
+
+	let num_precommits_before = justification.commit.precommits.len();
+	let justification = optimize_justification::<TestHeader>(
+		header_id::<TestHeader>(1),
+		TEST_GRANDPA_SET_ID,
+		&voter_set(),
+		justification,
+	)
+	.unwrap();
+	let num_precommits_after = justification.commit.precommits.len();
+
+	assert_eq!(num_precommits_before - 1, num_precommits_after);
+}

primitives/test-utils/src/lib.rs

@@ -18,7 +18,7 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use bp_header_chain::justification::GrandpaJustification;
+use bp_header_chain::justification::{required_justification_precommits, GrandpaJustification};
 use codec::Encode;
 use sp_finality_grandpa::{AuthorityId, AuthoritySignature, AuthorityWeight, SetId};
 use sp_runtime::traits::{Header as HeaderT, One, Zero};
@@ -57,11 +57,12 @@ pub struct JustificationGeneratorParams<H> {
 
 impl<H: HeaderT> Default for JustificationGeneratorParams<H> {
 	fn default() -> Self {
+		let required_signatures = required_justification_precommits(test_keyring().len() as _);
 		Self {
 			header: test_header(One::one()),
 			round: TEST_GRANDPA_ROUND,
 			set_id: TEST_GRANDPA_SET_ID,
-			authorities: test_keyring(),
+			authorities: test_keyring().into_iter().take(required_signatures as _).collect(),
 			ancestors: 2,
 			forks: 1,
 		}