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

Add frontier support #828

Merged
merged 9 commits into from
Aug 21, 2020
Merged

Add frontier support #828

merged 9 commits into from
Aug 21, 2020

Conversation

jochem-brouwer
Copy link
Member

Builds upon #815

Linked issue: #652

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #828 into master will decrease coverage by 0.22%.
The diff coverage is 50.00%.

Impacted file tree graph

Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 77.58% <ø> (-0.49%) ⬇️
#blockchain 81.03% <ø> (-0.31%) ⬇️
#common 93.98% <ø> (ø)
#ethash 83.33% <ø> (ø)
#tx 94.16% <ø> (+0.13%) ⬆️
#vm 92.49% <50.00%> (-0.32%) ⬇️

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

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Jul 31, 2020

Frontier blockchain tests
1..2751
# tests 2751
# pass  2530
# fail  221

It seems that the block package will need some changes too (Homestead changed the difficulty algorithm). The Tx changes seem to be available already.

After 51c7099: (note: currently due to how tests work the test pass amount stays the same, but only the fail amount will decrease)

1..2744
# tests 2744
# pass  2530
# fail  214

After e9b70f1:

1..2650
# tests 2650
# pass  2530
# fail  120

@jochem-brouwer
Copy link
Member Author

Linking some Geth code here for reference (what happens if a CREATE message (either by normal message call or via CREATE opcode) finishes, but there is not enough gas to pay for the code deposit gas (200 gas per byte)?)

https://github.com/ethereum/go-ethereum/blob/295693759e5ded05fec0b2fb39359965b60da785/core/vm/evm.go#L467

https://github.com/ethereum/go-ethereum/blob/295693759e5ded05fec0b2fb39359965b60da785/core/vm/instructions.go#L622

@jochem-brouwer jochem-brouwer changed the base branch from master to fix-blockchain-tests July 31, 2020 14:46
@jochem-brouwer
Copy link
Member Author

Changed the base so it will be easier to review this one.

@jochem-brouwer jochem-brouwer force-pushed the fix-blockchain-tests branch 5 times, most recently from 17e0b70 to b303a21 Compare August 6, 2020 19:08
@holgerd77 holgerd77 mentioned this pull request Aug 9, 2020
27 tasks
Base automatically changed from fix-blockchain-tests to master August 12, 2020 15:27
@holgerd77
Copy link
Member

@jochem-brouwer: Pre-Homestead is already integrated in the block package difficulty calculation, so we are fine on this front! 😄

@holgerd77 holgerd77 force-pushed the add-frontier-support branch from e9b70f1 to c7ef60f Compare August 14, 2020 09:08
@holgerd77
Copy link
Member

Rebased this, @jochem-brouwer let me know if this was a counter service (in german we would say: "Bärendienst", bear service) and it rather messed something up, was intended to be some supportive task though. 😄

@holgerd77 holgerd77 force-pushed the add-frontier-support branch from c7ef60f to 0ab41f1 Compare August 18, 2020 10:30
@holgerd77
Copy link
Member

Rebased this on top of #833 and added Chainstart runs to state and blockchain tests.

} else {
// we are in chainstart and the error was the code deposit error
// we do like nothing happened.
this._state.commit()
Copy link
Contributor

@ryanio ryanio Aug 19, 2020

Choose a reason for hiding this comment

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

hm do we need an await here?

Suggested change
this._state.commit()
await this._state.commit()

@jochem-brouwer
Copy link
Member Author

Tests pass! Most errors where actually in the test suite, not in the code (with a few exceptions). Really good to see that most of the code of Frontier was already in block and tx (see point 2 and 4 of EIP 2)

@holgerd77 holgerd77 force-pushed the add-frontier-support branch from 46bdd46 to 0f4e222 Compare August 21, 2020 21:35
@holgerd77
Copy link
Member

Rebased this.

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.

Went through the changes on a commit basis and had a look at the test output, this actually looks good now.

All HFs implemented. Congratulations @jochem-brouwer, great work!!! 🎊 😀

@holgerd77 holgerd77 merged commit ea73164 into master Aug 21, 2020
@holgerd77 holgerd77 deleted the add-frontier-support branch August 21, 2020 21:56
@holgerd77
Copy link
Member

holgerd77 commented Aug 21, 2020

(ah, except from the DAO, I wonder if we shouldn't really do this and complete here, this shouldn't be too much work? 🤔 )

Here are the I would think core parts of the geth implementation.

EIP for the DAO HF is EIP-779.

@jochem-brouwer
Copy link
Member Author

Yeah we should do it, if you want I can start on this tomorrow? If it turns out to be rather complex it should not be something which blocks the V5 release though. (But it seems to be rather easy).

@holgerd77
Copy link
Member

That would be great! 😄

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.

3 participants