Skip to content
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

Change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS #1491

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
deab148
change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS
souradeep-das Apr 27, 2020
6174271
remove eth-block-time and credo fix
souradeep-das Apr 27, 2020
db4f1a2
update- change cabbage sla_sec
souradeep-das Apr 27, 2020
85fc0f5
fixed- configuration_api_test
souradeep-das Apr 28, 2020
da7964a
fix barebone test
souradeep-das Apr 28, 2020
8e9a537
dummy
souradeep-das Apr 28, 2020
a250c93
add ETHEREUM_BLOCK_TIME_SECONDS to variables_test_barebone
souradeep-das Apr 29, 2020
a0c7d4f
revert mix lock
souradeep-das Apr 29, 2020
e074347
temp assigning ethereum_block_time_seconds which wont be used later
souradeep-das May 4, 2020
b2157c9
credo fix
souradeep-das May 4, 2020
dc32c7d
lint fix
souradeep-das May 4, 2020
9e42630
update sla_seconds for test.exs
souradeep-das May 5, 2020
43bd892
change: @system_env_name_margin to @system_env_name and @app_env_name…
souradeep-das May 5, 2020
b4c5342
remove single pipeline
souradeep-das May 5, 2020
545034d
revert mix lock
souradeep-das May 5, 2020
3d60044
merge master
souradeep-das May 17, 2020
c8ea7aa
merge changes from master
souradeep-das May 17, 2020
b25e4dc
get timestamp from eth_height
souradeep-das May 18, 2020
1ba1a3d
revert to eth_height_now
souradeep-das May 18, 2020
872f1b5
fix: change eth_height to eth_timestamp and fix tests
souradeep-das May 28, 2020
67d08db
fix: update eth_timestamp_now in finalization and core tests
souradeep-das May 28, 2020
eac77e0
Merge branch 'master' into souradeep/margin_to_seconds
souradeep-das May 28, 2020
84b4472
fix: get_block_timestamp in exit_processor
souradeep-das May 28, 2020
4a98cda
refactor: remove unix time usage
souradeep-das May 29, 2020
2b663c2
fix: get block_timestamp from opts for tests
souradeep-das May 29, 2020
99cf072
style: lint fix
souradeep-das May 29, 2020
8c072f9
style: remove unused vars
souradeep-das Jun 1, 2020
719e7b2
merge master
souradeep-das Jun 1, 2020
34cd035
style: fix lint
souradeep-das Jun 1, 2020
d11ba3e
Merge branch 'master' into souradeep/margin_to_seconds
souradeep-das Jun 8, 2020
49587d8
Merge branch 'master' into souradeep/margin_to_seconds
unnawut Jun 10, 2020
96d9e31
refactor: remove irrelevant guards
souradeep-das Jun 12, 2020
b64b5eb
fix: typo
souradeep-das Jun 12, 2020
c1a0770
style: lint fix
souradeep-das Jun 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ defmodule OMG.Status.Monitor.MemoryMonitorTest do
assert_receive :got_clear_alarm
end

test "works with :buffered_memory and :cached_memory values are not provided", context do
test "works with :buffered_memory and :cached_memory values are not provided" do
unnawut marked this conversation as resolved.
Show resolved Hide resolved
set_memsup(total_memory: 1000, free_memory: 100)
assert_receive :got_raise_alarm
end
Expand Down
2 changes: 1 addition & 1 deletion apps/omg_watcher/lib/omg_watcher/api/configuration.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ defmodule OMG.Watcher.API.Configuration do
@spec get_configuration() :: {:ok, map()}
def get_configuration() do
configuration = %{
exit_processor_sla_margin: Configuration.exit_processor_sla_margin(),
exit_processor_sla_seconds: Configuration.exit_processor_sla_seconds(),
deposit_finality_margin: OMG.Configuration.deposit_finality_margin(),
contract_semver: OMG.Eth.Configuration.contract_semver(),
network: OMG.Eth.Configuration.network()
Expand Down
4 changes: 2 additions & 2 deletions apps/omg_watcher/lib/omg_watcher/configuration.ex
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ defmodule OMG.Watcher.Configuration do
Provides access to applications configuration
"""
@app :omg_watcher
def exit_processor_sla_margin() do
Application.fetch_env!(@app, :exit_processor_sla_margin)
def exit_processor_sla_seconds() do
Application.fetch_env!(@app, :exit_processor_sla_seconds)
end

def exit_processor_sla_margin_forced() do
Expand Down
19 changes: 8 additions & 11 deletions apps/omg_watcher/lib/omg_watcher/exit_processor.ex
Original file line number Diff line number Diff line change
Expand Up @@ -247,34 +247,31 @@ defmodule OMG.Watcher.ExitProcessor do
Reads the exit data from `OMG.DB`.

Options:
- `exit_processor_sla_margin`: number of blocks after exit start before it's considered late (and potentially:
- `exit_processor_sla_seconds`: seconds after exit start before it's considered late (and potentially:
unchallenged)
- `exit_processor_sla_margin_forced`: if `true` will override the check of `exit_processor_sla_margin` against
- `exit_processor_sla_margin_forced`: if `true` will override the check of `exit_processor_sla_seconds` against
`min_exit_period_seconds`
- `min_exit_period_seconds`: should reflect the value of this parameter for the specific child chain watched,
- `ethereum_block_time_seconds`: just to relate blocks to seconds for the `exit_processor_sla_margin` check
- `metrics_collection_interval`: how often are the metrics sent to `telemetry` (in milliseconds)
"""
def init(
exit_processor_sla_margin: exit_processor_sla_margin,
exit_processor_sla_seconds: exit_processor_sla_seconds,
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
min_exit_period_seconds: min_exit_period_seconds
) do
{:ok, db_exits} = DB.exit_infos()
{:ok, db_ifes} = DB.in_flight_exits_info()
{:ok, db_competitors} = DB.competitors_info()

:ok =
Core.check_sla_margin(
exit_processor_sla_margin,
Core.check_sla_seconds(
exit_processor_sla_seconds,
exit_processor_sla_margin_forced,
min_exit_period_seconds,
ethereum_block_time_seconds
min_exit_period_seconds
)

{:ok, processor} = Core.init(db_exits, db_ifes, db_competitors, exit_processor_sla_margin)
{:ok, processor} = Core.init(db_exits, db_ifes, db_competitors, exit_processor_sla_seconds)
{:ok, _} = :timer.send_interval(metrics_collection_interval, self(), :send_metrics)

_ = Logger.info("Initializing with: #{inspect(processor)}")
Expand Down
34 changes: 17 additions & 17 deletions apps/omg_watcher/lib/omg_watcher/exit_processor/core.ex
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ defmodule OMG.Watcher.ExitProcessor.Core do

use OMG.Utils.LoggerExt

@default_sla_margin 10
@default_sla_seconds 10
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@zero_address OMG.Eth.zero_address()

@max_inputs Transaction.Payment.max_inputs()
Expand All @@ -68,10 +68,10 @@ defmodule OMG.Watcher.ExitProcessor.Core do

@type new_piggyback_event_t() :: new_piggyback_input_event_t() | new_piggyback_output_event_t()

defstruct [:sla_margin, exits: %{}, in_flight_exits: %{}, exit_ids: %{}, competitors: %{}]
defstruct [:sla_seconds, exits: %{}, in_flight_exits: %{}, exit_ids: %{}, competitors: %{}]

@type t :: %__MODULE__{
sla_margin: non_neg_integer(),
sla_seconds: non_neg_integer(),
exits: %{Utxo.Position.t() => ExitInfo.t()},
in_flight_exits: %{Transaction.tx_hash() => InFlightExitInfo.t()},
# NOTE: maps only standard exit_ids to the natural keys of standard exits (input pointers/utxo_pos)
Expand Down Expand Up @@ -101,9 +101,9 @@ defmodule OMG.Watcher.ExitProcessor.Core do
db_exits :: [{{pos_integer, non_neg_integer, non_neg_integer}, map}],
db_in_flight_exits :: [{Transaction.tx_hash(), InFlightExitInfo.t()}],
db_competitors :: [{Transaction.tx_hash(), CompetitorInfo.t()}],
sla_margin :: non_neg_integer
sla_seconds :: non_neg_integer
) :: {:ok, t()}
def init(db_exits, db_in_flight_exits, db_competitors, sla_margin \\ @default_sla_margin) do
def init(db_exits, db_in_flight_exits, db_competitors, sla_seconds \\ @default_sla_seconds) do
exits = db_exits |> Enum.map(&ExitInfo.from_db_kv/1) |> Map.new()
exit_ids = exits |> Enum.into(%{}, fn {utxo_pos, %ExitInfo{exit_id: exit_id}} -> {exit_id, utxo_pos} end)

Expand All @@ -113,30 +113,30 @@ defmodule OMG.Watcher.ExitProcessor.Core do
in_flight_exits: db_in_flight_exits |> Enum.map(&InFlightExitInfo.from_db_kv/1) |> Map.new(),
exit_ids: exit_ids,
competitors: db_competitors |> Enum.map(&CompetitorInfo.from_db_kv/1) |> Map.new(),
sla_margin: sla_margin
sla_seconds: sla_seconds
}}
end

@doc """
Use to check if the settings regarding the `:exit_processor_sla_margin` config of `:omg_watcher` are OK.
Use to check if the settings regarding the `:exit_processor_sla_seconds` config of `:omg_watcher` are OK.

Since there are combinations of our configuration that may lead to a dangerous setup of the Watcher
(in particular - muting the reports of unchallenged_exits), we're enforcing that the `exit_processor_sla_margin`
(in particular - muting the reports of unchallenged_exits), we're enforcing that the `exit_processor_sla_seconds`
be not larger than `min_exit_period`.
"""
@spec check_sla_margin(pos_integer(), boolean(), pos_integer(), pos_integer()) :: :ok | {:error, :sla_margin_too_big}
def check_sla_margin(sla_margin, sla_margin_forced, min_exit_period_seconds, ethereum_block_time_seconds)
@spec check_sla_seconds(pos_integer(), boolean(), pos_integer()) :: :ok | {:error, :sla_margin_too_big}
def check_sla_seconds(sla_seconds, sla_margin_forced, min_exit_period_seconds)

def check_sla_margin(sla_margin, true, min_exit_period_seconds, ethereum_block_time_seconds) do
def check_sla_seconds(sla_seconds, true, min_exit_period_seconds) do
_ =
if !sla_margin_safe?(sla_margin, min_exit_period_seconds, ethereum_block_time_seconds),
do: Logger.warn("Allowing unsafe sla margin of #{sla_margin} blocks")
if !sla_seconds_safe?(sla_seconds, min_exit_period_seconds),
do: Logger.warn("Allowing unsafe sla margin of #{sla_seconds} seconds")

:ok
end

def check_sla_margin(sla_margin, false, min_exit_period_seconds, ethereum_block_time_seconds) do
if sla_margin_safe?(sla_margin, min_exit_period_seconds, ethereum_block_time_seconds),
def check_sla_seconds(sla_seconds, false, min_exit_period_seconds) do
if sla_seconds_safe?(sla_seconds, min_exit_period_seconds),
do: :ok,
else: {:error, :sla_margin_too_big}
end
Expand Down Expand Up @@ -587,6 +587,6 @@ defmodule OMG.Watcher.ExitProcessor.Core do
address != @zero_address
end

defp sla_margin_safe?(exit_processor_sla_margin, min_exit_period_seconds, ethereum_block_time_seconds),
do: exit_processor_sla_margin * ethereum_block_time_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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

end
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ defmodule OMG.Watcher.ExitProcessor.StandardExit do
"""
@spec get_invalid(Core.t(), %{Utxo.Position.t() => boolean}, pos_integer()) ::
{%{Utxo.Position.t() => ExitInfo.t()}, %{Utxo.Position.t() => ExitInfo.t()}}
def get_invalid(%Core{sla_margin: sla_margin} = state, utxo_exists?, eth_height_now) do
def get_invalid(%Core{sla_seconds: sla_seconds} = state, utxo_exists?, eth_height_now) do
active_exits = active_exits(state)

invalid_exit_positions =
Expand All @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

# 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}} ->
Copy link
Contributor

Choose a reason for hiding this comment

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

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

eth_height + sla_seconds / ethereum_block_time_seconds <= eth_height_now
Copy link
Contributor Author

@souradeep-das souradeep-das Apr 27, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

end)

{Map.new(invalid_exits), Map.new(late_invalid_exits)}
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@app_env_name_margin :exit_processor_sla_seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Too verbose I think :)

Suggested change
@app_env_name_margin :exit_processor_sla_seconds
@app_env_name :exit_processor_sla_seconds


@system_env_name_force "EXIT_PROCESSOR_SLA_MARGIN_FORCED"
@app_env_name_force :exit_processor_sla_margin_forced
Expand All @@ -33,13 +33,13 @@ defmodule OMG.Watcher.ReleaseTasks.SetExitProcessorSLAMargin do

Config.Reader.merge(config,
omg_watcher: [
exit_processor_sla_margin: get_exit_processor_sla_margin(),
exit_processor_sla_seconds: get_exit_processor_sla_seconds(),
exit_processor_sla_margin_forced: get_exit_processor_sla_forced()
]
)
end

defp get_exit_processor_sla_margin() do
defp get_exit_processor_sla_seconds() do
config_value = validate_int(get_env(@system_env_name_margin), Application.get_env(@app, @app_env_name_margin))
_ = Logger.info("CONFIGURATION: App: #{@app} Key: #{@system_env_name_margin} Value: #{inspect(config_value)}.")
config_value
Expand Down
8 changes: 3 additions & 5 deletions apps/omg_watcher/lib/omg_watcher/sync_supervisor.ex
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,23 @@ defmodule OMG.Watcher.SyncSupervisor do

defp children(args) do
contract_deployment_height = Keyword.fetch!(args, :contract_deployment_height)
exit_processor_sla_margin = Configuration.exit_processor_sla_margin()
exit_processor_sla_seconds = Configuration.exit_processor_sla_seconds()
exit_processor_sla_margin_forced = Configuration.exit_processor_sla_margin_forced()
metrics_collection_interval = Configuration.metrics_collection_interval()
finality_margin = Configuration.exit_finality_margin()
deposit_finality_margin = OMG.Configuration.deposit_finality_margin()
ethereum_events_check_interval_ms = OMG.Configuration.ethereum_events_check_interval_ms()
coordinator_eth_height_check_interval_ms = OMG.Configuration.coordinator_eth_height_check_interval_ms()
min_exit_period_seconds = OMG.Eth.Configuration.min_exit_period_seconds()
ethereum_block_time_seconds = OMG.Eth.Configuration.ethereum_block_time_seconds()
contracts = OMG.Eth.Configuration.contracts()

[
{ExitProcessor,
[
exit_processor_sla_margin: exit_processor_sla_margin,
exit_processor_sla_seconds: exit_processor_sla_seconds,
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

min_exit_period_seconds: min_exit_period_seconds
]},
%{
id: OMG.Watcher.BlockGetter.Supervisor,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,13 @@ defmodule OMG.Watcher.ExitProcessor.CoreTest do

describe "check_sla_margin/4" do
test "allows only safe margins if not forcing" do
assert {:error, :sla_margin_too_big} = Core.check_sla_margin(10, false, 100, 15)
assert :ok = Core.check_sla_margin(10, false, 300, 15)
assert {:error, :sla_margin_too_big} = Core.check_sla_seconds(150, false, 100)
assert :ok = Core.check_sla_seconds(150, false, 300)
end

test "allows anything if forcing" do
capture_log(fn -> assert :ok = Core.check_sla_margin(10, true, 100, 15) end)
assert :ok = Core.check_sla_margin(10, true, 300, 15)
capture_log(fn -> assert :ok = Core.check_sla_seconds(150, true, 100) end)
assert :ok = Core.check_sla_seconds(150, true, 300)
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ defmodule OMG.Watcher.Integration.BlockGetterTest do
end

@tag fixtures: [:in_beam_watcher, :stable_alice, :mix_based_child_chain, :token, :stable_alice_deposits, :test_server]
test "transaction which is spending an exiting output before the `sla_margin` causes an invalid_exit event only",
test "transaction which is spending an exiting output before the `sla_seconds` causes an invalid_exit event only",
%{stable_alice: alice, stable_alice_deposits: {deposit_blknum, _}, test_server: context} do
tx = OMG.TestHelper.create_encoded([{deposit_blknum, 0, 0, alice}], @eth, [{alice, 9}])
%{"blknum" => exit_blknum} = WatcherHelper.submit(tx)
Expand Down Expand Up @@ -223,14 +223,14 @@ defmodule OMG.Watcher.Integration.BlockGetterTest do

# Here we're manually submitting invalid block to the root chain
# NOTE: this **must** come after `start_exit` is mined (see just above) but still not later than
# `sla_margin` after exit start, hence the `config/test.exs` entry for the margin is rather high
# `sla_seconds` after exit start, hence the `config/test.exs` entry for the margin is rather high
{:ok, _} = Eth.submit_block(bad_block_hash, 2, 1)

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

@tag fixtures: [:in_beam_watcher, :stable_alice, :mix_based_child_chain, :token, :stable_alice_deposits, :test_server]
test "transaction which is spending an exiting output after the `sla_margin` causes an unchallenged_exit event",
test "transaction which is spending an exiting output after the `sla_seconds` causes an unchallenged_exit event",
%{stable_alice: alice, stable_alice_deposits: {deposit_blknum, _}, test_server: context} do
tx = OMG.TestHelper.create_encoded([{deposit_blknum, 0, 0, alice}], @eth, [{alice, 9}])
%{"blknum" => exit_blknum} = WatcherHelper.submit(tx)
Expand Down Expand Up @@ -259,7 +259,7 @@ defmodule OMG.Watcher.Integration.BlockGetterTest do
"utxo_pos" => utxo_pos
} = WatcherHelper.get_exit_data(exit_blknum, 0, 0)

{:ok, %{"status" => "0x1", "blockNumber" => eth_height}} =
{:ok, %{"status" => "0x1"}} =
RootChainHelper.start_exit(
utxo_pos,
txbytes,
Expand All @@ -268,8 +268,9 @@ defmodule OMG.Watcher.Integration.BlockGetterTest do
)
|> DevHelper.transact_sync!()

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)
exit_processor_sla_ms = exit_processor_sla_seconds * 1000
Process.sleep(exit_processor_sla_ms)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this fine? 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

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


# checking if both machines and humans learn about the byzantine condition
assert WatcherHelper.capture_log(fn ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ defmodule OMG.Watcher.Integration.InvalidExitTest do
end

@tag fixtures: [:in_beam_watcher, :stable_alice, :mix_based_child_chain, :token, :stable_alice_deposits]
test "invalid_exit without a challenge raises unchallenged_exit after sla_margin had passed, can be challenged",
test "invalid_exit without a challenge raises unchallenged_exit after sla_seconds had passed, can be challenged",
%{stable_alice: alice, stable_alice_deposits: {deposit_blknum, _}} do
%{"blknum" => first_tx_blknum} =
OMG.TestHelper.create_encoded([{deposit_blknum, 0, 0, alice}], @eth, [{alice, 9}]) |> WatcherHelper.submit()
Expand All @@ -109,14 +109,16 @@ defmodule OMG.Watcher.Integration.InvalidExitTest do
%{"txbytes" => txbytes, "proof" => proof, "utxo_pos" => tx_utxo_pos} =
WatcherHelper.get_exit_data(first_tx_blknum, 0, 0)

{:ok, %{"status" => "0x1", "blockNumber" => eth_height}} =
{:ok, %{"status" => "0x1"}} =
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 96d9e31

exit_processor_sla_ms = exit_processor_sla_seconds * 1000
# after exit, the event fetch time + waiting for sla_margin goes beyond the required time
Process.sleep(exit_processor_sla_ms)
IntegrationTest.wait_for_byzantine_events([%Event.InvalidExit{}.name, %Event.UnchallengedExit{}.name], @timeout)

# after the notification has been received, a challenged is composed and sent, regardless of it being late
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,37 @@ defmodule OMG.Watcher.ReleaseTasks.SetExitProcessorSLAMarginTest do
@app :omg_watcher

test "if environment variables get applied in the configuration" do
:ok = System.put_env("EXIT_PROCESSOR_SLA_MARGIN", "15")
:ok = System.put_env("EXIT_PROCESSOR_SLA_SECONDS", "15")
:ok = System.put_env("EXIT_PROCESSOR_SLA_MARGIN_FORCED", "TRUE")
config = SetExitProcessorSLAMargin.load([], [])
exit_processor_sla_margin = config |> Keyword.fetch!(@app) |> Keyword.fetch!(:exit_processor_sla_margin)
exit_processor_sla_seconds = config |> Keyword.fetch!(@app) |> Keyword.fetch!(:exit_processor_sla_seconds)

exit_processor_sla_margin_forced =
config |> Keyword.fetch!(@app) |> Keyword.fetch!(:exit_processor_sla_margin_forced)

assert exit_processor_sla_margin == 15
assert exit_processor_sla_seconds == 15
assert exit_processor_sla_margin_forced == true
end

test "if default configuration is used when there's no environment variables" do
:ok = System.delete_env("EXIT_PROCESSOR_SLA_MARGIN")
:ok = System.delete_env("EXIT_PROCESSOR_SLA_SECONDS")
:ok = System.delete_env("EXIT_PROCESSOR_SLA_MARGIN_FORCED")
config = SetExitProcessorSLAMargin.load([], [])
exit_processor_sla_margin = config |> Keyword.fetch!(@app) |> Keyword.fetch!(:exit_processor_sla_margin)
exit_processor_sla_seconds = config |> Keyword.fetch!(@app) |> Keyword.fetch!(:exit_processor_sla_seconds)

exit_processor_sla_margin_forced =
config |> Keyword.fetch!(@app) |> Keyword.fetch!(:exit_processor_sla_margin_forced)

exit_processor_sla_margin_updated = Application.get_env(@app, :exit_processor_sla_margin)
exit_processor_sla_seconds_updated = Application.get_env(@app, :exit_processor_sla_seconds)
exit_processor_sla_margin_forced_updated = Application.get_env(@app, :exit_processor_sla_margin_forced)
assert exit_processor_sla_margin == exit_processor_sla_margin_updated
assert exit_processor_sla_seconds == exit_processor_sla_seconds_updated
assert exit_processor_sla_margin_forced == exit_processor_sla_margin_forced_updated
end

test "if exit is thrown when faulty margin configuration is used" do
:ok = System.put_env("EXIT_PROCESSOR_SLA_MARGIN", "15a")
:ok = System.put_env("EXIT_PROCESSOR_SLA_SECONDS", "15a")
catch_exit(SetExitProcessorSLAMargin.load([], []))
:ok = System.delete_env("EXIT_PROCESSOR_SLA_MARGIN")
:ok = System.delete_env("EXIT_PROCESSOR_SLA_SECONDS")
end

test "if exit is thrown when faulty margin force configuration is used" do
Expand Down
Loading