Skip to content

Commit

Permalink
ibc: delete packet commitments instead of writing empty values (#4644)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
avahowell authored Jun 21, 2024
1 parent b6ce49a commit 623872b
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 5 deletions.
29 changes: 26 additions & 3 deletions crates/bin/pd/src/migrate/testnet78.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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?;

Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions crates/core/component/ibc/src/component/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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![],
);
}

Expand Down

0 comments on commit 623872b

Please sign in to comment.