test: improve test loop builders

[pugachAG]

This PR introduces alternative way to configure genesis and epoch configs for test loop.

Currently the most common way to do that is build_genesis_and_epoch_config_store. I have the following issues with it:

• GenesisAndEpochConfigParams forces you to set values for parameters even if you are perfectly happy with TestEpochConfigBuilder defaults. This pollutes test setup because it is unclear which parameter values actually matter for the tests.
• Callbacks to customise genesis and epoch config builders results in weird inversion of control. Also it just looks cumbersome for simple test setup.

The alternative way is to derive epoch config builder from genesis state and use it directly.

        let genesis = TestLoopBuilder::new_genesis_builder()
            .validators_spec(validators_spec.clone())
            .shard_layout(shard_layout.clone())
            .add_user_accounts_simple(..)
            .build();
        let epoch_config_store = TestEpochConfigBuilder::build_store_from_genesis(&genesis);

and for more complicated cases where further customisation of epoch config is required:

let genesis = TestLoopBuilder::new_genesis_builder()
            .validators_spec(validators_spec.clone())
            .shard_layout(shard_layout.clone())
            .add_user_accounts_simple(..)
            .build().
let epoch_config_store = TestEpochConfigBuilder.from_genesis(&genesis)
            .minimum_validators_per_shard(2)
            .build_store_for_single_version();

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.61%. Comparing base (1ce9e63) to head (3f05450).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12995      +/-   ##
==========================================
- Coverage   69.68%   69.61%   -0.07%     
==========================================
  Files         859      859              
  Lines      175699   175897     +198     
  Branches   175699   175897     +198     
==========================================
+ Hits       122435   122451      +16     
- Misses      48104    48282     +178     
- Partials     5160     5164       +4     

Flag	Coverage Δ
backward-compatibility	0.36% <0.00%> (-0.01%)	⬇️
db-migration	0.36% <0.00%> (-0.01%)	⬇️
genesis-check	1.43% <0.00%> (-0.01%)	⬇️
linux	69.00% <80.00%> (-0.07%)	⬇️
linux-nightly	69.16% <100.00%> (-0.09%)	⬇️
pytests	1.74% <0.00%> (-0.01%)	⬇️
sanity-checks	1.55% <0.00%> (-0.01%)	⬇️
unittests	69.44% <100.00%> (-0.07%)	⬇️
upgradability	0.36% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shreyan-gupta]

So, I took a detailed look at the epoch config builder and genesis builder and I have a couple of observations/suggestions

• Seems like we want to coordinate between the genesis we create and the epoch config we create. Specifically we want epoch_length, shard_layout, and validators_spec, (maybe protocol version) to be coordinated between them.
• Ideally TestGenesisBuilder shouldn't be "overloaded" to also return a default EpochConfigStore. These two should be kept as far apart as possible to avoid complexity.
• To me it seems like we can cleanly break building genesis and epoch_config_store into two separate disjoint steps, first where we build the genesis and second where we use the genesis to build the epoch_config_store.

How about we make the following changes?

• Let TestGenesisBuilder only deal with making genesis
• Change TestEpochConfigBuilder::new() to always take genesis as an input param. This would ensure that the configs are coordinated between genesis and epoch_config.
• Remove build_genesis_and_epoch_config_store in favor of this two step process, avoiding things like GenesisAndEpochConfigParams, customize_genesis_builder, customize_epoch_config_builder which just decrease readability and increase complexity imo.

[shreyan-gupta]

Do we need this additional function just for the call to genesis_time_from_clock?

[pugachAG]

yes, this is needed to avoid copying .genesis_time_from_clock(&near_async::time::FakeClock::default().clock()) for every test loop test:

• code duplication is not nice
• forgetting that will result in very hard-to-debug test loop failure

[shreyan-gupta]

The pattern would look something like this then...

let genesis = TestGenesisBuilder::new()
    .genesis_time_from_clock(&FakeClock::default().clock())
    .protocol_version(protocol_version)
    .epoch_length(epoch_length)
    .shard_layout(shard_layout)
    .validators_spec(validators_spec)
    ...
    .build();

let epoch_config = TestEpochConfigBuilder::new(&genesis)
    .minimum_validators_per_shard(1)
    .shuffle_shard_assignment_for_chunk_producers(true)
    ...
    .build();

let epoch_config_store = EpochConfigStore::test_single_version(protocol_version, epoch_config);

We can even combine the last two steps to get something like this

let epoch_config_store = TestEpochConfigBuilder::new(&genesis)
    .minimum_validators_per_shard(1)
    .shuffle_shard_assignment_for_chunk_producers(true)
    ...
    .build_store_for_single_version(protocol_version);

[pugachAG]

@shreyan-gupta thanks for you suggestions, totally makes sense! I've updated the implementation, please take a look.

[shreyan-gupta]

Looks great! Thank you Anton

[shreyan-gupta]

In a separate PR, should we consider replacing build_genesis_and_epoch_config_store with the new build mechanism or do you see value retaining it?

[pugachAG]

@shreyan-gupta yes, I would like to replace build_genesis_and_epoch_config_store, I plan submitting PR for that later