enable margin

[InoMurko]

📋 Add associated issues, tickets, docs URL here.

Overview

Describe what your Pull Request is about in a few sentences.

Changes

Describe your changes and implementation choices. More details make PRs easier to review.

• Change 1
• Change 2
• ...

Testing

Describe how to test your new feature/bug fix and if possible, a step by step guide on how to demo this.

[boolafish]

hmmm, I think the real bug is in this line:

elixir-omg/apps/omg/lib/omg/ethereum_event_listener/core.ex

Line 115 in ca108e0

|> max(sync_height)

It should be using min instead of max? The sync_height would take wait_for into account:

elixir-omg/apps/omg/lib/omg/root_chain_coordinator/core.ex

Lines 115 to 122 in ca108e0

	next_sync_height =
	config
	|> Keyword.get(:waits_for, [])
	|> get_height_of_awaited(state)
	|> consider_finality(configs[service_name], root_chain_height)
	|> min(current_sync_height + @maximum_leap_forward)
	|> min(root_chain_height)
	|> max(0)

However, since we are using max here, it will use the "root_chain_height" instead of the sync height that should be the real guidance.

The PR here is fixing this issue in an abusing way of how implementation works I think. It will solve the issue because the "root_chain_height" would take the finality margin into account.

elixir-omg/apps/omg/lib/omg/root_chain_coordinator/core.ex

Line 124 in ca108e0

finality_bearing_root = max(0, root_chain_height - finality_margin_for(config))

[InoMurko]

@boolafish you might be right, and fixing this probably makes sense. But since I looked into it, it would have a bigger impact then the approach in this PR.
This PR is actually verified - since that's how Watcher syncs.

So I suggest we go with what we have here and address/simplify Root Chain Coordinator at a later stage. WDYT

[pgebal]

hmmm, I think the real bug is in this line:

elixir-omg/apps/omg/lib/omg/ethereum_event_listener/core.ex

Line 115 in ca108e0

|> max(sync_height)

It should be using min instead of max? The sync_height would take wait_for into account:

elixir-omg/apps/omg/lib/omg/root_chain_coordinator/core.ex

Lines 115 to 122 in ca108e0

	next_sync_height =
	config
	|> Keyword.get(:waits_for, [])
	|> get_height_of_awaited(state)
	|> consider_finality(configs[service_name], root_chain_height)
	|> min(current_sync_height + @maximum_leap_forward)
	|> min(root_chain_height)
	|> max(0)

However, since we are using max here, it will use the "root_chain_height" instead of the sync height that should be the real guidance.

The PR here is fixing this issue in an abusing way of how implementation works I think. It will solve the issue because the "root_chain_height" would take the finality margin into account.

elixir-omg/apps/omg/lib/omg/root_chain_coordinator/core.ex

Line 124 in ca108e0

finality_bearing_root = max(0, root_chain_height - finality_margin_for(config))

@boolafish If we want to do that this way, we could remove root_chain_height from SyncGuide, since sync height is never greater than root_chain_height.

[unnawut]

The fix lgtm and we can continue the longer term fix in parallel 👍

[InoMurko]

The fix lgtm and we can continue the longer term fix in parallel 👍

#1624