From 93a145b0cefdf9a663ef8e5bab0150cebc845863 Mon Sep 17 00:00:00 2001 From: Daniil Polyakov Date: Wed, 11 Oct 2023 04:45:16 +0300 Subject: [PATCH] [fix]: Reduce configuration stack size Signed-off-by: Daniil Polyakov --- Cargo.lock | 14 ++++++ Cargo.toml | 1 + cli/src/lib.rs | 2 +- cli/src/samples.rs | 12 +++--- client/tests/integration/config.rs | 2 +- config/base/derive/src/proxy.rs | 47 +++++++++++++++++---- config/base/derive/src/utils.rs | 16 +++++++ config/base/derive/src/view.rs | 20 ++++++++- config/base/src/lib.rs | 48 +++++++++++++++++++++ config/src/iroha.rs | 68 ++++++++++++++++++------------ core/test_network/src/lib.rs | 18 ++++---- docs/source/references/config.md | 16 +++---- 12 files changed, 202 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 817e6d5d1db..f6c8e64d09e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3036,6 +3036,7 @@ dependencies = [ "proptest", "serde", "serde_json", + "stacker", "strum", "thiserror", "tracing", @@ -5442,6 +5443,19 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a8f112729512f8e442d81f95a8a7ddf2b7c6b8a1a6f509a95864142b30cab2d3" +[[package]] +name = "stacker" +version = "0.1.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c886bd4480155fd3ef527d45e9ac8dd7118a898a46530b7b94c3e21866259fce" +dependencies = [ + "cc", + "cfg-if", + "libc", + "psm", + "winapi", +] + [[package]] name = "static_assertions" version = "1.1.0" diff --git a/Cargo.toml b/Cargo.toml index cc3ebecb5fb..b5de412c2c3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -103,6 +103,7 @@ duct = "0.13.6" criterion = "0.3.6" proptest = "1.0.0" expect-test = "1.4.1" +stacker = "0.1.15" eyre = "0.6.8" color-eyre = "0.6.2" diff --git a/cli/src/lib.rs b/cli/src/lib.rs index e2bdb206436..794b55972f9 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -262,7 +262,7 @@ impl Iroha { |error| { iroha_logger::warn!(%error, "Failed to load wsv from snapshot, creating empty wsv"); WorldStateView::from_configuration( - config.wsv, + *config.wsv, world, Arc::clone(&kura), live_query_store_handle.clone(), diff --git a/cli/src/samples.rs b/cli/src/samples.rs index 1b46b2c0556..008a5e2ed26 100644 --- a/cli/src/samples.rs +++ b/cli/src/samples.rs @@ -61,16 +61,16 @@ pub fn get_config_proxy(peers: UniqueVec, key_pair: Option) -> ConfigurationProxy { public_key: Some(public_key.clone()), private_key: Some(private_key.clone()), - sumeragi: Some(iroha_config::sumeragi::ConfigurationProxy { + sumeragi: Some(Box::new(iroha_config::sumeragi::ConfigurationProxy { max_transactions_in_block: Some(2), trusted_peers: Some(TrustedPeers { peers }), ..iroha_config::sumeragi::ConfigurationProxy::default() - }), - torii: Some(iroha_config::torii::ConfigurationProxy { + })), + torii: Some(Box::new(iroha_config::torii::ConfigurationProxy { p2p_addr: Some(DEFAULT_TORII_P2P_ADDR.clone()), api_url: Some(DEFAULT_API_ADDR.clone()), ..iroha_config::torii::ConfigurationProxy::default() - }), + })), block_sync: Some(iroha_config::block_sync::ConfigurationProxy { block_batch_size: Some(1), gossip_period_ms: Some(500), @@ -79,10 +79,10 @@ pub fn get_config_proxy(peers: UniqueVec, key_pair: Option) -> queue: Some(iroha_config::queue::ConfigurationProxy { ..iroha_config::queue::ConfigurationProxy::default() }), - genesis: Some(iroha_config::genesis::ConfigurationProxy { + genesis: Some(Box::new(iroha_config::genesis::ConfigurationProxy { account_private_key: Some(Some(private_key)), account_public_key: Some(public_key), - }), + })), ..ConfigurationProxy::default() } } diff --git a/client/tests/integration/config.rs b/client/tests/integration/config.rs index 381f8173c3c..219d966cc73 100644 --- a/client/tests/integration/config.rs +++ b/client/tests/integration/config.rs @@ -24,6 +24,6 @@ fn get_config() { assert_eq!(cfg_proxy.network.unwrap().build().unwrap(), test.network); assert_eq!( cfg_proxy.telemetry.unwrap().build().unwrap(), - test.telemetry + *test.telemetry ); } diff --git a/config/base/derive/src/proxy.rs b/config/base/derive/src/proxy.rs index 8eb42326f1a..f387b66dfc8 100644 --- a/config/base/derive/src/proxy.rs +++ b/config/base/derive/src/proxy.rs @@ -122,8 +122,11 @@ pub fn impl_load_from_env(ast: &StructWithFields) -> TokenStream { .transpose()?; }; if field.has_inner { + let maybe_map_box = gen_maybe_map_box(inner_ty); set_field.extend(quote! { - let inner_proxy = <#inner_ty as #env_trait>::from_env(#env_fetcher_ident)?; + let inner_proxy = <#inner_ty as #env_trait>::from_env(#env_fetcher_ident) + #maybe_map_box + ?; let #ident = if let Some(old_inner) = #ident { Some(<#inner_ty as ::iroha_config_base::proxy::Override>::override_with(old_inner, inner_proxy)) } else { @@ -195,12 +198,7 @@ fn gen_proxy_struct(mut ast: StructWithFields) -> StructWithFields { // For fields of `Configuration` that have an inner config, the corresponding // proxy field should have a `..Proxy` type there as well if field.has_inner { - #[allow(clippy::expect_used)] - if let Type::Path(path) = &mut field.ty { - let old_ident = &path.path.segments.last().expect("Can't be empty").ident; - let new_ident = format_ident!("{}Proxy", old_ident); - path.path.segments.last_mut().expect("Can't be empty").ident = new_ident; - } + proxify_field_type(&mut field.ty); } let ty = &field.ty; field.ty = parse_quote! { @@ -229,6 +227,22 @@ fn gen_proxy_struct(mut ast: StructWithFields) -> StructWithFields { ast } +#[allow(clippy::expect_used)] +pub fn proxify_field_type(field_ty: &mut syn::Type) { + if let Type::Path(path) = field_ty { + let last_segment = path.path.segments.last_mut().expect("Can't be empty"); + if last_segment.ident == "Box" { + let box_generic = utils::extract_box_generic(last_segment); + // Recursion + proxify_field_type(box_generic) + } else { + // TODO: Wouldn't it be better to get it as an associated type? + let new_ident = format_ident!("{}Proxy", last_segment.ident); + last_segment.ident = new_ident; + } + } +} + pub fn impl_build(ast: &StructWithFields) -> TokenStream { let checked_fields = gen_none_fields_check(ast); let proxy_name = &ast.ident; @@ -258,12 +272,17 @@ fn gen_none_fields_check(ast: &StructWithFields) -> proc_macro2::TokenStream { if field.has_inner { let inner_ty = get_inner_type("Option", &field.ty); let builder_trait = quote! { ::iroha_config_base::proxy::Builder }; + + let maybe_map_box = gen_maybe_map_box(inner_ty); + quote! { #ident: <#inner_ty as #builder_trait>::build( self.#ident.ok_or( #missing_field{field: stringify!(#ident), message: ""} )? - )? + ) + #maybe_map_box + ? } } else { quote! { @@ -278,6 +297,18 @@ fn gen_none_fields_check(ast: &StructWithFields) -> proc_macro2::TokenStream { } } +fn gen_maybe_map_box(inner_ty: &syn::Type) -> proc_macro2::TokenStream { + if let Type::Path(path) = &inner_ty { + let last_segment = path.path.segments.last().expect("Can't be empty"); + if last_segment.ident == "Box" { + return quote! { + .map(Box::new) + }; + } + } + quote! {} +} + /// Helper function to be used as an empty fallback for [`impl LoadFromEnv`] or [`impl LoadFromDisk`]. /// Only meant for proxy types usage. fn gen_none_fields_proxy(ast: &StructWithFields) -> proc_macro2::TokenStream { diff --git a/config/base/derive/src/utils.rs b/config/base/derive/src/utils.rs index 239fd0c7c5c..5a9667d5f18 100644 --- a/config/base/derive/src/utils.rs +++ b/config/base/derive/src/utils.rs @@ -350,3 +350,19 @@ pub fn get_parent_ty(ast: &StructWithFields) -> Type { .map(|builder| builder.parent) .expect("Should not be called on structs with no `#[builder(..)]` attribute") } + +pub fn extract_box_generic(box_seg: &mut syn::PathSegment) -> &mut syn::Type { + let syn::PathArguments::AngleBracketed(generics) = &mut box_seg.arguments else { + panic!("`Box` should have explicit generic"); + }; + + assert!( + generics.args.len() == 1, + "`Box` should have exactly one generic argument" + ); + let syn::GenericArgument::Type(generic_type) = generics.args.first_mut().expect("Can't be empty") else { + panic!("`Box` should have type as a generic argument") + }; + + generic_type +} diff --git a/config/base/derive/src/view.rs b/config/base/derive/src/view.rs index dd80f80b2f9..217ea4380b2 100644 --- a/config/base/derive/src/view.rs +++ b/config/base/derive/src/view.rs @@ -87,6 +87,24 @@ mod gen { }) .collect::>(); + let field_froms: Vec<_> = fields + .iter() + .map(|field| { + let field_ident = &field.ident; + if let syn::Type::Path(syn::TypePath { path, .. }) = &field.ty { + let last_segment = path.segments.last().expect("Not empty"); + if last_segment.ident == "Box" { + return quote! { + #field_ident: Box::new(core::convert::From::<_>::from(*#field_ident)), + }; + } + } + quote! { + #field_ident: core::convert::From::<_>::from(#field_ident), + } + }) + .collect(); + quote! { impl #impl_generics core::convert::From<#original_ident> for #view_ident #ty_generics #where_clause { fn from(config: #original_ident) -> Self { @@ -100,7 +118,7 @@ mod gen { Self { #( #(#field_cfg_attrs)* - #field_idents: core::convert::From::<_>::from(#field_idents), + #field_froms )* } } diff --git a/config/base/src/lib.rs b/config/base/src/lib.rs index 317e2ad7f11..bee2b692efc 100644 --- a/config/base/src/lib.rs +++ b/config/base/src/lib.rs @@ -501,6 +501,32 @@ pub mod proxy { ) -> Result, Self::Error>; } + impl Documented for Box { + type Error = T::Error; + + fn get_docs() -> Value { + T::get_docs() + } + + fn get_inner_docs() -> String { + T::get_inner_docs() + } + + fn get_recursive<'tl, U>(&self, inner_field: U) -> Result + where + U: AsRef<[&'tl str]> + Send + 'tl, + { + T::get_recursive(self, inner_field) + } + + #[allow(single_use_lifetimes)] // False-positive + fn get_doc_recursive<'tl>( + field: impl AsRef<[&'tl str]>, + ) -> Result, Self::Error> { + T::get_doc_recursive(field) + } + } + /// Trait for combining two configuration instances pub trait Override: Serialize + DeserializeOwned + Sized { /// If any of the fields in `other` are filled, they @@ -509,6 +535,12 @@ pub mod proxy { fn override_with(self, other: Self) -> Self; } + impl Override for Box { + fn override_with(self, other: Self) -> Self { + Box::new(T::override_with(*self, *other)) + } + } + /// Trait for configuration loading and deserialization from /// the environment pub trait LoadFromEnv: Sized { @@ -540,6 +572,14 @@ pub mod proxy { } } + impl LoadFromEnv for Box { + type ReturnValue = T::ReturnValue; + + fn from_env(fetcher: &F) -> Self::ReturnValue { + T::from_env(fetcher) + } + } + /// Abstraction over the actual implementation of how env variables are gotten /// from the environment. Necessary for mocking in tests. pub trait FetchEnv { @@ -576,6 +616,14 @@ pub mod proxy { fn build(self) -> Self::ReturnValue; } + impl Builder for Box { + type ReturnValue = T::ReturnValue; + + fn build(self) -> Self::ReturnValue { + T::build(*self) + } + } + /// Deserialization helper for proxy fields that wrap an `Option` /// /// # Errors diff --git a/config/src/iroha.rs b/config/src/iroha.rs index ed022853431..5eee42b2b55 100644 --- a/config/src/iroha.rs +++ b/config/src/iroha.rs @@ -26,14 +26,14 @@ view! { pub disable_panic_terminal_colors: bool, /// `Kura` configuration #[config(inner)] - pub kura: kura::Configuration, + pub kura: Box, /// `Sumeragi` configuration #[config(inner)] - #[view(into = sumeragi::ConfigurationView)] - pub sumeragi: sumeragi::Configuration, + #[view(into = Box)] + pub sumeragi: Box, /// `Torii` configuration #[config(inner)] - pub torii: torii::Configuration, + pub torii: Box, /// `BlockSynchronizer` configuration #[config(inner)] pub block_sync: block_sync::Configuration, @@ -42,23 +42,23 @@ view! { pub queue: queue::Configuration, /// `Logger` configuration #[config(inner)] - pub logger: logger::Configuration, + pub logger: Box, /// `GenesisBlock` configuration #[config(inner)] - #[view(into = genesis::ConfigurationView)] - pub genesis: genesis::Configuration, + #[view(into = Box)] + pub genesis: Box, /// `WorldStateView` configuration #[config(inner)] - pub wsv: wsv::Configuration, + pub wsv: Box, /// Network configuration #[config(inner)] pub network: network::Configuration, /// Telemetry configuration #[config(inner)] - pub telemetry: telemetry::Configuration, + pub telemetry: Box, /// SnapshotMaker configuration #[config(inner)] - pub snapshot: snapshot::Configuration, + pub snapshot: Box, /// LiveQueryStore configuration #[config(inner)] pub live_query_store: live_query_store::Configuration, @@ -71,17 +71,17 @@ impl Default for ConfigurationProxy { public_key: None, private_key: None, disable_panic_terminal_colors: Some(bool::default()), - kura: Some(kura::ConfigurationProxy::default()), - sumeragi: Some(sumeragi::ConfigurationProxy::default()), - torii: Some(torii::ConfigurationProxy::default()), + kura: Some(Box::default()), + sumeragi: Some(Box::default()), + torii: Some(Box::default()), block_sync: Some(block_sync::ConfigurationProxy::default()), queue: Some(queue::ConfigurationProxy::default()), - logger: Some(logger::ConfigurationProxy::default()), - genesis: Some(genesis::ConfigurationProxy::default()), - wsv: Some(wsv::ConfigurationProxy::default()), + logger: Some(Box::default()), + genesis: Some(Box::default()), + wsv: Some(Box::default()), network: Some(network::ConfigurationProxy::default()), - telemetry: Some(telemetry::ConfigurationProxy::default()), - snapshot: Some(snapshot::ConfigurationProxy::default()), + telemetry: Some(Box::default()), + snapshot: Some(Box::default()), live_query_store: Some(live_query_store::ConfigurationProxy::default()), } } @@ -214,17 +214,17 @@ mod tests { fn arb_proxy()( (public_key, private_key) in arb_keys(), disable_panic_terminal_colors in prop::option::of(Just(true)), - kura in prop::option::of(kura::tests::arb_proxy()), - sumeragi in prop::option::of(sumeragi::tests::arb_proxy()), - torii in prop::option::of(torii::tests::arb_proxy()), + kura in prop::option::of(kura::tests::arb_proxy().prop_map(Box::new)), + sumeragi in (prop::option::of(sumeragi::tests::arb_proxy().prop_map(Box::new))), + torii in (prop::option::of(torii::tests::arb_proxy().prop_map(Box::new))), block_sync in prop::option::of(block_sync::tests::arb_proxy()), queue in prop::option::of(queue::tests::arb_proxy()), - logger in prop::option::of(logger::tests::arb_proxy()), - genesis in prop::option::of(genesis::tests::arb_proxy()), - wsv in prop::option::of(wsv::tests::arb_proxy()), + logger in prop::option::of(logger::tests::arb_proxy().prop_map(Box::new)), + genesis in prop::option::of(genesis::tests::arb_proxy().prop_map(Box::new)), + wsv in prop::option::of(wsv::tests::arb_proxy().prop_map(Box::new)), network in prop::option::of(network::tests::arb_proxy()), - telemetry in prop::option::of(telemetry::tests::arb_proxy()), - snapshot in prop::option::of(snapshot::tests::arb_proxy()), + telemetry in prop::option::of(telemetry::tests::arb_proxy().prop_map(Box::new)), + snapshot in prop::option::of(snapshot::tests::arb_proxy().prop_map(Box::new)), live_query_store in prop::option::of(live_query_store::tests::arb_proxy()), ) -> ConfigurationProxy { ConfigurationProxy { public_key, private_key, disable_panic_terminal_colors, kura, sumeragi, torii, block_sync, queue, @@ -232,9 +232,14 @@ mod tests { } } + #[test] + fn size_check() { + dbg!(std::mem::size_of::()); + dbg!(std::mem::size_of::()); + } + proptest! { - #[test] - fn iroha_proxy_build_fails_on_none(proxy in arb_proxy()) { + fn __iroha_proxy_build_fails_on_none(proxy in arb_proxy()) { let cfg = proxy.build(); let example_cfg = ConfigurationProxy::from_path(CONFIGURATION_PATH).build().expect("Failed to build example Iroha config"); if cfg.is_ok() { @@ -243,6 +248,13 @@ mod tests { } } + #[test] + fn iroha_proxy_build_fails_on_none() { + // Using `stacker` because test generated by `proptest!` takes too much stack space. + // Allocating 3MB. + stacker::grow(3 * 1024 * 1024, __iroha_proxy_build_fails_on_none) + } + #[test] fn parse_example_json() { let cfg_proxy = ConfigurationProxy::from_path(CONFIGURATION_PATH); diff --git a/core/test_network/src/lib.rs b/core/test_network/src/lib.rs index b0e58df14d3..b6085915f0c 100644 --- a/core/test_network/src/lib.rs +++ b/core/test_network/src/lib.rs @@ -389,19 +389,19 @@ impl Peer { /// Returns per peer config with all addresses, keys, and id set up. fn get_config(&self, configuration: Configuration) -> Configuration { Configuration { - sumeragi: SumeragiConfiguration { + sumeragi: Box::new(SumeragiConfiguration { key_pair: self.key_pair.clone(), peer_id: self.id.clone(), - ..configuration.sumeragi - }, - torii: ToriiConfiguration { + ..*configuration.sumeragi + }), + torii: Box::new(ToriiConfiguration { p2p_addr: self.p2p_address.clone(), api_url: self.api_address.clone(), - ..configuration.torii - }, - logger: LoggerConfiguration { - ..configuration.logger - }, + ..*configuration.torii + }), + logger: Box::new(LoggerConfiguration { + ..*configuration.logger + }), public_key: self.key_pair.public_key().clone(), private_key: self.key_pair.private_key().clone(), disable_panic_terminal_colors: true, diff --git a/docs/source/references/config.md b/docs/source/references/config.md index d8153062d07..a3f4468ca4b 100644 --- a/docs/source/references/config.md +++ b/docs/source/references/config.md @@ -189,7 +189,7 @@ false `GenesisBlock` configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_GENESIS` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_GENESIS` ```json { @@ -222,7 +222,7 @@ null `Kura` configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_KURA` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_KURA` ```json { @@ -310,7 +310,7 @@ Has type `Option`[^1]. Can be configured via environment variable `L `Logger` configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_LOGGER` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_LOGGER` ```json { @@ -473,7 +473,7 @@ Has type `Option`[^1]. Can be configured via environment variable `QUEUE_TR SnapshotMaker configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_SNAPSHOT` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_SNAPSHOT` ```json { @@ -517,7 +517,7 @@ Has type `Option`[^1]. Can be configured via environment variable `SNAPS `Sumeragi` configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_SUMERAGI` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_SUMERAGI` ```json { @@ -627,7 +627,7 @@ null Telemetry configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_TELEMETRY` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_TELEMETRY` ```json { @@ -693,7 +693,7 @@ null `Torii` configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_TORII` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_TORII` ```json { @@ -759,7 +759,7 @@ null `WorldStateView` configuration -Has type `Option`[^1]. Can be configured via environment variable `IROHA_WSV` +Has type `Option>`[^1]. Can be configured via environment variable `IROHA_WSV` ```json {