Cleaner GRANDPA RPC API for proving finality #7339

octol · 2020-10-16T13:52:26Z

Implements #7115 as in the description, with the exception that the RPC call takes block numbers instead of block hashes

polkadot companion: paritytech/polkadot#2100

TODO:

andresilva

Looks to be going in the right direction :)

andresilva · 2020-10-19T13:38:30Z

client/finality-grandpa/src/authority_set_changes.rs

+    pub(crate) fn get_set_id(&self, number: N) -> (u64, N) {
+        let set_id = self
+            .authority_set_changes
+            .binary_search(&number)


I know I suggested not storing the set_id and instead implicitly figuring out by the position in the array. The only disadvantage of this approach is that it breaks for nodes that didn't sync from scratch with this code. I guess it might be better to store the set_id explicitly. Additionally in the future we might start syncing from a specific checkpoint (rather from genesis always) or we may warp sync, in both cases we won't import all blocks.

andresilva · 2020-10-19T13:40:00Z

client/finality-grandpa/src/authority_set_changes.rs

+// Tracks authority set changes. We store the block numbers for the last block of each authority
+// set.
+#[derive(Debug, Encode, Decode)]
+pub(crate) struct AuthoritySetChanges<N> {


Maybe this can go inside AuthoritySet? We will always be operating on both at the same time, i.e. when we finalize something from AuthoritySet is also when we update this.

…ity-rpc-api

octol · 2020-11-26T23:20:59Z

Think this is probably ready for another round of feedback now, and finished enough to be upgraded from draft to PR

Most things have been reworked according to feedback from previous versions, as well as #7546 landing in master gives us a lot more freedom in changing finality_proofs.rs

wheresaddie

lgtm ty

andresilva · 2021-01-12T22:26:20Z

bot merge

ghost · 2021-01-12T22:26:23Z

Trying merge.

ghost · 2021-01-12T22:26:24Z

Merge failed: "At least 2 approving reviews are required by reviewers with write access."

bkchr

LGTM as far as I understand it :P

bkchr · 2021-01-15T19:53:48Z

client/finality-grandpa/src/finality_proof.rs

+		let err = format!(
+			"AuthoritySetChanges does not cover the requested block #{}. \
+			Maybe the subscription API is more appropriate.",
+			block,
+		);
+		trace!(target: "afg", "{}", &err);


Suggested change

let err = format!(

"AuthoritySetChanges does not cover the requested block #{}. \

Maybe the subscription API is more appropriate.",

block,

);

trace!(target: "afg", "{}", &err);

trace!(

target: "afg",

"AuthoritySetChanges does not cover the requested block #{}. \

Maybe the subscription API is more appropriate.",

block,

);

bkchr · 2021-01-15T20:06:19Z

client/finality-grandpa/src/finality_proof.rs

+				trace!(
+					target: "afg",
+					"Encountered new authorities when collecting unknown headers. \
+					Returning empty proof"


Suggested change

Returning empty proof"

Returning empty proof",

bkchr · 2021-01-15T20:12:33Z

client/finality-grandpa/src/authorities.rs

+	fn decode<I: Input>(value: &mut I) -> Result<Self, parity_scale_codec::Error> {
+		let legacy = LegacyAuthoritySet::decode(value)?;
+		let authority_set_changes = match <AuthoritySetChanges<N>>::decode(value) {
+			Ok(v) => v,
+			Err(_) => AuthoritySetChanges::empty(),
+		};


Where is this decode being used @andresilva ?

If someone uses this where there is still data in I, this could lead to decoding shit :P

Currently it's only used through this https://github.com/paritytech/substrate/blob/master/client/finality-grandpa/src/aux_schema.rs#L110, so I don't think it is ever decoded as part of a bigger structure (i.e. there is no more data to decode). It would be nice that we don't have a footgun here though, do you have any suggestion?

Maybe we should offer some migration? So, that we only apply this once? But I assume we don't have any way to store a version in aux?

Besides that, I don't have better ideas.

I was already not remembering the aux_schema code for GRANDPA. We indeed do have a version there and we have code there that handles migrations of GRANDPA data. So yeah, we should definitely handle this the same way.

bkchr · 2021-01-15T20:13:31Z

client/finality-grandpa/src/authorities.rs

@@ -631,6 +689,45 @@ impl<H, N: Add<Output=N> + Clone> PendingChange<H, N> {
 	}
 }

+// Tracks historical authority set changes. We store the block numbers for the first block of each
+// authority set, once they have been finalalized.


Suggested change

// authority set, once they have been finalalized.

// authority set, once they have been finalized.

bkchr · 2021-01-15T20:16:38Z

client/finality-grandpa/src/authorities.rs

+			// To make sure we have the right set we need to check that the one before it also exists.
+			if idx > 0 {
+				let (prev_set_id, _) = self.0[idx - 1usize];
+				if set_id != prev_set_id + 1u64 {


How can that fail? Wouldn't that mean that the entire object is wrong?

Yeah pretty much, if this happens the object is missing authority set changes and something must have gone wrong applying changes. It's my understanding that this can't happen, but I'm not certain enough of that to omit this check ... @andresilva what do you think?

We wanted to fail gracefully for nodes that will migrate to this and therefore won't have all set ids stored here (only when they sync from scratch), the only check that's really needed for this is the first one idx < self.0.len(). These checks don't hurt but they can be removed since they would imply that this data structure skipped set ids spuriously which is wrong.

…ity-rpc-api

andresilva

@octol LegacyAuthoritySet should be moved to aux_schema.rs and renamed to V2AuthoritySet. The CURRENT_VERSION const there should be incremented and a method migration_from_version2 should be added (and then called from load_persistent). This method will read all remaining GRANDPA structures as they are but decode the authority set as V2AuthoritySet and then return the regular AuthoritySet (with AuthoritySetChanges::empty()).

Sorry for not suggesting this earlier but I didn't remember that we already had migration code for auxiliary GRANDPA data.

octol · 2021-01-20T20:25:34Z

@octol LegacyAuthoritySet should be moved to aux_schema.rs and renamed to V2AuthoritySet. The CURRENT_VERSION const there should be incremented and a method migration_from_version2 should be added (and then called from load_persistent). This method will read all remaining GRANDPA structures as they are but decode the authority set as V2AuthoritySet and then return the regular AuthoritySet (with AuthoritySetChanges::empty()).

Oh thanks, yeah that will make it much better!

octol · 2021-01-21T10:28:29Z

Pushed a new commit that implements the migration logic as suggested by @andresilva

andresilva

LGTM, thanks!

andresilva · 2021-01-21T20:04:13Z

There were conflicts in finality_proof.rs because of #7711. Merged master and fixed them.

andresilva · 2021-01-21T20:27:42Z

bot merge

ghost · 2021-01-21T20:27:44Z

Waiting for commit status.

ghost · 2021-01-21T21:15:43Z

Checks failed; merge aborted.

octol · 2021-01-21T21:36:40Z

bot merge

ghost · 2021-01-21T21:36:43Z

Waiting for commit status.

octol added 4 commits October 12, 2020 22:43

grandpa: persist block number for last block of authority set

75d59d8

grandpa: fix authority_set_changes field in tests

c3dac36

grandpa: fix date on copyright notice

363ba7b

grandpa-rpc: implement cleaner api for prove finality rpc

1d54eb5

octol added A3-in_progress Pull request is in progress. No review needed at this stage. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Oct 16, 2020

octol added 2 commits October 16, 2020 16:33

grandpa-rpc: replace the old prove_finality with the new one

58477bc

grandpa: undo accidental whitespace change

10e507f

andresilva reviewed Oct 19, 2020

View reviewed changes

octol added 4 commits October 30, 2020 16:41

Merge remote-tracking branch 'upstream/master' into jon/cleaner-final…

8e99c66

…ity-rpc-api

grandpa-rpc: start work on redo of the finality_proof RPC API

83ff901

Merge remote-tracking branch 'upstream/master' into jon/cleaner-final…

107a4fa

…ity-rpc-api

grandpa: manual impl of Decode for AuthoritySet

df5cca1

octol linked an issue Nov 11, 2020 that may be closed by this pull request

grandpa: cleaner RPC API for proving finality #7115

Closed

octol added 4 commits November 12, 2020 12:39

grandpa: add comment about appending changes for forced changes

6ed408a

grandpa: flip order in set changes, tidy up some comments

bde6188

grandpa: update some of the doc comments

ec52b6f

Merge remote-tracking branch 'upstream/master' into jon/cleaner-final…

0126c4c

…ity-rpc-api

andresilva mentioned this pull request Nov 16, 2020

grandpa: remove light-client specific block import pipeline #7546

Merged

octol added 4 commits November 19, 2020 16:59

grandpa: store authority set changes when applying forced changes

efcd04b

Merge remote-tracking branch 'upstream/master' into jon/cleaner-final…

88ea322

…ity-rpc-api

grandpa: simplify finality_proof.rs

62a7245

grandpa: move checks and extend tests in finality_proof

65c4243

octol 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 Nov 26, 2020

octol marked this pull request as ready for review November 26, 2020 23:17

octol requested a review from andresilva November 26, 2020 23:17

wheresaddie approved these changes Nov 27, 2020

View reviewed changes

grandpa: add error specific to finality_proof

c8efdbb

andresilva approved these changes Jan 12, 2021

View reviewed changes

andresilva requested a review from bkchr January 12, 2021 22:27

bkchr approved these changes Jan 15, 2021

View reviewed changes

octol added 3 commits January 15, 2021 22:55

grandpa: fix review comments

ceee16a

Merge remote-tracking branch 'upstream/master' into jon/cleaner-final…

adc39b8

…ity-rpc-api

Merge remote-tracking branch 'upstream/master' into jon/cleaner-final…

9feb90c

…ity-rpc-api

andresilva suggested changes Jan 20, 2021

View reviewed changes

grandpa: proper migration to new AuthoritySet

aa6282e

grandpa: fix long lines

6d90ce3

andresilva mentioned this pull request Jan 21, 2021

Grandpa warp sync request-response protocol #7711

Merged

andresilva approved these changes Jan 21, 2021

View reviewed changes

Merge branch 'master' into jon/cleaner-finality-rpc-api

bb6cb32

grandpa: fix unused warning after merge

a8878a1

ghost merged commit bd5c9a6 into paritytech:master Jan 21, 2021

octol deleted the jon/cleaner-finality-rpc-api branch January 21, 2021 22:12

octol mentioned this pull request Jan 21, 2021

grandpa: remove runtime checks in prove_finality #7953

Merged

andresilva mentioned this pull request Feb 18, 2021

grandpa: rewrite warp sync proof generation #8148

Merged

andresilva mentioned this pull request May 13, 2021

grandpa: fix warp sync proof on missing data #8795

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleaner GRANDPA RPC API for proving finality #7339

Cleaner GRANDPA RPC API for proving finality #7339

octol commented Oct 16, 2020 •

edited

Loading

andresilva left a comment

andresilva Oct 19, 2020

andresilva Oct 19, 2020

octol commented Nov 26, 2020

wheresaddie left a comment

andresilva commented Jan 12, 2021

ghost commented Jan 12, 2021

ghost commented Jan 12, 2021

bkchr left a comment

bkchr Jan 15, 2021

bkchr Jan 15, 2021

bkchr Jan 15, 2021

andresilva Jan 20, 2021

bkchr Jan 20, 2021

bkchr Jan 20, 2021

andresilva Jan 20, 2021

bkchr Jan 15, 2021

bkchr Jan 15, 2021

octol Jan 15, 2021

andresilva Jan 20, 2021

andresilva left a comment •

edited

Loading

octol commented Jan 20, 2021

octol commented Jan 21, 2021

andresilva left a comment

andresilva commented Jan 21, 2021

andresilva commented Jan 21, 2021

ghost commented Jan 21, 2021

ghost commented Jan 21, 2021

octol commented Jan 21, 2021

ghost commented Jan 21, 2021

	// authority set, once they have been finalalized.
	// authority set, once they have been finalized.

Cleaner GRANDPA RPC API for proving finality #7339

Cleaner GRANDPA RPC API for proving finality #7339

Conversation

octol commented Oct 16, 2020 • edited Loading

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octol commented Nov 26, 2020

wheresaddie left a comment

Choose a reason for hiding this comment

andresilva commented Jan 12, 2021

ghost commented Jan 12, 2021

ghost commented Jan 12, 2021

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment • edited Loading

Choose a reason for hiding this comment

octol commented Jan 20, 2021

octol commented Jan 21, 2021

andresilva left a comment

Choose a reason for hiding this comment

andresilva commented Jan 21, 2021

andresilva commented Jan 21, 2021

ghost commented Jan 21, 2021

ghost commented Jan 21, 2021

octol commented Jan 21, 2021

ghost commented Jan 21, 2021

octol commented Oct 16, 2020 •

edited

Loading

andresilva left a comment •

edited

Loading