[refactor] #3501: Rewrite macros for generating configuration #3512

mversic · 2023-05-22T09:16:48Z

Description

Linked issue

Benefits

Checklist

I refactored the config
I am addressing comments (25/50)
Final re-review

0x009922

I like new approach - ConfigurationBuilder is clearer than ConfigurationProxy, and macro generates default fallback, allowing to define only required fields in the config, which solves:

[suggestion] Make most of the configuration parameters optional #3501

0x009922 · 2023-05-22T23:12:51Z

config/src/sumeragi.rs

 /// Custom deserializer that ensures that `trusted_peers` only
 /// contains unique `PeerId`'s.
 ///
 /// # Errors
 /// - Peer Ids not unique,
 /// - Not a sequence (array)
-fn deserialize_unique_trusted_peers<'de, D>(deserializer: D) -> Result<HashSet<PeerId>, D::Error>
+fn deserialize_unique_trusted_peers<'de, D>(deserializer: D) -> Result<Vec<PeerId>, D::Error>


Peers traversal order determinism doesn't matter anymore?

good point. I'm not sure if the order was preserved in the previous impl. I have fixed it now

Nvm, I was confused and messed HashSet with BTreeSet. The order doesn't matter for HashSet.

However, the method is called "deserialise unique trusted peers", so it does make sense to return a set, not a list.

order of peers is relevant for consensus so it should be a vector

Okay, but what about uniqueness?

the body of the function throws an error if they are not unique

I should add validation for the setter method

the body of the function throws an error if they are not unique

It's better when validation result is expressed as a type that cannot be created without that validation (parse-not-validate).

This is not touched upon in the documentation. Semantically this is a Set. The functional programmer would say "add a transparent newtype with deduplication semantics and iter", like @0x009922 suggests.

The imperative programmer in me thinks "Just use vector, people working on consensus have to read the docs anyway". If you're feeling fancy, call it

type PeerSet = Vec<PeerId>

0x009922 · 2023-05-22T23:15:54Z

core/src/wsv.rs

@@ -162,9 +159,7 @@ impl WorldStateView {
    #[inline]
    pub fn new(world: World, kura: Arc<Kura>) -> Self {
        // Added to remain backward compatible with other code primary in tests


Maybe, make this function #[cfg(test)] then?

unfortunately no because it's used in integration benches which are compiled into a different crate. cfg(test) only works for unit tests

One workaround which I don't think we ever considered was separating tests by debug status. We're not currently running tests in release, and if we did, we shouldn't be using private API anyway.

One workaround which I don't think we ever considered was separating tests by debug status. We're not currently running tests in release, and if we did, we shouldn't be using private API anyway.

this might be a good idea. We could rely on debug_assertions to separate tests that test API that is kinda private

config/derive/src/proxy.rs

0x009922

You say that it doesn't mean that the field should exist in the final configuration. Instead, it means that the user should provide it anyway. In other words, such a field doesn't have a default value, and it can't be None in the final configuration.

So, now it works this way (as I understand):

#[derive(Configuration)]
struct Config {
  // has default value
  foo: u32,
  // this fields doesn't have a default value
  #[config(required)]
  bar: u32,

  // some field you want to be completely optional, even without a default value
  // not possible now?
  // baz: Option<u32>
}

impl Config {
  const DEFAULT_FOO: u32 = 512;
}

Expanded:

struct ConfigBuilder {
  foo: Option<u32>,
  bar: Option<u32>
}

struct ConfigBuilder {
  foo: u32,
  bar: u32
}

Compare with this:

#[derive(Configuration)]
struct Config {
  // this field is not required for the user to provide, because it has a default fallback
  #[config(default_value = "512")]
  foo: u32,

  // this field doesn't have a default fallback, so user should provide it in some way
  bar: u32,

  // neither a default-fallback-able field and a required field
  // not sure it is needed now though
  #[config(optional)]
  baz: u32
}

Expanded:

struct ConfigBuilder {
  foo: Option<u32>,
  bar: Option<u32>,
  baz: Option<u32>
}

struct Config {
  foo: u32,
  bar: u32,
  baz: Option<u32>
}

impl Config {
  const DEFAULT_FOO: u32 = 512;
}

impl ConfigBuilder {
  fn build(self) -> Result<Config> {
    Ok(Config {
      foo: self.foo.unwrap_or(Config::DEFAULT_FOO),
      bar: self.bar.ok_or_else(|| MissingField("bar"))?,
      baz: self.baz
    })
  }
}

In the second option:

You don't have to manually specify DEFAULT_FOO - it is generated by macro from default_value attr.
You have a default value specified right next (well, before) to the field itself
You don't need to specify that the field is required: it is required by definition unless it doesn't have a default value.
You can have a completely optional field. I am not sure that we need it now, but you can painlessly add it later.
You have default fallback in ConfigBuilder::build(). Current way to have defaults is to merge ConfigBuilder::default() with ConfigBuilder::from_file() with ConfigBuilder::from_env() and then finally call .build():
```
fn build(self) -> Result<Config> {
  Ok(Config {
    foo: self.foo.ok_or_else(|| MissingField("foo")?,
    // ...
  })
}
```
Imagine that foo has a default value. Thus, this error will never be thrown, semantically, right? Field with a default value could never be missing.

However, if you simply merge ::from_file() with ::from_env() and then fallback to default in .build():
```
fn build(self) -> Result<Config> {
  Ok(Config {
    foo: self.foo.unwrap_or(Config::DEFAULT_FOO),
    // ...
  })
}
```
you don't even need to check for the field existence, you simply fallback. This way more clearly expresses that a field with a default value could not throw a MissingField error.

config/derive/src/proxy.rs

0x009922 · 2023-05-23T23:07:07Z

config/derive/src/proxy.rs

+            /// Build [`Configuration`]
+            ///
+            /// # Errors
+            ///
+            /// - if missing required fields
+            pub fn build(self) -> Result<#parent_ident, ::iroha_config::Error> {
+                let config = Self::default().override_with(self);
+
+                Ok(#parent_ident { #(
+                    #field_names: config.#field_names.ok_or(
+                        ::iroha_config::Error::MissingField(stringify!(#field_names))
+                    )? ),*
+                })
+            }


It seems that the final Configuration should not have any Nones, otherwise you will get a missing field error. Is it the case? Isn't it possible that the final configuration could have an empty field that is not defined neither by user and by defaults?

Also, it would be nice to refactor code so that fields with default values could never throw a MissingField on a static-level. See my root review comment with an explanation.

These questions feel like they need to be phrased as tests.

One thing we found was useful was pair programming. You could contribute those directly to @mversic 's fork and expedite the invariant discussion.

0x009922 · 2023-05-23T23:56:47Z

Also it might close:

[suggestion] use defaults for unspecified config params #3461

cli/src/lib.rs

appetrosyan · 2023-05-24T19:13:56Z

cli/src/samples.rs

+        .set_max_transactions_in_block(2)
+        .set_trusted_peers(peers)
+        .build()
+        .expect("Valid");
+    let genesis = iroha_config::genesis::ConfigurationBuilder::default()
+        .set_account_private_key(Some(private_key))
+        .set_account_public_key(public_key)
+        .build()
+        .expect("Valid");
+    let block_sync = iroha_config::block_sync::ConfigurationBuilder::default()
+        .set_block_batch_size(1)
+        .set_gossip_period_ms(500)
+        .build()
+        .expect("Valid");
+
+    iroha_config::iroha::ConfigurationBuilder::default()
+        .set_public_key(public_key.clone())
+        .set_private_key(private_key.clone())
+        .set_sumeragi(sumeragi)
+        .set_block_sync(block_sync)
+        .set_genesis(genesis)


Relevant for #3528.

appetrosyan · 2023-05-24T19:35:59Z

cli/src/samples.rs

+        .set_max_transactions_in_block(2)
+        .set_trusted_peers(peers)
+        .build()
+        .expect("Valid");


Suggested change

.expect("Valid");

.unwrap();

Suggested change

.expect("Valid");

.expect("Max transactions in block = 2 is sensible. Trusted peers could be not sensible, which is why this function should have Returned `Result`, but we might as well panic here. These are the only two required fields.");

#3528 is to avoid this issue of having to justify all the values in the panic message, while retaining the strict guideline of only panicking in cases where the error is not recoverable or unreachable.

appetrosyan · 2023-05-24T19:41:08Z

cli/src/samples.rs

-///
-/// # Panics
-/// - when [`KeyPair`] generation fails (rare case).
+/// Get a sample Iroha configuration. Use [`get_trusted_peers`] to populate `trusted_peers` if in doubt.
 pub fn get_config(trusted_peers: HashSet<PeerId>, key_pair: Option<KeyPair>) -> Configuration {


API inconsistency. For trusted peers order either matters or it doesn't. Personally, just accept a PeerSet and make it a newtype builder with SmallVec backing.

client/src/client.rs

appetrosyan · 2023-05-24T19:48:43Z

client/src/lib.rs

+
+        let account_id = "alice@wonderland"
+            .parse()
+            .expect("This account ID should be valid");


Suggested change

.expect("This account ID should be valid");

.expect("`account_id` is invalid, see https://hyperledger.github.io/iroha-2-docs/guide/rust.html#_4-registering-an-account. ");

Ideally this should point to an API doc, but that's blocked by #1639.

appetrosyan · 2023-05-24T19:56:13Z

client/src/lib.rs

+            .set_private_key(private_key)
+            .set_account_id(account_id)
+            .build()
+            .expect("Client config should build as all required fields were provided")


Not actionable:
Ideally, point to API doc that shows which fields are required. The error message will tell us the rest.

appetrosyan · 2023-05-24T20:01:38Z

tools/kagami/src/main.rs

-            .add_parameter(WASM_MAX_MEMORY, DEFAULT_MAX_MEMORY)?
+            .add_parameter(
+                WSV_ACCOUNT_METADATA_LIMITS,
+                WsvConfiguration::DEFAULT_ACCOUNT_METADATA_LIMITS,


Consider use WsvConfiguration::*.

Unlike C++, Rust's imports are checked for conflicts and are scoped, so we can get away with shortening qualified paths more aggressively.

appetrosyan · 2023-05-24T20:03:27Z

core/test_network/src/lib.rs

@@ -818,8 +818,8 @@ impl TestClient for Client {
    fn test_with_key(api_url: &SocketAddr, telemetry_url: &SocketAddr, keys: KeyPair) -> Self {
        let mut configuration = ClientConfiguration::test(api_url, telemetry_url);
        let (public_key, private_key) = keys.into();
-        configuration.public_key = public_key;
-        configuration.private_key = private_key;
+        configuration.set_public_key(public_key);


Consider set_key_pair((public_key, private_key)). It saves one line, but in a lot of places.

appetrosyan · 2023-05-24T20:11:05Z

core/src/wsv.rs

@@ -162,9 +159,7 @@ impl WorldStateView {
    #[inline]
    pub fn new(world: World, kura: Arc<Kura>) -> Self {
        // Added to remain backward compatible with other code primary in tests


One workaround which I don't think we ever considered was separating tests by debug status. We're not currently running tests in release, and if we did, we shouldn't be using private API anyway.

appetrosyan · 2023-06-07T11:27:13Z

Fixing #3461 was a trivial one-line change. I would instead focus on getting good defaults.

0x009922

About optional fields. Now it is possible to define them as Option<..>. In the ConfigurationBuilder they become Option<Option<..>>. Could we avoid it?

0x009922 · 2023-05-25T22:47:26Z

config/src/sumeragi.rs

+        #[config(required)]
+        key_pair: KeyPair,


You said that this PR closes #3504, but this field is still in the user-facing configuration, and user still should not provide it. This field simply should not be in ConfigurationBuilder.

What does #[view(ignore)] means? Why isn't it specified under #[config(...)]?

0x009922 · 2023-06-08T04:16:17Z

config/src/iroha.rs

        /// `Kura` configuration
-        #[config(inner)]
-        pub kura: kura::Configuration,
+        #[config(default = "kura::Configuration::default()")]


I think we can use config(nested) and move this boilerplate into the macro.

0x009922 · 2023-06-08T04:21:19Z

config/src/logger.rs

-        }
-    }
+    #[cfg(all(feature = "tokio-console", not(feature = "no-tokio-console")))]
+    #[config(default = "= "127.0.0.1:5555"")]


default = "= "127.0.0.1:5555"" ?

0x009922 · 2023-06-08T04:25:33Z

config/src/kura.rs

    /// Maximum number of blocks to write into a single storage file.
-    pub blocks_per_storage_file: NonZeroU64,
+    // SAFETY: Value is not zero
+    #[config(default = "unsafe { NonZeroU64::new_unchecked(1000_u64) }")]


I would prefer const panic!. Also, for complex defaults you can define a separate const (AFAIU):

const DEFAULT_BLOCKS_PER_STORAGE: NonZeroU64 = match NonZeroU64::new(1000) { Some(x) => x, None => unreachable!() }; // ... #[config(default = "DEFAULT_BLOCKS_PER_STORAGE")]

0x009922 · 2023-06-08T04:29:03Z

config/base/derive/src/proxy.rs

+            }
+
+            /// Override [`Self`] with values from another [`Self`]
+            pub fn override_with(&mut self, other: Self) -> &mut Self { #(


Suggested change

pub fn override_with(&mut self, other: Self) -> &mut Self { #(

pub fn merge(mut self, other: Self) -> Self { #(

0x009922 · 2023-06-08T04:35:22Z

config/base/derive/src/proxy.rs

+        impl From<#parent_ident> for #builder_ident {
+            fn from(source: #parent_ident) -> Self {
+                Self { #( #field_names: Some(source.#field_names) ),* }
            }
-        set_field
-        });
+        }


What about optional fields, which might be None in the final configuration?

Signed-off-by: Marin Veršić <[email protected]>

appetrosyan · 2023-06-19T08:19:24Z

Not actual.

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 22, 2023

mversic force-pushed the default_config branch 2 times, most recently from 7a38c9e to 420b5ae Compare May 22, 2023 15:31

0x009922 reviewed May 23, 2023

View reviewed changes

mversic force-pushed the default_config branch 5 times, most recently from 912cf95 to d9935fb Compare May 23, 2023 06:07

0x009922 reviewed May 23, 2023

View reviewed changes

mversic force-pushed the default_config branch 2 times, most recently from 9b6f2c2 to b041e9d Compare May 24, 2023 05:32

appetrosyan changed the title ~~[refactor]: Rewrite macros for generating configuration~~ [refactor] #3501: Rewrite macros for generating configuration May 24, 2023

appetrosyan reviewed May 24, 2023

View reviewed changes

mversic force-pushed the default_config branch from b041e9d to 5d1088b Compare May 25, 2023 12:55

Arjentix assigned 0x009922 and appetrosyan Jun 1, 2023

mversic force-pushed the default_config branch 9 times, most recently from d2688f1 to 119734d Compare June 6, 2023 18:09

0x009922 reviewed Jun 8, 2023

View reviewed changes

wip

48134ec

Signed-off-by: Marin Veršić <[email protected]>

mversic force-pushed the default_config branch from 119734d to 48134ec Compare June 8, 2023 06:11

appetrosyan closed this Jun 19, 2023

mversic deleted the default_config branch April 16, 2024 09:36

	.expect("Valid");
	.expect("Max transactions in block = 2 is sensible. Trusted peers could be not sensible, which is why this function should have Returned `Result`, but we might as well panic here. These are the only two required fields.");

	.expect("This account ID should be valid");
	.expect("`account_id` is invalid, see https://hyperledger.github.io/iroha-2-docs/guide/rust.html#_4-registering-an-account. ");

	pub fn override_with(&mut self, other: Self) -> &mut Self { #(
	pub fn merge(mut self, other: Self) -> Self { #(

[refactor] #3501: Rewrite macros for generating configuration #3512

[refactor] #3501: Rewrite macros for generating configuration #3512

Conversation

mversic commented May 22, 2023 • edited by appetrosyan Loading

Description

Linked issue

Benefits

Checklist

0x009922 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appetrosyan May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x009922 left a comment • edited Loading

Choose a reason for hiding this comment

0x009922 May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x009922 commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appetrosyan commented Jun 7, 2023

0x009922 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appetrosyan commented Jun 19, 2023

mversic commented May 22, 2023 •

edited by appetrosyan

Loading

appetrosyan May 24, 2023 •

edited

Loading

0x009922 left a comment •

edited

Loading

0x009922 May 23, 2023 •

edited

Loading