add warp to target block for parachains

[samelamin]

This PR is the first part of a 3 part PR to Substrate,Polkadot and Cumulus to address https://github.com/paritytech/cumulus/issues/1619

The goal of the PR is to enable warp sync support for parachains and is done by passing an enum WarpSyncParams that lets ChainSync start the warp sync to a target block

polkadot companion: https://github.com/paritytech/polkadot/pull/6334 cumulus companion: https://github.com/paritytech/cumulus/pull/1909

polkadot address: 13xS6fK6MHjApLnjdX7TJYw1niZmiXasSN91bNtiXQjgEtNx

[samelamin]

I think the failure continuous-integration/gitlab-check-dependent-polkadot is due to the fact that we need to do a PR into both Polkadot and Cumulus to apply the new changes.

How are these interdependencies managed? I am assuming PRs need to go into substrate first before making the changes downstream?

[altonen]

You need to add a polkadot companion: https://github.com/paritytech/substrate/blob/master/docs/CONTRIBUTING.adoc#updating-polkadot-as-well

[samelamin]

You need to add a polkadot companion: https://github.com/paritytech/substrate/blob/master/docs/CONTRIBUTING.adoc#updating-polkadot-as-well

Awesome, thanks @altonen ill make the changes required. Since I don't have rights to rerun the job ill just push a new commit to start the CI jobs.

[samelamin]

Failing builds are meant to be fixed by this pr

https://github.com/paritytech/scripts/pull/488

[samelamin]

Hi Everyone, let me know if there are any more changes that are required on this pr

[samelamin]

@bkchr @skunert @altonen is there anyone else I need to tag to review? I am sure the devs might still be busy after sub0 so please let me know if its just a matter of waiting

I am just worried perhaps I am missing something from the pr

[altonen]

Could you write some tests for this?

[samelamin]

Could you write some tests for this?

of course, I did think about how to approach testing on this but looking at the warp_sync it defaults to the warp provider https://github.com/paritytech/substrate/blob/master/client/network/test/src/sync.rs#L1230

To test this I would have to change the signature below to pass in the type of sync it is

https://github.com/paritytech/substrate/blob/0a27e54514338b71204e7321192fbd32dae3b950/client/network/test/src/lib.rs#L860

I am ok with doing that but it also means that would change everywhere that function is being used and I was not sure that was clean code design and I was sure the changes did not break the sync because the current sync test pass

Is there a better way to test this?

[samelamin]

@altonen I spoke to @bkchr who kindly suggested I use the config to test. Let me know if the added test is acceptable

[altonen]

I don't know how interesting that test this, maybe others can chime in. Maybe they would make more sense on cumulus side, proving that parachain warp sync works.

[samelamin]

I don't know how interesting that test this, maybe others can chime in. Maybe they would make more sense on cumulus side, proving that parachain warp sync works.

Yeah on the substrate side we are just reusing the target block functionality, the current test ensures that the flow works when we are using WaitForTarget rather than WithProvider

On the cumulus side we do have a test and on the pr we are ensuring we are using WarpSyncParams here

Here are the logs from said test

*2022-12-08 10:46:40 [//Eve (parachain)] target block reached*
2022-12-08 10:46:41 [//Eve (parachain)] ⏩ Warping, Downloading target block, 0.00 Mib (2 peers), best: #7 (0x2401…880f), finalized #6 (0x60b2…3c9e), ⬇ 0.2kiB/s ⬆ 0.7kiB/s    
2022-12-08 10:46:41 [//Charlie (relay chain)] 💤 Idle (4 peers), best: #29 (0x410c…917b), finalized #27 (0xc387…99ee), ⬇ 1.6kiB/s ⬆ 1.4kiB/s    
2022-12-08 10:46:41 [//Charlie (parachain)] ⏩ Warping, Downloading target block, 0.00 Mib (2 peers), best: #7 (0x2401…880f), finalized #6 (0x60b2…3c9e), ⬇ 0.6kiB/s ⬆ 0.1kiB/s    

[samelamin]

@bkchr @arkpar perhaps you can help with this

I got to the bottom of why the parachain was stuck on Importing state

It's because the verification of the authorities fail

Verifying 538(0xd6eb…a3b4) failed: Could not fetch authorities at 0x1b16dde459a4bc96742d24694c09f8b386ed5a7c43c7d17cca4d94c1512edd6d: Current state of blockchain has invalid authorities set   

Given we skipped the authorities for parachain warp sync, is there a simple way of skipping the verification?

[arkpar]

There should not be any calls to authorities when importing state data. Where does it originate from?

[samelamin]

There should not be any calls to authorities when importing state data. Where does it originate from?

After the sync completes, a call is made to on_blocks_processed which iterates over the results

The normal warp_sync goes here too but we then switch the sync mode to full and restart the process https://github.com/paritytech/substrate/blob/HEAD/client/network/sync/src/lib.rs#L2770-L2778

However, with the parachain warp sync there are no authorities so the import fails the verification

https://github.com/paritytech/substrate/blob/HEAD/client/consensus/common/src/import_queue.rs#L331-L348

[bkchr]

This should do it: https://github.com/paritytech/substrate/pull/13058

[samelamin]

This should do it: #13058

Thanks Basti, you are awesome!

Although that fixed it and the chain is now producing blocks, we are still getting a few errors. I will post them on the pr you raised

[bkchr]

@samelamin please revert c0ecb220d66dd8e7b1a5ee29831b776f4f18d024. This was a bad idea by me. Your current implementation would not work and I think for now we don't need this. Can you please also stop using force pushes, this destroys github reviews.

[bkchr]

/tip big

[substrate-tip-bot[bot]]

@bkchr ERROR: Error: Contributor did not properly post their account address.

Make sure the pull request description has: "{network} address: {address}". Error: Contributor did not properly post their account address.

Make sure the pull request description has: "{network} address: {address}". at parseContributorAccount (/usr/src/app/dist/tip.js:1:908) at onIssueComment (/usr/src/app/dist/bot.js:1:1120) at runMicrotasks () at processTicksAndRejections (node:internal/process/task_queues:96:5)

[samelamin]

@samelamin please revert c0ecb220d66dd8e7b1a5ee29831b776f4f18d024

reverted

[bkchr]

@samelamin please add your polkadot address to get some tip.

[samelamin]

@samelamin please add your polkadot address to get some tip.

added now

[samelamin]

@bkchr apologies, there is something weird happening with my merging, I am going to attempt to revert all commits the since i first merged master and start over

[bkchr]

bot merge

[paritytech-processbot[bot]]

Error: "Check reviews" status is not passing for https://github.com/paritytech/polkadot/pull/6334

[bkchr]

bot merge

[paritytech-processbot[bot]]

Error: "Check reviews" status is not passing for https://github.com/paritytech/cumulus/pull/1909

[bkchr]

bot merge

[bkchr]

/tip big

[substrate-tip-bot[bot]]

@bkchr ERROR: Error: Invalid tip size. Please specify one of small, medium, large. Error: Invalid tip size. Please specify one of small, medium, large. at getTipSize (/usr/src/app/dist/tip.js:1:559) at onIssueComment (/usr/src/app/dist/bot.js:1:1171) at runMicrotasks () at processTicksAndRejections (node:internal/process/task_queues:96:5)