Page MenuHomePhabricator

[SECP256K1] turn NativeSecp256k1 class into a regular class
Needs RevisionPublic

Authored by sken on Mar 17 2020, 03:18.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Restricted Owners Package(Owns No Changed Paths)
Summary

This also includes transferring the context call into NativeSecp256k1 so it is able to control its own creation, cloning, and destruction. Thus we wont need the c header file for Secp256k1Context
The only thing left is renaming Secp256k1Context.java file to something else. Maybe Secp256k1Loader.java, it is nice to have the System.loadlibrary call done separately from the secp256ki interface

Test Plan

execute all previous JNI tests
include tests for verfying multiple instances with different contexts

This can be done like this:

$ cd src/secp256k1
$ ./autogen.sh
$ ./configure --enable-jni --enable-experimental --enable-module-ecdh
$ make check-java

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D5490
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9913
Build 17685: Default Diff Build & Tests
Build 17684: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
deadalnix added inline comments.Mar 19 2020, 00:33
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
49 ↗(On Diff #16990)

The comment is useless.

60 ↗(On Diff #16990)

This seems like it should be static

70 ↗(On Diff #16990)

How can context be -1? This is never set to -1 anywhere. What if the creation fails? In which case, the context will be 0 (nullptr) and you'll get a segfault next time you use it.

Also, how is the context ever destroyed? This is leaking.

This revision now requires changes to proceed.Mar 19 2020, 00:33
sken marked 2 inline comments as done and an inline comment as not done.Mar 19 2020, 01:02

the context is set to -1 at "cleanup" (line 200~) when the context in the native lib is destroyed. We can set it up to -1 before assigning it at the constructor call. that way we can handle the case where the exception thrown at construction is caught for some dumb reason. Perhaps we should also check if the context is not -1 before every lib call. Theres also fixing cloneContext, which IMO should result in returning a new NativeSecp256k1 class with the cloned native lib context in, but i was hoping to fix that on another ticket

sken marked 3 inline comments as done.Mar 19 2020, 01:10
sken updated this revision to Diff 17024.Mar 19 2020, 01:25

make requested changes and also add the proposed ones

sken added a comment.Mar 19 2020, 01:28

arc diff just did somethign strange to that py file. is there a way to revert that?

sken updated this revision to Diff 17025.Mar 19 2020, 01:40

rolling back diff (hopefully)

sken updated this revision to Diff 17026.Mar 19 2020, 02:04

make requested changes and add proposed ones

Snippet of first build failure:

Build 'Bitcoin-ABC / Diffs / Diff Testing' #8665, branch 'phabricator/diff/17026'
Started 2020-03-19 02:05:07 on 'highperf2' by 'Phabricator Staging (phabricator-staging)'
Finished 2020-03-19 02:05:11 with status FAILURE 'Unable to collect changes'
VCS revisions: 'Bitcoin ABC Staging' (Git): N/A
TeamCity URL https://build.bitcoinabc.org/viewLog.html?buildId=31781&buildTypeId=BitcoinABC_BitcoinAbcStaging 
TeamCity server version is 2018.1.4 (build 58724), server timezone: UTC

[02:05:07]E: bt37 (4s)
[02:05:07]i: TeamCity server version is 2018.1.4 (build 58724)
[02:05:07] : The build is removed from the queue to be prepared for the start
[02:05:10] : Collecting changes in 1 VCS root (1s)
[02:05:10] :	 [Collecting changes in 1 VCS root] VCS Root details
[02:05:10] :		 [VCS Root details] "Bitcoin ABC Staging" {instance id=28, parent internal id=7, parent id=BitcoinABC_BitcoinAbcStaging, description: "ssh://vcs@reviews.bitcoinabc.org:2221/source/bitcoin-abc-staging.git#refs/heads/master"}
[02:05:11]i:	 [Collecting changes in 1 VCS root] Detecting changes in VCS root 'Bitcoin ABC Staging' (used in 'Bitcoin-ABC Gitian Diff Testing', 'Bitcoin-ABC Gitian Diff Testing' and 4 other configurations)
[02:05:11]i:	 [Collecting changes in 1 VCS root] Will collect changes for 'Bitcoin ABC Staging' starting from revision 8408a0cdcf4106fde3d093d4ed583b19fd7dbcdb
[02:05:11]E: Failed to collect changes, error: Builds in default branch are disabled in build configuration
sken updated this revision to Diff 17027.Mar 19 2020, 02:30

make requested changes and add proposed ones

sken updated this revision to Diff 17028.Mar 19 2020, 03:26

clean all resources in test

deadalnix requested changes to this revision.Mar 19 2020, 14:02

Theres also fixing cloneContext, which IMO should result in returning a new NativeSecp256k1 class with the cloned native lib context in, but i was hoping to fix that on another ticket.

I think you are 100% correct here. We can leave it for another patch as long a this is getting fixed, we are good.

Overall, it's getting there, but there is some polish to do. You might want to write something in the release notes.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
41 ↗(On Diff #17028)

Out of scope, but I don't think we need the lock to be reentrant.

44 ↗(On Diff #17028)

This guy should probably remain static.

57 ↗(On Diff #17028)

IMO a better pattern is something like

try {
    System.loadLibrary("secp256k1_jni");
    enabled = true;
} catch (UnsatisfiedLinkError e) {
    System.out.println("UnsatisfiedLinkError: " + e.toString());
}

So that enabled doesn't get set to true because there is an error condition we aren't aware of.

60 ↗(On Diff #17028)

isEnabled really isn't a great name.

64 ↗(On Diff #17028)

IllegalArgumentException is not really what you want here. There is no argument that is invalid. Same goes for getContext.

66 ↗(On Diff #17028)

This needs to throw if context == 0 (initialization failed).

69 ↗(On Diff #17028)

This probably shouldn't be a public method. The value returned is only meaningful if you holded a lock before fetching and for as long as you do hold the lock. If not, then it can be changed from under your feets.

220 ↗(On Diff #17028)

Shouldn't this be in the finally block before the unlock?

444 ↗(On Diff #17028)

Couldn't all of these be simplified as the sample code bellow?

synchronized(w) {
    return secp256k1_context_randomize(byteBuff, getContext()) == 1;
}

You don't need to do it in that specific patch, but it is worth thinking about.

This revision now requires changes to proceed.Mar 19 2020, 14:02
sken marked 4 inline comments as done.Mar 19 2020, 19:33

I responded to some of those comments and made the requested changes regardless. we can roll some of them back. as for isEnabled name im open to suggestions, i cant think of something more concise ;). Also, i added isValid to indicate the instance has a valid context. as you said, users dont need to know what context is, only if it works or it doesnt

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
41 ↗(On Diff #17028)

noted for a future patch?

44 ↗(On Diff #17028)

wouldn't it interfere if there is more than one instance of this class?

69 ↗(On Diff #17028)

You are right. internally we still want to check the context is valid. That said, a public method that can be useful is to check if an instance is valid. (i.e the context is not stale, and hasn't been cleaned up.) We don't want that to throw but just return a boolean

220 ↗(On Diff #17028)

if getContext or the destroy_context call fails. the instance will become invalid (context = -1) im not sure what happens on the c side. but if that seems fair then ill change it

444 ↗(On Diff #17028)

yes. i already have that on the works. other patch will come with the fix. its crazy the amount of repetition in this code.

sken updated this revision to Diff 17057.Mar 19 2020, 19:36
sken marked an inline comment as done.

Make requested changes

deadalnix requested changes to this revision.Mar 20 2020, 19:28

There is still no finalizer for that class unless I messed it, which means context will be leaked when things are garbage collected.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
58 ↗(On Diff #17057)

I'm not sure why you do not set the enabled variable directly. This seems like unnecessary complications.

69 ↗(On Diff #17057)

Considering checkState will throw, you don't need a temporary.

77 ↗(On Diff #17057)

If you are going to expose that fundction, do not duplicate the knowledge in it, just use it in getContext.

226 ↗(On Diff #17057)

The locking remain invalid here. I may end up reading a context that has been destroyed in another thread.

41 ↗(On Diff #17028)

Yes, it's fine not to do it immediately.

44 ↗(On Diff #17028)

The buffer seems to only be used to massage arguments when passing them to the binding, so no.

This revision now requires changes to proceed.Mar 20 2020, 19:28
sken marked 4 inline comments as done.Mar 20 2020, 22:51
sken added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
58 ↗(On Diff #17057)

the boolean is final and the code doesnt even compile like it if there is a possibility of it being unintialized

sken updated this revision to Diff 17082.Mar 20 2020, 22:52
sken marked an inline comment as done.

make requested changes and implement Autocloseable into the class

sken updated this revision to Diff 17083.Mar 20 2020, 22:54

Forgot to add this last minute change

deadalnix added inline comments.Mar 21 2020, 00:15
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
39 ↗(On Diff #17083)

Reading the spec, it doesn't looks like it really replaces finalize.

69 ↗(On Diff #17083)

throwing and catching this way is not really advised and make the control flow confusing.

79 ↗(On Diff #17083)
checkState(isValied());
89 ↗(On Diff #17083)

Just do the check here.

sken updated this revision to Diff 17089.Mar 21 2020, 02:30
sken marked 3 inline comments as done.

requested changes

sken added a comment.Mar 21 2020, 02:32

It doesn't replace finalize, but its the recommended way of handling resources. IMO, anyone with java experience would understand this, and treat this class as such. we can make it clear on the release notes and the comment

Also, i re added the initContext middle step. i realize that after assigning context to 0 and then throw, someone can catch it and then use the object with context set at 0. this way ensures that context remains -1;

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
39 ↗(On Diff #17083)

It doesn't, but its the recommended way of handling resources. IMO, anyone with java experience would understand this, and treat this class as such. we can make it clear on the release notes and the comment above

deadalnix requested changes to this revision.Mar 27 2020, 13:34

I'm sorry, but I don't buy the finalization argument, mostly because it's not an argument. I've seen enough cultish behavior in this industry to know people will say you shouldn't do X or must do Y the Z way with absolutely no reason whatsoever and it often turns out to be a bad idea. If there is a reason to not do it, then people should be able to state it and we should be able to assert why it applies or not to our current situation. Every time someone brings up that this is the "the way" without providing a proper rationale, this is only a stronger reason to dismiss this concern.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
50

This temporary is not useful either.

66

AutoClosable do not expect close to be idempotent, so you shouldn't check if the state is valid.

In addition, it doesn't do what you think it does due to lack of proper synchronization.

76

No, you can't resurrect the object even if you catch because this is the constructor. So the temporary is not useful.

This revision now requires changes to proceed.Mar 27 2020, 13:34