Clear packets on hermes start #1201
Conversation
relayer/src/worker/packet.rs
Outdated
| // at predefined block intervals. | ||
| if self.clear_packets_interval != 0 | ||
| && height.revision_height % self.clear_packets_interval == 0 | ||
| if link.a_to_b.clear_packets() |
There was a problem hiding this comment.
Two remarks for this change:
-
It can trigger
link.a_to_b.do_clear_packetsrepeatedly until theclear_packetsflag is set to false. This flag is set to false only when the worker receives a command with a batch of events (that is the first match arm above, on line 103). -
If I understand correctly, with this change there may be two separate invocations to
link.a_to_b.do_clear_packets:- one invocation upon NewBlock (this call site), which would clear packets for that
NewBlock's height - another invocation with the call site within
update_schedule(batch), which would clear packets for the height of thatbatch, see the snippet below:
- one invocation upon NewBlock (this call site), which would clear packets for that
If the height of NewBlock event and the batch height are the same, then we'll clear packets twice for the same height. Worst case, this will schedule twice the execution of the same operational data.
BTW I tried to reproduce point (2) but was not able. I was only able to reproduce problem (1) above.
There was a problem hiding this comment.
I attempted an alternative fix
628425b
Feel free to revert if this approach is not appropriate.
There was a problem hiding this comment.
If the height of NewBlock event and the batch height are the same, then we'll clear packets twice for the same height. Worst case, this will schedule twice the execution of the same operational data.
Ah yes, good point! This is in fact the case in v0.5.0 and older versions also. We used to get the IBC event before the New Block event (at same height) so we had to clear on IBC event also. With the changes in the scheduler and monitor we now always get the NewBlock before the IBC event. I think we can clear packets on NewBlock only. I will try some changes along these lines so we simplify the code quite a bit.
I was only able to reproduce problem (1) above.
How?
There was a problem hiding this comment.
How?
Steps to reproduce (1)
- dev-env with two chains, and create a channel between the two;
git co 872a503andstarthermes. The logs show
Jul 15 09:24:16.570 INFO [ibc-1:transfer/channel-0 -> ibc-0] clearing pending packets from events before height 1-312
Jul 15 09:24:16.581 INFO [ibc-1:transfer/channel-0 -> ibc-0] finished scheduling the clearing of pending packets
Jul 15 09:24:16.886 INFO [ibc-0:transfer/channel-0 -> ibc-1] clearing pending packets from events before height 0-324
Jul 15 09:24:16.902 INFO [ibc-0:transfer/channel-0 -> ibc-1] finished scheduling the clearing of pending packets
Jul 15 09:24:17.723 INFO [ibc-1:transfer/channel-0 -> ibc-0] clearing pending packets from events before height 1-313
Jul 15 09:24:17.733 INFO [ibc-1:transfer/channel-0 -> ibc-0] finished scheduling the clearing of pending packets
Jul 15 09:24:18.091 INFO [ibc-0:transfer/channel-0 -> ibc-1] clearing pending packets from events before height 0-325
Jul 15 09:24:18.101 INFO [ibc-0:transfer/channel-0 -> ibc-1] finished scheduling the clearing of pending packets
Jul 15 09:24:18.872 INFO [ibc-1:transfer/channel-0 -> ibc-0] clearing pending packets from events before height 1-314
Jul 15 09:24:18.880 INFO [ibc-1:transfer/channel-0 -> ibc-0] finished scheduling the clearing of pending packets
Jul 15 09:24:19.190 INFO [ibc-0:transfer/channel-0 -> ibc-1] clearing pending packets from events before height 0-326
The problem was: the flag link.a_to_b.clear_packets is never set to false, so upon every NewBlock event, the method do_clear_packets is invoked.
* Fix script to catch the packets not cleared on start * Clear packets on start * Attempt at an alternative fix * Clear packets only on NewBlock event * Changelog Co-authored-by: Adi Seredinschi <adi@informal.systems>
Closes: #1200
Partially closes: #1196
Description
Hermes does not clear packets on start if there are no IBC events and no
clear_packets_intervaltrigger.The e2e script masks this error because it sends some packets, starts hermes and then sends more packets (issue masked here because this triggers packet clearing) before checking that all packets are cleared.
For contributor use:
docs/) and code comments.Files changedin the Github PR explorer.