JNI bugfixes #1687

str4d · 2025-02-19T03:56:44Z

This fixes multiple JNI type signature inconsistencies, and cleans up argument handling on the Rust side to be less error-prone.

fixes #1657, fixes #1684

Note
This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.

Author

Self-review your own code in GitHub's web interface¹
Add automated tests as appropriate
Update the manual tests² as appropriate
Check the code coverage³ report for the automated tests
Update documentation as appropriate (e.g README.md, Architecture.md, etc.)
Run the demo app and try the changes
Pull in the latest changes from the main branch and squash your commits before assigning a reviewer⁴

Reviewer

Check the code with the Code Review Guidelines checklist
Perform an ad hoc review⁵
Review the automated tests
Review the manual tests
Review the documentation, README.md, Architecture.md, etc. as appropriate
Run the demo app and try the changes⁶

Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs. ↩
While we aim for automated testing of the SDK, some aspects require manual testing. If you had to manually test
something during development of this pull request, write those steps down. ↩
While we are not looking for perfect coverage, the tool can point out potential cases that have been missed. Code coverage can be generated with: ./gradlew check for Kotlin modules and ./gradlew connectedCheck -PIS_ANDROID_INSTRUMENTATION_TEST_COVERAGE_ENABLED=true for Android modules. ↩
Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit. ↩
In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass. ↩
While the CI server runs the demo app to look for build failures or crashes, humans running the demo app are
more likely to notice unexpected log messages, UI inconsistencies, or bad output data. Perform this step last, after verifying the code changes are safe to run locally. ↩

Fixes #1684.

Fixes #1657.

Unlike a C FFI where we need some mechanism to signal an error state, with JNI thrown exceptions trigger as soon as the JNI function returns. These functions had no declared return value on the Kotlin side and thus were not using the returned boolean in any way; this change fixes the type signatures to match.

backend-lib/src/main/rust/lib.rs

daira · 2025-02-19T12:07:54Z

backend-lib/src/main/rust/lib.rs

@@ -1809,7 +1802,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeTr
        let network = parse_network(network_id as u32)?;


On line 1801 the wrong method name is used in the info_span!.

daira · 2025-02-19T12:09:40Z

backend-lib/src/main/rust/lib.rs

+        let memo = utils::java_nullable_bytes_to_rust(env, &memo)?
+            .as_deref()
+            .map(MemoBytes::from_bytes)
+            .transpose()
+            .map_err(|e| anyhow!("Invalid MemoBytes: {}", e))?;


This pattern occurs twice (also in RustBackend.proposeShielding); consider adding a nullable_memo_bytes_from_jni helper.

backend-lib/src/main/rust/lib.rs

daira · 2025-02-19T12:23:37Z

backend-lib/src/main/rust/lib.rs

+                let seed_fingerprint =
+                    utils::java_nullable_bytes_to_rust(env, &seed_fingerprint_bytes)?
+                        .and_then(|b| b.as_slice().try_into().ok())
+                        .map(SeedFingerprint::from_bytes);


Although this happens to only be used once, it's doing the same kind of thing as the other conversion functions, so I might be inclined to add a nullable_seed_fingerprint_from_jni helper anyway.

backend-lib/src/main/rust/lib.rs

daira · 2025-02-19T12:48:16Z

backend-lib/src/main/rust/lib.rs

+        let addr = utils::java_string_to_rust(env, &addr)?;

        match Address::decode(&network, &addr) {
            Some(addr) => match addr {


Consider this helper:

fn parse_address(env: &JNIEnv, network_id: jint, addr: JString) -> anyhow::Result<ZcashAddress> { let network = parse_network(network_id as u32)?; let addr = utils::java_string_to_rust(env, &addr)?; Address::decode(&network, &addr).ok_or(anyhow!("Address is for the wrong network")) }

Then lines 830-839 become just:

match parse_address(network_id, addr)? { Address::Sapling(_) => Ok(JNI_TRUE), Address::Transparent(_) | Address::Unified(_) | Address::Tex(_) => Ok(JNI_FALSE), }

This can also be used in isValid{Transparent,Unified,Tex}Address, saving 15 non-blank lines overall I think.

backend-lib/src/main/rust/lib.rs

daira · 2025-02-19T13:19:10Z

backend-lib/src/main/rust/lib.rs

@@ -2036,12 +2025,12 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_createPro
        let network = parse_network(network_id as u32)?;


On line 2024 the wrong method name is used in the info_span!.

I checked all other uses of info_span! and made comments for any inconsistencies.

daira · 2025-02-19T13:22:16Z

backend-lib/src/main/rust/lib.rs

@@ -2169,8 +2157,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_requiresS
    let res = catch_unwind(&mut env, |env| {
        let _span = tracing::info_span!("RustBackend.pcztRequiresSaplingProofs").entered();


This is the name used in Kotlin at the wrapper layer, but at the JNI layer it is called requiresSaplingProofs. In other cases the name is the same. Rename the JNI method to pcztRequiresSaplingProofs.

We cannot rename the JNI method, because the arguments between the extern function and the Backend trait method are identical. You'll see I use different names for several such methods, whereas when the Backend trait method has fewer arguments (e.g. due to us inserting a db_data path for the JNI) we use the same method name.

It might be better to systematically use a different name (e.g. add an ffi prefix), rather than sometimes use the same name and sometimes a different one.

daira · 2025-02-19T13:36:45Z

backend-lib/src/main/rust/lib.rs

@@ -2169,8 +2157,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_requiresS
    let res = catch_unwind(&mut env, |env| {
        let _span = tracing::info_span!("RustBackend.pcztRequiresSaplingProofs").entered();

-        let pczt = Pczt::parse(&env.convert_byte_array(pczt)?[..])
-            .map_err(|e| anyhow!("Invalid PCZT: {:?}", e))?;
+        let pczt = parse_pczt(env, &pczt)?;

        let prover = Prover::new(pczt);



Aside (just below): I'm perplexed that there is no From<bool> for jboolean. Doesn't matter I guess; this pattern only occurs once. Filed jni-rs/jni-rs#564

backend-lib/src/main/rust/lib.rs

daira · 2025-02-19T14:45:44Z

backend-lib/src/main/rust/lib.rs

@@ -1050,7 +1050,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_writeBloc
    _: JClass<'local>,
    db_cache: JString<'local>,
    block_meta: JObjectArray<'local>,
-) -> jboolean {


The same argument applies to initBlockMetaDb: the return value is always discarded.

daira

utACK with non-blocking suggestions. The errors in info_span! strings should be addressed but it would be fine to do so in another PR.

Note that, as pointed out in #1688, there are easily fixable clippy lint failures. Also needs ktlint --format.

backend-lib/src/main/rust/lib.rs

daira · 2025-02-19T15:15:52Z

backend-lib/src/main/rust/utils.rs

-    env.get_string(jstring)
-        .expect("Couldn't get Java string!")
-        .into()
+pub(crate) fn java_bytes_to_rust(env: &JNIEnv, jbytes: &JByteArray) -> anyhow::Result<Vec<u8>> {


This could be jbytes: JByteArray since it is effectively already a pointer (see the comment on parse_secret).

Leaving as-is because the method it wraps takes a reference, and this method doesn't do anything special on top (the type-specific helper methods do by comparison; they effectively are converting type, so we want to move there).

daira · 2025-02-19T15:16:46Z

backend-lib/src/main/rust/utils.rs

-    (!jstring.is_null()).then(|| java_string_to_rust(env, jstring))
+pub(crate) fn java_nullable_bytes_to_rust(
+    env: &JNIEnv,
+    jbytes: &JByteArray,


This could be jbytes: JByteArray since it is effectively already a pointer (see the comment on parse_secret).

daira · 2025-02-19T15:17:04Z

backend-lib/src/main/rust/utils.rs

+        .transpose()
+}
+
+pub(crate) fn java_string_to_rust(env: &mut JNIEnv, jstring: &JString) -> anyhow::Result<String> {


This could be jstring: JString since it is effectively already a pointer (see the comment on parse_secret).

daira · 2025-02-19T15:17:32Z

backend-lib/src/main/rust/utils.rs

+
+pub(crate) fn java_nullable_string_to_rust(
+    env: &mut JNIEnv,
+    jstring: &JString,


This could be jstring: JString since it is effectively already a pointer (see the comment on parse_secret).

nuttycom

utACK c78afc7

nuttycom

utACK 5529922 with suggestion

nuttycom · 2025-02-21T22:26:27Z

backend-lib/src/main/rust/lib.rs

+impl zcash_address::TryFromRawAddress for UnifiedAddressParser {
+    type Error = anyhow::Error;
+
+    fn try_from_raw_unified(
+        data: zcash_address::unified::Address,
+    ) -> Result<Self, zcash_address::ConversionError<Self::Error>> {
+        data.try_into()
+            .map(UnifiedAddressParser)
+            .map_err(|e| anyhow!("Invalid Unified Address: {}", e).into())
+    }
+}


Why not just use https://github.com/zcash/librustzcash/blob/main/zcash_keys/src/address.rs#L28?

It's really weird to me to have to go through TryFromRawAddress for these straightforward operations.

(I mean, why not use it directly?)

I am using that; it's the data.try_into() call. The rest of the TryFromRawAddress machinery here is ensuring that if someone passes a string that isn't a UA (and is instead some other kind of address, or not an address at all), we get correct errors.

Ah. So, looking at the call sites of parse_ua, what is the reason to restrict these to unified addresses? If we parsed an arbitrary ZcashAddress at these call sites, we could still extract the transparent and/or Sapling receivers if present.

That might be worth considering, but that's outside the scope of this PR.

str4d added 5 commits February 19, 2025 15:46

Reorder JNI functions so they are in the same order in Kotlin and Rust

64f016f

rust: Fix ZIP 32 account index type on Rust side of JNI

2986809

Fixes #1684.

rust: Fix height type in RustBackend.putUtxo JNI function

d3b38f4

Fixes #1657.

rust: Clean up argument parsing

e3957f1

Milan-Cerovsky approved these changes Feb 19, 2025

View reviewed changes

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

daira mentioned this pull request Feb 19, 2025

Run clippy on backend-lib in CI #1688

Open

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

daira approved these changes Feb 19, 2025

View reviewed changes

daira reviewed Feb 19, 2025

View reviewed changes

backend-lib/src/main/rust/lib.rs Outdated Show resolved Hide resolved

daira reviewed Feb 19, 2025

View reviewed changes

Milan-Cerovsky mentioned this pull request Feb 21, 2025

Shielding transaction data handling update Electric-Coin-Company/zashi-android#1792

Merged

13 tasks

nuttycom approved these changes Feb 21, 2025

View reviewed changes

rust: Further JNI cleanups per review comments

5529922

nuttycom approved these changes Feb 21, 2025

View reviewed changes

str4d merged commit 7a8eb9c into rust-updates Feb 21, 2025
10 of 12 checks passed

str4d deleted the jni-bugfixes branch February 21, 2025 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JNI bugfixes #1687

JNI bugfixes #1687

str4d commented Feb 19, 2025 •

edited by daira

Loading

daira Feb 19, 2025

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025

daira Feb 19, 2025

daira Feb 19, 2025 •

edited

Loading

str4d Feb 21, 2025

daira Feb 22, 2025

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025

daira left a comment •

edited

Loading

daira Feb 19, 2025 •

edited

Loading

str4d Feb 21, 2025

daira Feb 19, 2025

daira Feb 19, 2025

daira Feb 19, 2025

nuttycom left a comment

nuttycom left a comment

nuttycom Feb 21, 2025

nuttycom Feb 21, 2025

str4d Feb 21, 2025

nuttycom Feb 21, 2025

str4d Feb 21, 2025

		@@ -1809,7 +1802,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeTr
		let network = parse_network(network_id as u32)?;

		@@ -2036,12 +2025,12 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_createPro
		let network = parse_network(network_id as u32)?;

		@@ -2169,8 +2157,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_requiresS
		let res = catch_unwind(&mut env, \|env\| {
		let _span = tracing::info_span!("RustBackend.pcztRequiresSaplingProofs").entered();

JNI bugfixes #1687

JNI bugfixes #1687

Conversation

str4d commented Feb 19, 2025 • edited by daira Loading

Author

Reviewer

Footnotes

Choose a reason for hiding this comment

daira Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

daira Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

daira Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment • edited Loading

Choose a reason for hiding this comment

daira Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Feb 19, 2025 •

edited by daira

Loading

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025 •

edited

Loading

daira Feb 19, 2025 •

edited

Loading

daira left a comment •

edited

Loading

daira Feb 19, 2025 •

edited

Loading