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.

Sign up for GitHub

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

Jump to bottom

rust: Migrate to latest revision of zcash/librustzcash crates #6784

Merged
merged 1 commit into from
Apr 1, 2024
Merged

rust: Migrate to latest revision of zcash/librustzcash crates #6784

merged 1 commit into from
Apr 1, 2024

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Nov 8, 2023 •
edited by daira

fixes #6566, fixes #6806

@str4d str4d added A-dependencies Area: Dependencies L-rust Involves Rust code. labels Nov 8, 2023
@str4d str4d mentioned this pull request Dec 1, 2023
Merged
@daira daira mentioned this pull request Dec 19, 2023
Closed
@daira daira added this to the Post 5.8.0 milestone Dec 20, 2023
@daira daira mentioned this pull request Dec 20, 2023
Merged
@str4d str4d mentioned this pull request Jan 2, 2024
Merged
@str4d str4d modified the milestones: Post 5.8.0, Release 5.9.0 Feb 7, 2024
@str4d str4d force-pushed the rust-sapling-refactor branch 3 times, most recently from 223ecfd to 3002a92 Compare February 9, 2024 20:48
@str4d str4d added the safe-to-build Used to send PR to prod CI environment label Feb 9, 2024
@ECC-CI ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Feb 9, 2024
@daira daira added the safe-to-build Used to send PR to prod CI environment label Mar 15, 2024
@ECC-CI ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Mar 15, 2024
@str4d str4d force-pushed the rust-sapling-refactor branch 3 times, most recently from c153b42 to b0ac5c3 Compare March 18, 2024 19:44
@str4d str4d marked this pull request as ready for review March 18, 2024 19:44
ebfull
ebfull reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@ebfull ebfull left a comment •
edited

Choose a reason for hiding this comment

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

As part of this PR we probably should also modify contrib/debian/copyright to change orchard's license away from BOSL, remove the license exception in deny.toml, and remove the mention of BOSL in the COPYING file since it's no longer relevant. (Are there other remnants of BOSL in our codebase after this?)

Nevermind, we'll merge those changes separately in #6800 after this lands.

daira
daira reviewed Mar 21, 2024
View reviewed changes
src/rust/src/tests/signatures.rs
0xc1, 0xdb, 0xb1, 0x0c, 0xc4, 0xfb, 0x15, 0xab, 0x02, 0xac, 0xae, 0xf9, 0x44, 0xdd,
0xab, 0x8b, 0x67, 0x22, 0x54, 0x5f, 0xda, 0x4c, 0x62, 0x04, 0x6d, 0x69, 0xd9, 0x8f,
0x92, 0x2f, 0x4e, 0x8c, 0x21, 0x0b, 0xc4, 0x7b, 0x4f, 0xdd, 0xe0, 0xa1, 0x94, 0x71,
0x79, 0x80, 0x4c, 0x1a, 0xce, 0x56, 0x90, 0x05,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have these test vectors changed?

daira
daira reviewed Mar 21, 2024
View reviewed changes
src/rust/src/wallet.rs
@@ -150,7 +149,8 @@ pub struct Wallet {
nullifiers: BTreeMap<Nullifier, OutPoint>,
/// The incremental Merkle tree used to track note commitments and witnesses for notes
/// belonging to the wallet.
commitment_tree: BridgeTree<MerkleHashOrchard, u32, NOTE_COMMITMENT_TREE_DEPTH>,
// TODO: Replace this with an `orchard` crate constant (they happen to be the same).
commitment_tree: BridgeTree<MerkleHashOrchard, u32, { sapling::NOTE_COMMITMENT_TREE_DEPTH }>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
commitment_tree: BridgeTree<MerkleHashOrchard, u32, { sapling::NOTE_COMMITMENT_TREE_DEPTH }>,
commitment_tree: BridgeTree<MerkleHashOrchard, u32, 32>,

Temporarily, it is better to hard-code than to use another constant that happens to be the same (also change the comment and specify the type of the constant if necessary).

daira
daira reviewed Mar 21, 2024
View reviewed changes
src/rust/src/wallet.rs
frontier: *const bridgetree::Frontier<MerkleHashOrchard, NOTE_COMMITMENT_TREE_DEPTH>,
frontier: *const bridgetree::Frontier<
MerkleHashOrchard,
{ sapling::NOTE_COMMITMENT_TREE_DEPTH },
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

daira
daira reviewed Mar 21, 2024
View reviewed changes
src/test/coins_tests.cpp
@@ -306,9 +306,10 @@ class TxWithNullifiers

// The Orchard bundle builder always pads to two Actions, so we can just
// use an empty builder to create a dummy Orchard bundle.
// TODO: With the new BundleType::DEFAULT this is no longer true. Fix this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please file an issue.

daira
daira reviewed Mar 21, 2024
View reviewed changes
src/wallet/gtest/test_wallet.cpp
@@ -2046,21 +2049,21 @@ TEST(WalletTests, UpdatedSaplingNoteData) {
auto m = GetTestMasterSaplingSpendingKey();

// Generate dummy Sapling address
auto sk = m.Derive(0);
auto sk = m.Derive(0 | HARDENED_KEY_LIMIT);
Copy link
Contributor

Choose a reason for hiding this comment

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

This constant only happens to have the right value. (Same comment on other instances.)

daira
daira approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK with comments.

@ebfull ebfull added the safe-to-build Used to send PR to prod CI environment label Mar 25, 2024
@ebfull
Copy link
Contributor

ebfull commented Mar 25, 2024 •
edited

Rebased onto master so we can get CI running on this.

@ECC-CI ECC-CI removed the safe-to-build Used to send PR to prod CI environment label Mar 25, 2024
nuttycom
nuttycom approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK the changes since the approving reviews

@ebfull ebfull merged commit f52c007 into zcash:master Apr 1, 2024
73 of 82 checks passed
@str4d str4d deleted the rust-sapling-refactor branch April 1, 2024 22:40
@daira daira mentioned this pull request Apr 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuttycom nuttycom nuttycom approved these changes

@daira daira daira approved these changes

@ebfull ebfull ebfull approved these changes

Assignees

@str4d str4d

Labels
A-dependencies Area: Dependencies L-rust Involves Rust code.
Projects
None yet
Milestone
Release 5.9.0
5 participants
@str4d @ebfull @nuttycom @daira @ECC-CI