diff --git a/Cargo.lock b/Cargo.lock index a5e431c7a4aa0..78a88109e4708 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2009,9 +2009,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", @@ -2019,7 +2019,7 @@ dependencies = [ "log", "num-traits", "parity-scale-codec", - "parking_lot 0.11.2", + "parking_lot 0.12.0", "rand 0.8.4", "scale-info", ] @@ -11205,7 +11205,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/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index 2f7fcd4d052b1..77cd847d48169 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/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/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index 0648c874f7dff..e8a3f24006e22 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/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/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 03a4f2ff450a3..ac243a1633ee1 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; @@ -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/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 39f24cb8ea57d..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 { @@ -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/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 7516fbb681c4b..85d80dcfe7cde 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/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 diff --git a/frame/grandpa/Cargo.toml b/frame/grandpa/Cargo.toml index cb4233a26fb33..2090a4ea2e228 100644 --- a/frame/grandpa/Cargo.toml +++ b/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/primitives/finality-grandpa/Cargo.toml b/primitives/finality-grandpa/Cargo.toml index f86e2ab07e673..32945eacf0b93 100644 --- a/primitives/finality-grandpa/Cargo.toml +++ b/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 }