storj-archived / core

Deprecated. Implementation of the Storj v2 protocol for Node.js.
https://storj.io
Other
396 stars 88 forks source link

use secp256k1 to sign messages #761

Closed littleskunk closed 6 years ago

braydonf commented 6 years ago

Nice! Is that all that's necessary?

Could be worth adding a benchmark in bench/index.js to see the performance improvement.

littleskunk commented 6 years ago

It is only one part of the problem. I try to change this line as well but I have problems to generate a compact signature. https://github.com/littleskunk/core/blob/a14e30fbe27aa1da6b38e8e656e47321a3480645/lib/crypto-tools/keypair.js#L85

littleskunk commented 6 years ago

Unit tests are running fine but the benchmark is throwing an error. The benchmark is running fine on the master branch...

root@develop:~/core# node bench/index.js
new hd contract x 27,206 ops/sec ±0.96% (151 runs sampled)
new contract x 28,290 ops/sec ±1.44% (157 runs sampled)
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'Error: Invalid derived public key' == null
    at /root/core/bench/index.js:94:12
    at Network._verifySignature (/root/core/lib/network/index.js:629:12)
    at /root/core/lib/network/index.js:588:10
    at Network._validateContact (/root/core/lib/network/index.js:559:3)
    at Network._verifyMessage (/root/core/lib/network/index.js:572:8)
    at verifyHDContact (/root/core/bench/index.js:93:7)
    at Deferred.d15188859248791012.fn (eval at createCompiled (/root/core/node_modules/benchmark/benchmark.js:1725:16), <anonymous>:5:28)
    at Deferred.eval (eval at createCompiled (/root/core/node_modules/benchmark/benchmark.js:1725:16), <anonymous>:10:68)
    at clock (/root/core/node_modules/benchmark/benchmark.js:1658:29)
    at new Deferred (/root/core/node_modules/benchmark/benchmark.js:405:7)
    at Deferred (/root/core/node_modules/benchmark/benchmark.js:402:16)
    at Benchmark.run (/root/core/node_modules/benchmark/benchmark.js:2112:13)
    at execute (/root/core/node_modules/benchmark/benchmark.js:860:74)
    at Timeout._onTimeout (/root/core/node_modules/lodash/lodash.js:2762:43)
    at ontimeout (timers.js:475:11)
    at tryOnTimeout (timers.js:310:5)
littleskunk commented 6 years ago

I can't get this running. I found 100 different ways to not compact the secp256k1 signature. I have no idea what else I could try.

braydonf commented 6 years ago

The signature for .sign at https://github.com/cryptocoinjs/secp256k1-node/blob/master/API.md#signbuffer-message-buffer-privatekey--object-options---signature-buffer-recovery-number includes a recover value. That value is what is input into .toCompact(recover, true). Pretty sure.

littleskunk commented 6 years ago

That was one of the 100 diffent ways or do you see any mistakes? See last commit.

braydonf commented 6 years ago

@littleskunk I added a test to verify it works and looks to be fine, see https://github.com/braydonf/storj-lib/commit/7836c4eca498d47e639c532623d103f24105662f

braydonf commented 6 years ago

@littleskunk I think the issue is that Message was used before, which does some extra steps when signing, including a "magic hash" see https://github.com/bitpay/bitcore-message/blob/master/lib/message.js#L42-L53 Will verify that's the case.

braydonf commented 6 years ago

Yes that is it. Have it working at https://github.com/braydonf/storj-lib/tree/sign

braydonf commented 6 years ago

Pretty large improvement, as expected:

w/ javascript     sign message x 82.05 ops/sec ±2.28% (130 runs sampled)
w/ c bindings     sign message x 6,786 ops/sec ±1.10% (156 runs sampled)
littleskunk commented 6 years ago

That improvement looks awesome. Thank you for your help :)