From f2b7aaa1299cb123d980a780e48ee75c24441bd8 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 23 Oct 2019 16:32:30 +0200 Subject: [PATCH 01/11] Make StorageProofChecker happy --- Cargo.lock | 1 - srml/bridge/Cargo.toml | 2 +- srml/bridge/src/lib.rs | 57 +++++++++++++++++++++++++++++++----------- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e8b18d94a793d..054e70f7f2581 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4085,7 +4085,6 @@ dependencies = [ "srml-session 2.0.0", "srml-support 2.0.0", "srml-system 2.0.0", - "substrate-externalities 2.0.0", "substrate-primitives 2.0.0", "substrate-state-machine 2.0.0", "substrate-trie 2.0.0", diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 92c93954bf186..5085acc60ced6 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -9,6 +9,7 @@ edition = "2018" [dependencies] codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } hash-db = { version = "0.15.2", default-features = false } +primitives = { package = "substrate-primitives", path = "../../core/primitives" } session = { package = "srml-session", path = "../session", default-features = false, features = ["historical"] } serde = { version = "1.0", optional = true } sr-primitives = { path = "../../core/sr-primitives", default-features = false } @@ -17,7 +18,6 @@ system = { package = "srml-system", path = "../system", default-features = false trie = { package = "substrate-trie", path = "../../core/trie", default-features = false } [dev-dependencies] -primitives = { package = "substrate-primitives", path = "../../core/primitives" } runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false } state-machine = { package = "substrate-state-machine", path = "../../core/state-machine" } diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index f437c0445dd5b..5b7dfced16c5b 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -35,7 +35,9 @@ mod error; mod storage_proof; +use crate::storage_proof::StorageProofChecker; use codec::{Encode, Decode}; +use primitives::{Blake2Hasher}; use sr_primitives::traits::{Header, Member}; use support::{ decl_error, decl_module, decl_storage, @@ -90,32 +92,27 @@ decl_storage! { decl_module! { pub struct Module for enum Call where origin: T::Origin { - // TODO: Figure out the proper type for these proofs + // TODO: Change proof type to StorageProof once #3834 is merged fn initialize_bridge( origin, block_header: T::Header, validator_set: Vec, - validator_set_proof: Vec, - storage_proof: Vec, + validator_set_proof: Vec>, ) { // NOTE: Will want to make this a governance issued call let _sender = ensure_signed(origin)?; - Self::check_storage_proof(storage_proof)?; - Self::check_validator_set_proof(validator_set_proof)?; - let block_number = block_header.number(); let block_hash = block_header.hash(); let state_root = block_header.state_root(); + + Self::check_validator_set_proof(state_root, validator_set_proof, &validator_set)?; + let bridge_info = BridgeInfo::new(block_number, &block_hash, state_root, validator_set); let new_bridge_id = NumBridges::get() + 1; - - // Hmm, can a call to `insert` fail? - // If this is an Option, why do we not need to wrap it in Some(T)? >::insert(new_bridge_id, bridge_info); - // Only increase the number of bridges if the insert call succeeds NumBridges::put(new_bridge_id); } @@ -139,8 +136,17 @@ impl Module { Ok(()) // Otherwise, Error::InvalidStorageProof } - fn check_validator_set_proof(_proof: Vec) -> std::result::Result<(), Error> { - // ... Do some math ... + fn check_validator_set_proof( + state_root: &T::Hash, + proof: Vec>, + _validator_set: &Vec, + ) -> std::result::Result<(), Error> { + let _checker = ::Hasher>>::new( + *state_root, + proof.clone() + ) + .unwrap(); + Ok(()) // Otherwise, Error::InvalidValidatorSetProof } } @@ -214,6 +220,31 @@ mod tests { }); } + #[test] + fn it_can_validate_validator_sets() { + use state_machine::{prove_read, backend::{Backend, InMemory}}; + + let validators = vec![0, 5, 10]; + + // construct storage proof + let backend = >::from(vec![ + (None, b"validator1".to_vec(), Some(b"alice".to_vec())), + (None, b"validator2".to_vec(), Some(b"bob".to_vec())) + ]); + let root = backend.storage_root(std::iter::empty()).0; + + // Generates a storage read proof + let proof = prove_read(backend, &[&b"validator1"[..], &b"validator2"[..]]).unwrap(); + + // check proof in runtime + let checker = >::new(root, proof.clone()).unwrap(); + assert_eq!(checker.read_value(b"validator1"), Ok(Some(b"alice".to_vec()))); + assert_eq!(checker.read_value(b"validator2"), Ok(Some(b"bob".to_vec()))); + + // with_externalities(&mut new_test_ext(), || { + // }); + } + #[test] fn it_creates_a_new_bridge() { let dummy_state_root = H256::default(); @@ -228,8 +259,6 @@ mod tests { new_test_ext().execute_with(|| { assert_eq!(MockBridge::num_bridges(), 0); - assert_eq!(MockBridge::tracked_bridges(0), None); - dbg!(&test_header); assert_ok!( MockBridge::initialize_bridge( From 8f1846a3636e27cafc9e8814bacef18a4a18fc97 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 23 Oct 2019 17:34:48 +0200 Subject: [PATCH 02/11] Update some tests --- srml/bridge/src/lib.rs | 43 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 5b7dfced16c5b..68a3b9d953e33 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -19,6 +19,7 @@ //! This will eventually have some useful documentation. //! For now though, enjoy this cow's wisdom. //! +//!```ignore //!________________________________________ //! / You are only young once, but you can \ //! \ stay immature indefinitely. / @@ -28,6 +29,7 @@ //! (__)\ )\/\ //! ||----w | //! || || +//!``` // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] @@ -37,7 +39,6 @@ mod storage_proof; use crate::storage_proof::StorageProofChecker; use codec::{Encode, Decode}; -use primitives::{Blake2Hasher}; use sr_primitives::traits::{Header, Member}; use support::{ decl_error, decl_module, decl_storage, @@ -131,16 +132,12 @@ decl_error! { } impl Module { - fn check_storage_proof(_proof: Vec) -> std::result::Result<(), Error> { - // ... Do some math ... - Ok(()) // Otherwise, Error::InvalidStorageProof - } - fn check_validator_set_proof( state_root: &T::Hash, proof: Vec>, _validator_set: &Vec, ) -> std::result::Result<(), Error> { + let _checker = ::Hasher>>::new( *state_root, proof.clone() @@ -155,7 +152,7 @@ impl Module { mod tests { use super::*; - use primitives::H256; + use primitives::{Blake2Hasher, H256}; use sr_primitives::{ Perbill, traits::{Header as HeaderT, IdentityLookup}, testing::Header, generic::Digest, }; @@ -169,7 +166,7 @@ mod tests { #[derive(Clone, PartialEq, Eq, Debug)] pub struct Test; - type System = system::Module; + type _System = system::Module; type MockBridge = Module; // TODO: Figure out what I actually need from here @@ -220,12 +217,9 @@ mod tests { }); } - #[test] - fn it_can_validate_validator_sets() { + fn create_dummy_validator_proof() -> (H256, Vec>) { use state_machine::{prove_read, backend::{Backend, InMemory}}; - let validators = vec![0, 5, 10]; - // construct storage proof let backend = >::from(vec![ (None, b"validator1".to_vec(), Some(b"alice".to_vec())), @@ -236,22 +230,26 @@ mod tests { // Generates a storage read proof let proof = prove_read(backend, &[&b"validator1"[..], &b"validator2"[..]]).unwrap(); - // check proof in runtime - let checker = >::new(root, proof.clone()).unwrap(); - assert_eq!(checker.read_value(b"validator1"), Ok(Some(b"alice".to_vec()))); - assert_eq!(checker.read_value(b"validator2"), Ok(Some(b"bob".to_vec()))); + (root, proof) + } + + #[test] + fn it_can_validate_validator_sets() { - // with_externalities(&mut new_test_ext(), || { - // }); + let validators = vec![0, 5, 10]; + let (root, proof) = create_dummy_validator_proof(); + + assert_ok!(MockBridge::check_validator_set_proof(&root, proof, &validators)); } #[test] fn it_creates_a_new_bridge() { - let dummy_state_root = H256::default(); + let (root, proof) = create_dummy_validator_proof(); + let test_header = Header { parent_hash: H256::default(), number: 42, - state_root: dummy_state_root, + state_root: root, extrinsics_root: H256::default(), digest: Digest::default(), }; @@ -265,8 +263,7 @@ mod tests { Origin::signed(1), test_header, vec![1, 2, 3], - vec![], - vec![], + proof, )); assert_eq!( @@ -274,7 +271,7 @@ mod tests { Some(BridgeInfo { last_finalized_block_number: 42, last_finalized_block_hash: test_hash, - last_finalized_state_root: dummy_state_root, + last_finalized_state_root: root, current_validator_set: vec![1, 2, 3], })); From c621fa544311b5faffcdbcbfecebf28b8f60b2de Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 25 Oct 2019 12:19:50 +0200 Subject: [PATCH 03/11] Check given validator set against set found in storage --- srml/bridge/src/lib.rs | 63 +++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 68a3b9d953e33..d1ee2edde4941 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -52,7 +52,7 @@ pub struct BridgeInfo { last_finalized_block_number: T::BlockNumber, last_finalized_block_hash: T::Hash, last_finalized_state_root: T::Hash, - current_validator_set: Vec, + current_validator_set: Vec<(T::ValidatorId, ValidatorWeight)>, } impl BridgeInfo { @@ -60,7 +60,7 @@ impl BridgeInfo { block_number: &T::BlockNumber, block_hash: &T::Hash, state_root: &T::Hash, - validator_set: Vec, + validator_set: Vec<(T::ValidatorId, ValidatorWeight)>, ) -> Self { // I don't like how this is done, should come back to... @@ -74,6 +74,7 @@ impl BridgeInfo { } type BridgeId = u64; +type ValidatorWeight = u64; pub trait Trait: system::Trait { /// A stable ID for a validator. @@ -97,7 +98,7 @@ decl_module! { fn initialize_bridge( origin, block_header: T::Header, - validator_set: Vec, + validator_set: Vec<(T::ValidatorId, ValidatorWeight)>, validator_set_proof: Vec>, ) { // NOTE: Will want to make this a governance issued call @@ -128,6 +129,7 @@ decl_error! { pub enum Error { InvalidStorageProof, InvalidValidatorSetProof, + ValidatorSetMismatch, } } @@ -135,16 +137,26 @@ impl Module { fn check_validator_set_proof( state_root: &T::Hash, proof: Vec>, - _validator_set: &Vec, + validator_set: &Vec<(T::ValidatorId, ValidatorWeight)>, ) -> std::result::Result<(), Error> { - let _checker = ::Hasher>>::new( + // pub const GRANDPA_AUTHORITIES_KEY: &'static [u8] = b":grandpa_authorities"; + // pub type AuthorityList = Vec<(AuthorityId, AuthorityWeight)>; + let checker = ::Hasher>>::new( *state_root, proof.clone() - ) - .unwrap(); + ).unwrap(); - Ok(()) // Otherwise, Error::InvalidValidatorSetProof + // By encoding the given set we should have an easy way to compare + // with the stuff we get out of storage via `read_value` + let encoded_validator_set = validator_set.encode(); + let actual_validator_set = checker.read_value(b":grandpa_authorities").unwrap().unwrap(); + + if encoded_validator_set == actual_validator_set { + Ok(()) + } else { + Err(Error::ValidatorSetMismatch) + } } } @@ -156,7 +168,7 @@ mod tests { use sr_primitives::{ Perbill, traits::{Header as HeaderT, IdentityLookup}, testing::Header, generic::Digest, }; - use support::{assert_ok, impl_outer_origin, parameter_types}; + use support::{assert_ok, assert_err, impl_outer_origin, parameter_types}; // NOTE: What's this for? impl_outer_origin! { @@ -217,34 +229,47 @@ mod tests { }); } - fn create_dummy_validator_proof() -> (H256, Vec>) { + fn create_dummy_validator_proof(validator_set: Vec<(DummyValidatorId, ValidatorWeight)>) -> (H256, Vec>) { use state_machine::{prove_read, backend::{Backend, InMemory}}; + let encoded_set = validator_set.encode(); + // construct storage proof let backend = >::from(vec![ - (None, b"validator1".to_vec(), Some(b"alice".to_vec())), - (None, b"validator2".to_vec(), Some(b"bob".to_vec())) + (None, b":grandpa_authorities".to_vec(), Some(encoded_set)), ]); let root = backend.storage_root(std::iter::empty()).0; // Generates a storage read proof - let proof = prove_read(backend, &[&b"validator1"[..], &b"validator2"[..]]).unwrap(); + let proof = prove_read(backend, &[&b":grandpa_authorities"[..]]).unwrap(); (root, proof) } #[test] fn it_can_validate_validator_sets() { - - let validators = vec![0, 5, 10]; - let (root, proof) = create_dummy_validator_proof(); + let validators = vec![(1, 1), (2, 1), (3, 1)]; + let (root, proof) = create_dummy_validator_proof(validators.clone()); assert_ok!(MockBridge::check_validator_set_proof(&root, proof, &validators)); } + #[test] + fn it_rejects_invalid_validator_sets() { + let validators = vec![(1, 1), (2, 1), (3, 1)]; + let (root, proof) = create_dummy_validator_proof(validators.clone()); + + let invalid_validators = vec![(3, 1), (2, 1), (1, 1)]; + assert_err!( + MockBridge::check_validator_set_proof(&root, proof, &invalid_validators), + Error::ValidatorSetMismatch + ); + } + #[test] fn it_creates_a_new_bridge() { - let (root, proof) = create_dummy_validator_proof(); + let validators = vec![(1, 1), (2, 1), (3, 1)]; + let (root, proof) = create_dummy_validator_proof(validators.clone()); let test_header = Header { parent_hash: H256::default(), @@ -262,7 +287,7 @@ mod tests { MockBridge::initialize_bridge( Origin::signed(1), test_header, - vec![1, 2, 3], + validators.clone(), proof, )); @@ -272,7 +297,7 @@ mod tests { last_finalized_block_number: 42, last_finalized_block_hash: test_hash, last_finalized_state_root: root, - current_validator_set: vec![1, 2, 3], + current_validator_set: validators.clone(), })); assert_eq!(MockBridge::num_bridges(), 1); From 8717be89ce599ccc32759626678de6ef4af780dd Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 29 Oct 2019 16:18:46 +0100 Subject: [PATCH 04/11] Create module for checking ancestry proofs --- srml/bridge/src/ancestry_proof.rs | 164 ++++++++++++++++++++++++++++++ srml/bridge/src/error.rs | 5 +- 2 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 srml/bridge/src/ancestry_proof.rs diff --git a/srml/bridge/src/ancestry_proof.rs b/srml/bridge/src/ancestry_proof.rs new file mode 100644 index 0000000000000..f342c000701a3 --- /dev/null +++ b/srml/bridge/src/ancestry_proof.rs @@ -0,0 +1,164 @@ +// Copyright 2017-2019 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + + +//! A way to check whether or not two headers are related to eachother. + +use crate::error::Error; + +// Question: Since this is from std, am I allowed to use it? +use std::collections::HashMap; +use sr_primitives::traits::Header; + +/// A struct used to check whether or not two headers +/// are related to one another. +pub struct AncestryProofChecker { + proof: HashMap, +} + +impl AncestryProofChecker + where H: Header { + /// Creates a new AncestryProofChecker, which is used + /// to verify whether two headers are related. + pub fn new(proof: HashMap) -> Self { + AncestryProofChecker { + proof + } + } + + /// A naive way to check whether a `child` header is an ancestor + /// of an `ancestor` header. It uses a proof which is a header + /// chain, which is submitted when creating the proof checker. This + /// could be updated to use something like Log2 Ancestors (#2053) + /// in the future. + pub fn verify_ancestry(&self, ancestor: H, child: H) -> Result<(), Error> { + let mut curr_header = &child; + let mut curr_hash = curr_header.hash(); + + loop { + // Check in our proof to see if we have a header (parent from previous round) + // We'll go around until we find a hash that isn't in the proof + curr_header = self.proof.get(&curr_hash).ok_or(Error::AncestorNotFound)?; + + // If we find that the header's hash matches our ancestor's hash we're done + if curr_header.hash() == ancestor.hash() { + return Ok(()) + } + + // Otherwise we'll need to try again + curr_hash = *curr_header.parent_hash(); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + use primitives::H256; + use sr_primitives::{ + traits::{Header as HeaderT}, testing::Header, generic::Digest, + }; + use support::{assert_ok, assert_err}; + + fn get_related_block_headers() -> (Header, Header, Header) { + let grandparent = Header { + parent_hash: H256::default(), + number: 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + let parent = Header { + parent_hash: grandparent.hash(), + number: grandparent.number() + 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + let child = Header { + parent_hash: parent.hash(), + number: parent.number() + 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + (grandparent, parent, child) + } + + #[test] + fn check_that_child_is_ancestor_of_grandparent() { + let (grandparent, parent, child) = get_related_block_headers(); + + let mut proof = HashMap::new(); + proof.insert(child.hash(), child.clone()); + proof.insert(parent.hash(), parent); + proof.insert(grandparent.hash(), grandparent.clone()); + + let checker = >::new(proof); + assert_ok!(checker.verify_ancestry(grandparent, child)); + } + + #[test] + fn check_that_child_ancestor_is_not_correct() { + let (grandparent, parent, child) = get_related_block_headers(); + + let mut proof = HashMap::new(); + proof.insert(child.hash(), child.clone()); + proof.insert(parent.hash(), parent); + proof.insert(grandparent.hash(), grandparent.clone()); + + let fake_grandparent = Header { + parent_hash: H256::from_slice(&[1u8; 32]), + number: 42, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + let checker = >::new(proof); + assert_err!( + checker.verify_ancestry(fake_grandparent, child), + Error::AncestorNotFound + ); + } + + #[test] + fn checker_fails_if_given_invalid_proof() { + let (grandparent, _parent, child) = get_related_block_headers(); + let fake_ancestor = Header { + parent_hash: H256::from_slice(&[1u8; 32]), + number: 42, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + let mut invalid_proof = HashMap::new(); + invalid_proof.insert(child.hash(), child.clone()); + invalid_proof.insert(fake_ancestor.hash(), fake_ancestor); + invalid_proof.insert(grandparent.hash(), grandparent.clone()); + + let checker = >::new(invalid_proof); + assert_err!( + checker.verify_ancestry(grandparent, child), + Error::AncestorNotFound + ); + } +} diff --git a/srml/bridge/src/error.rs b/srml/bridge/src/error.rs index 182b0883d1e62..3b1b3c2d506c2 100644 --- a/srml/bridge/src/error.rs +++ b/srml/bridge/src/error.rs @@ -19,7 +19,8 @@ #[derive(PartialEq)] #[cfg_attr(feature = "std", derive(Debug))] pub enum Error { - StorageRootMismatch, - StorageValueUnavailable, + StorageRootMismatch, + StorageValueUnavailable, + AncestorNotFound, } From 796029fa45129c46153b5f24cdaca1f0f71b44ae Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 30 Oct 2019 17:23:41 +0100 Subject: [PATCH 05/11] Use Vec of Headers instead of a HashMap --- srml/bridge/src/ancestry_proof.rs | 63 +++++++++++++++++-------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/srml/bridge/src/ancestry_proof.rs b/srml/bridge/src/ancestry_proof.rs index f342c000701a3..4d33bb732c40a 100644 --- a/srml/bridge/src/ancestry_proof.rs +++ b/srml/bridge/src/ancestry_proof.rs @@ -18,22 +18,19 @@ //! A way to check whether or not two headers are related to eachother. use crate::error::Error; - -// Question: Since this is from std, am I allowed to use it? -use std::collections::HashMap; use sr_primitives::traits::Header; /// A struct used to check whether or not two headers /// are related to one another. pub struct AncestryProofChecker { - proof: HashMap, + proof: Vec, } impl AncestryProofChecker where H: Header { /// Creates a new AncestryProofChecker, which is used /// to verify whether two headers are related. - pub fn new(proof: HashMap) -> Self { + pub fn new(proof: Vec) -> Self { AncestryProofChecker { proof } @@ -45,22 +42,29 @@ impl AncestryProofChecker /// could be updated to use something like Log2 Ancestors (#2053) /// in the future. pub fn verify_ancestry(&self, ancestor: H, child: H) -> Result<(), Error> { - let mut curr_header = &child; - let mut curr_hash = curr_header.hash(); + let mut curr_header = &self.proof[0]; + if curr_header.hash() != child.hash() { + return Err(Error::AncestorNotFound); + } - loop { - // Check in our proof to see if we have a header (parent from previous round) - // We'll go around until we find a hash that isn't in the proof - curr_header = self.proof.get(&curr_hash).ok_or(Error::AncestorNotFound)?; + let mut parent_hash = curr_header.parent_hash(); - // If we find that the header's hash matches our ancestor's hash we're done - if curr_header.hash() == ancestor.hash() { - return Ok(()) + // If we find that the header's parent hash matches our ancestor's hash we're done + for i in 1..self.proof.len() { + curr_header = &self.proof[i]; + + // Need to check that blocks are actually related + if curr_header.hash() != *parent_hash { + break; } - // Otherwise we'll need to try again - curr_hash = *curr_header.parent_hash(); + parent_hash = curr_header.parent_hash(); + if *parent_hash == ancestor.hash() { + return Ok(()) + } } + + Err(Error::AncestorNotFound) } } @@ -106,10 +110,10 @@ mod tests { fn check_that_child_is_ancestor_of_grandparent() { let (grandparent, parent, child) = get_related_block_headers(); - let mut proof = HashMap::new(); - proof.insert(child.hash(), child.clone()); - proof.insert(parent.hash(), parent); - proof.insert(grandparent.hash(), grandparent.clone()); + let mut proof = Vec::new(); + proof.push(child.clone()); + proof.push(parent); + proof.push(grandparent.clone()); let checker = >::new(proof); assert_ok!(checker.verify_ancestry(grandparent, child)); @@ -119,10 +123,10 @@ mod tests { fn check_that_child_ancestor_is_not_correct() { let (grandparent, parent, child) = get_related_block_headers(); - let mut proof = HashMap::new(); - proof.insert(child.hash(), child.clone()); - proof.insert(parent.hash(), parent); - proof.insert(grandparent.hash(), grandparent.clone()); + let mut proof = Vec::new(); + proof.push(child.clone()); + proof.push(parent); + proof.push(grandparent.clone()); let fake_grandparent = Header { parent_hash: H256::from_slice(&[1u8; 32]), @@ -141,7 +145,7 @@ mod tests { #[test] fn checker_fails_if_given_invalid_proof() { - let (grandparent, _parent, child) = get_related_block_headers(); + let (grandparent, parent, child) = get_related_block_headers(); let fake_ancestor = Header { parent_hash: H256::from_slice(&[1u8; 32]), number: 42, @@ -150,10 +154,11 @@ mod tests { digest: Digest::default(), }; - let mut invalid_proof = HashMap::new(); - invalid_proof.insert(child.hash(), child.clone()); - invalid_proof.insert(fake_ancestor.hash(), fake_ancestor); - invalid_proof.insert(grandparent.hash(), grandparent.clone()); + let mut invalid_proof = Vec::new(); + invalid_proof.push(child.clone()); + invalid_proof.push(fake_ancestor); + invalid_proof.push(parent); + invalid_proof.push(grandparent.clone()); let checker = >::new(invalid_proof); assert_err!( From 853f8eac6fb80b1438dee45baa768b4ad1511bdc Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 30 Oct 2019 17:40:50 +0100 Subject: [PATCH 06/11] Move the ancestry verification into the lib.rs file --- srml/bridge/src/ancestry_proof.rs | 169 ------------------------------ srml/bridge/src/lib.rs | 120 +++++++++++++++++++++ 2 files changed, 120 insertions(+), 169 deletions(-) delete mode 100644 srml/bridge/src/ancestry_proof.rs diff --git a/srml/bridge/src/ancestry_proof.rs b/srml/bridge/src/ancestry_proof.rs deleted file mode 100644 index 4d33bb732c40a..0000000000000 --- a/srml/bridge/src/ancestry_proof.rs +++ /dev/null @@ -1,169 +0,0 @@ -// Copyright 2017-2019 Parity Technologies (UK) Ltd. -// This file is part of Substrate. - -// Substrate is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Substrate is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Substrate. If not, see . - - -//! A way to check whether or not two headers are related to eachother. - -use crate::error::Error; -use sr_primitives::traits::Header; - -/// A struct used to check whether or not two headers -/// are related to one another. -pub struct AncestryProofChecker { - proof: Vec, -} - -impl AncestryProofChecker - where H: Header { - /// Creates a new AncestryProofChecker, which is used - /// to verify whether two headers are related. - pub fn new(proof: Vec) -> Self { - AncestryProofChecker { - proof - } - } - - /// A naive way to check whether a `child` header is an ancestor - /// of an `ancestor` header. It uses a proof which is a header - /// chain, which is submitted when creating the proof checker. This - /// could be updated to use something like Log2 Ancestors (#2053) - /// in the future. - pub fn verify_ancestry(&self, ancestor: H, child: H) -> Result<(), Error> { - let mut curr_header = &self.proof[0]; - if curr_header.hash() != child.hash() { - return Err(Error::AncestorNotFound); - } - - let mut parent_hash = curr_header.parent_hash(); - - // If we find that the header's parent hash matches our ancestor's hash we're done - for i in 1..self.proof.len() { - curr_header = &self.proof[i]; - - // Need to check that blocks are actually related - if curr_header.hash() != *parent_hash { - break; - } - - parent_hash = curr_header.parent_hash(); - if *parent_hash == ancestor.hash() { - return Ok(()) - } - } - - Err(Error::AncestorNotFound) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - use primitives::H256; - use sr_primitives::{ - traits::{Header as HeaderT}, testing::Header, generic::Digest, - }; - use support::{assert_ok, assert_err}; - - fn get_related_block_headers() -> (Header, Header, Header) { - let grandparent = Header { - parent_hash: H256::default(), - number: 1, - state_root: H256::default(), - extrinsics_root: H256::default(), - digest: Digest::default(), - }; - - let parent = Header { - parent_hash: grandparent.hash(), - number: grandparent.number() + 1, - state_root: H256::default(), - extrinsics_root: H256::default(), - digest: Digest::default(), - }; - - let child = Header { - parent_hash: parent.hash(), - number: parent.number() + 1, - state_root: H256::default(), - extrinsics_root: H256::default(), - digest: Digest::default(), - }; - - (grandparent, parent, child) - } - - #[test] - fn check_that_child_is_ancestor_of_grandparent() { - let (grandparent, parent, child) = get_related_block_headers(); - - let mut proof = Vec::new(); - proof.push(child.clone()); - proof.push(parent); - proof.push(grandparent.clone()); - - let checker = >::new(proof); - assert_ok!(checker.verify_ancestry(grandparent, child)); - } - - #[test] - fn check_that_child_ancestor_is_not_correct() { - let (grandparent, parent, child) = get_related_block_headers(); - - let mut proof = Vec::new(); - proof.push(child.clone()); - proof.push(parent); - proof.push(grandparent.clone()); - - let fake_grandparent = Header { - parent_hash: H256::from_slice(&[1u8; 32]), - number: 42, - state_root: H256::default(), - extrinsics_root: H256::default(), - digest: Digest::default(), - }; - - let checker = >::new(proof); - assert_err!( - checker.verify_ancestry(fake_grandparent, child), - Error::AncestorNotFound - ); - } - - #[test] - fn checker_fails_if_given_invalid_proof() { - let (grandparent, parent, child) = get_related_block_headers(); - let fake_ancestor = Header { - parent_hash: H256::from_slice(&[1u8; 32]), - number: 42, - state_root: H256::default(), - extrinsics_root: H256::default(), - digest: Digest::default(), - }; - - let mut invalid_proof = Vec::new(); - invalid_proof.push(child.clone()); - invalid_proof.push(fake_ancestor); - invalid_proof.push(parent); - invalid_proof.push(grandparent.clone()); - - let checker = >::new(invalid_proof); - assert_err!( - checker.verify_ancestry(grandparent, child), - Error::AncestorNotFound - ); - } -} diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index d1ee2edde4941..052084f9a018f 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -130,6 +130,7 @@ decl_error! { InvalidStorageProof, InvalidValidatorSetProof, ValidatorSetMismatch, + AncestorNotFound, } } @@ -160,6 +161,39 @@ impl Module { } } +// A naive way to check whether a `child` header is an ancestor +// of an `ancestor` header. For this it requires a proof which +// is a header chain, This could be updated to use something like +// Log2 Ancestors (#2053) in the future. +fn verify_ancestry(proof: Vec, ancestor: H, child: H) -> std::result::Result<(), Error> +where + H: Header +{ + let mut curr_header = &proof[0]; + if curr_header.hash() != child.hash() { + return Err(Error::AncestorNotFound); + } + + let mut parent_hash = curr_header.parent_hash(); + + // If we find that the header's parent hash matches our ancestor's hash we're done + for i in 1..proof.len() { + curr_header = &proof[i]; + + // Need to check that blocks are actually related + if curr_header.hash() != *parent_hash { + break; + } + + parent_hash = curr_header.parent_hash(); + if *parent_hash == ancestor.hash() { + return Ok(()) + } + } + + Err(Error::AncestorNotFound) +} + #[cfg(test)] mod tests { use super::*; @@ -303,4 +337,90 @@ mod tests { assert_eq!(MockBridge::num_bridges(), 1); }); } + + fn get_related_block_headers() -> (Header, Header, Header) { + let grandparent = Header { + parent_hash: H256::default(), + number: 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + let parent = Header { + parent_hash: grandparent.hash(), + number: grandparent.number() + 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + let child = Header { + parent_hash: parent.hash(), + number: parent.number() + 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + (grandparent, parent, child) + } + + #[test] + fn check_that_child_is_ancestor_of_grandparent() { + let (grandparent, parent, child) = get_related_block_headers(); + + let mut proof = Vec::new(); + proof.push(child.clone()); + proof.push(parent); + proof.push(grandparent.clone()); + + assert_ok!(verify_ancestry(proof, grandparent, child)); + } + + #[test] + fn check_that_child_ancestor_is_not_correct() { + let (grandparent, parent, child) = get_related_block_headers(); + + let mut proof = Vec::new(); + proof.push(child.clone()); + proof.push(parent); + proof.push(grandparent.clone()); + + let fake_grandparent = Header { + parent_hash: H256::from_slice(&[1u8; 32]), + number: 42, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + assert_err!( + verify_ancestry(proof, fake_grandparent, child), + Error::AncestorNotFound + ); + } + + #[test] + fn checker_fails_if_given_invalid_proof() { + let (grandparent, parent, child) = get_related_block_headers(); + let fake_ancestor = Header { + parent_hash: H256::from_slice(&[1u8; 32]), + number: 42, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + let mut invalid_proof = Vec::new(); + invalid_proof.push(child.clone()); + invalid_proof.push(fake_ancestor); + invalid_proof.push(parent); + invalid_proof.push(grandparent.clone()); + + assert_err!( + verify_ancestry(invalid_proof, grandparent, child), + Error::AncestorNotFound + ); + } } From 226804dffa2ddfced018f7d0d4cbe34ade11c13e Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 1 Nov 2019 14:21:25 +0100 Subject: [PATCH 07/11] Change the proof format to exclude `child` and `ancestor` headers --- srml/bridge/src/lib.rs | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 052084f9a018f..a7f927910c15c 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -139,7 +139,7 @@ impl Module { state_root: &T::Hash, proof: Vec>, validator_set: &Vec<(T::ValidatorId, ValidatorWeight)>, - ) -> std::result::Result<(), Error> { + ) -> Result<(), Error> { // pub const GRANDPA_AUTHORITIES_KEY: &'static [u8] = b":grandpa_authorities"; // pub type AuthorityList = Vec<(AuthorityId, AuthorityWeight)>; @@ -161,31 +161,25 @@ impl Module { } } -// A naive way to check whether a `child` header is an ancestor +// A naive way to check whether a `child` header is a decendent // of an `ancestor` header. For this it requires a proof which -// is a header chain, This could be updated to use something like +// is a chain of headers between (but not including) the `child` +// and `ancestor`. This could be updated to use something like // Log2 Ancestors (#2053) in the future. -fn verify_ancestry(proof: Vec, ancestor: H, child: H) -> std::result::Result<(), Error> +fn verify_ancestry(proof: Vec, ancestor: H, child: H) -> Result<(), Error> where H: Header { - let mut curr_header = &proof[0]; - if curr_header.hash() != child.hash() { - return Err(Error::AncestorNotFound); - } - - let mut parent_hash = curr_header.parent_hash(); + let mut parent_hash = child.parent_hash(); // If we find that the header's parent hash matches our ancestor's hash we're done - for i in 1..proof.len() { - curr_header = &proof[i]; - + for header in proof.iter() { // Need to check that blocks are actually related - if curr_header.hash() != *parent_hash { + if header.hash() != *parent_hash { break; } - parent_hash = curr_header.parent_hash(); + parent_hash = header.parent_hash(); if *parent_hash == ancestor.hash() { return Ok(()) } @@ -371,9 +365,7 @@ mod tests { let (grandparent, parent, child) = get_related_block_headers(); let mut proof = Vec::new(); - proof.push(child.clone()); proof.push(parent); - proof.push(grandparent.clone()); assert_ok!(verify_ancestry(proof, grandparent, child)); } @@ -383,9 +375,7 @@ mod tests { let (grandparent, parent, child) = get_related_block_headers(); let mut proof = Vec::new(); - proof.push(child.clone()); proof.push(parent); - proof.push(grandparent.clone()); let fake_grandparent = Header { parent_hash: H256::from_slice(&[1u8; 32]), @@ -413,10 +403,8 @@ mod tests { }; let mut invalid_proof = Vec::new(); - invalid_proof.push(child.clone()); invalid_proof.push(fake_ancestor); invalid_proof.push(parent); - invalid_proof.push(grandparent.clone()); assert_err!( verify_ancestry(invalid_proof, grandparent, child), From a7f2066c227507c0be7ce8607ea6680cfaf08a4c Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 1 Nov 2019 15:20:03 +0100 Subject: [PATCH 08/11] Add a testing function for builing header chains --- srml/bridge/src/lib.rs | 96 +++++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 39 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index a7f927910c15c..086022f955571 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -332,8 +332,32 @@ mod tests { }); } - fn get_related_block_headers() -> (Header, Header, Header) { - let grandparent = Header { + fn build_header_chain(root_header: Header, len: usize) -> Vec
{ + let mut header_chain = vec![root_header]; + for i in 1..len { + let parent = &header_chain[i - 1]; + + let h = Header { + parent_hash: parent.hash(), + number: parent.number() + 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + header_chain.push(h); + } + + // We want our proofs to go from newest to older headers + header_chain.reverse(); + // We don't actually need the oldest header in the proof + header_chain.pop(); + header_chain + } + + #[test] + fn check_that_child_is_ancestor_of_grandparent() { + let ancestor = Header { parent_hash: H256::default(), number: 1, state_root: H256::default(), @@ -341,43 +365,28 @@ mod tests { digest: Digest::default(), }; - let parent = Header { - parent_hash: grandparent.hash(), - number: grandparent.number() + 1, - state_root: H256::default(), - extrinsics_root: H256::default(), - digest: Digest::default(), - }; + // A valid proof doesn't include the child header, so remove it + let mut proof = build_header_chain(ancestor.clone(), 10); + let child = proof.remove(0); + + assert_ok!(verify_ancestry(proof, ancestor, child)); + } - let child = Header { - parent_hash: parent.hash(), - number: parent.number() + 1, + #[test] + fn fake_ancestor_is_not_found_in_child_ancestry() { + let ancestor = Header { + parent_hash: H256::default(), + number: 1, state_root: H256::default(), extrinsics_root: H256::default(), digest: Digest::default(), }; - (grandparent, parent, child) - } - - #[test] - fn check_that_child_is_ancestor_of_grandparent() { - let (grandparent, parent, child) = get_related_block_headers(); - - let mut proof = Vec::new(); - proof.push(parent); - - assert_ok!(verify_ancestry(proof, grandparent, child)); - } + // A valid proof doesn't include the child header, so remove it + let mut proof = build_header_chain(ancestor, 10); + let child = proof.remove(0); - #[test] - fn check_that_child_ancestor_is_not_correct() { - let (grandparent, parent, child) = get_related_block_headers(); - - let mut proof = Vec::new(); - proof.push(parent); - - let fake_grandparent = Header { + let fake_ancestor = Header { parent_hash: H256::from_slice(&[1u8; 32]), number: 42, state_root: H256::default(), @@ -386,14 +395,25 @@ mod tests { }; assert_err!( - verify_ancestry(proof, fake_grandparent, child), + verify_ancestry(proof, fake_ancestor, child), Error::AncestorNotFound ); } #[test] - fn checker_fails_if_given_invalid_proof() { - let (grandparent, parent, child) = get_related_block_headers(); + fn checker_fails_if_given_an_unrelated_header() { + let ancestor = Header { + parent_hash: H256::default(), + number: 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + // A valid proof doesn't include the child header, so remove it + let mut invalid_proof = build_header_chain(ancestor.clone(), 10); + let child = invalid_proof.remove(0); + let fake_ancestor = Header { parent_hash: H256::from_slice(&[1u8; 32]), number: 42, @@ -402,12 +422,10 @@ mod tests { digest: Digest::default(), }; - let mut invalid_proof = Vec::new(); - invalid_proof.push(fake_ancestor); - invalid_proof.push(parent); + invalid_proof.insert(5, fake_ancestor); assert_err!( - verify_ancestry(invalid_proof, grandparent, child), + verify_ancestry(invalid_proof, ancestor, child), Error::AncestorNotFound ); } From 11ad8c65bcff9dc40131e5a6321338480bb89706 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 1 Nov 2019 15:35:09 +0100 Subject: [PATCH 09/11] Rename AncestorNotFound error to InvalidAncestryProof --- srml/bridge/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 086022f955571..385a32316b710 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -130,7 +130,7 @@ decl_error! { InvalidStorageProof, InvalidValidatorSetProof, ValidatorSetMismatch, - AncestorNotFound, + InvalidAncestryProof, } } @@ -185,7 +185,7 @@ where } } - Err(Error::AncestorNotFound) + Err(Error::InvalidAncestryProof) } #[cfg(test)] @@ -396,7 +396,7 @@ mod tests { assert_err!( verify_ancestry(proof, fake_ancestor, child), - Error::AncestorNotFound + Error::InvalidAncestryProof ); } @@ -426,7 +426,7 @@ mod tests { assert_err!( verify_ancestry(invalid_proof, ancestor, child), - Error::AncestorNotFound + Error::InvalidAncestryProof ); } } From f861879347dac6b3c7ae5e039fface142f07cb42 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 1 Nov 2019 15:39:35 +0100 Subject: [PATCH 10/11] Use ancestor hash instead of header when verifying ancestry --- srml/bridge/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 385a32316b710..4bced161feef6 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -166,7 +166,7 @@ impl Module { // is a chain of headers between (but not including) the `child` // and `ancestor`. This could be updated to use something like // Log2 Ancestors (#2053) in the future. -fn verify_ancestry(proof: Vec, ancestor: H, child: H) -> Result<(), Error> +fn verify_ancestry(proof: Vec, ancestor_hash: H::Hash, child: H) -> Result<(), Error> where H: Header { @@ -180,7 +180,7 @@ where } parent_hash = header.parent_hash(); - if *parent_hash == ancestor.hash() { + if *parent_hash == ancestor_hash { return Ok(()) } } @@ -369,7 +369,7 @@ mod tests { let mut proof = build_header_chain(ancestor.clone(), 10); let child = proof.remove(0); - assert_ok!(verify_ancestry(proof, ancestor, child)); + assert_ok!(verify_ancestry(proof, ancestor.hash(), child)); } #[test] @@ -395,7 +395,7 @@ mod tests { }; assert_err!( - verify_ancestry(proof, fake_ancestor, child), + verify_ancestry(proof, fake_ancestor.hash(), child), Error::InvalidAncestryProof ); } @@ -425,7 +425,7 @@ mod tests { invalid_proof.insert(5, fake_ancestor); assert_err!( - verify_ancestry(invalid_proof, ancestor, child), + verify_ancestry(invalid_proof, ancestor.hash(), child), Error::InvalidAncestryProof ); } From 024c372954e5f93c51b45f635aa727668221fd71 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 1 Nov 2019 16:18:59 +0100 Subject: [PATCH 11/11] Clean up some stuff missed in the merge --- srml/bridge/src/error.rs | 26 -------------------------- srml/bridge/src/lib.rs | 2 +- 2 files changed, 1 insertion(+), 27 deletions(-) delete mode 100644 srml/bridge/src/error.rs diff --git a/srml/bridge/src/error.rs b/srml/bridge/src/error.rs deleted file mode 100644 index 3b1b3c2d506c2..0000000000000 --- a/srml/bridge/src/error.rs +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright 2017-2019 Parity Technologies (UK) Ltd. -// This file is part of Substrate. - -// Substrate is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Substrate is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Substrate. If not, see . - -//! Common error types for the srml-bridge crate. - -#[derive(PartialEq)] -#[cfg_attr(feature = "std", derive(Debug))] -pub enum Error { - StorageRootMismatch, - StorageValueUnavailable, - AncestorNotFound, -} - diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index b0f34a0b97321..78bf721e8fb9f 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -136,7 +136,7 @@ impl Module { state_root: &T::Hash, proof: Vec>, validator_set: &Vec<(AuthorityId, AuthorityWeight)>, - ) -> std::result::Result<(), Error> { + ) -> Result<(), Error> { let checker = ::Hasher>>::new( *state_root,