Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update mycelo config values to work with monorepo e2e tests #1579

Merged
merged 4 commits into from
Jun 30, 2021

Conversation

oneeman
Copy link
Contributor

@oneeman oneeman commented Jun 10, 2021

Description

This is a companion PR to celo-org/celo-monorepo#8086, with some changes which are needed to make the tests work:

  1. Monorepo uses a single validator group, so we update the mycelo monorepo template to do so as well.
  2. Make the DowntimeSlasher's slashable downtime small so it works with the small epoch sizes we use (if the slashable downtime is larger than the epoch size, you can't test out slashing)
  3. Modify the monorepo template's config values for epoch rewards and initial reserve balance to match what is done in the monorepo for the 'testing' context (which is what the e2e tests use). See https://github.com/celo-org/celo-monorepo/blob/c90edf1c938363e24746c90153d36263bb668aef/packages/protocol/migrationsConfig.js#L244-L281 .

Tested

  • Tested in conjunction with the monorepo tests PR above, locally and multiple times in CI. Did not witness flakiness in the e2e tests.

Related issues

Backwards compatibility

No incompatibility

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #1579 (e4e034c) into master (46be11f) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1579      +/-   ##
==========================================
- Coverage   54.64%   54.63%   -0.02%     
==========================================
  Files         607      607              
  Lines       69935    69935              
==========================================
- Hits        38213    38206       -7     
- Misses      28296    28309      +13     
+ Partials     3426     3420       -6     
Impacted Files Coverage Δ
p2p/discover/v4_udp.go 74.57% <0.00%> (-4.24%) ⬇️
les/distributor.go 70.31% <0.00%> (-2.35%) ⬇️
whisper/whisperv6/message.go 66.23% <0.00%> (-1.30%) ⬇️
rpc/handler.go 89.24% <0.00%> (-1.08%) ⬇️
rpc/client.go 79.60% <0.00%> (-0.81%) ⬇️
les/fetcher.go 44.57% <0.00%> (-0.68%) ⬇️
eth/fetcher/block_fetcher.go 82.90% <0.00%> (-0.65%) ⬇️
p2p/simulations/http.go 66.66% <0.00%> (-0.65%) ⬇️
eth/downloader/downloader.go 74.76% <0.00%> (-0.21%) ⬇️
trie/proof.go 75.40% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46be11f...e4e034c. Read the comment docs.

@oneeman oneeman force-pushed the oneeman/mycelo-tweaks-for-e2e-tests branch from 5706cee to ff4d060 Compare June 15, 2021 22:43
@oneeman
Copy link
Contributor Author

oneeman commented Jun 16, 2021

Why and who approved it the changes ?? Why are you changing ?? Do not change it

@alice-dfi Could you elaborate on your concern? Mycelo is a tool to facilitate development and testing. It it used to generate genesis files for new ephemeral testnets. The changes in this PR modify two things:

  1. The default slashable downtime value, because the previous one doesn't make sense given the default epoch size
  2. Settings for mycelo's 'monorepo' template to make it match more closely the configuration used in celo-monorepo (which is the reason that template exists)

These changes only affect the generation of new genesis files from scratch using mycelo. They do not affect existing networks nor even the generation of a genesis file from an existing mycelo directory with a genesis config. Plus, the genesis config as a whole can be customized by the user to suit their needs. So if you have any concerns about this, please provide more details.

Or Neeman added 2 commits June 16, 2021 16:00
1. Monorepo uses a single validator group
2. Make the DowntimeSlasher's slashable downtime small so it works with the small epoch sizes we use
@oneeman oneeman force-pushed the oneeman/mycelo-tweaks-for-e2e-tests branch from 952dbcd to 5d823c2 Compare June 16, 2021 22:00
@oneeman oneeman requested review from trianglesphere, a team and hbandura and removed request for a team June 16, 2021 22:24
@oneeman oneeman marked this pull request as ready for review June 16, 2021 22:24
@oneeman oneeman requested a review from a team as a code owner June 16, 2021 22:24
Copy link
Contributor

@mcortesi mcortesi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, left one comment

@@ -147,7 +147,7 @@ func BaseConfig() *Config {
DowntimeSlasher: DowntimeSlasherParameters{
Reward: bigIntStr("10000000000000000000"), // 10 cGLD
Penalty: bigIntStr("100000000000000000000"), // 100 cGLD
SlashableDowntime: 60, // Should be overridden on public testnets
SlashableDowntime: 4, // make it small so it works with small epoch sizes, e.g. 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this desirable for every network template or only for monorepo one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. The default epoch size (i.e. the epoch size of the default template) is 10, so the former slashable downtime of 60 doesn't make sense with that.

The only template that has a different epoch size is the load test (1000). If we wanted to avoid slashable downtime of 4 with epoch size 1000 it would make sense to override it on the loadtest template rather than have 60 be the default. But the low slashable downtime makes no difference if nobody does the slashing, so I don't see it as mattering for the load test template anyway.

@oneeman oneeman merged commit ad5ad3e into master Jun 30, 2021
@oneeman oneeman deleted the oneeman/mycelo-tweaks-for-e2e-tests branch June 30, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants