Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(dex): migration for dex trait proto encodings #4341

Merged
merged 2 commits into from
May 8, 2024
Merged

fix(dex): migration for dex trait proto encodings #4341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32055fd
Add migration for dex/ab/ values to store as proto-encoded
zbuc May 7, 2024
7524efd
ci: fix lfs=true on check-all-crates
conorsch May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ jobs:
runs-on: buildjet-8vcpu-ubuntu-2204
steps:
- uses: actions/checkout@v4
with:
lfs: true
- name: Load rust cache
uses: astriaorg/[email protected]
- name: install cargo-hack
Expand Down
38 changes: 36 additions & 2 deletions crates/bin/pd/src/migrate/testnet74.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,40 @@ async fn fix_arb_execution_outputs(delta: &mut StateDelta<Snapshot>) -> anyhow::
Ok(())
}

/// Update base liquidity index values to be proto-encoded. Previously they were stored as big-endian
/// encoded amounts, but in https://github.com/penumbra-zone/penumbra/pull/4188 they were changed
/// to be proto-encoded.
///
/// This will rewrite all values under the `dex/ab/` prefix to be proto-encoded.
async fn rewrite_base_liquidity_indices(delta: &mut StateDelta<Snapshot>) -> anyhow::Result<()> {
let prefix_key = "dex/ab/".as_bytes();
tracing::trace!(prefix_key = ?EscapedByteSlice(&prefix_key), "updating base liquidity index values");
let mut liquidity_stream = delta.nonverifiable_prefix_raw(&prefix_key).boxed();

while let Some(r) = liquidity_stream.next().await {
let (key, raw_amount): (Vec<u8>, Vec<u8>) = r?;
tracing::info!(?key, raw_amount = ?EscapedByteSlice(&raw_amount), "migrating base liquidity index entry");

let amount = Amount::from_be_bytes(raw_amount.as_slice().try_into()?);

// Store the correctly formatted new value:
delta.nonverifiable_put(key.clone(), amount);
tracing::info!(
key = ?EscapedByteSlice(&key),
raw_amount = ?EscapedByteSlice(&raw_amount),
?amount,
"updated base liquidity index"
);
}

Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this migration fn idempotent? We've been telling folks our migrations are idempotent, but not sure that's the case for this one. Zooming out, the changes in #4339 definitely aren't idempotent, so not blocking on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. No, it is not idempotent. I wasn't aware that was a requirement for migrations; I will follow-up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this not idempotent? if you run it multiple times you will get the same result, no? why not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it because the encoding changes? that's completely fine imo

Copy link
Member

@zbuc zbuc May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this not idempotent? if you run it multiple times you will get the same result, no? why not?

Looking more closely, I am not sure at the moment. If we attempt to decode protobuf encoded Amount as a BE-encoded Amount it could fail silently and the Amount would change. However, this requires the protobuf encoding to be the same length as the big-endian encoding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly not completely sure this is important but we can try to first decode it as a proto amount and return early if it works, and proceed if it correctly returns an error. This way it is resistant to accidental double runs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some edge cases:

        let a: Amount = 4951760157141521099596496895u128.into();

        println!("amount 1: {:?}", a);

        let proto_a_bytes: Vec<u8> = penumbra_proto::core::num::v1::Amount::from(a).encode_to_vec();

        let amount = Amount::from_be_bytes(proto_a_bytes.as_slice().try_into()?);

        println!("amount 2: {:?}", amount);
amount 1: 4951760157141521099596496895

amount 2: 11963051962064242856133983240072462207

so it could theoretically still munge some values and be non-idempotent even if we took that approach if everything aligned and the stored Amounts were in a specific range and order.

But I'm also not convinced it's worth trying to fix... migration idempotency seems better to solve at a higher level than within individual functions modifying data if it's a design goal, as there are likely other similar cases that will come up in migration code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah there's some Discord discussion about this already: https://discord.com/channels/824484045370818580/1052302055680790568/1228444060113698938

Right now, migrations are not idempotent. This may require publishing the post-upgrade root hash, however that doesn't account for migrations affecting non-verifiable storage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not tell people migrations are idempotent.

The right way to handle idempotency, later, is probably to use the ABCI application version and compare against a version number in the state.


/// Update the ordering of liquidity position indices to return in descending order (see #4189)
///
/// Lookups for liquidity positions based on starting asset were ordered backwards
/// and returning the positions with the least liquidity first. This migration
/// needs to modify the keys stored under the JMT `dex/ra/` prefix key to reverse
/// needs to modify the keys stored under the nonverifiable `dex/ra/` prefix key to reverse
/// the ordering of the existing data.
async fn update_lp_index_order(delta: &mut StateDelta<Snapshot>) -> anyhow::Result<()> {
let prefix_key = "dex/ra/".as_bytes();
Expand Down Expand Up @@ -81,11 +110,13 @@ async fn update_lp_index_order(delta: &mut StateDelta<Snapshot>) -> anyhow::Resu
/// This migration script is responsible for:
///
/// - Updating the ordering of liquidity position indices to return in descending order (see #4189)
/// * JMT: `dex/ra/`
/// * nonverifiable: `dex/ra/`
/// - Updating arb execution output amounts to include the input amount instead of reporting only profit (see #3790)
/// * JMT: `dex/arb_execution/`
/// - Add `AuctionParameters` to the chain state
/// * JMT: `dex/auction_parameters`
/// - Update the base liquidity index values to be proto-encoded (see #4188)
/// * nonverifiable: `dex/ab/`
pub async fn migrate(
path_to_export: PathBuf,
genesis_start: Option<tendermint::time::Time>,
Expand Down Expand Up @@ -117,6 +148,9 @@ pub async fn migrate(
// Write auction parameters
write_auction_parameters(&mut delta).await?;

// Rewrite base liquidity indices as proto-encoded
rewrite_base_liquidity_indices(&mut delta).await?;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we circle back to addressing whether or not we want to enforce that all migrations are idempotent, we can sprinkle in some anyhow::Context usage on these calls. The tracing spans are already quite helpful in debugging what failed in a migration, but more context is never a bad thing IMO.

delta.put_block_height(0u64);
let post_upgrade_root_hash = storage.commit_in_place(delta).await?;
tracing::info!(?post_upgrade_root_hash, "post-upgrade root hash");
Expand Down
Loading