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

fix(consensus): Refactor block subsidy to handle Height::MAX without panicking #5787

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 5, 2022

Motivation

Zebra panics when doing the block subsidy calculations for Height::MAX.

This is unlikely to ever happen on mainnet, because it only happens when block subsidies are all zero zats. But it's good to clean it up anyway, to avoid future changes causing actual panics in production.

Specifications

Screen Shot 2022-12-05 at 18 25 28

https://zips.z.cash/protocol/protocol.pdf#subsidies

Designs

Use Option<u64> internally whenever 2Halving(height) would overflow a u64.

Solution

Refactor the halving divisor to avoid overflows and panics.

Add tests for Height::MAX and other large values.

Review

This is a routine consensus rule fix, it would be good to get two people to review it.

This doesn't need to be included in the audit, because it only happens with very large heights.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates P-Medium ⚡ C-security Category: Security issues I-panic Zebra panics with an internal error message labels Dec 5, 2022
@teor2345 teor2345 requested a review from a team as a code owner December 5, 2022 08:29
@teor2345 teor2345 self-assigned this Dec 5, 2022
@teor2345 teor2345 requested review from dconnolly and removed request for a team December 5, 2022 08:29
@teor2345
Copy link
Contributor Author

teor2345 commented Dec 7, 2022

This isn't needed for anything else now, so it's a low priority.

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think this looks good so probably better to just merge now instead of leaving it hanging there.

mergify bot added a commit that referenced this pull request Dec 12, 2022
mergify bot added a commit that referenced this pull request Dec 12, 2022
@mergify mergify bot merged commit ea64585 into main Dec 13, 2022
@mergify mergify bot deleted the block-subsidy-panic branch December 13, 2022 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates C-bug Category: This is a bug C-security Category: Security issues I-panic Zebra panics with an internal error message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants