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

Jump to bottom

VM/tests: ensure verifyPostConditions works #1900

Merged
merged 3 commits into from
May 26, 2022
Merged

VM/tests: ensure verifyPostConditions works #1900

merged 3 commits into from
May 26, 2022

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented May 23, 2022 •
edited
Loading

If a blockchain test fails and we have the --debug flag on, we expect that we do verifyPostConditions:

ethereumjs-monorepo/packages/vm/tests/tester/runners/BlockchainTestsRunner.ts

Line 171 in 0b7cc1b

await verifyPostConditions(state, testData.postState, t)

However, this actually does not work. If the block is invalid and produces a different stateRoot than whatever we expect, then runBlockchain actually throws and the verifyPostConditions is never ran.

This PR also updates the formatter such that it is easier to read what goes wrong. On a VM which I intentially made producing wrong output:

Screenshot from 2022-05-23 23-21-05

(Was done by adding this to runBlock.ts:)

Screenshot from 2022-05-23 23-25-17

If the tests contain the postConditions we can now very quickly figure out what goes wrong if a test files 😄

@jochem-brouwer jochem-brouwer added PR state: WIP type: tests package: vm type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. labels May 23, 2022
@codecov
Copy link

codecov bot commented May 23, 2022 •
edited
Loading

Codecov Report

Merging #1900 (f3a28d2) into master (a133e27) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 85.57% <ø> (ø)
blockchain 83.82% <ø> (ø)
client 76.88% <ø> (-0.08%) ⬇️
common 94.19% <ø> (ø)
devp2p 82.49% <ø> (-0.14%) ⬇️
ethash 90.76% <ø> (ø)
trie 80.02% <ø> (ø)
tx 88.22% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented May 25, 2022 •
edited
Loading

Updated this one, sample messages:

# Account: 0x8888f1f195afa192cfee860698584c030f4c9db1
# Expected balance of 30312500000000021000, but got 30312500000000020999
# Expected nonce of 0, but got 1
# Account: 0x0000000000000000000000000000000000000011
# Account: 0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b
# Account: 0xb94f5374fce5ed0000000097c15331677e6ebf0b
# Account: 0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b
# Expected storage key 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 to have value 0x, but got 0x01}

There are some gotcha's in the diffs, will comment.

jochem-brouwer
jochem-brouwer commented May 25, 2022
View reviewed changes
packages/vm/tests/util.ts
@@ -144,14 +144,14 @@ export async function verifyPostConditions(state: any, testData: any, t: tape.Te
const promise = verifyAccountPostConditions(state, address, account, testData, t)
queue.push(promise)
} else {
t.fail('invalid account in the trie: ' + <string>key)
t.comment('invalid account in the trie: ' + <string>key)
Copy link
Member Author

Choose a reason for hiding this comment

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

Converted all tape commands to comment. The reason is that if you have failing tests, then this adds to the number of tests counter. Let's say we accidentally do not run some tests, then --verify-amount-alltests flag could still pass while this is actually not the case.

Also note that if any of these comments get triggered, then the state root of the block is wrong, so the test fails anyways.

jochem-brouwer
jochem-brouwer commented May 25, 2022
View reviewed changes
packages/vm/tests/util.ts
})
if (val !== hashedStorage[key]) {
t.comment(
`Expected storage key 0x${data.key.toString('hex')} at address ${address} to have value ${
Copy link
Member Author

Choose a reason for hiding this comment

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

Have to add address here since it is not clear when these promises are fulfilled.

jochem-brouwer
jochem-brouwer commented May 25, 2022
View reviewed changes
packages/vm/tests/util.ts
@@ -188,38 +195,40 @@ export function verifyAccountPostConditions(
] = acctData.storage[key]
}

if (storageKeys.length > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this if statement: if there is no storage in the postState but we somehow still store something, then we should detect that anyways.

@jochem-brouwer jochem-brouwer marked this pull request as ready for review May 25, 2022 20:39
@jochem-brouwer jochem-brouwer requested a review from ryanio May 25, 2022 20:47
ryanio
ryanio approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

nice, lgtm!

@holgerd77 holgerd77 force-pushed the blockchain-tester-update branch from 862fbb8 to f3a28d2 Compare May 26, 2022 17:40
@holgerd77
Copy link
Member

Updated this via rebase (via Web UI).

holgerd77
holgerd77 approved these changes May 26, 2022
View reviewed changes
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit 2b993aa into master May 26, 2022
@holgerd77 holgerd77 deleted the blockchain-tester-update branch May 26, 2022 18:08
@holgerd77 holgerd77 mentioned this pull request May 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holgerd77 holgerd77 holgerd77 approved these changes

@ryanio ryanio ryanio approved these changes

Assignees
No one assigned
Labels
package: vm PR state: needs review type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. type: tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jochem-brouwer @holgerd77 @ryanio