Papa/sec 27 watcher info ife support #1496

pnowosie · 2020-04-30T12:55:25Z

Fixes https://github.com/omisego/security-issues/issues/27

Overview

Watcher-Info subscribes to Watcher's event bus to catch up on network changes. As the issue describes IFE events wasn't handled at all. Raw contract events just report changes they are not sufficient for Watcher-Info to be able to work on them without storing its own context and Watcher already has this context any way.

Changes

This :pr: introduces 3 new watcher events (new category as we have contract's marked with :root_chain and plasma's :child_chain events)

{:watcher, "InFlightExitStarted"} - this events extends data from ExitProcessor's new_in_flight_exits that can be used to spend (all) in-flight transaction's inputs (with Eth_Event)
{:watcher, "InFlightTxOutputPiggybacked"} - guides Info API to spend output when in-flight transaction's output is piggybacked, is broadcasted from [piggyback_exits].
{:watcher, "InFlightExitIOputProcessed"} - propagated after IFE is processed in [finalize_in_flight_exits] - which contains UTXO that security Watcher spends. It is a bit late for Info API but still can be useful is edge cases or just as sanity check.

For the Child-chain the first 2 events are sufficient to eagerly spend IFE's I/Oputs, it should suffice in most cases for Info API as well. However here there is some latency involved in when events about new blocks and exits are processed so I think having 3rd event as a last-resort is a good idea.

Only the first event is persisted as a spending eth event in Postgres. Action on these event is idempotent.

Testing

[Cabbage's in_fligth_exits_test] demonstrates that issue is fixed.
More unit tests

coveralls · 2020-04-30T13:52:18Z

Coverage increased (+0.2%) to 78.324% when pulling a72aa51 on papa/sec-27-Watcher-info-IFE-support into e8d486d on master.

achiurizo · 2020-05-04T08:31:45Z

apps/omg_watcher/lib/omg_watcher/exit_processor.ex

+    :ok =
+      events_with_utxos
+      |> Tools.to_bus_events_data()
+      |> publish_internal_bus_events("InFlightExitIOputProcessed")


What is IOput?

Input or Output (credits to @JBunCE) alternative is Xput 👐

More context: we already publish InFlightExitInputWithdrawn & InFlightExitOutputWithdrawn event from rootchain. The above event from Watcher is focused to let consumer know that particular *put can be marked as spent (if not done yet) than on the ife processing... I'm open to better names

Okay, why not just call it InFlightExitProcessed if it can be either? Introducing yet another term for this isn't helpful since, both inputs and outputs are viable means to do an IFE. Also, let's consider that an input and output are references to an output.

I can't come up with any scenario where we need this. Watcher info can do without it as all the utxos involved here are either:

inputs of ife tx and we have an eth_event spending entry for that, as we put it into the db when exit starts

outputs, which had to be piggybacked and we put eth events for piggybacks into the db

@achiurizo: I'd go with InFlightExitOutputWithdrawn then - exit processed is fine however in my opinion it could be confusing as Info-Watcher (consumer) process no exits

@pgebal: #1496 (comment)

pgebal

~~I didn't get into implementation details yet. I've just glanced through it but I think we should have a test that checks what happens when~~

ife is started
transaction is included in a block
output is piggybacked and successfully challenged

~~edit: to clarify, output can be challenged by other tx spending this output, signed by output owner, it does not need to be included in a plasma block~~

~~I think the output should be available to spend in that case. I have a feeling it has not been addressed in this PR.~~

edit: that scenario is wrong

priv/cabbage/apps/itest/test/features/in_flight_exits.feature

apps/omg_watcher_info/lib/omg_watcher_info/exit_consumer.ex

pgebal · 2020-05-05T12:08:21Z

apps/omg_watcher/lib/omg_watcher/exit_processor.ex

+    :ok =
+      events_with_utxos
+      |> Tools.to_bus_events_data()
+      |> publish_internal_bus_events("InFlightExitIOputProcessed")


I can't come up with any scenario where we need this. Watcher info can do without it as all the utxos involved here are either:

inputs of ife tx and we have an eth_event spending entry for that, as we put it into the db when exit starts

outputs, which had to be piggybacked and we put eth events for piggybacks into the db

pgebal · 2020-05-05T12:21:24Z

priv/cabbage/apps/itest/test/features/in_flight_exits.feature

+    Then Alice verifies its in flight exit from the most recently created transaction
+    Given Alice piggybacks output from her most recent in flight exit
+    And "Alice" in flight transaction most recently piggybacked output is not spendable any more
+    Then Alice can processes her own most recent in flight exit


It would be nice to have a test where:

ife and piggyback come first

then comes the ch-ch block with a transaction that created the piggybacked output

piggybacked output is not available to spend

it might be hard to implement that in cabbage, but maybe a test involving just the db would do?

Yes - that's exactly the egde case I included handling the on-finalization event and also the aswer to you comment:

I can't come up with any scenario where we need this.

We don't have such control of the events in cabbage test - I'll include it elsewhere and point here. Thanks 👍

I've added ExitConsumer tests, however testing on DB level above scenario with all 3 steps above will be unnecessary bloat because in pt 1. we know we are trying to spent non-existing output then in pt 2. we're adding it and finally spending in 3.

I've check these points in the separate tests.

I think test we have in OMG.WatcherInfo.ExitConsumerTest do not add up to the scenario I posted.
We do not have a test where event comes before the utxo.
If this is implied from the tests we have, please put a comment explaining how that works.

Reasoning from what I can find in the code it looks like it will work well but tests confirming that are welcomed.

I still do not get why we need spending on finalization. I think all would work well without it. Could you please explain why we need that?

In the scenario you posted:

Watcher-Info receives an event IFE-started and spends inputs,

Watcher-Info receives an event Output piggybacked, but block including in-flight tx has not been received yet (👇 next point), so there are no utxos to spend in the DB,

Watcher-Info receives an event New block - which creates an in-flight tx outputs (it does not matter for Watcher-Info that inputs are already spend).

Without additinal „sanity” spending on finalizations the outputs will persist.

Regarding the explicit test for the scenario I think we have it splitted into separated (smaller) tests:
Pt 1 is covered,
Pt 2 - test showing that spending non-existing output does not break,
Pt 3 - test spending output by „finalization”-like event

I’ll add a comment in the test module to describe the scenario and it’s coverage by the tests already present

apps/omg_watcher_info/lib/omg_watcher_info/db/eth_event.ex

pgebal

Maybe I just can't find it, but I think that we still do not have any test that would somehow check a scenario where event comes before the utxo.

Other than that it looks good, just a couple of minors.

apps/omg_watcher_info/test/omg_watcher_info/exit_consumer_test.exs

apps/omg_watcher_info/test/omg_watcher_info/db/eth_event_test.exs

apps/omg_watcher_info/test/omg_watcher_info/exit_consumer_test.exs

pnowosie · 2020-05-13T10:03:08Z

Edit: I rethink the case raised by @pgebal and create #1515.
There is still the risk (under high load) that current solution won't work.

Preparing for more spending events like start_ife, ife_piggybacked, ... `start_ife` will spend all transaction's inputs

…reen

pnowosie requested review from InoMurko, kevsul, pgebal and kalouo April 30, 2020 12:55

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch from a60ab71 to fb26a03 Compare April 30, 2020 13:29

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch 3 times, most recently from 29cb3d3 to c866970 Compare May 4, 2020 07:51

pnowosie marked this pull request as ready for review May 4, 2020 07:51

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch from c866970 to 3723b31 Compare May 4, 2020 07:59

achiurizo reviewed May 4, 2020

View reviewed changes

pgebal reviewed May 5, 2020

View reviewed changes

priv/cabbage/apps/itest/test/features/in_flight_exits.feature Outdated Show resolved Hide resolved

pgebal reviewed May 5, 2020

View reviewed changes

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch 2 times, most recently from af45cbb to 6558cda Compare May 8, 2020 09:10

kevsul reviewed May 8, 2020

View reviewed changes

apps/omg_watcher_info/lib/omg_watcher_info/db/eth_event.ex Outdated Show resolved Hide resolved

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch from d106109 to 1807930 Compare May 8, 2020 14:42

pnowosie requested review from pgebal, achiurizo and kevsul May 8, 2020 14:49

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch from 1807930 to 9292d94 Compare May 12, 2020 10:31

pgebal reviewed May 12, 2020

View reviewed changes

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch from 39350bc to b0f73b7 Compare May 13, 2020 13:42

pnowosie requested a review from pgebal May 13, 2020 13:43

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch from b0f73b7 to b009a8c Compare May 13, 2020 13:47

pnowosie mentioned this pull request May 15, 2020

Watcher-Info data update might cause issue under a load #1515

Open

pgebal approved these changes May 15, 2020

View reviewed changes

kevsul approved these changes May 18, 2020

View reviewed changes

kalouo approved these changes May 18, 2020

View reviewed changes

Pawel Nowosielski added 14 commits May 19, 2020 10:41

feat: gather data for events

1c2a42a

feat: generating events from finalizations

8a79993

feat: add new events consumer to spent finalized ife-utxos

7af466a

test: add cabbage test

82507b2

feat: allow single eth event spend more utxos

b355a92

Preparing for more spending events like start_ife, ife_piggybacked, ... `start_ife` will spend all transaction's inputs

refactor: move & rename bus events helper function

074d11f

feat: unify events handling in watcher_info, pass ife_started, test g…

42c0d74

…reen

test: simplify & speed up cabbage test - but ife has to be processed

7c1e893

test: add failing test showing piggybacked output remains unspend

af30a88

feat: add TxOutput.get_by_output_id/2 and handle events with output id

497360a

feat: publish event when ife tx output is piggybacked

439dc31

chore: rebase onto master

75857d9

test: align to comments

0e4ad3c

chore: align to Pawel's comments

a72aa51

pnowosie force-pushed the papa/sec-27-Watcher-info-IFE-support branch from b009a8c to a72aa51 Compare May 19, 2020 08:42

pnowosie merged commit 4ab2549 into master May 19, 2020

pnowosie deleted the papa/sec-27-Watcher-info-IFE-support branch May 19, 2020 10:18

pnowosie mentioned this pull request May 28, 2020

feat: add event type when consumer is spending utxos #1538

Merged

unnawut added the enhancement New feature or request label Aug 28, 2020

pnowosie mentioned this pull request Nov 5, 2020

Utxo not found when attempting transaction with some utxos #1760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Papa/sec 27 watcher info ife support #1496

Papa/sec 27 watcher info ife support #1496

pnowosie commented Apr 30, 2020 •

edited

Loading

coveralls commented Apr 30, 2020 •

edited

Loading

achiurizo May 4, 2020

pnowosie May 4, 2020 •

edited

Loading

achiurizo May 4, 2020

pgebal May 5, 2020

pnowosie May 8, 2020 •

edited

Loading

pgebal left a comment •

edited

Loading

pgebal May 5, 2020

pgebal May 5, 2020 •

edited

Loading

pnowosie May 7, 2020 •

edited

Loading

pnowosie May 8, 2020

pgebal May 12, 2020

pgebal May 12, 2020 •

edited

Loading

pnowosie May 12, 2020 •

edited

Loading

pnowosie May 12, 2020

pgebal left a comment •

edited

Loading

pnowosie commented May 13, 2020 •

edited

Loading

Papa/sec 27 watcher info ife support #1496

Papa/sec 27 watcher info ife support #1496

Conversation

pnowosie commented Apr 30, 2020 • edited Loading

Overview

Changes

Testing

coveralls commented Apr 30, 2020 • edited Loading

achiurizo May 4, 2020

Choose a reason for hiding this comment

pnowosie May 4, 2020 • edited Loading

Choose a reason for hiding this comment

achiurizo May 4, 2020

Choose a reason for hiding this comment

pgebal May 5, 2020

Choose a reason for hiding this comment

pnowosie May 8, 2020 • edited Loading

Choose a reason for hiding this comment

pgebal left a comment • edited Loading

Choose a reason for hiding this comment

pgebal May 5, 2020

Choose a reason for hiding this comment

pgebal May 5, 2020 • edited Loading

Choose a reason for hiding this comment

pnowosie May 7, 2020 • edited Loading

Choose a reason for hiding this comment

pnowosie May 8, 2020

Choose a reason for hiding this comment

pgebal May 12, 2020

Choose a reason for hiding this comment

pgebal May 12, 2020 • edited Loading

Choose a reason for hiding this comment

pnowosie May 12, 2020 • edited Loading

Choose a reason for hiding this comment

pnowosie May 12, 2020

Choose a reason for hiding this comment

pgebal left a comment • edited Loading

Choose a reason for hiding this comment

pnowosie commented May 13, 2020 • edited Loading

pnowosie commented Apr 30, 2020 •

edited

Loading

coveralls commented Apr 30, 2020 •

edited

Loading

pnowosie May 4, 2020 •

edited

Loading

pnowosie May 8, 2020 •

edited

Loading

pgebal left a comment •

edited

Loading

pgebal May 5, 2020 •

edited

Loading

pnowosie May 7, 2020 •

edited

Loading

pgebal May 12, 2020 •

edited

Loading

pnowosie May 12, 2020 •

edited

Loading

pgebal left a comment •

edited

Loading

pnowosie commented May 13, 2020 •

edited

Loading