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

panic in NonFinalizedState::commit_block before Canopy #1909

Merged
merged 3 commits into from
Mar 17, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Mar 15, 2021

Motivation

Panic for blocks before canopy is required for NonFinalizedState::commit_block after #1898 gets merged.

More info at #1903

Blocked on #1099

Solution

In this PR we add the panic however test cases will fail as we need mainnet blocks after Canopy. #1099

If we decide to also make this tests for the testnet then we also need #1104

If we only test mainnet we just need to replace the test vector numbers in the failing tests by the new ones. If we also do the testnet then a bit of the logic will need to change.

Related Tickets

Closes #1903

Review

This is in draft state until unblocked. No review needed yet.

@teor2345 teor2345 self-requested a review March 15, 2021 23:20
@teor2345
Copy link
Contributor

If we only test mainnet we just need to replace the test vector numbers in the failing tests by the new ones. If we also do the testnet then a bit of the logic will need to change.

We try to do mainnet and testnet tests whenever we can, because it gets us better coverage.

@oxarbitrage oxarbitrage marked this pull request as ready for review March 16, 2021 20:26
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

These look good, let's merge

@teor2345 teor2345 merged commit 9e1662d into ZcashFoundation:main Mar 17, 2021
@dconnolly dconnolly mentioned this pull request Mar 23, 2021
23 tasks
dconnolly added a commit that referenced this pull request Mar 23, 2021
Zebra's latest alpha checkpoints on Canopy activation, continues our work on NU5, and fixes a security issue.

Some notable changes include:

## Added
- Log address book metrics when PeerSet or CandidateSet don't have many peers (#1906)
- Document test coverage workflow (#1919)
- Add a final job to CI, so we can easily require all the CI jobs to pass (#1927)

## Changed
- Zebra has moved its mandatory checkpoint from Sapling to Canopy (#1898, #1926)
  - This is a breaking change for users that depend on the exact height of the mandatory checkpoint.

## Fixed
- tower-batch: wake waiting workers on close to avoid hangs (#1908)
- Assert that pre-Canopy blocks use checkpointing (#1909)
- Fix CI disk space usage by disabling incremental compilation in coverage builds (#1923)

## Security
- Stop relying on unchecked length fields when preallocating vectors (#1925)
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.

Panic on pre-Canopy blocks in NonFinalizedState::commit_block
2 participants