CCTP lib CommitmentTree integration #45

i-Alex · 2021-04-16T10:02:13Z

No description provided.

…ependency temporary set.

…ficial/zendoo-sc-cryptolib into commitment_jni_tweedle

…rialization.

…ts resolved.

nastenko

In general looks fine; Most of the comments are about possible refactoring for code size optimization and minor fixes in tests

nastenko · 2021-04-20T16:05:48Z

jni/src/test/java/com/horizen/commitmenttree/CommitmentTreeTest.java

+        Optional<ScAbsenceProof> absenceOpt = commTree.getScAbsenceProof(scId[0]);
+        // TODO Uncomment this code when rust library will be able to operate with absence proof on empty commitment tree.
+        assertTrue("Absence proof expected to be present.", absenceOpt.isPresent());
+        assertTrue("Absence verification expected to be successful", commTree.verifyScAbsence(scId[0], absenceOpt.get() ,commitmentOpt.get()));


commTree.verifyScAbsence - static member is accessed via instance reference; can be replaced with CommitmentTree.verifyScAbsence

nastenko · 2021-04-20T16:06:51Z

jni/src/main/java/com/horizen/commitmenttree/CommitmentTree.java

+
+
+    private native Optional<FieldElement[]>  nativeGetCrtLeaves(byte[] scId);
+    public Optional<List<FieldElement>> getCrtLeaves(byte[] scId) {


Looks like this method is not covered with tests in CommitmentTreeTest

nastenko · 2021-04-20T16:07:59Z

jni/src/test/java/com/horizen/commitmenttree/CommitmentTreeTest.java

+        absenceOpt = commTree.getScAbsenceProof(scId[0]);
+        commitmentOpt = commTree.getCommitment();
+        assertTrue("Absence proof expected to be present.", absenceOpt.isPresent());
+        assertTrue("Absence verification expected to be successful", commTree.verifyScAbsence(scId[0], absenceOpt.get() ,commitmentOpt.get()));


static member is accessed via instance reference

nastenko · 2021-04-20T16:08:07Z

jni/src/test/java/com/horizen/commitmenttree/CommitmentTreeTest.java

+        absenceOpt = commTree.getScAbsenceProof(scId[2]);
+        commitmentOpt = commTree.getCommitment();
+        assertTrue("Absence proof expected to be present.", absenceOpt.isPresent());
+        assertTrue("Absence verification expected to be successful", commTree.verifyScAbsence(scId[2], absenceOpt.get() ,commitmentOpt.get()));


static member is accessed via instance reference

nastenko · 2021-04-20T16:08:13Z

jni/src/test/java/com/horizen/commitmenttree/CommitmentTreeTest.java

+        absenceOpt = commTree.getScAbsenceProof(scId[4]);
+        commitmentOpt = commTree.getCommitment();
+        assertTrue("Absence proof expected to be present.", absenceOpt.isPresent());
+        assertTrue("Absence verification expected to be successful", commTree.verifyScAbsence(scId[4], absenceOpt.get() ,commitmentOpt.get()));


static member is accessed via instance reference

nastenko · 2021-04-20T16:08:37Z

jni/src/test/java/com/horizen/commitmenttree/CommitmentTreeTest.java

+
+        assertTrue("Deserialized absence proof should be serialized to same bytes", Arrays.equals(deserializedAbsenceProof.serialize(), absenceProofBytes));
+        assertTrue("Absence verification of original proof expected to be successful", commTree.verifyScAbsence(scId[1], absenceOpt.get() ,commitmentOpt.get()));
+        assertTrue("Absence verification of deserialized proof expected to be successful", commTree.verifyScAbsence(scId[1], deserializedAbsenceProof ,commitmentOpt.get()));


static member is accessed via instance reference

Cargo.toml

nastenko · 2021-04-20T16:24:03Z

api/src/lib.rs

+) -> jboolean
+{
+    let sc_id = {
+        let t = _env.convert_byte_array(_sc_id)


this part of code (lines 2649 - 2657) actually is duplicated many times along the further code, so it could be implemented as an utility method like:
get_byte_array(_env: &JNIEnv, javaByteArray: &jbyteArray, length: usize) -> [u8]

nastenko · 2021-04-20T16:34:19Z

api/src/lib.rs

+
+    let commitment_tree = {
+
+        let t =_env.get_field(_commitment_tree, "commitmentTreePointer", "J")


to avoid duplications of such a part of the code the method like this could be useful:
https://github.com/nastenko/zendoo-sc-cryptolib/blob/76e533cd3ff2de91871f354f599633140644c03e/api/src/zenbox_smt/jni_calls.rs#L31

nastenko · 2021-04-20T16:45:07Z

jni/src/main/java/com/horizen/sigproofnative/NaiveThresholdSigProof.java

@@ -40,6 +40,7 @@ private static native boolean nativeVerifyProof(BackwardTransfer[] btList,
                                      byte[] endEpochBlockHash, byte[] prevEndEpochBlockHash,
                                      FieldElement constant, long quality, byte[] proof, boolean checkProof, String verificationKeyPath, boolean checkVk);

+    // TODO: check type of `constant`. Why not a byte[]?


FieldElement can be passed to the Rust-side without a need for intermediate serialization into bytes

I would propose to use byte array representations everywhere.
All the objects like constant, certDataHash, customFields and others on the SDK side are kept and stored as a pure java byte arrays. And, for example, when we need to use constant to verify snark proof we need to deserialize it to Rust FieldElement object first, then pass it to verify the proof, then free the object to avoid memory leaks.
Such an approach costs a lot, because we emit 2 JNI calls: FE deserialization, proof verification.
Instead we may use only 1 JNI call by passing constant as is and deserializing it inside nativeVerifyProof method.

The main goal is to decrease number of JNI calls as much as possible. Here it's just a proposal for the further discussions.

api/src/lib.rs

jni/src/test/java/com/horizen/commitmenttree/CommitmentTreeTest.java

…lved.

… branch code modifications.

i-Alex · 2021-05-14T15:06:00Z

This PR is substituted with #49

i-Alex and others added 25 commits March 18, 2021 11:26

CommitmentTree JNI draft.

3f4eb61

Added implementation of existance proof and absence proof.

9814528

Fixed existence and absence build errors.

74b24f7

Merge updatable_poseidon_tweedle into commitment_jni_tweedle.

330529f

Merge branch 'commitment_jni' into commitment_jni_tweedle

4bd428e

CCTP lib dependency updated. CommitmentTree fixes. Local ginger lib d…

f4ba7f5

…ependency temporary set.

CommitmentTree native jni fixes.

dd6b5dc

CommitmentTree implementation updated.

7d8841f

CommitmentTree init method fix.

a04d171

Added unit tests for rust commitment tree.

9c62f0e

Minor renaming.

3106c66

Merge branch 'updatable_poseidon_tweedle' into commitment_jni_tweedle

0654dcc

CommitmentTree fixes.

d28646f

CommitmentTree jni fixes.

aa5c02b

CommitmentTree jni checks improvement. Unit tests update.

79f900d

Fix merging issues.

8ddd9d4

Merge branch 'commitment_jni_tweedle' of https://github.com/HorizenOf…

2a0ab54

…ficial/zendoo-sc-cryptolib into commitment_jni_tweedle

Slit unit test in separate ones.

d8d4d3e

Fixed existance and absence proof implementation. Updated unit tests.

c9ac515

Added wrappers for existance and absence proof serialization and dese…

81ec068

…rialization.

Java empty comm tree regression test added.

1a5d5a9

Added existence/absence proofs serialization.

00921cb

Fixed build script.

fe0e971

Merge updateble_poseidon_tweedle into commitment_jni_tweedle. Conflic…

874b6df

…ts resolved.

cctp lib dependency updated.

fff4143

i-Alex requested review from nastenko and VolodymyrBabaiev April 16, 2021 10:02

i-Alex assigned i-Alex and VolodymyrBabaiev Apr 16, 2021

nastenko reviewed Apr 20, 2021

View reviewed changes

Fixing issues from codereview.

1a2a108

i-Alex commented Apr 28, 2021

View reviewed changes

api/src/lib.rs Outdated Show resolved Hide resolved

jni/src/test/java/com/horizen/commitmenttree/CommitmentTreeTest.java Outdated Show resolved Hide resolved

Fixed getCertLeaves function. Updated test for getCertLeaves.

3aca007

i-Alex changed the base branch from updatable_poseidon_tweedle to development_tmp May 4, 2021 06:51

i-Alex added 2 commits May 4, 2021 17:22

Merge refactor_interfaces into commitment_jni_tweedle. Conflicts reso…

43ebba7

…lved.

Commitment tree interface changes to support cctp refactor_interfaces…

58ee742

… branch code modifications.

i-Alex marked this pull request as ready for review May 6, 2021 07:19

i-Alex added 2 commits May 6, 2021 14:21

CCTP interface updated.

3fc00d3

CommitmentTree interface updated according to CCTP lib changes.

59785f0

i-Alex requested a review from nastenko May 7, 2021 12:47

nastenko approved these changes May 14, 2021

View reviewed changes

i-Alex mentioned this pull request May 14, 2021

CCTP Commitment Tree #49

Merged

albertog78 merged commit 5e918e8 into development_tmp Jun 3, 2021

DanieleDiBenedetto deleted the commitment_jni_tweedle branch March 23, 2023 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCTP lib CommitmentTree integration #45

CCTP lib CommitmentTree integration #45

i-Alex commented Apr 16, 2021

nastenko left a comment

nastenko Apr 20, 2021

nastenko Apr 20, 2021

VolodymyrBabaiev Apr 27, 2021

nastenko Apr 20, 2021

nastenko Apr 20, 2021

nastenko Apr 20, 2021

nastenko Apr 20, 2021

nastenko Apr 20, 2021

nastenko Apr 20, 2021

nastenko Apr 20, 2021

i-Alex Apr 27, 2021

i-Alex commented May 14, 2021



		private native Optional<FieldElement[]> nativeGetCrtLeaves(byte[] scId);
		public Optional<List<FieldElement>> getCrtLeaves(byte[] scId) {


		let commitment_tree = {

		let t =_env.get_field(_commitment_tree, "commitmentTreePointer", "J")

CCTP lib CommitmentTree integration #45

CCTP lib CommitmentTree integration #45

Conversation

i-Alex commented Apr 16, 2021

nastenko 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

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

i-Alex commented May 14, 2021