Skip to content

[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

Merged
merged 5 commits into from
Oct 19, 2020

Conversation

mmrozek
Copy link
Contributor

@mmrozek mmrozek commented Oct 15, 2020

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

@mmrozek mmrozek requested review from KonradStaniec, mirkoAlic and a user October 15, 2020 15:42
Copy link
Contributor

@KonradStaniec KonradStaniec left a 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),
Copy link
Contributor

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)

Copy link
Contributor Author

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
Copy link
Contributor

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

@mmrozek
Copy link
Contributor Author

mmrozek commented Oct 16, 2020

@KonradStaniec I will run ETS and let you know

@lemastero
Copy link
Contributor

After margin [ETCM-141] scalafmt . This PR can be updated by git merge etcm-212-fix-tx-mining-fmt

+486 −348
+80 -29

@mmrozek mmrozek requested a review from kapke October 19, 2020 08:55
Copy link
Contributor

@kapke kapke left a comment

Choose a reason for hiding this comment

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

LGTM!

@mmrozek mmrozek merged commit 2fb06c0 into develop Oct 19, 2020
@mmrozek mmrozek deleted the etcm-212-fix-tx-mining branch October 19, 2020 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants