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

Snowbridge: Synchronize from Snowfork repository #3761

Merged
merged 47 commits into from
Apr 2, 2024

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented Mar 20, 2024

This PR includes the following 2 improvements:

Ethereum Client

Author: @yrong

Original Upstream PRs

Description

The Ethereum client syncs beacon headers as they are finalized, and imports every execution header. When a message is received, it is verified against the import execution header. This is unnecessary, since the execution header can be sent with the message as proof. The recent Deneb Ethereum upgrade made it easier to locate the relevant beacon header from an execution header, and so this improvement was made possible. This resolves a concern @svyatonik had in our initial Rococo PR: #2522 (comment)

Inbound Queue

Author: @yrong

Original Upstream PR

Description

When the AH sovereign account (who pays relayer rewards) is depleted, the inbound message will not fail. The relayer just will not receive rewards.

Both these changes were done by @yrong, many thanks. ❤️

claravanstaden and others added 20 commits February 8, 2024 13:37
* pallet order

* fmt

* adds test for bridge order

* revert unintended change

---------

Co-authored-by: claravanstaden <Cats 4 life!>
* fix coverage

* fix codecov config

---------

Co-authored-by: claravanstaden <Cats 4 life!>
# Conflicts:
#	.github/workflows/check-labels.yml
#	.github/workflows/check-prdoc.yml
#	.github/workflows/check-workspace.yml
#	.github/workflows/gitspiegel-trigger.yml
#	.github/workflows/release-99_notif-published.yml
#	bridges/snowbridge/README.md
#	bridges/snowbridge/runtime/test-common/src/lib.rs
#	bridges/snowbridge/scripts/contribute-upstream.sh
#	cumulus/parachains/runtimes/assets/test-utils/src/test_cases_over_bridge.rs
#	cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/snowbridge.rs
#	polkadot/node/subsystem-bench/src/approval/mod.rs
#	polkadot/node/subsystem-bench/src/availability/mod.rs
#	polkadot/node/subsystem-bench/src/cli.rs
#	polkadot/node/subsystem-bench/src/core/environment.rs
#	polkadot/node/subsystem-bench/src/subsystem-bench.rs
* extract ethereum chain id

* remove cargo.toml

* fix chain id data type

* fix missing chain id param

* fix missing chain id param

* fix coverage and missing param

* fix CI

* fix CI

* revert scripts

* revert coverage jobs

* revert formatting

---------

Co-authored-by: claravanstaden <Cats 4 life!>
* Ignore rewarding when funds unavailable

* More tests

* Fix taplo

* Fix emulated test

* Improve comments

* Update bridges/snowbridge/pallets/inbound-queue/src/lib.rs

Co-authored-by: Vincent Geddes <vincent@snowfork.com>

* Fix format

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
* test e2e scripts in GA

* fix GA

* test current dir

* fix path

* install moreutils

* sponge

* install go

* export path

* try something else

* run nix

* switch branch

* nix develop

* check setup

* use nix setup

* e2e stack script

* e2e stack script

* run command

* check current dir

* correct dir

* correct dir

* just cd

* try something else

* rn e2e script

* adds submodules

* ls

* print vars

* remove ls

* cd instead of pushdir

* try checking out differently

* ls

* ls

* change checkout order

* check what is in a dir

* check what is in a dir

* check something

* check something

* try something

* try something

* try e2e script again

* remove ls

* cleanup script

* adds ssz gen

* install js components

* install js components

* fix dir

* adds output dir

* create dir

* try something else

* bump

* fix config paths

* test output dir

* adds wasm target

* check wasm target

* setup rustup in nix

* missing quote

* install rustup stable

* adds e2e script in again

* remove skip wasm flag

* comment out things

* start lodestar

* print out log

* print out log

* print out lodestar

* check geth version

* bump

* remove commented out code

* update go files

* adds smoketests

* create dir

* adjust scripts

* only redirect start-services.sh output

* only redirect start-services.sh output

* switch dir

* permissions

* check script

* allow service to be down

* bump

* last run, hopefully

* fix tests

* fixes

* bump

* fix

* check lodestar

* check lodestar

* check lodestar

* bump

* bump

* try again

* try again

* bump

* bump

* update go

* update go

* cleanup

* check what is happening now

* lodestar should run the background

* run init.sh

* check what happens with the download

* change OS check

* remove other steps to see what is happening

* bump

* run smoketests

* run smoketests

* cleanup jobs

* extract ethereum chain id

* remove cargo.toml

* fix chain id data type

* fix missing chain id param

* fix missing chain id param

* fix coverage and missing param

* fix CI

* fix CI

* revert scripts

* revert coverage jobs

* revert formatting

* check log

* fix log

* separate steps

* cleanup job

* remove running smoketests on a PR, only when merged

* update branch

* remove http endpoint from relayer

* update nightly, revert unrelated changes

* check rust version

* chaneg branch for testing

* adds beacon relay log

* increase slot time

* increase block time check

* lower block check count

* only run smoketests on merge to snowbridge

---------

Co-authored-by: claravanstaden <Cats 4 life!>
* Sync execution update on demand

* Fix updating LatestExecutionState

* Revert change unrelated

* Remove unused

* Remove execution header from update & Remove LatestExecutionState
* adds finalized header gap check

* fix comment

* fmt

* use constant for max gap

* remove redundant config

* fmt

* fix tests

* fix again

* fmt

* test works

* revert change

* Update bridges/snowbridge/pallets/ethereum-client/src/lib.rs

Co-authored-by: Vincent Geddes <vincent@snowfork.com>

* fmt

---------

Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Vincent Geddes <vincent@snowfork.com>
…ion header storage (#125)

* Remove execution header storage & submit inbound message with execution update

* Fix ci breaking

* More refactoring

* Update beacon test fixtures

* Format code

* Initialize storage with finalized checkpoint for benchmark

* Update weights include verify the execution proof

* Fix breaking test

* Remove fixture not in use

* Add detail for InvalidExecutionProof

* Initialize checkpoint for tests on demand

* Revert error as NotBootstrapped for submit_update_with_missing_bootstrap

* Add verify_execution_header tests back

* Add error test back

* Cleanup templates

* Update bridges/snowbridge/primitives/beacon/src/types.rs

Co-authored-by: Vincent Geddes <vincent@snowfork.com>

* Narrowed inputs

* Remove fields irrelevant from proof

* Update bridges/snowbridge/primitives/core/src/inbound.rs

Co-authored-by: Vincent Geddes <vincent@snowfork.com>

* Update bridges/snowbridge/pallets/ethereum-client/src/impls.rs

Co-authored-by: Vincent Geddes <vincent@snowfork.com>

* Polish

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
* fix smoketests ci

* remove merge from upstream

* first commit before pulling from master

---------

Co-authored-by: claravanstaden <Cats 4 life!>
@claravanstaden claravanstaden marked this pull request as ready for review March 21, 2024 08:47
@paritytech-review-bot paritytech-review-bot bot requested a review from a team March 21, 2024 08:48
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Have been mostly checking direct changes to pallet code - all looks great, thank you!

@@ -191,25 +174,6 @@ pub mod pallet {
pub(super) type NextSyncCommittee<T: Config> =
StorageValue<_, SyncCommitteePrepared, ValueQuery>;

/// Latest imported execution header
Copy link
Contributor

Choose a reason for hiding this comment

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

A migration for removal those values will be added later, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely. As soon as Multi-block Migrations are ready on BH.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this could be useful: #3828

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks great! Will that feature be OK even if all the storage items cannot be deleted in one block?

bridges/snowbridge/pallets/inbound-queue/src/lib.rs Outdated Show resolved Hide resolved
@@ -53,18 +36,11 @@ impl<T: Config> Verifier for Pallet<T> {
if !receipt.contains_log(&event_log) {
log::error!(
target: "ethereum-client",
"💫 Event log not found in receipt for transaction at index {} in block {}",
proof.tx_index, proof.block_hash,
"💫 Event log not found in receipt for transaction",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove log params? I think this log became pretty non-relevant and useless, because you also remove input log with block_hash Verifying message with block hash.
How will you investigate Event log not found in receipt for transaction without knowing block_hash or tx?

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 reasoning behind this is on the original PR, over here: Snowfork#125 (comment)

A side-note, we don't generally have access to the parachain logs either, so we try and solve issues without having to look at the logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, got it, and what about adding there some "identifier" from proof.execution_proof or proof.execution_proof.execution_header, does it make sense?

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 don't know if it would be useful. We have the same info in the relayer and that is more accessible to us.

@@ -191,25 +174,6 @@ pub mod pallet {
pub(super) type NextSyncCommittee<T: Config> =
StorageValue<_, SyncCommitteePrepared, ValueQuery>;

/// Latest imported execution header
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this could be useful: #3828

prdoc/pr_3761.prdoc Outdated Show resolved Hide resolved
@bkontur bkontur added the T15-bridges This PR/Issue is related to bridges. label Apr 2, 2024
@bkontur bkontur added this pull request to the merge queue Apr 2, 2024
Merged via the queue into paritytech:master with commit 5d9826c Apr 2, 2024
135 of 136 checks passed
Ank4n pushed a commit that referenced this pull request Apr 9, 2024
This PR includes the following 2 improvements:

## Ethereum Client

Author: @yrong 
### Original Upstream PRs
- Snowfork#123
- Snowfork#125

### Description
The Ethereum client syncs beacon headers as they are finalized, and
imports every execution header. When a message is received, it is
verified against the import execution header. This is unnecessary, since
the execution header can be sent with the message as proof. The recent
Deneb Ethereum upgrade made it easier to locate the relevant beacon
header from an execution header, and so this improvement was made
possible. This resolves a concern @svyatonik had in our initial Rococo
PR:
#2522 (comment)

## Inbound Queue

Author: @yrong 
### Original Upstream PR
- Snowfork#118

### Description
When the AH sovereign account (who pays relayer rewards) is depleted,
the inbound message will not fail. The relayer just will not receive
rewards.

Both these changes were done by @yrong, many thanks. ❤️

---------

Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Ron <yrong1997@gmail.com>
Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Apr 9, 2024
This PR includes the following 2 improvements:

## Ethereum Client

Author: @yrong 
### Original Upstream PRs
- Snowfork#123
- Snowfork#125

### Description
The Ethereum client syncs beacon headers as they are finalized, and
imports every execution header. When a message is received, it is
verified against the import execution header. This is unnecessary, since
the execution header can be sent with the message as proof. The recent
Deneb Ethereum upgrade made it easier to locate the relevant beacon
header from an execution header, and so this improvement was made
possible. This resolves a concern @svyatonik had in our initial Rococo
PR:
paritytech#2522 (comment)

## Inbound Queue

Author: @yrong 
### Original Upstream PR
- Snowfork#118

### Description
When the AH sovereign account (who pays relayer rewards) is depleted,
the inbound message will not fail. The relayer just will not receive
rewards.

Both these changes were done by @yrong, many thanks. ❤️

---------

Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Ron <yrong1997@gmail.com>
Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
@claravanstaden claravanstaden deleted the beacon-client-improvements branch May 24, 2024 19:22
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request Jun 19, 2024
This PR includes the following 2 improvements:

Author: @yrong
- #123
- #125

The Ethereum client syncs beacon headers as they are finalized, and
imports every execution header. When a message is received, it is
verified against the import execution header. This is unnecessary, since
the execution header can be sent with the message as proof. The recent
Deneb Ethereum upgrade made it easier to locate the relevant beacon
header from an execution header, and so this improvement was made
possible. This resolves a concern @svyatonik had in our initial Rococo
PR:
paritytech#2522 (comment)

Author: @yrong
- #118

When the AH sovereign account (who pays relayer rewards) is depleted,
the inbound message will not fail. The relayer just will not receive
rewards.

Both these changes were done by @yrong, many thanks. ❤️

---------

Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Ron <yrong1997@gmail.com>
Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
acatangiu pushed a commit that referenced this pull request Jun 25, 2024
This is a cherry-pick from master of
#3761, excluding PR
Snowfork#118

Expected patches for (1.7.0):
- name: snowbridge-pallet-ethereum-client
- name: snowbridge-pallet-inbound-queue
- name: snowbridge-beacon-primitives
- name: snowbridge-core
- name: snowbridge-runtime-test-common

---------

Co-authored-by: Ron <yrong1997@gmail.com>
Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request Jul 3, 2024
Upgrades Snowbridge to have a much more cost effective Ethereum light
client. In the current light client in production, all Ethereum
execution headers are stored, whether they contain a transfer message or
not. This PR upgrades Snowbridge to not store execution headers at all,
but provide the execution header proof alongside a message. Ethereum
beacon headers are still stored, to track finalized headers and do
ancestry proofs.

More details on the change on the original PR:
paritytech/polkadot-sdk#3761

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants