From 4686068c912dad9458afc445c6d258862f2fc7d1 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 20 Feb 2023 11:57:54 +0300 Subject: [PATCH] optimize justification before submit (#1887) * optimize justification before submit * fmt * spelling * clippy * fmt again * aaand compilation * clippy --- bridges/modules/grandpa/src/lib.rs | 5 + .../header-chain/src/justification.rs | 121 +++++++++++++++++- .../header-chain/src/storage_keys.rs | 26 ++++ .../header-chain/tests/justification.rs | 86 ++++++++++++- bridges/primitives/test-utils/src/lib.rs | 5 +- .../src/finality/engine.rs | 51 +++++++- .../src/finality/target.rs | 4 + 7 files changed, 290 insertions(+), 8 deletions(-) diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 197726dd47a5..b11da53f7b3b 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/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::::storage_value_final_key().to_vec(), + bp_header_chain::storage_keys::current_authority_set_key("Grandpa").0, + ); + assert_eq!( BestFinalized::::storage_value_final_key().to_vec(), bp_header_chain::storage_keys::best_finalized_key("Grandpa").0, diff --git a/bridges/primitives/header-chain/src/justification.rs b/bridges/primitives/header-chain/src/justification.rs index dadd48a48502..43adc8012544 100644 --- a/bridges/primitives/header-chain/src/justification.rs +++ b/bridges/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( raw_justification: &[u8], @@ -80,6 +88,27 @@ pub fn decode_justification_target( .map_err(|_| Error::JustificationDecode) } +/// Verify and optimize given justification by removing unknown and duplicate votes. +pub fn optimize_justification( + finalized_target: (Header::Hash, Header::Number), + authorities_set_id: SetId, + authorities_set: &VoterSet, + justification: GrandpaJustification
, +) -> Result, 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( finalized_target: (Header::Hash, Header::Number), @@ -87,6 +116,83 @@ pub fn verify_justification( authorities_set: &VoterSet, justification: &GrandpaJustification
, ) -> 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); + +impl OptimizationCallbacks { + fn optimize( + self, + mut justification: GrandpaJustification
, + ) -> GrandpaJustification
{ + 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( + finalized_target: (Header::Hash, Header::Number), + authorities_set_id: SetId, + authorities_set: &VoterSet, + justification: &GrandpaJustification
, + 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 { diff --git a/bridges/primitives/header-chain/src/storage_keys.rs b/bridges/primitives/header-chain/src/storage_keys.rs index bb642b1817f7..c4dbe53bd9a7 100644 --- a/bridges/primitives/header-chain/src/storage_keys.rs +++ b/bridges/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 diff --git a/bridges/primitives/header-chain/tests/justification.rs b/bridges/primitives/header-chain/tests/justification.rs index 5b4981a0f69a..e18163313c93 100644 --- a/bridges/primitives/header-chain/tests/justification.rs +++ b/bridges/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::(&test_header(1)); + + let num_precommits_before = justification.commit.precommits.len(); + let justification = optimize_justification::( + header_id::(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::(&test_header(1)); + justification.commit.precommits.push(signed_precommit::( + &bp_test_utils::Account(42), + header_id::(1), + justification.round, + TEST_GRANDPA_SET_ID, + )); + + let num_precommits_before = justification.commit.precommits.len(); + let justification = optimize_justification::( + header_id::(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::(&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::( + header_id::(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::(&test_header(1)); + justification.commit.precommits.push(signed_precommit::( + &EVE, + header_id::(1), + justification.round, + TEST_GRANDPA_SET_ID, + )); + + let num_precommits_before = justification.commit.precommits.len(); + let justification = optimize_justification::( + header_id::(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); +} diff --git a/bridges/primitives/test-utils/src/lib.rs b/bridges/primitives/test-utils/src/lib.rs index c1e95ec6fefd..a6bb3d9b8feb 100644 --- a/bridges/primitives/test-utils/src/lib.rs +++ b/bridges/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 { impl Default for JustificationGeneratorParams { 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, } diff --git a/bridges/relays/lib-substrate-relay/src/finality/engine.rs b/bridges/relays/lib-substrate-relay/src/finality/engine.rs index 4c2da5a53195..f5ac8539a689 100644 --- a/bridges/relays/lib-substrate-relay/src/finality/engine.rs +++ b/bridges/relays/lib-substrate-relay/src/finality/engine.rs @@ -22,7 +22,7 @@ use bp_header_chain::{ justification::{verify_justification, GrandpaJustification}, ConsensusLogReader, FinalityProof, GrandpaConsensusLogReader, }; -use bp_runtime::{BasicOperatingMode, OperatingMode}; +use bp_runtime::{BasicOperatingMode, HeaderIdProvider, OperatingMode}; use codec::{Decode, Encode}; use finality_grandpa::voter_set::VoterSet; use num_traits::{One, Zero}; @@ -87,6 +87,13 @@ pub trait Engine: Send { client.subscribe_finality_justifications::().await } + /// Optimize finality proof before sending it to the target node. + async fn optimize_proof( + target_client: &Client, + header: &C::Header, + proof: Self::FinalityProof, + ) -> Result; + /// Prepare initialization data for the finality bridge pallet. async fn prepare_initialization_data( client: Client, @@ -139,6 +146,48 @@ impl Engine for Grandpa { bp_header_chain::storage_keys::pallet_operating_mode_key(C::WITH_CHAIN_GRANDPA_PALLET_NAME) } + async fn optimize_proof( + target_client: &Client, + header: &C::Header, + proof: Self::FinalityProof, + ) -> Result { + let current_authority_set_key = bp_header_chain::storage_keys::current_authority_set_key( + C::WITH_CHAIN_GRANDPA_PALLET_NAME, + ); + let (authority_set, authority_set_id): ( + sp_finality_grandpa::AuthorityList, + sp_finality_grandpa::SetId, + ) = target_client + .storage_value(current_authority_set_key, None) + .await? + .map(Ok) + .unwrap_or(Err(SubstrateError::Custom(format!( + "{} `CurrentAuthoritySet` is missing from the {} storage", + C::NAME, + TargetChain::NAME, + ))))?; + let authority_set = + finality_grandpa::voter_set::VoterSet::new(authority_set).expect("TODO"); + // we're risking with race here - we have decided to submit justification some time ago and + // actual authorities set (which we have read now) may have changed, so this + // `optimize_justification` may fail. But if target chain is configured properly, it'll fail + // anyway, after we submit transaction and failing earlier is better. So - it is fine + bp_header_chain::justification::optimize_justification( + (header.hash(), *header.number()), + authority_set_id, + &authority_set, + proof, + ) + .map_err(|e| { + SubstrateError::Custom(format!( + "Failed to optimize {} GRANDPA jutification for header {:?}: {:?}", + C::NAME, + header.id(), + e, + )) + }) + } + /// Prepare initialization data for the GRANDPA verifier pallet. async fn prepare_initialization_data( source_client: Client, diff --git a/bridges/relays/lib-substrate-relay/src/finality/target.rs b/bridges/relays/lib-substrate-relay/src/finality/target.rs index 9c6ec7c3055e..81a22520fa95 100644 --- a/bridges/relays/lib-substrate-relay/src/finality/target.rs +++ b/bridges/relays/lib-substrate-relay/src/finality/target.rs @@ -111,6 +111,10 @@ where header: SyncHeader>, proof: SubstrateFinalityProof

, ) -> Result { + // runtime module at target chain may require optimized finality proof + let proof = P::FinalityEngine::optimize_proof(&self.client, &header, proof).await?; + + // now we may submit optimized finality proof let transaction_params = self.transaction_params.clone(); let call = P::SubmitFinalityProofCallBuilder::build_submit_finality_proof_call(header, proof);