Conditional compilation for the runtime proposals parameters.

[shamil-gadelshin]

Summary

We created a separate runtime module(file) In order to be able to pass the proposal parameter constants during the compilation time. The primary goal for the conditional compilation is integration tests. All parameters for the proposal could be set via ALL_PROPOSALS_PARAMETERS_JSON environment variable. The value of the variable should be a valid JSON file (a sample file is included in the PR). The PR contains a migration of the single proposal parameter set for demo purposes.

Comments

• We used lite-json crate as "no_std" option for parsing JSON recommended for Substrate. serde_json is not 'no_std' ready. serder_json_core library is not working.
• Tests are ready for the code but disabled (marked ignore).

[bedeho]

This certainly looks very promising! Did you review other options beyond environment variable approach? I hope you did not presume that I had looked into it as the best approach.

[shamil-gadelshin]

This certainly looks very promising! Did you review other options beyond environment variable approach? I hope you did not presume that I had looked into it as the best approach.

I considered other options as well:

1. fine-grained environment variables for each parameter for each proposal.

2. compile time code/json file inclusion.

3. code/json generation during the "build.rs" phase.

4. non-json formats

"JSON via enviroment variable" approach was chosen because it has at least basic library support and provides type-safety for the main (default parameters) path. Other options are less convinient for both configuring and using or more dangerous because of lacking type safety.

[shamil-gadelshin]

There is a way to do a conditional compilation in rust:

rustc --cfg 'unix'

let machine_kind = if cfg!(unix) {
  "unix"
} else if cfg!(windows) {
  "windows"
} else {
  "unknown"
};

It also could be used as key-value pairs: for examples, --cfg 'verbose' or --cfg 'feature="serde"'. These correspond to cfg!(verbose) and cfg!(feature = "serde") respectively.

We have three problems:

• A. Different data configuration based on compilation parameters (conditional compilation).
• B. Data configuration format.
• C. Data configuration source.

C-like "cfg" option helps to solve (A) in a convenient way. But (B) and (C) lead to a very verbose format of both specifying and parsing eight parameters for a dozen proposals.

The environment variable also solves (A). One of the first considered approaches was to use "ENV" as an indicator that JSON (B) should be loaded whether from a predefined path or path derived from this/another ENV (C).

rust --cfg is typically used as switch in code branches rather than for configuration-rich purposes.

JSON was chosen as (B) as a widely used format that has perfect support in JS integration tests environment. Also, it allows to define similar parameter structure for different proposal and avoid Cartesian product (CROSS JOIN) of proposals and parameters, unlike the "cfg" or "env-var per parameter" approach.

Having JSON in a separate file would make 'JSON-path' (C) to be part of the "configuration interface" which is unnecessary let alone that filesystem IO could lead to such rare problems as file access permissions.

So, this how "environment-variable JSON" approach was selected.

[bedeho]

Isn't this, at runtime, reading a JSON object via an environment variable and attempting to decode all parameters from it? If so, then parameters are not set at compile time. You could run the same binary multiple times with a different set of input values, or put differently, I could run my full node, and if something corrupted my json file, then I would be out of consensus.
I am also not really catching where this is really dealing with the possibility of the json not being well formatted?

[shamil-gadelshin]

1. This PR contains option_env macro as a solution for the conditional compilation:

If the named environment variable is present at compile time, this will expand into an expression of type Option<&'static str> whose value is Some of the value of the environment variable.

2. If the JSON document is empty or malformed or has missing parts - default parameters would be used for parts that cannot be extracted from it.

[mnaamani]

This looks good. I'm happy with the selection of env variable to pass in the custom configuration, especially when considering building docker images of the joystream node. I think there might be issues with the --cfg approach when it comes to build the WASM runtime, not clear how the custom runtime/build.rs and wasm builder tools would interfere there.

From what I can tell that yes the "static" section:

lazy_static! {
    static ref ALL_PROPOSALS_PARAMETERS: AllProposalsParameters =
        get_all_proposals_parameters_objects();
}

ensures this initial value is set at compile time, not dynamically at runtime.

I would prefer that all parameter properties would be required and not optional (being overridden with default when omitted) this helps to ensure the users customizing the parameters does not mistakenly omit a value, although I appreciate the that it makes it easier on the testing framework. However I think if make all parameters required and lite-json has traits that can be derived on the ProposalParameters it can make the code for extract_proposal_parameters and extract_numeric_value less verbose.

I will try to build and test from this branch to ensure in native and wasm builds of the runtime the env variable does indeed work as designed.

But initially my gut tells me we might have to consider how build.rs and the wasm builder https://github.com/Joystream/joystream/blob/b2057380b50ba23be278b7e87b557f19dd9dba4e/runtime/build.rs#L20 behave to ensure env variable changes do indeed trigger a re-bulid of the runtime (since no source files are touched)

[mnaamani]

Just a clarification on the point regarding re-triggering of the runtime build. My concern is running cargo build which results in a node binary where the native runtime does not match the wasm blob runtime, but would have the same runtime version which results in a binary that will break from consensus as a result. This is not an issue when cleaning the cargo build artifacts before running a build, or make a clean docker image build (without caching of cargo artifacts)

Its also possible that an env var change doesn't even trigger a rebuild of native code for that matter.

[shamil-gadelshin]

lite-json doesn't have support for #[derive] attribute similar to serde-json. And serde-json doesn't work in the 'no_std' environment as well as other serde compatible libraries. However, I was able to reduce code verbosity by introducing some helper macro's.

[bedeho]

This PR contains option_env macro as a solution for the conditional compilation:

Great.

If the JSON document is empty or malformed or has missing parts - default parameters would be used for parts that cannot be extracted from it.

Is this silent error advisable? Seems to be worth failing more loudly during building. It will take next to nothing to get such an input file corrupted in some way.

[shamil-gadelshin]

Our current use case is to set only grace_period for some proposals. Other parameters won't be changed. And I'm not sure we would even configure all proposals. Also, when we add a new proposal we won't break any tests because we would get default parameters for it. So allowing partially valid JSON is the desired behavior.

[shamil-gadelshin]

I added 'panic' for the corrupted JSON env. It could panic only if the env-var set on compilation time, so it is safe by default.

[mnaamani]

Its also possible that an env var change doesn't even trigger a rebuild of native code for that matter.

So this is a non-issue because of how the macro works in:

let json_str: Option<&'static str> = option_env!("ALL_PROPOSALS_PARAMETERS_JSON");

The macros added in b29aaa7 are great to easily add the additional proposals 👍

Code builds, but there are some cargo-fmt changes needed.

I can't seem to get the build to panic when passing invalid JSON like this:

ALL_PROPOSALS_PARAMETERS_JSON="{" scripts/cargo-build.sh

[shamil-gadelshin]

I pushed the code formatting commit.

I can't seem to get the build to panic when passing invalid JSON like this:
ALL_PROPOSALS_PARAMETERS_JSON="{" scripts/cargo-build.sh

It won't panic during the build, because it just creates a string during the compilation. It will panic later during the execution (during the string parsing).

[mnaamani]

I pushed the code formatting commit.

I can't seem to get the build to panic when passing invalid JSON like this:
ALL_PROPOSALS_PARAMETERS_JSON="{" scripts/cargo-build.sh

It won't panic during the build, because it just creates a string during the compilation. It will panic later during the execution (during the string parsing).

Thanks I was able to force the panic, as you suggested when the ::get() call is invoked when the value is read.
It would be better to test this bad state early, so I added the config value as a const in the decl_module! section of the codex module.
So now as soon as a polkadot/api client connects to the node, it will attempt to read all the constants and cause the panic:

Thread 'event.loop0' panicked at 'Invalid JSON with proposals parameters provided.', runtime/src/proposals_configuration/mod.rs:52

This is a bug. Please report it at:

	https://www.joystream.org/

2020-11-09 16:34:03 Unknown error: Client(Execution(RuntimePanicked("Invalid JSON with proposals parameters provided.")))
2020-11-09 16:34:05 💤 Idle (0 peers), best: #6 (0xf85d…4cd9), finalized #4 (0xd0b9…eb91), ⬇ 0 ⬆ 0
2020-11-09 16:34:06 🙌 Starting consensus session on top of parent 0xf85d79308ccb62db4eb62db6dda2c4cbea9d4f4f6b3ae6941a9a9415201f4cd9
2020-11-09 16:34:06 🎁 Prepared block for proposing at 7 [hash: 0xbb2e3be77fbe55797da9b955275dfdb8a4957134206ea5d8e4f58be67c3c1917; parent_hash: 0xf85d…4cd9; extrinsics (2): [0x3622…bb19, 0x61b4…8502]]
2020-11-09 16:34:06 🔖 Pre-sealed block for proposal at 7. Hash now 0xaf3b2f6b8ffba35e534596e5a785c549d47ee8728cb271adfb27aa56072b55d4, previously 0xbb2e3be77fbe55797da9b955275dfdb8a4957134206ea5d8e4f58be67c3c1917.
2020-11-09 16:34:06 ✨ Imported #7 (0xaf3b…55d4)
2020-11-09 16:34:10 💤 Idle (0 peers), best: #7 (0xaf3b…55d4), finalized #5 (0x0ba1…b8c3), ⬇ 0 ⬆ 0
2020-11-09 16:34:12 🙌 Starting consensus session on top of parent 0xaf3b2f6b8ffba35e534596e5a785c549d47ee8728cb271adfb27aa56072b55d4
2020-11-09 16:34:12 🎁 Prepared block for proposing at 8 [hash: 0x950c749e5c79358f10cda5aa2b9f34911ede2f15a99795125d491730989540f3; parent_hash: 0xaf3b…55d4; extrinsics (2): [0x4c68…7bc8, 0xb209…3897]]
2020-11-09 16:34:12 🔖 Pre-sealed block for proposal at 8. Hash now 0x897c63c7da775539378ff7c398ad2e51f4a1c4b7d071ecef677ab0517a85e679, previously 0x950c749e5c79358f10cda5aa2b9f34911ede2f15a99795125d491730989540f3.
2020-11-09 16:34:12 ✨ Imported #8 (0x897c…e679)
2020-11-09 16:34:15 💤 Idle (0 peers), best: #8 (0x897c…e679), finalized #6 (0xf85d…4cd9), ⬇ 0 ⬆ 0
2020-11-09 16:34:18 🙌 Starting consensus session on top of parent 0x897c63c7da775539378ff7c398ad2e51f4a1c4b7d071ecef677ab0517a85e679
2020-11-09 16:34:18 🎁 Prepared block for proposing at 9 [hash: 0xc0bc57d6cb35a094c944c16bf3c0c92335c2a686f45ae7e6869dfa07dde04c80; parent_hash: 0x897c…e679; extrinsics (2): [0x9f4c…e08a, 0x2f34…684b]]
2020-11-09 16:34:18 🔖 Pre-sealed block for proposal at 9. Hash now 0xf1ea6bc97f59671a5389e7afe339c9e52f98048e5c15f25edc3a3ae4299048a2, previously 0xc0bc57d6cb35a094c944c16bf3c0c92335c2a686f45ae7e6869dfa07dde04c80.

Interestingly the node keeps producing blocks, but they don't get finalized.

On the client side the error is also propagated:

 2020-11-09 16:34:03        RPC-CORE: getMetadata(at?: BlockHash): Metadata:: -32603: Unknown error occurred: Client(Execution(RuntimePanicked("Invalid JSON with proposals parameters provided.))

[mnaamani]

With these values I'm still getting Invalid JSON panic.

ALL_PROPOSALS_PARAMETERS_JSON="{ev
  "set_validator_count_proposal": {
    "voting_period": 1,
    "grace_period": 2,
    "approval_quorum_percentage": 3,
    "approval_threshold_percentage": 4,
    "slashing_quorum_percentage": 5,
    "slashing_threshold_percentage": 6,
    "required_stake": 7,
    "constitutionality": 8
  }
}" yarn cargo-build

Also an observation/question. since required_stake is an Option I assume I can set it to None by assigning it to null, and if omitted in the JSON, it should get the default value which is Some(100_000). Correct ?

But I'm not able to test and confirm this because the JSON is not being validated.

[shamil-gadelshin]

The JSON you provided is invalid indeed near the beginning ALL_PROPOSALS_PARAMETERS_JSON="{ev

Also an observation/question. since required_stake is an Option I assume I can set it to None by assigning it to null, and if omitted in the JSON, it should get the default value which is Some(100_000). Correct ?

Currently, there is no possibility to set None. But I will add this by converting 0(zero) to the None. Omitted fields are defaulted.

[mnaamani]

Currently, there is no possibility to set None. But I will add this by converting 0(zero) to the None. Omitted fields are defaulted.

I guess that is acceptable, but double check if lite-json can detect a null json value.

Regarding that extra "{ev in the string i think it was just some shell weirdness when I was scrolling up, but I will try again to be extra sure

[mnaamani]

Silly me it was an issue with quotes, I needed to use single quotes around the json like this:

ALL_PROPOSALS_PARAMETERS_JSON='{
  "set_validator_count_proposal": {
    "voting_period": 1,
    "grace_period": 2,
    "approval_quorum_percentage": 3,
    "approval_threshold_percentage": 4,
    "slashing_quorum_percentage": 5,
    "slashing_threshold_percentage": 6,
    "required_stake": 7,
    "constitutionality": 8
  }
}' yarn cargo-build

Works. Tested using: #6