Mandatory checkpoint on Canopy - breaking cached state test change #1898
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Looks good, just missing an update to the testnet checkpoints. (And a few typos.)
I already have that synced, so I'll start generating the list now, and push it to this branch when it's done.
I also want to checkout this PR, and make sure we've replaced every relevant instance of Sapling with Canopy.
This change requires a cached state rebuild. The rebuilt state will be significantly larger.
I pushed this change. In general, we want to keep both checkpoint lists up to date, whenever we change one of them. |
There were lots of missing changes. Please use an interactive search and replace tool to create future PRs like this. I used the following commands: fastmod Sapling Canopy
fastmod sapling canopy
fastmod sapling canopy .githubThen I checked each change to make sure that it was correct. (Sapling is the name of a network upgrade, and a shielded pool, so we can't just do an automatic search and replace.) The |
|
@dconnolly this PR changes the cached tests to use Canopy, so it will require a cached state rebuild. Can you show @oxarbitrage how to do that? |
teor2345
changed the title
There was a problem hiding this comment.
@oxarbitrage can you check my changes to make sure they're ok?
I also added some follow-up tasks.
There was a problem hiding this comment.
- I didn't changed the RFCs in the initial PR because i wasn't sure if that was something we should do, for example i am unsure if a ZIP can be changed after is implemented. In this case i think is ok.
- I didn't added new testnet checkpoints by lack of time on Friday(had to sync a zcashd testnet), thanks for adding them.
- I missed some changes, thanks for the fixes.
Looks good.
There was a problem hiding this comment.
@oxarbitrage has checked my changes, and I've checked his.
Leaving this PR open so @dconnolly can check the cached state changes if she wants.
In general, I don't think we need to spend a lot of effort updating RFCs - if there is a significant change, we should do a rewrite or a new RFC. But in this case, it took only a few minutes to update. And I didn't want us to get confused between Sapling and Canopy checkpoints. |
teor2345
requested review from
dconnolly and
teor2345
and removed request for
teor2345
teor2345
changed the title
teor2345
changed the title
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)
Motivation
We want to checkpoint in Canopy to avoid the development of some validation code. More details in #1846
Solution
Change the mandatory checkpoint from Sapling to Canopy in the testnet and in the mainnet.
I synchronized zebra using this PR up to tip in 2 hours and 40 minutes. This is an improve (expected) from the last tries in the same machine of 3 hours.
In this PR mainnet checkpoints were upgraded while testnet ones were already passing Canopy so this was not needed.
Review
@teor2345
Related Issues
If merged this PR will close #1846
Follow Up Tasks
Post-canopy test vectors #1104 and #1099
Panic on pre-Canopy blocks in
NonFinalizedState::commit_block#1903