From 140eefa8dcd0096b2b4d5bb26626a80e1d7e38a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksandar=20Terenti=C4=87?= Date: Fri, 4 Aug 2023 13:40:33 +0200 Subject: [PATCH 1/3] Remove spec_version check. --- src/main.rs | 3 +-- src/rpc.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/main.rs b/src/main.rs index 5f34b54e4..eb3c1494b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -151,9 +151,8 @@ async fn run(error_sender: Sender) -> Result<()> { let db = init_db(&cfg.avail_path).context("Cannot initialize database")?; - let network_version = rpc::Version { + let network_version = rpc::ExpectedVersion { version: "1.6".to_string(), - spec_version: 11, spec_name: "data-avail".to_string(), }; diff --git a/src/rpc.rs b/src/rpc.rs index f4c7540ba..3272a76f4 100644 --- a/src/rpc.rs +++ b/src/rpc.rs @@ -196,20 +196,35 @@ fn shuffle_full_nodes(full_nodes: &[String], last_full_node: Option) -> candidates } -pub struct Version { +pub struct ExpectedVersion { pub version: String, - pub spec_version: u32, pub spec_name: String, } -impl Version { +impl ExpectedVersion { + /// Checks if expected version matches network version. + /// Since the light client uses subset of the node APIs, `matches` checks only prefix of a node version. + /// This means that if expected version is `1.6`, versions `1.6.x` of the node will match. + /// Specification name is checked for exact match. + /// Since runtime `spec_version` can be changed with runtime upgrade, `spec_version` is removed. + /// NOTE: Runtime compatiblity check is currently not implemented. pub fn matches(&self, other: &Version) -> bool { - (self.version.starts_with(&other.version) || other.version.starts_with(&self.version)) - && self.spec_name == other.spec_name - && self.spec_version == other.spec_version + other.version.starts_with(&self.version) && self.spec_name == other.spec_name + } +} + +impl Display for ExpectedVersion { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "v{}/{}", self.version, self.spec_name) } } +pub struct Version { + pub version: String, + pub spec_version: u32, + pub spec_name: String, +} + impl Display for Version { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( @@ -225,7 +240,7 @@ impl Display for Version { pub async fn connect_to_the_full_node( full_nodes: &[String], last_full_node: Option, - expected_version: Version, + expected_version: ExpectedVersion, ) -> Result<(avail::Client, String)> { for full_node_ws in shuffle_full_nodes(full_nodes, last_full_node).iter() { let log_warn = |error| { @@ -282,6 +297,8 @@ pub fn cell_count_for_confidence(confidence: f64) -> u32 { #[cfg(test)] mod tests { + use super::{ExpectedVersion, Version}; + use crate::rpc::shuffle_full_nodes; use proptest::{ prelude::any_with, prop_assert, prop_assert_eq, proptest, @@ -289,8 +306,7 @@ mod tests { strategy::{BoxedStrategy, Strategy}, }; use rand::{seq::SliceRandom, thread_rng}; - - use crate::rpc::shuffle_full_nodes; + use test_case::test_case; fn full_nodes() -> BoxedStrategy<(Vec, Option)> { any_with::>(size_range(10).lift()) @@ -301,6 +317,29 @@ mod tests { .boxed() } + #[test_case("1.6" , "data_avail" , "1.6.1" , "data_avail" , true; "1.6/data_avail matches 1.6.1/data_avail/0")] + #[test_case("1.2" , "data_avail" , "1.2.9" , "data_avail" , true; "1.2/data_avail matches 1.2.9/data_avail/0")] + #[test_case("1.6" , "data_avail" , "1.6.1" , "no_data_avail" , false; "1.6/data_avail matches 1.6.1/no_data_avail/0")] + #[test_case("1.6" , "data_avail" , "1.7.0" , "data_avail" , false; "1.6/data_avail doesn't match 1.7.0/data_avail/0")] + fn test_version_match( + expected_version: &str, + expected_spec_name: &str, + version: &str, + spec_name: &str, + matches: bool, + ) { + let expected = ExpectedVersion { + version: expected_version.to_string(), + spec_name: expected_spec_name.to_string(), + }; + let version = Version { + version: version.to_string(), + spec_name: spec_name.to_string(), + spec_version: 0, + }; + assert_eq!(expected.matches(&version), matches); + } + proptest! { #[test] fn shuffle_without_last((full_nodes, _) in full_nodes()) { From f31c4f871f68262aabc32bb80fd6f7b250f94e99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksandar=20Terenti=C4=87?= Date: Fri, 4 Aug 2023 14:23:22 +0200 Subject: [PATCH 2/3] Remove Version abstraction. --- src/rpc.rs | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/src/rpc.rs b/src/rpc.rs index 3272a76f4..879d34f88 100644 --- a/src/rpc.rs +++ b/src/rpc.rs @@ -208,8 +208,8 @@ impl ExpectedVersion { /// Specification name is checked for exact match. /// Since runtime `spec_version` can be changed with runtime upgrade, `spec_version` is removed. /// NOTE: Runtime compatiblity check is currently not implemented. - pub fn matches(&self, other: &Version) -> bool { - other.version.starts_with(&self.version) && self.spec_name == other.spec_name + pub fn matches(&self, node_version: String, spec_name: String) -> bool { + node_version.starts_with(&self.version) && self.spec_name == spec_name } } @@ -219,22 +219,6 @@ impl Display for ExpectedVersion { } } -pub struct Version { - pub version: String, - pub spec_version: u32, - pub spec_name: String, -} - -impl Display for Version { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!( - f, - "v{}/{}/{}", - self.version, self.spec_name, self.spec_version - ) - } -} - /// Connects to the random full node from the list, /// trying to connect to the last connected full node as least priority. pub async fn connect_to_the_full_node( @@ -252,13 +236,12 @@ pub async fn connect_to_the_full_node( let Ok(system_version) = get_system_version(&client).await.map_err(log_warn) else { continue; }; let Ok(runtime_version) = get_runtime_version(&client).await.map_err(log_warn) else { continue; }; - let version = Version { - version: system_version, - spec_name: runtime_version.spec_name, - spec_version: runtime_version.spec_version, - }; + let version = format!( + "v{}/{}/{}", + system_version, runtime_version.spec_name, runtime_version.spec_version, + ); - if !expected_version.matches(&version) { + if !expected_version.matches(system_version, runtime_version.spec_name) { log_warn(anyhow!("expected {expected_version}, found {version}")); continue; } @@ -297,8 +280,7 @@ pub fn cell_count_for_confidence(confidence: f64) -> u32 { #[cfg(test)] mod tests { - use super::{ExpectedVersion, Version}; - use crate::rpc::shuffle_full_nodes; + use crate::rpc::{shuffle_full_nodes, ExpectedVersion}; use proptest::{ prelude::any_with, prop_assert, prop_assert_eq, proptest, @@ -332,12 +314,11 @@ mod tests { version: expected_version.to_string(), spec_name: expected_spec_name.to_string(), }; - let version = Version { - version: version.to_string(), - spec_name: spec_name.to_string(), - spec_version: 0, - }; - assert_eq!(expected.matches(&version), matches); + + assert_eq!( + expected.matches(version.to_string(), spec_name.to_string()), + matches + ); } proptest! { From 3f06618606392c0a39deb17b64b8594976ab7026 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksandar=20Terenti=C4=87?= Date: Fri, 4 Aug 2023 14:47:53 +0200 Subject: [PATCH 3/3] Move expected network version to consts. --- src/consts.rs | 8 ++++++++ src/main.rs | 18 ++++++++---------- src/rpc.rs | 16 ++++++++-------- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/consts.rs b/src/consts.rs index 72c88bd98..a4cf703b7 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -1,5 +1,7 @@ //! Column family names and other constants. +use crate::rpc::ExpectedVersion; + /// Column family for confidence factor pub const CONFIDENCE_FACTOR_CF: &str = "avail_light_confidence_factor_cf"; @@ -11,3 +13,9 @@ pub const APP_DATA_CF: &str = "avail_light_app_data_cf"; /// Column family for state pub const STATE_CF: &str = "avail_light_state_cf"; + +/// Expected network version +pub const EXPECTED_NETWORK_VERSION: ExpectedVersion = ExpectedVersion { + version: "1.6", + spec_name: "data_avail", +}; diff --git a/src/main.rs b/src/main.rs index eb3c1494b..2d2760a79 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,7 +25,7 @@ use tracing_subscriber::{ }; use crate::{ - consts::{APP_DATA_CF, BLOCK_HEADER_CF, CONFIDENCE_FACTOR_CF}, + consts::{APP_DATA_CF, BLOCK_HEADER_CF, CONFIDENCE_FACTOR_CF, EXPECTED_NETWORK_VERSION}, data::store_last_full_node_ws_in_db, types::{Mode, RuntimeConfig}, }; @@ -151,11 +151,6 @@ async fn run(error_sender: Sender) -> Result<()> { let db = init_db(&cfg.avail_path).context("Cannot initialize database")?; - let network_version = rpc::ExpectedVersion { - version: "1.6".to_string(), - spec_name: "data-avail".to_string(), - }; - // Spawn tokio task which runs one http server for handling RPC let counter = Arc::new(Mutex::new(0u32)); @@ -164,7 +159,7 @@ async fn run(error_sender: Sender) -> Result<()> { cfg: cfg.clone(), counter: counter.clone(), version: format!("v{}", clap::crate_version!().to_string()), - network_version: network_version.to_string(), + network_version: EXPECTED_NETWORK_VERSION.to_string(), }; tokio::task::spawn(server.run()); @@ -261,9 +256,12 @@ async fn run(error_sender: Sender) -> Result<()> { let last_full_node_ws = data::get_last_full_node_ws_from_db(db.clone())?; - let (rpc_client, last_full_node_ws) = - rpc::connect_to_the_full_node(&cfg.full_node_ws, last_full_node_ws, network_version) - .await?; + let (rpc_client, last_full_node_ws) = rpc::connect_to_the_full_node( + &cfg.full_node_ws, + last_full_node_ws, + EXPECTED_NETWORK_VERSION, + ) + .await?; store_last_full_node_ws_in_db(db.clone(), last_full_node_ws)?; diff --git a/src/rpc.rs b/src/rpc.rs index 879d34f88..96aadbbc0 100644 --- a/src/rpc.rs +++ b/src/rpc.rs @@ -196,12 +196,12 @@ fn shuffle_full_nodes(full_nodes: &[String], last_full_node: Option) -> candidates } -pub struct ExpectedVersion { - pub version: String, - pub spec_name: String, +pub struct ExpectedVersion<'a> { + pub version: &'a str, + pub spec_name: &'a str, } -impl ExpectedVersion { +impl ExpectedVersion<'_> { /// Checks if expected version matches network version. /// Since the light client uses subset of the node APIs, `matches` checks only prefix of a node version. /// This means that if expected version is `1.6`, versions `1.6.x` of the node will match. @@ -213,7 +213,7 @@ impl ExpectedVersion { } } -impl Display for ExpectedVersion { +impl Display for ExpectedVersion<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "v{}/{}", self.version, self.spec_name) } @@ -224,7 +224,7 @@ impl Display for ExpectedVersion { pub async fn connect_to_the_full_node( full_nodes: &[String], last_full_node: Option, - expected_version: ExpectedVersion, + expected_version: ExpectedVersion<'_>, ) -> Result<(avail::Client, String)> { for full_node_ws in shuffle_full_nodes(full_nodes, last_full_node).iter() { let log_warn = |error| { @@ -311,8 +311,8 @@ mod tests { matches: bool, ) { let expected = ExpectedVersion { - version: expected_version.to_string(), - spec_name: expected_spec_name.to_string(), + version: expected_version, + spec_name: expected_spec_name, }; assert_eq!(