Page MenuHomePhabricator

[secp256k1] add schnorr sign jni binding
ClosedPublic

Authored by floreslorca on Sat, Mar 30, 04:39.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC60e30bca748a: [secp256k1] add schnorr sign jni binding
Summary

add the jni binding for schnorr sign

Test Plan

run the single test but ill add more tests. any suggestions where i can take failing and positive tests is welcome

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

floreslorca created this revision.Sat, Mar 30, 04:39
Owners added a reviewer: Restricted Owners Package.Sat, Mar 30, 04:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Mar 30, 04:39
Herald added a subscriber: schancel. · View Herald Transcript
Fabien requested changes to this revision.Sat, Mar 30, 10:14
Fabien added a subscriber: Fabien.

The diff title is is about schnorr_verify but the content is about schnorr_sig.
Maybe you can look at the tests from libsecp256k1 to add the missing failing cases.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
427 ↗(On Diff #7874)

Is an ECDSA buffer appropriated for Schnorr signature ?

src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
370 ↗(On Diff #7874)

Why not intsarray[0] = secp256k1_schnorr_sign(ctx, sig, data, secKey, NULL, NULL); ?

This revision now requires changes to proceed.Sat, Mar 30, 10:14
floreslorca retitled this revision from [secp256k1] add schnorr verify jni binding to [secp256k1] add schnorr sign jni binding.Sat, Mar 30, 19:16
floreslorca edited the summary of this revision. (Show Details)
floreslorca updated this revision to Diff 7880.Sat, Mar 30, 21:41
floreslorca marked an inline comment as done.

make revision changes

jasonbcox requested changes to this revision.Tue, Apr 2, 01:27
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
435 ↗(On Diff #7880)

Just from looking at this diff, it looks like`byteBuff` and seckey are not being explicitly cleaned up before this function exits. Given that Java's GC is "at will" the secret key could be floating around in memory.

If this memory is not being cleaned up somewhere, we may have to zero them out and clear them before exiting. The exact behavior may be impacted by my other question below about memory leaking. If a new object is NOT created, then we need to know the consequences of altering that memory.

Note that this issue may also impact other ECDSA functions in this file as well.

427 ↗(On Diff #7874)

Changes to this will likely depend on D2756

src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
360 ↗(On Diff #7880)

I could only find the Java doc for this function: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/jni-14.html#GetDirectBufferAddress it seems to indicate that a new array is allocated. Does that mean this is leaking? I don't see data being cleaned up.

This revision now requires changes to proceed.Tue, Apr 2, 01:27
jasonbcox accepted this revision.Wed, Apr 10, 06:18

Accepted, but it will need to be rebased on master.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
435 ↗(On Diff #7880)

Created a task for this since it's not necessary to block this diff for it: https://reviews.bitcoinabc.org/T587

src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
360 ↗(On Diff #7880)

After more digging, @floreslorca pointed me to https://www.ibm.com/support/knowledgecenter/en/SSYKE2_8.0.0/com.ibm.java.vm.80.doc/docs/jni_refs.html which indicates that objects created by the JNI will be cleaned up when the native function returns to the calling JVM. Assuming this is right, there should be no leak here.

This revision now requires review to proceed.Wed, Apr 10, 06:18
Fabien accepted this revision.Mon, Apr 15, 18:11
Fabien added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java
321 ↗(On Diff #8061)

Nit: move the comment above

This revision is now accepted and ready to land.Mon, Apr 15, 18:11
floreslorca updated this revision to Diff 8062.Mon, Apr 15, 18:34

fix comment

This revision was landed with ongoing or failed builds.Mon, Apr 15, 18:35
This revision was automatically updated to reflect the committed changes.