Sync execution update on demand

[yrong]

No description provided.

[claravanstaden]

Do we need to verify and store these executions at all? Because I see in https://github.com/Snowfork/snowbridge/pull/1154/files#diff-ba222152cf61a695305ad44e12ccc757ffa95a1f6c1c8ee698e21a7650155229R165 we sync the execution header containing the message, without checking if has been synced already. We also do not check if a header was already stored in process_execution_header_update, which we maybe should add, i.e. check if it exists in storage.

[yrong]

I think it's still nessessary. As you can see we fetchEthereumNonce
https://github.com/Snowfork/snowbridge/blob/e8557c79fa5e20142cb6ae053ce33a3f3a517623/relayer/relays/execution/main.go#L116 at the LastExecutionHeaderState which should be a state finalized.

[claravanstaden]

Ah I see, good point. 👍🏻

What do you think about potentially importing the same execution header twice? Maybe we should check if it exists in process_execution_header_update?

[yrong]

Potentially, and I add some extra check here
https://github.com/Snowfork/snowbridge/blob/e8557c79fa5e20142cb6ae053ce33a3f3a517623/relayer/relays/beacon/header/header.go#L353-L360 to make sure SyncExecutionHeader work as expected.

[claravanstaden]

That check isn't related to checking if the header has already been imported on the light client side though. I think in process_execution_header_update we should check if the execution header has already been imported, before we try and verify it. What do you think?

[yrong]

I prefer we do the check on relayer side, Snowfork/snowbridge@af93289

[vgeddes]

I think it's still nessessary. As you can see we fetchEthereumNonce
https://github.com/Snowfork/snowbridge/blob/e8557c79fa5e20142cb6ae053ce33a3f3a517623/relayer/relays/execution/main.go#L116 at the LastExecutionHeaderState which should be a state finalized.

Since execution headers can now be (re)imported in arbitrary order, ie not in sequence, I don't think its safe for the light client to maintain a LastExecutionHeaderState storage item.

For example, an attacker can import execution header number 0 into the light client and then effectively force other relayers to resume relaying execution headers after number 0, since they are using LastExecutionHeaderState to know when to relay.

Similarly, each execution relayer only relays messages for its own channel. And so we need the ability for different execution relayers to be able to resume relaying from the correct ethereum block.

Just a rough idea - Simple solution I think is for the relayers to store a local file on disk containing the block number they need to resume relaying from. And they can also query the channel nonces on chain, to make sure the block number in the file points to the last sync'ed nonce.

[yrong]

An attacker can import execution header number 0 into the light client and then effectively force other relayers to resume relaying execution headers after number 0

Nice catch, made a fix in 6d8536d

Also good point about remove LastExecutionHeaderState storage item completely and the relayers to store a local file on disk containing the block number they need to resume relaying from, but I'd rather improve that in a separate PR later.

[vgeddes]

Thanks Ron, though 6d8536d doesn't solve all the problems with LastExecutionHeaderState.

Also, circling back to the original comments in this thread:

Clara: Do we need to verify and store these executions at all?

Ron: I think it's still nessessary. As you can see we fetchEthereumNonce
https://github.com/Snowfork/snowbridge/blob/e8557c79fa5e20142cb6ae053ce33a3f3a517623/relayer/relays/execution/main.go#L116 at the LastExecutionHeaderState which should be a state finalized.

My understanding from this that we are including the execution header in the Update struct passed to submit(update), purely so that LastExecutionHeaderState can be initialized? If that's the case, we should not do that because LastExecutionHeaderState needs to be removed in any case.

Or is there another reason why submit_execution_header cannot be the only way of submitting execution headers?

[yrong]

b82df5b for the light client and Snowfork/snowbridge@ab191f9 for the relayer.

Just remove the LastExecutionState on chain and update the relaying algorithm to not depend on it. It did clean up the codebase a lot.

Some more details in https://github.com/Snowfork/snowbridge/pull/1154/files#r1524053814 for the change on relayer side.

Btw: @vgeddes I'm not sure we still need a local file system for that, please add more details if that's required.

[claravanstaden]

Looks great Ron! I have two comments, and then I just want to clarify, this doesn't affect the execution headers stored in the ring buffer, right?

[vgeddes]

I think it's still nessessary. As you can see we fetchEthereumNonce
https://github.com/Snowfork/snowbridge/blob/e8557c79fa5e20142cb6ae053ce33a3f3a517623/relayer/relays/execution/main.go#L116 at the LastExecutionHeaderState which should be a state finalized.

Since execution headers can now be (re)imported in arbitrary order, ie not in sequence, I don't think its safe for the light client to maintain a LastExecutionHeaderState storage item.

For example, an attacker can import execution header number 0 into the light client and then effectively force other relayers to resume relaying execution headers after number 0, since they are using LastExecutionHeaderState to know when to relay.

Similarly, each execution relayer only relays messages for its own channel. And so we need the ability for different execution relayers to be able to resume relaying from the correct ethereum block.

Just a rough idea - Simple solution I think is for the relayers to store a local file on disk containing the block number they need to resume relaying from. And they can also query the channel nonces on chain, to make sure the block number in the file points to the last sync'ed nonce.

[claravanstaden]

Nice, much simpler! ✨