Initial GRANDPA integration by rphmeier · Pull Request #866 · paritytech/substrate

rphmeier · 2018-10-02T14:54:47Z

This adds a wrapper (currently unused) for the finality gadget in the substrate-finality-grandpa crate, providing the necessary scaffolding to cast votes, integrate in the network, and finalize blocks. It currently hinges on a fixed list of validators, but a follow-up PR will include validator-set handoffs.

Most of the work is implementing the grandpa::voter::Environment trait, which does the work of setting up timers and actually finalizing blocks when GRANDPA says they should be.

This also exposes a bunch of test helpers from the substrate-network crate.

ltfschoen · 2018-10-04T06:19:28Z

core/finality-grandpa/src/lib.rs

+}
+
+impl<Block: BlockT, Status: BlockStatus<Block>, I> Stream for UntilImported<Block, Status, I>
+	where I: Stream<Item=SignedMessage<Block>,Error=Error>


(nitpick) would space after comma be appropriate?

i don't think so, it's not typical for these kinds of bounds

ltfschoen · 2018-10-04T06:27:47Z

core/finality-grandpa/src/lib.rs

+// the output stream checks signatures also.
+fn checked_message_stream<Block: BlockT, S>(inner: S, voters: Vec<AuthorityId>)
+	-> impl Stream<Item=SignedMessage<Block>,Error=Error> where
+	S: Stream<Item=Vec<u8>,Error=()>


(nitpick) space after comma on 300 and 301?

ltfschoen · 2018-10-04T06:33:19Z

core/finality-grandpa/src/lib.rs

+	network: N,
+) -> (
+	impl Stream<Item=SignedMessage<Block>,Error=Error>,
+	impl Sink<SinkItem=Message<Block>,SinkError=Error>,


(nitpick) space after comma on 337 and 338?

ltfschoen · 2018-10-04T06:46:37Z

core/finality-grandpa/src/lib.rs

+	client: Arc<Client<B, E, Block>>,
+	voters: HashMap<AuthorityId, usize>,
+	network: N,
+) -> Result<impl Future<Item=(),Error=()>,client::error::Error> where


(nitpick) spaces after commas

ltfschoen · 2018-10-04T06:53:34Z

core/finality-grandpa/src/lib.rs

+	}
+
+	impl Network for TestGrandpaNetwork {
+		type In = Box<Stream<Item=Vec<u8>,Error=()>>;


(nitpick) space after comma

pepyakin · 2018-10-03T12:40:29Z

core/network/src/consensus_gossip.rs

-	pub fn collect_garbage(&mut self, best_hash: Option<&B::Hash>) {
+	/// Prune old or no longer relevant consensus messages. Provide a predicate
+	/// for pruning, which returns `true` when the items with a given topic should be pruned.
+	pub fn collect_garbage<P: Fn(&B::Hash) -> bool>(&mut self, predicate: P) {


Ah damn, it's the opposite of the idiomatic practice for predicates to return true to keep an item in a list/set, like in filter, retain, etc...

pepyakin · 2018-10-04T09:34:50Z

core/finality-grandpa/src/lib.rs

+	N::In: 'static,
+	NumberFor<Block>: As<u32>,
+{
+    type Timer = Box<Future<Item = (), Error = Self::Error>>;


spaces here and below

pepyakin · 2018-10-04T09:35:00Z

core/finality-grandpa/src/lib.rs

+
+		voter::RoundData {
+			prevote_timer: Box::new(prevote_timer.map_err(Error::Timer)),
+    		precommit_timer: Box::new(precommit_timer.map_err(Error::Timer)),


spaces here and below

gnunicorn

Minor nitpicks and an interface naming question. Aside from that, LGTM.

gnunicorn · 2018-10-04T11:15:40Z

core/finality-grandpa/src/lib.rs

@@ -0,0 +1,767 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gnunicorn · 2018-10-04T11:32:52Z

core/finality-grandpa/src/lib.rs

+}
+
+/// Run a GRANDPA voter as a task. The returned future should be executed in a tokio runtime.
+pub fn run_voter<B, E, Block: BlockT, N>(


Isn't this the Voter we can/should run in order to just observe the GRANDPA-Process taking place without actually voting ourselves? I understand in that case I'd just provide a dummy Network, which isn't actually sending messages, but giving us information about potential misbehavior we could flag, but I feel like the function name and docs might already want to anticipate that, don't you think? Something more generic like run_grandpa instead (and maybe add a TODO to the docs to add example usage of both cases?). What do you think?

this is a voter that can send messages and cast votes. it can be a non-active voter. changing it to run_grandpa is uncontroversial to me

arkpar · 2018-10-04T11:49:22Z

core/finality-grandpa/src/lib.rs

+	/// Return `Ok(Some(number))` or `Ok(None)` depending on whether the block
+	/// is definitely known and has been imported.
+	/// If an unexpected error occurs, return that.
+	fn block_number(&self, hash: Block::Hash) -> Result<Option<u32>, Error>;


why u32 and not Block::Number?

the grandpa crate isn't generic over number and just uses u32

can we file an issue for that?

arkpar · 2018-10-04T11:56:53Z

core/finality-grandpa/src/lib.rs

+	inner: Fuse<I>,
+	ready: VecDeque<SignedMessage<Block>>,
+	check_pending: Interval,
+	pending: HashMap<Block::Hash, Vec<SignedMessage<Block>>>,


Can this be attacked to grow indefinitely?

potentially yes. i'm not sure what the mitigation strategy is there, other than to move the deferment of processing into the grandpa crate (i.e. catch equivocations before waiting for blocks to arrive)

i'll file that as an issue

arkpar · 2018-10-04T12:33:45Z

core/finality-grandpa/src/lib.rs

+		if let Err(e) = self.inner.backend()
+			.insert_aux(&[(LAST_COMPLETED_KEY, &encoded_state[..])], &[])
+		{
+			warn!(target: "afg", "Error bookkeeping last completed round in DB: {:?}", e);


Error here is quite possible. Out of disk for example. Is simple warning enough?

@rphmeier ^^^

…ech#866) * Allow disabling ink-alloc-related features for assets module * .

Bumps [thiserror](https://github.com/dtolnay/thiserror) from 1.0.39 to 1.0.40. - [Release notes](https://github.com/dtolnay/thiserror/releases) - [Commits](dtolnay/thiserror@1.0.39...1.0.40) --- updated-dependencies: - dependency-name: thiserror dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Wilson <james@jsdw.me>

rphmeier added 8 commits September 28, 2018 12:32

implement grandpa client

6aefbd1

consensus gossip with arbitrary topics

208bfe2

defer GRANDPA messages until referenced blocks imported

ccdee31

set up communication for voter in a transparent way

80f9511

instantiate GRANDPA voter

3ce3da9

Merge branch 'master' into rh-grandpa

d709fa6

keep last round state on disk

1add776

switch back to crates.io finality-grandpa

7006b4c

rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 2, 2018

update cargo.lock

ba34a8a

rphmeier requested review from arkpar and gnunicorn October 2, 2018 15:38

rphmeier added 4 commits October 2, 2018 12:05

use new collect_garbage API

6fe373b

update sync test framework and make public

38d25e8

test that observers can observe

934b7c0

fix warning

4118e2b

rphmeier 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 Oct 3, 2018

gavofyork approved these changes Oct 3, 2018

View reviewed changes

gavofyork added A6-seemsok and removed A0-please_review Pull request needs code review. labels Oct 3, 2018

ltfschoen reviewed Oct 4, 2018

View reviewed changes

pepyakin approved these changes Oct 4, 2018

View reviewed changes

gnunicorn approved these changes Oct 4, 2018

View reviewed changes

arkpar reviewed Oct 4, 2018

View reviewed changes

rphmeier added 3 commits October 4, 2018 12:41

use more idiomatic predicate for collecting garbage in gossip

cce4668

kill spaces

9a4db6e

fix date

690ffe2

gavofyork merged commit 9eeaae8 into master Oct 5, 2018

liuchengxu added a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021

Allow disabling ink-alloc-related features for assets module (parityt…

255de2d

…ech#866) * Allow disabling ink-alloc-related features for assets module * .

rphmeier mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#4

Merged

		@@ -0,0 +1,767 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

Conversation

rphmeier commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

gnunicorn left a comment

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!

rphmeier Oct 4, 2018 • 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!

rphmeier Oct 4, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rphmeier commented Oct 2, 2018 •

edited

Loading

rphmeier Oct 4, 2018 •

edited

Loading

rphmeier Oct 4, 2018 •

edited

Loading