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

Fixed sparse parachains finality handling in on-demand parachains relay #1419

Merged

Conversation

svyatonik
Copy link
Contributor

That's the issue I've found when working on #1418:

  1. RialtoParachain -> Millau messages relay is seeing new message at RialtoParachain block RialtoParachainBlock=100. It asks on-demand parachains relay to relay RialtoParachainBlock=100;
  2. on-demand relay relays RialtoBlock=150 first, which actually has head of RialtoParachainBlock=110. So it is some head from the future;
  3. on-demand relay never returns any RialtoParachain blocks with number > 100 => RialtoParachainBlock=110 is never relayed and relay stucks.

The solution is to always explicitly read finalized parachain head from relayed relay chain block when switching from RelayingRelayHeader to RelayingParaHeader state. A good side effect of that is that now we don't need this headers_map_cache, originally introduced in the #1405

…vered, then we must select para header that may be proved using this relay header
@svyatonik
Copy link
Contributor Author

Merging unreviewed, noted in #1407

@svyatonik svyatonik merged commit 6067d73 into master May 27, 2022
@svyatonik svyatonik deleted the fix-sparse-finality-handling-in-on-demand-parachains-relay branch May 27, 2022 13:49
/// 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,
Copy link
Collaborator

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.

Copy link
Contributor Author

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 {
Copy link
Collaborator

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;
...

Copy link
Contributor Author

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!

Copy link
Collaborator

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.

@serban300 serban300 mentioned this pull request Aug 1, 2022
svyatonik pushed a commit that referenced this pull request Aug 2, 2022
* 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
@serban300
Copy link
Collaborator

serban300 commented Aug 2, 2022

The comments were addressed as part of #1531 . Marking the PR as reviewed.

svyatonik pushed a commit that referenced this pull request Jul 17, 2023
* insert zero epoch index into relay sproof

* fix
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
…vered, then we must select para header that may be proved using this relay header (paritytech#1419)
serban300 added a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
* 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
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
…vered, then we must select para header that may be proved using this relay header (paritytech#1419)
serban300 added a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants