Skip to content

Sync finalized headers on demand #1206

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

Merged
merged 24 commits into from
Jun 20, 2024
Merged

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented May 24, 2024

Removing syncing finalized headers as they finalize. Only sync sync committee updates and interim finalized headers if the gap between headers are larger than 8192.

When receiving a message in the Ethereum relayer, check if there is a relevant on-chain finalized header to use for the ancestry proof, otherwise sync the finalized header in a batch call together with the message and its proof.

Resolves: SNO-1015

@claravanstaden claravanstaden marked this pull request as ready for review May 27, 2024 10:29
@@ -69,7 +69,7 @@ func (wr *ParachainWriter) Start(ctx context.Context, eg *errgroup.Group) error
return nil
}

func (wr *ParachainWriter) BatchCall(ctx context.Context, extrinsic string, calls []interface{}) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change function to allow calling different extrinsics.

@@ -71,7 +71,7 @@ func (h *Header) Sync(ctx context.Context, eg *errgroup.Group) error {

log.Info("starting to sync finalized headers")

ticker := time.NewTicker(time.Second * 10)
ticker := time.NewTicker(time.Minute * 1)
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 beacon relayer can check for new updates every minute, since we only sync sync committee updates. Can even make this even 10 minutes or so.

@@ -187,7 +186,7 @@ func (h *Header) SyncFinalizedHeader(ctx context.Context) error {
}
}

return h.updateFinalizedHeaderOnchain(ctx, update)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer sync the latest header on-chain.

@yrong
Copy link
Contributor

yrong commented May 27, 2024

We may also need some revamp for delivery_cost to take cost of finality update into consideration.

@claravanstaden
Copy link
Contributor Author

We may also need some revamp for delivery_cost to take cost of finality update into consideration.

That's a fair point. We haven't implemented relayer rewards for the consensus relayers like the beacon and beefy relayer, right? Perhaps we should consider implementing that soon and handle it with that work? The reason why I think so, instead of handling it in this PR is:

  • The goal with this PR is to make the beacon relayer less expensive to run.
  • We are trying to limit this change to an off-chain change.

@vgeddes would like your input here too.

@alistair-singh
Copy link
Contributor

alistair-singh commented May 30, 2024

We may also need some revamp for delivery_cost to take cost of finality update into consideration.

That's a fair point. We haven't implemented relayer rewards for the consensus relayers like the beacon and beefy relayer, right? Perhaps we should consider implementing that soon and handle it with that work? The reason why I think so, instead of handling it in this PR is:

  • The goal with this PR is to make the beacon relayer less expensive to run.
  • We are trying to limit this change to an off-chain change.

@vgeddes would like your input here too.

The execution relayer is sending a consensus update to "speed up" the transaction so that the message is delivered now instead of waiting 27 hours or so for the next consensus update. If the the execution relayer is paying for speed up then we should not re-imburse them as part of the delivery cost because technically they could wait ~27 hours and get free consensus delivery.

The issue is that waiting 27 hours is too long. We maybe should have a way to make the beacon relayer do intervals in between sync committee updates such that every 4 hours a consensus update occurs (matching BEEFY). That way the execution relayer can wait 4 hours or pay more, out of their own pocket, to speed up the message.

@claravanstaden
Copy link
Contributor Author

@alistair-singh great point! Added over here: 11aadb8

@claravanstaden
Copy link
Contributor Author

@yrong @alistair-singh I added a flag to disable sending a beacon header with the message, in case a channel wants to wait for the beacon relayer finalized header: 60d029c

@claravanstaden
Copy link
Contributor Author

@alistair-singh improved error messages here: 0c22608 The fallback methods hid the initial error.

Validation for config files here: 4a76149 It looks like Viper doesn't have this built in: spf13/viper#702 (comment)

Comment on lines +222 to +225
extrinsics := []string{"EthereumBeaconClient.submit", "EthereumInboundQueue.submit"}
payloads := []interface{}{proof.FinalizedPayload.Payload, inboundMsg}
// Batch the finalized header update with the inbound message
err := r.writer.BatchCall(ctx, extrinsics, payloads)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we use execution relayer to submit the beacon finality update.

I would suggest not do like this, in recent change of beefy relayer we add a one-shot command to sync finality update. Maybe we can follow the same pattern.

More context in #1217 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this implementation, the beacon relayer submits a finality update every 4(ish) hours: https://github.com/Snowfork/snowbridge/pull/1206/files#diff-92cc554d5b7218e7576839ca25a3b71ae0bbfa8200552beaf703a95a1a0da2a3R204

The execution header can be configured to either send a finality update with the message (for instant verification) or to wait for the beacon relayer to submit the finality update.

I would suggest not do like this, in recent change of beefy relayer we add a one-shot command to sync finality update. Maybe we can follow the same pattern.

We briefly spoke about this in the Monday meeting and decided against this since the beacon finality updates are much cheaper than the BEEFY ones. We can discuss it in our meeting later today if you'd like.

# Conflicts:
#	relayer/cmd/run/beacon/command.go
#	relayer/cmd/run/execution/command.go
#	relayer/relays/beacon/header/header.go
#	web/pnpm-lock.yaml
Copy link
Contributor

@yrong yrong left a comment

Choose a reason for hiding this comment

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

+1

@claravanstaden claravanstaden merged commit 3a0efb4 into main Jun 20, 2024
1 check passed
@claravanstaden claravanstaden deleted the finalized-headers-on-demand branch June 20, 2024 07:04
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.

3 participants