Skip to content
New issue

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

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

Already on GitHub? # to your account

Commit window fixes #2177

Merged
merged 14 commits into from
Dec 16, 2020
Merged

Commit window fixes #2177

merged 14 commits into from
Dec 16, 2020

Conversation

kantai
Copy link
Contributor

@kantai kantai commented Dec 15, 2020

This PR addresses #2141, by introducing a "burn parent modulus" to allow the stacks-node to detect when a commitment may be missed. This PR also updates the smoothing function to:

min(last_burn, median)

Which is less punitive than mean(min, median)

Type of Change

  • Bug fix

Does this introduce a breaking change?

Yes, this changes the wire format for block commits (change is documented in SIP-001), which would lead to a consensus-breaking change.

Documentation updates are probably required -- any mining references we have should be updated to reflect that the smoothing function is changing from

mean(min, median) to min(last_burn, median)

Testing information

There's a few different ways that this is tested:

  1. Changes to distribution are unit tested in that module.
  2. Changes to the parsing of LeaderBlockCommitOp are tested in that module
  3. An integration test for missed block commits is included in the chainstate::coordinator tests. This required adding cfg(test) enabled storage of burn distributions so that the evaluated distribution could be checked after a burn block is processed.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

This LGTM, but please add the bitmask in the block-commit before merging. Thanks!

Copy link
Contributor

@lgalabru lgalabru 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!

@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 27, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants