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

HardFork4 #3478

Merged
merged 5 commits into from
Nov 24, 2020
Merged

HardFork4 #3478

merged 5 commits into from
Nov 24, 2020

Conversation

tromp
Copy link
Contributor

@tromp tromp commented Oct 18, 2020


name: HardFork4
about: make header version 5 valid forever at 2*YEAR_HEIGHT
title: Hard Fork 4
labels: consensus-breaking, must-have, critical
assignees: antiochp, quentinlesceller, yeastplume


Still needs proper setting of TESTNET_FOURTH_HARD_FORK;
waiting for testnet to progress at predictable pace...

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.

This looks good to me.
Couple of minor comments.

👍

core/src/consensus.rs Outdated Show resolved Hide resolved
core/src/global.rs Show resolved Hide resolved
new_cuckarood_ctx(edge_bits, proof_size)
if chain_type == ChainTypes::Mainnet || chain_type == ChainTypes::Testnet {
// Mainnet and Testnet have Cuckatoo31+ for AF and Cuckaroo{,d,m,z}29 for AR
if edge_bits > 29 {
Copy link
Member

@antiochp antiochp Nov 18, 2020

Choose a reason for hiding this comment

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

Consider making this explicit on header_version() also?

if header_version(height) >= 5 || edge_bits > 29 {
   ...
}

Would we then want to explicitly fail if we actually had edge_bits <= 29 after HF4?

I guess this depends on how we are thinking about this. This is driven off edge_bits in this PR and we fallthrough to no_cuckaroo_ctx() if edge_bits are too low and we are beyond HF4.
My only concern here is that is not immediately intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asking for a 29 context past HF4 should result in a clear error such as no_cuckaroo_ctx() rather than a dubious CuckAToo29 context, which is not an outright error, but otherwise quite invalid.

Copy link
Member

Choose a reason for hiding this comment

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

Yes absolutely - just wondering if there is a more explicit way of implementing this, failing earlier in the code with a no_cuckatoo_ctx().

@lehnberg
Copy link
Collaborator

fixes: #3472

@tromp tromp changed the title [WIP] HardFork4 HardFork4 Nov 23, 2020
@antiochp antiochp merged commit a5b8968 into mimblewimble:master Nov 24, 2020
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.

3 participants