refactor: disentangle test genesis and epoch config building #12554

eagr · 2024-12-04T06:08:09Z

Any inputs before I start to update the tests?

Longarithm

Great work. I don't fully understand all the implications but looks like a huge step forward :)
Leaving some readability suggestions, please ping me to add to merge queue.

Longarithm · 2024-12-06T19:47:40Z

core/primitives/src/epoch_manager.rs

+    pub fn add_config(&mut self, protocol_version: ProtocolVersion, config: EpochConfig) {
+        self.store.insert(protocol_version, Arc::new(config));
+    }
+


Looks like we don't need it anymore.

I was imagining this scenario of test cases that need to alter one or more of the mainnet configs, or adding to them. But if this is you-ain't-gonna-need-it, I'll remove it.

Longarithm · 2024-12-09T10:43:48Z

core/chain-configs/src/test_genesis.rs

+        let mut epoch_config =
+            Genesis::test_epoch_config(num_block_producer_seats, shard_layout, epoch_length);
+        epoch_config.num_block_producer_seats = num_block_producer_seats;
+        epoch_config.block_producer_kickout_threshold = 0;


nit: it's better to unite with the code below, like

epoch_config.block_producer_kickout_threshold = self.block_producer_kickout_threshold.unwrap_or_else(|| { let default = 0; tracing::warn!("Block producer kickout threshold not explicitly set, defaulting to {}.", default); default });

like it is done in TestGenesisBuilder. Same for other parameters.

Now that I look at this again, it seems like maybe we should bake some of the defaults into the builders rather than doing it this ad hoc way.

What is this warning ceremony useful for (given this is test-only)? I mean I sort of understand, but we might as well just log the entire configurations instead of one warning for each default being used, esp. when we're using the defaults most of the time.

Yeah, why not.

Looks like the purpose is to filter out explicitly set parameters - because the user likely knows about them - and display each of the implicit ones on separate line, because default Display would write all in one line which is completely unreadable. So the current approach looks slightly better than custom Display.

should bake some of the defaults into the builders rather than doing it this ad hoc way

I'll do it in a follow-up :))

Longarithm · 2024-12-09T10:50:25Z

core/chain-configs/src/test_genesis.rs

+        .genesis_time_from_clock(&FakeClock::default().clock())
+        .protocol_version(protocol_version)
+        .epoch_length(epoch_length)
+        .shard_layout(shard_layout.clone())


nit: I think we can remove this, because shard layout should be controlled by epoch config. And then we could move shard_layout to customize_epoch_config_builder. If that's not possible now, please add TODO.

If you want to work on it further, I think we can do the same with epoch_length... Currently we take it from genesis (but also put it to epoch config for whatever reason), but we could use it from epoch configs only.

Yeah, I'll try.

Longarithm · 2024-12-09T10:53:03Z

core/chain-configs/src/test_genesis.rs

@@ -565,3 +627,36 @@ fn derive_validator_setup(specs: ValidatorsSpec) -> DerivedValidatorSetup {
        },
    }
 }
+
+pub fn genesis_epoch_config_store(


Small concern: based on my experience with setup functions, it is problematic to read their calls on github because they look like epoch_config(2, 1, 2, 90, 60, 0) (actual example from code).
Could you consider introducing struct like GenesisAndEpochSharedParameters with named fields and pass it here? Also build_genesis_and_epoch_config_store looks like a slightly better name.

And please add comment what it is for - I guess, "Common method to construct genesis and epoch config store, use it if it is enough to have one epoch config for your test" (?)

Longarithm · 2024-12-09T10:57:51Z

integration-tests/src/tests/client/features/in_memory_tries.rs

+    let shard_layout = ShardLayout::simple_v1(&["account3", "account5", "account7"]);
+    let validators_spec = ValidatorsSpec::desired_roles(&["account0", "account1"], &[]);
+
+    let (genesis, _) = genesis_epoch_config_store(


nit: if we don't need epoch config, it seems better to construct genesis directly.

codecov · 2024-12-09T22:06:26Z

Codecov Report

Attention: Patch coverage is 76.55678% with 64 lines in your changes missing coverage. Please review.

Project coverage is 70.19%. Comparing base (0287e07) to head (a40838a).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
core/chain-configs/src/test_genesis.rs	73.87%	54 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12554      +/-   ##
==========================================
- Coverage   70.20%   70.19%   -0.02%     
==========================================
  Files         840      840              
  Lines      169837   169982     +145     
  Branches   169837   169982     +145     
==========================================
+ Hits       119233   119311      +78     
- Misses      45547    45602      +55     
- Partials     5057     5069      +12

Flag	Coverage Δ
backward-compatibility	`0.16% <0.00%> (-0.01%)`	⬇️
db-migration	`0.16% <0.00%> (?)`
genesis-check	`1.29% <0.00%> (+<0.01%)`	⬆️
linux	`69.42% <75.09%> (+<0.01%)`	⬆️
linux-nightly	`69.77% <73.62%> (-0.01%)`	⬇️
pytests	`1.59% <0.00%> (+<0.01%)`	⬆️
sanity-checks	`1.40% <0.00%> (-0.01%)`	⬇️
unittests	`70.01% <76.55%> (-0.02%)`	⬇️
upgradability	`0.20% <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.

eagr · 2024-12-09T22:08:52Z

Except for the audit issue, ready to go :) @Longarithm

Longarithm · 2024-12-10T00:18:43Z

Still it looks suboptimal to me to do some assignments twice:

        default_if_not_set!(
            minimum_stake_ratio,
            epoch_config.validator_selection_config.minimum_stake_ratio
        );
        epoch_config.validator_selection_config.minimum_stake_ratio = minimum_stake_ratio;

In general, I'm against macro, because they make code less readable than anything we discussed. Every time during debug, when I see custom macro_name!, I have to see what is inside, because macros can do arbitrary stuff.

I think the compromise is to get rid of Genesis::test_epoch_config and take default values from inside it. It's rarely used anyway. Then, we could use the following pattern:

fn config_value(builder_value: Option<T>, default_value: T, name: &str) -> T { ... }
...
let a = config_value(self.a, <default value>, stringify!(self.a)); // <default value> is ShardLayout::single_shard(), PROTOCOL_UPGRADE_STAKE_THRESHOLD, etc
...
EpochConfig {
  a: a,
  b: derived_validators_setup.b,
  ...
}

stringify! is much more common.

Side note - technically num_block_producer_seats_per_shard is currently incorrect. Though it's not used anymore, so we don't really care.

eagr · 2024-12-10T05:47:37Z

That pattern really stands out, couldn't resist the urge. :)) The projects I worked with before had much higher tolerance about macro usage. Guess I need to recalibrate it here.

I think the compromise is to get rid of Genesis::test_epoch_config and take default values from inside it.

That's exactly the plan for a follow up, and also the genesis config.

eagr · 2024-12-10T06:43:10Z

Let's do the defaults in a follow-up. This is already big enough and changes span over all the integration tests. Took me a while to solve the merge conflicts last time.

closes #12548 Any inputs before I start to update the tests?

eagr requested a review from a team as a code owner December 4, 2024 06:08

eagr requested a review from Longarithm December 4, 2024 06:08

disentangle genesis and epoch config building

efa087b

eagr force-pushed the disentangle-test-builder branch from 35f9743 to efa087b Compare December 4, 2024 13:10

eagr added 6 commits December 4, 2024 22:52

use proper builder pattern

18ee26a

better defaults

888a1d0

genesis & epoch config store factory

d9910d4

update tests

9eee80e

Merge branch 'master' into disentangle-test-builder

aca2e20

fixes

02221a1

Longarithm approved these changes Dec 9, 2024

View reviewed changes

fixes

0e4ffae

no macro

a40838a

Longarithm added this pull request to the merge queue Dec 10, 2024

Merged via the queue into near:master with commit 2251c76 Dec 10, 2024
24 of 27 checks passed

stedfn pushed a commit that referenced this pull request Dec 11, 2024

refactor: disentangle test genesis and epoch config building (#12554)

2455ec2

closes #12548 Any inputs before I start to update the tests?

eagr mentioned this pull request Dec 13, 2024

refactor: bake defaults into test builders #12618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: disentangle test genesis and epoch config building #12554

refactor: disentangle test genesis and epoch config building #12554

eagr commented Dec 4, 2024 •

edited

Loading

Longarithm left a comment

Longarithm Dec 6, 2024

eagr Dec 9, 2024

Longarithm Dec 9, 2024

eagr Dec 9, 2024 •

edited

Loading

Longarithm Dec 9, 2024

eagr Dec 9, 2024

Longarithm Dec 9, 2024

eagr Dec 9, 2024

Longarithm Dec 9, 2024

Longarithm Dec 9, 2024

Longarithm Dec 9, 2024

codecov bot commented Dec 9, 2024 •

edited

Loading

eagr commented Dec 9, 2024

Longarithm commented Dec 10, 2024

eagr commented Dec 10, 2024

eagr commented Dec 10, 2024

refactor: disentangle test genesis and epoch config building #12554

refactor: disentangle test genesis and epoch config building #12554

Conversation

eagr commented Dec 4, 2024 • edited Loading

Longarithm 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

eagr Dec 9, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

eagr commented Dec 9, 2024

Longarithm commented Dec 10, 2024

eagr commented Dec 10, 2024

eagr commented Dec 10, 2024

eagr commented Dec 4, 2024 •

edited

Loading

eagr Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading