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.

Sign up for GitHub

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

SpuriousDragon HF Support #791

Merged
merged 6 commits into from
Jul 2, 2020
+105 −15
Merged

SpuriousDragon HF Support #791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ef4add
vm > spuriousDragon: added HF and alias (EIP-158) support to tester, …
holgerd77 Jun 25, 2020
d6fcb8e
vm > spuriousDragon: added allowed Code size check to EVM message exe…
holgerd77 Jun 25, 2020
e91cbb4
vm -> spuriousDragon: added EIP-211 Byzantium HF checks for RETURNDAT…
holgerd77 Jun 25, 2020
7b4bf93
vm -> spuriousDragon: added Byzantium HF checks for STATICCALL (EIP-2…
holgerd77 Jun 25, 2020
024f5c9
vm -> spuriousDragon: added dedicated Byzantium HF list to getOpcodes…
holgerd77 Jun 25, 2020
2e9572f
vm -> spuriousDragon: added pre-Byzantium tx receipt format (EIP-658)…
holgerd77 Jun 26, 2020
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
12 changes: 10 additions & 2 deletions packages/vm/lib/evm/evm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,18 @@ export default class EVM {
totalGas = totalGas.add(returnFee)
}

// if not enough gas
// Check for SpuriousDragon EIP-170 code size limit
let allowedCodeSize = true
if (
this._vm._common.gteHardfork('spuriousDragon') &&
result.returnValue.length > this._vm._common.param('vm', 'maxCodeSize')
) {
allowedCodeSize = false
}
// If enough gas and allowed code size
if (
totalGas.lte(message.gasLimit) &&
(this._vm.allowUnlimitedContractSize || result.returnValue.length <= 24576)
(this._vm.allowUnlimitedContractSize || allowedCodeSize)
jochem-brouwer marked this conversation as resolved.
Show resolved Hide resolved
) {
result.gasUsed = totalGas
} else {
Expand Down
12 changes: 12 additions & 0 deletions packages/vm/lib/evm/opFns.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,9 +401,15 @@ export const handlers: { [k: string]: OpHandler } = {
runState.stack.push(new BN(keccak256(code)))
},
RETURNDATASIZE: function (runState: RunState) {
if (!runState._common.gteHardfork('byzantium')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a side-note: we won't be needing these checks anymore after getOpcodesForHF supports all the forks.

Copy link
Member

Choose a reason for hiding this comment

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

Yup these can be removed as these changes are in master.

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, but this will be another PR.

Can you approve here again so that we can merge? Sina already did but I dismissed by doing the rebase. 😋

trap(ERROR.INVALID_OPCODE)
}
runState.stack.push(runState.eei.getReturnDataSize())
},
RETURNDATACOPY: function (runState: RunState) {
if (!runState._common.gteHardfork('byzantium')) {
trap(ERROR.INVALID_OPCODE)
}
let [memOffset, returnDataOffset, length] = runState.stack.popN(3)

if (returnDataOffset.add(length).gt(runState.eei.getReturnDataSize())) {
Expand Down Expand Up @@ -769,6 +775,9 @@ export const handlers: { [k: string]: OpHandler } = {
runState.stack.push(ret)
},
STATICCALL: async function (runState: RunState) {
if (!runState._common.gteHardfork('byzantium')) {
trap(ERROR.INVALID_OPCODE)
}
const value = new BN(0)
let [gasLimit, toAddress, inOffset, inLength, outOffset, outLength] = runState.stack.popN(6)
const toAddressBuf = addressToBuffer(toAddress)
Expand Down Expand Up @@ -797,6 +806,9 @@ export const handlers: { [k: string]: OpHandler } = {
runState.eei.finish(returnData)
},
REVERT: function (runState: RunState) {
if (!runState._common.gteHardfork('byzantium')) {
trap(ERROR.INVALID_OPCODE)
}
const [offset, length] = runState.stack.popN(2)
subMemUsage(runState, offset, length)
let returnData = Buffer.alloc(0)
Expand Down
1 change: 1 addition & 0 deletions packages/vm/lib/index.ts
View file
Open in desktop
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 = [
'spuriousDragon',
'byzantium',
'constantinople',
'petersburg',
Expand Down
52 changes: 43 additions & 9 deletions packages/vm/lib/runBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,21 +44,17 @@ export interface RunBlockResult {
/**
* Receipts generated for transactions in the block
*/
receipts: TxReceipt[]
receipts: (PreByzantiumTxReceipt | PostByzantiumTxReceipt)[]
/**
* Results of executing the transactions in the block
*/
results: RunTxResult[]
}

/**
* Receipt generated for a transaction
* Abstract interface with common transaction receipt fields
*/
export interface TxReceipt {
/**
* Status of transaction, `1` if successful, `0` if an exception occured
*/
status: 0 | 1
interface TxReceipt {
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
/**
* Gas used
*/
Expand All @@ -73,6 +69,28 @@ export interface TxReceipt {
logs: any[]
}

/**
* Pre-Byzantium receipt type with a field
* for the intermediary state root
*/
export interface PreByzantiumTxReceipt extends TxReceipt {
/**
* Intermediary state root
*/
stateRoot: Buffer
}

/**
* Receipt type for Byzantium and beyond replacing the intermediary
* state root field with a status code field (EIP-658)
*/
export interface PostByzantiumTxReceipt extends TxReceipt {
/**
* Status of transaction, `1` if successful, `0` if an exception occured
*/
status: 0 | 1
}

/**
* @ignore
*/
Expand Down Expand Up @@ -219,12 +237,28 @@ async function applyTransactions(this: VM, block: any, opts: RunBlockOpts) {
// Combine blooms via bitwise OR
bloom.or(txRes.bloom)

const txReceipt: TxReceipt = {
status: txRes.execResult.exceptionError ? 0 : 1, // Receipts have a 0 as status on error
const abstractTxReceipt: TxReceipt = {
gasUsed: gasUsed.toArrayLike(Buffer),
bitvector: txRes.bloom.bitvector,
logs: txRes.execResult.logs || [],
}
let txReceipt
if (this._common.gteHardfork('byzantium')) {
txReceipt = {
status: txRes.execResult.exceptionError ? 0 : 1, // Receipts have a 0 as status on error
...abstractTxReceipt,
} as PostByzantiumTxReceipt
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
} else {
// This is just using a dummy place holder for the state root right now.
// Giving the correct intermediary state root would need a too depp intervention
// into the current checkpointing mechanism which hasn't been considered
// to be worth it on a HF backport, 2020-06-26
txReceipt = {
stateRoot: Buffer.alloc(32),
...abstractTxReceipt,
} as PreByzantiumTxReceipt
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, @s1na @alcuadrado please have a look if you are satisfied with this implementation, I thought it is not worth to update the checkpointing mechanism here, since this is a feature (which we then doesn't fully provide for pre-Byzantium) rather than consensus critical code.

We can alternatively also just fill in an empty Buffer and avoid changing the StateManager interface (might generally be a useful addition, not sure 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.

Some working note: just tested to run the blockchain tests on older HFs with node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Byzantium.

These are currently NOT working and throwing errors like invalid block stateRoot and invalid receiptTrie. I can confirm though that this is also happening on master so this is not induced by the changes here and rather need some separate investigation. @evertonfraga We should also run - I would suggest - 1 additional fork config on the blockchain tests on the nightly test runs.

The Istanbul tests are running ('node -r ts-node/register --stack-size=1500 ./tests/tester --blockchain --fork=Istanbul'). The older tests not running doesn't necessarily mean that this originates in one or several bugs in our code but can also have its origin on the tests side and eventually the older HF blockchain tests don't get accurately generated any more respectively have been at the state of the testssnapshot.

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 would rather suggest not to do this investigation here but do this separately. Otherwise the scope here gets too big.

Copy link
Contributor

Choose a reason for hiding this comment

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

The receiptTrie field in the block header is a commitment to the tx receipts, so I wouldn't be surprised if they were incorrect. But I agree we can probably handle this separately because as you said it needs deeper changes. I'd suggest however till then to keep the StateManager interface unchanged and if the intermediate state root is incorrect anyway then just return a dummy value until we dig deeper.

receipts.push(txReceipt)

// Add receipt to trie to later calculate receipt root
Expand Down
4 changes: 2 additions & 2 deletions packages/vm/package.json
View file
Open in desktop
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=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=Istanbul && npm run test:state -- --fork=MuirGlacier",
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

State tests are our bottleneck, I was thinking in splitting state tests in two jobs, so instead of a 12min job, it could be two jobs of 6:30s each. Let me know if that would speed up your work @holgerd77, so I can implement it right into this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the offer, not necessary here on this round, but definitely useful to split these up. We can do test:state:selectedLatestForks and test:state:selectedNonLatestForks to give this some (somewhat blurry) semantics for better orientation, rather that test:state:selectedForks1 and ...2 or something.

Just a first-shot idea, feel free to take on or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking about an experiment re state tests, and that'd be to have the test runner accept multiple forks, prepare everything once and run each test for all the forks consecutively. It probably won't yield much of a speed-up, but might be worth trying out if the effort required isn't that high.

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 re-add Istanbul and MuirGlacier to seletedForks state test since those are run by CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

My suggestion was to leave this to Everton to readd (see comment below) along splitting the state test run CI command into two (for performance reasons). Could you live with that as well?

"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
29 changes: 29 additions & 0 deletions packages/vm/tests/api/runBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,32 @@ tape('should run valid block', async (t) => {

t.end()
})

async function runWithHf(hardfork) {
const vm = setupVM({ hardfork: hardfork })
const suite = setup(vm)

const block = new Block(util.rlp.decode(suite.data.blocks[0].rlp))

await setupPreConditions(suite.vm.stateManager._trie, suite.data)

let res = await suite.p.runBlock({
block,
generate: true,
skipBlockValidation: true,
})
return res
}

tape('should return correct HF receipts', async (t) => {
let res = await runWithHf('byzantium')
t.equal(res.receipts[0].status, 1, 'should return correct post-Byzantium receipt format')

res = await runWithHf('spuriousDragon')
jochem-brouwer marked this conversation as resolved.
Show resolved Hide resolved
t.deepEqual(
res.receipts[0].stateRoot,
Buffer.alloc(32),
'should return correct pre-Byzantium receipt format')

t.end()
})
8 changes: 7 additions & 1 deletion packages/vm/tests/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,17 @@ const SKIP_VM = [
* @returns {String} Either an alias of the forkConfig param, or the forkConfig param itself
*/
function getRequiredForkConfigAlias(forkConfig) {
// SpuriousDragon is named EIP158 (attention: misleading name)
// in the client-independent consensus test suite
if (String(forkConfig).match(/^spuriousDragon$/i)) {
jochem-brouwer marked this conversation as resolved.
Show resolved Hide resolved
return 'EIP158'
holgerd77 marked this conversation as resolved.
Show resolved Hide resolved
}
// Run the Istanbul tests for MuirGlacier since there are no dedicated tests
if (String(forkConfig).match(/^muirGlacier/i)) {
return 'Istanbul'
}
// Petersburg is named ConstantinopleFix in the client-independent consensus test suite
// Petersburg is named ConstantinopleFix
// in the client-independent consensus test suite
if (String(forkConfig).match(/^petersburg$/i)) {
return 'ConstantinopleFix'
}
Expand Down
2 changes: 1 addition & 1 deletion packages/vm/tests/tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function runTests() {

const FORK_CONFIG = (argv.fork || config.DEFAULT_FORK_CONFIG)
const FORK_CONFIG_TEST_SUITE = config.getRequiredForkConfigAlias(FORK_CONFIG)
// Istanbul -> istanbul, MuirGlacier -> muirGlacier
// Examples: Istanbul -> istanbul, MuirGlacier -> muirGlacier
const FORK_CONFIG_VM = FORK_CONFIG.charAt(0).toLowerCase() + FORK_CONFIG.substring(1)

/**
Expand Down