-
Notifications
You must be signed in to change notification settings - Fork 131
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
Fixed sparse parachains finality handling in on-demand parachains relay #1419
Fixed sparse parachains finality handling in on-demand parachains relay #1419
Conversation
…vered, then we must select para header that may be proved using this relay header
Merging unreviewed, noted in #1407 |
/// It is a "mild" error, which may appear when e.g. on-demand parachains relay is used. | ||
/// This variant must be treated as "we don't want to update parachain head value at the | ||
/// target chain at this moment". | ||
Unavailable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe NotRequested
or NotNeeded
would be more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been looking at it as "The source client refuses to report parachain head hash at this moment." primarily. But yeah - your option also seems sensible to me :)
@@ -52,6 +52,23 @@ pub enum ParachainSyncStrategy { | |||
All, | |||
} | |||
|
|||
/// Parachain head hash, available at the source (relay) chain. | |||
#[derive(Clone, Copy, Debug)] | |||
pub enum ParaHashAtSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather related to #1484 . I would make this structure more generic, for example:
pub enum ExtOption<T> { // needs better name
None,
Some(T),
Unavailable,
}
And in parachain_head()
instead of having
let mut para_hash_at_source = ParaHashAtSource::None;
let mut para_header_number_at_source = None;
...
I would use ExtOption<HeaderId>
in order to manage both variables at once:
let para_header_id = ExtOption::<HeaderId>::None;
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems like a good idea. But we're using some non-trivial combintations there - e.g.
para_hash_at_source = ParaHashAtSource::Unavailable;
para_header_number_at_source = None;
So it shall be done with care (if done at all). Can you, please, file an issue - I could try to handle it a bit later? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was already experimenting with this. If it works out and if that's ok for you, I will publish a PR later.
* Parachains source cosmetic changes - Make `ParaHashAtSource` more generic - Modify `on_chain_parachain_header` to return `HeaderId` - Shortening variable names Signed-off-by: Serban Iorga <[email protected]> * Change ParachainsSource::max_head_id type Change ParachainsSource::max_head_id to Arc<Mutex<NoopOption>> Signed-off-by: Serban Iorga <[email protected]> * code review changes
The comments were addressed as part of #1531 . Marking the PR as reviewed. |
* insert zero epoch index into relay sproof * fix
…vered, then we must select para header that may be proved using this relay header (paritytech#1419)
* Parachains source cosmetic changes - Make `ParaHashAtSource` more generic - Modify `on_chain_parachain_header` to return `HeaderId` - Shortening variable names Signed-off-by: Serban Iorga <[email protected]> * Change ParachainsSource::max_head_id type Change ParachainsSource::max_head_id to Arc<Mutex<NoopOption>> Signed-off-by: Serban Iorga <[email protected]> * code review changes
…vered, then we must select para header that may be proved using this relay header (paritytech#1419)
* Parachains source cosmetic changes - Make `ParaHashAtSource` more generic - Modify `on_chain_parachain_header` to return `HeaderId` - Shortening variable names Signed-off-by: Serban Iorga <[email protected]> * Change ParachainsSource::max_head_id type Change ParachainsSource::max_head_id to Arc<Mutex<NoopOption>> Signed-off-by: Serban Iorga <[email protected]> * code review changes
That's the issue I've found when working on #1418:
The solution is to always explicitly read finalized parachain head from relayed relay chain block when switching from
RelayingRelayHeader
toRelayingParaHeader
state. A good side effect of that is that now we don't need thisheaders_map_cache
, originally introduced in the #1405