From 622048e89b24304158a243258f2086e1f449195a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 11:33:41 +0000 Subject: [PATCH 01/19] grandpa: Add debug logs for rounds and set IDs Signed-off-by: Alexandru Vasile --- substrate/primitives/consensus/grandpa/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index 3aa4dc2b4cfb3..89623ee4a6478 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -453,7 +453,10 @@ where if !valid { let log_target = if cfg!(feature = "std") { CLIENT_LOG_TARGET } else { RUNTIME_LOG_TARGET }; - log::debug!(target: log_target, "Bad signature on message from {:?}", id); + log::debug!( + target: log_target, + "Bad signature on message from id={id:?} round={round:?} set_id={set_id:?}", + ); } valid From caff0bc065df7a4311d4d1c86f6d06422d3d7ac7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 11:54:52 +0000 Subject: [PATCH 02/19] grandpa: Add debug for previous set decoding Signed-off-by: Alexandru Vasile --- substrate/primitives/consensus/grandpa/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index 89623ee4a6478..1514b8c2245e2 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -457,6 +457,13 @@ where target: log_target, "Bad signature on message from id={id:?} round={round:?} set_id={set_id:?}", ); + + localized_payload_with_buffer(round, set_id - 1, message, buf); + let valid = id.verify(&buf, signature); + log::debug!( + target: log_target, + "Signature result for id={id:?} round={round:?} set_id={:?} (previous set) valid={valid:?}", set_id + ); } valid From a86313f71eeb1198b3871308fb6f59c4397ab21a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 15:06:49 +0000 Subject: [PATCH 03/19] grandpa: Check signatures of justifications against prev sets Signed-off-by: Alexandru Vasile --- .../primitives/consensus/grandpa/src/lib.rs | 57 +++++++++++++------ 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index 1514b8c2245e2..d7574c37f16bd 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -412,6 +412,20 @@ pub fn localized_payload_with_buffer( (message, round, set_id).encode_to(buf) } +/// Result of checking a message signature. +#[derive(Clone, Encode, Decode, PartialEq, Eq)] +#[cfg_attr(feature = "std", derive(Debug))] +pub enum SignatureResult { + /// Valid signature. + Valid, + + /// Invalid signature. + Invalid, + + /// Invalid signature, but the message was signed in the previous set. + OutdatedSet, +} + /// Check a message signature by encoding the message as a localized payload and /// verifying the provided signature using the expected authority id. pub fn check_message_signature( @@ -420,7 +434,7 @@ pub fn check_message_signature( signature: &AuthoritySignature, round: RoundNumber, set_id: SetId, -) -> bool +) -> SignatureResult where H: Encode, N: Encode, @@ -439,7 +453,7 @@ pub fn check_message_signature_with_buffer( round: RoundNumber, set_id: SetId, buf: &mut Vec, -) -> bool +) -> SignatureResult where H: Encode, N: Encode, @@ -448,25 +462,34 @@ where localized_payload_with_buffer(round, set_id, message, buf); - let valid = id.verify(&buf, signature); - - if !valid { - let log_target = if cfg!(feature = "std") { CLIENT_LOG_TARGET } else { RUNTIME_LOG_TARGET }; + if id.verify(&buf, signature) { + return SignatureResult::Valid; + } - log::debug!( - target: log_target, - "Bad signature on message from id={id:?} round={round:?} set_id={set_id:?}", - ); + let log_target = if cfg!(feature = "std") { CLIENT_LOG_TARGET } else { RUNTIME_LOG_TARGET }; + log::debug!( + target: log_target, + "Bad signature on message from id={id:?} round={round:?} set_id={set_id:?}", + ); - localized_payload_with_buffer(round, set_id - 1, message, buf); - let valid = id.verify(&buf, signature); - log::debug!( - target: log_target, - "Signature result for id={id:?} round={round:?} set_id={:?} (previous set) valid={valid:?}", set_id - ); + // Check if the signature is valid in the previous set. + let prev_set_id = set_id.checked_sub(1).unwrap_or(0); + if prev_set_id == set_id { + return SignatureResult::Invalid; } - valid + localized_payload_with_buffer(round, prev_set_id, message, buf); + let valid = id.verify(&buf, signature); + log::debug!( + target: log_target, + "Double check signature for id={id:?} round={round:?} previous_set={prev_set_id:?} valid={valid:?}" + ); + + if valid { + SignatureResult::OutdatedSet + } else { + SignatureResult::Invalid + } } /// Localizes the message to the given set and round and signs the payload. From a820d02aa0937e38a322b99b84a9b0fa6a3a10c4 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 15:10:36 +0000 Subject: [PATCH 04/19] consensus: Adjust code to the new SignatureResult Signed-off-by: Alexandru Vasile --- .../src/justification/verification/mod.rs | 4 +++- .../consensus/grandpa/src/communication/mod.rs | 8 ++++++-- .../client/consensus/grandpa/src/justification.rs | 4 +++- substrate/primitives/consensus/grandpa/src/lib.rs | 13 +++++++++++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/bridges/primitives/header-chain/src/justification/verification/mod.rs b/bridges/primitives/header-chain/src/justification/verification/mod.rs index 9941537eb0953..9d9f6a36b5e8b 100644 --- a/bridges/primitives/header-chain/src/justification/verification/mod.rs +++ b/bridges/primitives/header-chain/src/justification/verification/mod.rs @@ -306,7 +306,9 @@ trait JustificationVerifier { justification.round, context.authority_set_id, &mut signature_buffer, - ) { + ) + .is_valid() + { self.process_invalid_signature_vote(precommit_idx).map_err(Error::Precommit)?; continue } diff --git a/substrate/client/consensus/grandpa/src/communication/mod.rs b/substrate/client/consensus/grandpa/src/communication/mod.rs index 336a2a939a688..d1a1c158ba473 100644 --- a/substrate/client/consensus/grandpa/src/communication/mod.rs +++ b/substrate/client/consensus/grandpa/src/communication/mod.rs @@ -865,7 +865,9 @@ fn check_compact_commit( round.0, set_id.0, &mut buf, - ) { + ) + .is_valid() + { debug!(target: LOG_TARGET, "Bad commit message signature {}", id); telemetry!( telemetry; @@ -952,7 +954,9 @@ fn check_catch_up( if !sp_consensus_grandpa::check_message_signature_with_buffer( &msg, id, sig, round, set_id, buf, - ) { + ) + .is_valid() + { debug!(target: LOG_TARGET, "Bad catch up message signature {}", id); telemetry!( telemetry; diff --git a/substrate/client/consensus/grandpa/src/justification.rs b/substrate/client/consensus/grandpa/src/justification.rs index 934c0d695fdab..9e3fdc2f2ca92 100644 --- a/substrate/client/consensus/grandpa/src/justification.rs +++ b/substrate/client/consensus/grandpa/src/justification.rs @@ -212,7 +212,9 @@ impl GrandpaJustification { self.justification.round, set_id, &mut buf, - ) { + ) + .is_valid() + { return Err(ClientError::BadJustification( "invalid signature for precommit in grandpa justification".to_string(), )) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index d7574c37f16bd..09e5164e76aab 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -368,7 +368,8 @@ where &$equivocation.first.1, $equivocation.round_number, report.set_id, - ); + ) + .is_valid(); let valid_second = check_message_signature( &$message($equivocation.second.0), @@ -376,7 +377,8 @@ where &$equivocation.second.1, $equivocation.round_number, report.set_id, - ); + ) + .is_valid(); return valid_first && valid_second }; @@ -426,6 +428,13 @@ pub enum SignatureResult { OutdatedSet, } +impl SignatureResult { + /// Returns `true` if the signature is valid. + pub fn is_valid(&self) -> bool { + matches!(self, SignatureResult::Valid) + } +} + /// Check a message signature by encoding the message as a localized payload and /// verifying the provided signature using the expected authority id. pub fn check_message_signature( From 3ae2eedbde90c4c8df947db1722c14ea445cacd6 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 15:26:59 +0000 Subject: [PATCH 05/19] consensus: Propagate outdated justification errors Signed-off-by: Alexandru Vasile --- .../client/consensus/grandpa/src/import.rs | 8 +++++++- .../consensus/grandpa/src/justification.rs | 17 ++++++++++------- substrate/primitives/blockchain/src/error.rs | 3 +++ .../primitives/consensus/common/src/error.rs | 3 +++ 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/substrate/client/consensus/grandpa/src/import.rs b/substrate/client/consensus/grandpa/src/import.rs index 5cec5204c7396..e2343409f3ea2 100644 --- a/substrate/client/consensus/grandpa/src/import.rs +++ b/substrate/client/consensus/grandpa/src/import.rs @@ -795,7 +795,13 @@ where ); let justification = match justification { - Err(e) => return Err(ConsensusError::ClientImport(e.to_string())), + Err(e) => { + return match e { + sp_blockchain::Error::OutdatedJustification => + Err(ConsensusError::OutdatedJustification), + _ => Err(ConsensusError::ClientImport(e.to_string())), + }; + }, Ok(justification) => justification, }; diff --git a/substrate/client/consensus/grandpa/src/justification.rs b/substrate/client/consensus/grandpa/src/justification.rs index 9e3fdc2f2ca92..d46fe5935c59c 100644 --- a/substrate/client/consensus/grandpa/src/justification.rs +++ b/substrate/client/consensus/grandpa/src/justification.rs @@ -205,19 +205,22 @@ impl GrandpaJustification { let mut buf = Vec::new(); let mut visited_hashes = HashSet::new(); for signed in self.justification.commit.precommits.iter() { - if !sp_consensus_grandpa::check_message_signature_with_buffer( + let signature_result = sp_consensus_grandpa::check_message_signature_with_buffer( &finality_grandpa::Message::Precommit(signed.precommit.clone()), &signed.id, &signed.signature, self.justification.round, set_id, &mut buf, - ) - .is_valid() - { - return Err(ClientError::BadJustification( - "invalid signature for precommit in grandpa justification".to_string(), - )) + ); + match signature_result { + sp_consensus_grandpa::SignatureResult::Invalid => + return Err(ClientError::BadJustification( + "invalid signature for precommit in grandpa justification".to_string(), + )), + sp_consensus_grandpa::SignatureResult::OutdatedSet => + return Err(ClientError::OutdatedJustification), + sp_consensus_grandpa::SignatureResult::Valid => {}, } if base_hash == signed.precommit.target_hash { diff --git a/substrate/primitives/blockchain/src/error.rs b/substrate/primitives/blockchain/src/error.rs index e8ac148d7511e..d1cf0aca2f350 100644 --- a/substrate/primitives/blockchain/src/error.rs +++ b/substrate/primitives/blockchain/src/error.rs @@ -102,6 +102,9 @@ pub enum Error { #[error("bad justification for header: {0}")] BadJustification(String), + #[error("outdated justification")] + OutdatedJustification, + #[error("This method is not currently available when running in light client mode")] NotAvailableOnLightClient, diff --git a/substrate/primitives/consensus/common/src/error.rs b/substrate/primitives/consensus/common/src/error.rs index fb8d0447fe3d6..5fa8984544a89 100644 --- a/substrate/primitives/consensus/common/src/error.rs +++ b/substrate/primitives/consensus/common/src/error.rs @@ -41,6 +41,9 @@ pub enum Error { /// Justification requirements not met. #[error("Invalid justification")] InvalidJustification, + /// The justification provided is outdated and corresponds to a previous set. + #[error("Import failed with outdated justification")] + OutdatedJustification, /// Error from the client while importing. #[error("Import failed: {0}")] ClientImport(String), From 46934a38e22e0669878034c573fc2f5ef18cbbaa Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 15:39:27 +0000 Subject: [PATCH 06/19] sync: Only ban invalid justifications, not outdated ones Signed-off-by: Alexandru Vasile --- .../consensus/common/src/import_queue.rs | 1 + .../common/src/import_queue/basic_queue.rs | 18 ++++++++++++------ .../common/src/import_queue/buffered_link.rs | 10 ++++++---- substrate/client/network/sync/src/engine.rs | 17 +++++++++++------ .../client/network/sync/src/service/mock.rs | 1 + .../sync/src/service/syncing_service.rs | 9 +++++---- 6 files changed, 36 insertions(+), 20 deletions(-) diff --git a/substrate/client/consensus/common/src/import_queue.rs b/substrate/client/consensus/common/src/import_queue.rs index 602683907d482..a5cdf70e69d5e 100644 --- a/substrate/client/consensus/common/src/import_queue.rs +++ b/substrate/client/consensus/common/src/import_queue.rs @@ -160,6 +160,7 @@ pub trait Link: Send + Sync { _hash: &B::Hash, _number: NumberFor, _success: bool, + _should_ban: bool, ) { } diff --git a/substrate/client/consensus/common/src/import_queue/basic_queue.rs b/substrate/client/consensus/common/src/import_queue/basic_queue.rs index 21270859dd75c..147873b500561 100644 --- a/substrate/client/consensus/common/src/import_queue/basic_queue.rs +++ b/substrate/client/consensus/common/src/import_queue/basic_queue.rs @@ -342,8 +342,9 @@ impl BlockImportWorker { ) { let started = std::time::Instant::now(); - let success = match self.justification_import.as_mut() { - Some(justification_import) => justification_import + let (success, should_ban) = match self.justification_import.as_mut() { + Some(justification_import) => { + let result = justification_import .import_justification(hash, number, justification) .await .map_err(|e| { @@ -356,16 +357,21 @@ impl BlockImportWorker { e, ); e - }) - .is_ok(), - None => false, + }); + match result { + Ok(()) => (true, false), + Err(sp_consensus::Error::OutdatedJustification) => (false, false), + Err(_) => (false, true), + } + }, + None => (false, false), }; if let Some(metrics) = self.metrics.as_ref() { metrics.justification_import_time.observe(started.elapsed().as_secs_f64()); } - self.result_sender.justification_imported(who, &hash, number, success); + self.result_sender.justification_imported(who, &hash, number, success, should_ban); } } diff --git a/substrate/client/consensus/common/src/import_queue/buffered_link.rs b/substrate/client/consensus/common/src/import_queue/buffered_link.rs index 67131b06a32e5..b7397f81ee316 100644 --- a/substrate/client/consensus/common/src/import_queue/buffered_link.rs +++ b/substrate/client/consensus/common/src/import_queue/buffered_link.rs @@ -84,7 +84,7 @@ impl Clone for BufferedLinkSender { /// Internal buffered message. pub enum BlockImportWorkerMsg { BlocksProcessed(usize, usize, Vec<(BlockImportResult, B::Hash)>), - JustificationImported(RuntimeOrigin, B::Hash, NumberFor, bool), + JustificationImported(RuntimeOrigin, B::Hash, NumberFor, bool, bool), RequestJustification(B::Hash, NumberFor), } @@ -106,8 +106,10 @@ impl Link for BufferedLinkSender { hash: &B::Hash, number: NumberFor, success: bool, + should_ban: bool, ) { - let msg = BlockImportWorkerMsg::JustificationImported(who, *hash, number, success); + let msg = + BlockImportWorkerMsg::JustificationImported(who, *hash, number, success, should_ban); let _ = self.tx.unbounded_send(msg); } @@ -129,8 +131,8 @@ impl BufferedLinkReceiver { match msg { BlockImportWorkerMsg::BlocksProcessed(imported, count, results) => link.blocks_processed(imported, count, results), - BlockImportWorkerMsg::JustificationImported(who, hash, number, success) => - link.justification_imported(who, &hash, number, success), + BlockImportWorkerMsg::JustificationImported(who, hash, number, success, should_ban) => + link.justification_imported(who, &hash, number, success, should_ban), BlockImportWorkerMsg::RequestJustification(hash, number) => link.request_justification(&hash, number), } diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs index 3ae8c465e1b49..0ba084dc34d34 100644 --- a/substrate/client/network/sync/src/engine.rs +++ b/substrate/client/network/sync/src/engine.rs @@ -670,17 +670,22 @@ where ToServiceCommand::BlocksProcessed(imported, count, results) => { self.strategy.on_blocks_processed(imported, count, results); }, - ToServiceCommand::JustificationImported(peer_id, hash, number, success) => { + ToServiceCommand::JustificationImported(peer_id, hash, number, success, should_ban) => { self.strategy.on_justification_import(hash, number, success); + if !success { log::info!( target: LOG_TARGET, - "💔 Invalid justification provided by {peer_id} for #{hash}", + "💔 Invalid justification provided by {peer_id} for #{hash} (should_ban={should_ban})", ); - self.network_service - .disconnect_peer(peer_id, self.block_announce_protocol_name.clone()); - self.network_service - .report_peer(peer_id, ReputationChange::new_fatal("Invalid justification")); + if should_ban { + self.network_service + .disconnect_peer(peer_id, self.block_announce_protocol_name.clone()); + self.network_service.report_peer( + peer_id, + ReputationChange::new_fatal("Invalid justification"), + ); + } } }, ToServiceCommand::AnnounceBlock(hash, data) => self.announce_block(hash, data), diff --git a/substrate/client/network/sync/src/service/mock.rs b/substrate/client/network/sync/src/service/mock.rs index 300aa076515f8..3d9fe60408a9b 100644 --- a/substrate/client/network/sync/src/service/mock.rs +++ b/substrate/client/network/sync/src/service/mock.rs @@ -56,6 +56,7 @@ mockall::mock! { hash: &B::Hash, number: NumberFor, success: bool, + should_ban: bool, ); fn request_justification(&self, hash: &B::Hash, number: NumberFor); } diff --git a/substrate/client/network/sync/src/service/syncing_service.rs b/substrate/client/network/sync/src/service/syncing_service.rs index b56af2b9976a1..8e41c9cbb9915 100644 --- a/substrate/client/network/sync/src/service/syncing_service.rs +++ b/substrate/client/network/sync/src/service/syncing_service.rs @@ -44,7 +44,7 @@ pub enum ToServiceCommand { usize, Vec<(Result>, BlockImportError>, B::Hash)>, ), - JustificationImported(PeerId, B::Hash, NumberFor, bool), + JustificationImported(PeerId, B::Hash, NumberFor, bool, bool), AnnounceBlock(B::Hash, Option>), NewBestBlockImported(B::Hash, NumberFor), EventStream(TracingUnboundedSender), @@ -193,10 +193,11 @@ impl Link for SyncingService { hash: &B::Hash, number: NumberFor, success: bool, + should_ban: bool, ) { - let _ = self - .tx - .unbounded_send(ToServiceCommand::JustificationImported(who, *hash, number, success)); + let _ = self.tx.unbounded_send(ToServiceCommand::JustificationImported( + who, *hash, number, success, should_ban, + )); } fn request_justification(&self, hash: &B::Hash, number: NumberFor) { From c2951d1649af4ba8d054f45e009b854f68fa0121 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 15:56:47 +0000 Subject: [PATCH 07/19] consensus: Introduce a JustificationImportResult Signed-off-by: Alexandru Vasile --- .../consensus/common/src/import_queue.rs | 16 ++++++++-- .../common/src/import_queue/basic_queue.rs | 18 ++++++----- .../common/src/import_queue/buffered_link.rs | 14 ++++---- substrate/client/consensus/common/src/lib.rs | 3 +- substrate/client/network/sync/src/engine.rs | 32 ++++++++++++++----- .../client/network/sync/src/service/mock.rs | 3 +- .../sync/src/service/syncing_service.rs | 14 +++++--- 7 files changed, 66 insertions(+), 34 deletions(-) diff --git a/substrate/client/consensus/common/src/import_queue.rs b/substrate/client/consensus/common/src/import_queue.rs index a5cdf70e69d5e..be14d780b2e06 100644 --- a/substrate/client/consensus/common/src/import_queue.rs +++ b/substrate/client/consensus/common/src/import_queue.rs @@ -141,6 +141,19 @@ pub trait ImportQueue: Send { async fn run(self, link: &dyn Link); } +/// The result of importing a justification. +#[derive(Debug, PartialEq)] +pub enum JustificationImportResult { + /// Justification was imported successfully. + Success, + + /// Justification was not imported successfully. + Failure, + + /// Justification was not imported successfully, because it is outdated. + OutdatedJustification, +} + /// Hooks that the verification queue can use to influence the synchronization /// algorithm. pub trait Link: Send + Sync { @@ -159,8 +172,7 @@ pub trait Link: Send + Sync { _who: RuntimeOrigin, _hash: &B::Hash, _number: NumberFor, - _success: bool, - _should_ban: bool, + _import_result: JustificationImportResult, ) { } diff --git a/substrate/client/consensus/common/src/import_queue/basic_queue.rs b/substrate/client/consensus/common/src/import_queue/basic_queue.rs index 147873b500561..d8879731654c1 100644 --- a/substrate/client/consensus/common/src/import_queue/basic_queue.rs +++ b/substrate/client/consensus/common/src/import_queue/basic_queue.rs @@ -34,7 +34,8 @@ use crate::{ buffered_link::{self, BufferedLinkReceiver, BufferedLinkSender}, import_single_block_metered, verify_single_block_metered, BlockImportError, BlockImportStatus, BoxBlockImport, BoxJustificationImport, ImportQueue, ImportQueueService, - IncomingBlock, Link, RuntimeOrigin, SingleBlockVerificationOutcome, Verifier, LOG_TARGET, + IncomingBlock, JustificationImportResult, Link, RuntimeOrigin, + SingleBlockVerificationOutcome, Verifier, LOG_TARGET, }, metrics::Metrics, }; @@ -342,7 +343,7 @@ impl BlockImportWorker { ) { let started = std::time::Instant::now(); - let (success, should_ban) = match self.justification_import.as_mut() { + let import_result = match self.justification_import.as_mut() { Some(justification_import) => { let result = justification_import .import_justification(hash, number, justification) @@ -359,19 +360,20 @@ impl BlockImportWorker { e }); match result { - Ok(()) => (true, false), - Err(sp_consensus::Error::OutdatedJustification) => (false, false), - Err(_) => (false, true), + Ok(()) => JustificationImportResult::Success, + Err(sp_consensus::Error::OutdatedJustification) => + JustificationImportResult::OutdatedJustification, + Err(_) => JustificationImportResult::Failure, } }, - None => (false, false), + None => JustificationImportResult::Failure, }; if let Some(metrics) = self.metrics.as_ref() { metrics.justification_import_time.observe(started.elapsed().as_secs_f64()); } - self.result_sender.justification_imported(who, &hash, number, success, should_ban); + self.result_sender.justification_imported(who, &hash, number, import_result); } } @@ -585,7 +587,7 @@ mod tests { _who: RuntimeOrigin, hash: &Hash, _number: BlockNumber, - _success: bool, + _import_result: JustificationImportResult, ) { self.events.lock().push(Event::JustificationImported(*hash)) } diff --git a/substrate/client/consensus/common/src/import_queue/buffered_link.rs b/substrate/client/consensus/common/src/import_queue/buffered_link.rs index b7397f81ee316..19b0668da24dc 100644 --- a/substrate/client/consensus/common/src/import_queue/buffered_link.rs +++ b/substrate/client/consensus/common/src/import_queue/buffered_link.rs @@ -38,7 +38,7 @@ //! }); //! ``` -use crate::import_queue::{Link, RuntimeOrigin}; +use crate::import_queue::{JustificationImportResult, Link, RuntimeOrigin}; use futures::prelude::*; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnboundedSender}; use sp_runtime::traits::{Block as BlockT, NumberFor}; @@ -84,7 +84,7 @@ impl Clone for BufferedLinkSender { /// Internal buffered message. pub enum BlockImportWorkerMsg { BlocksProcessed(usize, usize, Vec<(BlockImportResult, B::Hash)>), - JustificationImported(RuntimeOrigin, B::Hash, NumberFor, bool, bool), + JustificationImported(RuntimeOrigin, B::Hash, NumberFor, JustificationImportResult), RequestJustification(B::Hash, NumberFor), } @@ -105,11 +105,9 @@ impl Link for BufferedLinkSender { who: RuntimeOrigin, hash: &B::Hash, number: NumberFor, - success: bool, - should_ban: bool, + import_result: JustificationImportResult, ) { - let msg = - BlockImportWorkerMsg::JustificationImported(who, *hash, number, success, should_ban); + let msg = BlockImportWorkerMsg::JustificationImported(who, *hash, number, import_result); let _ = self.tx.unbounded_send(msg); } @@ -131,8 +129,8 @@ impl BufferedLinkReceiver { match msg { BlockImportWorkerMsg::BlocksProcessed(imported, count, results) => link.blocks_processed(imported, count, results), - BlockImportWorkerMsg::JustificationImported(who, hash, number, success, should_ban) => - link.justification_imported(who, &hash, number, success, should_ban), + BlockImportWorkerMsg::JustificationImported(who, hash, number, import_result) => + link.justification_imported(who, &hash, number, import_result), BlockImportWorkerMsg::RequestJustification(hash, number) => link.request_justification(&hash, number), } diff --git a/substrate/client/consensus/common/src/lib.rs b/substrate/client/consensus/common/src/lib.rs index 6bf1ed0b48b4d..070e663451ffb 100644 --- a/substrate/client/consensus/common/src/lib.rs +++ b/substrate/client/consensus/common/src/lib.rs @@ -29,7 +29,8 @@ pub use block_import::{ }; pub use import_queue::{ import_single_block, BasicQueue, BlockImportError, BlockImportStatus, BoxBlockImport, - BoxJustificationImport, DefaultImportQueue, ImportQueue, IncomingBlock, Link, Verifier, + BoxJustificationImport, DefaultImportQueue, ImportQueue, IncomingBlock, + JustificationImportResult, Link, Verifier, }; mod longest_chain; diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs index 0ba084dc34d34..e25787f43574b 100644 --- a/substrate/client/network/sync/src/engine.rs +++ b/substrate/client/network/sync/src/engine.rs @@ -670,22 +670,38 @@ where ToServiceCommand::BlocksProcessed(imported, count, results) => { self.strategy.on_blocks_processed(imported, count, results); }, - ToServiceCommand::JustificationImported(peer_id, hash, number, success, should_ban) => { + ToServiceCommand::JustificationImported(peer_id, hash, number, import_result) => { + let success = + matches!(import_result, sc_consensus::JustificationImportResult::Success); self.strategy.on_justification_import(hash, number, success); - if !success { - log::info!( - target: LOG_TARGET, - "💔 Invalid justification provided by {peer_id} for #{hash} (should_ban={should_ban})", - ); - if should_ban { + match import_result { + sc_consensus::JustificationImportResult::OutdatedJustification => { + log::info!( + target: LOG_TARGET, + "💔 Outdated justification provided by {peer_id} for #{hash}", + ); + }, + + sc_consensus::JustificationImportResult::Failure => { + log::info!( + target: LOG_TARGET, + "💔 Invalid justification provided by {peer_id} for #{hash}", + ); self.network_service .disconnect_peer(peer_id, self.block_announce_protocol_name.clone()); self.network_service.report_peer( peer_id, ReputationChange::new_fatal("Invalid justification"), ); - } + }, + + _ => { + log::debug!( + target: LOG_TARGET, + "Justification for block #{hash} ({number}) imported from {peer_id} successfully", + ); + }, } }, ToServiceCommand::AnnounceBlock(hash, data) => self.announce_block(hash, data), diff --git a/substrate/client/network/sync/src/service/mock.rs b/substrate/client/network/sync/src/service/mock.rs index 3d9fe60408a9b..1974e70227e3d 100644 --- a/substrate/client/network/sync/src/service/mock.rs +++ b/substrate/client/network/sync/src/service/mock.rs @@ -55,8 +55,7 @@ mockall::mock! { who: PeerId, hash: &B::Hash, number: NumberFor, - success: bool, - should_ban: bool, + import_result: sc_consensus::JustificationImportResult, ); fn request_justification(&self, hash: &B::Hash, number: NumberFor); } diff --git a/substrate/client/network/sync/src/service/syncing_service.rs b/substrate/client/network/sync/src/service/syncing_service.rs index 8e41c9cbb9915..7c0c0b428ddb0 100644 --- a/substrate/client/network/sync/src/service/syncing_service.rs +++ b/substrate/client/network/sync/src/service/syncing_service.rs @@ -21,7 +21,9 @@ use crate::types::{ExtendedPeerInfo, SyncEvent, SyncEventStream, SyncStatus, Syn use futures::{channel::oneshot, Stream}; use sc_network_types::PeerId; -use sc_consensus::{BlockImportError, BlockImportStatus, JustificationSyncLink, Link}; +use sc_consensus::{ + BlockImportError, BlockImportStatus, JustificationImportResult, JustificationSyncLink, Link, +}; use sc_network::{NetworkBlock, NetworkSyncForkRequest}; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedSender}; use sp_runtime::traits::{Block as BlockT, NumberFor}; @@ -44,7 +46,7 @@ pub enum ToServiceCommand { usize, Vec<(Result>, BlockImportError>, B::Hash)>, ), - JustificationImported(PeerId, B::Hash, NumberFor, bool, bool), + JustificationImported(PeerId, B::Hash, NumberFor, JustificationImportResult), AnnounceBlock(B::Hash, Option>), NewBestBlockImported(B::Hash, NumberFor), EventStream(TracingUnboundedSender), @@ -192,11 +194,13 @@ impl Link for SyncingService { who: PeerId, hash: &B::Hash, number: NumberFor, - success: bool, - should_ban: bool, + import_result: JustificationImportResult, ) { let _ = self.tx.unbounded_send(ToServiceCommand::JustificationImported( - who, *hash, number, success, should_ban, + who, + *hash, + number, + import_result, )); } From 56afc58e095ee357d8d2f1242e526b06ba5f20bb Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 27 Jun 2025 16:40:14 +0000 Subject: [PATCH 08/19] grandpa: Check if the signature is valid via wrapper fn Signed-off-by: Alexandru Vasile --- .../client/consensus/grandpa/src/communication/gossip.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/substrate/client/consensus/grandpa/src/communication/gossip.rs b/substrate/client/consensus/grandpa/src/communication/gossip.rs index a6aa063357cb8..79696310aa40d 100644 --- a/substrate/client/consensus/grandpa/src/communication/gossip.rs +++ b/substrate/client/consensus/grandpa/src/communication/gossip.rs @@ -927,7 +927,9 @@ impl Inner { &full.message.signature, full.round.0, full.set_id.0, - ) { + ) + .is_valid() + { debug!(target: LOG_TARGET, "Bad message signature {}", full.message.id); telemetry!( self.config.telemetry; From 57d0234ae39d06a83526c2c7bbed0506b45644d4 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 30 Jun 2025 09:22:32 +0000 Subject: [PATCH 09/19] Update from github-actions[bot] running command 'prdoc --audience node_dev --bump patch minor' --- prdoc/pr_9015.prdoc | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 prdoc/pr_9015.prdoc diff --git a/prdoc/pr_9015.prdoc b/prdoc/pr_9015.prdoc new file mode 100644 index 0000000000000..9e09c0688535c --- /dev/null +++ b/prdoc/pr_9015.prdoc @@ -0,0 +1,45 @@ +title: 'consensus/grandpa: Fix high number of peer disconnects with invalid justification' +doc: +- audience: Node Dev + description: |- + A grandpa race-casse has been identified in the versi-net stack around authority set changes, which leads to the following: + + - T0 / Node A: Completes round (15) + - T1 / Node A: Applies new authority set change and increments the SetID (from 0 to 1) + - T2 / Node B: Sends Precommit for round (15) with SetID (0) -- previous set ID + - T3 / Node B: Applies new authority set change and increments the SetID (1) + + In this scenario, Node B is not aware at the moment of sending justifications that the Set ID has changed. + The downstream effect is that Node A will not be able to verify the signature of justifications, since a different SetID is taken into account. This will cascade through the sync engine, where the Node B is wrongfully banned and disconnected. + + This PR aims to fix the edge-case by making the grandpa resilient to verifying prior setIDs for signatures. + When the signature of the grandpa justification fails to decode, the prior SetID is also verified. If the prior SetID produces a valid signature, then the outdated justification error is propagated through the code (ie `SignatureResult::OutdatedSet`). + + The sync engine will handle the outdated justifications as invalid, but without banning the peer. This leads to increased stability of the network during authority changes, which caused frequent disconnects to versi-net in the past. + + ### Review Notes + - Main changes that verify prior SetId on failures are placed in [check_message_signature_with_buffer](https://github.com/paritytech/polkadot-sdk/pull/9015/files#diff-359d7a46ea285177e5d86979f62f0f04baabf65d595c61bfe44b6fc01af70d89R458-R501) + - Sync engine no longer disconnects outdated justifications in [process_service_command](https://github.com/paritytech/polkadot-sdk/pull/9015/files#diff-9ab3391aa82ee2b2868ece610100f84502edcf40638dba9ed6953b6e572dfba5R678-R703) + + ### Testing Done + - Deployed the PR to versi-net with 40 validators + - Prior we have noticed 10/40 validators disconnecting every 15-20 minutes, leading to instability + - Over past 24h the issue has been mitigated: https://grafana.teleport.parity.io/goto/FPNWlmsHR?orgId=1 + - Note: bootnodes 0 and 1 are currently running outdated versions that do not incorporate this SetID verification improvement + + Part of: https://github.com/paritytech/polkadot-sdk/issues/8872 +crates: +- name: sp-consensus-grandpa + bump: patch +- name: bp-header-chain + bump: patch +- name: sc-consensus-grandpa + bump: patch +- name: sp-blockchain + bump: patch +- name: sp-consensus + bump: patch +- name: sc-consensus + bump: patch +- name: sc-network-sync + bump: patch From 1484009b2cb7c3e389017fcb051f7335c55052cb Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 30 Jun 2025 09:25:02 +0000 Subject: [PATCH 10/19] prdoc: Add node operator Signed-off-by: Alexandru Vasile --- prdoc/pr_9015.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_9015.prdoc b/prdoc/pr_9015.prdoc index 9e09c0688535c..3b5b882c3dc72 100644 --- a/prdoc/pr_9015.prdoc +++ b/prdoc/pr_9015.prdoc @@ -1,6 +1,6 @@ title: 'consensus/grandpa: Fix high number of peer disconnects with invalid justification' doc: -- audience: Node Dev + - audience: [Node Dev, Node Operator] description: |- A grandpa race-casse has been identified in the versi-net stack around authority set changes, which leads to the following: From 0622c430a59360feb3365abf0f3cb28559783399 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 30 Jun 2025 09:30:10 +0000 Subject: [PATCH 11/19] prdoc: Fix format Signed-off-by: Alexandru Vasile --- prdoc/pr_9015.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_9015.prdoc b/prdoc/pr_9015.prdoc index 3b5b882c3dc72..4f317240715af 100644 --- a/prdoc/pr_9015.prdoc +++ b/prdoc/pr_9015.prdoc @@ -1,6 +1,6 @@ title: 'consensus/grandpa: Fix high number of peer disconnects with invalid justification' doc: - - audience: [Node Dev, Node Operator] +- audience: [Node Dev, Node Operator] description: |- A grandpa race-casse has been identified in the versi-net stack around authority set changes, which leads to the following: From 2028753fe02f8a0b140cea3a5003c59c3671c3fc Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 30 Jun 2025 10:31:55 +0000 Subject: [PATCH 12/19] prdoc: Change bumps Signed-off-by: Alexandru Vasile --- prdoc/pr_9015.prdoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/prdoc/pr_9015.prdoc b/prdoc/pr_9015.prdoc index 4f317240715af..47776f12cf01e 100644 --- a/prdoc/pr_9015.prdoc +++ b/prdoc/pr_9015.prdoc @@ -30,16 +30,16 @@ doc: Part of: https://github.com/paritytech/polkadot-sdk/issues/8872 crates: - name: sp-consensus-grandpa - bump: patch + bump: minor - name: bp-header-chain bump: patch - name: sc-consensus-grandpa bump: patch - name: sp-blockchain - bump: patch + bump: minor - name: sp-consensus bump: patch - name: sc-consensus - bump: patch + bump: minor - name: sc-network-sync bump: patch From f6600e626876f7a3b6887dd86e415dbf76af1515 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 30 Jun 2025 10:43:15 +0000 Subject: [PATCH 13/19] prdoc: sp-consensus adjust to minor Signed-off-by: Alexandru Vasile --- prdoc/pr_9015.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_9015.prdoc b/prdoc/pr_9015.prdoc index 47776f12cf01e..7aff98cf2b740 100644 --- a/prdoc/pr_9015.prdoc +++ b/prdoc/pr_9015.prdoc @@ -38,7 +38,7 @@ crates: - name: sp-blockchain bump: minor - name: sp-consensus - bump: patch + bump: minor - name: sc-consensus bump: minor - name: sc-network-sync From dc04fc83231b412b24d099ac972d14bd2f9802fa Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 1 Jul 2025 15:12:46 +0300 Subject: [PATCH 14/19] Update substrate/primitives/consensus/grandpa/src/lib.rs Co-authored-by: Dmitry Markin --- substrate/primitives/consensus/grandpa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index 09e5164e76aab..14b84212063d7 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -424,7 +424,7 @@ pub enum SignatureResult { /// Invalid signature. Invalid, - /// Invalid signature, but the message was signed in the previous set. + /// Valid signature, but the message was signed in the previous set. OutdatedSet, } From cf0081c5d342fb27e0718c18044bbda59d0d59a5 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 1 Jul 2025 15:12:52 +0300 Subject: [PATCH 15/19] Update substrate/primitives/consensus/grandpa/src/lib.rs Co-authored-by: Dmitry Markin --- substrate/primitives/consensus/grandpa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index 14b84212063d7..34e267ca1b7d7 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -491,7 +491,7 @@ where let valid = id.verify(&buf, signature); log::debug!( target: log_target, - "Double check signature for id={id:?} round={round:?} previous_set={prev_set_id:?} valid={valid:?}" + "Previous set signature check for id={id:?} round={round:?} previous_set={prev_set_id:?} valid={valid:?}" ); if valid { From bf9ac86e73a46082237538490c4af33a6f9afe25 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 1 Jul 2025 15:13:00 +0300 Subject: [PATCH 16/19] Update substrate/client/network/sync/src/engine.rs Co-authored-by: Dmitry Markin --- substrate/client/network/sync/src/engine.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs index e25787f43574b..2c145eac5b441 100644 --- a/substrate/client/network/sync/src/engine.rs +++ b/substrate/client/network/sync/src/engine.rs @@ -682,7 +682,6 @@ where "💔 Outdated justification provided by {peer_id} for #{hash}", ); }, - sc_consensus::JustificationImportResult::Failure => { log::info!( target: LOG_TARGET, From 62eec608d6780318ca1f003af982b9ecc19edbae Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 1 Jul 2025 15:13:20 +0300 Subject: [PATCH 17/19] Update substrate/client/network/sync/src/engine.rs Co-authored-by: Dmitry Markin --- substrate/client/network/sync/src/engine.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs index 2c145eac5b441..a8a99c78c4dd2 100644 --- a/substrate/client/network/sync/src/engine.rs +++ b/substrate/client/network/sync/src/engine.rs @@ -694,7 +694,6 @@ where ReputationChange::new_fatal("Invalid justification"), ); }, - _ => { log::debug!( target: LOG_TARGET, From 368df03581103a7ea35dfcaf712d839afc04708f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 1 Jul 2025 15:13:26 +0300 Subject: [PATCH 18/19] Update substrate/client/network/sync/src/engine.rs Co-authored-by: Dmitry Markin --- substrate/client/network/sync/src/engine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs index a8a99c78c4dd2..77abb56a4affd 100644 --- a/substrate/client/network/sync/src/engine.rs +++ b/substrate/client/network/sync/src/engine.rs @@ -694,7 +694,7 @@ where ReputationChange::new_fatal("Invalid justification"), ); }, - _ => { + sc_consensus::JustificationImportResult::Success => { log::debug!( target: LOG_TARGET, "Justification for block #{hash} ({number}) imported from {peer_id} successfully", From 5773ae62205390f993c564c17be7a3ae4a25cd96 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 21 Jul 2025 10:07:09 +0000 Subject: [PATCH 19/19] grandpa: Check directly for zero Signed-off-by: Alexandru Vasile --- substrate/primitives/consensus/grandpa/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/primitives/consensus/grandpa/src/lib.rs b/substrate/primitives/consensus/grandpa/src/lib.rs index 34e267ca1b7d7..6c973964cfdee 100644 --- a/substrate/primitives/consensus/grandpa/src/lib.rs +++ b/substrate/primitives/consensus/grandpa/src/lib.rs @@ -482,11 +482,11 @@ where ); // Check if the signature is valid in the previous set. - let prev_set_id = set_id.checked_sub(1).unwrap_or(0); - if prev_set_id == set_id { + if set_id == 0 { return SignatureResult::Invalid; } + let prev_set_id = set_id - 1; localized_payload_with_buffer(round, prev_set_id, message, buf); let valid = id.verify(&buf, signature); log::debug!(