Simplify beacon light client by vgeddes · Pull Request #839 · Snowfork/snowbridge

vgeddes · 2023-05-21T21:43:25Z

I've refactored the light client to conform more closely to the ALC consensus specs.

The light client now has a singlesubmit extrinsic for processing both finality and sync committee updates.

Everything is green and running fine: Unit tests, benchmarks, relaying, E2E env

Storage

The storage in the light client is also simpler. These new storage items have replaced other ones:

FinalizedBeaconState: some necessary beacon state, indexed by finalized block root.
CurrentSyncCommittee: sync committee for current session
NextSyncCommittee: sync committee for next session

It is no longer necessary to store historical sync committees, so I have removed that storage map.

Removed Features

I've decided to remove weak subjectivity checks from the scope of our initial release. I think the problem of dealing with long-range attacks has turned out to be quite a bit more complex than we anticipated. Then, given what we know now about the security flaws in the ALC protocol, long-range attacks are no longer the most pressing security vulnerability.

Since the current code in pallet was incomplete, and still needed to support governance-backed re-activation of the bridge, I felt it best to just remove it all for now.

Development Experience

Finally, the development experience is more straightforward. I've removed the minimal feature flag, and replaced it with a beacon-spec-mainnet flag. So I've inverted the behavior, and this ends up actually working a lot better.

When running unit tests, we usually want to runt the minimal spec tests, as those are extensive and easier to maintain and troubleshoot. So I've enabled the miminal spec tests by default, and you don't need to pass any feature flags to run them.

cargo test

However, when working with benchmarks, we always need to use the mainnet spec. So when the runtime-benchmarks feature is enabled, the beacon-spec-mainnet is automatically enabled too.

So can just run benchmark tests using:

cargo test --features runtime-benchmarks

Fixes: SNO-446, SNO-439

relayer/cmd/generate_beacon_data.go

yrong · 2023-05-23T04:17:22Z

parachain/pallets/ethereum-beacon-client/src/lib.rs

-	pub(super) type FinalizedBeaconHeaderStates<T: Config> =
-		StorageValue<_, BoundedVec<FinalizedHeaderState, T::MaxFinalizedHeadersToKeep>, ValueQuery>;


FinalizedHeaderState used here is for pruning obsolete data in FinalizedBeaconState. I would prefer we keep it.

snowbridge/parachain/pallets/ethereum-beacon-client/src/lib.rs

Lines 786 to 790 in 8f6ed31

let oldest = b_vec.remove(0);

// Removing corresponding finalized header data of popped slot

// as that data will not be used by relayer anyway.

<FinalizedBeaconHeadersBlockRoot<T>>::remove(oldest.beacon_block_root);

<FinalizedBeaconHeaders<T>>::remove(oldest.beacon_block_root);

Resolved in #840

Well, after thinking about the pruning algorithm some more, I don't think we should be using FinalizedBeaconHeaderStates for pruning. That was a mistake, I believe.

FinalizedBeaconHeaderStates was originally introduced to help the offchain relayer generate ancestry proofs for execution headers updates. The storage item only needed contain the last few dozen recently imported finalized header states.

Then for pruning, we decided to reuse FinalizedBeaconHeaderStates for another purpose - to determine an older finalized header state we could prune. This was a mistake.

However this introduces a very bad constraint. If we want to keep the last months of beacon headers, then we need to store a month of state in FinalizedBeaconHeaderStates. But this storage item is a vector. We can't keep so much data in a vector in storage. This will eventually cause weight problems.

So I want to us to eliminate FinalizedBeaconHeaderStates. I don't think its necessary. I'm certain we can change the algorithm in the relayer to not rely on it.

And for pruning finalized beacon state, that's not a priority right now. We can introduce the Ring Buffer approach in another PR. But not right now - I want us to focus on these cleanups first. We can revisit pruning at a later time.

Yes, that make sense. Just remove pruing stuff.

If we want to keep the last months of beacon headers

I think no need to cache that much data, based on the calculateClosestCheckpointSlot algorithm here checkpoint only valid in range of 1 sync period(i.e. 27.3 hour). Consider finalized_update changes very infrequently(i.e. almost 12mins) in mainnet, that means 27*60/12=135 checkpoints at most.

Another reason is since we've removed SyncCommittees Map storage with only the current/next SyncCommittee. That means we can only verify updates in recent sync period, cache a lot of history headers does not make sense any more.

Any way, I agree we can make this PR focus on ALC spec compliant and revisit pruning at a later time.

I think no need to cache that much data, based on the calculateClosestCheckpointSlot algorithm here checkpoint only valid in range of 1 sync period(i.e. 27.3 hour). Consider finalized_update changes very infrequently(i.e. almost 12mins) in mainnet, that means 27*60/12=135 checkpoints at most.

If a finalized checkpoint is always the first slot in a finalized epoch, then I think it is not necessary for the relayer to need the FinalizedBeaconHeaderSlotsCache storage . I've added more details in SNO-449.

claravanstaden

This is awesome! 🙌

claravanstaden · 2023-05-23T10:49:44Z

parachain/pallets/ethereum-beacon-client/tests/fixtures/execution-header-update.minimal.json

@@ -0,0 +1,43 @@
+{
+  "header": {


I wonder if this shouldn't be beacon_header, for clarity?

claravanstaden · 2023-05-23T10:51:30Z

parachain/primitives/beacon/src/updates.rs

 	serde(deny_unknown_fields, bound(serialize = ""), bound(deserialize = ""))
 )]
 pub struct ExecutionHeaderUpdate {
+	/// Header for the beacon block containing the execution payload


Nice! I neglected comments like these, apologies. 😄

claravanstaden · 2023-05-23T10:55:57Z

relayer/cmd/generate_beacon_data.go

+	SyncCommitteeUpdate   beaconjson.Update
+	FinalizedHeaderUpdate beaconjson.Update


Can we use only Update in the relayer as well and remove one of these?

we need both. The relayer syncing algorithm hasn't changed at all. Only the data structures we send to the light client

claravanstaden · 2023-05-23T10:56:29Z

relayer/cmd/generate_beacon_data.go

 const (
 	pathToBeaconBenchmarkData    = "parachain/pallets/ethereum-beacon-client/src/benchmarking"
-	pathToBenchmarkDataTemplate  = "parachain/templates/beacon_benchmarking_data.rs.mustache"
+	pathToBenchmarkDataTemplate  = "parachain/templates/benchmark-fixtures.mustache"


I wonder if we shouldn't indicate that this is beacon data related?

claravanstaden · 2023-05-23T10:57:03Z

relayer/cmd/generate_beacon_data.go

 		checkPointBytes, _ := types.EncodeToBytes(checkPointScale)
 		// Call index for EthereumBeaconClient.force_checkpoint
-		checkPointCallIndex := "0x3201"
+		checkPointCallIndex := "0x3200"


What does this do?

Call index for extrinsic force_checkpoint, we use it for beacon client provisioning, replace the genesis config before.

claravanstaden · 2023-05-23T10:58:20Z

relayer/relays/beacon/header/header.go

+	// lastFinalizedHeader, err := h.writer.GetLastFinalizedHeaderState()
+	// if err != nil {
+	// 	return fmt.Errorf("fetch last finalized header from parachain: %w", err)
+	// }
+	// lastFinalizedHeaderPeriod := h.syncer.ComputeSyncPeriodAtSlot(lastFinalizedHeader.BeaconSlot)

 	// Period + 1 because the sync committee update contains the next period's sync committee
-	if lastSyncedSyncCommitteePeriod != period+1 {
-		return fmt.Errorf("synced committee period %d not imported successfully", lastSyncedSyncCommitteePeriod)
-	}
+	// if lastFinalizedHeaderPeriod != period+1 {
+	// 	return fmt.Errorf("synced committee period %d not imported successfully", lastFinalizedHeaderPeriod)
+	// }


Can we remove this? Is this because we have a combined header and sync committee period submit extrinsic now?

claravanstaden · 2023-05-23T10:59:51Z

relayer/relays/beacon/header/syncer/json/beacon_json.go

-	SignatureSlot           uint64        `json:"signature_slot"`
-	BlockRootsRoot          string        `json:"block_roots_root"`
-	BlockRootBranch         []string      `json:"block_roots_branch"`
+type Update struct {


What would the frequency of this update to the beacon client be? Trying to wrap my head around what the change means practically. 😄

Since SyncCommitteeUpdate is just a FinalizedUpdate with one extra field NextSyncCommittee, we merge these two data types together In this PR and frequency is just same as the previous FinalizedUpdate.

I haven't actually changed the syncing algorithm much at all. Only the data structures and the light client APIs the relayer talks to. The syncing algorithm used by the relayer remains compatible with the new design. Which is great.

When the relayer needs to submit a new sync committee, it sends this Update data structure. And the same for when it only needs to submit a new finalized header.

Great, thanks for the explanation @vgeddes and @yrong!

* Strict follows ALC * Polish * Merge recent change * Fix tests * Polish * Generate all updates in same command * Sync version of lodestar * Clean code * Add more checks when generate test data

yrong

Awesome!

vgeddes added 10 commits May 20, 2023 14:03

Improve ancestry proofs

d3d51aa

foo

17e4e25

foo

1e65d42

refactor tests

0927782

Update relayer

d4a5aac

get everything working again

a673610

disable husky temporarily

f15128d

update cumulus branch

2ab4242

refactor

df4779f

remove unused fixtures

5c0f8b6

vgeddes requested a review from yrong May 21, 2023 21:43

Simplify LatestFinalizedHeader

56718c4

yrong reviewed May 22, 2023

View reviewed changes

relayer/cmd/generate_beacon_data.go Show resolved Hide resolved

vgeddes added 13 commits May 22, 2023 16:37

Update benchmarks

b436491

update relayer to work with LatestFinalizedBlockRoot storage item

3cd8c74

update cumulus

7d7effc

Improve beacon client benchmarks

0eedb31

test something with gitub actions

afc7084

reduce stack use

a46ee08

foo

1ed599d

foo

16988ee

increase stack size

c99ce21

foo

e61406d

foo

32f189a

foo

a0c5df0

add newline

ffc6f36

yrong reviewed May 23, 2023

View reviewed changes

vgeddes mentioned this pull request May 23, 2023

Follow consensus-specs sync protocol more closely #840

Merged

claravanstaden approved these changes May 23, 2023

View reviewed changes

Follow consensus-specs sync protocol more closely (#840)

2156d17

* Strict follows ALC * Polish * Merge recent change * Fix tests * Polish * Generate all updates in same command * Sync version of lodestar * Clean code * Add more checks when generate test data

yrong approved these changes May 23, 2023

View reviewed changes

Update benchmarking template

f432201

vgeddes mentioned this pull request May 23, 2023

Simplify beacon light client Snowfork/cumulus#28

Merged

update cumulus

2614267

vgeddes merged commit 789fa3e into main May 23, 2023

vgeddes deleted the vincent/improve-ancestry-proofs branch May 23, 2023 16:20

nazar-pc mentioned this pull request Jun 16, 2023

Upgrade Substrate (with core-evm-relay removed) autonomys/subspace#1549

Merged

1 task

		pub(super) type FinalizedBeaconHeaderStates<T: Config> =
		StorageValue<_, BoundedVec<FinalizedHeaderState, T::MaxFinalizedHeadersToKeep>, ValueQuery>;

	let oldest = b_vec.remove(0);
	// Removing corresponding finalized header data of popped slot
	// as that data will not be used by relayer anyway.
	<FinalizedBeaconHeadersBlockRoot<T>>::remove(oldest.beacon_block_root);
	<FinalizedBeaconHeaders<T>>::remove(oldest.beacon_block_root);

		SyncCommitteeUpdate beaconjson.Update
		FinalizedHeaderUpdate beaconjson.Update

Conversation

vgeddes commented May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Storage

Removed Features

Development Experience

Uh oh!

Uh oh!

yrong May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrong May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claravanstaden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrong May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgeddes May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vgeddes commented May 21, 2023 •

edited

Loading

yrong May 23, 2023 •

edited

Loading

yrong May 23, 2023 •

edited

Loading

yrong May 23, 2023 •

edited

Loading

vgeddes May 23, 2023 •

edited

Loading