Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

GRANDPA finality proof draft #1268

Merged
merged 11 commits into from
Jan 11, 2019
Merged

GRANDPA finality proof draft #1268

merged 11 commits into from
Jan 11, 2019

Conversation

svyatonik
Copy link
Contributor

This is a draft for generating GRANDPA finality proof + its validation. In brief:

  • I assume that light client is going to use this ONLY to prove finality of currently importing blocks;
  • since GrandpaJustification::verify involves checking signatures of messages that include current GRANDPA authorities set_id AND it isn't stored in the runtime storage, I assume that light client will be able to track this set_id somehow. It is enough to track only the latest id, because of (1). The tracking-strategy is probably reflect set_id changes in the GRANDPA-specific Header::DigestItem? Are there any plans to do this? Anyway - it is for the future PRs;
  • the proof-of-finality for the block B is the:
    • headers path [B ... F], where F is the earliest known block with GRANDPA justification which is >= B;
    • justification for the block F;
    • proof-of-execution of GrandpaApi::grandpa_authorities at the block F. Since we require blocks that are enacting new GRANDPA authorities set to have justification, this means that the set in blocks B and F are the same. Having this 'subproof' is somewhat questionable and depends on how we're going to track the GRANDPA authorities set_id - if there'll be DigestItem for every change, we could just take both authorities and set_id from this DigestItem => proof-of-execution is not required;
  • calling check_finality_proof only checks that justification is valid with respect to passed set_id and authorities. Validity of the headers subchain [ B ... F ] should be checked outside => this proof is valid iff the whole range of block [ B ... F ] is imported at once and imports succeed.

So the approximate import queue for light client will look like: when it tries to import block B with DigestItem::AuthoritiesChange, it pauses until proof-of-finality of the B is received. When it is received, all blocks [ B ... F ] are imported at once and then sync continues.

@svyatonik svyatonik added the A0-please_review Pull request needs code review. label Dec 14, 2018
@rphmeier rphmeier self-requested a review December 14, 2018 14:47
@rphmeier
Copy link
Contributor

rphmeier commented Dec 14, 2018

This might be well-coupled with a change to create justifications for authority-changing blocks.
Right now, justifications are only created when GRANDPA sets change, which could be a long time.

// now that we know that the block is finalized, we can generate finalization proof

// we need to prove grandpa authorities set that has generated justification
let authorities_proof = generate_execution_proof(&block_id, "GrandpaApi_grandpa_authorities", &[])?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rather get a proof of this execution on the parent block -- it's undocumented at this point but what grandpa_authorities really tells you is which set of authorities is responsible for finalizing children of this block.

This is because when we do a set change at block X, grandpa_authorities(at: X) will give you the new set, but it's actually the old set who is responsible for finalizing X and completing the handoff.

Adding extra documentation to the GrandpaApi trait would be really helpful as well

@svyatonik svyatonik added A4-gotissues and removed A0-please_review Pull request needs code review. labels Dec 14, 2018
@svyatonik svyatonik force-pushed the grandpa_finality_proof branch from 9740d2f to 720951b Compare December 17, 2018 08:49
@svyatonik
Copy link
Contributor Author

@rphmeier I see that first version of your first comment was about generating justification every N blocks. Isn't that better than generating it only "for authority-changing blocks"? I could see that this makes proof-of-finality for these blocks minimal, but for other blocks it still could be large, right? So if the authorities set isn't changing at all, we will never generate justification => won't be able to prove finality to light clients. Or maybe combined approach (every N blocks + when authorities-set changes) will work?

@svyatonik svyatonik added A0-please_review Pull request needs code review. and removed A4-gotissues labels Dec 17, 2018
@rphmeier
Copy link
Contributor

rphmeier commented Dec 17, 2018

@svyatonik yep, that seems reasonable. We can use a slightly larger N for sure if we do both.
We might want to employ a slightly different strategy for finality proofs using log2-ancestors as in #1208 .

@svyatonik svyatonik added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Dec 18, 2018
@svyatonik
Copy link
Contributor Author

@rphmeier I've altered PR with additional justification-generation cases:

  1. it is generated when block with new_authorities.is_some() == true is imported (named it ConsensusChanges - didn't wanted more confuse with authorities). It is simpler than authorities tracking, because we do not actually need to track value, do not need delay + it only tracks change if there's no justification ready for the block we're importing right now. The tracking also occurs in AuxStore;
  2. justification is generated every N blocks (approximately).

Not sure about #1208 - did you mean that proof-of-finality described there supersede proof-of-finality implemented here? What's the "big" ancestry proof then?

@svyatonik svyatonik 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 18, 2018
@rphmeier
Copy link
Contributor

rphmeier commented Dec 19, 2018

Just as an alternate approach to generating ancestry proofs. The idea is to go back further than one block at a time by storing log2 ancestors in the state.

@gavofyork gavofyork requested a review from rphmeier January 7, 2019 14:55
@gavofyork gavofyork added A3-needsresolving and removed A0-please_review Pull request needs code review. labels Jan 7, 2019
@svyatonik svyatonik added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A3-needsresolving labels Jan 9, 2019
@svyatonik svyatonik 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 Jan 11, 2019
@gavofyork gavofyork added this to the 1.0gamma milestone Jan 11, 2019
let parent_block_id = BlockId::Hash(*current_header.parent_hash());
let authorities_proof = generate_execution_proof(
&parent_block_id,
"GrandpaApi_grandpa_authorities",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated @bkchr @cheme is it possible to have the decl_runtime_apis! export some constants?

@rphmeier rphmeier added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jan 11, 2019
@rphmeier
Copy link
Contributor

Seems fine. It would be nice to "traitify" the new_authorities.is_some() thing so we could run GRANDPA over consensus algos without authorities.

@gavofyork gavofyork merged commit b57c458 into master Jan 11, 2019
@gavofyork gavofyork deleted the grandpa_finality_proof branch January 11, 2019 18:25
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* grandpa finality proof

* prove GrandpaApi::grandpa_authorities using parent block + some docs

* create justification when consensus data is changed

* generate justifications periodically

* test for ConsensusChanges
liuchengxu pushed a commit to liuchengxu/substrate that referenced this pull request May 31, 2023
…otocol

Disable unused gossipsub protocol in the DSN stack.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants