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

Provide Zebra compatibility: re-enable and fix zcash_client_backend, update prost dependency version #75

Merged
merged 16 commits into from
Oct 28, 2024

Conversation

dmidem
Copy link
Collaborator

@dmidem dmidem commented Oct 14, 2024

Updates the repository to ensure compatibility with the zebra project.

Changes:

  1. Uncommented the zcash_client_backend crate in the root Cargo.toml. Initially it was commented out under the assumption that it wasn't utilized by Zebra. However, it has been identified that Zebra relies on this crate.

  2. Synchronized zcash_client_backend crate with zcash_note_encryption updates.

  3. Updated the prost dependency version in the root Cargo.toml (the pinned version 0.12.3 caused compatibility issues with Zebra’s dependencies, preventing compilation).

@QED-it QED-it deleted a comment from what-the-diff bot Oct 14, 2024
Comment on lines +9 to 10
"zcash_client_backend",
# "zcash_client_sqlite",

Choose a reason for hiding this comment

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

Let's add a comment above line 9
# zcash_client_backend and zcash_client_sqlite are not compatible with OrchardZSA

Cargo.toml Outdated
@@ -95,7 +95,7 @@ maybe-rayon = { version = "0.1.0", default-features = false }
rayon = "1.5"

# Protobuf and gRPC
prost = "=0.12.3"
prost = "0.12.6"

Choose a reason for hiding this comment

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

maybe this is the source of CI fail?

Copy link
Collaborator Author

@dmidem dmidem Oct 16, 2024

Choose a reason for hiding this comment

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

It turns out that's not the case (or not the only case). I created a new branch from zsa1 and made only this single prost version change there. I committed it and created a new PR - the cargo vet CI check passed without any issues. This can even be verified without a PR: after I made that change in the repository, I ran cargo check, then git diff Cargo.lock: the Cargo.lock file wasn't changed.

However, when I uncommented zcash_client_backend in Cargo.toml and then ran cargo check, git diff Cargo.lock, it started to show a lot of differences like:

+[[package]]
+name = "anyhow"
+version = "1.0.89"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "86fdf8605db99b54d3cd748a44c6d04df638eb5dafb219b135d0149bd0db01f6"
...

This means that adding zcash_client_backend causes the cargo vet issues in CI that we observed.

I guess this happens because zcash_client_backend uses additional dependencies in its Cargo.toml that require newer versions of sub-dependencies like anyhow, which are not marked as audited for cargo vet in our git branch (the audited list is contained in the supply-chain folder of the repository).

I think the reason for this is that the maintainers of the original librustzcash repo maintain consistency between the Cargo.toml files of the crates, the Cargo.toml/Cargo.lock in the root, and the audited list in the supply-chain folder. But in our fork, we have copies of zcash_client_backend/Cargo.toml and the supply-chain folder from different versions of the original branches. I'm not sure though - we need to check the history of our commits to find the source of the difference.

Additionally, I tried a simple copying the supply-chain folder from the upstream version (https://github.com/zcash/librustzcash/tree/main/supply-chain), but it didn't resolve the issue. Maybe I missed something - I'm continuing to work on it.

Copy link
Collaborator Author

@dmidem dmidem Oct 22, 2024

Choose a reason for hiding this comment

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

I managed to solve the cargo vet issue. Here's what I did:

  1. Ran git log zcash_client_backend/Cargo.toml and found the latest commit message for it:
Zsa1 is updated to a more current upstream version, commit:
c3eace4982dfea82f89758f40c1db2e82a138e98
  1. Switched to commit c3eace4982dfea82f89758f40c1db2e82a138e98 (which corresponds to zcash/librustzcash#1383 in the original repo), took the Cargo.lock file from that commit and placed it into the zebra-compat-enable-backend branch.

  2. Ran cargo check, cargo test, cargo build, and also cargo vet --locked locally in zebra-compat-enable-backend branch to ensure everything works.

  3. Committed the changes — now all CI checks have passed.

Summary:

I managed to use the existing set of audited dependencies here, but had to use an older version of Cargo.lock (updated by cargo check to include our QED-it-specific dependency crates like orchard, etc.)

@@ -6,7 +6,8 @@ members = [
"components/zcash_encoding",
"components/zcash_protocol",
"components/zip321",
# "zcash_client_backend",
# zcash_client_backend and zcash_client_sqlite are not compatible with OrchardZSA
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is also commented in CI workflows. Specifically - workflows/ci.yml:L196-198

Shall we ucomment there too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense, thanks @alexeykoren, I just fixed it and commited the change.

Copy link
Collaborator

@alexeykoren alexeykoren left a comment

Choose a reason for hiding this comment

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

LGTM (except CI failure of course)

@dmidem dmidem merged commit 2684455 into zsa1 Oct 28, 2024
26 checks passed
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.

3 participants