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

build(deps): update ecc dependencies for zcashd 5.6.0, and create legacy state format compatibility layer #7053

Merged
merged 47 commits into from
Jul 17, 2023

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Jun 23, 2023

Motivation

Update ecc dependencies.

Close #6859

Depends-On: #7158

Solution

Create legacy code ion order to keep serialization the same as it was.

Review

@teor2345 and @conradoplg . There was already a lot of discussion about the options, solutions, etc.

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?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

@oxarbitrage oxarbitrage self-assigned this Jun 23, 2023
@oxarbitrage oxarbitrage added A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code do-not-merge Tells Mergify not to merge this PR A-cryptography Area: Cryptography related A-compatibility Area: Compatibility with other nodes or wallets, or standard rules P-Medium ⚡ labels Jun 23, 2023
@teor2345

This comment was marked as resolved.

@teor2345

This comment was marked as resolved.

@teor2345 teor2345 requested a review from dconnolly June 23, 2023 07:05
@oxarbitrage
Copy link
Contributor Author

I was able to make some progress by using some of the read/write functions from librustzcash::zcash_primitives::merke_tree @conradoplg pointed me too.

There are several things pending, some of them are:

  • There are no functions in the library to deal with sprout.
  • The library has read_frontier_v0 and read_frontier_v1 but only write_frontier_v1. It is not totally clear what to use n each case but it seems read_frontier_v0 should be used in sapling.
  • Checking the output of the following serialization tests:

Sapling:

$ cargo test sapling_note_commitment_tree_serialization -- --nocapture
    Finished test [unoptimized + debuginfo] target(s) in 3.17s
     Running unittests src/lib.rs (target/debug/deps/tower_batch_control-c2bd02070ca729ce)

...

running 1 test

The application panicked (crashed).
Message:  assertion failed: `(left == right)`
  left: `"0102000000000000007c3ea01a6e3a3d90cf59cd789e467044b5cd78eb2c84cc6816f960746d0e036c000162324ff2c329e99193a74d28a585a3c167a93bf41a255135529c913bd9b1e666ddaa1ab86de5c153993414f34ba97e9674c459dfadde112b89eeeafa0e5a204c"`,
 right: `"0102007c3ea01a6e3a3d90cf59cd789e467044b5cd78eb2c84cc6816f960746d0e036c0162324ff2c329e99193a74d28a585a3c167a93bf41a255135529c913bd9b1e66601ddaa1ab86de5c153993414f34ba97e9674c459dfadde112b89eeeafa0e5a204c"`
Location: zebra-state/src/service/finalized_state/tests/vectors.rs:41

...

Orchard:

$ cargo test orchard_note_commitment_tree_serialization -- --nocapture
...

     Running unittests src/lib.rs (target/debug/deps/zebra_state-a7a295af75b2f260)

running 1 test

The application panicked (crashed).
Message:  assertion failed: `(left == right)`
  left: `"010200000000000000ee9488053a30c596b43014105d3477e6f578c89240d1d1ee1743b77bb6adc40a0001a34b69a4e4d9ccf954d46e5da1004d361a5497f511aeb4d481d23c0be1778133a0be6dab19bc2c65d8299258c16e14d48ec4d4959568c6412aa85763c222a702"`,
 right: `"010200ee9488053a30c596b43014105d3477e6f578c89240d1d1ee1743b77bb6adc40a01a34b69a4e4d9ccf954d46e5da1004d361a5497f511aeb4d481d23c0be177813301a0be6dab19bc2c65d8299258c16e14d48ec4d4959568c6412aa85763c222a702"`
Location: zebra-state/src/service/finalized_state/tests/vectors.rs:90
...

Shows that the serialization is a bit longer. This seems to be because the Position is now a u64 field.

Will post more details as i discover them.

@teor2345 teor2345 removed the do-not-merge Tells Mergify not to merge this PR label Jul 3, 2023
@mpguerra

This comment was marked as resolved.

@teor2345 teor2345 requested a review from conradoplg July 5, 2023 20:16
@oxarbitrage oxarbitrage requested a review from a team as a code owner July 14, 2023 13:10
@oxarbitrage oxarbitrage requested review from dconnolly and removed request for a team July 14, 2023 13:10
@oxarbitrage oxarbitrage requested review from conradoplg and teor2345 and removed request for upbqdn and dconnolly July 14, 2023 18:26
conradoplg
conradoplg previously approved these changes Jul 14, 2023
Copy link
Contributor

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

deny.toml Show resolved Hide resolved
deny.toml Show resolved Hide resolved
deny.toml Show resolved Hide resolved
@mpguerra
Copy link
Contributor

Let's try to wrap up this PR so we can unblock the following work. If there is any other work that would be nice to have, let's create separate PRs for it

Copy link
Collaborator

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

I moved the remaining dependency updates to ticket #6638, which is now a release blocker

@teor2345 teor2345 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jul 17, 2023
@teor2345 teor2345 requested a review from conradoplg July 17, 2023 19:19
@teor2345 teor2345 changed the title build(deps): update ecc dependencies build(deps): update ecc dependencies for zcashd 5.6.0, and create legacy state format compatibility layer Jul 17, 2023
@teor2345 teor2345 removed the extra-reviews This PR needs at least 2 reviews to merge label Jul 17, 2023
mergify bot added a commit that referenced this pull request Jul 17, 2023
@mergify mergify bot merged commit e2f010e into main Jul 17, 2023
324 checks passed
@mergify mergify bot deleted the issue6859 branch July 17, 2023 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-cryptography Area: Cryptography related A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking: Upgrade shared ECC dependencies and zcash_script for zcashd 5.6.0
4 participants