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

Parachains finality relay #1199

Merged
merged 2 commits into from
May 20, 2022
Merged

Parachains finality relay #1199

merged 2 commits into from
May 20, 2022

Conversation

svyatonik
Copy link
Contributor

on top of #1068

@AurevoirXavier
Copy link

I'd like to know the current status of this PR.
What block this?

Thanks.

@svyatonik
Copy link
Contributor Author

svyatonik commented Mar 24, 2022

I'd like to know the current status of this PR. What block this?

Thanks.

I've responded here (#1218 (comment)). I hope we'll have resources to work on this soon

@fig666
Copy link

fig666 commented Mar 24, 2022

invalid command

@svyatonik
Copy link
Contributor Author

@eskimor Since you've reviewed parachains finality pallet recently (really appreciate that), would you be so kind to review this PR as well, when you have a spare hour? It introduces a relay (offchain actor) that connects to the source (relay) chain and target chain (relay or parachain) and polls source client for parachain heads updates. Whenever head changes, it submits updated-parachain-head transaction to the target chain. This code is abstracted from real chains, because it needs to work with our homegrown Rialto and Millau and with public Rococo/Wococo/Kusama/Polkadot chains.

The main logic is in the relays/parachains/src/parachains_loop.rs

@svyatonik svyatonik marked this pull request as ready for review May 13, 2022 08:21
@eskimor
Copy link
Member

eskimor commented May 13, 2022

Things keep piling up on my list, but I'll try to squeeze it in.

@svyatonik svyatonik enabled auto-merge (squash) May 20, 2022 08:18
@svyatonik svyatonik merged commit a4d52d5 into master May 20, 2022
@svyatonik svyatonik deleted the parachains-finality-relay branch May 20, 2022 08:34
Copy link
Collaborator

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Looks good, from a functional point of view I only have a single question below. 👍

relays/parachains/src/parachains_loop.rs Show resolved Hide resolved
Comment on lines +307 to +308
if head_at_target.at_relay_block_number < best_finalized_relay_block.0 &&
head_at_target.head_hash != *head_at_source =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not "<=" in head_at_target.at_relay_block_number <= best_finalized_relay_block.0? It's my understanding that pallet_bridge_grandpa will sync headers then this pallet syncs para heads so it should be ok to sync para heads to the latest synced relay block.

Current code seems to want to keep para heads syncing one block behind finalized relay blocks syncing. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea (that still may be criticized, though) was to avoid decoding parachain heads in the bridge-prachains pallet code. That's because of two things:

  1. polkadot is also not doing that in their parachain-related pallets - heads are always opaque there;
  2. the pallet supports syncing multiple parachains at once, so if e.g. two parachains are using different header structs (i.e. one is frame-based and another is not), it'd require some non-trivial configuration.

But we still want to import parachain heads in order. To do that I'm relying on the fact that the best-finalized-parachain-head is always increasing. I.e. best finalized parachain head at relay block 100 is always the ancsestor of the best finalized parachain head at relay block 200. This also means that if we have already delivered best finalized parachain head at relay block 100, we don't need to do it again - it'll be the same.

This condition is exactly for handling this latter case: head_at_target.at_relay_block_number < best_finalized_relay_block.0 means "if we MAY know better finalized-parachain-head than is currently available at the target" and the head_at_target.head_hash != *head_at_source means "and it is ACTUALLY BETTER". I.e. if first condition is true, and the second one is false, then the relay chain hasn't been updating parachain head for a while (it is a normal situation).

If we would change head_at_target.at_relay_block_number < best_finalized_relay_block.0 to head_at_target.at_relay_block_number <= best_finalized_relay_block.0, then it'd mean "if we MAY know better OR THE SAME finalized-parachain-head than is currently available at the target". And we don't want to deliver the same parachain head multiple times (pallet would reject such transaction). But actually, it is safe to change this condition to "<=", because then the second condition would fail anyway and the parachain head won't pass filter. But imo "<" is stricter and safer to use here :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, that makes sense, let's keep the stricter/safer version and revisit if we see any need for lag improvements 👍

@svyatonik svyatonik mentioned this pull request May 26, 2022
wuminzhe added a commit to darwinia-network/darwinia-messages-substrate that referenced this pull request Aug 8, 2022
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
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.

6 participants