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

alphanet: merge build changes and consensus params #4431

Merged
merged 51 commits into from
Aug 25, 2022
Merged

Conversation

cce
Copy link
Contributor

@cce cce commented Aug 19, 2022

Summary

This merges in a few changes related to alphanet

  • build changes to add an alphanet channel (alongside betanet, devnet, testnet, etc)
  • alphanet genesis file
  • alphanet test scenario recipe
  • alphanet consensus parameters

Test Plan

Existing tests should pass.
Added a new test to assert that non-official consensus version names (like "alpha3") don't make it into the upgrade history to ConsensusCurrentVersion.

cce and others added 30 commits May 26, 2022 12:53
Adjust the DNS Bootstrap Id for Alphanet
alphanet: define vAlpha2 consensus parameters
DevOps: Updated alphanet algonet recipe genesis files to use alpha2 protocol.
@cce cce changed the title testing: merge alphanet build changes and consensus params alphanet: merge build changes and consensus params Aug 19, 2022
@cce cce marked this pull request as ready for review August 19, 2022 16:34
@cce cce requested a review from algolucky August 19, 2022 16:35
onetechnical
onetechnical previously approved these changes Aug 19, 2022
Copy link
Contributor

@onetechnical onetechnical left a comment

Choose a reason for hiding this comment

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

The topology seems a little strange for me, but otherwise this looks good

algolucky
algolucky previously approved these changes Aug 19, 2022
egieseke
egieseke previously approved these changes Aug 19, 2022
Copy link
Contributor

@egieseke egieseke 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.

algolucky
algolucky previously approved these changes Aug 24, 2022
egieseke
egieseke previously approved these changes Aug 24, 2022
@cce cce dismissed stale reviews from egieseke and algolucky via c211e8d August 24, 2022 18:56
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

I do not know much about alphanet but LGTM

"VersionModifier": "",
"ConsensusProtocol": "alpha1",
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we change genesis'proto?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we update this for performance testing. The live network genesis doesn't change (it's in the installer dir) but we need to keep this updated.

Copy link
Contributor Author

@cce cce Aug 25, 2022

Choose a reason for hiding this comment

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

This is just to make the Jenkins tests running the "alphanet" recipe start up from the latest version — the real alphanet genesis is in installer/genesis/alphanet/genesis.json. I guess another approach would be to have an alphacurrent or alphafuture version defined to set this to

a.True(leadsTo, "Consensus protocol must have upgrade path from %v to %v", currentVersionName, latestVersionName)
}

func consensusUpgradesTo(a *require.Assertions, currentName, targetName protocol.ConsensusVersion) bool {
func checkConsensusVersionName(a *require.Assertions, name string) {
// ensure versions come from official specs repo
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this check important?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is a sanity check to ensure we don't accidentally enable vAlpha in official builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it was an idea I had before merging these alternate consensus versions, that I should add something to be extra careful to ensure a version like "alpha3" doesn't make it into the path from v7 to ConsensusCurrentVersion and into a release

@cce cce merged commit 5a81d99 into master Aug 25, 2022
@cce cce deleted the feature/alphanet branch August 25, 2022 17:37
@cce cce restored the feature/alphanet branch August 25, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants