Move box/unbox message into Rust #1872
Conversation
| ff = "0.12.0" | ||
| group = "0.12.0" | ||
| jubjub = "0.9.0" | ||
| lazy_static = "1.4.0" | ||
| rand = "0.8.5" | ||
| rust-crypto-wasm = "0.3.1" # in favor of rust-crypto as this one is wasm friendly | ||
| tiny-bip39 = "1.0.0" | ||
| tiny-bip39 = "0.8" |
There was a problem hiding this comment.
We were on 0.8 until today when I merged the rust dep upgrades, but 1.0 has a version conflict with some library deeeeeeeep in crypto_box so rolling back for now.
|
I can review this once it's ready, but you might consider adding a test decrypting messages generated by tweetnacl. It'd be nice to have the reverse as well (checking that tweetnacl can decrypt messages encrypted by this version), but it'd probably require us to keep tweetnacl around as a dev dependency which is a bit of a pain. |
|
Since we're moving this into the main thread, can you convince me that this won't block the event loop now compared to it being off the main thread? Some performance numbers would suffice. What's your thinking here? |
Ah, I meant to include the perf screenshot that kicked this off. Derek mentioned that he was curious about box/unbox in rust, so I ran some tests. This is 25-30x faster, taking roughly 100 _micro_seconds, which is substantially less time than it even takes to serialize the data to the worker pool (as seen by comparing the direct node function call and the worker pool node call). The messages I used to test this were real webrtc messages grabbed from my live node. |
|
notes to self: speed comparison of "newKeyPair" in ms: NODE: 7.387 NODE: 0.67 NODE: 0.597 NODE: 0.606 NODE: 0.661 NODE: 1.397 NODE: 0.593 NODE: 0.594 NODE: 0.584 NODE: 0.593 |
Trying to store NAPI types causes really weird issues if you later pass that data back into rust functions. I suspect this is a bug in NAPI, but will have to investigate another time. Instead, using getters to convert the rust native types into the Javascript types seems to work well, without much overhead. Average getter speed in benchmarks was around 2 microseconds, so I think that is more than performant enough for now.
| } | ||
|
|
||
| impl Error for IronfishError {} | ||
|
|
There was a problem hiding this comment.
I think eventually we might think about putting our errors into one enum to make them a little easier to work with, similar to https://doc.rust-lang.org/std/io/enum.ErrorKind.html or something. I don't know exactly the best approach yet, but I think this works for now.
There was a problem hiding this comment.
Yeah, the current error handling setup is kind of inconvenient. Not too concerned with making a better one as part of this PR
| message = null | ||
| } | ||
|
|
||
| return { message } |
There was a problem hiding this comment.
This code is strange - previously it would just kind of silently fail. Now we have errors being thrown. I could make it fail silently like the previous approach, but that doesn't seem right. I'm a little too rust-brained at the moment to think of a better way to deal with this right now.
There was a problem hiding this comment.
I don't know that the previous approach was correct, but I think it's fine to maintain the behavior here and fix it in a different PR if necessary.
| @@ -0,0 +1,83 @@ | |||
| /* This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
I added a tests folder for this test file. It's just a compatibility test with tweetnacl, which has been moved to a dev dependency. I think it's probably fine to delete this file a couple weeks after this merges, and then we can remove the tweetnacl dependency completely.
Co-authored-by: Derek Guenther <derek@ironfish.network>
Summary
Replaces
tweetnaclwith napi bindings to a similar Rust API. Perf testing shows its 10-30x faster, with tasks generally taking <0.05ms, which is faster than it takes to send a job to the worker pool, so this should be an overall slight performance increase.Closes IRO-2448
Testing Plan
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.