Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Simplify transaction's constructor and EIP155 handling #153

Merged
merged 10 commits into from
May 21, 2019
Merged

Simplify transaction's constructor and EIP155 handling #153

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
37d43fc
Simplify EIP155 handling.
alcuadrado May 17, 2019
d0c7375
Update tests
alcuadrado May 17, 2019
48e4f57
Validate v in its setter
alcuadrado May 18, 2019
1159315
Fix bug when signing an already signed tx
alcuadrado May 18, 2019
27b8876
Remove outdated comment
alcuadrado May 19, 2019
7f08267
Restore the Transaction's toJSON method
alcuadrado May 19, 2019
1698f75
Remove any previous signature before signing again
alcuadrado May 19, 2019
12f5de3
Update fake's tests
alcuadrado May 19, 2019
ed4b695
Add a tests for the new behaviour
alcuadrado May 19, 2019
bba15cb
Add an explanation to an EIP155 test.
alcuadrado May 21, 2019
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
138 changes: 94 additions & 44 deletions src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
ecsign,
toBuffer,
rlp,
stripZeros,
} from 'ethereumjs-util'
import Common from 'ethereumjs-common'
import { Buffer } from 'buffer'
Expand All @@ -32,7 +33,6 @@ export default class Transaction {
public s!: Buffer

private _common: Common
private _chainId: number
private _senderPubKey?: Buffer
protected _from?: Buffer

Expand All @@ -46,6 +46,10 @@ export default class Transaction {
* @param opts - The transaction's options, used to indicate the chain and hardfork the
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
* transactions belongs to.
*
* @note Transaction objects implement EIP155 by default. To disable it, use the constructor's
* second parameter to set a chain and hardfork before EIP155 activation (i.e. before Spurious
* Dragon.)
*
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
* @example
* ```js
* const txData = {
Expand All @@ -68,13 +72,17 @@ export default class Transaction {
) {
// instantiate Common class instance based on passed options
if (opts.common) {
if (opts.chain) {
throw new Error('Instantiation with both opts.common and opts.chain parameter not allowed!')
if (opts.chain || opts.hardfork) {
throw new Error(
'Instantiation with both opts.common, and opts.chain and opts.hardfork parameter not allowed!',
)
}

this._common = opts.common
} else {
const chain = opts.chain ? opts.chain : 'mainnet'
const hardfork = opts.hardfork ? opts.hardfork : 'byzantium'
const hardfork = opts.hardfork ? opts.hardfork : 'petersburg'
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved

this._common = new Common(chain, hardfork)
}

Expand Down Expand Up @@ -120,7 +128,7 @@ export default class Transaction {
{
name: 'v',
allowZero: true,
default: new Buffer([opts.chain || opts.common ? this._common.chainId() : 0x1c]),
default: new Buffer([]),
},
{
name: 'r',
Expand Down Expand Up @@ -161,18 +169,8 @@ export default class Transaction {
get: this.getSenderAddress.bind(this),
})

// calculate chainId from signature
const sigV = bufferToInt(this.v)
let chainId = Math.floor((sigV - 35) / 2)
if (chainId < 0) chainId = 0

// set chainId
if (opts.chain || opts.common) {
this._chainId = this._common.chainId()
} else {
const dataAsTransactionObject = data as TxData
this._chainId = chainId || dataAsTransactionObject.chainId || 0
}
this._validateV(this.v)
this._overrideVSetterWithValidation()
}

/**
Expand All @@ -191,28 +189,14 @@ export default class Transaction {
if (includeSignature) {
items = this.raw
} else {
// EIP155 spec:
// If block.number >= 2,675,000 and v = CHAIN_ID * 2 + 35 or v = CHAIN_ID * 2 + 36, then when computing
// the hash of a transaction for purposes of signing or recovering, instead of hashing only the first six
// elements (i.e. nonce, gasprice, startgas, to, value, data), hash nine elements, with v replaced by
// CHAIN_ID, r = 0 and s = 0.

const v = bufferToInt(this.v)
const onEIP155BlockOrLater = this._common.gteHardfork('spuriousDragon')
const vAndChainIdMeetEIP155Conditions =
v === this._chainId * 2 + 35 || v === this._chainId * 2 + 36
const meetsAllEIP155Conditions = vAndChainIdMeetEIP155Conditions && onEIP155BlockOrLater

const unsigned = !this.r.length && !this.s.length
const seeksReplayProtection = this._chainId > 0

if ((unsigned && seeksReplayProtection) || (!unsigned && meetsAllEIP155Conditions)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is the heaviest code logic change in the PR and I get that this upper part has been moved into this._implementsEIP155() but I am not completely sure how to read this conditional simplification.

Could you give a short reasoning for the two cases in the if clause? That has likely already been done implicitly in your other explanations on the overall changes, but just want to make sure we are not missing anything here.

Won't make this a blocker request though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, they aren't completely equivalent. Not for unsigned transactions.

The decision to use EIP155 or not when signing was previously based on the _chainId field, and its complex initialization logic in the constructor. It's simpler now: if the hardfork is spurious dragon or later, we use EIP155.

If a transaction is already signed and we are validating it, the logic is the same. It should be in spurious dragon or later and have an EIP155-encoded chain id as v to enable EIP155 validation.

const raw = this.raw.slice()
this.v = toBuffer(this._chainId)
this.r = toBuffer(0)
this.s = toBuffer(0)
items = this.raw
this.raw = raw
if (this._implementsEIP155()) {
items = [
...this.raw.slice(0, 6),
toBuffer(this.getChainId()),
// TODO: stripping zeros should probably be a responsibility of the rlp module
stripZeros(toBuffer(0)),
stripZeros(toBuffer(0)),
]
} else {
items = this.raw.slice(0, 6)
}
Expand All @@ -226,7 +210,7 @@ export default class Transaction {
* returns chain ID
*/
getChainId(): number {
return this._chainId
return this._common.chainId()
}

/**
Expand Down Expand Up @@ -266,13 +250,13 @@ export default class Transaction {
try {
const v = bufferToInt(this.v)
const useChainIdWhileRecoveringPubKey =
v >= this._chainId * 2 + 35 && this._common.gteHardfork('spuriousDragon')
v >= this.getChainId() * 2 + 35 && this._common.gteHardfork('spuriousDragon')
this._senderPubKey = ecrecover(
msgHash,
v,
this.r,
this.s,
useChainIdWhileRecoveringPubKey ? this._chainId : undefined,
useChainIdWhileRecoveringPubKey ? this.getChainId() : undefined,
Copy link
Member

Choose a reason for hiding this comment

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

All verifySignature() changes unproblematic, ok.

)
} catch (e) {
return false
Expand All @@ -288,9 +272,11 @@ export default class Transaction {
sign(privateKey: Buffer) {
const msgHash = this.hash(false)
const sig = ecsign(msgHash, privateKey)
if (this._chainId > 0) {
sig.v += this._chainId * 2 + 8

if (this._implementsEIP155()) {
sig.v += this.getChainId() * 2 + 8
}

Object.assign(this, sig)
}
Copy link
Member

Choose a reason for hiding this comment

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

Changes in sign() are ok.


Expand Down Expand Up @@ -356,4 +342,68 @@ export default class Transaction {
// Note: This never gets executed, defineProperties overwrites it.
return rlp.encode(this.raw)
}

private _validateV(v?: Buffer): void {
if (v === undefined || v.length === 0) {
return
}
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved

if (!this._common.gteHardfork('spuriousDragon')) {
return
}

const vInt = bufferToInt(v)

if (vInt === 27 || vInt === 28) {
return
}

const isValidEIP155V =
vInt === this.getChainId() * 2 + 35 || vInt === this.getChainId() * 2 + 36

if (!isValidEIP155V) {
throw new Error(
`Incompatible EIP155-based V ${vInt} and chain id ${this.getChainId()}. See the second parameter of the Transaction constructor to set the chain id.`,
)
}
}
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved

private _isSigned(): boolean {
return this.v.length > 0 && this.r.length > 0 && this.s.length > 0
}

private _overrideVSetterWithValidation() {
const vDescriptor = Object.getOwnPropertyDescriptor(this, 'v')!

Object.defineProperty(this, 'v', {
...vDescriptor,
set: v => {
if (v !== undefined) {
this._validateV(toBuffer(v))
}

vDescriptor.set!(v)
},
})
}

private _implementsEIP155(): boolean {
const onEIP155BlockOrLater = this._common.gteHardfork('spuriousDragon')

if (!this._isSigned()) {
// We sign with EIP155 all unsigned transactions after spuriousDragon
return onEIP155BlockOrLater
}

// EIP155 spec:
// If block.number >= 2,675,000 and v = CHAIN_ID * 2 + 35 or v = CHAIN_ID * 2 + 36, then when computing
// the hash of a transaction for purposes of signing or recovering, instead of hashing only the first six
// elements (i.e. nonce, gasprice, startgas, to, value, data), hash nine elements, with v replaced by
// CHAIN_ID, r = 0 and s = 0.
const v = bufferToInt(this.v)

const vAndChainIdMeetEIP155Conditions =
v === this.getChainId() * 2 + 35 || v === this.getChainId() * 2 + 36
return vAndChainIdMeetEIP155Conditions && onEIP155BlockOrLater
}
}
5 changes: 0 additions & 5 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ export type BufferLike = Buffer | TransformableToBuffer | PrefixedHexString | nu
* A transaction's data.
*/
export interface TxData {
/**
* EIP 155 chainId - mainnet: 1, ropsten: 3
*/
chainId?: number

/**
* The transaction's gas limit.
*/
Expand Down
53 changes: 46 additions & 7 deletions test/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { rlp, zeros, privateToPublic, toBuffer } from 'ethereumjs-util'

import Transaction from '../src/transaction'
import { TxsJsonEntry, VitaliksTestsDataEntry } from './types'
import Common from 'ethereumjs-common'

const txFixtures: TxsJsonEntry[] = require('./txs.json')
const txFixturesEip155: VitaliksTestsDataEntry[] = require('./ttTransactionTestEip155VitaliksTests.json')
Expand Down Expand Up @@ -228,10 +229,10 @@ tape('[Transaction]: Basic functions', function(t) {
'4646464646464646464646464646464646464646464646464646464646464646',
'hex',
)
const pt = new Transaction(txRaw, { chain: 1 })
const pt = new Transaction(txRaw)
st.equal(
pt.serialize().toString('hex'),
'ec098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a764000080018080',
'ec098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a764000080808080',
Copy link
Member

Choose a reason for hiding this comment

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

I likely don't have the complete picture yet, but I am not getting atm why the serialization now differs from the example from the EIP page linked above in the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, is this the RLP empty buffer case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's because of that.

Note that what the EIP calls "signing data" isn't the output of serialize(), it's the preimage of the hash returned by hash(false). We don't have a getter for such value in this class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added an explanation about this in the test

)
pt.sign(privateKey)
st.equal(
Expand Down Expand Up @@ -272,20 +273,58 @@ tape('[Transaction]: Basic functions', function(t) {
)

t.test('sign tx with chainId specified in params', function(st) {
const tx = new Transaction({ chainId: 42 })
const tx = new Transaction({}, { chain: 42 })
st.equal(tx.getChainId(), 42)
const privKey = new Buffer(txFixtures[0].privateKey, 'hex')
tx.sign(privKey)
const serialized = tx.serialize()
const reTx = new Transaction(serialized)
const reTx = new Transaction(serialized, { chain: 42 })
st.equal(reTx.verifySignature(), true)
st.equal(reTx.getChainId(), 42)
st.end()
})

t.test('allow chainId more than 1 byte', function(st) {
const tx = new Transaction({ chainId: 0x16b2 })
st.equal(tx.getChainId(), 0x16b2)
t.test('throws when creating a a transaction with incompatible chainid and v value', function(
st,
) {
const tx = new Transaction({}, { chain: 42 })
st.equal(tx.getChainId(), 42)
const privKey = new Buffer(txFixtures[0].privateKey, 'hex')
tx.sign(privKey)
const serialized = tx.serialize()
st.throws(() => new Transaction(serialized))
st.end()
})

t.test('Throws if chain/hardfork and commmon options are given', function(st) {
st.throws(
() => new Transaction({}, { common: new Common('mainnet', 'petersburg'), chain: 'mainnet' }),
)
st.throws(
() => new Transaction({}, { common: new Common('mainnet', 'petersburg'), chain: 'ropsten' }),
)
st.throws(
() =>
new Transaction(
{},
{ common: new Common('mainnet', 'petersburg'), hardfork: 'petersburg' },
),
)
st.end()
})

t.test('Throws if v is set to an EIP155-encoded value incompatible with the chain id', function(
st,
) {
const tx = new Transaction({}, { chain: 42 })
const privKey = new Buffer(txFixtures[0].privateKey, 'hex')
tx.sign(privKey)

st.throws(() => (tx.v = toBuffer(1)))

const unsignedTx = new Transaction(tx.raw.slice(0, 6))
st.throws(() => (unsignedTx.v = tx.v))

st.end()
})

Expand Down
13 changes: 6 additions & 7 deletions test/fake.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ const txData: FakeTxData = {
tape('[FakeTransaction]: Basic functions', function(t) {
t.test('instantiate with from / create a hash', function(st) {
st.plan(3)
const tx = new FakeTransaction(txData)
// This test doesn't use EIP155
const tx = new FakeTransaction(txData, { chain: 'mainnet', hardfork: 'homestead' })
const hash = tx.hash()
const cmpHash = Buffer.from(
'f74b039f6361c4351a99a7c6a10867369fe6701731d85dc07c15671ac1c1b648',
Expand Down Expand Up @@ -102,12 +103,10 @@ tape('[FakeTransaction]: Basic functions', function(t) {

t.test('should return getChainId', st => {
const tx = new FakeTransaction(txData)
const txWithV = new FakeTransaction({ ...txData, v: '0x28' })
const txWithChainId = new FakeTransaction({ ...txData, chainId: 4 })
st.plan(3)
st.equal(tx.getChainId(), 0, 'should return correct chainId')
st.equal(txWithV.getChainId(), 2, 'should return correct chainId')
st.equal(txWithChainId.getChainId(), 4, 'should return correct chainId')
const txWithChain = new FakeTransaction(txData, { chain: 3 })
st.plan(2)
st.equal(tx.getChainId(), 1, 'should return correct chainId')
st.equal(txWithChain.getChainId(), 3, 'should return correct chainId')
})

t.test('should getSenderAddress and getSenderPublicKey', st => {
Expand Down