From 598d8c9cf8b93f1b058c720dc3d156d51cd677fd Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 25 Sep 2019 15:16:57 +0200 Subject: [PATCH 01/34] Create `bridge` module skeleton --- srml/bridge/Cargo.toml | 10 +++++++++ srml/bridge/src/lib.rs | 46 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 srml/bridge/Cargo.toml create mode 100644 srml/bridge/src/lib.rs diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml new file mode 100644 index 0000000000000..27fd8dd140ebb --- /dev/null +++ b/srml/bridge/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "bridge" +version = "0.1.0" +authors = ["Parity Technologies "] +edition = "2018" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +support = { package = "srml-support", path = "../support", default-features = false } diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs new file mode 100644 index 0000000000000..0437b37ec084c --- /dev/null +++ b/srml/bridge/src/lib.rs @@ -0,0 +1,46 @@ +// 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 . + + +//! # Bridge Module +//! +//! This will eventually have some useful documentation. +//! For now though, enjoy this cow's wisdom. +//! +//! ________________________________________ +//! / You are only young once, but you can \ +//! \ stay immature indefinitely. / +//! ---------------------------------------- +//! \ ^__^ +//! \ (oo)\_______ +//! (__)\ )\/\ +//! ||----w | +//! || || + + +// Ensure we're `no_std` when compiling for Wasm. +#![cfg_attr(not(feature = "std"), no_std)] + +use support::{ + decl_module, decl_event, decl_storage, decl_error, +}; + +pub trait Trait {} + +decl_storage! {} +decl_event!() +decl_module! {} +decl_error! {} From e3d36440b0391fa5d1a4012b6250641a9cb0f59d Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Thu, 26 Sep 2019 16:41:01 +0200 Subject: [PATCH 02/34] Add more skeleton code --- Cargo.lock | 11 ++++ Cargo.toml | 1 + srml/bridge/Cargo.toml | 14 ++++ srml/bridge/src/lib.rs | 144 ++++++++++++++++++++++++++++++++++++++--- 4 files changed, 160 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2c3def1e28abc..7c73fee91fe09 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -292,6 +292,17 @@ dependencies = [ "byte-tools 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "bridge" +version = "0.1.0" +dependencies = [ + "parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", + "sr-primitives 2.0.0", + "srml-session 2.0.0", + "srml-support 2.0.0", + "srml-system 2.0.0", +] + [[package]] name = "bs58" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index 42f78db493b50..846067ec1ec38 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,6 +71,7 @@ members = [ "srml/assets", "srml/aura", "srml/balances", + "srml/bridge", "srml/contracts", "srml/contracts/rpc", "srml/collective", diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 27fd8dd140ebb..717f67c3d0d27 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -7,4 +7,18 @@ edition = "2018" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] +codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } +session = { package = "srml-session", path = "../session", default-features = false, features = ["historical"] } +sr-primitives = { path = "../../core/sr-primitives", default-features = false } support = { package = "srml-support", path = "../support", default-features = false } +system = { package = "srml-system", path = "../system", default-features = false } + +[features] +default = ["std"] +std = [ + "codec/std", + "session/std", + "sr-primitives/std", + "support/std", + "system/std", +] diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 0437b37ec084c..c7c5f576406ed 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -14,7 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . - //! # Bridge Module //! //! This will eventually have some useful documentation. @@ -22,7 +21,7 @@ //! //! ________________________________________ //! / You are only young once, but you can \ -//! \ stay immature indefinitely. / +//! \ stay immature indefinitely. / //! ---------------------------------------- //! \ ^__^ //! \ (oo)\_______ @@ -30,17 +29,142 @@ //! ||----w | //! || || - // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] -use support::{ - decl_module, decl_event, decl_storage, decl_error, +use codec::{Decode, Encode}; +use sr_primitives::{ + generic::BlockId, + traits::{Block as BlockT, Header as HeaderT}, }; +use support::{decl_error, decl_event, decl_module, decl_storage, dispatch::Result}; +use system::{ensure_root, ensure_signed}; + +// struct Bridge {} +// +// impl Bridge { +// pub fn new() -> Self {} +// pub fn submit_claim() -> Result<(), Err> {} +// pub fn get_id(&self) -> u64 {} +// pub fn get_recently_finalized_block(&self) -> Block {} +// } + +pub trait Trait: system::Trait + session::Trait { + // The identifier type for an authority. + // type AuthorityId: Member + Parameter + RuntimeAppPublic + Default; +} + +decl_storage! { + trait Store for Module as Bridge { + /// Get the ID of the current bridge + pub BridgeId get(bridge_id): T::Hash; + + /// Get the ID of the current bridge + // TODO: Figure out how to ge a Block here + pub LastFinalizedBlock get(last_finalized_block): u64; // Block; + + /// Get latest block number included in the chain + pub BlockNumber get(lastest_block_num): T::BlockNumber; + + // What I want to have wrt to the Block are: + // 1. Block Number + // 2. Block Hash + // 3. State Root + // Can I get these from a `Block`, or do they need to + // be stored individually? + + /// Latest set of validators + pub Validators get(validators): Vec; + } +} + +decl_module! { + pub struct Module for enum Call where origin: T::Origin { + // TODO: Figure out the proper type for these proofs + fn new(origin, block_header: T::Header, validator_set: Vec, validator_set_proof: Vec, storage_proof: Vec) { + // + // The macro can't handle arguments like this :( + // fn new( + // origin, + // block_header: T::Header, + // validator_set: Vec, + // validator_set_proof: Vec, + // storage_proof: Vec, + // ) { + + // NOTE: Should this be a root call? + let _sender = ensure_signed(origin)?; + + Self::check_storage_proof()?; + Self::check_validator_set_proof()?; + + // TODO: Do something better than this + let bridge_id = >::random(b"this_is_not_a_good_source_of_randomness"); + >::put(bridge_id); + } + + fn submit_finalized_headers(origin) { + let _sender = ensure_signed(origin)?; + } + } + +} + +decl_error! { + // Error for the Bridge module + pub enum Error { + InvalidStorageProof, + InvalidValidatorSetProof, + } +} + +impl Module { + // fn check_storage_proof() -> Result<(), Error> { + fn check_storage_proof() -> Result { + // ... Do some math ... + Ok(()) // Otherwise, Error::InvalidStorageProof + } + + // fn check_validator_set_proof() -> Result<(), Error> { + fn check_validator_set_proof() -> Result { + // ... Do some math ... + Ok(()) // Otherwise, Error::InvalidValidatorSetProof + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn should_initialize_default_bridge_module() { + let bridge = Bridge::default(); + } + + #[test] + fn should_initialize_bridge_module() { + let bridge = Bridge::new(header, validator_set, v_set_proof, code_hash); + } + + #[test] + fn should_accept_finalized_headers() { + let bridge = Bridge::default(); + bridge.submit_finalized_header(last_block_hash, header, ancestry_proof, grandpa_proof); + } + + #[test] + fn should_get_recently_finalized_block() {} + + #[test] + fn should_do_all_the_things() { + let bridge = Bridge::new(); // or Bridge::default(); + bridge.track_chain(); // Maybe part of init process? -pub trait Trait {} + while curr_chain.has_blocks() { + // Using Fred's spec this would be something like `submit_claim()` + bridge.submit_finalized_headers(); + } -decl_storage! {} -decl_event!() -decl_module! {} -decl_error! {} + bridge.close(); + } +} From ce3385c3016b768a1a168d27df4ed874ba8d68e1 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 27 Sep 2019 14:52:53 +0200 Subject: [PATCH 03/34] Clean up some warnings --- srml/bridge/src/lib.rs | 52 +++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index c7c5f576406ed..62b684e694655 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -32,13 +32,8 @@ // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] -use codec::{Decode, Encode}; -use sr_primitives::{ - generic::BlockId, - traits::{Block as BlockT, Header as HeaderT}, -}; -use support::{decl_error, decl_event, decl_module, decl_storage, dispatch::Result}; -use system::{ensure_root, ensure_signed}; +use support::{decl_error, decl_event, decl_module, decl_storage}; +use system::{ensure_signed}; // struct Bridge {} // @@ -59,19 +54,14 @@ decl_storage! { /// Get the ID of the current bridge pub BridgeId get(bridge_id): T::Hash; - /// Get the ID of the current bridge - // TODO: Figure out how to ge a Block here - pub LastFinalizedBlock get(last_finalized_block): u64; // Block; - /// Get latest block number included in the chain - pub BlockNumber get(lastest_block_num): T::BlockNumber; + pub LastBlockNumber get(lastest_block_num): T::BlockNumber; + + /// Get the latest block header included in the chain + pub LastBlockHeader get(lastest_block_header): Option; - // What I want to have wrt to the Block are: - // 1. Block Number - // 2. Block Hash - // 3. State Root - // Can I get these from a `Block`, or do they need to - // be stored individually? + /// Get the latest state root included in the chain + pub LastStateRoot get(lastest_state_root): T::Hash; /// Latest set of validators pub Validators get(validators): Vec; @@ -81,18 +71,15 @@ decl_storage! { decl_module! { pub struct Module for enum Call where origin: T::Origin { // TODO: Figure out the proper type for these proofs - fn new(origin, block_header: T::Header, validator_set: Vec, validator_set_proof: Vec, storage_proof: Vec) { - // - // The macro can't handle arguments like this :( - // fn new( - // origin, - // block_header: T::Header, - // validator_set: Vec, - // validator_set_proof: Vec, - // storage_proof: Vec, - // ) { - + fn new( + origin, + _block_header: T::Header, + _validator_set: Vec, + _validator_set_proof: Vec, + _storage_proof: Vec, + ) { // NOTE: Should this be a root call? + // Use srml/collective/src/lib.rs? let _sender = ensure_signed(origin)?; Self::check_storage_proof()?; @@ -107,7 +94,6 @@ decl_module! { let _sender = ensure_signed(origin)?; } } - } decl_error! { @@ -119,14 +105,12 @@ decl_error! { } impl Module { - // fn check_storage_proof() -> Result<(), Error> { - fn check_storage_proof() -> Result { + fn check_storage_proof() -> std::result::Result<(), Error> { // ... Do some math ... Ok(()) // Otherwise, Error::InvalidStorageProof } - // fn check_validator_set_proof() -> Result<(), Error> { - fn check_validator_set_proof() -> Result { + fn check_validator_set_proof() -> std::result::Result<(), Error> { // ... Do some math ... Ok(()) // Otherwise, Error::InvalidValidatorSetProof } From b4daa86501a8e6646ead789695f595284b2319cf Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 1 Oct 2019 11:10:19 +0200 Subject: [PATCH 04/34] Get the mock runtime for tests set up --- Cargo.lock | 3 + srml/bridge/Cargo.toml | 7 ++ srml/bridge/src/lib.rs | 148 ++++++++++++++++++++++++++++------------- 3 files changed, 112 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7c73fee91fe09..9e57622397aa3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -297,10 +297,13 @@ name = "bridge" version = "0.1.0" dependencies = [ "parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.97 (registry+https://github.com/rust-lang/crates.io-index)", + "sr-io 2.0.0", "sr-primitives 2.0.0", "srml-session 2.0.0", "srml-support 2.0.0", "srml-system 2.0.0", + "substrate-primitives 2.0.0", ] [[package]] diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 717f67c3d0d27..bcd8f5863523b 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -9,16 +9,23 @@ edition = "2018" [dependencies] codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } 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 } support = { package = "srml-support", path = "../support", default-features = false } system = { package = "srml-system", path = "../system", default-features = false } +[dev-dependencies] +primitives = { package = "substrate-primitives", path = "../../core/primitives" } +runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false } + [features] default = ["std"] std = [ + "serde", "codec/std", "session/std", "sr-primitives/std", "support/std", "system/std", + "runtime-io/std", ] diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 62b684e694655..2027577fc0936 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -19,7 +19,7 @@ //! This will eventually have some useful documentation. //! For now though, enjoy this cow's wisdom. //! -//! ________________________________________ +//!________________________________________ //! / You are only young once, but you can \ //! \ stay immature indefinitely. / //! ---------------------------------------- @@ -32,39 +32,37 @@ // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] -use support::{decl_error, decl_event, decl_module, decl_storage}; +use sr_primitives::traits::Member; +use support::{ + decl_error, decl_module, decl_storage, + Parameter, +}; use system::{ensure_signed}; -// struct Bridge {} -// -// impl Bridge { -// pub fn new() -> Self {} -// pub fn submit_claim() -> Result<(), Err> {} -// pub fn get_id(&self) -> u64 {} -// pub fn get_recently_finalized_block(&self) -> Block {} -// } - -pub trait Trait: system::Trait + session::Trait { +pub trait Trait: system::Trait { // The identifier type for an authority. // type AuthorityId: Member + Parameter + RuntimeAppPublic + Default; + /// A stable ID for a validator. + type ValidatorId: Member + Parameter; } decl_storage! { trait Store for Module as Bridge { - /// Get the ID of the current bridge - pub BridgeId get(bridge_id): T::Hash; + // Get the ID of the current bridge + pub BridgeId get(bridge_id) config(): u64; - /// Get latest block number included in the chain - pub LastBlockNumber get(lastest_block_num): T::BlockNumber; + // Get latest block number included in the chain + pub LastBlockNumber get(latest_block_num) config(): T::BlockNumber; + // pub BlockNum get(block_num) config(): u64; - /// Get the latest block header included in the chain - pub LastBlockHeader get(lastest_block_header): Option; + // Get the latest block header included in the chain + pub LastBlockHeader get(latest_block_header): Option; - /// Get the latest state root included in the chain - pub LastStateRoot get(lastest_state_root): T::Hash; + // Get the latest state root included in the chain + // pub LastStateRoot get(latest_state_root) config(): T::Hash; - /// Latest set of validators - pub Validators get(validators): Vec; + // Latest set of validators + pub Validators get(validators) config(): Vec; } } @@ -85,9 +83,8 @@ decl_module! { Self::check_storage_proof()?; Self::check_validator_set_proof()?; - // TODO: Do something better than this - let bridge_id = >::random(b"this_is_not_a_good_source_of_randomness"); - >::put(bridge_id); + let new_bridge_id = BridgeId::get() + 1; + BridgeId::put(new_bridge_id); } fn submit_finalized_headers(origin) { @@ -120,35 +117,94 @@ impl Module { mod tests { use super::*; - #[test] - fn should_initialize_default_bridge_module() { - let bridge = Bridge::default(); + use primitives::{H256, Blake2Hasher}; + use sr_primitives::{ + Perbill, traits::IdentityLookup, testing::Header, generic::Digest + }; + use support::{assert_ok, impl_outer_origin, parameter_types}; + use runtime_io::with_externalities; + + // NOTE: What's this for? + impl_outer_origin! { + pub enum Origin for Test {} } - #[test] - fn should_initialize_bridge_module() { - let bridge = Bridge::new(header, validator_set, v_set_proof, code_hash); + #[derive(Clone, PartialEq, Eq, Debug)] + pub struct Test; + + type System = system::Module; + type Bridge = Module; + + // TODO: Figure out what I actually need from here + parameter_types! { + pub const BlockHashCount: u64 = 250; + pub const MaximumBlockWeight: u32 = 1024; + pub const MaximumBlockLength: u32 = 2 * 1024; + pub const MinimumPeriod: u64 = 5; + pub const AvailableBlockRatio: Perbill = Perbill::one(); } - #[test] - fn should_accept_finalized_headers() { - let bridge = Bridge::default(); - bridge.submit_finalized_header(last_block_hash, header, ancestry_proof, grandpa_proof); + type DummyValidatorId = u64; + + impl system::Trait for Test { + type Origin = Origin; + type Index = u64; + type BlockNumber = u64; + type Call = (); + type Hash = H256; + type Hashing = sr_primitives::traits::BlakeTwo256; + type AccountId = DummyValidatorId; + type Lookup = IdentityLookup; + type Header = Header; + type WeightMultiplierUpdate = (); + type Event = (); + type BlockHashCount = (); + type MaximumBlockWeight = (); + type AvailableBlockRatio = (); + type MaximumBlockLength = (); + type Version = (); } - #[test] - fn should_get_recently_finalized_block() {} + impl Trait for Test { + type ValidatorId = DummyValidatorId; + } - #[test] - fn should_do_all_the_things() { - let bridge = Bridge::new(); // or Bridge::default(); - bridge.track_chain(); // Maybe part of init process? + fn new_test_ext() -> runtime_io::TestExternalities { + let mut t = system::GenesisConfig::default().build_storage::().unwrap(); + GenesisConfig:: { + bridge_id: 0, + latest_block_num: 0, + //latest_block_header: None, + + // How do I get a default Hash? + // latest_state_root: Hash::default(), + validators: vec![], + }.assimilate_storage(&mut t).unwrap(); + t.into() + } - while curr_chain.has_blocks() { - // Using Fred's spec this would be something like `submit_claim()` - bridge.submit_finalized_headers(); - } + #[test] + fn it_works_for_default_value() { + with_externalities(&mut new_test_ext(), || { + assert_eq!(Bridge::bridge_id(), 0); + assert_eq!(Bridge::latest_block_num(), 0); + }); + } - bridge.close(); + #[test] + fn it_creates_a_new_bridge() { + let test_header = Header { + parent_hash: H256::default(), + number: 0, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + with_externalities(&mut new_test_ext(), || { + assert_eq!(Bridge::bridge_id(), 0); + assert_ok!(Bridge::new(Origin::signed(1), test_header, vec![], vec![], vec![])); + assert_eq!(Bridge::bridge_id(), 1); + }); } } From 9f2710638e5dfbae7098dadd24cd547d8c9e440c Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 1 Oct 2019 14:55:04 +0200 Subject: [PATCH 05/34] Add BridgeId => Bridge mapping --- Cargo.lock | 28 +++++++++---------- srml/bridge/Cargo.toml | 2 +- srml/bridge/src/lib.rs | 63 +++++++++++++++++++++++++++++------------- 3 files changed, 59 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9e57622397aa3..24c8d22751758 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -292,20 +292,6 @@ dependencies = [ "byte-tools 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", ] -[[package]] -name = "bridge" -version = "0.1.0" -dependencies = [ - "parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.97 (registry+https://github.com/rust-lang/crates.io-index)", - "sr-io 2.0.0", - "sr-primitives 2.0.0", - "srml-session 2.0.0", - "srml-support 2.0.0", - "srml-system 2.0.0", - "substrate-primitives 2.0.0", -] - [[package]] name = "bs58" version = "0.3.0" @@ -4490,6 +4476,20 @@ dependencies = [ "substrate-primitives 2.0.0", ] +[[package]] +name = "srml-bridge" +version = "0.1.0" +dependencies = [ + "parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.97 (registry+https://github.com/rust-lang/crates.io-index)", + "sr-io 2.0.0", + "sr-primitives 2.0.0", + "srml-session 2.0.0", + "srml-support 2.0.0", + "srml-system 2.0.0", + "substrate-primitives 2.0.0", +] + [[package]] name = "srml-collective" version = "2.0.0" diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index bcd8f5863523b..804e35e78e2c8 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "bridge" +name = "srml-bridge" version = "0.1.0" authors = ["Parity Technologies "] edition = "2018" diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 2027577fc0936..4ac8541e1a95b 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -32,36 +32,53 @@ // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] +use codec::{Encode, Decode}; use sr_primitives::traits::Member; +#[cfg(feature = "std")] +use sr_primitives::{Serialize, Deserialize}; use support::{ decl_error, decl_module, decl_storage, Parameter, }; use system::{ensure_signed}; + +#[derive(Encode, Decode, Default, Clone, PartialEq)] +#[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))] +pub struct Bridge; + +impl Bridge { + pub fn new() -> Self { + Bridge + } +} + +type BridgeId = u64; + pub trait Trait: system::Trait { - // The identifier type for an authority. - // type AuthorityId: Member + Parameter + RuntimeAppPublic + Default; /// A stable ID for a validator. type ValidatorId: Member + Parameter; } decl_storage! { trait Store for Module as Bridge { - // Get the ID of the current bridge - pub BridgeId get(bridge_id) config(): u64; + /// The number of current bridges managed by the module. + pub NumBridges get(num_bridges) config(): BridgeId; + + /// Maps a bridge id to a bridge struct. Allows a single + /// `bridge` module to manage multiple bridges. + pub BridgeFoo get(bridge_foo) config(): map BridgeId => Bridge; - // Get latest block number included in the chain + /// Get latest block number included in the chain pub LastBlockNumber get(latest_block_num) config(): T::BlockNumber; - // pub BlockNum get(block_num) config(): u64; - // Get the latest block header included in the chain + /// Get the latest block header included in the chain pub LastBlockHeader get(latest_block_header): Option; // Get the latest state root included in the chain // pub LastStateRoot get(latest_state_root) config(): T::Hash; - // Latest set of validators + /// Latest set of validators pub Validators get(validators) config(): Vec; } } @@ -69,7 +86,7 @@ decl_storage! { decl_module! { pub struct Module for enum Call where origin: T::Origin { // TODO: Figure out the proper type for these proofs - fn new( + fn initialize_bridge( origin, _block_header: T::Header, _validator_set: Vec, @@ -83,8 +100,13 @@ decl_module! { Self::check_storage_proof()?; Self::check_validator_set_proof()?; - let new_bridge_id = BridgeId::get() + 1; - BridgeId::put(new_bridge_id); + let new_bridge_id = NumBridges::get() + 1; + + // Hmm, can this call fail? + BridgeFoo::insert(new_bridge_id, Bridge::new()); + + // Only increase the number of bridges if the insert call succeeds + NumBridges::put(new_bridge_id); } fn submit_finalized_headers(origin) { @@ -133,7 +155,8 @@ mod tests { pub struct Test; type System = system::Module; - type Bridge = Module; + // type Bridge = Module; // With the Bridge struct this isn't great + type MockBridge = Module; // TODO: Figure out what I actually need from here parameter_types! { @@ -172,9 +195,9 @@ mod tests { fn new_test_ext() -> runtime_io::TestExternalities { let mut t = system::GenesisConfig::default().build_storage::().unwrap(); GenesisConfig:: { - bridge_id: 0, + num_bridges: 0, latest_block_num: 0, - //latest_block_header: None, + bridge_foo: vec![(0, Bridge::new()), (1, Bridge::new())], // How do I get a default Hash? // latest_state_root: Hash::default(), @@ -186,8 +209,8 @@ mod tests { #[test] fn it_works_for_default_value() { with_externalities(&mut new_test_ext(), || { - assert_eq!(Bridge::bridge_id(), 0); - assert_eq!(Bridge::latest_block_num(), 0); + assert_eq!(MockBridge::num_bridges(), 0); + assert_eq!(MockBridge::latest_block_num(), 0); }); } @@ -202,9 +225,11 @@ mod tests { }; with_externalities(&mut new_test_ext(), || { - assert_eq!(Bridge::bridge_id(), 0); - assert_ok!(Bridge::new(Origin::signed(1), test_header, vec![], vec![], vec![])); - assert_eq!(Bridge::bridge_id(), 1); + assert_eq!(MockBridge::num_bridges(), 0); + assert_eq!(MockBridge::bridge_foo(0), Bridge::new()); + assert_ok!(MockBridge::initialize_bridge(Origin::signed(1), test_header, vec![], vec![], vec![])); + assert_eq!(MockBridge::bridge_foo(1), Bridge::new()); + assert_eq!(MockBridge::num_bridges(), 1); }); } } From e623cb8c52f6bf6278c38a3df273e682949d7309 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 4 Oct 2019 15:21:00 +0300 Subject: [PATCH 06/34] Allow tracking of multiple bridges --- Cargo.lock | 2 +- srml/bridge/src/lib.rs | 116 ++++++++++++++++++++++++----------------- 2 files changed, 68 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 24c8d22751758..28dc2485d3d24 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4481,7 +4481,7 @@ name = "srml-bridge" version = "0.1.0" dependencies = [ "parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.97 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.101 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", "sr-primitives 2.0.0", "srml-session 2.0.0", diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 4ac8541e1a95b..f24653a4f7987 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -33,23 +33,37 @@ #![cfg_attr(not(feature = "std"), no_std)] use codec::{Encode, Decode}; -use sr_primitives::traits::Member; -#[cfg(feature = "std")] -use sr_primitives::{Serialize, Deserialize}; +use sr_primitives::traits::{Header, Member}; use support::{ decl_error, decl_module, decl_storage, Parameter, }; use system::{ensure_signed}; +#[derive(Encode, Decode, Clone, PartialEq)] +#[cfg_attr(feature = "std", derive(Debug))] +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, +} -#[derive(Encode, Decode, Default, Clone, PartialEq)] -#[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))] -pub struct Bridge; - -impl Bridge { - pub fn new() -> Self { - Bridge +impl BridgeInfo { + pub fn new( + block_number: &T::BlockNumber, + block_hash: &T::Hash, + state_root: &T::Hash, + validator_set: Vec, + ) -> Self + { + // I don't like how this is done, should come back to... + BridgeInfo { + last_finalized_block_number: *block_number, + last_finalized_block_hash: *block_hash, + last_finalized_state_root: *state_root, + current_validator_set: validator_set, + } } } @@ -67,19 +81,7 @@ decl_storage! { /// Maps a bridge id to a bridge struct. Allows a single /// `bridge` module to manage multiple bridges. - pub BridgeFoo get(bridge_foo) config(): map BridgeId => Bridge; - - /// Get latest block number included in the chain - pub LastBlockNumber get(latest_block_num) config(): T::BlockNumber; - - /// Get the latest block header included in the chain - pub LastBlockHeader get(latest_block_header): Option; - - // Get the latest state root included in the chain - // pub LastStateRoot get(latest_state_root) config(): T::Hash; - - /// Latest set of validators - pub Validators get(validators) config(): Vec; + pub TrackedBridges get(tracked_bridges): map BridgeId => Option>; } } @@ -88,22 +90,27 @@ decl_module! { // TODO: Figure out the proper type for these proofs fn initialize_bridge( origin, - _block_header: T::Header, - _validator_set: Vec, - _validator_set_proof: Vec, - _storage_proof: Vec, + block_header: T::Header, + validator_set: Vec, + validator_set_proof: Vec, + storage_proof: Vec, ) { - // NOTE: Should this be a root call? - // Use srml/collective/src/lib.rs? + // NOTE: Will want to make this a governance issued call let _sender = ensure_signed(origin)?; - Self::check_storage_proof()?; - Self::check_validator_set_proof()?; + 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(); + let bridge_info = BridgeInfo::new(block_number, &block_hash, state_root, validator_set); let new_bridge_id = NumBridges::get() + 1; - // Hmm, can this call fail? - BridgeFoo::insert(new_bridge_id, Bridge::new()); + // 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); @@ -124,12 +131,12 @@ decl_error! { } impl Module { - fn check_storage_proof() -> std::result::Result<(), Error> { + fn check_storage_proof(_proof: Vec) -> std::result::Result<(), Error> { // ... Do some math ... Ok(()) // Otherwise, Error::InvalidStorageProof } - fn check_validator_set_proof() -> std::result::Result<(), Error> { + fn check_validator_set_proof(_proof: Vec) -> std::result::Result<(), Error> { // ... Do some math ... Ok(()) // Otherwise, Error::InvalidValidatorSetProof } @@ -155,7 +162,6 @@ mod tests { pub struct Test; type System = system::Module; - // type Bridge = Module; // With the Bridge struct this isn't great type MockBridge = Module; // TODO: Figure out what I actually need from here @@ -194,14 +200,8 @@ mod tests { fn new_test_ext() -> runtime_io::TestExternalities { let mut t = system::GenesisConfig::default().build_storage::().unwrap(); - GenesisConfig:: { + GenesisConfig { num_bridges: 0, - latest_block_num: 0, - bridge_foo: vec![(0, Bridge::new()), (1, Bridge::new())], - - // How do I get a default Hash? - // latest_state_root: Hash::default(), - validators: vec![], }.assimilate_storage(&mut t).unwrap(); t.into() } @@ -210,25 +210,43 @@ mod tests { fn it_works_for_default_value() { with_externalities(&mut new_test_ext(), || { assert_eq!(MockBridge::num_bridges(), 0); - assert_eq!(MockBridge::latest_block_num(), 0); }); } #[test] fn it_creates_a_new_bridge() { + let dummy_state_root = H256::default(); let test_header = Header { parent_hash: H256::default(), - number: 0, - state_root: H256::default(), + number: 42, + state_root: dummy_state_root, extrinsics_root: H256::default(), digest: Digest::default(), }; with_externalities(&mut new_test_ext(), || { assert_eq!(MockBridge::num_bridges(), 0); - assert_eq!(MockBridge::bridge_foo(0), Bridge::new()); - assert_ok!(MockBridge::initialize_bridge(Origin::signed(1), test_header, vec![], vec![], vec![])); - assert_eq!(MockBridge::bridge_foo(1), Bridge::new()); + assert_eq!(MockBridge::tracked_bridges(0), None); + + dbg!(&test_header); + assert_ok!( + MockBridge::initialize_bridge( + Origin::signed(1), + test_header, + vec![1, 2, 3], + vec![], + vec![], + )); + + assert_eq!( + MockBridge::tracked_bridges(1), + Some(BridgeInfo { + last_finalized_block_number: 42, + last_finalized_block_hash: test_header.hash(), // FIXME: This is broken + last_finalized_state_root: dummy_state_root, + current_validator_set: vec![1, 2, 3], + })); + assert_eq!(MockBridge::num_bridges(), 1); }); } From 74c63d028f5eb55515164dedd900acd2696f09fa Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Thu, 10 Oct 2019 12:18:58 +0200 Subject: [PATCH 07/34] Logic for checking Substrate proofs from within runtime module. (#3783) --- Cargo.lock | 3 + srml/bridge/Cargo.toml | 4 ++ srml/bridge/src/error.rs | 25 ++++++++ srml/bridge/src/lib.rs | 8 ++- srml/bridge/src/storage_proof.rs | 103 +++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 srml/bridge/src/error.rs create mode 100644 srml/bridge/src/storage_proof.rs diff --git a/Cargo.lock b/Cargo.lock index 28dc2485d3d24..cd5c502f0ea69 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4379,6 +4379,8 @@ dependencies = [ "srml-support 2.0.0", "srml-system 2.0.0", "substrate-primitives 2.0.0", + "substrate-state-machine 2.0.0", + "substrate-trie 2.0.0", ] [[package]] @@ -4480,6 +4482,7 @@ dependencies = [ name = "srml-bridge" version = "0.1.0" dependencies = [ + "hash-db 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.101 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 804e35e78e2c8..92c93954bf186 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -8,15 +8,18 @@ edition = "2018" [dependencies] codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } +hash-db = { version = "0.15.2", default-features = false } 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 } support = { package = "srml-support", path = "../support", default-features = false } 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" } [features] default = ["std"] @@ -27,5 +30,6 @@ std = [ "sr-primitives/std", "support/std", "system/std", + "trie/std", "runtime-io/std", ] diff --git a/srml/bridge/src/error.rs b/srml/bridge/src/error.rs new file mode 100644 index 0000000000000..182b0883d1e62 --- /dev/null +++ b/srml/bridge/src/error.rs @@ -0,0 +1,25 @@ +// 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, +} + diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index f24653a4f7987..625e22915d21c 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -32,6 +32,9 @@ // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] +mod error; +mod storage_proof; + use codec::{Encode, Decode}; use sr_primitives::traits::{Header, Member}; use support::{ @@ -148,7 +151,7 @@ mod tests { use primitives::{H256, Blake2Hasher}; use sr_primitives::{ - Perbill, traits::IdentityLookup, testing::Header, generic::Digest + Perbill, traits::{Header as HeaderT, IdentityLookup}, testing::Header, generic::Digest, }; use support::{assert_ok, impl_outer_origin, parameter_types}; use runtime_io::with_externalities; @@ -223,6 +226,7 @@ mod tests { extrinsics_root: H256::default(), digest: Digest::default(), }; + let test_hash = test_header.hash(); with_externalities(&mut new_test_ext(), || { assert_eq!(MockBridge::num_bridges(), 0); @@ -242,7 +246,7 @@ mod tests { MockBridge::tracked_bridges(1), Some(BridgeInfo { last_finalized_block_number: 42, - last_finalized_block_hash: test_header.hash(), // FIXME: This is broken + last_finalized_block_hash: test_hash, last_finalized_state_root: dummy_state_root, current_validator_set: vec![1, 2, 3], })); diff --git a/srml/bridge/src/storage_proof.rs b/srml/bridge/src/storage_proof.rs new file mode 100644 index 0000000000000..8217bb3b32e2f --- /dev/null +++ b/srml/bridge/src/storage_proof.rs @@ -0,0 +1,103 @@ +// 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 . + +//! Logic for checking Substrate storage proofs. + +use hash_db::{Hasher, HashDB, EMPTY_PREFIX}; +use trie::{MemoryDB, Trie, trie_types::TrieDB}; + +use crate::error::Error; + +/// This struct is used to read storage values from a subset of a Merklized database. The "proof" +/// is a subset of the nodes in the Merkle structure of the database, so that it provides +/// authentication against a known Merkle root as well as the values in the database themselves. +pub struct StorageProofChecker + where H: Hasher +{ + root: H::Out, + db: MemoryDB, +} + +impl StorageProofChecker + where H: Hasher +{ + /// Constructs a new storage proof checker. + /// + /// This returns an error if the given proof is invalid with respect to the given root. + pub fn new(root: H::Out, proof: Vec>) -> Result { + let mut db = MemoryDB::default(); + for item in proof { + db.insert(EMPTY_PREFIX, &item); + } + let checker = StorageProofChecker { + root, + db, + }; + // Return error if trie would be invalid. + let _ = checker.trie()?; + Ok(checker) + } + + /// Reads a value from the available subset of storage. If the value cannot be read due to an + /// incomplete or otherwise invalid proof, this returns an error. + pub fn read_value(&self, key: &[u8]) -> Result>, Error> { + self.trie()? + .get(key) + .map(|value| value.map(|value| value.into_vec())) + .map_err(|_| Error::StorageValueUnavailable) + } + + fn trie(&self) -> Result, Error> { + TrieDB::new(&self.db, &self.root) + .map_err(|_| Error::StorageRootMismatch) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + use primitives::{Blake2Hasher, H256}; + use state_machine::{prove_read, backend::{Backend, InMemory}}; + // use trie::{PrefixedMemoryDB, TrieDBMut}; + + #[test] + fn storage_proof_check() { + // construct storage proof + let backend = >::from(vec![ + (None, b"key1".to_vec(), Some(b"value1".to_vec())), + (None, b"key2".to_vec(), Some(b"value2".to_vec())), + (None, b"key3".to_vec(), Some(b"value3".to_vec())), + // Value is too big to fit in a branch node + (None, b"key11".to_vec(), Some(vec![0u8; 32])), + ]); + let root = backend.storage_root(std::iter::empty()).0; + let proof = prove_read(backend, &[&b"key1"[..], &b"key2"[..], &b"key22"[..]]).unwrap(); + + // check proof in runtime + let checker = >::new(root, proof.clone()).unwrap(); + assert_eq!(checker.read_value(b"key1"), Ok(Some(b"value1".to_vec()))); + assert_eq!(checker.read_value(b"key2"), Ok(Some(b"value2".to_vec()))); + assert_eq!(checker.read_value(b"key11111"), Err(Error::StorageValueUnavailable)); + assert_eq!(checker.read_value(b"key22"), Ok(None)); + + // checking proof against invalid commitment fails + assert_eq!( + >::new(H256::random(), proof).err(), + Some(Error::StorageRootMismatch) + ); + } +} From 73fa3cb27873bbe39438f04586f5a4bcfdefe212 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 23 Oct 2019 14:55:37 +0200 Subject: [PATCH 08/34] Make tests work after the changes introduced in #3793 (#3874) * Make tests work after the changes introduced in #3793 * Remove unneccessary import --- Cargo.lock | 3 ++- srml/bridge/src/lib.rs | 10 ++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cd5c502f0ea69..9498ed1d49bfd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4483,13 +4483,14 @@ name = "srml-bridge" version = "0.1.0" dependencies = [ "hash-db 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)", - "parity-scale-codec 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", + "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.101 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", "sr-primitives 2.0.0", "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", ] diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 625e22915d21c..f437c0445dd5b 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -149,12 +149,11 @@ impl Module { mod tests { use super::*; - use primitives::{H256, Blake2Hasher}; + use primitives::H256; use sr_primitives::{ Perbill, traits::{Header as HeaderT, IdentityLookup}, testing::Header, generic::Digest, }; use support::{assert_ok, impl_outer_origin, parameter_types}; - use runtime_io::with_externalities; // NOTE: What's this for? impl_outer_origin! { @@ -188,7 +187,6 @@ mod tests { type AccountId = DummyValidatorId; type Lookup = IdentityLookup; type Header = Header; - type WeightMultiplierUpdate = (); type Event = (); type BlockHashCount = (); type MaximumBlockWeight = (); @@ -201,7 +199,7 @@ mod tests { type ValidatorId = DummyValidatorId; } - fn new_test_ext() -> runtime_io::TestExternalities { + fn new_test_ext() -> runtime_io::TestExternalities { let mut t = system::GenesisConfig::default().build_storage::().unwrap(); GenesisConfig { num_bridges: 0, @@ -211,7 +209,7 @@ mod tests { #[test] fn it_works_for_default_value() { - with_externalities(&mut new_test_ext(), || { + new_test_ext().execute_with(|| { assert_eq!(MockBridge::num_bridges(), 0); }); } @@ -228,7 +226,7 @@ mod tests { }; let test_hash = test_header.hash(); - with_externalities(&mut new_test_ext(), || { + new_test_ext().execute_with(|| { assert_eq!(MockBridge::num_bridges(), 0); assert_eq!(MockBridge::tracked_bridges(0), None); From 4836977ddff3daae4fcd087e0f3f5dc6935f0b04 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 1 Nov 2019 15:41:58 +0100 Subject: [PATCH 09/34] Check given Grandpa validator set against set found in storage (#3915) * Make StorageProofChecker happy * Update some tests * Check given validator set against set found in storage * Use Finality Grandpa's Authority Id and Weight * Add better error handling * Use error type from decl_error! macro --- Cargo.lock | 2 +- srml/bridge/Cargo.toml | 3 +- srml/bridge/src/error.rs | 25 ------ srml/bridge/src/lib.rs | 148 +++++++++++++++++++++---------- srml/bridge/src/storage_proof.rs | 2 +- 5 files changed, 106 insertions(+), 74 deletions(-) delete mode 100644 srml/bridge/src/error.rs diff --git a/Cargo.lock b/Cargo.lock index 9498ed1d49bfd..5fe3486000bba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4490,7 +4490,7 @@ dependencies = [ "srml-session 2.0.0", "srml-support 2.0.0", "srml-system 2.0.0", - "substrate-externalities 2.0.0", + "substrate-finality-grandpa-primitives 2.0.0", "substrate-primitives 2.0.0", ] diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 92c93954bf186..6e63d0d6f443c 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -8,7 +8,9 @@ edition = "2018" [dependencies] codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } +fg_primitives = { package = "substrate-finality-grandpa-primitives", path = "../../core/finality-grandpa/primitives" } 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 +19,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/error.rs b/srml/bridge/src/error.rs deleted file mode 100644 index 182b0883d1e62..0000000000000 --- a/srml/bridge/src/error.rs +++ /dev/null @@ -1,25 +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, -} - diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index f437c0445dd5b..b217b15ffe8ae 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,18 +29,19 @@ //! (__)\ )\/\ //! ||----w | //! || || +//!``` // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] -mod error; mod storage_proof; +use crate::storage_proof::StorageProofChecker; use codec::{Encode, Decode}; -use sr_primitives::traits::{Header, Member}; +use fg_primitives::{AuthorityId, AuthorityWeight}; +use sr_primitives::traits::Header; use support::{ decl_error, decl_module, decl_storage, - Parameter, }; use system::{ensure_signed}; @@ -49,7 +51,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<(AuthorityId, AuthorityWeight)>, } impl BridgeInfo { @@ -57,7 +59,7 @@ impl BridgeInfo { block_number: &T::BlockNumber, block_hash: &T::Hash, state_root: &T::Hash, - validator_set: Vec, + validator_set: Vec<(AuthorityId, AuthorityWeight)>, ) -> Self { // I don't like how this is done, should come back to... @@ -72,10 +74,7 @@ impl BridgeInfo { type BridgeId = u64; -pub trait Trait: system::Trait { - /// A stable ID for a validator. - type ValidatorId: Member + Parameter; -} +pub trait Trait: system::Trait {} decl_storage! { trait Store for Module as Bridge { @@ -90,32 +89,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: Vec<(AuthorityId, AuthorityWeight)>, + 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); } @@ -129,19 +123,37 @@ decl_error! { // Error for the Bridge module pub enum Error { InvalidStorageProof, + StorageRootMismatch, + StorageValueUnavailable, InvalidValidatorSetProof, + ValidatorSetMismatch, } } impl Module { - fn check_storage_proof(_proof: Vec) -> std::result::Result<(), Error> { - // ... Do some math ... - Ok(()) // Otherwise, Error::InvalidStorageProof - } - - fn check_validator_set_proof(_proof: Vec) -> std::result::Result<(), Error> { - // ... Do some math ... - Ok(()) // Otherwise, Error::InvalidValidatorSetProof + fn check_validator_set_proof( + state_root: &T::Hash, + proof: Vec>, + validator_set: &Vec<(AuthorityId, AuthorityWeight)>, + ) -> std::result::Result<(), Error> { + + let checker = ::Hasher>>::new( + *state_root, + proof.clone() + )?; + + // 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")? + .ok_or(Error::StorageValueUnavailable)?; + + if encoded_validator_set == actual_validator_set { + Ok(()) + } else { + Err(Error::ValidatorSetMismatch) + } } } @@ -149,13 +161,12 @@ impl Module { mod tests { use super::*; - use primitives::H256; + use primitives::{Blake2Hasher, H256, Public}; 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! { pub enum Origin for Test {} } @@ -163,7 +174,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 @@ -175,7 +186,7 @@ mod tests { pub const AvailableBlockRatio: Perbill = Perbill::one(); } - type DummyValidatorId = u64; + type DummyAuthorityId = u64; impl system::Trait for Test { type Origin = Origin; @@ -184,7 +195,7 @@ mod tests { type Call = (); type Hash = H256; type Hashing = sr_primitives::traits::BlakeTwo256; - type AccountId = DummyValidatorId; + type AccountId = DummyAuthorityId; type Lookup = IdentityLookup; type Header = Header; type Event = (); @@ -195,9 +206,7 @@ mod tests { type Version = (); } - impl Trait for Test { - type ValidatorId = DummyValidatorId; - } + impl Trait for Test {} fn new_test_ext() -> runtime_io::TestExternalities { let mut t = system::GenesisConfig::default().build_storage::().unwrap(); @@ -214,13 +223,63 @@ mod tests { }); } + fn get_dummy_authorities() -> Vec<(AuthorityId, AuthorityWeight)> { + let authority1 = (AuthorityId::from_slice(&[1; 32]), 1); + let authority2 = (AuthorityId::from_slice(&[2; 32]), 1); + let authority3 = (AuthorityId::from_slice(&[3; 32]), 1); + + vec![authority1, authority2, authority3] + } + + fn create_dummy_validator_proof(validator_set: Vec<(AuthorityId, AuthorityWeight)>) -> (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":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":grandpa_authorities"[..]]).unwrap(); + + (root, proof) + } + + #[test] + fn it_can_validate_validator_sets() { + let authorities = get_dummy_authorities(); + let (root, proof) = create_dummy_validator_proof(authorities.clone()); + + assert_ok!(MockBridge::check_validator_set_proof(&root, proof, &authorities)); + } + + #[test] + fn it_rejects_invalid_validator_sets() { + let mut authorities = get_dummy_authorities(); + let (root, proof) = create_dummy_validator_proof(authorities.clone()); + + // Do something to make the authority set invalid + authorities.reverse(); + let invalid_authorities = authorities; + + assert_err!( + MockBridge::check_validator_set_proof(&root, proof, &invalid_authorities), + Error::ValidatorSetMismatch + ); + } + #[test] fn it_creates_a_new_bridge() { - let dummy_state_root = H256::default(); + let authorities = get_dummy_authorities(); + let (root, proof) = create_dummy_validator_proof(authorities.clone()); + 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(), }; @@ -228,16 +287,13 @@ 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( Origin::signed(1), test_header, - vec![1, 2, 3], - vec![], - vec![], + authorities.clone(), + proof, )); assert_eq!( @@ -245,8 +301,8 @@ mod tests { Some(BridgeInfo { last_finalized_block_number: 42, last_finalized_block_hash: test_hash, - last_finalized_state_root: dummy_state_root, - current_validator_set: vec![1, 2, 3], + last_finalized_state_root: root, + current_validator_set: authorities.clone(), })); assert_eq!(MockBridge::num_bridges(), 1); diff --git a/srml/bridge/src/storage_proof.rs b/srml/bridge/src/storage_proof.rs index 8217bb3b32e2f..b88192d56f2c4 100644 --- a/srml/bridge/src/storage_proof.rs +++ b/srml/bridge/src/storage_proof.rs @@ -19,7 +19,7 @@ use hash_db::{Hasher, HashDB, EMPTY_PREFIX}; use trie::{MemoryDB, Trie, trie_types::TrieDB}; -use crate::error::Error; +use crate::Error; /// This struct is used to read storage values from a subset of a Merklized database. The "proof" /// is a subset of the nodes in the Merkle structure of the database, so that it provides From 49ae74b7b9629da28eebbec06309ea76bf7990b8 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 5 Nov 2019 00:50:34 +0100 Subject: [PATCH 10/34] Verify Ancestry between Headers (#3963) * Create module for checking ancestry proofs * Use Vec of Headers instead of a HashMap * Move the ancestry verification into the lib.rs file * Change the proof format to exclude `child` and `ancestor` headers * Add a testing function for building header chains * Rename AncestorNotFound error to InvalidAncestryProof * Use ancestor hash instead of header when verifying ancestry * Clean up some stuff missed in the merge --- srml/bridge/src/lib.rs | 128 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 127 insertions(+), 1 deletion(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index b217b15ffe8ae..78bf721e8fb9f 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -127,6 +127,7 @@ decl_error! { StorageValueUnavailable, InvalidValidatorSetProof, ValidatorSetMismatch, + InvalidAncestryProof, } } @@ -135,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, @@ -157,6 +158,33 @@ impl Module { } } +// 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 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_hash: H::Hash, child: H) -> Result<(), Error> +where + H: Header +{ + 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 header in proof.iter() { + // Need to check that blocks are actually related + if header.hash() != *parent_hash { + break; + } + + parent_hash = header.parent_hash(); + if *parent_hash == ancestor_hash { + return Ok(()) + } + } + + Err(Error::InvalidAncestryProof) +} + #[cfg(test)] mod tests { use super::*; @@ -308,4 +336,102 @@ mod tests { assert_eq!(MockBridge::num_bridges(), 1); }); } + + 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(), + 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.hash(), child)); + } + + #[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(), + }; + + // 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); + + let fake_ancestor = 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_ancestor.hash(), child), + Error::InvalidAncestryProof + ); + } + + #[test] + 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, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + invalid_proof.insert(5, fake_ancestor); + + assert_err!( + verify_ancestry(invalid_proof, ancestor.hash(), child), + Error::InvalidAncestryProof + ); + } } From 50c6a2f1a055847dcffd10ae473a38d6b4cfa012 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 11 Nov 2019 21:56:40 +0100 Subject: [PATCH 11/34] Use new StorageProof type from #3834 --- srml/bridge/Cargo.toml | 2 +- srml/bridge/src/lib.rs | 8 ++++---- srml/bridge/src/storage_proof.rs | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 6e63d0d6f443c..9b6518cc93a00 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -14,13 +14,13 @@ 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 } +state-machine = { package = "substrate-state-machine", path = "../../core/state-machine" } support = { package = "srml-support", path = "../support", default-features = false } system = { package = "srml-system", path = "../system", default-features = false } trie = { package = "substrate-trie", path = "../../core/trie", default-features = false } [dev-dependencies] runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false } -state-machine = { package = "substrate-state-machine", path = "../../core/state-machine" } [features] default = ["std"] diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 78bf721e8fb9f..750afc2365122 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -40,6 +40,7 @@ use crate::storage_proof::StorageProofChecker; use codec::{Encode, Decode}; use fg_primitives::{AuthorityId, AuthorityWeight}; use sr_primitives::traits::Header; +use state_machine::StorageProof; use support::{ decl_error, decl_module, decl_storage, }; @@ -89,12 +90,11 @@ decl_storage! { decl_module! { pub struct Module for enum Call where origin: T::Origin { - // TODO: Change proof type to StorageProof once #3834 is merged fn initialize_bridge( origin, block_header: T::Header, validator_set: Vec<(AuthorityId, AuthorityWeight)>, - validator_set_proof: Vec>, + validator_set_proof: StorageProof, ) { // NOTE: Will want to make this a governance issued call let _sender = ensure_signed(origin)?; @@ -134,7 +134,7 @@ decl_error! { impl Module { fn check_validator_set_proof( state_root: &T::Hash, - proof: Vec>, + proof: StorageProof, validator_set: &Vec<(AuthorityId, AuthorityWeight)>, ) -> Result<(), Error> { @@ -259,7 +259,7 @@ mod tests { vec![authority1, authority2, authority3] } - fn create_dummy_validator_proof(validator_set: Vec<(AuthorityId, AuthorityWeight)>) -> (H256, Vec>) { + fn create_dummy_validator_proof(validator_set: Vec<(AuthorityId, AuthorityWeight)>) -> (H256, StorageProof) { use state_machine::{prove_read, backend::{Backend, InMemory}}; let encoded_set = validator_set.encode(); diff --git a/srml/bridge/src/storage_proof.rs b/srml/bridge/src/storage_proof.rs index b88192d56f2c4..a81468be1971c 100644 --- a/srml/bridge/src/storage_proof.rs +++ b/srml/bridge/src/storage_proof.rs @@ -17,6 +17,7 @@ //! Logic for checking Substrate storage proofs. use hash_db::{Hasher, HashDB, EMPTY_PREFIX}; +use state_machine::StorageProof; use trie::{MemoryDB, Trie, trie_types::TrieDB}; use crate::Error; @@ -37,9 +38,9 @@ impl StorageProofChecker /// Constructs a new storage proof checker. /// /// This returns an error if the given proof is invalid with respect to the given root. - pub fn new(root: H::Out, proof: Vec>) -> Result { + pub fn new(root: H::Out, proof: StorageProof) -> Result { let mut db = MemoryDB::default(); - for item in proof { + for item in proof.iter_nodes() { db.insert(EMPTY_PREFIX, &item); } let checker = StorageProofChecker { From 19295c508d27e39d445a2794ac3d83345d7d5236 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Thu, 14 Nov 2019 20:04:33 +0100 Subject: [PATCH 12/34] Store block headers instead of individual parts of header --- Cargo.lock | 6 +++--- srml/bridge/src/lib.rs | 39 +++++++++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5fe3486000bba..78f91c37f11a7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4379,8 +4379,6 @@ dependencies = [ "srml-support 2.0.0", "srml-system 2.0.0", "substrate-primitives 2.0.0", - "substrate-state-machine 2.0.0", - "substrate-trie 2.0.0", ] [[package]] @@ -4484,7 +4482,7 @@ version = "0.1.0" dependencies = [ "hash-db 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.101 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", "sr-primitives 2.0.0", "srml-session 2.0.0", @@ -4492,6 +4490,8 @@ dependencies = [ "srml-system 2.0.0", "substrate-finality-grandpa-primitives 2.0.0", "substrate-primitives 2.0.0", + "substrate-state-machine 2.0.0", + "substrate-trie 2.0.0", ] [[package]] diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 750afc2365122..069a3581c1355 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -49,25 +49,19 @@ use system::{ensure_signed}; #[derive(Encode, Decode, Clone, PartialEq)] #[cfg_attr(feature = "std", derive(Debug))] pub struct BridgeInfo { - last_finalized_block_number: T::BlockNumber, - last_finalized_block_hash: T::Hash, - last_finalized_state_root: T::Hash, + last_finalized_block_header: T::Header, current_validator_set: Vec<(AuthorityId, AuthorityWeight)>, } impl BridgeInfo { pub fn new( - block_number: &T::BlockNumber, - block_hash: &T::Hash, - state_root: &T::Hash, + block_header: T::Header, validator_set: Vec<(AuthorityId, AuthorityWeight)>, ) -> Self { // I don't like how this is done, should come back to... BridgeInfo { - last_finalized_block_number: *block_number, - last_finalized_block_hash: *block_hash, - last_finalized_state_root: *state_root, + last_finalized_block_header: block_header, current_validator_set: validator_set, } } @@ -99,13 +93,11 @@ decl_module! { // NOTE: Will want to make this a governance issued call let _sender = ensure_signed(origin)?; - 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 bridge_info = BridgeInfo::new(block_header, validator_set); let new_bridge_id = NumBridges::get() + 1; >::insert(new_bridge_id, bridge_info); @@ -113,8 +105,26 @@ decl_module! { NumBridges::put(new_bridge_id); } - fn submit_finalized_headers(origin) { + fn submit_finalized_headers( + origin, + bridge_id: BridgeId, + header: T::Header, + ancestry_proof: Vec, + _grandpa_proof: StorageProof, + ) { let _sender = ensure_signed(origin)?; + + // Check that the bridge exists + let mut bridge = >::get(bridge_id).ok_or(Error::NoSuchBridgeExists)?; + + // Check that the new header is a decendent of the old header + let last_header = bridge.last_finalized_block_header; + verify_ancestry(ancestry_proof, last_header.hash(), &header)?; + + // Check that the header has been finalized + + // Update last valid header seen by the bridge + bridge.last_finalized_block_header = header; } } } @@ -128,6 +138,7 @@ decl_error! { InvalidValidatorSetProof, ValidatorSetMismatch, InvalidAncestryProof, + NoSuchBridgeExists, } } @@ -163,7 +174,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_hash: H::Hash, child: H) -> Result<(), Error> +fn verify_ancestry(proof: Vec, ancestor_hash: H::Hash, child: &H) -> Result<(), Error> where H: Header { From ca20bb3c5f909710d0d4c0419a0ad2739c2a61e5 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 15 Nov 2019 11:47:19 +0100 Subject: [PATCH 13/34] Steal `justification.rs` from `grandpa-finality` crate --- srml/bridge/src/justification.rs | 225 +++++++++++++++++++++++++++++++ 1 file changed, 225 insertions(+) create mode 100644 srml/bridge/src/justification.rs diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs new file mode 100644 index 0000000000000..f5965df3e1228 --- /dev/null +++ b/srml/bridge/src/justification.rs @@ -0,0 +1,225 @@ +// Copyright 2018-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 . + +use std::collections::{HashMap, HashSet}; + +use client::{CallExecutor, Client}; +use client::backend::Backend; +use client::error::Error as ClientError; +use codec::{Encode, Decode}; +use grandpa::voter_set::VoterSet; +use grandpa::{Error as GrandpaError}; +use sr_primitives::generic::BlockId; +use sr_primitives::traits::{NumberFor, Block as BlockT, Header as HeaderT}; +use primitives::{H256, Blake2Hasher}; +use fg_primitives::AuthorityId; + +use crate::{Commit, Error}; +use crate::communication; + +/// A GRANDPA justification for block finality, it includes a commit message and +/// an ancestry proof including all headers routing all precommit target blocks +/// to the commit target block. Due to the current voting strategy the precommit +/// targets should be the same as the commit target, since honest voters don't +/// vote past authority set change blocks. +/// +/// This is meant to be stored in the db and passed around the network to other +/// nodes, and are used by syncing nodes to prove authority set handoffs. +#[derive(Encode, Decode)] +pub struct GrandpaJustification { + round: u64, + pub(crate) commit: Commit, + votes_ancestries: Vec, +} + +impl> GrandpaJustification { + /// Create a GRANDPA justification from the given commit. This method + /// assumes the commit is valid and well-formed. + pub(crate) fn from_commit( + client: &Client, + round: u64, + commit: Commit, + ) -> Result, Error> where + B: Backend, + E: CallExecutor + Send + Sync, + RA: Send + Sync, + { + let mut votes_ancestries_hashes = HashSet::new(); + let mut votes_ancestries = Vec::new(); + + let error = || { + let msg = "invalid precommits for target commit".to_string(); + Err(Error::Client(ClientError::BadJustification(msg))) + }; + + for signed in commit.precommits.iter() { + let mut current_hash = signed.precommit.target_hash.clone(); + loop { + if current_hash == commit.target_hash { break; } + + match client.header(&BlockId::Hash(current_hash))? { + Some(current_header) => { + if *current_header.number() <= commit.target_number { + return error(); + } + + let parent_hash = current_header.parent_hash().clone(); + if votes_ancestries_hashes.insert(current_hash) { + votes_ancestries.push(current_header); + } + current_hash = parent_hash; + }, + _ => return error(), + } + } + } + + Ok(GrandpaJustification { round, commit, votes_ancestries }) + } + + /// Decode a GRANDPA justification and validate the commit and the votes' + /// ancestry proofs finalize the given block. + pub(crate) fn decode_and_verify_finalizes( + encoded: &[u8], + finalized_target: (Block::Hash, NumberFor), + set_id: u64, + voters: &VoterSet, + ) -> Result, ClientError> where + NumberFor: grandpa::BlockNumberOps, + { + + let justification = GrandpaJustification::::decode(&mut &*encoded) + .map_err(|_| ClientError::JustificationDecode)?; + + if (justification.commit.target_hash, justification.commit.target_number) != finalized_target { + let msg = "invalid commit target in grandpa justification".to_string(); + Err(ClientError::BadJustification(msg)) + } else { + justification.verify(set_id, voters).map(|_| justification) + } + } + + /// Validate the commit and the votes' ancestry proofs. + pub(crate) fn verify(&self, set_id: u64, voters: &VoterSet) -> Result<(), ClientError> + where + NumberFor: grandpa::BlockNumberOps, + { + use grandpa::Chain; + + let ancestry_chain = AncestryChain::::new(&self.votes_ancestries); + + match grandpa::validate_commit( + &self.commit, + voters, + &ancestry_chain, + ) { + Ok(ref result) if result.ghost().is_some() => {}, + _ => { + let msg = "invalid commit in grandpa justification".to_string(); + return Err(ClientError::BadJustification(msg)); + } + } + + let mut visited_hashes = HashSet::new(); + for signed in self.commit.precommits.iter() { + if let Err(_) = communication::check_message_sig::( + &grandpa::Message::Precommit(signed.precommit.clone()), + &signed.id, + &signed.signature, + self.round, + set_id, + ) { + return Err(ClientError::BadJustification( + "invalid signature for precommit in grandpa justification".to_string()).into()); + } + + if self.commit.target_hash == signed.precommit.target_hash { + continue; + } + + match ancestry_chain.ancestry(self.commit.target_hash, signed.precommit.target_hash) { + Ok(route) => { + // ancestry starts from parent hash but the precommit target hash has been visited + visited_hashes.insert(signed.precommit.target_hash); + for hash in route { + visited_hashes.insert(hash); + } + }, + _ => { + return Err(ClientError::BadJustification( + "invalid precommit ancestry proof in grandpa justification".to_string()).into()); + }, + } + } + + let ancestry_hashes = self.votes_ancestries + .iter() + .map(|h: &Block::Header| h.hash()) + .collect(); + + if visited_hashes != ancestry_hashes { + return Err(ClientError::BadJustification( + "invalid precommit ancestries in grandpa justification with unused headers".to_string()).into()); + } + + Ok(()) + } +} + +/// A utility trait implementing `grandpa::Chain` using a given set of headers. +/// This is useful when validating commits, using the given set of headers to +/// verify a valid ancestry route to the target commit block. +struct AncestryChain { + ancestry: HashMap, +} + +impl AncestryChain { + fn new(ancestry: &[Block::Header]) -> AncestryChain { + let ancestry: HashMap<_, _> = ancestry + .iter() + .cloned() + .map(|h: Block::Header| (h.hash(), h)) + .collect(); + + AncestryChain { ancestry } + } +} + +impl grandpa::Chain> for AncestryChain where + NumberFor: grandpa::BlockNumberOps +{ + fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result, GrandpaError> { + let mut route = Vec::new(); + let mut current_hash = block; + loop { + if current_hash == base { break; } + match self.ancestry.get(¤t_hash) { + Some(current_header) => { + current_hash = *current_header.parent_hash(); + route.push(current_hash); + }, + _ => return Err(GrandpaError::NotDescendent), + } + } + route.pop(); // remove the base + + Ok(route) + } + + fn best_chain_containing(&self, _block: Block::Hash) -> Option<(Block::Hash, NumberFor)> { + None + } +} From 97e36f42c4c7323b929c9ec738c2a938f196a767 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Sun, 17 Nov 2019 17:12:49 +0100 Subject: [PATCH 14/34] WIP: Make `justification.rs` no_std compatable --- Cargo.lock | 4 +++ srml/bridge/Cargo.toml | 4 +++ srml/bridge/src/justification.rs | 59 +++++++++++++++++++++++++++----- srml/bridge/src/lib.rs | 1 + 4 files changed, 59 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 78f91c37f11a7..52e24e5cc460e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4480,14 +4480,18 @@ dependencies = [ name = "srml-bridge" version = "0.1.0" dependencies = [ + "finality-grandpa 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "hash-db 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", "sr-primitives 2.0.0", + "sr-std 2.0.0", "srml-session 2.0.0", "srml-support 2.0.0", "srml-system 2.0.0", + "substrate-client 2.0.0", + "substrate-finality-grandpa 2.0.0", "substrate-finality-grandpa-primitives 2.0.0", "substrate-primitives 2.0.0", "substrate-state-machine 2.0.0", diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 9b6518cc93a00..50e27b9643c39 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -8,9 +8,13 @@ edition = "2018" [dependencies] codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } +client = { package = "substrate-client", path = "../../core/client" } +fg = { package = "substrate-finality-grandpa", path = "../../core/finality-grandpa/" } fg_primitives = { package = "substrate-finality-grandpa-primitives", path = "../../core/finality-grandpa/primitives" } +grandpa = { package = "finality-grandpa", version = "0.9.0", features = ["derive-codec"] } hash-db = { version = "0.15.2", default-features = false } primitives = { package = "substrate-primitives", path = "../../core/primitives" } +rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false } 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 } diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index f5965df3e1228..1473c52114710 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -14,7 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -use std::collections::{HashMap, HashSet}; +// TODO: Use BTreeMap +// use std::collections::{HashMap, HashSet}; use client::{CallExecutor, Client}; use client::backend::Backend; @@ -22,13 +23,27 @@ use client::error::Error as ClientError; use codec::{Encode, Decode}; use grandpa::voter_set::VoterSet; use grandpa::{Error as GrandpaError}; +// Might be able to get this from primitives re-export +use rstd::collections::{ + btree_map::BTreeMap, + btree_set::BTreeSet, +}; use sr_primitives::generic::BlockId; +// use sr_primitives::sr_std; + use sr_primitives::traits::{NumberFor, Block as BlockT, Header as HeaderT}; use primitives::{H256, Blake2Hasher}; -use fg_primitives::AuthorityId; -use crate::{Commit, Error}; -use crate::communication; +// Actually, I don't think I can use Authority pair +// It's cfg(std) in grandpa-primitives +use fg_primitives::{AuthorityId, AuthorityPair, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; + +// Should I make this a part of fg_primitives? +use fg::{Commit, Error, Message}; + +// NOTE: Can't use this stuff +// use crate::{Commit, Error}; +// use crate::communication; /// A GRANDPA justification for block finality, it includes a commit message and /// an ancestry proof including all headers routing all precommit target blocks @@ -57,7 +72,8 @@ impl> GrandpaJustification { E: CallExecutor + Send + Sync, RA: Send + Sync, { - let mut votes_ancestries_hashes = HashSet::new(); + // Can't use this HashSet + let mut votes_ancestries_hashes = BTreeSet::new(); let mut votes_ancestries = Vec::new(); let error = || { @@ -133,9 +149,11 @@ impl> GrandpaJustification { } } - let mut visited_hashes = HashSet::new(); + // Jim says he would skip the stuff with `visited_hashes` + let mut visited_hashes = BTreeSet::new(); for signed in self.commit.precommits.iter() { - if let Err(_) = communication::check_message_sig::( + // NOTE: Rip this out, use sr_io primitives instead + if let Err(_) = check_message_sig::( &grandpa::Message::Precommit(signed.precommit.clone()), &signed.id, &signed.signature, @@ -183,12 +201,12 @@ impl> GrandpaJustification { /// This is useful when validating commits, using the given set of headers to /// verify a valid ancestry route to the target commit block. struct AncestryChain { - ancestry: HashMap, + ancestry: BTreeMap, } impl AncestryChain { fn new(ancestry: &[Block::Header]) -> AncestryChain { - let ancestry: HashMap<_, _> = ancestry + let ancestry: BTreeMap<_, _> = ancestry .iter() .cloned() .map(|h: Block::Header| (h.hash(), h)) @@ -223,3 +241,26 @@ impl grandpa::Chain> for AncestryCh None } } + +pub(crate) fn localized_payload(round: RoundNumber, set_id: SetIdNumber, message: &E) -> Vec { + (message, round, set_id).encode() +} + +// NOTE: Stolen from `communication/mod.rs` +// check a message. +fn check_message_sig( + message: &Message, + id: &AuthorityId, + signature: &AuthoritySignature, + round: RoundNumber, + set_id: SetIdNumber, +) -> Result<(), ()> { + let as_public = id.clone(); + let encoded_raw = localized_payload(round, set_id, message); + if AuthorityPair::verify(signature, &encoded_raw, &as_public) { + Ok(()) + } else { + // debug!(target: "afg", "Bad signature on message from {:?}", id); + Err(()) + } +} diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 069a3581c1355..27746771e2221 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -35,6 +35,7 @@ #![cfg_attr(not(feature = "std"), no_std)] mod storage_proof; +mod justification; use crate::storage_proof::StorageProofChecker; use codec::{Encode, Decode}; From 8e3e8edf329073b7c1b949574817cb2d7ce6a2db Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 19 Nov 2019 10:45:08 +0100 Subject: [PATCH 15/34] Swap HashMap for BTreeMap --- srml/bridge/Cargo.toml | 2 +- srml/bridge/src/justification.rs | 32 ++++++++++++++------------------ 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 50e27b9643c39..971ec4342a234 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -15,6 +15,7 @@ grandpa = { package = "finality-grandpa", version = "0.9.0", features = ["derive hash-db = { version = "0.15.2", default-features = false } primitives = { package = "substrate-primitives", path = "../../core/primitives" } rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false } +runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false } 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 } @@ -24,7 +25,6 @@ system = { package = "srml-system", path = "../system", default-features = false trie = { package = "substrate-trie", path = "../../core/trie", default-features = false } [dev-dependencies] -runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false } [features] default = ["std"] diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index 1473c52114710..95f809ee28f42 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -14,37 +14,30 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -// TODO: Use BTreeMap -// use std::collections::{HashMap, HashSet}; - use client::{CallExecutor, Client}; use client::backend::Backend; use client::error::Error as ClientError; use codec::{Encode, Decode}; use grandpa::voter_set::VoterSet; use grandpa::{Error as GrandpaError}; + // Might be able to get this from primitives re-export use rstd::collections::{ btree_map::BTreeMap, btree_set::BTreeSet, }; use sr_primitives::generic::BlockId; -// use sr_primitives::sr_std; - use sr_primitives::traits::{NumberFor, Block as BlockT, Header as HeaderT}; +use runtime_io; use primitives::{H256, Blake2Hasher}; -// Actually, I don't think I can use Authority pair +// NOTE: Actually, I don't think I can use Authority pair // It's cfg(std) in grandpa-primitives -use fg_primitives::{AuthorityId, AuthorityPair, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; +use fg_primitives::{AuthorityId, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; // Should I make this a part of fg_primitives? use fg::{Commit, Error, Message}; -// NOTE: Can't use this stuff -// use crate::{Commit, Error}; -// use crate::communication; - /// A GRANDPA justification for block finality, it includes a commit message and /// an ancestry proof including all headers routing all precommit target blocks /// to the commit target block. Due to the current voting strategy the precommit @@ -200,23 +193,25 @@ impl> GrandpaJustification { /// A utility trait implementing `grandpa::Chain` using a given set of headers. /// This is useful when validating commits, using the given set of headers to /// verify a valid ancestry route to the target commit block. -struct AncestryChain { - ancestry: BTreeMap, +// Since keys in a BTreeMap need to implement `Ord` we can't use Block::Hash directly. +// We need to turn the Hash into a slice of u8, which does implement Ord. +struct AncestryChain<'a, Block: BlockT> { + ancestry: BTreeMap<&'a [u8], Block::Header>, } -impl AncestryChain { +impl AncestryChain<'_, Block> { fn new(ancestry: &[Block::Header]) -> AncestryChain { let ancestry: BTreeMap<_, _> = ancestry .iter() .cloned() - .map(|h: Block::Header| (h.hash(), h)) + .map(|h: Block::Header| (h.hash().as_ref(), h)) .collect(); AncestryChain { ancestry } } } -impl grandpa::Chain> for AncestryChain where +impl grandpa::Chain> for AncestryChain<'_, Block> where NumberFor: grandpa::BlockNumberOps { fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result, GrandpaError> { @@ -224,7 +219,7 @@ impl grandpa::Chain> for AncestryCh let mut current_hash = block; loop { if current_hash == base { break; } - match self.ancestry.get(¤t_hash) { + match self.ancestry.get(current_hash.as_ref()) { Some(current_header) => { current_hash = *current_header.parent_hash(); route.push(current_hash); @@ -257,7 +252,8 @@ fn check_message_sig( ) -> Result<(), ()> { let as_public = id.clone(); let encoded_raw = localized_payload(round, set_id, message); - if AuthorityPair::verify(signature, &encoded_raw, &as_public) { + if runtime_io::crypto::ed25519_verify(signature, &encoded_raw, &as_public.into()) { + // if AuthorityPair::verify(signature, &encoded_raw, &as_public) { Ok(()) } else { // debug!(target: "afg", "Bad signature on message from {:?}", id); From efdaed86dbddd755fea3542908615ec9c765bd72 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 19 Nov 2019 19:22:50 +0100 Subject: [PATCH 16/34] Verify Grandpa signatures in `no_std` --- srml/bridge/src/justification.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index 95f809ee28f42..5c001a6d39249 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -26,13 +26,12 @@ use rstd::collections::{ btree_map::BTreeMap, btree_set::BTreeSet, }; + +use sr_primitives::app_crypto::RuntimeAppPublic; use sr_primitives::generic::BlockId; use sr_primitives::traits::{NumberFor, Block as BlockT, Header as HeaderT}; -use runtime_io; use primitives::{H256, Blake2Hasher}; -// NOTE: Actually, I don't think I can use Authority pair -// It's cfg(std) in grandpa-primitives use fg_primitives::{AuthorityId, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; // Should I make this a part of fg_primitives? @@ -252,8 +251,8 @@ fn check_message_sig( ) -> Result<(), ()> { let as_public = id.clone(); let encoded_raw = localized_payload(round, set_id, message); - if runtime_io::crypto::ed25519_verify(signature, &encoded_raw, &as_public.into()) { - // if AuthorityPair::verify(signature, &encoded_raw, &as_public) { + // Since `app::Public` implements `RuntimeAppPublic` we can call `verify()` + if as_public.verify(&encoded_raw, signature) { Ok(()) } else { // debug!(target: "afg", "Bad signature on message from {:?}", id); From 635c898db53de68e04eb53f05ec20c20b8e084f5 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 20 Nov 2019 11:50:22 +0100 Subject: [PATCH 17/34] Create a wrapper type for Block::Hash --- srml/bridge/src/justification.rs | 43 +++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index 5c001a6d39249..eb99fc769ee8d 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -189,28 +189,57 @@ impl> GrandpaJustification { } } +use core::cmp::{Ord, Ordering}; + +#[derive(Eq)] +struct BlockHashKey(Block::Hash); + +impl BlockHashKey { + fn new(hash: Block::Hash) -> Self { + Self(hash) + } +} + +impl Ord for BlockHashKey { + fn cmp(&self, other: &Self) -> Ordering { + self.0.as_ref().cmp(other.0.as_ref()) + } +} + +impl PartialOrd for BlockHashKey { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.0.as_ref().cmp(other.0.as_ref())) + } +} + +impl PartialEq for BlockHashKey { + fn eq(&self, other: &Self) -> bool { + self.0.as_ref() == other.0.as_ref() + } +} + /// A utility trait implementing `grandpa::Chain` using a given set of headers. /// This is useful when validating commits, using the given set of headers to /// verify a valid ancestry route to the target commit block. // Since keys in a BTreeMap need to implement `Ord` we can't use Block::Hash directly. // We need to turn the Hash into a slice of u8, which does implement Ord. -struct AncestryChain<'a, Block: BlockT> { - ancestry: BTreeMap<&'a [u8], Block::Header>, +struct AncestryChain { + ancestry: BTreeMap, Block::Header>, } -impl AncestryChain<'_, Block> { +impl AncestryChain { fn new(ancestry: &[Block::Header]) -> AncestryChain { let ancestry: BTreeMap<_, _> = ancestry .iter() .cloned() - .map(|h: Block::Header| (h.hash().as_ref(), h)) + .map(|h: Block::Header| (BlockHashKey::new(h.hash()), h)) .collect(); AncestryChain { ancestry } } } -impl grandpa::Chain> for AncestryChain<'_, Block> where +impl grandpa::Chain> for AncestryChain where NumberFor: grandpa::BlockNumberOps { fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result, GrandpaError> { @@ -218,7 +247,9 @@ impl grandpa::Chain> for AncestryCh let mut current_hash = block; loop { if current_hash == base { break; } - match self.ancestry.get(current_hash.as_ref()) { + + let key = BlockHashKey::new(current_hash); + match self.ancestry.get(&key) { Some(current_header) => { current_hash = *current_header.parent_hash(); route.push(current_hash); From f310f661762b28ebdf089503847d6b970c1ff636 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Thu, 21 Nov 2019 10:48:41 +0100 Subject: [PATCH 18/34] Clean up comments and imports a bit --- srml/bridge/src/justification.rs | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index eb99fc769ee8d..397821c2a21ea 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -18,6 +18,9 @@ use client::{CallExecutor, Client}; use client::backend::Backend; use client::error::Error as ClientError; use codec::{Encode, Decode}; +use core::cmp::{Ord, Ordering}; +use fg::{Commit, Error, Message}; // Q: Should I make any of these a part of fg_primitives? +use fg_primitives::{AuthorityId, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; use grandpa::voter_set::VoterSet; use grandpa::{Error as GrandpaError}; @@ -32,11 +35,6 @@ use sr_primitives::generic::BlockId; use sr_primitives::traits::{NumberFor, Block as BlockT, Header as HeaderT}; use primitives::{H256, Blake2Hasher}; -use fg_primitives::{AuthorityId, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; - -// Should I make this a part of fg_primitives? -use fg::{Commit, Error, Message}; - /// A GRANDPA justification for block finality, it includes a commit message and /// an ancestry proof including all headers routing all precommit target blocks /// to the commit target block. Due to the current voting strategy the precommit @@ -64,7 +62,6 @@ impl> GrandpaJustification { E: CallExecutor + Send + Sync, RA: Send + Sync, { - // Can't use this HashSet let mut votes_ancestries_hashes = BTreeSet::new(); let mut votes_ancestries = Vec::new(); @@ -98,6 +95,7 @@ impl> GrandpaJustification { Ok(GrandpaJustification { round, commit, votes_ancestries }) } + /// Decode a GRANDPA justification and validate the commit and the votes' /// ancestry proofs finalize the given block. pub(crate) fn decode_and_verify_finalizes( @@ -141,10 +139,8 @@ impl> GrandpaJustification { } } - // Jim says he would skip the stuff with `visited_hashes` let mut visited_hashes = BTreeSet::new(); for signed in self.commit.precommits.iter() { - // NOTE: Rip this out, use sr_io primitives instead if let Err(_) = check_message_sig::( &grandpa::Message::Precommit(signed.precommit.clone()), &signed.id, @@ -189,8 +185,9 @@ impl> GrandpaJustification { } } -use core::cmp::{Ord, Ordering}; - +// Since keys in a `BTreeMap` need to implement `Ord` we can't use Block::Hash directly. +// Instead we'll use a wrapper which implements `Ord` by leveraging the fact that +// `Block::Hash` implements `AsRef`, which itself implements `Ord` #[derive(Eq)] struct BlockHashKey(Block::Hash); @@ -221,8 +218,6 @@ impl PartialEq for BlockHashKey { /// A utility trait implementing `grandpa::Chain` using a given set of headers. /// This is useful when validating commits, using the given set of headers to /// verify a valid ancestry route to the target commit block. -// Since keys in a BTreeMap need to implement `Ord` we can't use Block::Hash directly. -// We need to turn the Hash into a slice of u8, which does implement Ord. struct AncestryChain { ancestry: BTreeMap, Block::Header>, } @@ -271,8 +266,8 @@ pub(crate) fn localized_payload(round: RoundNumber, set_id: SetIdNumb (message, round, set_id).encode() } -// NOTE: Stolen from `communication/mod.rs` -// check a message. +// Check the signature of a Grandpa message. +// This was originally taken from `communication/mod.rs` fn check_message_sig( message: &Message, id: &AuthorityId, @@ -282,6 +277,7 @@ fn check_message_sig( ) -> Result<(), ()> { let as_public = id.clone(); let encoded_raw = localized_payload(round, set_id, message); + // Since `app::Public` implements `RuntimeAppPublic` we can call `verify()` if as_public.verify(&encoded_raw, signature) { Ok(()) From fc3712f50cf62e66b2759fc33429a1e45e719715 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 22 Nov 2019 11:30:25 +0100 Subject: [PATCH 19/34] Bump `finality-grandpa` from v0.9.0 to v0.9.1 --- Cargo.lock | 16 +++++----------- core/finality-grandpa/Cargo.toml | 4 ++-- srml/bridge/Cargo.toml | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 52e24e5cc460e..8985e2854d8b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1107,11 +1107,11 @@ dependencies = [ [[package]] name = "finality-grandpa" -version = "0.9.0" +version = "0.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "futures 0.1.29 (registry+https://github.com/rust-lang/crates.io-index)", - "hashmap_core 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", + "hashbrown 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1519,11 +1519,6 @@ dependencies = [ "autocfg 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)", ] -[[package]] -name = "hashmap_core" -version = "0.1.11" -source = "registry+https://github.com/rust-lang/crates.io-index" - [[package]] name = "heapsize" version = "0.4.2" @@ -4480,7 +4475,7 @@ dependencies = [ name = "srml-bridge" version = "0.1.0" dependencies = [ - "finality-grandpa 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "finality-grandpa 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)", "hash-db 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", @@ -5728,7 +5723,7 @@ name = "substrate-finality-grandpa" version = "2.0.0" dependencies = [ "env_logger 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", - "finality-grandpa 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "finality-grandpa 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)", "fork-tree 2.0.0", "futures 0.1.29 (registry+https://github.com/rust-lang/crates.io-index)", "futures-preview 0.3.0-alpha.19 (registry+https://github.com/rust-lang/crates.io-index)", @@ -7684,7 +7679,7 @@ dependencies = [ "checksum fallible-iterator 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "4443176a9f2c162692bd3d352d745ef9413eec5782a80d8fd6f8a1ac692a07f7" "checksum fdlimit 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "b1ee15a7050e5580b3712877157068ea713b245b080ff302ae2ca973cfcd9baa" "checksum file-per-thread-logger 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "8505b75b31ef7285168dd237c4a7db3c1f3e0927e7d314e670bc98e854272fe9" -"checksum finality-grandpa 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "9681c1f75941ea47584573dd2bc10558b2067d460612945887e00744e43393be" +"checksum finality-grandpa 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)" = "34754852da8d86bc509715292c73140a5b678656d0b16132acd6737bdb5fd5f8" "checksum fixed-hash 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "6357b15872f8126e4ea7cf79d579473f132ccd2de239494ad1bf4aa892faea68" "checksum fixedbitset 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)" = "86d4de0081402f5e88cdac65c8dcdcc73118c1a7a465e2a05f0da05843a8ea33" "checksum flate2 1.0.12 (registry+https://github.com/rust-lang/crates.io-index)" = "ad3c5233c9a940c8719031b423d7e6c16af66e031cb0420b0896f5245bf181d3" @@ -7730,7 +7725,6 @@ dependencies = [ "checksum hash256-std-hasher 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)" = "92c171d55b98633f4ed3860808f004099b36c1cc29c42cfc53aa8591b21efcf2" "checksum hashbrown 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)" = "3bae29b6653b3412c2e71e9d486db9f9df5d701941d86683005efb9f2d28e3da" "checksum hashbrown 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)" = "6587d09be37fb98a11cb08b9000a3f592451c1b1b613ca69d949160e313a430a" -"checksum hashmap_core 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)" = "2d6852e5a86250521973b0c1d39677166d8a9c0047c908d7e04f1aa04177973c" "checksum heapsize 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "1679e6ea370dee694f91f1dc469bf94cf8f52051d147aec3e1f9497c6fc22461" "checksum heck 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "20564e78d53d2bb135c343b3f47714a56af2061f1c928fdb541dc7b9fdd94205" "checksum hex 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = "805026a5d0141ffc30abb3be3173848ad46a1b1664fe632428479619a3644d77" diff --git a/core/finality-grandpa/Cargo.toml b/core/finality-grandpa/Cargo.toml index d9b7fd176d639..35677101566cc 100644 --- a/core/finality-grandpa/Cargo.toml +++ b/core/finality-grandpa/Cargo.toml @@ -26,10 +26,10 @@ inherents = { package = "substrate-inherents", path = "../../core/inherents" } network = { package = "substrate-network", path = "../network" } srml-finality-tracker = { path = "../../srml/finality-tracker" } fg_primitives = { package = "substrate-finality-grandpa-primitives", path = "primitives" } -grandpa = { package = "finality-grandpa", version = "0.9.0", features = ["derive-codec"] } +grandpa = { package = "finality-grandpa", version = "0.9.1", features = ["derive-codec"] } [dev-dependencies] -grandpa = { package = "finality-grandpa", version = "0.9.0", features = ["derive-codec", "test-helpers"] } +grandpa = { package = "finality-grandpa", version = "0.9.1", features = ["derive-codec", "test-helpers"] } network = { package = "substrate-network", path = "../network", features = ["test-helpers"] } keyring = { package = "substrate-keyring", path = "../keyring" } test-client = { package = "substrate-test-runtime-client", path = "../test-runtime/client"} diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 971ec4342a234..b608082470442 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -11,7 +11,7 @@ codec = { package = "parity-scale-codec", version = "1.0.0", default-features = client = { package = "substrate-client", path = "../../core/client" } fg = { package = "substrate-finality-grandpa", path = "../../core/finality-grandpa/" } fg_primitives = { package = "substrate-finality-grandpa-primitives", path = "../../core/finality-grandpa/primitives" } -grandpa = { package = "finality-grandpa", version = "0.9.0", features = ["derive-codec"] } +grandpa = { package = "finality-grandpa", version = "0.9.1", features = ["derive-codec"] } hash-db = { version = "0.15.2", default-features = false } primitives = { package = "substrate-primitives", path = "../../core/primitives" } rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false } From 16fcd4131b543594ab1b92a8b8835344dab282bd Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 26 Nov 2019 13:50:29 +0100 Subject: [PATCH 20/34] Address some review comments --- srml/bridge/src/justification.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index 397821c2a21ea..039145920ce87 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -14,12 +14,16 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . +//! A module for creating a verifying the GRANDPA justifications. A justification +//! can be thought of as a finality proof. GRANDPA justifications consist +//! of a commit message plus an ancestry proof for pre-commits. + use client::{CallExecutor, Client}; use client::backend::Backend; use client::error::Error as ClientError; use codec::{Encode, Decode}; use core::cmp::{Ord, Ordering}; -use fg::{Commit, Error, Message}; // Q: Should I make any of these a part of fg_primitives? +use fg::{Commit, Error, Message}; use fg_primitives::{AuthorityId, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; use grandpa::voter_set::VoterSet; use grandpa::{Error as GrandpaError}; @@ -46,7 +50,7 @@ use primitives::{H256, Blake2Hasher}; #[derive(Encode, Decode)] pub struct GrandpaJustification { round: u64, - pub(crate) commit: Commit, + commit: Commit, votes_ancestries: Vec, } @@ -183,6 +187,11 @@ impl> GrandpaJustification { Ok(()) } + + /// Get the current commit message from the GRANDPA justification + pub(crate) fn get_commit(&self) -> Commit { + self.commit + } } // Since keys in a `BTreeMap` need to implement `Ord` we can't use Block::Hash directly. @@ -278,7 +287,6 @@ fn check_message_sig( let as_public = id.clone(); let encoded_raw = localized_payload(round, set_id, message); - // Since `app::Public` implements `RuntimeAppPublic` we can call `verify()` if as_public.verify(&encoded_raw, signature) { Ok(()) } else { From 5ac506513a9a991a5dc0fcb98285b6478a21cf39 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 26 Nov 2019 13:52:57 +0100 Subject: [PATCH 21/34] WIP: Verify justifications from module interface --- Cargo.lock | 1 + srml/bridge/Cargo.toml | 1 + srml/bridge/src/lib.rs | 58 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8985e2854d8b6..6f9a8cf99e7c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4477,6 +4477,7 @@ version = "0.1.0" dependencies = [ "finality-grandpa 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)", "hash-db 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)", + "num-traits 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)", "sr-io 2.0.0", diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index b608082470442..07af672ab4a71 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -14,6 +14,7 @@ fg_primitives = { package = "substrate-finality-grandpa-primitives", path = "../ grandpa = { package = "finality-grandpa", version = "0.9.1", features = ["derive-codec"] } hash-db = { version = "0.15.2", default-features = false } primitives = { package = "substrate-primitives", path = "../../core/primitives" } +num = { package = "num-traits", version = "0.2", default-features = false} rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false } runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false } session = { package = "srml-session", path = "../session", default-features = false, features = ["historical"] } diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 27746771e2221..0c4f06b124cfa 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -37,10 +37,17 @@ mod storage_proof; mod justification; +use crate::justification::GrandpaJustification; use crate::storage_proof::StorageProofChecker; + +use core::iter::FromIterator; use codec::{Encode, Decode}; -use fg_primitives::{AuthorityId, AuthorityWeight}; -use sr_primitives::traits::Header; +use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList}; +use grandpa::voter_set::VoterSet; // TODO: Check for `no_std` +use primitives::H256; +use num::AsPrimitive; +use sr_primitives::Justification; +use sr_primitives::traits::{Block as BlockT, Header, NumberFor}; use state_machine::StorageProof; use support::{ decl_error, decl_module, decl_storage, @@ -51,16 +58,15 @@ use system::{ensure_signed}; #[cfg_attr(feature = "std", derive(Debug))] pub struct BridgeInfo { last_finalized_block_header: T::Header, - current_validator_set: Vec<(AuthorityId, AuthorityWeight)>, + current_validator_set: AuthorityList, } impl BridgeInfo { pub fn new( block_header: T::Header, - validator_set: Vec<(AuthorityId, AuthorityWeight)>, + validator_set: AuthorityList, ) -> Self { - // I don't like how this is done, should come back to... BridgeInfo { last_finalized_block_header: block_header, current_validator_set: validator_set, @@ -70,7 +76,10 @@ impl BridgeInfo { type BridgeId = u64; -pub trait Trait: system::Trait {} +pub trait Trait: system::Trait { + // Type checker isn't happy with this :( + type Block: BlockT + AsPrimitive; +} decl_storage! { trait Store for Module as Bridge { @@ -88,7 +97,7 @@ decl_module! { fn initialize_bridge( origin, block_header: T::Header, - validator_set: Vec<(AuthorityId, AuthorityWeight)>, + validator_set: AuthorityList, validator_set_proof: StorageProof, ) { // NOTE: Will want to make this a governance issued call @@ -111,6 +120,7 @@ decl_module! { bridge_id: BridgeId, header: T::Header, ancestry_proof: Vec, + validator_set: AuthorityList, _grandpa_proof: StorageProof, ) { let _sender = ensure_signed(origin)?; @@ -122,7 +132,19 @@ decl_module! { let last_header = bridge.last_finalized_block_header; verify_ancestry(ancestry_proof, last_header.hash(), &header)?; + // Type checker isn't happy with this :( + let block_hash = header.hash(); + let block_num = header.number(); + // Check that the header has been finalized + let voter_set = VoterSet::from_iter(validator_set); + let v = verify_grandpa_proof::( + vec![], // Use this though -> justification: Justification, + block_hash, + block_num, + 0, + &voter_set, + ); // Update last valid header seen by the bridge bridge.last_finalized_block_header = header; @@ -197,6 +219,28 @@ where Err(Error::InvalidAncestryProof) } +// NOTE: Currently looking at `import::import_justification()` as a reference +fn verify_grandpa_proof( + justification: Justification, + hash: B::Hash, + number: NumberFor, + set_id: u64, + voters: &VoterSet, +) -> Result<(), Error> +where + B: BlockT, + NumberFor: grandpa::BlockNumberOps, +{ + let grandpa_justification = GrandpaJustification::::decode_and_verify_finalizes( + &justification, + (hash, number), + set_id, + voters, + ).unwrap(); + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; From bb013362c722c1a9112417b90af1196505ecde91 Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Tue, 26 Nov 2019 16:45:44 -0500 Subject: [PATCH 22/34] Fix compilation issues. --- srml/bridge/src/justification.rs | 2 +- srml/bridge/src/lib.rs | 22 ++++++++++++++++------ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index 039145920ce87..3ca205cd3e279 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -190,7 +190,7 @@ impl> GrandpaJustification { /// Get the current commit message from the GRANDPA justification pub(crate) fn get_commit(&self) -> Commit { - self.commit + self.commit.clone() } } diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 0c4f06b124cfa..7e2314041652d 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -76,13 +76,16 @@ impl BridgeInfo { type BridgeId = u64; -pub trait Trait: system::Trait { +pub trait Trait: system::Trait { // Type checker isn't happy with this :( - type Block: BlockT + AsPrimitive; + type Block: BlockT; } decl_storage! { - trait Store for Module as Bridge { + trait Store for Module as Bridge + where + NumberFor: AsPrimitive + { /// The number of current bridges managed by the module. pub NumBridges get(num_bridges) config(): BridgeId; @@ -93,7 +96,11 @@ decl_storage! { } decl_module! { - pub struct Module for enum Call where origin: T::Origin { + pub struct Module for enum Call + where + origin: T::Origin, + NumberFor: AsPrimitive + { fn initialize_bridge( origin, block_header: T::Header, @@ -134,7 +141,7 @@ decl_module! { // Type checker isn't happy with this :( let block_hash = header.hash(); - let block_num = header.number(); + let block_num = *header.number(); // Check that the header has been finalized let voter_set = VoterSet::from_iter(validator_set); @@ -165,7 +172,10 @@ decl_error! { } } -impl Module { +impl Module + where + NumberFor: AsPrimitive +{ fn check_validator_set_proof( state_root: &T::Hash, proof: StorageProof, From 0b11a29ad8bee7c5dfa0a81ef169cac977186482 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Thu, 28 Nov 2019 14:11:53 +0100 Subject: [PATCH 23/34] Make old tests compile again --- srml/bridge/src/lib.rs | 52 +++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 7e2314041652d..aae0602e5a0ce 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -42,7 +42,7 @@ use crate::storage_proof::StorageProofChecker; use core::iter::FromIterator; use codec::{Encode, Decode}; -use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList}; +use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList, SetId}; use grandpa::voter_set::VoterSet; // TODO: Check for `no_std` use primitives::H256; use num::AsPrimitive; @@ -77,7 +77,6 @@ impl BridgeInfo { type BridgeId = u64; pub trait Trait: system::Trait { - // Type checker isn't happy with this :( type Block: BlockT; } @@ -128,7 +127,7 @@ decl_module! { header: T::Header, ancestry_proof: Vec, validator_set: AuthorityList, - _grandpa_proof: StorageProof, + grandpa_proof: Justification, ) { let _sender = ensure_signed(origin)?; @@ -139,19 +138,18 @@ decl_module! { let last_header = bridge.last_finalized_block_header; verify_ancestry(ancestry_proof, last_header.hash(), &header)?; - // Type checker isn't happy with this :( let block_hash = header.hash(); let block_num = *header.number(); // Check that the header has been finalized let voter_set = VoterSet::from_iter(validator_set); - let v = verify_grandpa_proof::( - vec![], // Use this though -> justification: Justification, + verify_grandpa_proof::( + grandpa_proof, block_hash, block_num, - 0, + 0, // TODO: Use an actual set id &voter_set, - ); + )?; // Update last valid header seen by the bridge bridge.last_finalized_block_header = header; @@ -169,6 +167,7 @@ decl_error! { ValidatorSetMismatch, InvalidAncestryProof, NoSuchBridgeExists, + InvalidFinalityProof, } } @@ -241,14 +240,19 @@ where B: BlockT, NumberFor: grandpa::BlockNumberOps, { - let grandpa_justification = GrandpaJustification::::decode_and_verify_finalizes( + // We don't really care about the justification, as long as it's valid + let j = GrandpaJustification::::decode_and_verify_finalizes( &justification, (hash, number), set_id, voters, - ).unwrap(); + ); - Ok(()) + // Uh, there's got to be a nicer way of doing this... + match j { + Err(_e) => Err(Error::InvalidFinalityProof), + Ok(_) => Ok(()), + } } #[cfg(test)] @@ -257,7 +261,13 @@ mod tests { use primitives::{Blake2Hasher, H256, Public}; use sr_primitives::{ - Perbill, traits::{Header as HeaderT, IdentityLookup}, testing::Header, generic::Digest, + Perbill, + traits::{ + Header as HeaderT, + IdentityLookup + }, + testing::{Block, Header, TestXt}, + generic::Digest, }; use support::{assert_ok, assert_err, impl_outer_origin, parameter_types}; @@ -300,7 +310,9 @@ mod tests { type Version = (); } - impl Trait for Test {} + impl Trait for Test { + type Block = Block>; + } fn new_test_ext() -> runtime_io::TestExternalities { let mut t = system::GenesisConfig::default().build_storage::().unwrap(); @@ -385,7 +397,7 @@ mod tests { assert_ok!( MockBridge::initialize_bridge( Origin::signed(1), - test_header, + test_header.clone(), authorities.clone(), proof, )); @@ -393,10 +405,8 @@ mod tests { assert_eq!( MockBridge::tracked_bridges(1), Some(BridgeInfo { - last_finalized_block_number: 42, - last_finalized_block_hash: test_hash, - last_finalized_state_root: root, - current_validator_set: authorities.clone(), + last_finalized_block_header: test_header, + current_validator_set: authorities, })); assert_eq!(MockBridge::num_bridges(), 1); @@ -440,7 +450,7 @@ mod tests { let mut proof = build_header_chain(ancestor.clone(), 10); let child = proof.remove(0); - assert_ok!(verify_ancestry(proof, ancestor.hash(), child)); + assert_ok!(verify_ancestry(proof, ancestor.hash(), &child)); } #[test] @@ -466,7 +476,7 @@ mod tests { }; assert_err!( - verify_ancestry(proof, fake_ancestor.hash(), child), + verify_ancestry(proof, fake_ancestor.hash(), &child), Error::InvalidAncestryProof ); } @@ -496,7 +506,7 @@ mod tests { invalid_proof.insert(5, fake_ancestor); assert_err!( - verify_ancestry(invalid_proof, ancestor.hash(), child), + verify_ancestry(invalid_proof, ancestor.hash(), &child), Error::InvalidAncestryProof ); } From 0ddbc40e297b41da683f7318577434eec5b40d95 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 2 Dec 2019 14:53:50 +0100 Subject: [PATCH 24/34] WIP: Add test for creating justifications --- Cargo.lock | 2 ++ srml/bridge/Cargo.toml | 2 ++ srml/bridge/src/lib.rs | 66 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 6f9a8cf99e7c2..24c458e18d885 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4489,8 +4489,10 @@ dependencies = [ "substrate-client 2.0.0", "substrate-finality-grandpa 2.0.0", "substrate-finality-grandpa-primitives 2.0.0", + "substrate-keyring 2.0.0", "substrate-primitives 2.0.0", "substrate-state-machine 2.0.0", + "substrate-test-runtime-client 2.0.0", "substrate-trie 2.0.0", ] diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 07af672ab4a71..38f29e25a8f12 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -26,6 +26,8 @@ system = { package = "srml-system", path = "../system", default-features = false trie = { package = "substrate-trie", path = "../../core/trie", default-features = false } [dev-dependencies] +keyring = { package = "substrate-keyring", path = "../../core/keyring" } +test-client = { package = "substrate-test-runtime-client", path = "../../core/test-runtime/client" } [features] default = ["std"] diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index aae0602e5a0ce..e618873099edb 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -263,6 +263,7 @@ mod tests { use sr_primitives::{ Perbill, traits::{ + Block as BlockT, Header as HeaderT, IdentityLookup }, @@ -510,4 +511,69 @@ mod tests { Error::InvalidAncestryProof ); } + + // TODO: Move this to the a better place + use test_client::prelude::*; + use fg::Commit; + use keyring::Ed25519Keyring; + + // Currently stealing this from `core/finality-grandpa/src/test.rs` + fn create_grandpa_justification>() -> GrandpaJustification { + let client = test_client::new(); + let header = Header { + parent_hash: H256::default(), + number: 1, + state_root: H256::default(), + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + // TODO: Change to authority + let peers = &[Ed25519Keyring::Alice]; + + let justification = { + let round = 1; + let set_id = 0; + + let precommit = grandpa::Precommit { + target_hash: header.hash(), + target_number: *header.number(), + }; + + let msg = grandpa::Message::Precommit(precommit.clone()); + let encoded = justification::localized_payload(round, set_id, &msg); + let signature = peers[0].sign(&encoded[..]).into(); + + let precommit = grandpa::SignedPrecommit { + precommit, + signature, + id: peers[0].public().into(), + }; + + let commit = grandpa::Commit { + target_hash: header.hash(), + target_number: *header.number(), + precommits: vec![precommit], + }; + + GrandpaJustification::::from_commit( + &client, + round, + commit, + ).unwrap() + }; + + justification + } + + #[test] + fn can_create_valid_justifications() { + let justification = create_grandpa_justification(); + let encoded = justification.encode(); + } + + #[test] + fn can_verify_grandpa_proofs() { + unimplemented!() + } } From 3a6bc0dd244da53890c600796b599c2d47c16f86 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 3 Dec 2019 17:01:23 +0100 Subject: [PATCH 25/34] Add a test for verifying and updating new headers --- srml/bridge/src/lib.rs | 102 ++++++++++++++++++++++++++++++----------- 1 file changed, 75 insertions(+), 27 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index e618873099edb..d4a611be2a5ff 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -42,7 +42,7 @@ use crate::storage_proof::StorageProofChecker; use core::iter::FromIterator; use codec::{Encode, Decode}; -use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList, SetId}; +use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList, SetId, RoundNumber}; use grandpa::voter_set::VoterSet; // TODO: Check for `no_std` use primitives::H256; use num::AsPrimitive; @@ -132,7 +132,7 @@ decl_module! { let _sender = ensure_signed(origin)?; // Check that the bridge exists - let mut bridge = >::get(bridge_id).ok_or(Error::NoSuchBridgeExists)?; + let bridge = >::get(bridge_id).ok_or(Error::NoSuchBridgeExists)?; // Check that the new header is a decendent of the old header let last_header = bridge.last_finalized_block_header; @@ -151,8 +151,14 @@ decl_module! { &voter_set, )?; - // Update last valid header seen by the bridge - bridge.last_finalized_block_header = header; + // Update storage with current bridge's latest finalized header + >::mutate(bridge_id, |bridge| { + bridge + .as_mut() + .expect( + "We succesfully got this bridge earlier, therefore it exists; qed" + ).last_finalized_block_header = header; + }); } } } @@ -249,8 +255,9 @@ where ); // Uh, there's got to be a nicer way of doing this... + // TODO: Remote dbg! match j { - Err(_e) => Err(Error::InvalidFinalityProof), + Err(_e) => { dbg!(_e); Err(Error::InvalidFinalityProof) }, Ok(_) => Ok(()), } } @@ -330,7 +337,7 @@ mod tests { }); } - fn get_dummy_authorities() -> Vec<(AuthorityId, AuthorityWeight)> { + fn get_dummy_authorities() -> AuthorityList { let authority1 = (AuthorityId::from_slice(&[1; 32]), 1); let authority2 = (AuthorityId::from_slice(&[2; 32]), 1); let authority3 = (AuthorityId::from_slice(&[3; 32]), 1); @@ -394,7 +401,6 @@ mod tests { new_test_ext().execute_with(|| { assert_eq!(MockBridge::num_bridges(), 0); - dbg!(&test_header); assert_ok!( MockBridge::initialize_bridge( Origin::signed(1), @@ -408,7 +414,7 @@ mod tests { Some(BridgeInfo { last_finalized_block_header: test_header, current_validator_set: authorities, - })); + })); assert_eq!(MockBridge::num_bridges(), 1); }); @@ -518,23 +524,13 @@ mod tests { use keyring::Ed25519Keyring; // Currently stealing this from `core/finality-grandpa/src/test.rs` - fn create_grandpa_justification>() -> GrandpaJustification { + fn create_grandpa_justification(header: Header, round: RoundNumber, set_id: SetId) -> GrandpaJustification { let client = test_client::new(); - let header = Header { - parent_hash: H256::default(), - number: 1, - state_root: H256::default(), - extrinsics_root: H256::default(), - digest: Digest::default(), - }; // TODO: Change to authority let peers = &[Ed25519Keyring::Alice]; let justification = { - let round = 1; - let set_id = 0; - let precommit = grandpa::Precommit { target_hash: header.hash(), target_number: *header.number(), @@ -556,7 +552,7 @@ mod tests { precommits: vec![precommit], }; - GrandpaJustification::::from_commit( + GrandpaJustification::from_commit( &client, round, commit, @@ -567,13 +563,65 @@ mod tests { } #[test] - fn can_create_valid_justifications() { - let justification = create_grandpa_justification(); - let encoded = justification.encode(); - } + fn correctly_accepts_a_new_finalized_header() { + let authorities = vec![(Ed25519Keyring::Alice.public().into(), 1)]; + let (storage_root, validator_proof) = create_dummy_validator_proof(authorities.clone()); - #[test] - fn can_verify_grandpa_proofs() { - unimplemented!() + let ancestor = Header { + parent_hash: H256::default(), + number: 1, + state_root: storage_root, + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + // A valid proof doesn't include the child header, so remove it + let mut block_ancestry_proof = build_header_chain(ancestor.clone(), 5); + let child = block_ancestry_proof.remove(0); + + let round = 1; + let set_id = 0; + let justification = create_grandpa_justification(child.clone(), round, set_id); + let encoded: Justification = justification.encode(); + + new_test_ext().execute_with(|| { + assert_eq!(MockBridge::num_bridges(), 0); + assert_ok!( + MockBridge::initialize_bridge( + Origin::signed(1), + ancestor.clone(), + authorities.clone(), + validator_proof, + )); + + // Check that the header we sent on initialization was stored + assert_eq!( + MockBridge::tracked_bridges(1), + Some(BridgeInfo { + last_finalized_block_header: ancestor.clone(), + current_validator_set: authorities.clone(), + }) + ); + + // Send over the new header + proofs + let bridge_id = 1; + assert_ok!(MockBridge::submit_finalized_headers( + Origin::signed(1), + bridge_id, + child.clone(), + block_ancestry_proof, + authorities.clone(), + encoded, + )); + + // Check that the header was correctly updated + assert_eq!( + MockBridge::tracked_bridges(1), + Some(BridgeInfo { + last_finalized_block_header: child, + current_validator_set: authorities, + }) + ); + }); } } From be98ec8a5149dba2e8e051f87c72377fa26f8829 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 4 Dec 2019 12:07:08 +0100 Subject: [PATCH 26/34] Add test for checking that commits were signed by correct authorities --- srml/bridge/src/lib.rs | 92 +++++++++++++++++++++++++++++++++++------- 1 file changed, 77 insertions(+), 15 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index d4a611be2a5ff..7d6b53f28683b 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -159,6 +159,8 @@ decl_module! { "We succesfully got this bridge earlier, therefore it exists; qed" ).last_finalized_block_header = header; }); + + // TODO: Update validator set if necessary. Still need to figure out details. } } } @@ -255,9 +257,8 @@ where ); // Uh, there's got to be a nicer way of doing this... - // TODO: Remote dbg! match j { - Err(_e) => { dbg!(_e); Err(Error::InvalidFinalityProof) }, + Err(_e) => Err(Error::InvalidFinalityProof), Ok(_) => Ok(()), } } @@ -266,6 +267,7 @@ where mod tests { use super::*; + use keyring::Ed25519Keyring; use primitives::{Blake2Hasher, H256, Public}; use sr_primitives::{ Perbill, @@ -397,7 +399,6 @@ mod tests { extrinsics_root: H256::default(), digest: Digest::default(), }; - let test_hash = test_header.hash(); new_test_ext().execute_with(|| { assert_eq!(MockBridge::num_bridges(), 0); @@ -518,18 +519,16 @@ mod tests { ); } - // TODO: Move this to the a better place - use test_client::prelude::*; - use fg::Commit; - use keyring::Ed25519Keyring; // Currently stealing this from `core/finality-grandpa/src/test.rs` - fn create_grandpa_justification(header: Header, round: RoundNumber, set_id: SetId) -> GrandpaJustification { + fn create_grandpa_justification( + authority: Ed25519Keyring, + header: Header, + round: RoundNumber, + set_id: SetId, + ) -> GrandpaJustification { let client = test_client::new(); - // TODO: Change to authority - let peers = &[Ed25519Keyring::Alice]; - let justification = { let precommit = grandpa::Precommit { target_hash: header.hash(), @@ -538,12 +537,12 @@ mod tests { let msg = grandpa::Message::Precommit(precommit.clone()); let encoded = justification::localized_payload(round, set_id, &msg); - let signature = peers[0].sign(&encoded[..]).into(); + let signature = authority.sign(&encoded[..]).into(); let precommit = grandpa::SignedPrecommit { precommit, signature, - id: peers[0].public().into(), + id: authority.public().into(), }; let commit = grandpa::Commit { @@ -564,7 +563,8 @@ mod tests { #[test] fn correctly_accepts_a_new_finalized_header() { - let authorities = vec![(Ed25519Keyring::Alice.public().into(), 1)]; + let signer = Ed25519Keyring::Alice; + let authorities = vec![(signer.public().into(), 1)]; let (storage_root, validator_proof) = create_dummy_validator_proof(authorities.clone()); let ancestor = Header { @@ -581,7 +581,7 @@ mod tests { let round = 1; let set_id = 0; - let justification = create_grandpa_justification(child.clone(), round, set_id); + let justification = create_grandpa_justification(signer, child.clone(), round, set_id); let encoded: Justification = justification.encode(); new_test_ext().execute_with(|| { @@ -624,4 +624,66 @@ mod tests { ); }); } + + #[test] + fn rejects_header_if_proof_is_signed_by_wrong_authorities() { + let signer = Ed25519Keyring::Alice; + let bad_signer = Ed25519Keyring::Bob; + let authorities = vec![(signer.public().into(), 1)]; + + let (storage_root, validator_proof) = create_dummy_validator_proof(authorities.clone()); + + let ancestor = Header { + parent_hash: H256::default(), + number: 1, + state_root: storage_root, + extrinsics_root: H256::default(), + digest: Digest::default(), + }; + + // A valid proof doesn't include the child header, so remove it + let mut block_ancestry_proof = build_header_chain(ancestor.clone(), 5); + let child = block_ancestry_proof.remove(0); + + let round = 1; + let set_id = 0; + + // Create a justification with an authority that's *not* part of the authority set + let justification = create_grandpa_justification(bad_signer, child.clone(), round, set_id); + let encoded: Justification = justification.encode(); + + new_test_ext().execute_with(|| { + assert_eq!(MockBridge::num_bridges(), 0); + assert_ok!( + MockBridge::initialize_bridge( + Origin::signed(1), + ancestor.clone(), + authorities.clone(), + validator_proof, + )); + + // Check that the header we sent on initialization was stored + assert_eq!( + MockBridge::tracked_bridges(1), + Some(BridgeInfo { + last_finalized_block_header: ancestor.clone(), + current_validator_set: authorities.clone(), + }) + ); + + // Send over the new header + proofs + let bridge_id = 1; + assert_err!( + MockBridge::submit_finalized_headers( + Origin::signed(1), + bridge_id, + child.clone(), + block_ancestry_proof, + authorities.clone(), + encoded, + ), + Error::InvalidFinalityProof.into() + ); + }); + } } From 6b3ec92f70824325287db1690ddadd42df97952a Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 4 Dec 2019 12:16:53 +0100 Subject: [PATCH 27/34] Use a non-hardcoded authority set id --- srml/bridge/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 7d6b53f28683b..9911a9cfc8eda 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -42,8 +42,8 @@ use crate::storage_proof::StorageProofChecker; use core::iter::FromIterator; use codec::{Encode, Decode}; -use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList, SetId, RoundNumber}; -use grandpa::voter_set::VoterSet; // TODO: Check for `no_std` +use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList, SetId}; +use grandpa::voter_set::VoterSet; use primitives::H256; use num::AsPrimitive; use sr_primitives::Justification; @@ -127,6 +127,7 @@ decl_module! { header: T::Header, ancestry_proof: Vec, validator_set: AuthorityList, + validator_set_id: SetId, grandpa_proof: Justification, ) { let _sender = ensure_signed(origin)?; @@ -147,7 +148,7 @@ decl_module! { grandpa_proof, block_hash, block_num, - 0, // TODO: Use an actual set id + validator_set_id, &voter_set, )?; @@ -236,7 +237,6 @@ where Err(Error::InvalidAncestryProof) } -// NOTE: Currently looking at `import::import_justification()` as a reference fn verify_grandpa_proof( justification: Justification, hash: B::Hash, @@ -267,6 +267,7 @@ where mod tests { use super::*; + use fg_primitives::RoundNumber; use keyring::Ed25519Keyring; use primitives::{Blake2Hasher, H256, Public}; use sr_primitives::{ @@ -611,6 +612,7 @@ mod tests { child.clone(), block_ancestry_proof, authorities.clone(), + set_id, encoded, )); @@ -680,6 +682,7 @@ mod tests { child.clone(), block_ancestry_proof, authorities.clone(), + set_id, encoded, ), Error::InvalidFinalityProof.into() From cc4920798707ce30b3d0abf9ea8a4f3fbe42ffed Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 4 Dec 2019 14:26:21 +0100 Subject: [PATCH 28/34] Handle ClientErrors in a nicer way --- srml/bridge/src/lib.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 9911a9cfc8eda..10a82e276aaf2 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -42,6 +42,7 @@ use crate::storage_proof::StorageProofChecker; use core::iter::FromIterator; use codec::{Encode, Decode}; +use client::error::Error as ClientError; // Not sure if this is allowed to be brought in... use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList, SetId}; use grandpa::voter_set::VoterSet; use primitives::H256; @@ -177,6 +178,18 @@ decl_error! { InvalidAncestryProof, NoSuchBridgeExists, InvalidFinalityProof, + UnknownClientError, + } +} + +impl From for Error { + fn from(e: ClientError) -> Self { + match e { + ClientError::BadJustification(_) | ClientError::JustificationDecode => { + Error::InvalidFinalityProof + }, + _ => Error::UnknownClientError, + } } } @@ -249,18 +262,14 @@ where NumberFor: grandpa::BlockNumberOps, { // We don't really care about the justification, as long as it's valid - let j = GrandpaJustification::::decode_and_verify_finalizes( + let _ = GrandpaJustification::::decode_and_verify_finalizes( &justification, (hash, number), set_id, voters, - ); + )?; - // Uh, there's got to be a nicer way of doing this... - match j { - Err(_e) => Err(Error::InvalidFinalityProof), - Ok(_) => Ok(()), - } + Ok(()) } #[cfg(test)] From ee7ef25e373499b4287864d0d100264abb1a3073 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 9 Dec 2019 02:32:42 +0100 Subject: [PATCH 29/34] Turn off `std` feature for some imports --- srml/bridge/Cargo.toml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 38f29e25a8f12..d55caa2422d09 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -10,11 +10,11 @@ edition = "2018" codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } client = { package = "substrate-client", path = "../../core/client" } fg = { package = "substrate-finality-grandpa", path = "../../core/finality-grandpa/" } -fg_primitives = { package = "substrate-finality-grandpa-primitives", path = "../../core/finality-grandpa/primitives" } -grandpa = { package = "finality-grandpa", version = "0.9.1", features = ["derive-codec"] } +fg_primitives = { package = "substrate-finality-grandpa-primitives", path ="../../core/finality-grandpa/primitives", default-features = false } +grandpa = { package = "finality-grandpa", version = "0.9.1", default-features = false, features = ["derive-codec"] } hash-db = { version = "0.15.2", default-features = false } -primitives = { package = "substrate-primitives", path = "../../core/primitives" } -num = { package = "num-traits", version = "0.2", default-features = false} +primitives = { package = "substrate-primitives", path = "../../core/primitives", default-features = false } +num = { package = "num-traits", version = "0.2", default-features = false } rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false } runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false } session = { package = "srml-session", path = "../session", default-features = false, features = ["historical"] } From 91935c4f9b5e0241b13c5df4f633f44adca53474 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 9 Dec 2019 03:11:48 +0100 Subject: [PATCH 30/34] Get rid of `state-machine` dependency --- srml/bridge/Cargo.toml | 2 +- srml/bridge/src/lib.rs | 8 +++++--- srml/bridge/src/storage_proof.rs | 11 +++++++---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index d55caa2422d09..8b73ab519707e 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -20,13 +20,13 @@ runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = 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 } -state-machine = { package = "substrate-state-machine", path = "../../core/state-machine" } support = { package = "srml-support", path = "../support", default-features = false } system = { package = "srml-system", path = "../system", default-features = false } trie = { package = "substrate-trie", path = "../../core/trie", default-features = false } [dev-dependencies] keyring = { package = "substrate-keyring", path = "../../core/keyring" } +state-machine = { package = "substrate-state-machine", path = "../../core/state-machine" } test-client = { package = "substrate-test-runtime-client", path = "../../core/test-runtime/client" } [features] diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 10a82e276aaf2..c404509b59237 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -38,7 +38,7 @@ mod storage_proof; mod justification; use crate::justification::GrandpaJustification; -use crate::storage_proof::StorageProofChecker; +use crate::storage_proof::{StorageProof, StorageProofChecker}; use core::iter::FromIterator; use codec::{Encode, Decode}; @@ -49,7 +49,6 @@ use primitives::H256; use num::AsPrimitive; use sr_primitives::Justification; use sr_primitives::traits::{Block as BlockT, Header, NumberFor}; -use state_machine::StorageProof; use support::{ decl_error, decl_module, decl_storage, }; @@ -369,7 +368,10 @@ mod tests { let root = backend.storage_root(std::iter::empty()).0; // Generates a storage read proof - let proof = prove_read(backend, &[&b":grandpa_authorities"[..]]).unwrap(); + let proof: StorageProof = prove_read(backend, &[&b":grandpa_authorities"[..]]) + .unwrap() + .iter_nodes() + .collect(); (root, proof) } diff --git a/srml/bridge/src/storage_proof.rs b/srml/bridge/src/storage_proof.rs index a81468be1971c..b9a9baa9c36de 100644 --- a/srml/bridge/src/storage_proof.rs +++ b/srml/bridge/src/storage_proof.rs @@ -17,11 +17,12 @@ //! Logic for checking Substrate storage proofs. use hash_db::{Hasher, HashDB, EMPTY_PREFIX}; -use state_machine::StorageProof; use trie::{MemoryDB, Trie, trie_types::TrieDB}; use crate::Error; +pub(crate) type StorageProof = Vec>; + /// This struct is used to read storage values from a subset of a Merklized database. The "proof" /// is a subset of the nodes in the Merkle structure of the database, so that it provides /// authentication against a known Merkle root as well as the values in the database themselves. @@ -40,7 +41,7 @@ impl StorageProofChecker /// This returns an error if the given proof is invalid with respect to the given root. pub fn new(root: H::Out, proof: StorageProof) -> Result { let mut db = MemoryDB::default(); - for item in proof.iter_nodes() { + for item in proof { db.insert(EMPTY_PREFIX, &item); } let checker = StorageProofChecker { @@ -73,7 +74,6 @@ mod tests { use primitives::{Blake2Hasher, H256}; use state_machine::{prove_read, backend::{Backend, InMemory}}; - // use trie::{PrefixedMemoryDB, TrieDBMut}; #[test] fn storage_proof_check() { @@ -86,7 +86,10 @@ mod tests { (None, b"key11".to_vec(), Some(vec![0u8; 32])), ]); let root = backend.storage_root(std::iter::empty()).0; - let proof = prove_read(backend, &[&b"key1"[..], &b"key2"[..], &b"key22"[..]]).unwrap(); + let proof: StorageProof = prove_read(backend, &[&b"key1"[..], &b"key2"[..], &b"key22"[..]]) + .unwrap() + .iter_nodes() + .collect(); // check proof in runtime let checker = >::new(root, proof.clone()).unwrap(); From 0012fc36f6ccf00832dda055d9c3ffb7f26ba654 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 9 Dec 2019 03:41:40 +0100 Subject: [PATCH 31/34] Fix some review comments --- srml/bridge/src/justification.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index 3ca205cd3e279..ad4fff2913fd4 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -110,7 +110,6 @@ impl> GrandpaJustification { ) -> Result, ClientError> where NumberFor: grandpa::BlockNumberOps, { - let justification = GrandpaJustification::::decode(&mut &*encoded) .map_err(|_| ClientError::JustificationDecode)?; @@ -197,7 +196,7 @@ impl> GrandpaJustification { // Since keys in a `BTreeMap` need to implement `Ord` we can't use Block::Hash directly. // Instead we'll use a wrapper which implements `Ord` by leveraging the fact that // `Block::Hash` implements `AsRef`, which itself implements `Ord` -#[derive(Eq)] +#[derive(Eq, PartialEq)] struct BlockHashKey(Block::Hash); impl BlockHashKey { @@ -218,12 +217,6 @@ impl PartialOrd for BlockHashKey { } } -impl PartialEq for BlockHashKey { - fn eq(&self, other: &Self) -> bool { - self.0.as_ref() == other.0.as_ref() - } -} - /// A utility trait implementing `grandpa::Chain` using a given set of headers. /// This is useful when validating commits, using the given set of headers to /// verify a valid ancestry route to the target commit block. @@ -271,6 +264,12 @@ impl grandpa::Chain> for AncestryCh } } +#[cfg(not(test))] +fn localized_payload(round: RoundNumber, set_id: SetIdNumber, message: &E) -> Vec { + (message, round, set_id).encode() +} + +#[cfg(test)] pub(crate) fn localized_payload(round: RoundNumber, set_id: SetIdNumber, message: &E) -> Vec { (message, round, set_id).encode() } From 6e4fd3e135e1544ade3c05c61be7c964c2b31ce7 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 9 Dec 2019 21:49:18 +0100 Subject: [PATCH 32/34] Remove dependency on `client` --- Cargo.lock | 1 - srml/bridge/Cargo.toml | 1 - srml/bridge/src/justification.rs | 52 ++++++++++++++++++-------------- srml/bridge/src/lib.rs | 10 +++--- 4 files changed, 33 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 24c458e18d885..871b28e5a5761 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4486,7 +4486,6 @@ dependencies = [ "srml-session 2.0.0", "srml-support 2.0.0", "srml-system 2.0.0", - "substrate-client 2.0.0", "substrate-finality-grandpa 2.0.0", "substrate-finality-grandpa-primitives 2.0.0", "substrate-keyring 2.0.0", diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index 8b73ab519707e..d371949f4309c 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -8,7 +8,6 @@ edition = "2018" [dependencies] codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false } -client = { package = "substrate-client", path = "../../core/client" } fg = { package = "substrate-finality-grandpa", path = "../../core/finality-grandpa/" } fg_primitives = { package = "substrate-finality-grandpa-primitives", path ="../../core/finality-grandpa/primitives", default-features = false } grandpa = { package = "finality-grandpa", version = "0.9.1", default-features = false, features = ["derive-codec"] } diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index ad4fff2913fd4..fb5eef348b95b 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -18,12 +18,9 @@ //! can be thought of as a finality proof. GRANDPA justifications consist //! of a commit message plus an ancestry proof for pre-commits. -use client::{CallExecutor, Client}; -use client::backend::Backend; -use client::error::Error as ClientError; use codec::{Encode, Decode}; use core::cmp::{Ord, Ordering}; -use fg::{Commit, Error, Message}; +use fg::{Commit, Message}; // TODO: I can't import this stuff use fg_primitives::{AuthorityId, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; use grandpa::voter_set::VoterSet; use grandpa::{Error as GrandpaError}; @@ -35,9 +32,17 @@ use rstd::collections::{ }; use sr_primitives::app_crypto::RuntimeAppPublic; -use sr_primitives::generic::BlockId; use sr_primitives::traits::{NumberFor, Block as BlockT, Header as HeaderT}; -use primitives::{H256, Blake2Hasher}; +use primitives::{H256}; + +#[cfg(test)] +use sr_primitives::generic::BlockId; +#[cfg(test)] +use primitives::Blake2Hasher; +#[cfg(test)] +use client::{CallExecutor, Client}; +#[cfg(test)] +use client::backend::Backend; /// A GRANDPA justification for block finality, it includes a commit message and /// an ancestry proof including all headers routing all precommit target blocks @@ -57,11 +62,12 @@ pub struct GrandpaJustification { impl> GrandpaJustification { /// Create a GRANDPA justification from the given commit. This method /// assumes the commit is valid and well-formed. + #[cfg(test)] pub(crate) fn from_commit( client: &Client, round: u64, commit: Commit, - ) -> Result, Error> where + ) -> Result, JustificationError> where B: Backend, E: CallExecutor + Send + Sync, RA: Send + Sync, @@ -70,8 +76,7 @@ impl> GrandpaJustification { let mut votes_ancestries = Vec::new(); let error = || { - let msg = "invalid precommits for target commit".to_string(); - Err(Error::Client(ClientError::BadJustification(msg))) + Err(JustificationError::BadJustification) }; for signed in commit.precommits.iter() { @@ -107,22 +112,22 @@ impl> GrandpaJustification { finalized_target: (Block::Hash, NumberFor), set_id: u64, voters: &VoterSet, - ) -> Result, ClientError> where + ) -> Result, JustificationError> where NumberFor: grandpa::BlockNumberOps, { let justification = GrandpaJustification::::decode(&mut &*encoded) - .map_err(|_| ClientError::JustificationDecode)?; + .map_err(|_| JustificationError::JustificationDecode)?; if (justification.commit.target_hash, justification.commit.target_number) != finalized_target { - let msg = "invalid commit target in grandpa justification".to_string(); - Err(ClientError::BadJustification(msg)) + // let msg = "invalid commit target in grandpa justification".to_string(); + Err(JustificationError::BadJustification) } else { justification.verify(set_id, voters).map(|_| justification) } } /// Validate the commit and the votes' ancestry proofs. - pub(crate) fn verify(&self, set_id: u64, voters: &VoterSet) -> Result<(), ClientError> + pub(crate) fn verify(&self, set_id: u64, voters: &VoterSet) -> Result<(), JustificationError> where NumberFor: grandpa::BlockNumberOps, { @@ -137,8 +142,7 @@ impl> GrandpaJustification { ) { Ok(ref result) if result.ghost().is_some() => {}, _ => { - let msg = "invalid commit in grandpa justification".to_string(); - return Err(ClientError::BadJustification(msg)); + return Err(JustificationError::BadJustification); } } @@ -151,8 +155,7 @@ impl> GrandpaJustification { self.round, set_id, ) { - return Err(ClientError::BadJustification( - "invalid signature for precommit in grandpa justification".to_string()).into()); + return Err(JustificationError::BadJustification) } if self.commit.target_hash == signed.precommit.target_hash { @@ -168,8 +171,7 @@ impl> GrandpaJustification { } }, _ => { - return Err(ClientError::BadJustification( - "invalid precommit ancestry proof in grandpa justification".to_string()).into()); + return Err(JustificationError::BadJustification) }, } } @@ -180,15 +182,14 @@ impl> GrandpaJustification { .collect(); if visited_hashes != ancestry_hashes { - return Err(ClientError::BadJustification( - "invalid precommit ancestries in grandpa justification with unused headers".to_string()).into()); + return Err(JustificationError::BadJustification) } Ok(()) } /// Get the current commit message from the GRANDPA justification - pub(crate) fn get_commit(&self) -> Commit { + pub(crate) fn _get_commit(&self) -> Commit { self.commit.clone() } } @@ -293,3 +294,8 @@ fn check_message_sig( Err(()) } } + +pub(crate) enum JustificationError { + BadJustification, + JustificationDecode, +} diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index c404509b59237..8711cde14f391 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -37,12 +37,11 @@ mod storage_proof; mod justification; -use crate::justification::GrandpaJustification; +use crate::justification::{GrandpaJustification, JustificationError}; use crate::storage_proof::{StorageProof, StorageProofChecker}; use core::iter::FromIterator; use codec::{Encode, Decode}; -use client::error::Error as ClientError; // Not sure if this is allowed to be brought in... use fg_primitives::{AuthorityId, AuthorityWeight, AuthorityList, SetId}; use grandpa::voter_set::VoterSet; use primitives::H256; @@ -181,13 +180,12 @@ decl_error! { } } -impl From for Error { - fn from(e: ClientError) -> Self { +impl From for Error { + fn from(e: JustificationError) -> Self { match e { - ClientError::BadJustification(_) | ClientError::JustificationDecode => { + JustificationError::BadJustification | JustificationError::JustificationDecode => { Error::InvalidFinalityProof }, - _ => Error::UnknownClientError, } } } From 7cbed95c05851542826cdf673d7f635215561dd1 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 10 Dec 2019 10:37:07 -0800 Subject: [PATCH 33/34] Unbreak the tests that depended on `client` --- Cargo.lock | 1 + srml/bridge/Cargo.toml | 1 + srml/bridge/src/justification.rs | 15 ++++++++++++++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 871b28e5a5761..24c458e18d885 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4486,6 +4486,7 @@ dependencies = [ "srml-session 2.0.0", "srml-support 2.0.0", "srml-system 2.0.0", + "substrate-client 2.0.0", "substrate-finality-grandpa 2.0.0", "substrate-finality-grandpa-primitives 2.0.0", "substrate-keyring 2.0.0", diff --git a/srml/bridge/Cargo.toml b/srml/bridge/Cargo.toml index d371949f4309c..c623144104a7e 100644 --- a/srml/bridge/Cargo.toml +++ b/srml/bridge/Cargo.toml @@ -24,6 +24,7 @@ system = { package = "srml-system", path = "../system", default-features = false trie = { package = "substrate-trie", path = "../../core/trie", default-features = false } [dev-dependencies] +client = { package = "substrate-client", path = "../../core/client" } keyring = { package = "substrate-keyring", path = "../../core/keyring" } state-machine = { package = "substrate-state-machine", path = "../../core/state-machine" } test-client = { package = "substrate-test-runtime-client", path = "../../core/test-runtime/client" } diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index fb5eef348b95b..fb7230931b932 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -43,6 +43,8 @@ use primitives::Blake2Hasher; use client::{CallExecutor, Client}; #[cfg(test)] use client::backend::Backend; +#[cfg(test)] +use client::error::Error as ClientError; /// A GRANDPA justification for block finality, it includes a commit message and /// an ancestry proof including all headers routing all precommit target blocks @@ -104,7 +106,6 @@ impl> GrandpaJustification { Ok(GrandpaJustification { round, commit, votes_ancestries }) } - /// Decode a GRANDPA justification and validate the commit and the votes' /// ancestry proofs finalize the given block. pub(crate) fn decode_and_verify_finalizes( @@ -295,7 +296,19 @@ fn check_message_sig( } } +#[cfg_attr(test, derive(Debug))] pub(crate) enum JustificationError { BadJustification, JustificationDecode, } + +#[cfg(test)] +impl From for JustificationError { + fn from(e: ClientError) -> Self { + match e { + ClientError::BadJustification(_) => JustificationError::BadJustification, + ClientError::JustificationDecode => JustificationError::JustificationDecode, + _ => unreachable!(), + } + } +} From 975e2a66af769080c7521efdf616e6b12220f524 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 10 Dec 2019 10:59:06 -0800 Subject: [PATCH 34/34] Add TODO for removing usage of `core/finality-grandpa` --- srml/bridge/src/justification.rs | 4 +++- srml/bridge/src/lib.rs | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/srml/bridge/src/justification.rs b/srml/bridge/src/justification.rs index fb7230931b932..5c952ccc10b5e 100644 --- a/srml/bridge/src/justification.rs +++ b/srml/bridge/src/justification.rs @@ -20,7 +20,9 @@ use codec::{Encode, Decode}; use core::cmp::{Ord, Ordering}; -use fg::{Commit, Message}; // TODO: I can't import this stuff +// TODO: Since I can't use types from `core/finality-grandpa`, wait until #3868 +// is merged as that'll move these into `primitives/finality-grandpa`. +use fg::{Commit, Message}; use fg_primitives::{AuthorityId, RoundNumber, SetId as SetIdNumber, AuthoritySignature}; use grandpa::voter_set::VoterSet; use grandpa::{Error as GrandpaError}; diff --git a/srml/bridge/src/lib.rs b/srml/bridge/src/lib.rs index 8711cde14f391..5baecef6f2219 100644 --- a/srml/bridge/src/lib.rs +++ b/srml/bridge/src/lib.rs @@ -529,7 +529,6 @@ mod tests { ); } - // Currently stealing this from `core/finality-grandpa/src/test.rs` fn create_grandpa_justification( authority: Ed25519Keyring,