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
Details
Details
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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- arcpatch-D5490
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 9894 Build 17647: Default Diff Build & Tests Build 17646: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| 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. |
Comment Actions
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
Comment Actions
Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) failed.
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 configurationComment Actions
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. |
Comment Actions
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. |
Comment Actions
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 | I'm not sure why you do not set the enabled variable directly. This seems like unnecessary complications. | |
| 69 | Considering checkState will throw, you don't need a temporary. | |
| 77 | If you are going to expose that fundction, do not duplicate the knowledge in it, just use it in getContext. | |
| 226 | 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. |
| src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java | ||
|---|---|---|
| 58 | the boolean is final and the code doesnt even compile like it if there is a possibility of it being unintialized | |
| 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. |
Comment Actions
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 |
Comment Actions
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 ↗ | (On Diff #17089) | This temporary is not useful either. |
| 66 ↗ | (On Diff #17089) | 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 ↗ | (On Diff #17089) | No, you can't resurrect the object even if you catch because this is the constructor. So the temporary is not useful. |