celo-org / optimism

Optimism is Ethereum, scaled.
https://optimism.io
MIT License
3 stars 2 forks source link

Migration script fixes #179

Closed piersy closed 1 week ago

piersy commented 1 week ago

These fixes ensure that the migration script works when there are no ancients and also when the final celo L1 block is a pre-gingerbread block.

The migration script was assuming that ancients would be present and was considering the numAncients-1 to be first non ancient block to migrate but when numAncients is zero that's a problem.

Also removed logic for picking up where db migration left of for the level db since it was complicating the logic for correctly calculating the non ancient block to start migrating from.

Finally these changes ensure that the migration block has a non zero gas limit, even if the parent block had a zero gas limit

palango commented 1 week ago

Finally these changes ensure that the migration block has a non zero gas limit, even if the parent block had a zero gas limit

This looks fine to me, the rest I leave for @alecps .

Out of curiosity, the gas limit of zero would be problematic because it would be used in the next block as well or is there another reason?

piersy commented 1 week ago

@palango its because gas limit zero is used to determine whether a block is pre or post gingerbread and so with a gas limit of zero the trasition block wouldn't be decoded properly. It would be missing all the post gingerbread fields and all the extra fields from op-geth. I should probably add a comment to that effect.

palango commented 1 week ago

@palango its because gas limit zero is used to determine whether a block is pre or post gingerbread and so with a gas limit of zero the trasition block wouldn't be decoded properly. It would be missing all the post gingerbread fields and all the extra fields from op-geth. I should probably add a comment to that effect.

That makes sense. Yeah, please add a comment and we should also add a page to the spec mentioning all those details.

palango commented 1 week ago

Also, the base branch needs to be changed to celo7

piersy commented 1 week ago

@palango Added a ticket here for adding to the spec - https://github.com/celo-org/celo-blockchain-planning/issues/356

codecov-commenter commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.47%. Comparing base (9907e11) to head (6cedcfa).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## celo7 #179 +/- ## =========================================== + Coverage 61.32% 75.47% +14.15% =========================================== Files 20 16 -4 Lines 1753 1358 -395 Branches 71 12 -59 =========================================== - Hits 1075 1025 -50 + Misses 646 301 -345 Partials 32 32 ``` | [Flag](https://app.codecov.io/gh/celo-org/optimism/pull/179/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org) | Coverage Δ | | |---|---|---| | [cannon-go-tests](https://app.codecov.io/gh/celo-org/optimism/pull/179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org) | `81.03% <ø> (ø)` | | | [chain-mon-tests](https://app.codecov.io/gh/celo-org/optimism/pull/179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org) | `27.14% <ø> (ø)` | | | [sdk-tests](https://app.codecov.io/gh/celo-org/optimism/pull/179/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#carryforward-flags-in-the-pull-request-comment) to find out more. [see 4 files with indirect coverage changes](https://app.codecov.io/gh/celo-org/optimism/pull/179/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org)