-
-
Notifications
You must be signed in to change notification settings - Fork 479
fix: use parent block slot for attestation payload availability #9731
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
Changes from all commits
42b9998
efc6acc
695e6d2
be537e5
a51c0b7
bec59bb
cecdc37
08e30b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,10 @@ export const defaultSkipOpts: SkipOpts = { | |
| // New test suite added in v1.7.0-alpha.8 (consensus-specs #5206); gloas PTC fork choice | ||
| // handling is not yet implemented in Lodestar. | ||
| /^gloas\/fork_choice\/on_payload_attestation_message\/.*$/, | ||
| // TODO-GLOAS: re-enable after the gloas should_apply_proposer_boost rule is implemented. | ||
| // New test suite added in v1.7.0-alpha.13 (consensus-specs #5441); Lodestar still applies | ||
| // the pre-gloas proposer boost, so the head weight differs by the boost amount. | ||
| /^gloas\/fork_choice\/should_apply_proposer_boost\/.*$/, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ensi321 looks like we are failing the new tests you added in ethereum/consensus-specs#5441
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional. Supposed to pass with #9233 |
||
| // TODO GLOAS: enable this after gloas fork choice is ready | ||
| /^gloas\/fork_choice_compliance\/.*/, | ||
| ], | ||
|
|
@@ -93,7 +97,8 @@ export const defaultSkipOpts: SkipOpts = { | |
| // TODO GLOAS: Proposer-boost dependent-root gate uses stale cached head across epoch-boundary ticks; | ||
| // boost wrongly denied. Fails identically on every pre-gloas fork. | ||
| // Enable this after https://github.com/ChainSafe/lodestar/issues/9666 is resolved | ||
| /fork_choice_compliance\/block_tree_test\/pyspec_tests\/block_tree_test_16_201284350_1$/, | ||
| // The case name embeds the generation seed, so it changes whenever comptests are regenerated. | ||
| /fork_choice_compliance\/block_tree_test\/pyspec_tests\/block_tree_test_17_381675768_1$/, | ||
| ], | ||
| // TODO GLOAS: Investigate why networking tests are failing since alpha.5 | ||
| skippedRunners: ["networking"], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,17 @@ import { | |
| WHISTLEBLOWER_REWARD_QUOTIENT, | ||
| WHISTLEBLOWER_REWARD_QUOTIENT_ELECTRA, | ||
| isForkPostElectra, | ||
| isForkPostGloas, | ||
| } from "@lodestar/params"; | ||
| import {BeaconBlock, altair, phase0, rewards} from "@lodestar/types"; | ||
| import {BeaconBlock, Slot, altair, phase0, rewards} from "@lodestar/types"; | ||
| import {processAttestationsAltair} from "../block/processAttestationsAltair.js"; | ||
| import {RewardCache} from "../cache/rewardCache.js"; | ||
| import {CachedBeaconStateAllForks, CachedBeaconStateAltair, CachedBeaconStatePhase0} from "../cache/stateCache.js"; | ||
| import { | ||
| CachedBeaconStateAllForks, | ||
| CachedBeaconStateAltair, | ||
| CachedBeaconStateGloas, | ||
| CachedBeaconStatePhase0, | ||
| } from "../cache/stateCache.js"; | ||
| import {getAttesterSlashableIndices} from "../util/attestation.js"; | ||
|
|
||
| type SubRewardValue = number; // All reward values should be integer | ||
|
|
@@ -36,10 +42,18 @@ export async function computeBlockRewards( | |
| let syncAggregateReward = cachedSyncAggregateReward; | ||
|
|
||
| if (blockAttestationReward === 0) { | ||
| const parentSlot = isForkPostGloas(fork) | ||
| ? (preState as CachedBeaconStateGloas).latestExecutionPayloadBid.slot | ||
| : null; | ||
| blockAttestationReward = | ||
| fork === ForkName.phase0 | ||
| ? computeBlockAttestationRewardPhase0(block as phase0.BeaconBlock, preState as CachedBeaconStatePhase0) | ||
| : computeBlockAttestationRewardAltair(config, block as altair.BeaconBlock, preState as CachedBeaconStateAltair); | ||
| : computeBlockAttestationRewardAltair( | ||
| config, | ||
| block as altair.BeaconBlock, | ||
| preState as CachedBeaconStateAltair, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't apply parent execution payload for preState so executionPayloadAvailability passed in is not correct
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I follow correctly, maybe you can make a proposal for that, if you are looking into that right now, can target my branch, I am not sure how to merge this unless we skip a lot of spec tests
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in #9736 |
||
| parentSlot | ||
| ); | ||
| } | ||
|
|
||
| if (syncAggregateReward === 0) { | ||
|
|
@@ -79,12 +93,13 @@ function computeBlockAttestationRewardPhase0( | |
| function computeBlockAttestationRewardAltair( | ||
| config: BeaconConfig, | ||
| block: altair.BeaconBlock, | ||
| preState: CachedBeaconStateAltair | ||
| preState: CachedBeaconStateAltair, | ||
| parentSlot: Slot | null | ||
| ): SubRewardValue { | ||
| const fork = config.getForkSeq(block.slot); | ||
| const {attestations} = block.body; | ||
|
|
||
| processAttestationsAltair(fork, preState, attestations, false); | ||
| processAttestationsAltair(fork, preState, attestations, parentSlot, false); | ||
|
|
||
| return preState.proposerRewards.attestations; | ||
| } | ||
|
|
||
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.
@nazarhussain this might be an issue with the test harness? in any case, I would like to keep this out of this PR, so skipping for now