Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lib/grandpa): verify GRANDPA justification before applying the block #4144

Merged
merged 6 commits into from
Sep 10, 2024

Conversation

timwu20
Copy link
Contributor

@timwu20 timwu20 commented Aug 23, 2024

Changes

Taken from #4084:

This PR promotes some changes on how Gossamer import a block while full sync
Currently, Gossamer first imports a block/apply the blocks' extrinsic against the state and then it will verify the justification that exists in the block. This PR wants to change that by moving the verification step to be the first in the importing process, then we avoid to import or modify the state with invalid informations (impacting the resiliency of the node)

Differently than I thought, these modifications will require more changes in how our current validation function performs the check, the main point is that the validation rely on the state to check ancestry and also that our current Justification struct miss one field called Ancestors.
That is done by bringing the changes from the feature branch feat/grandpa given that the branch already contains the changes needed to verify the justification correctly.
The main drawback is that it brings a lot of changes from the feature branch, however I tried to reduce the amount of components bring only the required ones for the justification validation

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

close #3468

@timwu20 timwu20 force-pushed the tim/eclesio/verify-justification branch 2 times, most recently from b7f335e to f06b42c Compare August 23, 2024 17:49
@timwu20 timwu20 marked this pull request as ready for review August 24, 2024 02:23
@timwu20 timwu20 changed the title Tim/eclesio/verify justification feat(lib/grandpa): verify GRANDPA justification before applying the block Aug 24, 2024
Copy link
Contributor

@jimjbrettj jimjbrettj left a comment

Choose a reason for hiding this comment

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

Couple small comments, but overall lgtm

Base automatically changed from tim/pkg-finality-grandpa to development September 5, 2024 17:41
@timwu20 timwu20 force-pushed the tim/eclesio/verify-justification branch from f06b42c to 0d0794f Compare September 5, 2024 17:49
Copy link
Contributor

@dimartiro dimartiro left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits and some questions

dot/sync/chain_sync.go Outdated Show resolved Hide resolved
dot/sync/chain_sync.go Outdated Show resolved Hide resolved
internal/primitives/core/hash/hash.go Show resolved Hide resolved
internal/primitives/core/hashing/hashing.go Show resolved Hide resolved
@timwu20 timwu20 merged commit 1836e44 into development Sep 10, 2024
24 checks passed
@timwu20 timwu20 deleted the tim/eclesio/verify-justification branch September 10, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

While syncing problematic block keeps in the state after VerifyBlockJustification fails
4 participants