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

chore: fix CI failure due to recent merge from unstable #6646

Merged
merged 7 commits into from
Apr 12, 2024

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Apr 6, 2024

Fix CI errors on electra-fork branch

Motivation

Currently electra-fork branch has some CIs not passing due to the recent merge from unstable. This includes

  1. Lint fail - electra-interop.test.ts imports lodash package, which was removed in unstable. See the job log for detail
  2. Check types fail - blockRewards.test.ts has relies on generatePerfTestCachedStateAltair() to generate CachedBeaconStateAltair (I know this is bad and we need to fix this soon). CachedBeaconStateAltair is later casted as CachedBeaconStateAllForks. This is due to a private property named historicalValidatorLengths being added to EpochCache. See the job log
  3. Spec test fail - We are using v1.4.0-beta.6 for spec test which contains experimental fork eip6110. In electra-fork we remove eip6110 fork and 6110 related code to fork electra. Spec test failed because it is looking for fork eip6110 See the job log
  4. e2e test fail - Attempt to download presets from electra fork, which doesn't exist until later spec version

Description

  1. Cease lodash dependency in electra-interop.test.ts
  2. Remove private modifier for Epoch.historicalValidatorLengths
  3. Ignore eip6110 spec test

Long Term Solution

  1. Stops blockRewards.test.ts from using generatePerfTestCachedStateAltair() when there is a better utility class to generate a well-formed state
  2. Bump spec test version - Recent spec test release removes eip6110 fork and adds electra fork

@nflaig nflaig changed the title fix: fix CI failure due to recent merge from unstable ci: fix failure due to recent merge from unstable Apr 6, 2024
@nflaig nflaig changed the title ci: fix failure due to recent merge from unstable chore: fix CI failure due to recent merge from unstable Apr 6, 2024
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

❗ No coverage uploaded for pull request base (electra-fork@473a04f). Click here to learn what that means.
The diff coverage is 100.00%.

Additional details and impacted files
@@               Coverage Diff               @@
##             electra-fork    #6646   +/-   ##
===============================================
  Coverage                ?   61.97%           
===============================================
  Files                   ?      562           
  Lines                   ?    59844           
  Branches                ?     1905           
===============================================
  Hits                    ?    37089           
  Misses                  ?    22714           
  Partials                ?       41           

@ensi321 ensi321 marked this pull request as ready for review April 6, 2024 14:42
@ensi321 ensi321 requested a review from a team as a code owner April 6, 2024 14:42
@twoeths
Copy link
Contributor

twoeths commented Apr 8, 2024

@ensi321 could you clarify which errors and link to the failed CI

const calculatedBlockReward = await computeBlockRewards(block.message, state as CachedBeaconStateAllForks);
const calculatedBlockReward = await computeBlockRewards(
block.message,
state as unknown as CachedBeaconStateAllForks

This comment was marked as off-topic.

Copy link
Member

@nflaig nflaig Apr 9, 2024

Choose a reason for hiding this comment

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

But jokes aside, what happened on electra branch that states between forks can't even be type casted anymore? Doing as unknown as someOtherType is almost the same as just doing as any which means we have disabled typescript at this point. It might be required in some rare cases but should always add a comment to clarify why this is needed

On unstable branch this is not an issue

const postState = stateTransition(preState as CachedBeaconStateAllForks, block, {

so not sure if this is the right fix here

Copy link
Member

Choose a reason for hiding this comment

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

I looked a this locally and don't see the issue, probably not a blocker to merge this but needs to be investigated sooner or later

Copy link
Member

Choose a reason for hiding this comment

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

In electra-fork, CachedBeaonStateElectra is introduced to CachedBeaconStateAllForks which adds more difference, this is why check-types complains about "insufficiently overlaps". See the job log

Not sure if this explains it, Deneb, or even Bellatrix state have sufficient difference, adding more properties shouldn't break the type cast. I would assume it breaks if you modify or remove a property

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, should add a comment to look into that later. Otherwise I'm afraid the same pattern will be applied everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In electra-fork, CachedBeaonStateElectra is introduced to CachedBeaconStateAllForks which adds more difference, this is why check-types complains about "insufficiently overlaps". See the job log

Not sure if this explains it, Deneb, or even Bellatrix state have sufficient difference, adding more properties shouldn't break the type cast. I would assume it breaks if you modify or remove a property

I just spent some time looking into this. It looks like it's because in electra-fork branch there is a newly added field in EpochCache named private historialValidatorLengths.

So when casting from CachedBeaconStateAltair to CachedBeaconStateAllForks, instead of comparing the two structurally, it's now comparing nominally because of the private property.

I just removed the private modifier of historialValidatorLengths in favour of cleaner code.

Sorry for the incorrect explanation I previous made

Copy link
Member

Choose a reason for hiding this comment

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

So when casting from CachedBeaconStateAltair to CachedBeaconStateAllForks, instead of comparing the two structurally, it's now comparing nominally because of the private property.

Thanks for investigating that, this is much better than having to cast as unknown. Why marking the property as private causes issues is a bit strange though, it should only check the pubic interface.

Might be related to a typescript bug microsoft/TypeScript#37621, @matthewkeil this is similar to what you observed on the shuffling refactor branch?

@ensi321
Copy link
Contributor Author

ensi321 commented Apr 9, 2024

@ensi321 could you clarify which errors and link to the failed CI

@tuyennhv My bad, should've clarified it when opening the PR. Updated the PR description.

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

LGTM

@nflaig
Copy link
Member

nflaig commented Apr 11, 2024

e2e tests are failing due to the fork config not being available on github yet

url: `https://raw.githubusercontent.com/ethereum/consensus-specs/${commit}/presets/${preset}/${forkName}.yaml`,

See https://github.com/ChainSafe/lodestar/actions/runs/8645222684/job/23702584714?pr=6646#step:6:35

@g11tech g11tech force-pushed the electra-fork branch 2 times, most recently from 8a4f66e to 473a04f Compare April 11, 2024 11:12
@ensi321
Copy link
Contributor Author

ensi321 commented Apr 12, 2024

e2e tests are failing due to the fork config not being available on github yet

url: `https://raw.githubusercontent.com/ethereum/consensus-specs/${commit}/presets/${preset}/${forkName}.yaml`,

See https://github.com/ChainSafe/lodestar/actions/runs/8645222684/job/23702584714?pr=6646#step:6:35

Yea the error should go away when we upgrade our spec test version

@ensi321 ensi321 merged commit 5808ddb into electra-fork Apr 12, 2024
16 of 17 checks passed
@ensi321 ensi321 deleted the nc/electra-fork-ci-fix branch April 12, 2024 15:38
g11tech pushed a commit to g11tech/lodestar that referenced this pull request Jun 19, 2024
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.

None yet

3 participants