From 06cf8ad076c16a301757e3ab7dbae52db2867d69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Tue, 14 Jun 2022 11:41:11 +0100 Subject: [PATCH] grandpa: fix creation of justification with equivocating precommits in commit (#11302) * 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. * grandpa: bump to 0.16.0 * grandpa: add test for justification with equivocation * grandpa: fix failing test --- substrate/Cargo.lock | 8 +-- substrate/client/finality-grandpa/Cargo.toml | 4 +- .../client/finality-grandpa/rpc/Cargo.toml | 2 +- .../finality-grandpa/src/finality_proof.rs | 9 ++- .../finality-grandpa/src/justification.rs | 49 ++++++++++++-- .../client/finality-grandpa/src/observer.rs | 2 +- .../client/finality-grandpa/src/tests.rs | 67 +++++++++++++++++++ substrate/frame/grandpa/Cargo.toml | 2 +- .../primitives/finality-grandpa/Cargo.toml | 2 +- 9 files changed, 125 insertions(+), 20 deletions(-) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 10d34da9cf..cf56bf0703 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -2021,9 +2021,9 @@ dependencies = [ [[package]] name = "finality-grandpa" -version = "0.15.0" +version = "0.16.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d9def033d8505edf199f6a5d07aa7e6d2d6185b164293b77f0efd108f4f3e11d" +checksum = "b22349c6a11563a202d95772a68e0fcf56119e74ea8a2a19cf2301460fcd0df5" dependencies = [ "either", "futures", @@ -2031,7 +2031,7 @@ dependencies = [ "log", "num-traits", "parity-scale-codec", - "parking_lot 0.11.2", + "parking_lot 0.12.0", "rand 0.8.4", "scale-info", ] @@ -11242,7 +11242,7 @@ version = "1.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4ee73e6e4924fe940354b8d4d98cad5231175d615cd855b758adc658c0aac6a0" dependencies = [ - "cfg-if 0.1.10", + "cfg-if 1.0.0", "digest 0.10.3", "rand 0.8.4", "static_assertions", diff --git a/substrate/client/finality-grandpa/Cargo.toml b/substrate/client/finality-grandpa/Cargo.toml index 2f7fcd4d05..77cd847d48 100644 --- a/substrate/client/finality-grandpa/Cargo.toml +++ b/substrate/client/finality-grandpa/Cargo.toml @@ -17,7 +17,7 @@ targets = ["x86_64-unknown-linux-gnu"] ahash = "0.7.6" async-trait = "0.1.50" dyn-clone = "1.0" -finality-grandpa = { version = "0.15.0", features = ["derive-codec"] } +finality-grandpa = { version = "0.16.0", features = ["derive-codec"] } futures = "0.3.21" futures-timer = "3.0.1" hex = "0.4.2" @@ -50,7 +50,7 @@ sp-runtime = { version = "6.0.0", path = "../../primitives/runtime" } [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/substrate/client/finality-grandpa/rpc/Cargo.toml b/substrate/client/finality-grandpa/rpc/Cargo.toml index 0648c874f7..e8a3f24006 100644 --- a/substrate/client/finality-grandpa/rpc/Cargo.toml +++ b/substrate/client/finality-grandpa/rpc/Cargo.toml @@ -10,7 +10,7 @@ readme = "README.md" homepage = "https://substrate.io" [dependencies] -finality-grandpa = { version = "0.15.0", features = ["derive-codec"] } +finality-grandpa = { version = "0.16.0", features = ["derive-codec"] } futures = "0.3.16" jsonrpsee = { version = "0.13.0", features = ["server", "macros"] } log = "0.4.8" diff --git a/substrate/client/finality-grandpa/src/finality_proof.rs b/substrate/client/finality-grandpa/src/finality_proof.rs index 03a4f2ff45..ac243a1633 100644 --- a/substrate/client/finality-grandpa/src/finality_proof.rs +++ b/substrate/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; @@ -271,6 +271,7 @@ pub(crate) 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 @@ pub(crate) 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 @@ pub(crate) 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/substrate/client/finality-grandpa/src/justification.rs b/substrate/client/finality-grandpa/src/justification.rs index 39f24cb8ea..44abb4b95b 100644 --- a/substrate/client/finality-grandpa/src/justification.rs +++ b/substrate/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 { @@ -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(), @@ -142,13 +160,30 @@ 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)) }, } + // 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 diff --git a/substrate/client/finality-grandpa/src/observer.rs b/substrate/client/finality-grandpa/src/observer.rs index 7516fbb681..85d80dcfe7 100644 --- a/substrate/client/finality-grandpa/src/observer.rs +++ b/substrate/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/substrate/client/finality-grandpa/src/tests.rs b/substrate/client/finality-grandpa/src/tests.rs index 5083cbfc21..2d12232b04 100644 --- a/substrate/client/finality-grandpa/src/tests.rs +++ b/substrate/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 diff --git a/substrate/frame/grandpa/Cargo.toml b/substrate/frame/grandpa/Cargo.toml index cb4233a26f..2090a4ea2e 100644 --- a/substrate/frame/grandpa/Cargo.toml +++ b/substrate/frame/grandpa/Cargo.toml @@ -31,7 +31,7 @@ sp-staking = { version = "4.0.0-dev", default-features = false, path = "../../pr sp-std = { version = "4.0.0", default-features = false, path = "../../primitives/std" } [dev-dependencies] -grandpa = { package = "finality-grandpa", version = "0.15.0", features = ["derive-codec"] } +grandpa = { package = "finality-grandpa", version = "0.16.0", features = ["derive-codec"] } frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking" } frame-election-provider-support = { version = "4.0.0-dev", path = "../election-provider-support" } pallet-balances = { version = "4.0.0-dev", path = "../balances" } diff --git a/substrate/primitives/finality-grandpa/Cargo.toml b/substrate/primitives/finality-grandpa/Cargo.toml index f86e2ab07e..32945eacf0 100644 --- a/substrate/primitives/finality-grandpa/Cargo.toml +++ b/substrate/primitives/finality-grandpa/Cargo.toml @@ -15,7 +15,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.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.17", optional = true } scale-info = { version = "2.1.1", default-features = false, features = ["derive"] } serde = { version = "1.0.136", features = ["derive"], optional = true }