Add error handling logic to relayer spec #236

milosevic · 2020-09-17T12:33:28Z

Fixes #234 and #237.

codecov-commenter · 2020-09-17T12:43:51Z

Codecov Report

Merging #236 into master will increase coverage by 23.9%.
The diff coverage is 54.5%.

@@            Coverage Diff            @@
##           master    #236      +/-   ##
=========================================
+ Coverage    13.6%   37.6%   +23.9%     
=========================================
  Files          69      79      +10     
  Lines        3752    5652    +1900     
  Branches     1374    1870     +496     
=========================================
+ Hits          513    2127    +1614     
- Misses       2618    3318     +700     
+ Partials      621     207     -414

Impacted Files	Coverage Δ
modules/src/events.rs	`0.0% <ø> (ø)`
modules/src/ics02_client/events.rs	`0.0% <ø> (ø)`
modules/src/ics02_client/raw.rs	`0.0% <0.0%> (ø)`
modules/src/ics03_connection/error.rs	`9.6% <0.0%> (-23.7%)`	⬇️
modules/src/ics04_channel/error.rs	`23.0% <0.0%> (-2.0%)`	⬇️
modules/src/ics04_channel/packet.rs	`0.0% <0.0%> (ø)`
modules/src/ics07_tendermint/client_def.rs	`0.0% <0.0%> (ø)`
modules/src/ics07_tendermint/error.rs	`30.0% <0.0%> (+30.0%)`	⬆️
modules/src/ics07_tendermint/msgs/update_client.rs	`0.0% <0.0%> (ø)`
modules/src/mock_client/header.rs	`0.0% <0.0%> (ø)`
... and 134 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4877ee9...441e3bb. Read the comment docs.

docs/spec/relayer/Definitions.md

ancazamfir · 2020-09-21T07:23:59Z

docs/spec/relayer/Packets.md


+    if channel == nil OR channel.state != OPEN  { (nil, Error.RETRY) } 
+    // TODO: Maybe we shouldn't even enter handle loop for packets if the corresponding channel is not open!


Yes but I think here is the first time when we can make that determination.

What I meant here is basically having supervisor logic that would decide when to call handleEvent based on event type, i.e., there would be some preconditions that are based on relayer state. For example, packet related events are preconditioned on channel and connections being open, all events are preconditioned on light client correctly installed, etc.

Does this assume that the relayer keeps state for channel and connections? And the supervisor does not perform any queries to determine that. Trying to save on the number of queries :)

Exactly. Supervisor will need anyway to keep some state (for example for pending packets so it can trigger timeouts), so why not keeping status of connections and channels, current block height and time, etc. It can save queries and potentially operates faster.

docs/spec/relayer/Relayer.md

docs/spec/relayer/Definitions.md

docs/spec/relayer/Relayer.md

docs/spec/relayer/Packets.md

istoilkovska · 2020-09-17T15:12:59Z

docs/spec/relayer/Relayer.md

+1. Determine destination chain (`chainB`)
+2. Updates (on `chainB`) the IBC client for `chainA` to a certain height `H` where `H >= h+1`.
+3. Create IBC datagram at height `H-1`.
+4. Submit the datagram from stage (2) to `chainB`.


Suggested change

4. Submit the datagram from stage (2) to `chainB`.

4. Submit the datagram from stage (3) to `chainB`.

docs/spec/relayer/Definitions.md

docs/spec/relayer/Relayer.md

ancazamfir

Looks great! A few comments looking for modularizing a bit the checks so the pseudocode is easier to read, if possible.

ancazamfir · 2020-09-24T14:26:43Z

docs/spec/relayer/Packets.md


+    if channel == nil OR channel.state != OPEN  { (nil, Error.RETRY) } 
+    // TODO: Maybe we shouldn't even enter handle loop for packets if the corresponding channel is not open!


Does this assume that the relayer keeps state for channel and connections? And the supervisor does not perform any queries to determine that. Trying to save on the number of queries :)

ancazamfir · 2020-09-24T14:29:02Z

docs/spec/relayer/Packets.md

+    // Fetch channelEnd from the chainA to be able to compute port and chain ids on destination chain
+    channelA, proof, error = GetChannel(chainA, ev.port, ev.channel, ev.height)
+    if error != nil { return (nil, error) }
+
+    channelB, proof, error = 
+        GetChannel(chainB, channelA.counterpartyPortIdentifier, channelA.counterpartyChannelIdentifier, LATEST_HEIGHT)
+    if error != nil { return (nil, error) }
+
+    if channelB == nil OR channel.state != OPEN  { (nil, Error.DROP) } 
+    // Note that we checked implicitly above that counterparty identifiers match each other
+


Are these channel retrievals and checks the same as for the SendPacket event above? If yes can we make a single function for this? If not maybe we cover the differences and why they are.

Checks are similar, but error message is different. With recv packets we can retry on error (if connection and channel are not open) while for acks we should drop event in this case. We can make a function for this (and we should at the code level) but at the spec level maybe it's better making it more verbose so reading it is easier.

ancazamfir · 2020-09-24T14:29:27Z

docs/spec/relayer/Packets.md

+    connectionId = channelB.connectionHops[0]
+    connection, proof, error = GetConnection(chainB, connectionId, LATEST_HEIGHT) 
+    if error != nil { return (nil, error) }
+
+    if connection == nil OR connection.state != OPEN { return (nil, Error.DROP) }


same question here.

ancazamfir · 2020-09-24T14:30:12Z

docs/spec/relayer/Packets.md

+    packetCommitment, packetCommitmentProof, error = 
+        GetPacketCommitment(chainB, channelA.counterpartyPortIdentifier, 
+                            channelA.counterpartyChannelIdentifier, ev.sequence, LATEST_HEIGHT)     
+    if error != nil { return (nil, error) }
+
+    if packetCommitment == nil OR
+       packetCommitment != hash(concat(ev.data, ev.timeoutHeight, ev.timeoutTimestamp)) { 
+            // invalid event; bad provider
+            return (nil, Error.BADPROVIDER) 
+    }
+


ancazamfir

Looks good, thanks Zarko!

Add error handling and reorganise the spec

milosevic added 4 commits September 16, 2020 15:46

Add initial error handling logic in relayer main loop

80e4f39

Add error handling for the createDatagram function

4f540fd

Add error handling and reorganise spec better

ab21ffa

Fix off by one height

441e3bb

milosevic requested review from ancazamfir and romac as code owners September 17, 2020 12:33

milosevic requested a review from istoilkovska September 17, 2020 12:33

milosevic added 3 commits September 18, 2020 12:28

Add WriteAcknowledgementEvent handler

1809f1c

Small cleaning

4904478

Small cleaning

1a42436

ancazamfir reviewed Sep 21, 2020

View reviewed changes

istoilkovska reviewed Sep 21, 2020

View reviewed changes

milosevic added 2 commits September 22, 2020 12:02

Improve error handling

d3ce6af

Improve error handling

777957e

ancazamfir reviewed Sep 24, 2020

View reviewed changes

ancazamfir assigned milosevic Sep 25, 2020

ancazamfir approved these changes Sep 29, 2020

View reviewed changes

milosevic merged commit 0fa2b89 into master Sep 29, 2020

This was referenced Sep 30, 2020

Extend relayer spec to handle packet acknowledgments #237

Closed

English specification of SendPacketEvent handling #229

Closed

romac deleted the zarko/add-error-handling-234 branch April 8, 2021 20:41

hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022

Add error handling logic to relayer spec (informalsystems#236)

4271d16

Add error handling and reorganise the spec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error handling logic to relayer spec #236

Add error handling logic to relayer spec #236

milosevic commented Sep 17, 2020 •

edited

Loading

codecov-commenter commented Sep 17, 2020

ancazamfir Sep 21, 2020

milosevic Sep 22, 2020

ancazamfir Sep 24, 2020

milosevic Sep 29, 2020

istoilkovska Sep 17, 2020

ancazamfir left a comment

ancazamfir Sep 24, 2020

ancazamfir Sep 24, 2020

milosevic Sep 29, 2020

ancazamfir Sep 24, 2020

ancazamfir Sep 24, 2020

ancazamfir left a comment


		if channel == nil OR channel.state != OPEN { (nil, Error.RETRY) }
		// TODO: Maybe we shouldn't even enter handle loop for packets if the corresponding channel is not open!

	4. Submit the datagram from stage (2) to `chainB`.
	4. Submit the datagram from stage (3) to `chainB`.

Add error handling logic to relayer spec #236

Add error handling logic to relayer spec #236

Conversation

milosevic commented Sep 17, 2020 • edited Loading

codecov-commenter commented Sep 17, 2020

Codecov Report

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

ancazamfir 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

ancazamfir left a comment

Choose a reason for hiding this comment

milosevic commented Sep 17, 2020 •

edited

Loading