Verify Grandpa proofs from within runtime by HCastano · Pull Request #4167 · paritytech/substrate

HCastano · 2019-11-21T10:02:57Z

I'm looking to get some feedback on this. For the bridge module, we need to be able to verify the validity of Grandpa proofs. The problem is however, that there is no way to do this from within the runtime as the current Grandpa proof code requires std. What I've done here is taken some of the code from the Substrate finality-grandpa crate and modified it to make it work in a no_std environment. The two main changes were:

Switch HashMap and HashSet to BTreeMap and BTreeSet
Move from using AuthorityPair for signature verification, to using AuthoritySignature + RuntimeAppPublic

Ideally this code wouldn't be duplicated in two places, but right now I'm just trying to make this work in the context of the bridge module.

Before merging this PR I'd like to have the submit_finalized_headers() function actually using the new code to verify Grandpa proofs.

…tytech#3783)

…tytech#3874) * Make tests work after the changes introduced in paritytech#3793 * Remove unneccessary import

…ytech#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

* 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

HCastano · 2019-11-21T10:17:11Z

As @svyatonik pointed out to me, not all the functions are needed by the bridge module (e.g from_commit(). While this is true, I think they'd be useful for creating unit tests.

srml/bridge/src/justification.rs

rphmeier · 2019-11-21T17:26:52Z

srml/bridge/src/justification.rs

+/// 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<Block: BlockT> {


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

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).

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

srml/bridge/src/justification.rs

andresilva

I think migrating to BTrees is sensible, we just did the same thing on the finality-grandpa crate.

andresilva · 2019-11-21T18:35:27Z

srml/bridge/src/justification.rs

+	if as_public.verify(&encoded_raw, signature) {
+		Ok(())
+	} else {
+		// debug!(target: "afg", "Bad signature on message from {:?}", id);


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

srml/bridge/src/justification.rs

srml/bridge/src/lib.rs

HCastano · 2019-12-04T14:05:58Z

There are a couple of things that I want to do in later PRs:

Move the justification code into finality-grandpa/primitives
Properly handle authority set changes

I think it's fine to get this PR in without these two things. Once this is merged I can then go ahead and rebase the whole bridge branch past the-big-reorg which will make my life easier when dealing with (1). As for (2), I need to do a bit more digging before handling it.

jimpo

Just did a very cursory review as you need to drop some disallowed dependencies which show up in a bunch of places.

srml/bridge/Cargo.toml

jimpo · 2019-12-04T15:39:38Z

srml/bridge/Cargo.toml

 [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/" }


You can't import this.

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

srml/bridge/Cargo.toml

srml/bridge/src/justification.rs

srml/bridge/src/lib.rs

HCastano · 2019-12-09T02:49:25Z

I think it's going to be a little annoying to get rid of the client and core/finality-grandpa dependencies. It looks like I'm going to have to move a few things into finality-grandpa/primitives, but it's not totally clear to me how just yet.

HCastano · 2019-12-10T19:02:01Z

I think some of the #[cfg(test)] stuff looks a little gross, but we should be able to get rid of it once we move the code into primitives/finality-grandpa

jimpo · 2019-12-11T15:34:55Z

srml/bridge/src/justification.rs

+
+use codec::{Encode, Decode};
+use core::cmp::{Ord, Ordering};
+// TODO: Since I can't use types from `core/finality-grandpa`, wait until #3868


Put this TODO in the Cargo.toml too, which is more important.

jimpo · 2019-12-11T15:37:23Z

srml/bridge/src/justification.rs

+/// 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


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

jimpo · 2019-12-11T15:38:09Z

srml/bridge/src/justification.rs

+	/// 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>(


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

jimpo · 2019-12-11T15:39:06Z

srml/bridge/src/justification.rs

+		let mut votes_ancestries_hashes = BTreeSet::new();
+		let mut votes_ancestries = Vec::new();
+
+		let error = || {


I would just inline this.

jimpo · 2019-12-11T15:44:49Z

srml/bridge/src/justification.rs

+
+	/// Decode a GRANDPA justification and validate the commit and the votes'
+	/// ancestry proofs finalize the given block.
+	pub(crate) fn decode_and_verify_finalizes(


jimpo · 2019-12-11T15:53:43Z

srml/bridge/src/justification.rs

+}
+
+#[cfg_attr(test, derive(Debug))]
+pub(crate) enum JustificationError {


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 ().

jimpo · 2019-12-11T15:56:06Z

srml/bridge/src/justification.rs

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


nit: this type hint shouldn't be necessary.

jimpo · 2019-12-11T15:58:15Z

srml/bridge/src/justification.rs

+/// 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>,


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

jimpo · 2019-12-11T16:10:51Z

srml/bridge/src/lib.rs

 		);
 	}
+
+	// Currently stealing this from `core/finality-grandpa/src/test.rs`


Do you expect it to be replaced? Otherwise, don't say "Currently".

jimpo · 2019-12-11T16:18:10Z

srml/bridge/src/storage_proof.rs


 use crate::Error;

+pub(crate) type StorageProof = Vec<Vec<u8>>;


HCastano · 2019-12-11T18:01:00Z

I talked with Jim about some of the options for moving this forward. Since there's a lot of code that came from core/finality-grandpa we could either

Merge this code as is and refactor it later
Put this on ice, and make a new PR once grandpa: report equivocations #3868 is merged

We think that the best way forward is (2) as it'll allow us to have a cleaner implementation that doesn't duplicate Grandpa code.

HCastano · 2020-02-20T02:54:17Z

Closing in favour of paritytech/parity-bridges-common#19.

HCastano and others added 18 commits November 11, 2019 21:51

Create bridge module skeleton

598d8c9

Add more skeleton code

e3d3644

Clean up some warnings

ce3385c

Get the mock runtime for tests set up

b4daa86

Add BridgeId => Bridge mapping

9f27106

Allow tracking of multiple bridges

e623cb8

Logic for checking Substrate proofs from within runtime module. (pari…

74c63d0

…tytech#3783)

Make tests work after the changes introduced in paritytech#3793 (pari…

73fa3cb

…tytech#3874) * Make tests work after the changes introduced in paritytech#3793 * Remove unneccessary import

Use new StorageProof type from paritytech#3834

50c6a2f

Store block headers instead of individual parts of header

19295c5

Steal justification.rs from grandpa-finality crate

ca20bb3

WIP: Make justification.rs no_std compatable

97e36f4

Swap HashMap for BTreeMap

8e3e8ed

Verify Grandpa signatures in no_std

efdaed8

Create a wrapper type for Block::Hash

635c898

Clean up comments and imports a bit

f310f66

HCastano added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 21, 2019

This comment has been minimized.

Sign in to view

rphmeier reviewed Nov 21, 2019

View reviewed changes

srml/bridge/src/justification.rs Outdated Show resolved Hide resolved

rphmeier reviewed Nov 21, 2019

View reviewed changes

srml/bridge/src/justification.rs Show resolved Hide resolved

rphmeier reviewed Nov 21, 2019

View reviewed changes

srml/bridge/src/justification.rs Outdated Show resolved Hide resolved

andresilva reviewed Nov 21, 2019

View reviewed changes

HCastano and others added 4 commits November 22, 2019 11:30

Bump finality-grandpa from v0.9.0 to v0.9.1

fc3712f

Address some review comments

16fcd41

WIP: Verify justifications from module interface

5ac5065

Fix compilation issues.

bb01336

niklasad1 reviewed Dec 2, 2019

View reviewed changes

srml/bridge/src/lib.rs Outdated Show resolved Hide resolved

HCastano added 5 commits December 2, 2019 14:53

WIP: Add test for creating justifications

0ddbc40

Add a test for verifying and updating new headers

3a6bc0d

Add test for checking that commits were signed by correct authorities

be98ec8

Use a non-hardcoded authority set id

6b3ec92

Handle ClientErrors in a nicer way

cc49207

HCastano marked this pull request as ready for review December 4, 2019 14:06

HCastano requested a review from Demi-Marie as a code owner December 4, 2019 14:06

HCastano added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Dec 4, 2019

HCastano requested a review from jimpo December 4, 2019 14:06

jimpo suggested changes Dec 4, 2019

View reviewed changes

HCastano added 3 commits December 9, 2019 02:32

Turn off std feature for some imports

ee7ef25

Get rid of state-machine dependency

91935c4

Fix some review comments

0012fc3

HCastano added 3 commits December 9, 2019 21:49

Remove dependency on client

6e4fd3e

Unbreak the tests that depended on client

7cbed95

Add TODO for removing usage of core/finality-grandpa

975e2a6

jimpo suggested changes Dec 11, 2019

View reviewed changes

HCastano added A1-onice and removed A0-please_review Pull request needs code review. labels Dec 11, 2019

HCastano force-pushed the hc-jp-bridge-module branch from 50c6a2f to 4f7397d Compare December 12, 2019 00:57

HCastano force-pushed the hc-jp-bridge-module branch from 296d8a4 to 2064106 Compare January 10, 2020 20:21

HCastano mentioned this pull request Feb 20, 2020

Verify Grandpa proofs from within runtime modules paritytech/parity-bridges-common#19

Closed

HCastano closed this Feb 20, 2020


		use crate::Error;

		pub(crate) type StorageProof = Vec<Vec<u8>>;

Conversation

HCastano commented Nov 21, 2019

Uh oh!

This comment has been minimized.

HCastano commented Nov 21, 2019

Uh oh!

Uh oh!

rphmeier Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HCastano commented Dec 4, 2019

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HCastano commented Dec 9, 2019

Uh oh!

HCastano commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HCastano commented Dec 11, 2019

Uh oh!

HCastano commented Feb 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rphmeier Nov 21, 2019 •

edited

Loading