This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Storing multiple Justifications per block #7640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
octol
added
A3-in_progress
Initial implementation of append_justification on the Backend trait, and also remove unused skeleton functions for append_justificaton on Finaziler trait. k
|
Implemented that last bit of functionality (db migration) so this should now be ready for the first round of reviews! |
andresilva
approved these changes
Mar 15, 2021
There was a problem hiding this comment.
I pushed some last changes to fix some nits. Currently the networking and import queue still act as if there's only one justification per block. Let's handle the remaining work in follow-up PRs.
I tested the migration on both Kusama and Polkadot and didn't find any issues.
I also merged master and I think the justification sync tests are failing (if you could please have a look). Once the tests are green we can merge.
andresilva
added
the
D5-nicetohaveaudit ⚠️
|
bot merge |
|
Waiting for commit status. |
|
Checks failed; merge aborted. |
|
bot merge force |
|
Trying merge. |
ghost
merged commit 
HCastano
added a commit
to paritytech/parity-bridges-common
that referenced
this pull request
Apr 6, 2021
Related Substrate PR: paritytech/substrate#7640
HCastano
added a commit
to paritytech/parity-bridges-common
that referenced
this pull request
Apr 7, 2021
* Bump Substrate * Change usage of "Module" to "Pallet" Related Substrate PR: paritytech/substrate#8372 * Add `OnSetCode` config param Related Substrate PR: paritytech/substrate#8496 * Update Aura Slot duration time type Related Substrate PR: paritytech/substrate#8386 * Add `OnSetCode` to mock runtimes * Add support for multiple justifications Related Substrate PR: paritytech/substrate#7640 * Use updated justification type in more places * Make GenesisConfig type non-optional Related Substrate PR: paritytech/substrate#8275 * Update service to use updated telemetry Related Substrate PR: paritytech/substrate#8143 * Appease Clippy
hirschenberger
pushed a commit
to hirschenberger/substrate
that referenced
this pull request
Apr 14, 2021
* primitives/runtime: initial changes on supporting multiple Justifications * primitives/runtime: make Justifications strongly typed * Encode/decode Justifications * primitives/runtime: add Justification type * backend: apply_finality and finalize_block takes a single Justification * manual-seal: create engine id and let rpc take encoded justification * backend: skeleton functions for appending justifications * backend: initial implementation append_justification Initial implementation of append_justification on the Backend trait, and also remove unused skeleton functions for append_justificaton on Finaziler trait. k * backend: guard against duplicate consensus engine id * client/db: add check for block finality * client/api: add append_justification to in_mem db * client/light: add no-op append_justification * network: fix decode call for Justification * network: only send a single Justification in BlockData * network: minor comment update * protocol: update field names to distinguish single justification * client: further field renames to plural * client: update function names to plural justifications * client/db: upgrade existing database for new format * network: remove dependency on grandpa crate * db: fix check for finalized block * grandpa: check for multiple grandpa justifications hwne importing * backend: update Finalizer trait to take multiple Justifications * db: remove debugging statements in migration code * manual-seal: update note about engine id * db: fix check for finalized block * client: update variable name to reflect it is now plural * grandpa: fix incorrect empty Justications in test * primitives: make Justifications opaque to avoid being empty * network: fix detecting empty Justification * runtime: doc strings for Justifications functions * runtime: add into_justifications * primitives: check for duplicates in when adding to Justifications * network/test: use real grandpa engine id in test * client: fix reviewer comments * primitives: rename Justifications::push to append * backend: revert changes to Finalizer trait * backend: revert mark_finalized * backend: revert changes to finalize_block * backend: revert finalized_blocks * db: add a quick early return for performance * client: minor reviewer comments * service/test: use local ConsensusEngineId * network: add link to issue for sending multiple Justifications * Apply suggestions from code review Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * Apply suggestions from code review Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * network: tweaks to review suggestions * network: revert change to BlockData for backwards compatibility * Apply suggestion from code review Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * Apply suggestions from code review Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * primitives: update doc comment for Justifications * client/db/upgrade: avoid grandpa crate dependency * consensus: revert to single Justification for import_justification * primitives: improve justifications docs * style cleanups * use and_then * client: rename JUSTIFICATIONS db column * network: revert to using FRNK in network-test Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> Co-authored-by: André Silva <andrerfosilva@gmail.com>
This was referenced Apr 16, 2021
viniul
added
D1-audited 👍
jordy25519
pushed a commit
to cennznet/substrate
that referenced
this pull request
Sep 17, 2021
* primitives/runtime: initial changes on supporting multiple Justifications * primitives/runtime: make Justifications strongly typed * Encode/decode Justifications * primitives/runtime: add Justification type * backend: apply_finality and finalize_block takes a single Justification * manual-seal: create engine id and let rpc take encoded justification * backend: skeleton functions for appending justifications * backend: initial implementation append_justification Initial implementation of append_justification on the Backend trait, and also remove unused skeleton functions for append_justificaton on Finaziler trait. k * backend: guard against duplicate consensus engine id * client/db: add check for block finality * client/api: add append_justification to in_mem db * client/light: add no-op append_justification * network: fix decode call for Justification * network: only send a single Justification in BlockData * network: minor comment update * protocol: update field names to distinguish single justification * client: further field renames to plural * client: update function names to plural justifications * client/db: upgrade existing database for new format * network: remove dependency on grandpa crate * db: fix check for finalized block * grandpa: check for multiple grandpa justifications hwne importing * backend: update Finalizer trait to take multiple Justifications * db: remove debugging statements in migration code * manual-seal: update note about engine id * db: fix check for finalized block * client: update variable name to reflect it is now plural * grandpa: fix incorrect empty Justications in test * primitives: make Justifications opaque to avoid being empty * network: fix detecting empty Justification * runtime: doc strings for Justifications functions * runtime: add into_justifications * primitives: check for duplicates in when adding to Justifications * network/test: use real grandpa engine id in test * client: fix reviewer comments * primitives: rename Justifications::push to append * backend: revert changes to Finalizer trait * backend: revert mark_finalized * backend: revert changes to finalize_block * backend: revert finalized_blocks * db: add a quick early return for performance * client: minor reviewer comments * service/test: use local ConsensusEngineId * network: add link to issue for sending multiple Justifications * Apply suggestions from code review Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * Apply suggestions from code review Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * network: tweaks to review suggestions * network: revert change to BlockData for backwards compatibility * Apply suggestion from code review Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * Apply suggestions from code review Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * primitives: update doc comment for Justifications * client/db/upgrade: avoid grandpa crate dependency * consensus: revert to single Justification for import_justification * primitives: improve justifications docs * style cleanups * use and_then * client: rename JUSTIFICATIONS db column * network: revert to using FRNK in network-test Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> Co-authored-by: André Silva <andrerfosilva@gmail.com>
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First step in implementing #7556, this PR aims to add support for passing around and storing multiple
Justifications per block. This includes appending newJustifications to an already existing block.In this step we aim to keep backwards compatibility by only sending GRANDPA justifications across the network, and assuming that any incoming justifications are for GRANDPA. The RPC API still returns only GRANDPA justifications.
In a follow-up PR we will implement the suggestion in the issue for adding support to send multiple Justification.
TODO
Justificationto carry multiple encoded Justifications indexed by engine IDJustificationstype safeJustificationstoVec<u8>polkadot companion: paritytech/polkadot#2358