This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Create AuthoritiesChange digest item in correct block#2512
Merged
Conversation
Member
|
Happy to break stuff in master now, so this can go in. Two alternatives for fixes:
|
gavofyork
approved these changes
May 10, 2019
MTDK1
pushed a commit
to bdevux/substrate
that referenced
this pull request
Jul 10, 2019
* finalize srml modules in reverse order * update runtime versions
This file contains hidden or 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
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.
This is rather major light-related issue (my bad that it is found too late), so I'll describe it in details. Please note, that now light client doesn't relies on
AuthoritiesChangedigest item => the situation I'm describing will be valid once #1669 is in.Original requirements: header of the block
Ashould includeAuthoritiesChange(new_authorities)digest item if authorities set changes at this block. This new set from the digest item is used by light client starting from next blockB.The issue:
AuthoritiesChangedigest item actually appears in the blockB(instead ofA), meaning thatBitself is checked using previous authorities set on light client => verification will fail.Why: the digest item is emitted by
Consensus::on_finalizeand authorities change occurs inSession::on_finalize. Unfortunately,Consensusis finalized beforeSession=> digest item is scheduled for the next block.Possible solutions I have considered:
construct_runtime!) and finalize in reverse order. Currently it is: initialize and deinitialize in declaration order. This way (at least in node' runtime) we first callSession::on_finalize, thenConsensus::on_finalize, thus emitting digest item in correct block. This isn't the best solution, imo - it relies on yet-undocumented init+fin order. But it is the (hacky) solution that requires least code changes;Consensus&& make something likeConsensus::mutate_authorities. EmitAuthoritiesChangeat the end of this call. There's a problem if authorities are changed several times in one block => we'll emit >1 digest items. Solving that would require modifications of already preparedDigest(i.e. breaking API - something likeDigest::logs_mut(&mut self));Consensus::mutate_authorities=> we'll teach native how to treat multipleAuthoritiesChangeitems in single block - we'll take the last change. The bad thing is that if there are many changes within block (like currently - we're editing authorities one-by-one), there'll be a lot ofAuthoritiesChangeitems (i.e. header could be huge);construct_runtime- that is similar to the (1).The common problem of all solutions is that once #1669 is in, light client will fail to sync existing chains (even if we'll update full nodes with code that is able to serve finality proof requests). I could make a hackish workaround for that in Aura - if header verification fails, fallback to what we have now (i.e. remote execution of
AuthoritiesApi::authorities()). But we need to decide whether we need this, or not. If I understand correctly, this won't affect syncing existing chains by full nodes (need check), because it changes runtime code => new wasm will be used on 'old' nodes.