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

Initialize beacon light client from checkpoint #828

Merged
merged 12 commits into from
May 17, 2023
Merged

Initialize beacon light client from checkpoint #828

merged 12 commits into from
May 17, 2023

Conversation

yrong
Copy link
Contributor

@yrong yrong commented May 11, 2023

Refactoring including:

  • rename InitialUpdate to CheckPointUpdate
  • rename HeaderUpdate to ExecutionHeaderUpdate

Extrinsic added:

  • check_point_update
  • deactivate_bridge

Essentially InitialUpdate is just a CheckPoint for initialization, it's possible we do checkpoint any time at runtime(e.g. checkpoint after a routine maintenance or emergency which cause bridge stopped for a long time).

Snowfork/cumulus#23 required

Fixes: SNO-429, SNO-430

@yrong
Copy link
Contributor Author

yrong commented May 11, 2023

Seems there is no Sudo or Democracy pallet in cumlus bridge, so would assume check_point_update which requires sudo authority can only be triggered from relaychain by XCM.

To make e2e test easy, we keep the GenesisConfig.

@alistair-singh
Copy link
Contributor

requires sudo authority can only be triggered from relaychain by XCM

Working on a script for this. Will PR shortly.

@alistair-singh
Copy link
Contributor

Working on a script for this. Will PR shortly.

#829

@yrong
Copy link
Contributor Author

yrong commented May 12, 2023

@yrong yrong marked this pull request as ready for review May 15, 2023 03:05
core/package.json Outdated Show resolved Hide resolved
@vgeddes vgeddes changed the title SNO-429 Initialize beacon light client from checkpoint May 15, 2023
@yrong
Copy link
Contributor Author

yrong commented May 15, 2023

E2E setup seems a bit too slow require almost 4 minutes even skip building steps, so speedup in this commit decreased to almost 90 seconds.
5a1263c

@vgeddes
Copy link
Collaborator

vgeddes commented May 16, 2023

E2E setup seems a bit too slow require almost 4 minutes even skip building steps, so speedup in this commit decreased to almost 90 seconds.
5a1263c

I would rather not reduce the slot time. Clara reverted that optimization for several reasons I can't remember.

I don't think waiting 4 minutes for sync is too bad.

.gitmodules Outdated Show resolved Hide resolved
@yrong
Copy link
Contributor Author

yrong commented May 16, 2023

I would rather not reduce the slot time. Clara reverted that optimization for several reasons I can't remember.

It's different. Last time what we change is SLOTS_PER_EPOCH from 32 to 8 which will impact the generation of ancestry proof whereas change SECONDS_PER_SLOT won't.

.gitmodules Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
@vgeddes
Copy link
Collaborator

vgeddes commented May 16, 2023

I would rather not reduce the slot time. Clara reverted that optimization for several reasons I can't remember.

It's different. Last time what we change is SLOTS_PER_EPOCH from 32 to 8 which will impact the generation of ancestry proof whereas change SECONDS_PER_SLOT won't.

Does this mean geth will produce an execution header every 2 seconds? If so, this could cause the light client to fall behind importing execution headers every 2 seconds (due to the overhead of BLS signature verification).

@yrong
Copy link
Contributor Author

yrong commented May 16, 2023

Does this mean geth will produce an execution header every 2 seconds? If so, this could cause the light client to fall behind importing execution headers every 2 seconds (due to the overhead of BLS signature verification).

Yes, that's true. But the hack is for minimal spec and dev usage only. It did help for fast checking basic functions like beacon/beefy sync work as expected so I would prefer keep it.

Just add an env config here dc488f0 will not enable fast mode by default.

Copy link
Collaborator

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

+1

core/packages/test/scripts/xcm-helper.sh Outdated Show resolved Hide resolved
@yrong yrong merged commit 3382618 into main May 17, 2023
@yrong yrong deleted the ron/sno-429 branch May 17, 2023 02:58
@claravanstaden
Copy link
Contributor

@vgeddes @yrong Just mentioning, I would rather we not deviate from the official spec minimal config, since it a standard spec that all the client implementation uses. Speeding up those configs may save time with testing, but we lose time with development since it has wasted some time figuring out why some proofs fail, only to find because the spec is different. I'd wager that more cases like this will occur if we deviate from the standard spec in the future.

@vgeddes
Copy link
Collaborator

vgeddes commented May 18, 2023

@vgeddes @yrong Just mentioning, I would rather we not deviate from the official spec minimal config, since it a standard spec that all the client implementation uses. Speeding up those configs may save time with testing, but we lose time with development since it has wasted some time figuring out why some proofs fail, only to find because the spec is different. I'd wager that more cases like this will occur if we deviate from the standard spec in the future.

That's a good point @claravanstaden. I see that these config parameters are all tightly related, and decreasing the slot time may interfere with ancestry proofs and the SLOTS_PER_HISTORICAL_ROOT setting.

@yrong can you revert this slot change please. Since it is disabled by default, I think no one will actually use it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants