From 39bf5b28bb0e841ae4017ac8fb064a0d2c3c6e33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 4 Jan 2023 12:48:28 +0000 Subject: [PATCH 1/2] grandpa: remove deprecated afg log target --- .../src/communication/gossip.rs | 1 + .../finality-grandpa/src/communication/mod.rs | 3 ++- client/finality-grandpa/src/justification.rs | 3 ++- client/tracing/src/logging/mod.rs | 8 +++---- frame/grandpa/src/benchmarking.rs | 4 ++-- frame/grandpa/src/lib.rs | 2 +- primitives/finality-grandpa/Cargo.toml | 4 ++-- primitives/finality-grandpa/src/lib.rs | 22 +++++++++++++------ 8 files changed, 29 insertions(+), 18 deletions(-) diff --git a/client/finality-grandpa/src/communication/gossip.rs b/client/finality-grandpa/src/communication/gossip.rs index cbcafc727d436..6d459d637217f 100644 --- a/client/finality-grandpa/src/communication/gossip.rs +++ b/client/finality-grandpa/src/communication/gossip.rs @@ -937,6 +937,7 @@ impl Inner { &full.message.signature, full.round.0, full.set_id.0, + LOG_TARGET, ) { debug!(target: LOG_TARGET, "Bad message signature {}", full.message.id); telemetry!( diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index e7e3c12989c96..e641d62835465 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -849,6 +849,7 @@ fn check_compact_commit( round.0, set_id.0, &mut buf, + LOG_TARGET, ) { debug!(target: LOG_TARGET, "Bad commit message signature {}", id); telemetry!( @@ -935,7 +936,7 @@ fn check_catch_up( signatures_checked += 1; if !sp_finality_grandpa::check_message_signature_with_buffer( - &msg, id, sig, round, set_id, buf, + &msg, id, sig, round, set_id, buf, LOG_TARGET, ) { debug!(target: LOG_TARGET, "Bad catch up message signature {}", id); telemetry!( diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 96c31d4c0cc29..bef80b38bcb2a 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -28,7 +28,7 @@ use sp_blockchain::{Error as ClientError, HeaderBackend}; use sp_finality_grandpa::AuthorityId; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; -use crate::{AuthorityList, Commit, Error}; +use crate::{AuthorityList, Commit, Error, LOG_TARGET}; /// A GRANDPA justification for block finality, it includes a commit message and /// an ancestry proof including all headers routing all precommit target blocks @@ -212,6 +212,7 @@ impl GrandpaJustification { self.justification.round, set_id, &mut buf, + LOG_TARGET, ) { return Err(ClientError::BadJustification( "invalid signature for precommit in grandpa justification".to_string(), diff --git a/client/tracing/src/logging/mod.rs b/client/tracing/src/logging/mod.rs index 978e24df68d78..77307ad999daf 100644 --- a/client/tracing/src/logging/mod.rs +++ b/client/tracing/src/logging/mod.rs @@ -377,7 +377,7 @@ mod tests { fn test_logger_filters() { run_test_in_another_process("test_logger_filters", || { let test_directives = - "afg=debug,sync=trace,client=warn,telemetry,something-with-dash=error"; + "grandpa=debug,sync=trace,client=warn,telemetry,something-with-dash=error"; init_logger(&test_directives); tracing::dispatcher::get_default(|dispatcher| { @@ -402,9 +402,9 @@ mod tests { dispatcher.enabled(&metadata) }; - assert!(test_filter("afg", Level::INFO)); - assert!(test_filter("afg", Level::DEBUG)); - assert!(!test_filter("afg", Level::TRACE)); + assert!(test_filter("grandpa", Level::INFO)); + assert!(test_filter("grandpa", Level::DEBUG)); + assert!(!test_filter("grandpa", Level::TRACE)); assert!(test_filter("sync", Level::TRACE)); assert!(test_filter("client", Level::WARN)); diff --git a/frame/grandpa/src/benchmarking.rs b/frame/grandpa/src/benchmarking.rs index 1240859149920..93682af9658e9 100644 --- a/frame/grandpa/src/benchmarking.rs +++ b/frame/grandpa/src/benchmarking.rs @@ -55,9 +55,9 @@ benchmarks! { let equivocation_proof2 = equivocation_proof1.clone(); }: { - sp_finality_grandpa::check_equivocation_proof(equivocation_proof1); + sp_finality_grandpa::check_equivocation_proof(equivocation_proof1, "grandpa"); } verify { - assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2)); + assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2, "grandpa")); } note_stalled { diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index 716cf54865773..c7dc5eded1ce6 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -542,7 +542,7 @@ impl Pallet { // validate equivocation proof (check votes are different and // signatures are valid). - if !sp_finality_grandpa::check_equivocation_proof(equivocation_proof) { + if !sp_finality_grandpa::check_equivocation_proof(equivocation_proof, LOG_TARGET) { return Err(Error::::InvalidEquivocationProof.into()) } diff --git a/primitives/finality-grandpa/Cargo.toml b/primitives/finality-grandpa/Cargo.toml index 1c8011ff764e3..de8de99eed891 100644 --- a/primitives/finality-grandpa/Cargo.toml +++ b/primitives/finality-grandpa/Cargo.toml @@ -16,7 +16,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] } grandpa = { package = "finality-grandpa", version = "0.16.0", default-features = false, features = ["derive-codec"] } -log = { version = "0.4.17", optional = true } +log = { version = "0.4.17", default-features = false } scale-info = { version = "2.1.1", default-features = false, features = ["derive"] } serde = { version = "1.0.136", features = ["derive"], optional = true } sp-api = { version = "4.0.0-dev", default-features = false, path = "../api" } @@ -31,7 +31,7 @@ default = ["std"] std = [ "codec/std", "grandpa/std", - "log", + "log/std", "scale-info/std", "serde", "sp-api/std", diff --git a/primitives/finality-grandpa/src/lib.rs b/primitives/finality-grandpa/src/lib.rs index f92fee4f12963..9c0eca3a1bf96 100644 --- a/primitives/finality-grandpa/src/lib.rs +++ b/primitives/finality-grandpa/src/lib.rs @@ -35,9 +35,6 @@ use sp_runtime::{ }; use sp_std::{borrow::Cow, vec::Vec}; -#[cfg(feature = "std")] -use log::debug; - /// Key type for GRANDPA module. pub const KEY_TYPE: sp_core::crypto::KeyTypeId = sp_application_crypto::key_types::GRANDPA; @@ -322,7 +319,7 @@ impl Equivocation { /// Verifies the equivocation proof by making sure that both votes target /// different blocks and that its signatures are valid. -pub fn check_equivocation_proof(report: EquivocationProof) -> bool +pub fn check_equivocation_proof(report: EquivocationProof, log_target: &str) -> bool where H: Clone + Encode + PartialEq, N: Clone + Encode + PartialEq, @@ -345,6 +342,7 @@ where &$equivocation.first.1, $equivocation.round_number, report.set_id, + log_target, ); let valid_second = check_message_signature( @@ -353,6 +351,7 @@ where &$equivocation.second.1, $equivocation.round_number, report.set_id, + log_target, ); return valid_first && valid_second @@ -397,12 +396,21 @@ pub fn check_message_signature( signature: &AuthoritySignature, round: RoundNumber, set_id: SetId, + log_target: &str, ) -> bool where H: Encode, N: Encode, { - check_message_signature_with_buffer(message, id, signature, round, set_id, &mut Vec::new()) + check_message_signature_with_buffer( + message, + id, + signature, + round, + set_id, + &mut Vec::new(), + log_target, + ) } /// Check a message signature by encoding the message as a localized payload and @@ -416,6 +424,7 @@ pub fn check_message_signature_with_buffer( round: RoundNumber, set_id: SetId, buf: &mut Vec, + log_target: &str, ) -> bool where H: Encode, @@ -428,8 +437,7 @@ where let valid = id.verify(&buf, signature); if !valid { - #[cfg(feature = "std")] - debug!(target: "afg", "Bad signature on message from {:?}", id); + log::debug!(target: log_target, "Bad signature on message from {:?}", id); } valid From 2f175726925ef47d96dafb39c9b2f26299faf9d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 5 Jan 2023 11:02:18 +0000 Subject: [PATCH 2/2] grandpa: define log targets in primitives --- .../src/communication/gossip.rs | 1 - .../finality-grandpa/src/communication/mod.rs | 3 +-- client/finality-grandpa/src/justification.rs | 3 +-- client/finality-grandpa/src/lib.rs | 6 ++--- frame/grandpa/src/benchmarking.rs | 4 ++-- frame/grandpa/src/lib.rs | 6 ++--- primitives/finality-grandpa/src/lib.rs | 23 ++++++++----------- 7 files changed, 18 insertions(+), 28 deletions(-) diff --git a/client/finality-grandpa/src/communication/gossip.rs b/client/finality-grandpa/src/communication/gossip.rs index 6d459d637217f..cbcafc727d436 100644 --- a/client/finality-grandpa/src/communication/gossip.rs +++ b/client/finality-grandpa/src/communication/gossip.rs @@ -937,7 +937,6 @@ impl Inner { &full.message.signature, full.round.0, full.set_id.0, - LOG_TARGET, ) { debug!(target: LOG_TARGET, "Bad message signature {}", full.message.id); telemetry!( diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index e641d62835465..e7e3c12989c96 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -849,7 +849,6 @@ fn check_compact_commit( round.0, set_id.0, &mut buf, - LOG_TARGET, ) { debug!(target: LOG_TARGET, "Bad commit message signature {}", id); telemetry!( @@ -936,7 +935,7 @@ fn check_catch_up( signatures_checked += 1; if !sp_finality_grandpa::check_message_signature_with_buffer( - &msg, id, sig, round, set_id, buf, LOG_TARGET, + &msg, id, sig, round, set_id, buf, ) { debug!(target: LOG_TARGET, "Bad catch up message signature {}", id); telemetry!( diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index bef80b38bcb2a..96c31d4c0cc29 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -28,7 +28,7 @@ use sp_blockchain::{Error as ClientError, HeaderBackend}; use sp_finality_grandpa::AuthorityId; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; -use crate::{AuthorityList, Commit, Error, LOG_TARGET}; +use crate::{AuthorityList, Commit, Error}; /// A GRANDPA justification for block finality, it includes a commit message and /// an ancestry proof including all headers routing all precommit target blocks @@ -212,7 +212,6 @@ impl GrandpaJustification { self.justification.round, set_id, &mut buf, - LOG_TARGET, ) { return Err(ClientError::BadJustification( "invalid signature for precommit in grandpa justification".to_string(), diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 1597e60bd6061..8758efef6037f 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -76,6 +76,9 @@ use sp_application_crypto::AppKey; use sp_blockchain::{Error as ClientError, HeaderBackend, HeaderMetadata, Result as ClientResult}; use sp_consensus::SelectChain; use sp_core::crypto::ByteArray; +use sp_finality_grandpa::{ + AuthorityList, AuthoritySignature, SetId, CLIENT_LOG_TARGET as LOG_TARGET, +}; use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; use sp_runtime::{ generic::BlockId, @@ -93,8 +96,6 @@ use std::{ time::Duration, }; -const LOG_TARGET: &str = "grandpa"; - // utility logging macro that takes as first argument a conditional to // decide whether to log under debug or info level (useful to restrict // logging under initial sync). @@ -142,7 +143,6 @@ pub use voting_rule::{ use aux_schema::PersistentData; use communication::{Network as NetworkT, NetworkBridge}; use environment::{Environment, VoterSetState}; -use sp_finality_grandpa::{AuthorityList, AuthoritySignature, SetId}; use until_imported::UntilGlobalMessageBlocksImported; // Re-export these two because it's just so damn convenient. diff --git a/frame/grandpa/src/benchmarking.rs b/frame/grandpa/src/benchmarking.rs index 93682af9658e9..1240859149920 100644 --- a/frame/grandpa/src/benchmarking.rs +++ b/frame/grandpa/src/benchmarking.rs @@ -55,9 +55,9 @@ benchmarks! { let equivocation_proof2 = equivocation_proof1.clone(); }: { - sp_finality_grandpa::check_equivocation_proof(equivocation_proof1, "grandpa"); + sp_finality_grandpa::check_equivocation_proof(equivocation_proof1); } verify { - assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2, "grandpa")); + assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2)); } note_stalled { diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index c7dc5eded1ce6..aa09b445c6bdd 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -37,7 +37,7 @@ use codec::{self as codec, Decode, Encode, MaxEncodedLen}; pub use fg_primitives::{AuthorityId, AuthorityList, AuthorityWeight, VersionedAuthorityList}; use fg_primitives::{ ConsensusLog, EquivocationProof, ScheduledChange, SetId, GRANDPA_AUTHORITIES_KEY, - GRANDPA_ENGINE_ID, + GRANDPA_ENGINE_ID, RUNTIME_LOG_TARGET as LOG_TARGET, }; use frame_support::{ dispatch::{DispatchResultWithPostInfo, Pays}, @@ -70,8 +70,6 @@ pub use equivocation::{ pub use pallet::*; -const LOG_TARGET: &str = "runtime::grandpa"; - #[frame_support::pallet] pub mod pallet { use super::*; @@ -542,7 +540,7 @@ impl Pallet { // validate equivocation proof (check votes are different and // signatures are valid). - if !sp_finality_grandpa::check_equivocation_proof(equivocation_proof, LOG_TARGET) { + if !sp_finality_grandpa::check_equivocation_proof(equivocation_proof) { return Err(Error::::InvalidEquivocationProof.into()) } diff --git a/primitives/finality-grandpa/src/lib.rs b/primitives/finality-grandpa/src/lib.rs index 9c0eca3a1bf96..7b3b55f49cee0 100644 --- a/primitives/finality-grandpa/src/lib.rs +++ b/primitives/finality-grandpa/src/lib.rs @@ -35,6 +35,11 @@ use sp_runtime::{ }; use sp_std::{borrow::Cow, vec::Vec}; +/// The log target to be used by client code. +pub const CLIENT_LOG_TARGET: &str = "grandpa"; +/// The log target to be used by runtime code. +pub const RUNTIME_LOG_TARGET: &str = "runtime::grandpa"; + /// Key type for GRANDPA module. pub const KEY_TYPE: sp_core::crypto::KeyTypeId = sp_application_crypto::key_types::GRANDPA; @@ -319,7 +324,7 @@ impl Equivocation { /// Verifies the equivocation proof by making sure that both votes target /// different blocks and that its signatures are valid. -pub fn check_equivocation_proof(report: EquivocationProof, log_target: &str) -> bool +pub fn check_equivocation_proof(report: EquivocationProof) -> bool where H: Clone + Encode + PartialEq, N: Clone + Encode + PartialEq, @@ -342,7 +347,6 @@ where &$equivocation.first.1, $equivocation.round_number, report.set_id, - log_target, ); let valid_second = check_message_signature( @@ -351,7 +355,6 @@ where &$equivocation.second.1, $equivocation.round_number, report.set_id, - log_target, ); return valid_first && valid_second @@ -396,21 +399,12 @@ pub fn check_message_signature( signature: &AuthoritySignature, round: RoundNumber, set_id: SetId, - log_target: &str, ) -> bool where H: Encode, N: Encode, { - check_message_signature_with_buffer( - message, - id, - signature, - round, - set_id, - &mut Vec::new(), - log_target, - ) + check_message_signature_with_buffer(message, id, signature, round, set_id, &mut Vec::new()) } /// Check a message signature by encoding the message as a localized payload and @@ -424,7 +418,6 @@ pub fn check_message_signature_with_buffer( round: RoundNumber, set_id: SetId, buf: &mut Vec, - log_target: &str, ) -> bool where H: Encode, @@ -437,6 +430,8 @@ where let valid = id.verify(&buf, signature); 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); }