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

Properly save justifications and GrandPa commits for later #2400

Merged
merged 4 commits into from
Jun 20, 2022

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jun 17, 2022

At the moment, when a chain is initialized, you will see messages saying "error while verifying grandpa commit message: commit targets a block not in the chain".
For context, a "Grandpa commit" is simply a proof of the finality of a block. It contains votes made by the validators.

What happens is: right after we have finished warp syncing to the current finalized block, a peer sends us a proof of finality. But the proof of finality targets a block that we haven't downloaded yet (as we've just finished warp syncing), and thus can't be verified.
In this context, the correct thing to do is instead to store the proof of finality and verify it later. This is what this PR does.

This however causes an issue, as sources could just send us hundreds of proofs of finality concerning unknown blocks. To prevent this from happening, we store for each peer only 2 proofs maximum: the one that target the lowest block number and the one that targets the higher block number.
If the peer is well behaving, then the one targeting the highest block number is likely the one we want to actually verify and apply in order to be up to date with the finality of the chain.

However, we also keep the one targeting the lowest block number, in order to make sure to be able to make some progress at some point in the future. The idea behind this is that the lowest-block-number-targeting finality proof should never change (again, assuming that the peer is well behaving). And thus even if for example for some reason downloading blocks is suuuuper slow, at some point in the future we will manage to download the block necessary to verify the lowest-block-number-targeting finality proof. Whereas if we only kept the latest finality proof, which gets overwritten every time a new proof is received, we might never be able to catch up.

This PR also fixes a current issue, which is that we only keep one unverified justification per block. For context, a justification is the same thing as a finality proof, except that they need to be explicitly required and not received passively. Storing one unverified justification per block means that if for some reason we download the same block from two peers, the justification sent by the second one will overwrite the one sent by the first one, even though the justification sent by the second one might be incorrect and the justification sent by the first one correct.
Now, we store justifications per peer, as described above.

This is a big step towards making the full syncing feature-complete.

Due to all these changes, many of the functions or enums that said "justification" now also apply to Grandpa commit, and thus I've renamed them to "finality proof".

Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@tomaka tomaka changed the title Properly save justifications and GrandPa commits Properly save justifications and GrandPa commits for later Jun 17, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────
      -11191 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h054a169e748748ca
      +11191 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h7931b10db25093ab
      +10615 ┊ smoldot::network::service::ChainNetwork<TNow>::next_event::h6d6ea058c4491ad3
      -10579 ┊ smoldot::network::service::ChainNetwork<TNow>::next_event::h1f33bd85024abca9
       +7690 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::heb51c5c9e90aa264
       -7608 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hb460492ff5a14fae
       +4955 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h6f11b3ca258a6531
       -4481 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hc02dcde0245d6557
       +4195 ┊ smoldot_light_base::sync_service::standalone::Task<TPlat>::inject_network_event::hc88d36d821b815e3
       -4155 ┊ smoldot_light_base::sync_service::standalone::Task<TPlat>::inject_network_event::h3b24793db3ab9f74
       -4040 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h3f21c9a52e9ef96b
       +4040 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h590aa083e47ab149
       +3114 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h7a8da2b1004da59f
       -3114 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hc6ad47fd9679603f
       +2491 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hcb1b5a00280fa1db
       -2491 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hd03465c85fcd17e2
       +1514 ┊ smoldot::sync::all_forks::AllForksSync<TBl,TRq,TSrc>::block_announce::hac6c1fe43469910e
       -1514 ┊ smoldot::sync::all_forks::AllForksSync<TBl,TRq,TSrc>::block_announce::hd32735e610851999
       +1187 ┊ smoldot::sync::all_forks::AddBlockVacant<TBl,TRq,TSrc>::insert::hfaaeada8a7026045
       -1170 ┊ smoldot::sync::all_forks::AnnouncedBlockUnknown<TBl,TRq,TSrc>::insert_and_update_source::h65013e1bff5e970c
       +6052 ┊ ... and 936 more.
      +10457 ┊ Σ [956 Total Rows]

@melekes
Copy link
Contributor

melekes commented Jun 17, 2022

thanks for the write-up 👍 would be good to include it in the commit message

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@tomaka
Copy link
Contributor Author

tomaka commented Jun 17, 2022

would be good to include it in the commit message

I don't think that's the appropriate place.
Commits are squashed, so the explanation would be lost anyway. And putting that explanation in the source code is IMO a better idea, as I'm explaining how things work and will (normally) continue to work in the future rather than why I'm doing a change.

@melekes
Copy link
Contributor

melekes commented Jun 17, 2022

And putting that explanation in the source code is IMO a better idea, as I'm explaining how things work and will (normally) continue to work in the future rather than why I'm doing a change.

Could be architecture docs or something like that, yeah

@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Jun 20, 2022
@mergify mergify bot merged commit 4bdf701 into paritytech:main Jun 20, 2022
@tomaka tomaka deleted the correct-finality-proof branch June 20, 2022 08:26
mergify bot pushed a commit that referenced this pull request Oct 4, 2022
…2800)

Fixes a TODO in the code.

This TODO was from a time when we were handling justifications very
naively by attaching them to blocks.
After #2400, we now properly
handle justifications by attaching them to a (block, source) tuple,
meaning that the problem of replacing existing justifications with other
ones doesn't exist anymore.

This TODO should have been fixed in
#2400, but I forgot.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants