Dynamic authority sets in GRANDPA by rphmeier · Pull Request #1014 · paritytech/substrate

rphmeier · 2018-10-25T16:06:20Z

Closes #875

The rough idea behind changing authority sets in GRANDPA is that at some point, we obtain agreement for some maximum block height that the current set can finalize, and once a block with that height is finalized the next set will pick up finalization from there.

Technically speaking, this would be implemented as a voting rule which says, "if there is a signal for a change in N blocks in block B, only vote on chains with length NUM(B) + N if they contain B". This conditional-inclusion logic is complex to compute because it requires looking arbitrarily far back in the chain.

Instead, we keep track of a list of all signals we've seen so far, sorted ascending by the block number they would be applied at. We never vote on chains with number higher than the earliest handoff block number (this is num(signal) + N). When finalizing a block, we either apply or prune any signaled changes based on whether the signaling block is included in the newly-finalized chain.

Implementation details:

This PR adds a BlockImport wrapper which queries the runtime to see if the digest contains any signals of set changes. All blocks imported must go through this wrapper for correctness.

When voting on blocks, the voters won't vote beyond the height of the lowest pending change.
When finalizing blocks, we check to see if any changes are meant to be applied. If the change is applied, we restart the voter with the new authority set by exiting from the current voter early with an ExitOrError enum. The voter is wrapped in a future::loop_fn which starts a new voter on intentional exit, using the new authority set.

Remaining work for this PR:

Favor a chain API where exposed WASM APIs are used to handle GRANDPA digests and grandpa authorities
Restart voter upon finalizing change-block
Write and load changes to DB
Test changes
publish GRANDPA 0.3.0 with any changes and use crates.io version

gavofyork · 2018-11-02T19:53:14Z

core/finality-grandpa/primitives/src/lib.rs

+	/// applied in the runtime after those N blocks have passed.
+	///
+	/// The consensus protocol will coordinate the handoff externally.
+	pub trait Api<B: BlockT> {


There's a convention to use a CAPS_CASE version of the ApiTraitName as an ApiId ("API identifier") that allows for extensible runtime versioning for each API:

pub mod id { use super::ApiId; pub const BLOCK_BUILDER: ApiId = *b"blkbuild"; pub const TAGGED_TRANSACTION_QUEUE: ApiId = *b"validatx"; pub const METADATA: ApiId = *b"metadata"; } decl_apis! { pub trait Core<Block: BlockT, AuthorityId> { ... } pub trait Metadata<Data> { ... } pub trait TaggedTransactionQueue<Block: BlockT> { ... } pub trait BlockBuilder<Block: BlockT> ExtraClientSide <OverlayedChanges> { ... } }

A terse name like Api makes this impractical or error prone. I would suggest Grandpa/GrandpaInterface or some other such name to prevent clashes and confusion.

Some documentation on the decl_apis! macro would be helpful; there is no way to know about this convention otherwise.

On the naming, I intended the name to be used like this:

extern crate grandpa_primitives as grandpa; fn uses_api<T: grandpa::Api>(T) { }

andresilva

Gave this a first look (along with grandpa and the rest of the substrate grandpa code) and left some minor comments.

andresilva · 2018-11-07T13:22:05Z

core/finality-grandpa/src/authorities.rs

+pub(crate) struct Status<H, N> {
+	/// Whether internal changes were made.
+	pub(crate) changed: bool,
+	/// `Some` when underlying authority set has changed, containign the


andresilva · 2018-11-07T13:32:32Z

core/finality-grandpa/src/authorities.rs

+				change.effective_number(),
+				change.canon_height.clone(),
+			))
+			.unwrap_or_else(|i| i);


Is it possible to signal multiple changes on the same block? Because in this case the last change to be signaled would take precedence.

no, only once per block

@rphmeier are we ensuring that anywhere or is that just by convention? I couldn't find anything (in this PR) that rejects a change at a certain block if a change is already scheduled for that block. Maybe we should add such a restriction to prevent undefined behavior? (e.g. we created a change block at 10+30 and then one at 20+20 - which one is supposed to happen at block 40?)

andresilva · 2018-11-07T15:20:28Z

core/finality-grandpa/src/lib.rs

+				Some((hash, header.number().clone()))
 			}
+			// Ok(None) can be returned when `block` is after `limit`. That might cause issues.
+			// might be better to return the header itself in this (rare) case.


When block is beyond limit can't we vote on the limit instead?

that's not a chain containing block, so it wouldn't be correct under the contract of the function

and this would also only happen if honest authorities are not voting according to this same rule

andresilva · 2018-11-07T15:36:14Z

core/finality-grandpa/src/authorities.rs

+/// Status of the set after changes were applied.
+pub(crate) struct Status<H, N> {
+	/// Whether internal changes were made.
+	pub(crate) changed: bool,


Can't we remove this and use the Option below to track whether changes ocurred? The only case where changed is true but new_set_block is None is when the block imported finalizes a change at a given height which is not part of the canon chain, the change is discarded but the authority set remains unchanged.

if internal changes are made (such as pruning a non-canonicalized pending transition), we have to write to disk for correctness.

andresilva · 2018-11-07T16:01:59Z

core/finality-grandpa/src/lib.rs

+	{
+		self.call_api_at(
+			&BlockId::hash(header.parent_hash().clone()),
+			::fg_primitives::PENDING_CHANGE_CALL,


fg_primitives::PENDING_CHANGE_CALL,

andresilva · 2018-11-07T17:12:33Z

Also, do we need to filter prevotes and precommits for blocks after the first pending change limit? Honest voters shouldn't be casting those votes/commits.

rphmeier · 2018-11-14T12:35:32Z

Also, do we need to filter prevotes and precommits for blocks after the first pending change limit? Honest voters shouldn't be casting those votes/commits.

We could do this filtering but I am assuming that since honest voters won't cast such votes that it is unnecessary.

bkchr · 2018-11-14T19:24:20Z

core/client/src/client.rs

 	E: CallExecutor<Block, Blake2Hasher> + Clone + Send + Sync,
 	Block: BlockT<Hash=H256>,
-	RA: CoreAPI<Block>
+	RA: Send + Sync,


Suggested change

RA: Send + Sync,

RA: CoreAPI<Block>,

This should not be changed!

why? everything compiles without?

Yeah it compiles without it, but it enforces that the CoreAPI is implemented. The CoreAPI is required to be implemented by each runtime. It internally also uses a function from the CoreAPI, the initialise_block function. This function is currently not called by the corresponding trait function, but putting the CoreAPI as an requirement, makes sure the function is present.

ok, i will re-introduce and add a comment to that effect.

bkchr · 2018-11-14T19:32:39Z

core/client/src/runtime_api/mod.rs

+
+	/// Call the given api function with strong arguments at the given block
+	/// and returns the decoded result.
+	fn call_api_at_strong<In: Encode, Out: Decode>(


This trait should just be used by the runtime api stuff and that does not require this function. Please remove it.

bkchr · 2018-11-14T19:36:41Z

core/finality-grandpa/primitives/src/lib.rs

+/// applied in the runtime after those N blocks have passed.
+///
+/// The consensus protocol will coordinate the handoff externally.
+pub trait GrandpaApi<B: BlockT> {


This declaration should be placed into the decl_runtime_apis! macro.
When using the macro, please rename B: BlockT to Block: BlockT. That is currently a restriction of the macro.

bkchr · 2018-11-14T19:37:45Z

core/finality-grandpa/src/lib.rs

+	B: Backend<Block, Blake2Hasher> + 'static,
+	E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
+	DigestFor<Block>: Encode,
+	RA: Send + Sync,


Suggested change

RA: Send + Sync,

RA: GrandpaApi<Block>,

bkchr · 2018-11-14T19:39:39Z

core/finality-grandpa/src/lib.rs

+	fn genesis_authorities(&self) -> Result<Vec<(AuthorityId, u64)>, ClientError> {
+		use runtime_primitives::traits::Zero;
+
+		self.call_api_at_strong(


This should be changed to self.runtime_api().grandpa_authorities(&BlockId::Number(NumberFor::<Block>::zero()).

bkchr · 2018-11-14T19:39:56Z

core/finality-grandpa/src/lib.rs

+	fn scheduled_change(&self, header: &Block::Header)
+		-> Result<Option<ScheduledChange<NumberFor<Block>>>, ClientError>
+	{
+		self.call_api_at_strong(


This should be changed as above.

andresilva

LGTM

andresilva · 2018-11-15T01:35:38Z

Merge conflict needs resolving.

gnunicorn

just some minor wording things.

gnunicorn · 2018-11-15T09:21:26Z

core/finality-grandpa/primitives/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "substrate-fg-primitives"


Can we please use substrate-finality-grandpa-primitives as the crate name? I know it is verbose, but fg isn't really a commonly used shorthand (other than sr- for e.g.) and it makes it harder to understand what the crate is about when included somewhere (extern crate substrate_finality_grandpa_primitves is a lot clearer than extern crate substrate_fg_primitives) . Also matching path and crate name is a helpful convention we use everywhere else...

gnunicorn · 2018-11-15T09:31:13Z

core/finality-grandpa/primitives/src/lib.rs

+	use client::runtime_api::ApiId;
+
+	/// ApiId for the GrandpaApi trait.
+	pub const GRANDPA_API: ApiId = *b"fgrandpa";


any particular reason this has a prefix f the other defined names don't have?

to make it 8 characters: "finality-grandpa"

gnunicorn · 2018-11-15T09:41:24Z

core/finality-grandpa/src/authorities.rs

+				change.effective_number(),
+				change.canon_height.clone(),
+			))
+			.unwrap_or_else(|i| i);


@rphmeier are we ensuring that anywhere or is that just by convention? I couldn't find anything (in this PR) that rejects a change at a certain block if a change is already scheduled for that block. Maybe we should add such a restriction to prevent undefined behavior? (e.g. we created a change block at 10+30 and then one at 20+20 - which one is supposed to happen at block 40?)

gnunicorn · 2018-11-15T09:42:30Z

core/finality-grandpa/src/authorities.rs

+	/// Apply or prune any pending transitions. Provide a closure that can be used to check for the
+	/// finalized block with given number.
+	///
+	/// When the set has changed, the return value will be `Ok(Some((H, N)))` which is the cnaonical


gnunicorn · 2018-11-15T11:50:58Z

core/finality-grandpa/src/lib.rs

+			};
+
+			if let Err(e) = write_result {
+				warn!(target: "finality", "Failed to write updated authority set to disk. Bailing.");


so we have the trace-target "finality" and "afg" in the same struct. Maybe we want to stick with one?

gnunicorn · 2018-11-15T11:51:35Z

core/finality-grandpa/src/lib.rs

+
+			if let Err(e) = write_result {
+				warn!(target: "finality", "Failed to write updated authority set to disk. Bailing.");
+				warn!(target: "finality", "Node is in a potentially inconsistent state.");


how about including the error in this warn already?

* Fix building master * Fix westend chainspec

rphmeier added 8 commits October 17, 2018 14:51

grandpa-compatible digest items

7d89d65

round-localized signatures on GRANDPA messages

2f9c1d2

generalize some import_queue params

1146ca7

start import queue in chain_ops as well

edc9cd9

localize messages to set-index and add pending changes

38d4eb0

Merge branch 'master' into rh-grandpa-dynamic2

0d786bc

note future changes when importing block

a03d48f

use generic block number in GRANDPA

370c109

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

rphmeier added 15 commits October 26, 2018 19:19

apply authority set changes

0dfa924

compiles

7f51db9

add some tests for authority set logic

c0c6597

restart voter with new authority set as necessary

a33180f

Merge branch 'master' into rh-grandpa-dynamic2

3f26058

minimize public interface

2160e44

add substrate finality grandpa primitives for WASM

0768c11

call into chain WASM to check for changes

a3667df

write authority set to DB

13ee7a7

read authority set from DB on startup

9478b90

refactor import queue to use explicit block import references

be260ac

generalizable data for peers in test network

2362386

tests compile using new test framework

914f21c

extract out tests to file

4b29c0d

tests compile after changes

6a7e6cf

rphmeier mentioned this pull request Oct 31, 2018

Pluggable Consensus: Import Queue #784

Closed

rphmeier added 5 commits October 31, 2018 12:39

initial test to ensure auxiliary data is generated correctly

770f1f6

set aux in block import

82115aa

implement aux-storing in client-db

a47d765

client sets aux in import pipeline

aea36fc

finish GRANDPA test: dynamic authority sets

c552349

rphmeier added 3 commits November 1, 2018 20:25

use published finality-grandpa

a6acdc6

Merge branch 'master' into rh-grandpa-dynamic2

dd045a7

all tests compile

427dd54

gavofyork reviewed Nov 2, 2018

View reviewed changes

rphmeier added 3 commits November 3, 2018 16:45

API id module

13a4d9a

ignore test temporarily

437f0b8

fix test

1522b5c

rphmeier mentioned this pull request Nov 3, 2018

GRANDPA SRML Module #1065

Closed

rphmeier added 2 commits November 5, 2018 10:47

re-export ApiId from sr-api

d436cf8

add some docs

f26699a

andresilva reviewed Nov 7, 2018

View reviewed changes

rphmeier added 2 commits November 14, 2018 14:32

Merge branch 'master' into rh-grandpa-dynamic2

df928f3

address grumbles

152f481

bkchr reviewed Nov 14, 2018

View reviewed changes

andresilva approved these changes Nov 15, 2018

View reviewed changes

gnunicorn approved these changes Nov 15, 2018

View reviewed changes

gnunicorn added A5-grumble and removed A0-please_review Pull request needs code review. labels Nov 15, 2018

rphmeier and others added 2 commits November 15, 2018 16:36

start addressing basti comments

782363a

Fixes GrandpaApi implementation and usage

23b075c

rphmeier merged commit 23b075c into master Nov 15, 2018

rphmeier mentioned this pull request Nov 16, 2018

rename fg-primitives to more verbose name #1123

Merged

This was referenced Nov 26, 2018

GRANDPA: Dynamic Validator Sets #882

Closed

Finality Gadget Handoff #864

Closed

Write provided auxiliary data changes to DB in client when importing block. #1023

Closed

lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020

Fix building master (paritytech#1014)

d12bcdb

* Fix building master * Fix westend chainspec

rphmeier mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#4

Merged

Conversation

rphmeier commented Oct 25, 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!

andresilva 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!

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!

andresilva commented Nov 7, 2018

Uh oh!

rphmeier commented Nov 14, 2018

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!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

andresilva commented Nov 15, 2018

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!

rphmeier Nov 15, 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

rphmeier commented Oct 25, 2018 •

edited

Loading

rphmeier Nov 15, 2018 •

edited

Loading