Change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS #1491

souradeep-das · 2020-04-27T23:01:30Z

Overview

This unifies sla_margin to hold values in seconds instead of the number of blocks and renames EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS. The internal logic to detect invalid exits has also been changed to use timestamps instead of number of blocks

EXIT_PROCESSOR_SLA_MARGIN_FORCED hasn't been renamed

Changes

EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS
Changing eth_height_now to eth_timestamp_now
Changed logic to work with seconds
Modified Tests
EXIT_PROCESSOR_SLA_MARGIN_FORCED is unchanged till now

souradeep-das · 2020-04-27T23:08:42Z

apps/omg_watcher/lib/omg_watcher/exit_processor/standard_exit.ex

    # get exits which are still invalid and after the SLA margin
    late_invalid_exits =
      invalid_exits
-      |> Enum.filter(fn {_, %ExitInfo{eth_height: eth_height}} -> eth_height + sla_margin <= eth_height_now end)
+      |> Enum.filter(fn {_, %ExitInfo{eth_height: eth_height}} ->
+        eth_height + sla_seconds / ethereum_block_time_seconds <= eth_height_now


Is this a good way or should the timestamp of each exit be stored in the DB and retrieved here to check if sla_seconds is elapsed

I think this is fine for this PR since it's still the existing behaviour, but could you maybe raise this discussion separately in the engineering channel?

will change the logic to exit_timestamp + sla_seconds <= time_now when #1495 is merged

coveralls · 2020-04-28T00:09:46Z

Coverage increased (+0.009%) to 78.295% when pulling 99cf072 on souradeep/margin_to_seconds into 2e2893c on master.

apps/omg_status/test/omg_status/monitor/memory_monitor_test.exs

unnawut · 2020-04-29T06:26:28Z

apps/omg_watcher/lib/omg_watcher/exit_processor/core.ex

@@ -44,7 +44,7 @@ defmodule OMG.Watcher.ExitProcessor.Core do

  use OMG.Utils.LoggerExt

-  @default_sla_margin 10
+  @default_sla_seconds 10


Not sure should the value reflect the original value, which I think should be 10 block * 15s per block = 150?

I guess this only takes place on tests. @souradeep-das can we confirm this - raise a value and whether tests fails?

true, I think these are for tests. Fails when sla_seconds is >10

if this is for tests then let's remove it. we should also remove the default setting below:
sla_seconds \\ @default_sla_seconds

unnawut · 2020-04-29T06:31:45Z

apps/omg_watcher/lib/omg_watcher/exit_processor/standard_exit.ex

    # get exits which are still invalid and after the SLA margin
    late_invalid_exits =
      invalid_exits
-      |> Enum.filter(fn {_, %ExitInfo{eth_height: eth_height}} -> eth_height + sla_margin <= eth_height_now end)
+      |> Enum.filter(fn {_, %ExitInfo{eth_height: eth_height}} ->
+        eth_height + sla_seconds / ethereum_block_time_seconds <= eth_height_now


I think this is fine for this PR since it's still the existing behaviour, but could you maybe raise this discussion separately in the engineering channel?

unnawut · 2020-04-29T06:32:29Z

apps/omg_watcher/lib/omg_watcher/exit_processor/standard_exit.ex

    # get exits which are still invalid and after the SLA margin
    late_invalid_exits =
      invalid_exits
-      |> Enum.filter(fn {_, %ExitInfo{eth_height: eth_height}} -> eth_height + sla_margin <= eth_height_now end)
+      |> Enum.filter(fn {_, %ExitInfo{eth_height: eth_height}} ->


We're moving away from single pipes. Could you update invalid_exits |> Enum.filter(fn... to Enum.filter(invalid_exits, fn...? Thanks!

unnawut · 2020-04-29T06:34:51Z

apps/omg_watcher/lib/omg_watcher/release_tasks/set_exit_processor_sla_margin.ex

@@ -18,8 +18,8 @@ defmodule OMG.Watcher.ReleaseTasks.SetExitProcessorSLAMargin do
  require Logger
  @app :omg_watcher

-  @system_env_name_margin "EXIT_PROCESSOR_SLA_MARGIN"
-  @app_env_name_margin :exit_processor_sla_margin
+  @system_env_name_margin "EXIT_PROCESSOR_SLA_SECONDS"


Note that before merging this PR, we'll need to update our infra to have this environment variable so we don't break the CI/CD environment.

apps/omg_watcher_rpc/priv/swagger/info_api_specs.yaml

unnawut · 2020-04-29T06:41:00Z

Also, could you add this change to https://github.com/omisego/elixir-omg/blob/master/CHANGELOG.md under the Unreleased header?

pnowosie

Nice work!

pnowosie · 2020-04-29T11:00:57Z

apps/omg_watcher/lib/omg_watcher/exit_processor/core.ex

@@ -44,7 +44,7 @@ defmodule OMG.Watcher.ExitProcessor.Core do

  use OMG.Utils.LoggerExt

-  @default_sla_margin 10
+  @default_sla_seconds 10


I guess this only takes place on tests. @souradeep-das can we confirm this - raise a value and whether tests fails?

pnowosie · 2020-04-29T11:10:59Z

apps/omg_watcher/lib/omg_watcher/exit_processor/standard_exit.ex

@@ -79,10 +79,13 @@ defmodule OMG.Watcher.ExitProcessor.StandardExit do
    exits_invalid_by_ife = get_invalid_exits_based_on_ifes(active_exits, tx_appendix)
    invalid_exits = active_exits |> Map.take(invalid_exit_positions) |> Enum.concat(exits_invalid_by_ife) |> Enum.uniq()

+    ethereum_block_time_seconds = OMG.Eth.Configuration.ethereum_block_time_seconds()


Only here we introduce this antipattern ;)
Please restore it as init param - which will be set where others are - and pass it to the Core's init (Core module serves usually as GenServer state). So you could retrieve its value as you do with sla_seconds

pnowosie · 2020-04-29T11:34:01Z

apps/omg_watcher/lib/omg_watcher/release_tasks/set_exit_processor_sla_margin.ex

-  @system_env_name_margin "EXIT_PROCESSOR_SLA_MARGIN"
-  @app_env_name_margin :exit_processor_sla_margin
+  @system_env_name_margin "EXIT_PROCESSOR_SLA_SECONDS"
+  @app_env_name_margin :exit_processor_sla_seconds


Too verbose I think :)

Suggested change

@app_env_name_margin :exit_processor_sla_seconds

@app_env_name :exit_processor_sla_seconds

pnowosie · 2020-04-29T11:35:53Z

apps/omg_watcher/lib/omg_watcher/sync_supervisor.ex

         exit_processor_sla_margin_forced: exit_processor_sla_margin_forced,
         metrics_collection_interval: metrics_collection_interval,
-         min_exit_period_seconds: min_exit_period_seconds,
-         ethereum_block_time_seconds: ethereum_block_time_seconds


Restore this and pass through as I suggested in exit_processor/standard_exit.ex

pnowosie · 2020-04-29T12:21:03Z

config/test.exs

@@ -138,7 +138,7 @@ config :omg_watcher,
  block_getter_loops_interval_ms: 50,
  # NOTE `exit_processor_sla_margin` can't be made shorter. At 3 it sometimes
  # causes :unchallenged_exit because `geth --dev` is too fast
-  exit_processor_sla_margin: 5,
+  exit_processor_sla_seconds: 75,


But this is test - prev we had 1 secs blocks - is it OK?

yes yes 1 sec blocks. Updating!

…_margin to @app_env_name

unnawut · 2020-05-13T04:06:52Z

apps/omg_watcher/lib/omg_watcher/exit_processor/standard_exit.ex

    # get exits which are still invalid and after the SLA margin
+    # temporarily assigning ethereum_block_time_seconds= 1, will be removed on merge of #1495


#1495 is now merged

Thanks O! Updating.

souradeep-das · 2020-06-01T15:30:49Z

apps/omg_watcher/test/omg_watcher/integration/block_getter_test.exs

-    DevHelper.wait_for_root_chain_block(eth_height + exit_processor_sla_margin, @timeout)
+    exit_processor_sla_seconds = Application.fetch_env!(:omg_watcher, :exit_processor_sla_seconds)
+    exit_processor_sla_ms = exit_processor_sla_seconds * 1000
+    Process.sleep(exit_processor_sla_ms)


Is this fine? 🙇

Looks fine to me, we're really waiting for the seconds to come

souradeep-das · 2020-06-01T15:37:56Z

Also @pnowosie asking for a re-review to confirm the latest changes. Thanks 🙇

unnawut · 2020-06-02T04:15:26Z

Adding upgrade path required label so we can collect them up and inform infra/users easily of manual upgrade tasks needed.

unnawut

lgtm!

unnawut · 2020-06-02T04:20:39Z

apps/omg_watcher/test/omg_watcher/integration/block_getter_test.exs

-    DevHelper.wait_for_root_chain_block(eth_height + exit_processor_sla_margin, @timeout)
+    exit_processor_sla_seconds = Application.fetch_env!(:omg_watcher, :exit_processor_sla_seconds)
+    exit_processor_sla_ms = exit_processor_sla_seconds * 1000
+    Process.sleep(exit_processor_sla_ms)


Looks fine to me, we're really waiting for the seconds to come

unnawut · 2020-06-02T04:22:29Z

config/test.exs

-  # NOTE `exit_processor_sla_margin` can't be made shorter. At 8 it sometimes
-  # causes unchallenged exits events because `geth --dev` is too fast
-  exit_processor_sla_margin: 10,
+  # NOTE `exit_processor_sla_margin` can't be made shorter. At 3 it sometimes


Nice that you also checked and updated this 👍 🙏

souradeep-das · 2020-06-08T10:40:07Z

@unnawut do we need to update any env vars?

unnawut · 2020-06-10T08:44:48Z

@unnawut do we need to update any env vars?

Yeah the ones you have in the infra PR should suffice

InoMurko · 2020-06-11T13:47:57Z

apps/omg_watcher/lib/omg_watcher/exit_processor/core.ex

          utxos_to_check: utxos_to_check,
          utxo_exists_result: utxo_exists_result,
          blocks_result: blocks
        },
        %__MODULE__{} = state
      )
-      when not is_nil(eth_height_now) do
+      when not is_nil(eth_timestamp_now) do


this guard is irrelevant and should be removed!

removed 96d9e31

InoMurko · 2020-06-11T13:48:55Z

apps/omg_watcher/lib/omg_watcher/exit_processor/core.ex

+  defp sla_seconds_safe?(exit_processor_sla_seconds, min_exit_period_seconds),
+    do: exit_processor_sla_seconds < min_exit_period_seconds


Suggested change

defp sla_seconds_safe?(exit_processor_sla_seconds, min_exit_period_seconds),

do: exit_processor_sla_seconds < min_exit_period_seconds

defp sla_seconds_safe?(exit_processor_sla_seconds, min_exit_period_seconds) do

exit_processor_sla_seconds < min_exit_period_seconds

end

the function spreads over multiple lines, let's allow it to do so.

InoMurko · 2020-06-11T13:50:20Z

apps/omg_watcher/lib/omg_watcher/exit_processor/standard_exit.ex

+  def get_invalid(
+        %Core{sla_seconds: sla_seconds} = state,
+        utxo_exists?,
+        eth_timestamp_now
+      ) do


Suggested change

def get_invalid(

%Core{sla_seconds: sla_seconds} = state,

utxo_exists?,

eth_timestamp_now

) do

def get_invalid(state, utxo_exists?, eth_timestamp_now) do

state.sla_seconds

done 96d9e31

InoMurko · 2020-06-11T13:51:22Z

apps/omg_watcher/test/omg_watcher/exit_processor/canonicity_test.exs

@@ -48,7 +48,7 @@ defmodule OMG.Watcher.ExitProcessor.CanonicityTest do
    test "none if input never spent elsewhere",
         %{processor_filled: processor} do
      assert {:ok, []} =
-               %ExitProcessor.Request{blknum_now: 1000, eth_height_now: 5}
+               %ExitProcessor.Request{blknum_now: 1000, eth_timestamp_now: 5}


hm... how does this make sense? what is eth_timestamp_now 5?

InoMurko · 2020-06-11T13:52:45Z

apps/omg_watcher/test/omg_watcher/integration/invalid_exit_test.exs

      RootChainHelper.start_exit(tx_utxo_pos, txbytes, proof, alice.addr)
      |> DevHelper.transact_sync!()

    IntegrationTest.wait_for_byzantine_events([%Event.InvalidExit{}.name], @timeout)

-    exit_processor_sla_margin = Application.fetch_env!(:omg_watcher, :exit_processor_sla_margin)
-    DevHelper.wait_for_root_chain_block(eth_height + exit_processor_sla_margin, @timeout)
+    exit_processor_sla_seconds = Application.fetch_env!(:omg_watcher, :exit_processor_sla_seconds)


Suggested change

exit_processor_sla_seconds = Application.fetch_env!(:omg_watcher, :exit_processor_sla_seconds)

exit_processor_sla_seconds = OMG.Watcher.Configuraion.exit_processor_sla_seconds()

done 96d9e31

InoMurko

Few things:

this PR changes a API endpoint (cc @T-Dnzt @unnawut ). A property is removed and replaced with a different one, the new one has a different meaning.
I've seen eth_timestamp_now. I don't exactly understand what it means and how it's used. The tests use values like 5,13,... What is this? Seems like a very odd timestamp.

souradeep-das · 2020-06-11T17:27:56Z

Hey @InoMurko! Thanks for the review 🙇
So, eth_timestamp_now is mostly storing the timestamp ( eth_height_now ) replacing eth_height_now we had previously.

Previously we had -

late_invalid_exits =
      Enum.filter(invalid_exits, fn {_, %ExitInfo{eth_height: eth_height}} ->
        eth_height + sla_margin <= eth_height_now

Since, we added the exit timestamp already, using it for the check and checking with timestamp(eth_height_now)

late_invalid_exits =
       Enum.filter(invalid_exits, fn {_, %ExitInfo{block_timestamp: block_timestamp}} ->
        block_timestamp + sla_seconds <= eth_timestamp_now

assigning the actual timestamp of the block here,
https://github.com/omgnetwork/elixir-omg/pull/1491/files#diff-f1ce1871bf4b96d60bda0fbd02e3891cR632

For the tests,
trying for the exit_timestamp to be similar (in value) to the exit_height, mostly fixing the value of the exit_timestamp for the tests to constant like we do for eth_height.
And then passing values for eth_timestamp_now as the number of seconds that is ++ from the block_timestamp (exit_timestamp)
https://github.com/omgnetwork/elixir-omg/pull/1491/files#diff-feeb2e7cdff5093e568214f358154985R46

Should eth_timestamp_now be renamed for clarity?

souradeep-das · 2020-06-22T09:03:25Z

Do we need a successive change for the change in API endpoint? @InoMurko @unnawut @T-Dnzt

unnawut · 2020-06-25T07:49:21Z

Do we need a successive change for the change in API endpoint? @InoMurko @unnawut @T-Dnzt

@T-Dnzt @achiurizo I think we should get a thumbs up from either of you for any API breaking change.

It's the watcher & watcher_info /configuration.get's exit_processor_sla_margin -> exit_processor_sla_seconds. I don't think there's a workaround e.g. keeping the old key-value, because the value would no longer applicable.

So I guess either:

Scrap this proposal. Downside is we have to stick with sla_margin which is different from the contract's sla_seconds
A 👍 from @T-Dnzt @achiurizo and the release manager making sure the breaking change is in the release comms.

I recommend 2 for consistency with the contracts outweighs dropping this key from /configuration.get

T-Dnzt

lgtm

souradeep-das added 2 commits April 28, 2020 03:24

change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS

deab148

remove eth-block-time and credo fix

6174271

souradeep-das commented Apr 27, 2020

View reviewed changes

update- change cabbage sla_sec

db4f1a2

souradeep-das requested review from unnawut, InoMurko, pnowosie and kevsul April 27, 2020 23:53

souradeep-das added 3 commits April 28, 2020 12:53

fixed- configuration_api_test

85fc0f5

fix barebone test

da7964a

dummy

8e9a537

unnawut reviewed Apr 29, 2020

View reviewed changes

add ETHEREUM_BLOCK_TIME_SECONDS to variables_test_barebone

a250c93

souradeep-das requested review from achiurizo and T-Dnzt as code owners April 29, 2020 10:31

revert mix lock

a0c7d4f

pnowosie approved these changes Apr 29, 2020

View reviewed changes

souradeep-das added 7 commits May 4, 2020 20:33

temp assigning ethereum_block_time_seconds which wont be used later

e074347

credo fix

b2157c9

lint fix

dc32c7d

update sla_seconds for test.exs

9e42630

change: @system_env_name_margin to @system_env_name and @app_env_name…

43bd892

…_margin to @app_env_name

remove single pipeline

b4c5342

revert mix lock

545034d

unnawut suggested changes May 13, 2020

View reviewed changes

souradeep-das added 3 commits May 18, 2020 00:48

merge master

3d60044

merge changes from master

c8ea7aa

get timestamp from eth_height

b25e4dc

souradeep-das added 3 commits June 1, 2020 16:13

style: remove unused vars

8c072f9

merge master

719e7b2

style: fix lint

34cd035

souradeep-das commented Jun 1, 2020

View reviewed changes

souradeep-das marked this pull request as ready for review June 1, 2020 15:35

souradeep-das requested a review from unnawut June 1, 2020 15:36

souradeep-das requested a review from pnowosie June 1, 2020 15:38

unnawut added the upgrade path required Need to inform infra and users of upgrade path, e.g. running an extra command or config change. label Jun 2, 2020

unnawut approved these changes Jun 2, 2020

View reviewed changes

Merge branch 'master' into souradeep/margin_to_seconds

d11ba3e

Merge branch 'master' into souradeep/margin_to_seconds

49587d8

InoMurko reviewed Jun 11, 2020

View reviewed changes

InoMurko suggested changes Jun 11, 2020

View reviewed changes

souradeep-das added 3 commits June 12, 2020 15:42

refactor: remove irrelevant guards

96d9e31

fix: typo

b64b5eb

style: lint fix

c1a0770

pnowosie removed request for achiurizo and pnowosie January 5, 2021 08:18

T-Dnzt approved these changes Mar 11, 2021

View reviewed changes

	@app_env_name_margin :exit_processor_sla_seconds
	@app_env_name :exit_processor_sla_seconds

		# get exits which are still invalid and after the SLA margin
		# temporarily assigning ethereum_block_time_seconds= 1, will be removed on merge of #1495

		defp sla_seconds_safe?(exit_processor_sla_seconds, min_exit_period_seconds),
		do: exit_processor_sla_seconds < min_exit_period_seconds

	exit_processor_sla_seconds = Application.fetch_env!(:omg_watcher, :exit_processor_sla_seconds)
	exit_processor_sla_seconds = OMG.Watcher.Configuraion.exit_processor_sla_seconds()

Change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS #1491

Are you sure you want to change the base?

Change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS #1491

Conversation

souradeep-das commented Apr 27, 2020 • edited Loading

Overview

Changes

souradeep-das Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unnawut commented Apr 29, 2020

pnowosie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

souradeep-das commented Jun 1, 2020

unnawut commented Jun 2, 2020

unnawut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

souradeep-das commented Jun 8, 2020

unnawut commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InoMurko left a comment

Choose a reason for hiding this comment

souradeep-das commented Jun 11, 2020

souradeep-das commented Jun 22, 2020

unnawut commented Jun 25, 2020 • edited Loading

T-Dnzt left a comment

Choose a reason for hiding this comment

souradeep-das commented Apr 27, 2020 •

edited

Loading

souradeep-das Apr 27, 2020 •

edited

Loading

coveralls commented Apr 28, 2020 •

edited

Loading

unnawut commented Jun 25, 2020 •

edited

Loading