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 - Ethereum Client - Reject finalized updates without a sync committee in next store period #4478

Merged
merged 8 commits into from
May 16, 2024

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented May 16, 2024

While syncing Ethereum consensus updates to the Snowbridge Ethereum light client, the syncing process stalled due to error InvalidSyncCommitteeUpdate when importing the next sync committee for period 1087.

This bug manifested specifically because our light client checkpoint is a few weeks old (submitted to governance weeks ago) and had to catchup until a recent block. Since then, we have done thorough testing of the catchup sync process.

Symptoms

  • Import next sync committee for period 1086 (essentially period 1087). Light client store period = 1086.
  • Import header in period 1087. Light client store period = 1087. The current and next sync committee is not updated, and is now in an outdated state. (current sync committee = 1086 and current sync committee = 1087, where it should be current sync committee = 1087 and current sync committee = None)
  • Import next sync committee for period 1087 (essentially period 1088) fails because the expected next sync committee's roots don't match.

Bug

The bug here is that the current and next sync committee's didn't handover when an update in the next period was received.

Fix

There are two possible fixes here:

  1. Correctly handover sync committees when a header in the next period is received.
  2. Reject updates in the next period until the next sync committee period is known.

We opted for solution 2, which is more conservative and requires less changes.

Polkadot-sdk versions

This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: Snowfork#145

claravanstaden and others added 3 commits May 16, 2024 09:01
#145)

* sync committee handover fix

* remove comment

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

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>

* add extra check

* move checks around

* remove extra validation

* fix compiler error

* reject updates in next period without sync committee

* fix test

* polish

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

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
@claravanstaden claravanstaden marked this pull request as ready for review May 16, 2024 07:38
@claravanstaden
Copy link
Contributor Author

@vgeddes @yrong @alistair-singh please review.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 16, 2024 07:39
Copy link
Contributor

@alistair-singh alistair-singh left a comment

Choose a reason for hiding this comment

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

+1

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.

The change itself is pretty clear and the issue is very similar to what we have in K<>P bridge - we've accepted the header without updating the authorities set (sync committee).

assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone()));
let block_root: H256 = update.finalized_header.clone().hash_tree_root().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it an intended removal? It looks like it should be fine even after the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Added back in 9fdcef3.

@svyatonik svyatonik added the T15-bridges This PR/Issue is related to bridges. label May 16, 2024
@acatangiu acatangiu enabled auto-merge May 16, 2024 13:47
Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

when merged, we should backport to the all other release branches:

  • release-crates-io-v1.7.0 - patch release the fellows BridgeHubs runtimes - @claravanstaden do you want to do it?
  • release-crates-io-v1.8.0
  • release-crates-io-v1.9.0
  • release-crates-io-v1.10.0
  • release-crates-io-v1.11.0
  • release-crates-io-v1.12.0 (commit soon)

@acatangiu acatangiu added this pull request to the merge queue May 16, 2024
@claravanstaden
Copy link
Contributor Author

claravanstaden commented May 16, 2024

when merged, we should backport to the all other release branches:

  • release-crates-io-v1.7.0 - patch release the fellows BridgeHubs runtimes - @claravanstaden do you want to do it?
  • release-crates-io-v1.8.0
  • release-crates-io-v1.9.0
  • release-crates-io-v1.10.0
  • release-crates-io-v1.11.0
  • release-crates-io-v1.12.0 (commit soon)

@bkontur I'll do all of the above, yes. I need to update the Ethereum client storage in the runtimes PR too.

Merged via the queue into paritytech:master with commit 943eb46 May 16, 2024
152 of 154 checks passed
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request May 16, 2024
…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: #145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request May 16, 2024
…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: #145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
@claravanstaden claravanstaden deleted the ethereum-client-fix branch May 16, 2024 14:34
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request May 16, 2024
…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: #145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request May 16, 2024
…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: #145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request May 16, 2024
…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: #145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
acatangiu pushed a commit that referenced this pull request May 17, 2024
…c committee in next store period (#4485)

This is a cherry-pick from master of
#4478

Expected patches for (1.10.0):
snowbridge-pallet-ethereum-client

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
acatangiu pushed a commit that referenced this pull request May 17, 2024
…c committee in next store period (#4483)

This is a cherry-pick from master of
#4478

Expected patches for (1.8.0):
snowbridge-pallet-ethereum-client

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
acatangiu pushed a commit that referenced this pull request May 17, 2024
…c committee in next store period (#4484)

This is a cherry-pick from master of
#4478

Expected patches for (1.9.0):
snowbridge-pallet-ethereum-client

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
acatangiu pushed a commit that referenced this pull request May 17, 2024
…c committee in next store period (#4486)

This is a cherry-pick from master of
#4478

Expected patches for (1.11.0):
snowbridge-pallet-ethereum-client

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: Snowfork#145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: Snowfork#145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.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.

6 participants