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 1 commit
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
Prev Previous commit
Next Next commit
Remove dependency on client
  • Loading branch information
HCastano committed Dec 9, 2019
commit 6e4fd3e135e1544ade3c05c61be7c964c2b31ce7
1 change: 0 additions & 1 deletion Cargo.lock

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

1 change: 0 additions & 1 deletion srml/bridge/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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/" }
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", default-features = false }
grandpa = { package = "finality-grandpa", version = "0.9.1", default-features = false, features = ["derive-codec"] }
Expand Down
52 changes: 29 additions & 23 deletions srml/bridge/src/justification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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)]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on with all this stuff? Just move all of it into a tests (sub-)module.

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
Expand All @@ -57,11 +62,12 @@ pub struct GrandpaJustification<Block: BlockT> {
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.
#[cfg(test)]
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
) -> Result<GrandpaJustification<Block>, JustificationError> where
B: Backend<Block, Blake2Hasher>,
E: CallExecutor<Block, Blake2Hasher> + Send + Sync,
RA: Send + Sync,
Expand All @@ -70,8 +76,7 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
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)))
Err(JustificationError::BadJustification)
};

for signed in commit.precommits.iter() {
Expand Down Expand Up @@ -107,22 +112,22 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
finalized_target: (Block::Hash, NumberFor<Block>),
set_id: u64,
voters: &VoterSet<AuthorityId>,
) -> Result<GrandpaJustification<Block>, ClientError> where
) -> Result<GrandpaJustification<Block>, JustificationError> where
NumberFor<Block>: grandpa::BlockNumberOps,
{
let justification = GrandpaJustification::<Block>::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)
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>
pub(crate) fn verify(&self, set_id: u64, voters: &VoterSet<AuthorityId>) -> Result<(), JustificationError>
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

where
NumberFor<Block>: grandpa::BlockNumberOps,
{
Expand All @@ -137,8 +142,7 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
) {
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);
}
}

Expand All @@ -151,8 +155,7 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
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 {
Expand All @@ -168,8 +171,7 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
}
},
_ => {
return Err(ClientError::BadJustification(
"invalid precommit ancestry proof in grandpa justification".to_string()).into());
return Err(JustificationError::BadJustification)
},
}
}
Expand All @@ -180,15 +182,14 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
.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<Block> {
pub(crate) fn _get_commit(&self) -> Commit<Block> {
self.commit.clone()
}
}
Expand Down Expand Up @@ -293,3 +294,8 @@ fn check_message_sig<Block: BlockT>(
Err(())
}
}

pub(crate) enum JustificationError {
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 get rid of this. Just use the normal error type. Any function that returns Result<(), JustificationError> where the error is always BadJustification (like from_commit and verify) can return an error type of ().

BadJustification,
JustificationDecode,
}
10 changes: 4 additions & 6 deletions srml/bridge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -181,13 +180,12 @@ decl_error! {
}
}

impl From<ClientError> for Error {
fn from(e: ClientError) -> Self {
impl From<JustificationError> for Error {
fn from(e: JustificationError) -> Self {
match e {
ClientError::BadJustification(_) | ClientError::JustificationDecode => {
JustificationError::BadJustification | JustificationError::JustificationDecode => {
Error::InvalidFinalityProof
},
_ => Error::UnknownClientError,
}
}
}
Expand Down