Repository: bitcoinjs/bitcoinjs-lib
Pull request #460 (2015-09-08 14:18:41)

Modulo tests
Adds a [failing] test for checking the RNG output for `ECPair.makeRandom`, see https://github.com/bitcoinjs/bitcoinjs-lib/pull/447#issuecomment-133856729

The test is failing only because, prior to this PR, `makeRandom` was accepting of a RNG value of `0`.
The `ECPair` constructor threw in this case,  (`Private key must be >0`),  so it is not an issue,  but may be unexpected.

Given the astronomically small possibility of this case,  it is almost certainly an error,  so I'm not even sure if looping is the right action.

However, to be consistent, I've fixed it for now.
Thoughts @weilu ?


Patch:
diff --git a/src/ecpair.js b/src/ecpair.js
index e469638f..701b6df0 100644
--- a/src/ecpair.js
+++ b/src/ecpair.js
@@ -89,7 +89,7 @@ ECPair.makeRandom = function (options) {
     typeforce(types.Buffer256bit, buffer)
 
     d = BigInteger.fromBuffer(buffer)
-  } while (d.compareTo(secp256k1.n) >= 0)
+  } while (d.signum() <= 0 || d.compareTo(secp256k1.n) >= 0)
 
   return new ECPair(d, null, options)
 }


Test patch:
diff --git a/test/ecpair.js b/test/ecpair.js
index db378a98..b9c1a8e0 100644
--- a/test/ecpair.js
+++ b/test/ecpair.js
@@ -11,7 +11,7 @@ var BigInteger = require('bigi')
 var ECPair = require('../src/ecpair')
 
 var fixtures = require('./fixtures/ecpair.json')
-var secp256k1 = ecurve.getCurveByName('secp256k1')
+var curve = ecdsa.__curve
 
 var NETWORKS = require('../src/networks')
 var NETWORKS_LIST = [] // Object.values(NETWORKS)
@@ -54,7 +54,7 @@ describe('ECPair', function () {
 
     it('throws if public and private key given', function () {
       var qBuffer = new Buffer('0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798', 'hex')
-      var Q = ecurve.Point.decodeFrom(secp256k1, qBuffer)
+      var Q = ecurve.Point.decodeFrom(curve, qBuffer)
 
       assert.throws(function () {
         new ECPair(BigInteger.ONE, Q)
@@ -100,7 +100,7 @@ describe('ECPair', function () {
 
     it('wraps Q.getEncoded', sinon.test(function () {
       this.mock(keyPair.Q).expects('getEncoded')
-        .once().calledWith(keyPair.compressed)
+        .once().withArgs(keyPair.compressed)
 
       keyPair.getPublicKeyBuffer()
     }))
@@ -160,18 +160,6 @@ describe('ECPair', function () {
         var keyPair = ProxiedECPair.makeRandom()
         assert.strictEqual(keyPair.toWIF(), exWIF)
       })
-
-      it('passes the options param', sinon.test(function () {
-        var options = {
-          compressed: true
-        }
-
-        // FIXME: waiting on https://github.com/cjohansen/Sinon.JS/issues/613
-//        this.mock(ECPair).expects('constructor')
-//          .once().calledWith(options)
-
-        ECPair.makeRandom(options)
-      }))
     })
 
     it('allows a custom RNG to be used', function () {
@@ -181,6 +169,16 @@ describe('ECPair', function () {
 
       assert.strictEqual(keyPair.toWIF(), exWIF)
     })
+
+    it('loops until d is within interval [1, n - 1]', sinon.test(function () {
+      var rng = this.mock()
+      rng.exactly(3)
+      rng.onCall(0).returns(new BigInteger('0').toBuffer(32)) // < 1
+      rng.onCall(1).returns(curve.n.toBuffer(32)) // > n-1
+      rng.onCall(2).returns(new BigInteger('42').toBuffer(32)) // valid
+
+      ECPair.makeRandom({ rng: rng })
+    }))
   })
 
   describe('getAddress', function () {
@@ -204,7 +202,7 @@ describe('ECPair', function () {
     describe('signing', function () {
       it('wraps ecdsa.sign', sinon.test(function () {
         this.mock(ecdsa).expects('sign')
-          .once().calledWith(secp256k1, hash, keyPair.d)
+          .once().withArgs(hash, keyPair.d)
 
         keyPair.sign(hash)
       }))
@@ -227,7 +225,7 @@ describe('ECPair', function () {
 
       it('wraps ecdsa.verify', sinon.test(function () {
         this.mock(ecdsa).expects('verify')
-          .once().calledWith(secp256k1, hash, signature, keyPair.Q)
+          .once().withArgs(hash, signature, keyPair.Q)
 
         keyPair.verify(hash, signature)
       }))