From b65e3d9756d2a7ba48ca3ee2b68b2350863b4bda Mon Sep 17 00:00:00 2001 From: Lucas Meier Date: Wed, 13 Nov 2024 11:12:24 -0700 Subject: [PATCH 01/10] UIP-6: App Version Safeguard --- crates/bin/pd/src/main.rs | 2 + crates/bin/pd/src/migrate/mainnet1.rs | 11 ++ crates/core/app/src/app_version.rs | 10 ++ crates/core/app/src/app_version/component.rs | 133 +++++++++++++++++++ crates/core/app/src/lib.rs | 5 +- 5 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 crates/core/app/src/app_version.rs create mode 100644 crates/core/app/src/app_version/component.rs diff --git a/crates/bin/pd/src/main.rs b/crates/bin/pd/src/main.rs index 4c2f003ae4..db02281fbe 100644 --- a/crates/bin/pd/src/main.rs +++ b/crates/bin/pd/src/main.rs @@ -19,6 +19,7 @@ use pd::{ join::network_join, }, }; +use penumbra_app::app_version::{assert_latest_app_version, migrate_app_version}; use penumbra_app::SUBSTORE_PREFIXES; use rand::Rng; use rand_core::OsRng; @@ -102,6 +103,7 @@ async fn main() -> anyhow::Result<()> { .context( "Unable to initialize RocksDB storage - is there another `pd` process running?", )?; + assert_latest_app_version(storage.clone()).await?; tracing::info!( ?abci_bind, diff --git a/crates/bin/pd/src/migrate/mainnet1.rs b/crates/bin/pd/src/migrate/mainnet1.rs index 2045c7f14c..1a51e78aeb 100644 --- a/crates/bin/pd/src/migrate/mainnet1.rs +++ b/crates/bin/pd/src/migrate/mainnet1.rs @@ -7,6 +7,7 @@ use ibc_types::core::channel::{Packet, PortId}; use ibc_types::transfer::acknowledgement::TokenTransferAcknowledgement; use jmt::RootHash; use penumbra_app::app::StateReadExt as _; +use penumbra_app::app_version::migrate_app_version; use penumbra_governance::StateWriteExt; use penumbra_ibc::{component::ChannelStateWriteExt as _, IbcRelay}; use penumbra_sct::component::clock::EpochManager; @@ -111,6 +112,16 @@ pub async fn migrate( let (migration_duration, post_upgrade_root_hash) = { let start_time = std::time::SystemTime::now(); + // Note, when this bit of code was added, the upgrade happened months ago, + // and the verison safeguard mechanism was not in place. However, + // adding this will prevent someone running version 0.80.X with the + // safeguard patch from accidentally running the migraton again, since they + // will already have version 8 written into the state. But, if someone is syncing + // up from genesis, then version 0.79 will not have written anything into the safeguard, + // and this method will not complain. So, this addition provides a safeguard + // for existing nodes, while also not impeding syncing up a node from scratch. + migrate_app_version(&mut delta, 8).await?; + // Reinsert all of the erroneously removed packets replace_lost_packets(&mut delta).await?; diff --git a/crates/core/app/src/app_version.rs b/crates/core/app/src/app_version.rs new file mode 100644 index 0000000000..93ad9afc47 --- /dev/null +++ b/crates/core/app/src/app_version.rs @@ -0,0 +1,10 @@ +/// Representation of the Penumbra application version. Notably, this is distinct +/// from the crate version(s). This number should only ever be incremented. +pub const APP_VERSION: u64 = 8; + +cfg_if::cfg_if! { + if #[cfg(feature="component")] { + mod component; + pub use component::{assert_latest_app_version, migrate_app_version}; + } +} diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs new file mode 100644 index 0000000000..52af54cd6c --- /dev/null +++ b/crates/core/app/src/app_version/component.rs @@ -0,0 +1,133 @@ +use std::fmt::Write as _; + +use anyhow::{anyhow, Context}; +use cnidarium::{StateDelta, StateRead, StateWrite, Storage}; + +use super::APP_VERSION; + +fn version_to_software_version(version: u64) -> &'static str { + match version { + 1 => "v0.70.x", + 2 => "v0.73.x", + 3 => "v0.74.x", + 4 => "v0.75.x", + 5 => "v0.76.x", + 6 => "v0.77.x", + 7 => "v0.79.x", + 8 => "v0.80.x", + _ => "unknown", + } +} + +#[derive(Debug, Clone, Copy)] +enum CheckContext { + Running, + Migration, +} + +fn check_version(ctx: CheckContext, expected: u64, found: Option) -> anyhow::Result<()> { + let found = match (expected, found) { + (x, Some(y)) if x != y => y, + _ => return Ok(()), + }; + match ctx { + CheckContext::Running => { + let expected_name = version_to_software_version(expected); + let found_name = version_to_software_version(expected); + let mut error = String::new(); + error.push_str("app version mismatch:\n"); + write!( + &mut error, + " expected {} (penumbra {})\n", + expected, expected_name + )?; + write!(&mut error, " found {} (penumbra {})\n", found, found_name)?; + write!( + &mut error, + "make sure you're running penumbra {}", + expected_name + )?; + Err(anyhow!(error)) + } + CheckContext::Migration => { + let expected_name = version_to_software_version(expected); + let found_name = version_to_software_version(expected); + let mut error = String::new(); + error.push_str("app version mismatch:\n"); + write!( + &mut error, + " expected {} (penumbra {})\n", + expected, expected_name + )?; + write!(&mut error, " found {} (penumbra {})\n", found, found_name)?; + write!( + &mut error, + "this migration should be run with penumbra {} instead", + version_to_software_version(expected + 1) + )?; + Err(anyhow!(error)) + } + } +} + +fn state_key() -> Vec { + b"penumbra_app_version_safeguard".to_vec() +} + +async fn read_app_version_safeguard(s: &S) -> anyhow::Result> { + const CTX: &'static str = "while reading app_version_safeguard"; + + let res = s.nonverifiable_get_raw(&state_key()).await.context(CTX)?; + match res { + None => Ok(None), + Some(x) => { + let bytes: [u8; 8] = x + .try_into() + .map_err(|bad: Vec| { + anyhow!("expected bytes to have length 8, found: {}", bad.len()) + }) + .context(CTX)?; + Ok(Some(u64::from_le_bytes(bytes))) + } + } +} + +// Neither async nor a result are needed, but only right now, so I'm putting these here +// to reserve the right to change them later. +async fn write_app_version_safeguard(s: &mut S, x: u64) -> anyhow::Result<()> { + let bytes = u64::to_le_bytes(x).to_vec(); + s.nonverifiable_put_raw(state_key(), bytes); + Ok(()) +} + +/// Assert that the app version saved in the state is the correct one. +/// +/// You should call this before starting a node. +/// +/// This will succeed if no app version is saved, or if the app version saved matches +/// exactly. +/// +/// This will also result in the current app version being stored, so that future +/// calls to this function will be checked against this state. +pub async fn assert_latest_app_version(s: Storage) -> anyhow::Result<()> { + let mut delta = StateDelta::new(s.latest_snapshot()); + let found = read_app_version_safeguard(&delta).await?; + check_version(CheckContext::Running, APP_VERSION, found)?; + write_app_version_safeguard(&mut delta, APP_VERSION).await?; + s.commit(delta).await?; + Ok(()) +} + +/// Migrate the app version to a given number. +/// +/// This will check that the app version is currently the previous version, if set at all. +/// +/// This is the only way to change the app version, and should be called during a migration +/// with breaking consensus logic. +pub async fn migrate_app_version(s: &mut S, to: u64) -> anyhow::Result<()> { + anyhow::ensure!(to > 1, "you can't migrate to the first penumbra version!"); + let found = read_app_version_safeguard(s).await?; + check_version(CheckContext::Migration, to - 1, found)?; + write_app_version_safeguard(s, to).await?; + Ok(()) +} diff --git a/crates/core/app/src/lib.rs b/crates/core/app/src/lib.rs index 21e75bd50d..46fc1f1d84 100644 --- a/crates/core/app/src/lib.rs +++ b/crates/core/app/src/lib.rs @@ -13,9 +13,8 @@ pub static SUBSTORE_PREFIXES: Lazy> = Lazy::new(|| { /// The substore prefix used for storing historical CometBFT block data. pub static COMETBFT_SUBSTORE_PREFIX: &'static str = "cometbft-data"; -/// Representation of the Penumbra application version. Notably, this is distinct -/// from the crate version(s). This number should only ever be incremented. -pub const APP_VERSION: u64 = 8; +pub mod app_version; +pub use app_version::APP_VERSION; pub mod genesis; pub mod params; From 8141304efa2de3c2eb0a71829eff96ce43a2df13 Mon Sep 17 00:00:00 2001 From: Lucas Meier Date: Wed, 13 Nov 2024 11:22:39 -0700 Subject: [PATCH 02/10] Remove unused imports --- crates/bin/pd/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bin/pd/src/main.rs b/crates/bin/pd/src/main.rs index db02281fbe..7f2ead78ca 100644 --- a/crates/bin/pd/src/main.rs +++ b/crates/bin/pd/src/main.rs @@ -19,7 +19,7 @@ use pd::{ join::network_join, }, }; -use penumbra_app::app_version::{assert_latest_app_version, migrate_app_version}; +use penumbra_app::app_version::assert_latest_app_version; use penumbra_app::SUBSTORE_PREFIXES; use rand::Rng; use rand_core::OsRng; From ce5d66703af06479be9c25ddf25b386e2cbf9368 Mon Sep 17 00:00:00 2001 From: Lucas Meier Date: Wed, 13 Nov 2024 12:37:49 -0700 Subject: [PATCH 03/10] Don't touch uninitialized storage for version safeguard This prevents an edge case where PD would crash if starting before the very first genesis. Not touching storage in that case will prevent nodes running continuously from genesis from benefitting from the safeguard, but an upgrade has already happened on mainnet, and so we don't care about not having the safeguard in this case. --- crates/core/app/src/app_version/component.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index 52af54cd6c..ddc5f8c946 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -110,6 +110,11 @@ async fn write_app_version_safeguard(s: &mut S, x: u64) -> anyhow /// This will also result in the current app version being stored, so that future /// calls to this function will be checked against this state. pub async fn assert_latest_app_version(s: Storage) -> anyhow::Result<()> { + // If the storage is not initialized, avoid touching it at all, + // to avoid complaints about it already being initialized before the first genesis. + if s.latest_version() == u64::MAX { + return Ok(()); + } let mut delta = StateDelta::new(s.latest_snapshot()); let found = read_app_version_safeguard(&delta).await?; check_version(CheckContext::Running, APP_VERSION, found)?; From af3d1c0a6bd623d18ff06b6132cdf2635b40ce36 Mon Sep 17 00:00:00 2001 From: Lucas Meier Date: Wed, 13 Nov 2024 13:08:14 -0700 Subject: [PATCH 04/10] Use State[Read/Write]Proto for app version safeguard Simplifies logic significantly Co-authored-by: @erwanor --- crates/core/app/src/app_version/component.rs | 32 +++++++------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index ddc5f8c946..9aa5dd63b6 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -1,7 +1,8 @@ use std::fmt::Write as _; use anyhow::{anyhow, Context}; -use cnidarium::{StateDelta, StateRead, StateWrite, Storage}; +use cnidarium::{StateDelta, Storage}; +use penumbra_proto::{StateReadProto, StateWriteProto}; use super::APP_VERSION; @@ -74,29 +75,18 @@ fn state_key() -> Vec { b"penumbra_app_version_safeguard".to_vec() } -async fn read_app_version_safeguard(s: &S) -> anyhow::Result> { - const CTX: &'static str = "while reading app_version_safeguard"; - - let res = s.nonverifiable_get_raw(&state_key()).await.context(CTX)?; - match res { - None => Ok(None), - Some(x) => { - let bytes: [u8; 8] = x - .try_into() - .map_err(|bad: Vec| { - anyhow!("expected bytes to have length 8, found: {}", bad.len()) - }) - .context(CTX)?; - Ok(Some(u64::from_le_bytes(bytes))) - } - } +async fn read_app_version_safeguard(s: &S) -> anyhow::Result> { + let out = s + .nonverifiable_get_proto(&state_key()) + .await + .context("while reading app version safeguard")?; + Ok(out) } // Neither async nor a result are needed, but only right now, so I'm putting these here // to reserve the right to change them later. -async fn write_app_version_safeguard(s: &mut S, x: u64) -> anyhow::Result<()> { - let bytes = u64::to_le_bytes(x).to_vec(); - s.nonverifiable_put_raw(state_key(), bytes); +async fn write_app_version_safeguard(s: &mut S, x: u64) -> anyhow::Result<()> { + s.nonverifiable_put_proto(state_key(), x); Ok(()) } @@ -129,7 +119,7 @@ pub async fn assert_latest_app_version(s: Storage) -> anyhow::Result<()> { /// /// This is the only way to change the app version, and should be called during a migration /// with breaking consensus logic. -pub async fn migrate_app_version(s: &mut S, to: u64) -> anyhow::Result<()> { +pub async fn migrate_app_version(s: &mut S, to: u64) -> anyhow::Result<()> { anyhow::ensure!(to > 1, "you can't migrate to the first penumbra version!"); let found = read_app_version_safeguard(s).await?; check_version(CheckContext::Migration, to - 1, found)?; From 2ebd1aecd8d53ac36e61fad8f5808191cefb021d Mon Sep 17 00:00:00 2001 From: Lucas Meier Date: Wed, 13 Nov 2024 13:14:08 -0700 Subject: [PATCH 05/10] Use state key module for app version safeguard Co-authored-by: @erwanor --- crates/core/app/src/app/state_key.rs | 6 ++++++ crates/core/app/src/app_version/component.rs | 13 +++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/crates/core/app/src/app/state_key.rs b/crates/core/app/src/app/state_key.rs index cddac82b45..e83b139700 100644 --- a/crates/core/app/src/app/state_key.rs +++ b/crates/core/app/src/app/state_key.rs @@ -1,3 +1,9 @@ +pub mod app_version { + pub fn safeguard() -> &'static str { + "application/version/safeguard" + } +} + pub mod genesis { pub fn app_state() -> &'static str { "application/genesis/app_state" diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index 9aa5dd63b6..bad3c50fca 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -71,13 +71,9 @@ fn check_version(ctx: CheckContext, expected: u64, found: Option) -> anyhow } } -fn state_key() -> Vec { - b"penumbra_app_version_safeguard".to_vec() -} - async fn read_app_version_safeguard(s: &S) -> anyhow::Result> { let out = s - .nonverifiable_get_proto(&state_key()) + .nonverifiable_get_proto(crate::app::state_key::app_version::safeguard().as_bytes()) .await .context("while reading app version safeguard")?; Ok(out) @@ -86,7 +82,12 @@ async fn read_app_version_safeguard(s: &S) -> anyhow::Result< // Neither async nor a result are needed, but only right now, so I'm putting these here // to reserve the right to change them later. async fn write_app_version_safeguard(s: &mut S, x: u64) -> anyhow::Result<()> { - s.nonverifiable_put_proto(state_key(), x); + s.nonverifiable_put_proto( + crate::app::state_key::app_version::safeguard() + .as_bytes() + .to_vec(), + x, + ); Ok(()) } From 5473b2defc9fe7b8d8e17a95490db65055c556c5 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Wed, 13 Nov 2024 14:54:51 -0800 Subject: [PATCH 06/10] fix: minor comment cleanup --- crates/bin/pd/src/migrate/mainnet1.rs | 2 +- crates/core/app/src/app_version/component.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bin/pd/src/migrate/mainnet1.rs b/crates/bin/pd/src/migrate/mainnet1.rs index 1a51e78aeb..2123535793 100644 --- a/crates/bin/pd/src/migrate/mainnet1.rs +++ b/crates/bin/pd/src/migrate/mainnet1.rs @@ -113,7 +113,7 @@ pub async fn migrate( let start_time = std::time::SystemTime::now(); // Note, when this bit of code was added, the upgrade happened months ago, - // and the verison safeguard mechanism was not in place. However, + // and the version safeguard mechanism was not in place. However, // adding this will prevent someone running version 0.80.X with the // safeguard patch from accidentally running the migraton again, since they // will already have version 8 written into the state. But, if someone is syncing diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index bad3c50fca..d017396303 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -95,7 +95,7 @@ async fn write_app_version_safeguard(s: &mut S, x: u64) -> a /// /// You should call this before starting a node. /// -/// This will succeed if no app version is saved, or if the app version saved matches +/// This will succeed if no app version was found in local storage, or if the app version saved matches /// exactly. /// /// This will also result in the current app version being stored, so that future @@ -118,7 +118,7 @@ pub async fn assert_latest_app_version(s: Storage) -> anyhow::Result<()> { /// /// This will check that the app version is currently the previous version, if set at all. /// -/// This is the only way to change the app version, and should be called during a migration +/// This is the recommended way to change the app version, and should be called during a migration /// with breaking consensus logic. pub async fn migrate_app_version(s: &mut S, to: u64) -> anyhow::Result<()> { anyhow::ensure!(to > 1, "you can't migrate to the first penumbra version!"); From e4901078a36982d2056002d2a16d8bf2fa989326 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Wed, 13 Nov 2024 16:29:06 -0800 Subject: [PATCH 07/10] fix(pd): use correct versions in err msg --- crates/core/app/src/app_version/component.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index d017396303..d4429f4ab4 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -34,7 +34,7 @@ fn check_version(ctx: CheckContext, expected: u64, found: Option) -> anyhow match ctx { CheckContext::Running => { let expected_name = version_to_software_version(expected); - let found_name = version_to_software_version(expected); + let found_name = version_to_software_version(found); let mut error = String::new(); error.push_str("app version mismatch:\n"); write!( From 0f09fa445e5bfbb0dd2cd2951164e83b922e7479 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Thu, 14 Nov 2024 09:16:17 -0800 Subject: [PATCH 08/10] fix(pd): use correct versions in err msg, contd --- crates/core/app/src/app_version/component.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index d4429f4ab4..a525693f61 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -52,7 +52,7 @@ fn check_version(ctx: CheckContext, expected: u64, found: Option) -> anyhow } CheckContext::Migration => { let expected_name = version_to_software_version(expected); - let found_name = version_to_software_version(expected); + let found_name = version_to_software_version(found); let mut error = String::new(); error.push_str("app version mismatch:\n"); write!( From 10e35645be749e969c57c1e4e119f3265bbfcf27 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Thu, 14 Nov 2024 09:35:34 -0800 Subject: [PATCH 09/10] test(app): app_version needs a software version The `app_version_safeguard` logic extends the APP_VERSION logic with helper functions that translate an APP_VERSION to a software version. Previously, protocol versions increments only required changing the APP_VERSION const (as well as writing a migration), but now developers must also update the version match, otherwise tooling will report an "unknown" version by default. Adding a simple test to confirm the lookup returns a known value. --- crates/core/app/src/app_version/component.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index a525693f61..f1952078e8 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -127,3 +127,20 @@ pub async fn migrate_app_version(s: &mut S, to: u64) -> anyh write_app_version_safeguard(s, to).await?; Ok(()) } + +#[cfg(test)] +mod test { + use super::*; + #[test] + /// Confirm there's a matching branch on the APP_VERSION to crate version lookup. + /// It's possible to overlook that update when bumping the APP_VERSION, so this test + /// ensures that if the APP_VERSION was changed, so was the match arm. + fn ensure_app_version_is_current_in_checks() -> anyhow::Result<()> { + let result = version_to_software_version(APP_VERSION); + assert!( + result != "unknown", + "APP_VERSION lacks a corresponding software version" + ); + Ok(()) + } +} From 47230fd1e67e2175bc916169bd28e66569695313 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Fri, 15 Nov 2024 10:26:32 -0800 Subject: [PATCH 10/10] migrate: informative msg on re-attempted migration If an operator runs `pd migrate` and then runs it again, the error message should accurately describe the situation that the local state is already migrated, and state that pd is refusing to proceed as instructed. Notably this failure occurs even if the `--force` flag is provided to `pd migrate`, so I've updated the docstring on that flag accordingly. --- crates/bin/pd/src/cli.rs | 1 + crates/core/app/src/app_version/component.rs | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/crates/bin/pd/src/cli.rs b/crates/bin/pd/src/cli.rs index 6583a5fccd..80e1b00500 100644 --- a/crates/bin/pd/src/cli.rs +++ b/crates/bin/pd/src/cli.rs @@ -133,6 +133,7 @@ pub enum RootCommand { #[clap(long, display_order = 200)] comet_home: Option, /// If set, force a migration to occur even if the chain is not halted. + /// Will not override a detected mismatch in state versions. #[clap(long, display_order = 1000)] force: bool, /// If set, edit local state to permit the node to start, despite diff --git a/crates/core/app/src/app_version/component.rs b/crates/core/app/src/app_version/component.rs index f1952078e8..2ca749dd03 100644 --- a/crates/core/app/src/app_version/component.rs +++ b/crates/core/app/src/app_version/component.rs @@ -54,6 +54,14 @@ fn check_version(ctx: CheckContext, expected: u64, found: Option) -> anyhow let expected_name = version_to_software_version(expected); let found_name = version_to_software_version(found); let mut error = String::new(); + if found == APP_VERSION { + write!( + &mut error, + "state already migrated to version {}", + APP_VERSION + )?; + anyhow::bail!(error); + } error.push_str("app version mismatch:\n"); write!( &mut error,