Generic authorship and uncle inclusion module #2941

rphmeier · 2019-06-25T10:21:42Z

This module aims to make an author available to runtime modules in order to query. It does so in a way which intends to be used with the inherent digest abstraction: consensus engines will implement a FindAuthor trait for extracting an author ID from a block header. This will probably have to coupled with a special FindAuthor wrapper for Session, which converts from validator indices to account IDs (#2949)

It also manages a set of uncles (from up to configurable number of blocks back) and handles import of them. Generation of the Authorship::set_uncles inherent is not currently done in this PR. Extra care has to be taken about importing uncles in a PoS scenario because of the possibility of an unbounded number of equivocations. This PR includes an uncle inclusion filter for mitigating that.

TODO:

SessionFindAuthor wrapper
scrape author
Tests

rphmeier · 2019-06-25T16:50:40Z

@demimarie-parity Opinions on whether this is using inherent digests as you intended as well as compatibility with #2929 would be appreciated along with general review.

@dvc94ch Thoughts on #2949 w.r.t. your other session refactorings?

Demi-Marie

This looks very good. I have not reviewed the tests yet, but the only suggestions are trivial.

srml/authorship/src/lib.rs

Demi-Marie · 2019-06-25T19:45:46Z

srml/authorship/src/lib.rs

+/// A filter on uncles which verifies seals and does no additional checks.
+/// This is well-suited to consensus modes such as PoW where the cost of
+/// equivocating is high.
+pub struct SealVerify<T>(rstd::marker::PhantomData<T>);


Suggested change

pub struct SealVerify<T>(rstd::marker::PhantomData<T>);

pub struct OnlySealVerify<T>(rstd::marker::PhantomData<T>);

I thought for a moment of naming it something like this, but every piece of code implicitly should only do what it says on the tin. So I think it's redundant to call it that

That is a good point. That said, it might be a good idea to include Only as a warning.

Demi-Marie · 2019-06-25T19:45:57Z

srml/authorship/src/lib.rs

+pub struct SealVerify<T>(rstd::marker::PhantomData<T>);
+
+impl<Header, Author, T: VerifySeal<Header, Author>> FilterUncle<Header, Author>
+	for SealVerify<T>


Suggested change

for SealVerify<T>

for OnlySealVerify<T>

srml/authorship/src/lib.rs

Demi-Marie · 2019-06-25T19:53:11Z

srml/authorship/src/lib.rs

+				return Err("uncle not recent enough to be included");
+			}
+
+			let duplicate = uncles.iter().find(|entry| match entry {


This is not a very efficient data structure. Can we use a better one?

The uncles list or a variant thereof is decent enough for tracking garbage to collect. Maybe not ideal for scanning against duplicates. We could shift the actual data into a map of Hash -> bool

But then the main question is (given that in the code here we have the whole uncles list in contiguous memory) how much slower/faster the trie access to query the map is going to be vs. doing this iteration (for in practice, what is typically less than 50 elements and probably less than 20).

We could also construct a BTreeSet<Hash> or something from the uncles list and then query that. It might be well faster than doing the few iterations. On the other hand, maybe not.

If we need to load/store this every time, then an array is definitely faster. On the other hand, if this is loaded once and then accessed many times, I would go with a BTreeSet.

This also requires T::Hash: Ord, which isn't currently a bound on system::Trait. Let's consider this future optimization.

Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com>

rphmeier · 2019-06-25T20:27:04Z

Pushed a commit to cover an index-based variant of #2949 as well. The idea is that BABE/Aura/other consensus tells us the index of the author in the canonical list of validators.

dvc94ch · 2019-06-26T09:02:26Z

srml/authorship/src/lib.rs

+	fn note_uncle(author: Author, age: BlockNumber);
+}
+
+impl<A, B> EventHandler<A, B> for () {


Why not use for_each_tuple! like for example the session module does?

* generalized uncle processing * add some uncle tests * set author and do event handling * OnePerAuthorPerHeight no longer O(n^2) and test * bump impl_version of node * Documentation and style fixes Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> * fix paritytech#2949: index-based FindAuthor wrapper for srml-session * use for_each_tuple

generalized uncle processing

d91027c

rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 25, 2019

devops-parity added the A4-gotissues label Jun 25, 2019

gavofyork force-pushed the master branch from 96e17dc to 1c2eaa3 Compare June 25, 2019 11:29

add some uncle tests

050e3be

gavofyork mentioned this pull request Jun 25, 2019

Initial mechanics for 80:20 fee split #2912

Merged

1 task

set author and do event handling

010e739

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. A4-gotissues labels Jun 25, 2019

devops-parity added the A4-gotissues label Jun 25, 2019

OnePerAuthorPerHeight no longer O(n^2) and test

b030b7c

rphmeier mentioned this pull request Jun 25, 2019

FindAuthor wrapper for Session #2949

Closed

rphmeier added 2 commits June 25, 2019 18:06

bump impl_version of node

0aff8a4

Merge branch 'master' into rh-authorship-module

ea830cf

rphmeier removed the A4-gotissues label Jun 25, 2019

rphmeier requested review from Demi-Marie and dvc94ch June 25, 2019 16:49

devops-parity added the B2-breaksapi label Jun 25, 2019

Demi-Marie approved these changes Jun 25, 2019

View reviewed changes

Demi-Marie added the A6-mustntgrumble label Jun 25, 2019

rphmeier and others added 2 commits June 25, 2019 22:18

Documentation and style fixes

de96df0

Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com>

fix #2949: index-based FindAuthor wrapper for srml-session

c98b599

rphmeier mentioned this pull request Jun 26, 2019

Actually construct the Authorship::set_uncles inherent #2956

Closed

dvc94ch approved these changes Jun 26, 2019

View reviewed changes

use for_each_tuple

0aff824

rphmeier removed A0-please_review Pull request needs code review. A6-mustntgrumble labels Jun 26, 2019

rphmeier added the A8-looksgood label Jun 26, 2019

rphmeier merged commit c4ea8fb into master Jun 26, 2019

rphmeier deleted the rh-authorship-module branch June 26, 2019 10:25

rphmeier mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic authorship and uncle inclusion module #2941

Generic authorship and uncle inclusion module #2941

rphmeier commented Jun 25, 2019 •

edited

Loading

rphmeier commented Jun 25, 2019 •

edited

Loading

Demi-Marie left a comment

Demi-Marie Jun 25, 2019

rphmeier Jun 25, 2019 •

edited

Loading

Demi-Marie Jun 26, 2019

Demi-Marie Jun 25, 2019

Demi-Marie Jun 25, 2019

rphmeier Jun 25, 2019 •

edited

Loading

rphmeier Jun 25, 2019 •

edited

Loading

Demi-Marie Jun 26, 2019

rphmeier Jun 26, 2019

rphmeier commented Jun 25, 2019

dvc94ch Jun 26, 2019

	pub struct SealVerify<T>(rstd::marker::PhantomData<T>);
	pub struct OnlySealVerify<T>(rstd::marker::PhantomData<T>);

Generic authorship and uncle inclusion module #2941

Generic authorship and uncle inclusion module #2941

Conversation

rphmeier commented Jun 25, 2019 • edited Loading

rphmeier commented Jun 25, 2019 • edited Loading

Demi-Marie left a comment

Choose a reason for hiding this comment

Demi-Marie Jun 25, 2019

Choose a reason for hiding this comment

rphmeier Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Demi-Marie Jun 26, 2019

Choose a reason for hiding this comment

Demi-Marie Jun 25, 2019

Choose a reason for hiding this comment

Demi-Marie Jun 25, 2019

Choose a reason for hiding this comment

rphmeier Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

rphmeier Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Demi-Marie Jun 26, 2019

Choose a reason for hiding this comment

rphmeier Jun 26, 2019

Choose a reason for hiding this comment

rphmeier commented Jun 25, 2019

dvc94ch Jun 26, 2019

Choose a reason for hiding this comment

rphmeier commented Jun 25, 2019 •

edited

Loading

rphmeier commented Jun 25, 2019 •

edited

Loading

rphmeier Jun 25, 2019 •

edited

Loading

rphmeier Jun 25, 2019 •

edited

Loading

rphmeier Jun 25, 2019 •

edited

Loading