CityOfZion / neon-js

Javascript libraries that allow the applications to interact with NEO blockchain
https://docs.coz.io/neo3/neon-js/index.html
MIT License
183 stars 166 forks source link

5.3.0 is much slower on key encryption/decryption than 4.8.0 #891

Closed roman-khimov closed 1 year ago

roman-khimov commented 1 year ago

Simple code like

            const acc = new wallet.Account();
            const wall = Neon.create.wallet({ name: 'some' });
            wall.addAccount(acc);
            wall.encrypt(0, password)

or

            const account = new wallet.Account(parsedWallet.accounts[0].key);
            account.decrypt(password)

is 100% portable between 4.8.0 (Legacy) and 5.3.0 (N3). And it effectively does the same NEP-2 crypto behind the scenes. But we've noticed that using 4.8.0 it takes ~2-3 seconds per operation, while using 5.3.0 it takes ~15s. And we're not sure why it happens this way. Scrypt parameters (that can influence) are the same in both versions.

CC @mike-petrov.

ixje commented 1 year ago

I can't seem to reproduce this. It takes about ~1.5s per snippet above. It's not blazing fast but nowhere near ~15s.

Screenshot 2023-04-12 at 08 57 36

How are you testing/measuring this?

ixje commented 1 year ago

For completeness sake (as speed can be machine dependent), this is using v4.8.0. Pretty much identical results Screenshot 2023-04-12 at 09 20 39

roman-khimov commented 1 year ago

Unfortunately, @mike-petrov still can't reply directly, so I'll answer. Yeah, your tests work the same for us, but we're trying to use it with React

"react": "^17.0.2",
"react-scripts": "^5.0.1",

like this:

import Neon, { wallet } from "@cityofzion/neon-js";

const account = new wallet.Account();
console.time('time');
account.encrypt('123').then(() => {
    console.timeEnd('time');
    console.log(account.export())
});

And what we get in this case is: 4.8.0 5.3.0

ixje commented 1 year ago

I still can't seem to reproduce https://codesandbox.io/s/determined-sammet-9y3510?file=/src/App.js Screenshot 2023-04-12 at 11 38 08

roman-khimov commented 1 year ago

Have you tried running it locally? I've tried https://codesandbox.io/s/hopeful-kowalevski-wu7oop?file=/src/App.js and it shows delta: 8200ms (and that's close to what I get with 4.8.0 on my old clunky i7-8565U), however if I'm to open the same app in a new window: изображение it's more like delta: 16076ms in the console.

For @mike-petrov using his shiny new M1 the difference is much worse, ~15s with 5.3.0 and ~2s with 4.8.0.

ixje commented 1 year ago

With the open in new window "trick" I also get a high delta.

The scrypt-js package version used in 4.8.0 is 3.0.0 vs 3.0.1 for 5.3.0. The only difference there is 3.0.1 adds type declarations.

I was about to go to bed so I'm not going to investigate further tonight, but it is an interesting case

ixje commented 1 year ago

I'm not sure what todo with this. This seems like an issue with scrypt-js + react that is exposed through neon-js. However, our desktop wallet is also react based, uses this part of neon-js and doesn't seem to suffer from this issue.

Note that I get the same slow results using scrypt-js 3.0.0 (that neon-js 4.8.0 uses as with 3.0.1) using the code below.

import logo from './logo.svg';
import './App.css';
import {Buffer} from 'buffer';
import { scrypt } from "scrypt-js";
function App() {
    const password = Buffer.from("AABB", "hex")
    const salt = Buffer.from("1122", "hex")
    const start = Date.now();
    scrypt(password, salt, 16384, 8, 8, 64).then(() => {
      const end = Date.now();
      console.log("delta: %dms", end - start);
    })

  return (
    <div className="App">
      <header className="App-header">
        <img src={logo} className="App-logo" alt="logo" />
        <p>
          Edit <code>src/App.js</code> and save to reload.
        </p>
        <a
          className="App-link"
          href="https://reactjs.org"
          target="_blank"
          rel="noopener noreferrer"
        >
          Learn React
        </a>
      </header>
    </div>
  );
}

export default App;

Maybe @snowypowers has an idea what could be up. Perhaps we can find a native implementation like this emscripten compiled one to mask the issue. I know too little about JS/TS or this library to know what requirements a native/compiled library must have to provide a compatible replacement. I consider myself just around 1st line support level on this project.

roman-khimov commented 1 year ago

At least we have this issue confirmed now, that's already something, we know it exists. And yeah, it's a strange one.

ixje commented 1 year ago

I feel like this has something todo with task scheduling on the event loop in react. If you replace the async scrypt call with syncScrypt then it is fast again

        syncScrypt(password, salt, 16384, 8, 8, 64);
        const end = Date.now();
        console.log("delta: %dms", end - start);

Looking a this performance profile over time seems to confirm it, there are huge gaps between these columns (which are the calls to the mix function, and in between these calls it gives control back to the event loop. Screenshot 2023-04-13 at 14 28 23

2104 * 5 ms = 10.5 seconds of waiting time

ixje commented 1 year ago

@roman-khimov -> https://www.npmjs.com/package/@cityofzion/neon-js/v/5.4.0