-
Notifications
You must be signed in to change notification settings - Fork 75
[ECTM-212] Fix block preparation (taking into account EIP-161) #741
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we touched such general config and consensus maybe you cold run ets tests and report if every thing is fine ?
Otherwise lgtm.
blockchainConfig.eip160BlockNumber -> (4, PostEIP160ConfigBuilder), | ||
blockchainConfig.eip161BlockNumber -> (5, PostEIP161ConfigBuilder), | ||
blockchainConfig.byzantiumBlockNumber -> (6, ByzantiumConfigBuilder), | ||
blockchainConfig.atlantisBlockNumber -> (6, AtlantisConfigBuilder), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i understand that it is also not 100% deterministic as in case of same block and priority the chooses fork will be random one. Maybe leave some comment about it and create and leave some ticket to refactor our blockchain configs ? (we have probably been postponing it long enough)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not random but first checked. So in our test config, when we set all numbers to 0 expecting to use the newest but we are getting the first in the map (so it is the oldest fork)
@@ -456,18 +473,20 @@ class BlockchainImpl( | |||
) | |||
|
|||
//FIXME Maybe we can use this one in regular execution too and persist underlying storage when block execution is successful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe create ticket for this FIXME
? as it seems having two separate methods to retrieve execution world was a bit error prone
@KonradStaniec I will run ETS and let you know |
After margin [ETCM-141] scalafmt . This PR can be updated by git merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Fix block generation.
BlockPreparator didn't work correctly in case of an empty account (with value == 0). The change was introduced in EIP-161