-
Notifications
You must be signed in to change notification settings - Fork 716
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
[Libs] Update librustzcash #2870
Conversation
1bc92be Depends: Bump rust toolchain to v1.69.0 (Fuzzbawls) 5708543 Scripts: Bump minimum supported glibc to v2.27 (Fuzzbawls) f2cfadb GA: Run security-check on depends based builds (Fuzzbawls) 0a27b16 GA: Run symbol-check on depends based builds (Fuzzbawls) 02b7cb9 scripts: add PE dylib checking to symbol-check.py (fanquake) ee3bdc6 depends: build gmp with PIC on all platforms (Fuzzbawls) 85d1632 scripts: add MACHO dylib checking to symbol-check.py (fanquake) c2facf0 scripts: fix check-symbols & check-security argument passing (fanquake) 3af2b2f scripts: add MACHO NOUNDEFS check to security-check.py (fanquake) 83f86c3 scripts: add MACHO PIE check to security-check.py (fanquake) Pull request description: Alternative to #2871. This bumps our Rust Toolchain to v1.69.0, but comes at the cost of ALSO bumping our minimum supported linux glibc to 2.27, which officially drops support for Ubuntu 16.04, Debian 8 (Jessie), Debian 9 (Stretch), and CentOS 7 for binary releases. Ubuntu 16.04 and CentOS 7 technically don't reach EOL (End Of Life) until 2024, but both are largely unsupported in linux communities already. The major benefit to this outside of being able to use a more recent Rust toolchain version, is that it allows us to use the official Rust distribution toolchains and no longer maintain our own modified versions, a significant time saver if this ability is carried forward in the future. ACKs for top commit: Duddino: tACK 1bc92be, works fine with #2870 panleone: tACK 1bc92be Tree-SHA512: b59cf43045c189d6d68ca9770ef17f59baa4d5d4c854a9fb94afa8ac59a2fe576216926416689e6ad39161fccea51791552558da95d22850ba8b9af4c7abe015
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work! Spent most of the day testing the PR here the results:
- I managed to sync a new node from 0
- All type of transactions are successfully created and accepted by peers
So overall the code is working, I also started testing something in more details and I found a couple of bugs:
- The shared secret used to decrypt sapling notes is not generated properly (see the other comment for a possible fix)
- The ivk key is also not generated as it should be (but I still have to investigate on this bug)
Update: just figured out that the suggested fix solves both of the problems I reported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spent another day testing this PR:
nice job on removing orchard from the lib (it works flawless)
everything is retrocompatible with the current main, unit and functional test pass with the only 2 changes: (see the 2 other comments)
Doesn't compile here on github because CI is in offline mode and cannot fetch the lib from your github, but it should be easily fixable
src/rust/src/rustzcash.rs
Outdated
scalar | ||
.write_le(&mut result[..]) | ||
.expect("length is 32 bytes"); | ||
*&mut unsafe { *result } = scalar.to_bytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this line to:
unsafe{*result = scalar.to_bytes()};
or result is not updated
src/rust/src/rustzcash.rs
Outdated
.expect("should be able to serialize an ExtendedFullViewingKey"); | ||
|
||
*&mut unsafe { *xfvk_i } = bytes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this line to:
unsafe{*xfvk_i = bytes};
or xfvk_i is not updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 6074744
Codewise looks good, I have tested a fresh sync and sent shield tx's using 5.5.0 and one running this branch, no issues, no forking, shield data I was able to verify looked fine.
Staking was still successful, and normal functions were still fine as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 05d7993 let's get this merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the Cargo.lock
file was edited manually or by a diff patch instead of running cargo update
? This has resulted in including packages/crates that we won't be using...and are specifically avoiding. Even unused packages get downloaded/compiled, and in the case of depends builds, vendored.
This is what cargo update
shows for me if it is of any use:
Also of note: This renders the lib's internal unit tests inoperable, which are currently passing on master
2183984
to
1ff01e3
Compare
Co-authored-by: Fuzzbawls <fuzzbawls@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-tACK 60b6468 not passing tests are clearly not linked to the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 60b6468
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 60b6468
7203d91 Fix random fee generation (panleone) Pull request description: The librustzcash update (PR #2870) made shield txs slightly increase in size, this led the functional test feature_blockindexstats.py to fail when the (random) generated fee is too low. With this PR I simply increase the minimum generated fee in order to avoid the issue. ACKs for top commit: 7203d91 Liquid369: ACK 7203d91 Fuzzbawls: ACK 7203d91 Tree-SHA512: 2a30271dbe6a72b95aefc5a0fd6bc1a3a03710247a547842f56f9ebc6c3446439ff33d4cef6d909bd0d648e72dd80e2c36e5cb55287573232572fe5024b1aa3c
Updates librustzcash to version 0.11.0
I'm depending on a modified version, this is because some methods are not made public by upstream and to strip Orchard since its license is incompatible with MIT