From 623872bdc3015202ade744cffbd6eeab487d5ce7 Mon Sep 17 00:00:00 2001
From: Ava Howell <ava@avahowell.me>
Date: Thu, 20 Jun 2024 19:40:37 -0700
Subject: [PATCH] ibc: delete packet commitments instead of writing empty
 values (#4644)

The IBC spec calls for deletion of packet commitments after a
successful acknowledgement, in order to avoid double-acknowledgements.

Previously, our implementation accomplished this deletion by writing
an empty byte slice vec![] to the commitment path.

However, this will cause nonexistence proofs by exclusion (the
standard NX-proof type used in ics23) of adjacent keys to fail,
since ics23 does not support existence proofs of empty values.

This PR changes `delete_packet_commitment` to actually delete the
commitment, and introduces a migration for testnet 78 that will
delete all of the previously overwritten commitments.
---
 crates/bin/pd/src/migrate/testnet78.rs        | 29 +++++++++++++++++--
 .../component/ibc/src/component/channel.rs    |  3 +-
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/crates/bin/pd/src/migrate/testnet78.rs b/crates/bin/pd/src/migrate/testnet78.rs
index c942774964..0074b4b034 100644
--- a/crates/bin/pd/src/migrate/testnet78.rs
+++ b/crates/bin/pd/src/migrate/testnet78.rs
@@ -1,5 +1,6 @@
 //! Contains functions related to the migration script of Testnet78.
 use anyhow::Context;
+use cnidarium::StateRead;
 use cnidarium::{Snapshot, StateDelta, StateWrite, Storage};
 use futures::TryStreamExt as _;
 use futures::{pin_mut, StreamExt};
@@ -49,6 +50,7 @@ use crate::testnet::generate::TestnetConfig;
 ///   * Governance Signaling Proposals:
 ///    - `commit hash` (255 bytes)
 /// - Close and re-open all *open* positions so that they are re-indexed.
+/// - Update DEX parameters
 #[instrument]
 pub async fn migrate(
     storage: Storage,
@@ -78,6 +80,9 @@ pub async fn migrate(
     let (migration_duration, post_upgrade_root_hash) = {
         let start_time = std::time::SystemTime::now();
 
+        // Migrate empty (deleted) packet commitments to be deleted at the tree level
+        delete_empty_deleted_packet_commitments(&mut delta).await?;
+
         // Adjust the length of `Validator` fields.
         truncate_validator_fields(&mut delta).await?;
 
@@ -87,12 +92,12 @@ pub async fn migrate(
         // Adjust the length of governance proposal outcome fields.
         truncate_proposal_outcome_fields(&mut delta).await?;
 
-        // Re-index all open positions.
-        reindex_dex_positions(&mut delta).await?;
-
         // Write the new dex parameters (with the execution budget field) to the state.
         update_dex_params(&mut delta).await?;
 
+        // Re-index all open positions.
+        reindex_dex_positions(&mut delta).await?;
+
         // Reset the application height and halt flag.
         delta.ready_to_start();
         delta.put_block_height(0u64);
@@ -155,6 +160,24 @@ pub async fn migrate(
     Ok(())
 }
 
+async fn delete_empty_deleted_packet_commitments(
+    delta: &mut StateDelta<Snapshot>,
+) -> anyhow::Result<()> {
+    let prefix_key = "ibc-data/commitments/";
+    let stream = delta.prefix_raw(&prefix_key);
+
+    pin_mut!(stream);
+
+    while let Some(entry) = stream.next().await {
+        let (substore_key, value) = entry?;
+        if value.is_empty() {
+            delta.delete(format!("ibc-data/{substore_key}"));
+        }
+    }
+
+    Ok(())
+}
+
 /// Write the updated dex parameters to the chain state.
 async fn update_dex_params(delta: &mut StateDelta<Snapshot>) -> anyhow::Result<()> {
     let mut dex_params = delta
diff --git a/crates/core/component/ibc/src/component/channel.rs b/crates/core/component/ibc/src/component/channel.rs
index be16eeaf09..490f804737 100644
--- a/crates/core/component/ibc/src/component/channel.rs
+++ b/crates/core/component/ibc/src/component/channel.rs
@@ -81,11 +81,10 @@ pub trait StateWriteExt: StateWrite + StateReadExt {
         port_id: &PortId,
         sequence: u64,
     ) {
-        self.put_raw(
+        self.delete(
             IBC_COMMITMENT_PREFIX.apply_string(
                 CommitmentPath::new(port_id, channel_id, sequence.into()).to_string(),
             ),
-            vec![],
         );
     }