Fix the consensus chain benchmark command

[NingLin-P]

The benchmark command now requires the --genesis-builder-preset arg, which is a spec id that is used to load a JSON-encoded RuntimeGenesisConfig, if not specified "development" will be used. In our case, it will fail because we always return None despite the input:

subspace/crates/subspace-runtime/src/lib.rs

Lines 1537 to 1539 in 82e8e0a

	fn get_preset(id: &Option<sp_genesis_builder::PresetId>) -> Option<Vec<u8>> {
	get_preset::<RuntimeGenesisConfig>(id, |_| None)
	}

Ideally, we should follow the upstream practice of using spec id to load the responding RuntimeGenesisConfig for benchmark:
https://github.com/paritytech/polkadot-sdk/blob/cc4fe1ec1eee5d2141e8d6160d89bda2a9cf34b0/polkadot/runtime/westend/src/genesis_config_presets.rs#L412-L425
But because the domain runtime is also part of the consensus chain spec, which means the subspace-runtime will depend on all the domain runtime crates, doing so will increase the compile time a lot. cc @nazar-pc

This PR instead uses the default value of RuntimeGenesisConfig since each benchmark is coded for the worst scenario regardless of the RuntimeGenesisConfig value. The first commit is used to fix a deserialize issue about the default enable_rewards_at value because enable_rewards_at: EnableRewardsAt::Height(None) will encode as enable_rewards_at: {} and fail during decode.

Code contributor checklist:

• I have read, understood and followed contributing guide

[nazar-pc]

Why exactly does it fail? It is a valid value, so it should be possible to serialize and deserialize.

[NingLin-P]

Maybe an issue of serde_json, enable_rewards_at: EnableRewardsAt::Height(None) is encoded as "enable_rewards_at": {} and fail to decode EnableRewardsAt out of {}

[nazar-pc]

Encoding and decoding should be symmetrical, if it is not then it is a bug and needs to be fixed, not hacked around 🤔

[NingLin-P]

I agree, I will report the issue to serde_json with a minimal reproducible example, but IMO it is not a hack for us since EnableRewardsAt::Height(None) is used as the same as EnableRewardsAt::Height(Some(1)) in genesis_build

[nazar-pc]

I'm pretty confident this is not a bug in serde_json or serde, it is something on our end. While I'm not opposed to this, I'd prefer to understand what would be a solution first.

[teor2345]

Maybe an issue of serde_json, enable_rewards_at: EnableRewardsAt::Height(None) is encoded as "enable_rewards_at": {} and fail to decode EnableRewardsAt out of {}

That’s not the encoding of Height(None), where did you get it from?
Here’s a playground which shows all the encodings:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5c363e2b04832edcdf06c569c2d34cdf

We derive the serde encoding on that enum, and don’t add any attributes:

subspace/crates/pallet-subspace/src/lib.rs

Line 278 in 82e8e0a

#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]

Then the pallet macro adds a camel case attribute:

https://github.com/paritytech/polkadot-sdk/blob/a42dcbf610336f691c335943ab146ebeda17ae36/substrate/frame/support/procedural/src/pallet/expand/genesis_config.rs#L101

So the encodings are:

Height(None) -> {"height":null}
Height(Some(5)) -> {"height":5}
SolutionRange(10) -> {"solutionRange":10}
Manually -> "manually"

[NingLin-P]

Thanks @teor2345, I think this is an issue in the substrate, which will remove the kv pair from the json if it is a null value so {"height":null} will become {}:
https://github.com/paritytech/polkadot-sdk/blob/4e2473342fe861687e507a84dcf91e7024a846b9/substrate/client/chain-spec/src/genesis_config_builder.rs#L148-L154
https://github.com/paritytech/polkadot-sdk/blob/4e2473342fe861687e507a84dcf91e7024a846b9/substrate/client/chain-spec/src/json_patch.rs#L36-L38

[nazar-pc]

Please create a bug report with this, recursive dropping of null values is a very annoying behavior that makes values impossible to deserialize.

As to the fix, let's simply make EnableRewardsAt::Height contain BlockNumber rather than Option<BlockNumber>.

[NingLin-P]

Issue created paritytech/polkadot-sdk#6306

[teor2345]

I’m not sure what’s going wrong here, but that’s not the correct serde encoding.

[teor2345]

Maybe an issue of serde_json, enable_rewards_at: EnableRewardsAt::Height(None) is encoded as "enable_rewards_at": {} and fail to decode EnableRewardsAt out of {}

That’s not the encoding of Height(None), where did you get it from?
Here’s a playground which shows all the encodings:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5c363e2b04832edcdf06c569c2d34cdf

We derive the serde encoding on that enum, and don’t add any attributes:

subspace/crates/pallet-subspace/src/lib.rs

Line 278 in 82e8e0a

#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]

Then the pallet macro adds a camel case attribute:

https://github.com/paritytech/polkadot-sdk/blob/a42dcbf610336f691c335943ab146ebeda17ae36/substrate/frame/support/procedural/src/pallet/expand/genesis_config.rs#L101

So the encodings are:

Height(None) -> {"height":null}
Height(Some(5)) -> {"height":5}
SolutionRange(10) -> {"solutionRange":10}
Manually -> "manually"