omgnetwork / elixir-omg

OMG-Network repository of Watcher and Watcher Info
https://omg.network
Apache License 2.0
211 stars 59 forks source link

Return only one byzantine exit event for a given UTXO #785

Open kasima opened 5 years ago

kasima commented 5 years ago

As a watcher user, I can see a single byzantine exit event for a given UTXO, So that it's clear what state my UTXO is in.

From a [Samrong availability incident](), when calling /status.get, a given UTXO would show up in the byzantine_events array twice. Once as an invalid_exit and again as an unchallenged_exit. Given that unchallenged_exit is a state that follows invalid_exit after the MFP, showing both is unnecessary and may lead to confusion.

Example:

[{'details': {'amount': 1,
   'currency': '0x874ad6217760bb782405a77caf71a4c101d6c98c',
   'eth_height': 4559794,
   'name': 'unchallenged_exit',
   'owner': '0xd2e74e23a31b14bf5c10cfbc119fcf547b24262e',
   'utxo_pos': 238000000000000},
  'event': 'unchallenged_exit'},
 {'details': {'amount': 9,
   'currency': '0x874ad6217760bb782405a77caf71a4c101d6c98c',
   'eth_height': 4559795,
   'name': 'unchallenged_exit',
   'owner': '0xd2e74e23a31b14bf5c10cfbc119fcf547b24262e',
   'utxo_pos': 238000000000001},
  'event': 'unchallenged_exit'},
 {'details': {'amount': 1,
   'currency': '0x874ad6217760bb782405a77caf71a4c101d6c98c',
   'eth_height': 4559794,
   'name': 'invalid_exit',
   'owner': '0xd2e74e23a31b14bf5c10cfbc119fcf547b24262e',
   'utxo_pos': 238000000000000},
  'event': 'invalid_exit'},
 {'details': {'amount': 9,
   'currency': '0x874ad6217760bb782405a77caf71a4c101d6c98c',
   'eth_height': 4559795,
   'name': 'invalid_exit',
   'owner': '0xd2e74e23a31b14bf5c10cfbc119fcf547b24262e',
   'utxo_pos': 238000000000001},
  'event': 'invalid_exit'}]
pdobacz commented 5 years ago

This behavior is intentional, due to a different nature of these events and distinct action items related. Let me explain.

invalid_exit means just that and the action item is to challenge the exit. This could potentially be renamed to invalid_standard_exit to be more specific. Why? It'll become clearer further on.

unchallenged_exit means generally that something is about to go wrong, and to follow the protocol and keep safe, one must exit, unconditionally. It is true, that we don't do that (because we don't care about the tokens yet) and we just challenge the exit and yolo, but the protocol is clear about this: the exit should've been challenged, but it hasn't been so run!

(to be 100% clear - you still might be able to and should challenge)

Going forward, unchallenged_exit can apply to different kinds of exits/conditions and it still has would have the same action item "attached", which is "run!". E.g. wrongly-canonical IFE which goes without challenge for long. This hasn't yet been implemented though (some dangling story, somewhere...).

So the concept of unchallenged_exit is somewhat orthogonal to the particular "invalid-somethings" that can be reported. It is a bit more of a 0-1 thing - either the chain is in such condition or not, the underlying invalid exit is just the "reason" behind the condition.

unchallenged_exit could be alternatively called chain_about_to_be_invalid.

That all being said, the current implementation of the API tries to reflect this logic: the byzantine events align with CTA's they are linked to. Remarks:

  1. One could potentially serve just a single instance of unchallenged_exit perhaps, instead of many, to make it even more aligned. It would then behave like withholding and invalid_block events, which are always single too.
  2. Also the unchallenged_exit could be stripped of the metadata invalid_exit has, to make the distinction cleaner (unchallened_exit just references the (any or oldest?) invalid_exit - {name: unchallenged_exit, because_of: {name: invalid_exit, utxo_pos: 345678}} or similar)
  3. Lastly, the API could be revamped so that it is more clear, but we have to remember, that right now we have the nice property if byzantine events empty, then all good, which is nice to keep.

Closing comment: let's put this under the umbrella of UX-related improvements to the status.get reply/events and the UX of unhappy-path handling in general. Current impl is still PoC-level from this perspective. Calling out to @Pongch to have an eye on this.