Duplicate packets on start (informalsystems#2256)

* Pull in upstream changes

* Remove unnecessary destructuring assignment

* Decrement height when scheduling packet clearing

* Add changelog entry

* Clarify comment

* Remove unnecessary destructure

* Initiate clear packet flow when a IbcEvent is received

* Revert to more straightforward logic to see if CI still breaks

* Refactor handle_packet_cmd

* Clean up handle_packet_cmd

* Update handle_packet_cmd doc comment

* Incorporate PR feedback

.changelog/unreleased/bug-fixes/ibc-relayer/2093-fix-duplicate-packets-on-start.md

@@ -0,0 +1 @@
+- Fix duplicate packets being generated on start. ([#2093](https://github.com/informalsystems/ibc-rs/issues/2093))

relayer/src/worker/packet.rs

@@ -99,49 +99,71 @@ pub fn spawn_packet_cmd_worker<ChainA: ChainHandle, ChainB: ChainHandle>(
     })
 }
 
-/// Receives worker commands, which may be:
-///     - IbcEvent => then it updates schedule
-///     - NewBlock => schedules packet clearing
-///     - Shutdown => exits
+/// Receives worker commands and handles them accordingly.
 ///
-/// Regardless of the incoming command, this method
-/// also refreshes and executes any scheduled operational
-/// data that is ready.
+/// Given an `IbcEvent` command, updates the schedule and initiates
+/// packet clearing if the `should_clear_on_start` flag has been toggled.
+///
+/// Given a `NewBlock` command, checks if packet clearing should occur
+/// and performs it if so.
+///
+/// Given a `ClearPendingPackets` command, clears pending packets.
+///
+/// Regardless of the incoming command, this method also refreshes and
+/// and executes any scheduled operational data that is ready.
 fn handle_packet_cmd<ChainA: ChainHandle, ChainB: ChainHandle>(
     link: &mut Link<ChainA, ChainB>,
     should_clear_on_start: &mut bool,
     clear_interval: u64,
     path: &Packet,
     cmd: WorkerCmd,
 ) -> Result<(), TaskError<RunError>> {
-    match cmd {
-        WorkerCmd::IbcEvents { batch } => handle_update_schedule(link, clear_interval, path, batch),
-
-        // Handle the arrival of an event signaling that the
-        // source chain has advanced to a new block.
-        WorkerCmd::NewBlock {
-            height,
-            new_block: _,
-        } => {
+    // Handle packet clearing which is triggered from a command
+    let (do_clear, maybe_height) = match &cmd {
+        WorkerCmd::IbcEvents { batch } => {
             if *should_clear_on_start {
-                handle_clear_packet(link, clear_interval, path, Some(height))?;
+                (true, Some(batch.height))
+            } else {
+                (false, None)
+            }
+        }
 
-                // Clear the flag only if handle_clear_packet succeeds
-                *should_clear_on_start = false;
-                Ok(())
-            } else if should_clear_packets(clear_interval, height) {
-                handle_clear_packet(link, clear_interval, path, Some(height))
+        // Handle the arrival of an event signaling that the
+        // source chain has advanced to a new block
+        WorkerCmd::NewBlock { height, .. } => {
+            if *should_clear_on_start || should_clear_packets(clear_interval, *height) {
+                (true, Some(*height))
             } else {
-                Ok(())
+                (false, None)
             }
         }
 
-        WorkerCmd::ClearPendingPackets => handle_clear_packet(link, clear_interval, path, None),
+        WorkerCmd::ClearPendingPackets => (true, None),
+    };
+
+    if do_clear {
+        handle_clear_packet(link, clear_interval, path, maybe_height)?;
+
+        // Reset the `clear_on_start` flag
+        if *should_clear_on_start {
+            *should_clear_on_start = false;
+        }
+    }
+
+    // Handle command-specific task
+    if let WorkerCmd::IbcEvents { batch } = cmd {
+        handle_update_schedule(link, clear_interval, path, batch)
+    } else {
+        Ok(())
     }
 }
 
-/// Whether or not to clear pending packets at this `step` for the given height.
-/// Packets are cleared if `clear_interval` is not `0` and if we have reached the interval.
+/// Whether or not to clear pending packets at this `step` for some height.
+/// If the relayer has been configured to clear packets on start and that has not
+/// occurred yet, then packets are cleared.
+///
+/// If the specified height is reached, then packets are cleared if `clear_interval`
+/// is not `0` and if we have reached the interval.
 fn should_clear_packets(clear_interval: u64, height: Height) -> bool {
     clear_interval != 0 && height.revision_height % clear_interval == 0
 }