Skip to content
This repository was archived by the owner on Jun 17, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -359,15 +359,16 @@ exports.importPublic = function (publicKey) {
* ECDSA sign
* @param {Buffer} msgHash
* @param {Buffer} privateKey
* @param {Number} [chainId]
* @return {Object}
*/
exports.ecsign = function (msgHash, privateKey) {
exports.ecsign = function (msgHash, privateKey, chainId) {
const sig = secp256k1.sign(msgHash, privateKey)

const ret = {}
ret.r = sig.signature.slice(0, 32)
ret.s = sig.signature.slice(32, 64)
ret.v = sig.recovery + 27
ret.v = chainId ? sig.recovery + (chainId * 2 + 35) : sig.recovery + 27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, continuous operation of old/existing signature scheme is secured by making this optional.

return ret
}

Expand All @@ -390,12 +391,13 @@ exports.hashPersonalMessage = function (message) {
* @param {Number} v
* @param {Buffer} r
* @param {Buffer} s
* @param {Number} [chainId]
* @return {Buffer} publicKey
*/
exports.ecrecover = function (msgHash, v, r, s) {
exports.ecrecover = function (msgHash, v, r, s, chainId) {
const signature = Buffer.concat([exports.setLength(r, 32), exports.setLength(s, 32)], 64)
const recovery = v - 27
if (recovery !== 0 && recovery !== 1) {
const recovery = calculateSigRecovery(v, chainId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

if (!isValidSigRecovery(recovery)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to extract this, more readable/explicit.

throw new Error('Invalid signature v value')
}
const senderPubKey = secp256k1.recover(msgHash, signature, recovery)
Expand All @@ -407,12 +409,13 @@ exports.ecrecover = function (msgHash, v, r, s) {
* @param {Number} v
* @param {Buffer} r
* @param {Buffer} s
* @param {Number} [chainId]
* @return {String} sig
*/
exports.toRpcSig = function (v, r, s) {
// NOTE: with potential introduction of chainId this might need to be updated
if (v !== 27 && v !== 28) {
throw new Error('Invalid recovery id')
exports.toRpcSig = function (v, r, s, chainId) {
let recovery = calculateSigRecovery(v, chainId)
if (!isValidSigRecovery(recovery)) {
throw new Error('Invalid signature v value')
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, also good to change the error message here.


// geth (and the RPC eth_sign method) uses the 65 byte format used by Bitcoin
Expand Down Expand Up @@ -561,18 +564,19 @@ exports.addHexPrefix = function (str) {
* @param {Buffer} r
* @param {Buffer} s
* @param {Boolean} [homestead=true]
* @param {Number} [chainId]
* @return {Boolean}
*/

exports.isValidSignature = function (v, r, s, homestead) {
exports.isValidSignature = function (v, r, s, homestead, chainId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

const SECP256K1_N_DIV_2 = new BN('7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16)
const SECP256K1_N = new BN('fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141', 16)

if (r.length !== 32 || s.length !== 32) {
return false
}

if (v !== 27 && v !== 28) {
if (!isValidSigRecovery(calculateSigRecovery(v, chainId))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More beautiful and explicit then previous comparison, good.

return false
}

Expand Down Expand Up @@ -711,3 +715,11 @@ exports.defineProperties = function (self, fields, data) {
}
}
}

function calculateSigRecovery (v, chainId) {
return chainId ? v - (2 * chainId + 35) : v - 27
}

function isValidSigRecovery (recovery) {
return recovery === 0 || recovery === 1
}
27 changes: 25 additions & 2 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,7 @@ describe('baToJSON', function () {

var echash = Buffer.from('82ff40c0a986c6a5cfad4ddf4c3aa6996f1a7837f9c398e17e5de5cbd5a12b28', 'hex')
var ecprivkey = Buffer.from('3c9229289a6125f7fdf1885a77bb12c37a8d3b4962d936f7e3084dece32a3ca1', 'hex')
var chainId = 3 // ropsten

describe('ecsign', function () {
it('should produce a signature', function () {
Expand All @@ -483,13 +484,28 @@ describe('ecsign', function () {
assert.deepEqual(sig.s, Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex'))
assert.equal(sig.v, 27)
})

it('should produce a signature for Ropsten testnet', function () {
var sig = ethUtils.ecsign(echash, ecprivkey, chainId)
assert.deepEqual(sig.r, Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex'))
assert.deepEqual(sig.s, Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex'))
assert.equal(sig.v, 41)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New ecsign case is covered with a meaningful test.

})

describe('ecrecover', function () {
it('should recover a public key', function () {
var r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
var s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
var pubkey = ethUtils.ecrecover(echash, 27, r, s)
var v = 27
var pubkey = ethUtils.ecrecover(echash, v, r, s)
assert.deepEqual(pubkey, ethUtils.privateToPublic(ecprivkey))
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old test case equivalent, ok.

it('should recover a public key (chainId = 3)', function () {
var r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
var s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
var v = 41
var pubkey = ethUtils.ecrecover(echash, v, r, s, chainId)
assert.deepEqual(pubkey, ethUtils.privateToPublic(ecprivkey))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted Ropsten case, ok.

})
it('should fail on an invalid signature (v = 21)', function () {
Expand Down Expand Up @@ -546,7 +562,14 @@ describe('isValidSignature', function () {
it('should work otherwise', function () {
var r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
var s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
assert.equal(ethUtils.isValidSignature(27, r, s), true)
var v = 27
assert.equal(ethUtils.isValidSignature(v, r, s), true)
})
it('should work otherwise(chainId=3)', function () {
var r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')
var s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')
var v = 41
assert.equal(ethUtils.isValidSignature(v, r, s, false, chainId), true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

})
// FIXME: add homestead test
})
Expand Down