Manual sync committee update #1164

claravanstaden · 2024-03-27T14:28:51Z

If the sync committee update data retrieval fails (most likely because a beacon state is not available), build an update using the standard Beacon API endpoints.
Splits the protocol settings and methods into its own package.
Adds tests for the beacon data store.
First try to get an exact beacon state matching the slot provided, and if it can't be found, provide the best next update

claravanstaden

Apologies for the large change - the biggest cause of the change is moving the protocol package out.

relayer/relays/beacon/config/config.go

relayer/relays/beacon/header/header.go

claravanstaden · 2024-04-04T09:13:20Z

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

+		}
+		beaconState, err := s.unmarshalBeaconState(uint64(finalizedHeader.Slot), beaconStateData)
+
+		blockRootsProof, err = s.GetBlockRootsFromState(beaconState)


If the state is not available, check if it perhaps available in the beacon store.

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

yrong · 2024-04-09T15:53:01Z

If the sync committee update data retrieval fails (most likely because a beacon state is not available), build an update using the standard Beacon API endpoints.

That's Cool. Considering now we can always get a valid finality update from API endpoints, is the db option still necessary?

yrong · 2024-04-09T15:59:49Z

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


-	attestedSlot, err := s.findAttestedAndFinalizedHeadersAtBoundary(attestedSlot, lastSyncedFinalizedSlot)
+	attestedSlot, err := s.FindOldestAttestedHeaderAtInterval(slot, boundary)


Just notice there are 2 methods doing things similar:

FindLatestAttestedHeadersAtInterval vs FindOldestAttestedHeaderAtInterval

So can the search here be replaced with s.FindLatestAttestedHeadersAtInterval(boundry, slot),
or maybe we can try to merge the two functions into one?

The methods do similar but different things. FindLatestAttestedHeadersAtInterval finds the latest available slot, while FindOldestAttestedHeaderAtInterval finds the oldest available slot. I have merged the common code into a method called findValidUpdatePair here: https://github.com/Snowfork/snowbridge/pull/1164/files#diff-c589b232c311e67a8690c53fe1886c463a185178fde8ed4aa9dd2def499603b3R578. I think the 2 methods are now small and different enough to warrant 2 functions.

Yeah, I understand that.

Just curious seems we use FindLatestAttestedHeadersAtInterval for syncInterimFinalizedUpdate while FindOldestAttestedHeaderAtInterval for SyncCommitteePeriodUpdate, is there a special consideration for that?

Or be more specific can the search s.FindOldestAttestedHeaderAtInterval(slot, boundary) be replaced with s.FindLatestAttestedHeadersAtInterval(boundry, slot) and always search for the latest finality?

I started typing a response and never sent it. 😅 The reason why these two are different in my opinion is:

Finalized Update: Ideally want a newer update, so that it can cover a "larger" range of SLOTS_PER_HISTORICAL_ROOT (meaning, we want a finalized header as far from the previous finalized header) as we can.

Finalized Update with Sync Committee: for this case, an earlier update is better since we'd like to update the next sync committee as soon as we can.

Let me know if you think this is kinda unnecessary, and if we should just use the latest header we can (and scrap the "earlier" update for Finalized Update with Sync Committee.

claravanstaden · 2024-04-10T09:57:29Z

That's Cool. Considering now we can always get a valid finality update from API endpoints, is the db option still necessary?

The fallback finality update uses the DB option as a fallback as well. I would feel more comfortable with having the DB option as a backup. Do you have concerns with it?

yrong · 2024-04-10T14:16:02Z

I would feel more comfortable with having the DB option as a backup. Do you have concerns with it?

So we'll still stick to lodestar with --chain.archiveStateEpochFrequency=1 which already saves finality state every epoch, right?

Not much concern. Just from the perspective of DevOps seems we need a separate cloud instance to run the command storing beacon state periodically for high availability, then there is some maintenance cost.

claravanstaden · 2024-04-12T17:24:40Z

So we'll still stick to lodestar with --chain.archiveStateEpochFrequency=1 which already saves finality state every epoch, right?

Yes. 😄

Not much concern. Just from the perspective of DevOps seems we need a separate cloud instance to run the command storing beacon state periodically for high availability, then there is some maintenance cost.

I think it can be a simple cronjob on the EC2 server that will run the relayer anyway. Agreed on the cost. We can always remove it, but having it for launch gives me great assurance.

yrong · 2024-04-13T02:13:07Z

I think it can be a simple cronjob on the EC2 server that will run the relayer anyway

IMHO the cronjob make sense only on a seperate EC2 instance, it's for backup when the EC2 server running lodestar is down.

So I'd assume it points to a different beacon node for which maybe we can run a backup lodestar with the mock options --execution.engineMock --eth1=false

In this way we have a more reliable fallback option but it also means more cost.

claravanstaden · 2024-04-15T13:31:29Z

I think it can be a simple cronjob on the EC2 server that will run the relayer anyway

IMHO the cronjob make sense only on a seperate EC2 instance, it's for backup when the EC2 server running lodestar is down.

So I'd assume it points to a different beacon node for which maybe we can run a backup lodestar with the mock options --execution.engineMock --eth1=false

In this way we have a more reliable fallback option but it also means more cost.

I had more the scenario in mind where just the beacon relayer is down (perhaps restarting because of a bug or some other issue) like we had that weekend where the beacon relayer was down because it could not find the beacon state. In that case, it would have been fine to have a cron just download beacon states every now and then.

I am also not sure if we will be able to justify running 2 EC instances just for this backup, especially if other people also start running relayers.

yrong · 2024-04-16T02:55:37Z

Yeah, I'm also not keen to add another instance for this. Just in case when the instance is down for a while(i.e. including relayer, lodestar, db) seems we don't have a rescue solution, do we?

Anyway, this PR is very nice to provide another reliable fallback option so should be good to merge.

yrong

Cool!

# Conflicts: # relayer/cmd/store_beacon_state.go

* adds sync committee to update method * progress * manual sync committee update * fix tests * cleanup * remove unnecessary check * simplify checkpoint populate * fix method --------- Co-authored-by: claravanstaden <Cats 4 life!>

claravanstaden added 5 commits March 25, 2024 21:01

adds sync committee to update method

1fe8245

progress

faa316d

manual sync committee update

0fe21b2

fix tests

5481db5

cleanup

98ef98e

claravanstaden marked this pull request as ready for review April 4, 2024 09:18

claravanstaden commented Apr 4, 2024

View reviewed changes

claravanstaden requested review from yrong, vgeddes and alistair-singh April 4, 2024 09:19

yrong reviewed Apr 8, 2024

View reviewed changes

relayer/relays/beacon/header/syncer/syncer.go Outdated Show resolved Hide resolved

claravanstaden added 3 commits April 9, 2024 12:57

remove unnecessary check

9cbee1d

simplify checkpoint populate

2ab4ddf

fix method

fc89226

yrong reviewed Apr 9, 2024

View reviewed changes

claravanstaden mentioned this pull request Apr 12, 2024

Optimize finalized header storage #1175

Closed

yrong approved these changes Apr 17, 2024

View reviewed changes

Merge branch 'main' into manual-sync-committee-update

48e1756

# Conflicts: # relayer/cmd/store_beacon_state.go

claravanstaden merged commit 7f41605 into main Apr 18, 2024
1 check passed

claravanstaden deleted the manual-sync-committee-update branch April 18, 2024 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manual sync committee update #1164

Manual sync committee update #1164

claravanstaden commented Mar 27, 2024 •

edited

Loading

claravanstaden left a comment

claravanstaden Apr 4, 2024

yrong commented Apr 9, 2024

yrong Apr 9, 2024 •

edited

Loading

claravanstaden Apr 10, 2024

yrong Apr 10, 2024 •

edited

Loading

claravanstaden Apr 17, 2024

claravanstaden commented Apr 10, 2024

yrong commented Apr 10, 2024 •

edited

Loading

claravanstaden commented Apr 12, 2024

yrong commented Apr 13, 2024 •

edited

Loading

claravanstaden commented Apr 15, 2024

yrong commented Apr 16, 2024 •

edited

Loading

yrong left a comment


		attestedSlot, err := s.findAttestedAndFinalizedHeadersAtBoundary(attestedSlot, lastSyncedFinalizedSlot)
		attestedSlot, err := s.FindOldestAttestedHeaderAtInterval(slot, boundary)

Manual sync committee update #1164

Manual sync committee update #1164

Conversation

claravanstaden commented Mar 27, 2024 • edited Loading

claravanstaden left a comment

Choose a reason for hiding this comment

claravanstaden Apr 4, 2024

Choose a reason for hiding this comment

yrong commented Apr 9, 2024

yrong Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

claravanstaden Apr 10, 2024

Choose a reason for hiding this comment

yrong Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

claravanstaden Apr 17, 2024

Choose a reason for hiding this comment

claravanstaden commented Apr 10, 2024

yrong commented Apr 10, 2024 • edited Loading

claravanstaden commented Apr 12, 2024

yrong commented Apr 13, 2024 • edited Loading

claravanstaden commented Apr 15, 2024

yrong commented Apr 16, 2024 • edited Loading

yrong left a comment

Choose a reason for hiding this comment

claravanstaden commented Mar 27, 2024 •

edited

Loading

yrong Apr 9, 2024 •

edited

Loading

yrong Apr 10, 2024 •

edited

Loading

yrong commented Apr 10, 2024 •

edited

Loading

yrong commented Apr 13, 2024 •

edited

Loading

yrong commented Apr 16, 2024 •

edited

Loading