Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TangerineWhistle HF Support #807

Merged
merged 10 commits into from
Jul 15, 2020
2 changes: 1 addition & 1 deletion packages/vm/lib/evm/eei.ts
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ export default class EEI {
}

/**
* Returns true if account is empty (according to EIP-161).
* Returns true if account is empty or non-existent (according to EIP-161).
* @param address - Address of account
*/
async isAccountEmpty(address: Buffer): Promise<boolean> {
Expand Down
5 changes: 4 additions & 1 deletion packages/vm/lib/evm/evm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,10 @@ export default class EVM {
await this._vm._emit('newContract', newContractEvent)

toAccount = await this._state.getAccount(message.to)
toAccount.nonce = new BN(toAccount.nonce).addn(1).toArrayLike(Buffer)
// EIP-161 on account creation and CREATE execution
if (this._vm._common.gteHardfork('spuriousDragon')) {
toAccount.nonce = new BN(toAccount.nonce).addn(1).toArrayLike(Buffer)
}

// Add tx value to the `to` account
let errorMessage
Expand Down
42 changes: 33 additions & 9 deletions packages/vm/lib/evm/opFns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -660,9 +660,9 @@ export const handlers: { [k: string]: OpHandler } = {
if (runState.eei.isStatic() && !value.isZero()) {
trap(ERROR.STATIC_STATE_CHANGE)
}

subMemUsage(runState, inOffset, inLength)
subMemUsage(runState, outOffset, outLength)

if (!value.isZero()) {
runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callValueTransfer')))
}
Expand All @@ -673,9 +673,17 @@ export const handlers: { [k: string]: OpHandler } = {
data = runState.memory.read(inOffset.toNumber(), inLength.toNumber())
}

const empty = await runState.eei.isAccountEmpty(toAddressBuf)
if (empty) {
if (!value.isZero()) {
if (runState._common.gteHardfork('spuriousDragon')) {
// We are at or after Spurious Dragon
// Call new account gas: account is DEAD and we transfer nonzero value
if ((await runState.stateManager.accountIsEmpty(toAddressBuf)) && !value.isZero()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add these methods to EEI and use those here instead? We want to make sure the EVM has access only to what's exposed from the EEI and nothing else.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what you mean here. You mean the calls to the stateManager? I.e. put accountIsEmpty and accountExists in EEI?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah exactly. isAccountEmtpy is already in the EEI and can be used. accountExists needs to be added.
To clarify the reasoning, we want to isolate the execution of EVM code as much as possible. EEI is supposed to be the only external interface evm code has access to.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, makes sense!

runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
}
} else if (!(await runState.stateManager.accountExists(toAddressBuf))) {
// We are before Spurious Dragon
// Call new account gas: account does not exist (it is not in the state trie, not even as an "empty" account)
const accountDoesNotExist = !(await runState.stateManager.accountExists(toAddressBuf))
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to call accountExists twice

if (accountDoesNotExist) {
runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
}
}
Expand Down Expand Up @@ -789,13 +797,29 @@ export const handlers: { [k: string]: OpHandler } = {
}

const selfdestructToAddressBuf = addressToBuffer(selfdestructToAddress)
const balance = await runState.eei.getExternalBalance(runState.eei.getAddress())
if (balance.gtn(0)) {
const empty = await runState.eei.isAccountEmpty(selfdestructToAddressBuf)
if (empty) {
runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
let deductGas = false
if (runState._common.gteHardfork('spuriousDragon')) {
// EIP-161: State Trie Clearing
const balance = await runState.eei.getExternalBalance(runState.eei.getAddress())
if (balance.gtn(0)) {
// This technically checks if account is empty or non-existent
// TODO: improve on the API here (EEI and StateManager)
const empty = await runState.eei.isAccountEmpty(selfdestructToAddressBuf)
if (empty) {
const account = await runState.stateManager.getAccount(selfdestructToAddressBuf)
deductGas = true
}
}
} else {
// Pre EIP-161 (Spurious Dragon) gas semantics
const exists = await runState.stateManager.accountExists(selfdestructToAddressBuf)
if (!exists) {
deductGas = true
}
}
if (deductGas) {
runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
}

return runState.eei.selfDestruct(selfdestructToAddressBuf)
},
Expand Down
1 change: 1 addition & 0 deletions packages/vm/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export default class VM extends AsyncEventEmitter {
const chain = opts.chain ? opts.chain : 'mainnet'
const hardfork = opts.hardfork ? opts.hardfork : 'petersburg'
const supportedHardforks = [
'tangerineWhistle',
'spuriousDragon',
'byzantium',
'constantinople',
Expand Down
10 changes: 6 additions & 4 deletions packages/vm/lib/runTx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,17 +174,19 @@ async function _runTx(this: VM, opts: RunTxOpts): Promise<RunTxResult> {
const minerAccount = await state.getAccount(block.header.coinbase)
// add the amount spent on gas to the miner's account
minerAccount.balance = toBuffer(new BN(minerAccount.balance).add(results.amountSpent))
if (!new BN(minerAccount.balance).isZero()) {
await state.putAccount(block.header.coinbase, minerAccount)
}

// Put the miner account into the state. If the balance of the miner account remains zero, note that
// the state.putAccount function puts this into the "touched" accounts. This will thus be removed when
// we clean the touched accounts below in case we are in a fork >= SpuriousDragon
await state.putAccount(block.header.coinbase, minerAccount)

/*
* Cleanup accounts
*/
if (results.execResult.selfdestruct) {
const keys = Object.keys(results.execResult.selfdestruct)
for (const k of keys) {
await state.putAccount(Buffer.from(k, 'hex'), new Account())
await state.deleteAccount(Buffer.from(k, 'hex'))
}
}
await state.cleanupTouchedAccounts()
Expand Down
20 changes: 13 additions & 7 deletions packages/vm/lib/state/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,24 +54,30 @@ export default class Cache {
/**
* Looks up address in underlying trie.
* @param address - Address of account
* @param create - Create emtpy account if non-existent
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param create - Create emtpy account if non-existent
* @param create - Create empty account if non-existent

*/
async _lookupAccount(address: Buffer): Promise<Account> {
async _lookupAccount(address: Buffer, create: boolean = true): Promise<Account | undefined> {
const raw = await this._trie.get(address)
const account = new Account(raw)
return account
if (raw || create) {
const account = new Account(raw)
return account
}
}

/**
* Looks up address in cache, if not found, looks it up
* in the underlying trie.
* @param key - Address of account
* @param create - Create emtpy account if non-existent
*/
async getOrLoad(key: Buffer): Promise<Account> {
async getOrLoad(key: Buffer, create: boolean = true): Promise<Account | undefined> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any invocation of getOrLoad with create=false. Are these changes deprecated? in that case can you revert them? (also _lookupAccount)

Copy link
Member

Choose a reason for hiding this comment

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

Yup you are right, I will remove this.

let account = this.lookup(key)

if (!account) {
account = await this._lookupAccount(key)
this._update(key, account, false, false)
account = await this._lookupAccount(key, create)
if (account) {
this._update(key, account as Account, false, false)
}
}

return account
Expand All @@ -87,7 +93,7 @@ export default class Cache {
if (addressHex) {
const address = Buffer.from(addressHex, 'hex')
const account = await this._lookupAccount(address)
this._update(address, account, false, false)
this._update(address, account as Account, false, false)
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/vm/lib/state/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ export interface StorageDump {
export interface StateManager {
copy(): StateManager
getAccount(address: Buffer): Promise<Account>
putAccount(address: Buffer, account: Account): Promise<void>
putAccount(address: Buffer, account: Account | null): Promise<void>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the interface of DefaultStateManager.putAccount changing. Is this change still valid or maybe deleteAccount suffices here?

Copy link
Member

Choose a reason for hiding this comment

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

Yup I should revert the changes to the interface, thanks!

deleteAccount(address: Buffer): Promise<void>
touchAccount(address: Buffer): void
putContractCode(address: Buffer, value: Buffer): Promise<void>
getContractCode(address: Buffer): Promise<Buffer>
Expand All @@ -28,5 +29,6 @@ export interface StateManager {
generateCanonicalGenesis(): Promise<void>
generateGenesis(initState: any): Promise<void>
accountIsEmpty(address: Buffer): Promise<boolean>
accountExists(address: Buffer): Promise<boolean>
cleanupTouchedAccounts(): Promise<void>
}
43 changes: 33 additions & 10 deletions packages/vm/lib/state/stateManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export default class DefaultStateManager implements StateManager {
* @param address - Address of the `account` to get
*/
async getAccount(address: Buffer): Promise<Account> {
const account = await this._cache.getOrLoad(address)
const account = (await this._cache.getOrLoad(address)) as Account
Copy link
Contributor

Choose a reason for hiding this comment

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

this is cast with as Account because getOrLoad was updated to return with optional undefined (Promise<Account | undefined>), so it may be better to update the return signature of the function here instead of casting.

return account
}

Expand All @@ -87,7 +87,11 @@ export default class DefaultStateManager implements StateManager {
// if they have money or a non-zero nonce or code, then write to tree
this._cache.put(address, account)
this.touchAccount(address)
// this._trie.put(addressHex, account.serialize(), cb)
}

async deleteAccount(address: Buffer) {
this._cache.del(address)
this.touchAccount(address)
}

/**
Expand All @@ -111,7 +115,7 @@ export default class DefaultStateManager implements StateManager {
const codeHash = keccak256(value)

if (codeHash.equals(KECCAK256_NULL)) {
return
//return
Copy link
Contributor

@ryanio ryanio Jul 14, 2020

Choose a reason for hiding this comment

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

a note that this is commented out

Copy link
Contributor

Choose a reason for hiding this comment

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

@jochem-brouwer can you clarify why this should be commented out?

Copy link
Member

Choose a reason for hiding this comment

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

It should not be commented out, this is a remainder from toying around in order to resolve those failing tests. It will not change anything though; the only call is from evm.ts which does not call putContractCode if the code to put is the empty string.

}

const account = await this.getAccount(address)
Expand Down Expand Up @@ -468,7 +472,8 @@ export default class DefaultStateManager implements StateManager {
}

/**
* Checks if the `account` corresponding to `address` is empty as defined in
* Checks if the `account` corresponding to `address`
* is empty or non-existent as defined in
* EIP-161 (https://eips.ethereum.org/EIPS/eip-161).
* @param address - Address to check
*/
Expand All @@ -477,17 +482,35 @@ export default class DefaultStateManager implements StateManager {
return account.isEmpty()
}

/**
* Checks if the `account` corresponding to `address`
* exists
* @param address - Address of the `account` to check
*/
async accountExists(address: Buffer): Promise<boolean> {
const account = await this._cache.lookup(address)
if (account) {
return true
}
if (await this._cache._trie.get(address)) {
return true
}
return false
}

/**
* Removes accounts form the state trie that have been touched,
* as defined in EIP-161 (https://eips.ethereum.org/EIPS/eip-161).
*/
async cleanupTouchedAccounts(): Promise<void> {
const touchedArray = Array.from(this._touched)
for (const addressHex of touchedArray) {
const address = Buffer.from(addressHex, 'hex')
const empty = await this.accountIsEmpty(address)
if (empty) {
this._cache.del(address)
if (this._common.gteHardfork('spuriousDragon')) {
const touchedArray = Array.from(this._touched)
for (const addressHex of touchedArray) {
const address = Buffer.from(addressHex, 'hex')
const empty = await this.accountIsEmpty(address)
if (empty) {
this._cache.del(address)
}
}
}
this._touched.clear()
Expand Down
4 changes: 2 additions & 2 deletions packages/vm/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
"coverage:test": "npm run build && tape './tests/api/**/*.js' ./tests/tester.js --state --dist",
"docs:build": "typedoc --options typedoc.js",
"test:state": "ts-node ./tests/tester --state",
"test:state:allForks": "npm run test:state -- --fork=SpuriousDragon && npm run test:state -- --fork=Byzantium && npm run test:state -- --fork=Constantinople && npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",
"test:state:selectedForks": "npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=SpuriousDragon",
"test:state:allForks": "npm run test:state -- --fork=TangerineWhistle && npm run test:state -- --fork=SpuriousDragon && npm run test:state -- --fork=Byzantium && npm run test:state -- --fork=Constantinople && npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=Istanbul && npm run test:state -- --fork=MuirGlacier",
"test:state:selectedForks": "npm run test:state -- --fork=Petersburg && npm run test:state -- --fork=TangerineWhistle",
"test:state:slow": "npm run test:state -- --runSkipped=slow",
"test:buildIntegrity": "npm run test:state -- --test='stackOverflow'",
"test:blockchain": "node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain",
Expand Down
31 changes: 31 additions & 0 deletions packages/vm/tests/api/state/stateManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,37 @@ tape('StateManager', (t) => {
},
)

t.test(
'should return false for a non-existent account',
async (st) => {
const stateManager = new DefaultStateManager()
const address = Buffer.from('a94f5374fce5edbc8e2a8697c15331677e6ebf0b', 'hex')

let res = await stateManager.accountExists(address)

st.notOk(res)

st.end()
},
)

t.test(
'should return true for an existent account',
async (st) => {
const stateManager = new DefaultStateManager()
const account = createAccount('0x1', '0x1')
const address = Buffer.from('a94f5374fce5edbc8e2a8697c15331677e6ebf0b', 'hex')

await stateManager.putAccount(address, account)

let res = await stateManager.accountExists(address)

st.ok(res)

st.end()
},
)

t.test(
'should call the callback with a false boolean representing non-emptiness when the account is not empty',
async (st) => {
Expand Down
5 changes: 5 additions & 0 deletions packages/vm/tests/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ const SKIP_VM = [
* @returns {String} Either an alias of the forkConfig param, or the forkConfig param itself
*/
function getRequiredForkConfigAlias(forkConfig) {
// TangerineWhistle is named EIP150 (attention: misleading name)
// in the client-independent consensus test suite
if (String(forkConfig).match(/^tangerineWhistle$/i)) {
return 'EIP150'
}
// SpuriousDragon is named EIP158 (attention: misleading name)
// in the client-independent consensus test suite
if (String(forkConfig).match(/^spuriousDragon$/i)) {
Expand Down