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

Hardfork2 #3136

Merged
merged 4 commits into from
Nov 27, 2019
Merged

Hardfork2 #3136

merged 4 commits into from
Nov 27, 2019

Conversation

tromp
Copy link
Contributor

@tromp tromp commented Nov 25, 2019


name: HardFork2
about: This hard-fork allows blocks with YEAR_HEIGHT/FLOONET_SECOND_HARD_FORK <= height < YEAR_HEIGHT*3/2 with header version 3 and Cuckaroom29 as secondary PoW.
title: HardFork2
labels: consensus breaking
assignees: @antiochp @yeastplume @quentinlesceller


Currently, consensus::valid_header_version(height, version) returns false for all height >= YEAR_HEIGHT.
This PR changes that to return true for version 3 at either FLOONET_SECOND_HARD_FORK <= height < YEAR_HEIGHT*3/2 on floonet or YEAR_HEIGHT <= height < YEAR_HEIGHT*3/2 on mainnet.

This PR also changes global::create_pow_context to return new_cuckaroom_ctx(edge_bits, proof_size) when consensus::valid_header_version(height, version) holds for version 3, which means that Cuckaroom29 [1] is the only valid secondary PoW after the hard fork.

Successfully tested on a mainnet fast sync.

[1] https://www.grin-forum.org/t/next-pow-cuckaroom-unveiled-at-grincon1

@antiochp antiochp added the consensus breaking Use for issues or PRs that will break consensus and force a hard fork label Nov 25, 2019
@antiochp antiochp added this to the 3.0.0 milestone Nov 25, 2019
@tromp tromp changed the title [WIP] Hardfork2 Hardfork2 Nov 26, 2019
Copy link
Member

@quentinlesceller quentinlesceller 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 to me.

@@ -130,6 +130,9 @@ pub const HARD_FORK_INTERVAL: u64 = YEAR_HEIGHT / 2;
/// Floonet first hard fork height, set to happen around 2019-06-20
pub const FLOONET_FIRST_HARD_FORK: u64 = 185_040;

/// Floonet second hard fork height, set to happen around 2019-12-19
pub const FLOONET_SECOND_HARD_FORK: u64 = 298_080;

/// Check whether the block version is valid at a given height, implements
/// 6 months interval scheduled hard forks for the first 2 years.
pub fn valid_header_version(height: u64, version: HeaderVersion) -> bool {
Copy link
Member

@antiochp antiochp Nov 26, 2019

Choose a reason for hiding this comment

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

Not directly related to this PR but it would be awesome if we had a function that took a block height and returned the valid header version (if there was one, maybe Option<HeaderVersion> to maintain the "hard cutoff" like we have now). We could then refactor valid_header_version() to use this internally. We would also be able to leverage it for block height specific logic elsewhere.

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; I've also been thinking about adding that, just to simplify the logic in valid_header_version.
The latter does have the advantage in making certain heights invalid regardless of version, which is a nice way to stop obsolete clients in their track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see latest commit...

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

I can't speak to the actual cuckaroo* changes but the hard fork cutoff and the logic around valid header versions looks good to me.

@antiochp
Copy link
Member

@tromp Is this good to go now?

@yeastplume Want to take a look before we merge this?

Copy link
Member

@yeastplume yeastplume left a comment

Choose a reason for hiding this comment

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

LGTM

@tromp
Copy link
Contributor Author

tromp commented Nov 27, 2019

should be good to go...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking Use for issues or PRs that will break consensus and force a hard fork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants