From 750cdad259b147e7a4f643a413f9cc7b3630e491 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 26 Apr 2022 22:31:02 +0100 Subject: [PATCH 1/4] grandpa: fix creation of justification ancestry we would reject commits that have precommits targeting blocks lower than the commit target. when there is an equivocation (or if it the commit is not minimal) it is possible to have such precommits and we should assume that they are the round base. --- client/finality-grandpa/src/finality_proof.rs | 2 +- client/finality-grandpa/src/justification.rs | 43 +++++++++++++++++-- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 03a4f2ff450a3..e9c2da76a22c2 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -237,7 +237,7 @@ where } #[cfg(test)] -pub(crate) mod tests { +mod tests { use super::*; use crate::{authorities::AuthoritySetChanges, BlockNumberOps, ClientError, SetId}; use futures::executor::block_on; diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 39f24cb8ea57d..387352e0d1e70 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -66,16 +66,33 @@ impl GrandpaJustification { Err(Error::Client(ClientError::BadJustification(msg))) }; + // we pick the precommit for the lowest block as the base that + // should serve as the root block for populating ancestry (i.e. + // collect all headers from all precommit blocks to the base) + let (base_hash, base_number) = match commit + .precommits + .iter() + .map(|signed| &signed.precommit) + .min_by_key(|precommit| precommit.target_number) + .map(|precommit| (precommit.target_hash.clone(), precommit.target_number)) + { + None => return error(), + Some(base) => base, + }; + for signed in commit.precommits.iter() { let mut current_hash = signed.precommit.target_hash; loop { - if current_hash == commit.target_hash { + if current_hash == base_hash { break } match client.header(BlockId::Hash(current_hash))? { Some(current_header) => { - if *current_header.number() <= commit.target_number { + // NOTE: this should never happen as we pick the lowest block + // as base and only traverse backwards from the other blocks + // in the commit. but better be safe to avoid an unbound loop. + if *current_header.number() <= base_number { return error() } @@ -83,6 +100,7 @@ impl GrandpaJustification { if votes_ancestries_hashes.insert(current_hash) { votes_ancestries.push(current_header); } + current_hash = parent_hash; }, _ => return error(), @@ -149,6 +167,23 @@ impl GrandpaJustification { }, } + // we pick the precommit for the lowest block as the base that + // should serve as the root block for populating ancestry (i.e. + // collect all headers from all precommit blocks to the base) + let base_hash = self + .commit + .precommits + .iter() + .map(|signed| &signed.precommit) + .min_by_key(|precommit| precommit.target_number) + .map(|precommit| precommit.target_hash.clone()) + .expect( + "can only fail if precommits is empty; \ + commit has been validated above; \ + valid commits must include precommits; \ + qed.", + ); + let mut buf = Vec::new(); let mut visited_hashes = HashSet::new(); for signed in self.commit.precommits.iter() { @@ -165,11 +200,11 @@ impl GrandpaJustification { )) } - if self.commit.target_hash == signed.precommit.target_hash { + if base_hash == signed.precommit.target_hash { continue } - match ancestry_chain.ancestry(self.commit.target_hash, signed.precommit.target_hash) { + match ancestry_chain.ancestry(base_hash, signed.precommit.target_hash) { Ok(route) => { // ancestry starts from parent hash but the precommit target hash has been // visited From 5a48a9ae709618c710f4c3beb962dae889e91c7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 26 Apr 2022 22:34:12 +0100 Subject: [PATCH 2/4] grandpa: bump to 0.16.0 --- Cargo.lock | 6 ++---- client/finality-grandpa/Cargo.toml | 4 ++-- client/finality-grandpa/rpc/Cargo.toml | 2 +- client/finality-grandpa/src/justification.rs | 2 +- client/finality-grandpa/src/observer.rs | 2 +- frame/grandpa/Cargo.toml | 2 +- primitives/finality-grandpa/Cargo.toml | 2 +- 7 files changed, 9 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 486cb083bca99..0628bedb98604 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2059,9 +2059,7 @@ dependencies = [ [[package]] name = "finality-grandpa" -version = "0.15.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d9def033d8505edf199f6a5d07aa7e6d2d6185b164293b77f0efd108f4f3e11d" +version = "0.16.0" dependencies = [ "either", "futures 0.3.21", @@ -2069,7 +2067,7 @@ dependencies = [ "log 0.4.16", "num-traits", "parity-scale-codec", - "parking_lot 0.11.2", + "parking_lot 0.12.0", "rand 0.8.4", "scale-info", ] diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index 2d011fb96be59..b3d9ed85db320 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/client/finality-grandpa/Cargo.toml @@ -45,12 +45,12 @@ sc-network-gossip = { version = "0.10.0-dev", path = "../network-gossip" } sp-finality-grandpa = { version = "4.0.0-dev", path = "../../primitives/finality-grandpa" } prometheus-endpoint = { package = "substrate-prometheus-endpoint", path = "../../utils/prometheus", version = "0.10.0-dev" } sc-block-builder = { version = "0.10.0-dev", path = "../block-builder" } -finality-grandpa = { version = "0.15.0", features = ["derive-codec"] } +finality-grandpa = { version = "0.16.0", features = ["derive-codec"] } async-trait = "0.1.50" [dev-dependencies] assert_matches = "1.3.0" -finality-grandpa = { version = "0.15.0", features = [ +finality-grandpa = { version = "0.16.0", features = [ "derive-codec", "test-helpers", ] } diff --git a/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index 5e173e1a15fe4..ae07197eb0207 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/client/finality-grandpa/rpc/Cargo.toml @@ -14,7 +14,7 @@ sc-rpc = { version = "4.0.0-dev", path = "../../rpc" } sp-blockchain = { version = "4.0.0-dev", path = "../../../primitives/blockchain" } sp-core = { version = "6.0.0", path = "../../../primitives/core" } sp-runtime = { version = "6.0.0", path = "../../../primitives/runtime" } -finality-grandpa = { version = "0.15.0", features = ["derive-codec"] } +finality-grandpa = { version = "0.16.0", features = ["derive-codec"] } jsonrpc-core = "18.0.0" jsonrpc-core-client = "18.0.0" jsonrpc-derive = "18.0.0" diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 387352e0d1e70..11b8873dcdc22 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -160,7 +160,7 @@ impl GrandpaJustification { let ancestry_chain = AncestryChain::::new(&self.votes_ancestries); match finality_grandpa::validate_commit(&self.commit, voters, &ancestry_chain) { - Ok(ref result) if result.ghost().is_some() => {}, + Ok(ref result) if result.is_valid() => {}, _ => { let msg = "invalid commit in grandpa justification".to_string(); return Err(ClientError::BadJustification(msg)) diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index a7c951cc33db9..9b002aa316900 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -116,7 +116,7 @@ where Err(e) => return future::err(e.into()), }; - if validation_result.ghost().is_some() { + if validation_result.is_valid() { let finalized_hash = commit.target_hash; let finalized_number = commit.target_number; diff --git a/frame/grandpa/Cargo.toml b/frame/grandpa/Cargo.toml index 7312ae7da4cf8..aaa2ea910e36a 100644 --- a/frame/grandpa/Cargo.toml +++ b/frame/grandpa/Cargo.toml @@ -32,7 +32,7 @@ log = { version = "0.4.16", default-features = false } [dev-dependencies] frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking" } -grandpa = { package = "finality-grandpa", version = "0.15.0", features = ["derive-codec"] } +grandpa = { package = "finality-grandpa", version = "0.16.0", features = ["derive-codec"] } sp-keyring = { version = "6.0.0", path = "../../primitives/keyring" } pallet-balances = { version = "4.0.0-dev", path = "../balances" } pallet-offences = { version = "4.0.0-dev", path = "../offences" } diff --git a/primitives/finality-grandpa/Cargo.toml b/primitives/finality-grandpa/Cargo.toml index e6464207e67f2..4a502767b4c48 100644 --- a/primitives/finality-grandpa/Cargo.toml +++ b/primitives/finality-grandpa/Cargo.toml @@ -17,7 +17,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] } scale-info = { version = "2.0.1", default-features = false, features = ["derive"] } -grandpa = { package = "finality-grandpa", version = "0.15.0", default-features = false, features = ["derive-codec"] } +grandpa = { package = "finality-grandpa", version = "0.16.0", default-features = false, features = ["derive-codec"] } log = { version = "0.4.16", optional = true } serde = { version = "1.0.136", optional = true, features = ["derive"] } sp-api = { version = "4.0.0-dev", default-features = false, path = "../api" } From 367174b00e6f6cc50e972b02c1aa6c484456921b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 26 Apr 2022 22:39:31 +0100 Subject: [PATCH 3/4] grandpa: add test for justification with equivocation --- client/finality-grandpa/src/tests.rs | 67 ++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 5083cbfc21d1d..2d12232b04f15 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1569,6 +1569,73 @@ fn grandpa_environment_never_overwrites_round_voter_state() { assert_matches!(get_current_round(2).unwrap(), HasVoted::Yes(_, _)); } +#[test] +fn justification_with_equivocation() { + use sp_application_crypto::Pair; + + // we have 100 authorities + let pairs = (0..100).map(|n| AuthorityPair::from_seed(&[n; 32])).collect::>(); + let voters = pairs.iter().map(AuthorityPair::public).map(|id| (id, 1)).collect::>(); + let api = TestApi::new(voters.clone()); + let mut net = GrandpaTestNet::new(api.clone(), 1, 0); + + // we create a basic chain with 3 blocks (no forks) + net.peer(0).push_blocks(3, false); + + let client = net.peer(0).client().clone(); + let block1 = client.header(&BlockId::Number(1)).ok().flatten().unwrap(); + let block2 = client.header(&BlockId::Number(2)).ok().flatten().unwrap(); + let block3 = client.header(&BlockId::Number(3)).ok().flatten().unwrap(); + + let set_id = 0; + let justification = { + let round = 1; + + let make_precommit = |target_hash, target_number, pair: &AuthorityPair| { + let precommit = finality_grandpa::Precommit { target_hash, target_number }; + + let msg = finality_grandpa::Message::Precommit(precommit.clone()); + let encoded = sp_finality_grandpa::localized_payload(round, set_id, &msg); + + let precommit = finality_grandpa::SignedPrecommit { + precommit: precommit.clone(), + signature: pair.sign(&encoded[..]), + id: pair.public(), + }; + + precommit + }; + + let mut precommits = Vec::new(); + + // we have 66/100 votes for block #3 and therefore do not have threshold to finalize + for pair in pairs.iter().take(66) { + let precommit = make_precommit(block3.hash(), *block3.number(), pair); + precommits.push(precommit); + } + + // we create an equivocation for the 67th validator targetting blocks #1 and #2. + // this should be accounted as "voting for all blocks" and therefore block #3 will + // have 67/100 votes, reaching finality threshold. + { + precommits.push(make_precommit(block1.hash(), *block1.number(), &pairs[66])); + precommits.push(make_precommit(block2.hash(), *block2.number(), &pairs[66])); + } + + let commit = finality_grandpa::Commit { + target_hash: block3.hash(), + target_number: *block3.number(), + precommits, + }; + + GrandpaJustification::from_commit(&client.as_client(), round, commit).unwrap() + }; + + // the justification should include the minimal necessary vote ancestry and + // the commit should be valid + assert!(justification.verify(set_id, &voters).is_ok()); +} + #[test] fn imports_justification_for_regular_blocks_on_import() { // NOTE: this is a regression test since initially we would only import From 845f43a634a926455cba65dbacd5c470da56fd8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 13 Jun 2022 16:11:22 +0100 Subject: [PATCH 4/4] grandpa: fix failing test --- client/finality-grandpa/src/finality_proof.rs | 7 +++++-- client/finality-grandpa/src/justification.rs | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index e9c2da76a22c2..ac243a1633ee1 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -271,6 +271,7 @@ mod tests { let justification: GrandpaJustification = Decode::decode(&mut &proof.justification[..]) .map_err(|_| ClientError::JustificationDecode)?; + justification.verify(current_set_id, ¤t_authorities)?; Ok(proof) @@ -370,7 +371,7 @@ mod tests { #[test] fn finality_proof_check_fails_with_incomplete_justification() { - let (client, _, blocks) = test_blockchain(8, &[4, 5, 8]); + let (_, _, blocks) = test_blockchain(8, &[4, 5, 8]); // Create a commit without precommits let commit = finality_grandpa::Commit { @@ -378,7 +379,9 @@ mod tests { target_number: *blocks[7].header().number(), precommits: Vec::new(), }; - let grandpa_just = GrandpaJustification::from_commit(&client, 8, commit).unwrap(); + + let grandpa_just = + GrandpaJustification:: { round: 8, votes_ancestries: Vec::new(), commit }; let finality_proof = FinalityProof { block: header(2).hash(), diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 11b8873dcdc22..44abb4b95beba 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -42,9 +42,9 @@ use crate::{AuthorityList, Commit, Error}; /// nodes, and are used by syncing nodes to prove authority set handoffs. #[derive(Clone, Encode, Decode, PartialEq, Eq, Debug)] pub struct GrandpaJustification { - round: u64, + pub(crate) round: u64, pub(crate) commit: Commit, - votes_ancestries: Vec, + pub(crate) votes_ancestries: Vec, } impl GrandpaJustification {