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

Update multiple crates to ensure bitvec 0.22.3 is being used #2351

Merged
merged 4 commits into from
Jun 23, 2021

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Jun 18, 2021

Motivation

librustzcash was updated to use bitvec 0.22 and due to a package conflict with funty, we need to update all crates that use bitvec in order to use the recent librustzcash (in particular its zcash_primitives crate which will be used by the ZIP-244 PRs).

This will also solve issues with the updated zcash_script.

Solution

Update all crates that use bitvec to versions that use bitvec 0.22.

This required fixing some of Zebra code that directly uses bitvec since its API changed since 0.17 which was being used before. I added a lot of expects because bitvec now can return an error when instantiating from slices; but per its documentation that can only happen if the slice is too big, and all of our usages are with fixed-size small slices.

Note: I've included the zcash_primitives reference in Cargo.toml but it's not being actively used right now, but it will be used soon with the ZIP-244 PRs; I wanted to make sure everything works with it.

The jubjub upgrade also resolves #1798

Review

This blocks the ZIP-244 PRs (#2129 and #2165).
(Spoiler: I've tested this code with #2129 and it works 🎉 )

@jvff might want to review since he had to make pretty much the same changes while updating zcash_script
@dconnolly might want to review the bitvec usage changes to make sure I haven't accidentally changed the behavior

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

Update redjubjub when it's released: #2352
Update librustzcash when it's released: #2353

@dconnolly dconnolly added A-consensus Area: Consensus rule updates A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks labels Jun 18, 2021
@dconnolly dconnolly added this to the 2021 Sprint 12 milestone Jun 18, 2021
@dconnolly dconnolly self-requested a review June 18, 2021 22:36
dconnolly
dconnolly previously approved these changes Jun 18, 2021
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

This is great!

I want to find a way to document that as of these versions of jubjub and redjubjub, the ZIP-216 consensus rules are being checked. We could annotate this at each location in the source, but I don't know if that's best. Maybe @teor2345 has opinions

jvff
jvff previously requested changes Jun 19, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

The changes are looking good! I added some suggestions, but they're on the edge of being out of the PR's scope, so feel free to ignore them or leave them for later.

There's only one thing that is worth taking a closer look: accessing 10 bits from a BitSlice that seems to only have 8 bits. To be fair, this is likely also out of the PR's scope, but since it might be an existing bug, it might be a good idea for someone else to double check.

zebra-chain/src/orchard/commitment.rs Outdated Show resolved Hide resolved
zebra-chain/src/orchard/keys.rs Outdated Show resolved Hide resolved
zebra-chain/src/orchard/tree.rs Outdated Show resolved Hide resolved
zebra-chain/src/orchard/tree.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/commitment.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/tree.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

I want to find a way to document that as of these versions of jubjub and redjubjub, the ZIP-216 consensus rules are being checked. We could annotate this at each location in the source, but I don't know if that's best. Maybe @teor2345 has opinions

This is a hard one, since there's no obvious place to put it.

Can we just quote the relevant parts of ZIP-216 in the transaction verifier module doc comment?

Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>
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.

I'm ok with this PR, but I'd like @dconnolly to check the out-of-range bit read fix.

@dconnolly
Copy link
Contributor

I want to find a way to document that as of these versions of jubjub and redjubjub, the ZIP-216 consensus rules are being checked. We could annotate this at each location in the source, but I don't know if that's best. Maybe @teor2345 has opinions

This is a hard one, since there's no obvious place to put it.

Can we just quote the relevant parts of ZIP-216 in the transaction verifier module doc comment?

Done in ab14a0a

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.

I think everything got resolved and fixed.

I'm keen to merge this soon to avoid conflicts.

@teor2345 teor2345 dismissed jvff’s stale review June 23, 2021 03:16

All concerns have been addressed and bugs have been fixed.

@teor2345 teor2345 merged commit 9688811 into main Jun 23, 2021
@teor2345 teor2345 deleted the update-bitvec branch June 23, 2021 03:16
s.extend_from_bitslice(
&BitSlice::<Lsb0, _>::from_slice(&right).expect("must work for small slices")[0..255],
);
s.extend_from_bitslice(&BitArray::<Lsb0, _>::from([layer, 0])[0..10]);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ZIP-216: Require Canonical Point Encodings
4 participants