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

Another tweak to GrandPa warp sync #8514

Merged
3 commits merged into from
Apr 2, 2021
Merged

Another tweak to GrandPa warp sync #8514

3 commits merged into from
Apr 2, 2021

Conversation

@tomaka
Copy link
Contributor

@tomaka tomaka commented Apr 1, 2021

Another pull request on top of #8392

Changes:

  • Adds a check in verify that the verification targets the header that comes with it, otherwise someone could send a different header where the authorities set change isn't the same.
  • Adds to the proof the header corresponding to the latest finalized. It was notoriously missing, which would have forced us to do an additional network request just for this header.
  • Removes WarpSyncFinished. I don't understand why we needed to special-case the latest justification. It is now part of the proof, which is IMO more simple.

@tomaka tomaka added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Apr 1, 2021
@tomaka tomaka requested review from andresilva and expenses April 1, 2021 15:57
andresilva
andresilva reviewed Apr 1, 2021
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

This works as well but we should rename AuthoritySetChangeProof then since it is no longer required that the given header is actually changing the authority set. This means that a warp sync proof can also include justifications for arbitrary blocks of a given set_id and still be valid, whereas previously we'd guarantee that the warp sync proof is minimal, i.e. it only includes finality proofs for the transitions and one extra for the latest block at the end.

@tomaka
Copy link
Contributor Author

tomaka commented Apr 1, 2021

We can still guarantee that the sync proof is minimal. Only the last proof fragment, when is_finished is true, is allowed to not have an authority set change.

@tomaka
Copy link
Contributor Author

tomaka commented Apr 2, 2021

Asked Erin to deploy this on westend-connect nodes. Warp sync in smoldot is working.

@andresilva
Copy link
Contributor

andresilva commented Apr 2, 2021

bot merge

@ghost
Copy link

ghost commented Apr 2, 2021

Trying merge.

@ghost ghost merged commit fb8b68f into paritytech:master Apr 2, 2021
@tomaka tomaka deleted the gp-warp-sync-cleanup branch April 2, 2021 14:40
@tomaka tomaka mentioned this pull request Apr 2, 2021
Merged
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
@tomaka @hirschenberger
Another tweak to GrandPa warp sync (paritytech#8514)
1523d66 
* Another tweak to GrandPa warp sync

* Rename to WarpSyncFragment

* Ensure proof is minimal
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@andresilva andresilva andresilva approved these changes

@expenses expenses expenses approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tomaka @andresilva @expenses