Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
598d8c9
Create `bridge` module skeleton
HCastano Sep 25, 2019
e3d3644
Add more skeleton code
HCastano Sep 26, 2019
ce3385c
Clean up some warnings
HCastano Sep 27, 2019
b4daa86
Get the mock runtime for tests set up
HCastano Oct 1, 2019
9f27106
Add BridgeId => Bridge mapping
HCastano Oct 1, 2019
e623cb8
Allow tracking of multiple bridges
HCastano Oct 4, 2019
74c63d0
Logic for checking Substrate proofs from within runtime module. (#3783)
jimpo Oct 10, 2019
73fa3cb
Make tests work after the changes introduced in #3793 (#3874)
HCastano Oct 23, 2019
4836977
Check given Grandpa validator set against set found in storage (#3915)
HCastano Nov 1, 2019
49ae74b
Verify Ancestry between Headers (#3963)
HCastano Nov 4, 2019
50c6a2f
Use new StorageProof type from #3834
HCastano Nov 11, 2019
19295c5
Store block headers instead of individual parts of header
HCastano Nov 14, 2019
ca20bb3
Steal `justification.rs` from `grandpa-finality` crate
HCastano Nov 15, 2019
97e36f4
WIP: Make `justification.rs` no_std compatable
HCastano Nov 17, 2019
8e3e8ed
Swap HashMap for BTreeMap
HCastano Nov 19, 2019
efdaed8
Verify Grandpa signatures in `no_std`
HCastano Nov 19, 2019
635c898
Create a wrapper type for Block::Hash
HCastano Nov 20, 2019
f310f66
Clean up comments and imports a bit
HCastano Nov 21, 2019
fc3712f
Bump `finality-grandpa` from v0.9.0 to v0.9.1
HCastano Nov 22, 2019
16fcd41
Address some review comments
HCastano Nov 26, 2019
5ac5065
WIP: Verify justifications from module interface
HCastano Nov 26, 2019
bb01336
Fix compilation issues.
jimpo Nov 26, 2019
0b11a29
Make old tests compile again
HCastano Nov 28, 2019
0ddbc40
WIP: Add test for creating justifications
HCastano Dec 2, 2019
3a6bc0d
Add a test for verifying and updating new headers
HCastano Dec 3, 2019
be98ec8
Add test for checking that commits were signed by correct authorities
HCastano Dec 4, 2019
6b3ec92
Use a non-hardcoded authority set id
HCastano Dec 4, 2019
cc49207
Handle ClientErrors in a nicer way
HCastano Dec 4, 2019
ee7ef25
Turn off `std` feature for some imports
HCastano Dec 9, 2019
91935c4
Get rid of `state-machine` dependency
HCastano Dec 9, 2019
0012fc3
Fix some review comments
HCastano Dec 9, 2019
6e4fd3e
Remove dependency on `client`
HCastano Dec 9, 2019
7cbed95
Unbreak the tests that depended on `client`
HCastano Dec 10, 2019
975e2a6
Add TODO for removing usage of `core/finality-grandpa`
HCastano Dec 10, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ members = [
"srml/assets",
"srml/aura",
"srml/balances",
"srml/bridge",
"srml/contracts",
"srml/contracts/rpc",
"srml/collective",
Expand Down
40 changes: 40 additions & 0 deletions srml/bridge/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
[package]
name = "srml-bridge"
version = "0.1.0"
authors = ["Parity Technologies <[email protected]>"]
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 }
client = { package = "substrate-client", path = "../../core/client" }
fg = { package = "substrate-finality-grandpa", path = "../../core/finality-grandpa/" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't import this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna wait on #3868 before getting rid of it

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 }
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 }
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]

[features]
default = ["std"]
std = [
"serde",
"codec/std",
"session/std",
"sr-primitives/std",
"support/std",
"system/std",
"trie/std",
"runtime-io/std",
]
288 changes: 288 additions & 0 deletions srml/bridge/src/justification.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,288 @@
// 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 <http://www.gnu.org/licenses/>.

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};

// Might be able to get this from primitives re-export
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 primitives::{H256, Blake2Hasher};

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the comment seems wrong (stored in the db and networked).

/// nodes, and are used by syncing nodes to prove authority set handoffs.
#[derive(Encode, Decode)]
pub struct GrandpaJustification<Block: BlockT> {
Copy link
Contributor

@rphmeier rphmeier Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the contents of this module should be in the palette/grandpa crate, since it's generally useful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it should go into primitives/finality-grandpa and then it gets used by both client and pallet code (and also by the bridge pallet).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to wait until I rebase past the-big-reorg before moving the code into primitives/finality-grandpa.

round: u64,
pub(crate) commit: Commit<Block>,
votes_ancestries: Vec<Block::Header>,
}

impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
/// Create a GRANDPA justification from the given commit. This method
/// assumes the commit is valid and well-formed.
pub(crate) fn from_commit<B, E, RA>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Just make it pub, not pub(crate). The module is private.

client: &Client<B, E, Block, RA>,
round: u64,
commit: Commit<Block>,
) -> Result<GrandpaJustification<Block>, Error> where
B: Backend<Block, Blake2Hasher>,
E: CallExecutor<Block, Blake2Hasher> + Send + Sync,
RA: Send + Sync,
{
let mut votes_ancestries_hashes = BTreeSet::new();
let mut votes_ancestries = Vec::new();

let error = || {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just inline this.

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pub

encoded: &[u8],
finalized_target: (Block::Hash, NumberFor<Block>),
set_id: u64,
voters: &VoterSet<AuthorityId>,
) -> Result<GrandpaJustification<Block>, ClientError> where
NumberFor<Block>: grandpa::BlockNumberOps,
{

let justification = GrandpaJustification::<Block>::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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return the justification? It's just gonna get dropped anyway by the lone caller.

}
}

/// Validate the commit and the votes' ancestry proofs.
pub(crate) fn verify(&self, set_id: u64, voters: &VoterSet<AuthorityId>) -> Result<(), ClientError>
where
NumberFor<Block>: grandpa::BlockNumberOps,
{
use grandpa::Chain;

let ancestry_chain = AncestryChain::<Block>::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 = BTreeSet::new();
for signed in self.commit.precommits.iter() {
if let Err(_) = check_message_sig::<Block>(
&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(())
}
}

// 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<u8>`, which itself implements `Ord`
#[derive(Eq)]
struct BlockHashKey<Block: BlockT>(Block::Hash);

impl<Block: BlockT> BlockHashKey<Block> {
fn new(hash: Block::Hash) -> Self {
Self(hash)
}
}

impl<Block: BlockT> Ord for BlockHashKey<Block> {
fn cmp(&self, other: &Self) -> Ordering {
self.0.as_ref().cmp(other.0.as_ref())
}
}

impl<Block: BlockT> PartialOrd for BlockHashKey<Block> {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.0.as_ref().cmp(other.0.as_ref()))
}
}

impl<Block: BlockT> PartialEq for BlockHashKey<Block> {
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.
struct AncestryChain<Block: BlockT> {
ancestry: BTreeMap<BlockHashKey<Block>, Block::Header>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps not necessary, but this could be a BTreeMap<BlockHashKey<Block>, &'a Block::Header> (map to header refs to avoid cloning).

}

impl<Block: BlockT> AncestryChain<Block> {
fn new(ancestry: &[Block::Header]) -> AncestryChain<Block> {
let ancestry: BTreeMap<_, _> = ancestry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this type hint shouldn't be necessary.

.iter()
.cloned()
.map(|h: Block::Header| (BlockHashKey::new(h.hash()), h))
.collect();

AncestryChain { ancestry }
}
}

impl<Block: BlockT> grandpa::Chain<Block::Hash, NumberFor<Block>> for AncestryChain<Block> where
NumberFor<Block>: grandpa::BlockNumberOps
{
fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result<Vec<Block::Hash>, GrandpaError> {
let mut route = Vec::new();
let mut current_hash = block;
loop {
if current_hash == base { break; }

let key = BlockHashKey::new(current_hash);
match self.ancestry.get(&key) {
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<Block>)> {
None
}
}

pub(crate) fn localized_payload<E: Encode>(round: RoundNumber, set_id: SetIdNumber, message: &E) -> Vec<u8> {
(message, round, set_id).encode()
}

// Check the signature of a Grandpa message.
// This was originally taken from `communication/mod.rs`
fn check_message_sig<Block: BlockT>(
message: &Message<Block>,
id: &AuthorityId,
signature: &AuthoritySignature,
round: RoundNumber,
set_id: SetIdNumber,
) -> 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(())
} else {
// debug!(target: "afg", "Bad signature on message from {:?}", id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to keep this for the client code that uses it once this is refactored to re-use the same code.

Err(())
}
}
Loading