diff --git a/prdoc/pr_9733.prdoc b/prdoc/pr_9733.prdoc new file mode 100644 index 0000000000000..a8a6a6cb0a75d --- /dev/null +++ b/prdoc/pr_9733.prdoc @@ -0,0 +1,13 @@ +title: Revert "store headers and justifications during warp sync" +doc: +- audience: Node Operator + description: Reverts paritytech/polkadot-sdk#9424 +crates: +- name: sc-consensus-grandpa + bump: patch +- name: sc-network-sync + bump: major +- name: sc-rpc-spec-v2 + bump: patch +- name: sp-runtime + bump: major diff --git a/substrate/client/consensus/grandpa/src/warp_proof.rs b/substrate/client/consensus/grandpa/src/warp_proof.rs index 42701a9cb3edd..899686355b1ba 100644 --- a/substrate/client/consensus/grandpa/src/warp_proof.rs +++ b/substrate/client/consensus/grandpa/src/warp_proof.rs @@ -30,7 +30,6 @@ use sp_consensus_grandpa::{AuthorityList, SetId, GRANDPA_ENGINE_ID}; use sp_runtime::{ generic::BlockId, traits::{Block as BlockT, Header as HeaderT, NumberFor, One}, - Justifications, }; use std::{collections::HashMap, sync::Arc}; @@ -312,28 +311,13 @@ where .ok_or_else(|| "Empty proof".to_string())?; let (next_set_id, next_authorities) = proof.verify(set_id, authorities, &self.hard_forks).map_err(Box::new)?; - let justifications = proof - .proofs - .into_iter() - .map(|p| { - let justifications = - Justifications::new(vec![(GRANDPA_ENGINE_ID, p.justification.encode())]); - (p.header, justifications) - }) - .collect::>(); if proof.is_finished { - Ok(VerificationResult::::Complete( - next_set_id, - next_authorities, - last_header, - justifications, - )) + Ok(VerificationResult::::Complete(next_set_id, next_authorities, last_header)) } else { Ok(VerificationResult::::Partial( next_set_id, next_authorities, last_header.hash(), - justifications, )) } } diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index c815ca1b86d6a..a4c9c0868b794 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -18,8 +18,6 @@ //! Warp syncing strategy. Bootstraps chain by downloading warp proofs and state. -use sc_consensus::IncomingBlock; -use sp_consensus::BlockOrigin; pub use sp_consensus_grandpa::{AuthorityList, SetId}; use crate::{ @@ -63,9 +61,9 @@ pub struct WarpProofRequest { /// Proof verification result. pub enum VerificationResult { /// Proof is valid, but the target was not reached. - Partial(SetId, AuthorityList, Block::Hash, Vec<(Block::Header, Justifications)>), + Partial(SetId, AuthorityList, Block::Hash), /// Target finality is proved. - Complete(SetId, AuthorityList, Block::Header, Vec<(Block::Header, Justifications)>), + Complete(SetId, AuthorityList, Block::Header), } /// Warp sync backend. Handles retrieving and verifying warp sync proofs. @@ -405,43 +403,20 @@ where return }; - let proof_to_incoming_block = - |(header, justifications): (B::Header, Justifications)| -> IncomingBlock { - IncomingBlock { - hash: header.hash(), - header: Some(header), - body: None, - indexed_body: None, - justifications: Some(justifications), - origin: Some(*peer_id), - // We are still in warp sync, so we don't have the state. This means - // we also can't execute the block. - allow_missing_state: true, - skip_execution: true, - // Shouldn't already exist in the database. - import_existing: false, - state: None, - } - }; - match warp_sync_provider.verify(&response, *set_id, authorities.clone()) { Err(e) => { debug!(target: LOG_TARGET, "Bad warp proof response: {}", e); self.actions .push(SyncingAction::DropPeer(BadPeer(*peer_id, rep::BAD_WARP_PROOF))) }, - Ok(VerificationResult::Partial(new_set_id, new_authorities, new_last_hash, proofs)) => { + Ok(VerificationResult::Partial(new_set_id, new_authorities, new_last_hash)) => { log::debug!(target: LOG_TARGET, "Verified partial proof, set_id={:?}", new_set_id); *set_id = new_set_id; *authorities = new_authorities; *last_hash = new_last_hash; self.total_proof_bytes += response.0.len() as u64; - self.actions.push(SyncingAction::ImportBlocks { - origin: BlockOrigin::NetworkInitialSync, - blocks: proofs.into_iter().map(proof_to_incoming_block).collect(), - }); }, - Ok(VerificationResult::Complete(new_set_id, _, header, proofs)) => { + Ok(VerificationResult::Complete(new_set_id, _, header)) => { log::debug!( target: LOG_TARGET, "Verified complete proof, set_id={:?}. Continuing with target block download: {} ({}).", @@ -451,10 +426,6 @@ where ); self.total_proof_bytes += response.0.len() as u64; self.phase = Phase::TargetBlock(header); - self.actions.push(SyncingAction::ImportBlocks { - origin: BlockOrigin::NetworkInitialSync, - blocks: proofs.into_iter().map(proof_to_incoming_block).collect(), - }); }, } } @@ -773,7 +744,7 @@ mod test { use crate::{mock::MockBlockDownloader, service::network::NetworkServiceProvider}; use sc_block_builder::BlockBuilderBuilder; use sp_blockchain::{BlockStatus, Error as BlockchainError, HeaderBackend, Info}; - use sp_consensus_grandpa::{AuthorityList, SetId, GRANDPA_ENGINE_ID}; + use sp_consensus_grandpa::{AuthorityList, SetId}; use sp_core::H256; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use std::{io::ErrorKind, sync::Arc}; @@ -1259,38 +1230,19 @@ mod test { #[test] fn partial_warp_proof_doesnt_advance_phase() { - let client = Arc::new(TestClientBuilder::new().set_no_genesis().build()); + let client = mock_client_without_state(); let mut provider = MockWarpSyncProvider::::new(); provider .expect_current_authorities() .once() .return_const(AuthorityList::default()); - let target_block = BlockBuilderBuilder::new(&*client) - .on_parent_block(client.chain_info().best_hash) - .with_parent_block_number(client.chain_info().best_number) - .build() - .unwrap() - .build() - .unwrap() - .block; - let target_header = target_block.header().clone(); - let justifications = Justifications::new(vec![(GRANDPA_ENGINE_ID, vec![1, 2, 3, 4, 5])]); // Warp proof is partial. - { - let target_header = target_header.clone(); - let justifications = justifications.clone(); - provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Partial( - set_id, - authorities, - target_header.hash(), - vec![(target_header, justifications)], - )) - }); - } + provider.expect_verify().return_once(|_proof, set_id, authorities| { + Ok(VerificationResult::Partial(set_id, authorities, Hash::random())) + }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = WarpSync::new( - client, + Arc::new(client), config, Some(ProtocolName::Static("")), Arc::new(MockBlockDownloader::new()), @@ -1315,29 +1267,7 @@ mod test { warp_sync.on_warp_proof_response(&request_peer_id, EncodedProof(Vec::new())); - assert_eq!(warp_sync.actions.len(), 1); - let SyncingAction::ImportBlocks { origin, mut blocks } = warp_sync.actions.pop().unwrap() - else { - panic!("Expected `ImportBlocks` action."); - }; - assert_eq!(origin, BlockOrigin::NetworkInitialSync); - assert_eq!(blocks.len(), 1); - let import_block = blocks.pop().unwrap(); - assert_eq!( - import_block, - IncomingBlock { - hash: target_header.hash(), - header: Some(target_header), - body: None, - indexed_body: None, - justifications: Some(justifications), - origin: Some(request_peer_id), - allow_missing_state: true, - skip_execution: true, - import_existing: false, - state: None, - } - ); + assert!(warp_sync.actions.is_empty(), "No extra actions generated"); assert!(matches!(warp_sync.phase, Phase::WarpProof { .. })); } @@ -1358,20 +1288,10 @@ mod test { .unwrap() .block; let target_header = target_block.header().clone(); - let justifications = Justifications::new(vec![(GRANDPA_ENGINE_ID, vec![1, 2, 3, 4, 5])]); // Warp proof is complete. - { - let target_header = target_header.clone(); - let justifications = justifications.clone(); - provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Complete( - set_id, - authorities, - target_header.clone(), - vec![(target_header, justifications)], - )) - }); - } + provider.expect_verify().return_once(move |_proof, set_id, authorities| { + Ok(VerificationResult::Complete(set_id, authorities, target_header)) + }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = WarpSync::new( client, @@ -1399,29 +1319,7 @@ mod test { warp_sync.on_warp_proof_response(&request_peer_id, EncodedProof(Vec::new())); - assert_eq!(warp_sync.actions.len(), 1); - let SyncingAction::ImportBlocks { origin, mut blocks } = warp_sync.actions.pop().unwrap() - else { - panic!("Expected `ImportBlocks` action."); - }; - assert_eq!(origin, BlockOrigin::NetworkInitialSync); - assert_eq!(blocks.len(), 1); - let import_block = blocks.pop().unwrap(); - assert_eq!( - import_block, - IncomingBlock { - hash: target_header.hash(), - header: Some(target_header), - body: None, - indexed_body: None, - justifications: Some(justifications), - origin: Some(request_peer_id), - allow_missing_state: true, - skip_execution: true, - import_existing: false, - state: None, - } - ); + assert!(warp_sync.actions.is_empty(), "No extra actions generated."); assert!( matches!(warp_sync.phase, Phase::TargetBlock(header) if header == *target_block.header()) ); @@ -1474,7 +1372,7 @@ mod test { let target_header = target_block.header().clone(); // Warp proof is complete. provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Complete(set_id, authorities, target_header, Default::default())) + Ok(VerificationResult::Complete(set_id, authorities, target_header)) }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = @@ -1548,7 +1446,7 @@ mod test { let target_header = target_block.header().clone(); // Warp proof is complete. provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Complete(set_id, authorities, target_header, Default::default())) + Ok(VerificationResult::Complete(set_id, authorities, target_header)) }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = @@ -1587,7 +1485,7 @@ mod test { let target_header = target_block.header().clone(); // Warp proof is complete. provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Complete(set_id, authorities, target_header, Default::default())) + Ok(VerificationResult::Complete(set_id, authorities, target_header)) }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = @@ -1642,7 +1540,7 @@ mod test { let target_header = target_block.header().clone(); // Warp proof is complete. provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Complete(set_id, authorities, target_header, Default::default())) + Ok(VerificationResult::Complete(set_id, authorities, target_header)) }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = @@ -1720,7 +1618,7 @@ mod test { let target_header = target_block.header().clone(); // Warp proof is complete. provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Complete(set_id, authorities, target_header, Default::default())) + Ok(VerificationResult::Complete(set_id, authorities, target_header)) }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = @@ -1774,7 +1672,7 @@ mod test { let target_header = target_block.header().clone(); // Warp proof is complete. provider.expect_verify().return_once(move |_proof, set_id, authorities| { - Ok(VerificationResult::Complete(set_id, authorities, target_header, Default::default())) + Ok(VerificationResult::Complete(set_id, authorities, target_header)) }); let config = WarpSyncConfig::WithProvider(Arc::new(provider)); let mut warp_sync = diff --git a/substrate/client/network/test/src/lib.rs b/substrate/client/network/test/src/lib.rs index 79a0a42138bf7..068cbce2d2bec 100644 --- a/substrate/client/network/test/src/lib.rs +++ b/substrate/client/network/test/src/lib.rs @@ -673,7 +673,7 @@ impl WarpSyncProvider for TestWarpSyncProvider { ) -> Result, Box> { let EncodedProof(encoded) = proof; let header = B::Header::decode(&mut encoded.as_slice()).unwrap(); - Ok(VerificationResult::Complete(0, Default::default(), header, Default::default())) + Ok(VerificationResult::Complete(0, Default::default(), header)) } fn current_authorities(&self) -> AuthorityList { Default::default() diff --git a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs index fd43ffefc4934..48259f10ffc07 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs @@ -289,7 +289,6 @@ impl> BlockBackend fn block_indexed_body(&self, hash: Block::Hash) -> sp_blockchain::Result>>> { self.client.block_indexed_body(hash) } - fn requires_full_sync(&self) -> bool { self.client.requires_full_sync() } diff --git a/substrate/primitives/runtime/src/lib.rs b/substrate/primitives/runtime/src/lib.rs index 96946e5175906..1fc7c5a297dd7 100644 --- a/substrate/primitives/runtime/src/lib.rs +++ b/substrate/primitives/runtime/src/lib.rs @@ -156,15 +156,10 @@ pub type EncodedJustification = Vec; /// Collection of justifications for a given block, multiple justifications may /// be provided by different consensus engines for the same block. #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Default, Debug, Clone, PartialEq, Eq, Encode, Decode)] +#[derive(Debug, Clone, PartialEq, Eq, Encode, Decode)] pub struct Justifications(Vec); impl Justifications { - /// Create a new `Justifications` instance with the given justifications. - pub fn new(justifications: Vec) -> Self { - Self(justifications) - } - /// Return an iterator over the justifications. pub fn iter(&self) -> impl Iterator { self.0.iter()