Skip to content

fix: query shuffling from cache to compute proposer lookahead#7945

Closed
nflaig wants to merge 2 commits into
peerDASfrom
nflaig/fix-proposer-lookahead
Closed

fix: query shuffling from cache to compute proposer lookahead#7945
nflaig wants to merge 2 commits into
peerDASfrom
nflaig/fix-proposer-lookahead

Conversation

@nflaig

@nflaig nflaig commented Jun 10, 2025

Copy link
Copy Markdown
Member

No description provided.

@nflaig nflaig added the status-do-not-merge Merging this issue will break the build. Do not merge! label Jun 10, 2025
@codecov

codecov Bot commented Jun 10, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 8.69565% with 21 lines in your changes missing coverage. Please review.

Project coverage is 54.33%. Comparing base (d70dab2) to head (6f1dfe0).
Report is 58 commits behind head on peerDAS.

Additional details and impacted files
@@             Coverage Diff             @@
##           peerDAS    #7945      +/-   ##
===========================================
- Coverage    54.37%   54.33%   -0.05%     
===========================================
  Files          840      842       +2     
  Lines        61989    62077      +88     
  Branches      4654     4668      +14     
===========================================
+ Hits         33707    33727      +20     
- Misses       28214    28278      +64     
- Partials        68       72       +4     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wemeetagain

Copy link
Copy Markdown
Member

Lgtm, at least the metric usage bugfix

@nflaig

nflaig commented Jun 11, 2025

Copy link
Copy Markdown
Member Author

at least the metric usage bugfix

Should bring the minimal fixes to unstable branch #7948, this change needs more testing and is not critical for devnets as shuffling calculation is pretty cheap due to low validator count

const epoch = state.epochCtx.epoch + MIN_SEED_LOOKAHEAD + 1;

const shuffling =
state.epochCtx.shufflingCache?.getSync(epoch, cache.nextShufflingDecisionRoot, {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

don't need the last param because we handled computeEpochShuffling() below

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

so should we track metric separately? if we pass buildProps we get the metrics from shuffling cache

} else if (buildProps) {
// TODO: (@matthewkeil) This should possible log a warning??
this.metrics?.shufflingCache.shufflingPromiseNotResolvedAndThrownAway.inc();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

let's discuss here #7988 (comment), closing this PR

nflaig added a commit that referenced this pull request Jun 12, 2025
**Motivation**

Bring proposer lookahead fixes to unstable branch, does not include
shuffling calculation changes from
#7945 yet.

**Description**

- only use proposer lookahead from state if epoch is post fulu
- track epoch transition step time for proposer lookahead


Related #7902
KatyaRyazantseva pushed a commit to KatyaRyazantseva/lodestar that referenced this pull request Jun 19, 2025
**Motivation**

Bring proposer lookahead fixes to unstable branch, does not include
shuffling calculation changes from
ChainSafe#7945 yet.

**Description**

- only use proposer lookahead from state if epoch is post fulu
- track epoch transition step time for proposer lookahead


Related ChainSafe#7902
@nflaig

nflaig commented Jun 21, 2025

Copy link
Copy Markdown
Member Author

@twoeths should we just bring these changes to unstable and revisit metrics once we have a larger network (hoodi) with fulu enabled?

@nflaig

nflaig commented Jun 21, 2025

Copy link
Copy Markdown
Member Author

@twoeths should we just bring these changes to unstable and revisit metrics once we have a larger network (hoodi) with fulu enabled?

Opened a PR against unstable #7988

@nflaig

nflaig commented Jun 30, 2025

Copy link
Copy Markdown
Member Author

this branch was just for testing, closing it now

@nflaig nflaig closed this Jun 30, 2025
@nflaig nflaig deleted the nflaig/fix-proposer-lookahead branch June 30, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status-do-not-merge Merging this issue will break the build. Do not merge!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants