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

deps(ecc): Update zcash_primitives and orchard only for zebra-chain crate #8522

Closed
wants to merge 8 commits into from

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We are trying to split the dependency upgrades needed for zcashd 5.9.0. As doing everything at once is too complicated we are trying to split it up in multiple pull requests. This pull request do the upgrades only for zebra-script crate so is not mergable.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?

Solution

By now, this PR is just compare with what @upbqdn was working on in regards to upgrade the zebra-scan crate.

Testing

  • cargo test --package zebra-chain pass.
  • cargo test --package zebra-consensus pass.

Review

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

@oxarbitrage oxarbitrage added A-dependencies Area: Dependency file updates do-not-merge Tells Mergify not to merge this PR labels May 13, 2024
Copy link
Contributor Author

@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.

Almost everything here is renames and reimport except for some comments i added to the code as review comments.

&null_sapling_ovk,
output,
sapling::note_encryption::Zip212Enforcement::GracePeriod,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was because the coinbase_outputs_are_decryptable_for_historical_blocks test in zebra-consensus pass only if this variable isin the grace period, however i think we need to compare with the height of the output to decide what goes here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good, they've changed this already, I thought this was in 0.15.0.

ZIP-212 enforcement can always be on here because we only call this function after Canopy activation. We should leave a comment about needing to check the height if Zebra's mandatory checkpoint height ever falls below the last block before Canopy activation (or we could just double check the height here anyways).

Copy link
Contributor Author

@oxarbitrage oxarbitrage May 17, 2024

Choose a reason for hiding this comment

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

If we change to sapling::note_encryption::Zip212Enforcement::On then the test will fail with:

Message:  coinbase outputs must be decryptable with an all-zero key: CoinbaseOutputsNotDecryptable
Location: zebra-consensus/src/transaction/tests.rs:2796

We might need to fix the test.

Copy link
Contributor

@arya2 arya2 May 17, 2024

Choose a reason for hiding this comment

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

So the test will pass, and so we don't have to update this code if Zebra's mandatory checkpoint height is ever lowered below the last block before Canopy activation, we could derive Ord/PartialOrd impls on NetworkUpgrade and do:

    let zip_212_enforcement = if network_upgrade >= NetworkUpgrade::Canopy {
        sapling::note_encryption::Zip212Enforcement::On
    } else {
        sapling::note_encryption::Zip212Enforcement::Off
    };

    if let Some(bundle) = alt_tx.sapling_bundle() {
        for output in bundle.shielded_outputs().iter() {
            let recovery = sapling::note_encryption::try_sapling_output_recovery(
                &null_sapling_ovk,
                output,
                zip_212_enforcement,
            );
            if recovery.is_none() {
                return false;
            }
        }
    }

It could also just get the Canopy activation height and check that it's past it.

Zebra skips the check in production because of the mandatory checkpoint height but does check historical blocks before Canopy in the test, so the test actually covers more than strictly necessary here.

This is the relevant section of the ZIP: https://zips.z.cash/zip-0212#consensus-rule-change-for-coinbase-transactions

Note: This seems to be wrong in zcashd, see zcash/zcash#6890. It may be a good idea to do a sync test from Canopy activation until the end of the grace period with checkpoint sync disabled to make sure that all of the blocks in the grace period follow the ZIP, though I think it was implemented correctly in zcashd prior to v5.9.0.

Update: Actually, there's no point in a sync test, the mandatory checkpoint height is still after the grace period. We can do a sync test with checkpoint sync disabled when we're lowering the mandatory checkpoint height.

zebra-chain/src/primitives/zcash_primitives.rs Outdated Show resolved Hide resolved
@mpguerra
Copy link
Contributor

@oxarbitrage should we close this one in favour of #8548 ?

@oxarbitrage
Copy link
Contributor Author

@oxarbitrage should we close this one in favour of #8548 ?

No, that suggestion PR was already merged into here and already in main. This one is different and needs to be merged with zebra-scan upgrades that are being worked on here and in some code i have not pushed yet.

@oxarbitrage
Copy link
Contributor Author

Closing as the commits from here were included into #8568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates do-not-merge Tells Mergify not to merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants