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

Blockchain: Add reorg test + do not skip any reorg test in VM #926

Merged
merged 6 commits into from
Oct 30, 2020

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Oct 28, 2020

Closes #879

This PR cherry-picks from #895 and does:

  • Adds a specific (big) reorg test in the Blockchain

In this test, we create two chains: one chain which has a low difficulty and another one which has a higher difficulty, but this chain is actually 1 block behind the other chain. This is possible due to the difficulty formula: if time since last block > 18 seconds, decrease the difficulty, if it is < 9 seconds, increase the difficulty. Block number is thus not a good identifier to find the "best chain" (which has the highest sum of difficulties, i.e. the Total Difficulty). In practice, this test first writes 65 blocks to the chain. Then it puts all the blocks in (64 blocks) which have a higher total difficulty: this thus re-orgs 65 blocks. Without any necessary change this test passes. Thus, we can also:

  • Re-enables blockchain reorg tests on VM
  • Checks each skipped test in VM, if these pass, remove them from skipped tests.

@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #926 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
#block 76.14% <ø> (-0.08%) ⬇️
#blockchain 80.57% <ø> (-0.15%) ⬇️
#common 91.79% <ø> (-0.25%) ⬇️
#ethash 82.08% <ø> (ø)
#tx 88.18% <ø> (-0.19%) ⬇️
#vm 87.17% <ø> (ø)

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

@jochem-brouwer
Copy link
Member Author

Linking #895

@s1na: as discussed blockchain package actually has reorg support (which was also a surprise to me) 😜 .

ryanio
ryanio previously approved these changes Oct 29, 2020
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.

looks great!

@jochem-brouwer
Copy link
Member Author

@ryanio had to rebase this one, could you re-approve? 😄

@ryanio ryanio merged commit d601257 into master Oct 30, 2020
@ryanio ryanio deleted the add-reorg-test branch October 30, 2020 22:16
st.end()
}
)
})
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 really a beautiful beautiful test 🤩 , would even call this educational. Great.

Copy link
Member

Choose a reason for hiding this comment

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

(and all the comments along are fantastic)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand support of Blockchain for reorgs
3 participants