Skip to content

chore: stabilize fastConfirmation perf bench #9494

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.

Sign up for GitHub

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
nazarhussain merged 3 commits into from
Jun 10, 2026
Merged

chore: stabilize fastConfirmation perf bench #9494

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f3d194
fix: stabilize fastConfirmation perf bench
nazarhussain Jun 9, 2026
3c5c63e
fix: pass vote.slot (not nextEpoch) to addLatestMessage in everyoneVo…
lodekeeper Jun 10, 2026
c300a91
Merge branch 'unstable' into nh/fcr-benchmark-fix
nazarhussain Jun 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 14 additions & 23 deletions packages/fork-choice/test/perf/forkChoice/fastConfirmation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {bench, describe} from "@chainsafe/benchmark";
import {SLOTS_PER_EPOCH} from "@lodestar/params";
import {computeEpochAtSlot} from "@lodestar/state-transition";
import {Slot} from "@lodestar/types";
import {
buildFastConfirmationSnapshot,
Expand Down Expand Up @@ -41,10 +40,8 @@ function runFCRRulesBenchmark(opts: Opts): void {
const forkChoice = initializeForkChoice({...opts, fastConfirmation: true});

const head = forkChoice.updateHead();
const prevBlock = forkChoice.getBlockHexDefaultStatus(head.parentRoot);
if (!prevBlock) throw Error("no prevBlock");

// Vote everyone for head
// Vote everyone for head so the FCR vote-map paths see a populated voteNextIndices.

@gemini-code-assist gemini-code-assist Bot Jun 9, 2026

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.

high

There is a correctness bug in the helper function everyoneVotes (defined at the bottom of this file) that is called here.\n\nIn everyoneVotes, nextEpoch is passed as the second argument to forkChoice["addLatestMessage"]:\ntypescript\nfunction everyoneVotes(vote: ProtoBlock, forkChoice: ForkChoice): void {\n const nextEpoch = computeEpochAtSlot(vote.slot);\n const nextRoot = vote.blockRoot;\n for (let i = 0; i < forkChoice["balances"].length; i++) {\n forkChoice["addLatestMessage"](i, nextEpoch, nextRoot, PayloadStatus.FULL);\n }\n}\n\n\nHowever, addLatestMessage expects a Slot as its second parameter, not an Epoch:\ntypescript\n private addLatestMessage(\n validatorIndex: ValidatorIndex,\n nextSlot: Slot,\n nextRoot: RootHex,\n nextPayloadStatus: PayloadStatus\n ): void\n\n\nBecause an epoch number (e.g., 2) is passed instead of a slot number (e.g., 64), addLatestMessage internally computes the epoch of the vote as computeEpochAtSlot(2), which evaluates to 0.\n\nThis causes all validator votes to be registered as belonging to epoch 0 (instead of epoch 2). Since the benchmark advances the slot to epoch 3, these votes are treated as expired/inactive by the Fast Confirmation Rules (FCR), meaning the benchmark is not actually executing the realistic code paths where active votes are processed and counted.\n\n### Suggested Fix\nUpdate everyoneVotes to pass vote.slot instead of nextEpoch:\ntypescript\nfunction everyoneVotes(vote: ProtoBlock, forkChoice: ForkChoice): void {\n const nextSlot = vote.slot;\n const nextRoot = vote.blockRoot;\n for (let i = 0; i < forkChoice["balances"].length; i++) {\n forkChoice["addLatestMessage"](i, nextSlot, nextRoot, PayloadStatus.FULL);\n }\n}\n

@lodekeeper lodekeeper Jun 9, 2026

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.

Confirming Gemini's analysis: addLatestMessage signature is (validatorIndex, nextSlot, ...) and internally calls computeEpochAtSlot(nextSlot) to determine vote epoch. With vc:600000 bc:96, the head's vote.slot=96 → nextEpoch=3 is passed as nextSlot=3, so voteNextSlots[i]=3 and the internal epoch resolves to computeEpochAtSlot(3)=0. After updateTime(97) (epoch 3), all votes look like they belong to epoch 0 — i.e. stale by 3 epochs. The realistic path through the FCR vote-map paths is not being exercised.

Fix is one line as Gemini suggested:

forkChoice["addLatestMessage"](i, vote.slot, nextRoot, PayloadStatus.FULL);

cc @gemini-code-assist — good catch. @nazarhussain — the GC-pressure removal still delivers the flakiness fix on its own, but this means the benchmarked code path isn't faithfully representing real validator state. Worth folding into this PR before merge so the bench numbers actually reflect FCR-active votes.

@nazarhussain nazarhussain Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed in 99872b5. vote.slot is now passed directly; the now-unused computeEpochAtSlot import was dropped. Verified via local bench run (still stable, ~80K–135K samples/config).

Note: this is independent of the GC-pressure flakiness fix in the first commit — fn() at currentSlot=97 short-circuits at Rule 4 (confirmedEpoch+1 < currentEpoch after Rule 2 resets to genesis) so it never reaches the vote-map path either way. But fixing the bug now means the future state-backed bench (planned follow-up) will start from a correct setup.

@lodekeeper lodekeeper Jun 17, 2026

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.

👍 Confirmed — 99872b50ca matches what Gemini and I flagged: vote.slot is passed straight to addLatestMessage and the now-unused computeEpochAtSlot import is gone. Thanks for closing it out.

everyoneVotes(head, forkChoice);

// Advance to second slot of epoch 2 (not epoch boundary)
Expand All @@ -60,25 +57,17 @@ function runFCRRulesBenchmark(opts: Opts): void {
const store = forkChoice["fcStore"] as unknown as IFastConfirmationStore;
if (!ctx) throw Error("fastConfirmationContext not initialized");

return {forkChoice, ctx, store, head, prevBlock, currentVoteIs1: true, currentSlot};
},
beforeEach: (data) => {
// Flip votes to force fresh vote map computation each iteration
const target = data.currentVoteIs1 ? data.prevBlock : data.head;
everyoneVotes(target, data.forkChoice);
data.currentVoteIs1 = !data.currentVoteIs1;
data.currentSlot = (data.currentSlot + 1) as Slot;

// Advance the slot (this calls updateHead + processAttestationQueue but NOT the FCR)
// We need to advance the slot so getCurrentSlot() returns the right value
data.forkChoice["fcStore"].currentSlot = data.currentSlot;
// Apply votes to proto array so getHead/isDescendant work correctly
data.forkChoice.updateHead();

return data;
return {ctx, store};
},
// Pass-through: the bench harness invokes fn(beforeEach(inputAll)), so without a
// beforeEach fn would receive undefined. store is not mutated by the rule runner so
// no per-iteration reset is required.
beforeEach: (data) => data,
Comment on lines 61 to +65

@gemini-code-assist gemini-code-assist Bot Jun 9, 2026

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.

medium

The beforeEach pass-through and its accompanying comment are redundant.\n\nIn @chainsafe/benchmark, if beforeEach is omitted, the benchmark runner automatically defaults to passing the data returned by before directly to fn. We can safely remove this block to simplify the benchmark setup.

Suggested change
},
// Pass-through: the bench harness invokes fn(beforeEach(inputAll)), so without a
// beforeEach fn would receive undefined. store is not mutated by the rule runner so
// no per-iteration reset is required.
beforeEach: (data) => data,
},

@lodekeeper lodekeeper Jun 9, 2026

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.

Pushing back on this one — checked @chainsafe/benchmark's runBenchmarkFn.js:

const input = beforeEach ? await beforeEach(inputAll, runIdx) : undefined;

When beforeEach is omitted, fn receives undefined — not the data from before. With the pass-through removed, fn: ({ctx, store}) => ... would destructure undefined and throw on every iteration.

The comment is there precisely because the pass-through looks redundant at a glance but isn't. Suggestion to remove it would break the benchmark. cc @gemini-code-assist.

@nazarhussain — leaving this one as-is, comment is accurate.

@gemini-code-assist gemini-code-assist Bot Jun 9, 2026

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.

Thank you for the clarification, @lodekeeper. You are absolutely correct; my previous assessment regarding the redundancy of the beforeEach pass-through was based on an incorrect assumption about the benchmark runner's default behavior when beforeEach is omitted. I appreciate you pointing out that fn would receive undefined in that case, which would indeed cause the benchmark to fail. The current implementation is necessary for the benchmark to function correctly.

@lodekeeper lodekeeper Jun 9, 2026

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.

Thanks for confirming.

@nazarhussain nazarhussain Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree with @lodekeeper — keeping the pass-through. Confirmed in @chainsafe/benchmark's runBenchmarkFn.js (const input = beforeEach ? await beforeEach(inputAll, runIdx) : undefined): without beforeEach, fn would receive undefined and the destructure on {ctx, store} would throw on every iteration. The comment block above the function is there precisely to flag this for the next reader.

@lodekeeper lodekeeper Jun 17, 2026

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.

👍 Thanks for confirming. Agreed — the beforeEach pass-through is load-bearing: without it @chainsafe/benchmark hands fn undefined and the {ctx, store} destructure throws. Good to keep.

fn: ({ctx, store}) => {
// Measure ONLY the FCR rules — build cache + snapshot + run rules
// Measure ONLY the FCR rules — build cache + snapshot + run rules.
// store is not mutated by runFastConfirmationRules (only the rule runner's caller
// would assign store.confirmedRoot), so each iteration replays the same work and
// no beforeEach reset is needed.
const cache = createFastConfirmationCache();
const snapshot = buildFastConfirmationSnapshot(ctx, store, cache);
runFastConfirmationRules(snapshot, ctx, store, cache);
Expand All @@ -87,9 +76,11 @@ function runFCRRulesBenchmark(opts: Opts): void {
}

function everyoneVotes(vote: ProtoBlock, forkChoice: ForkChoice): void {
const nextEpoch = computeEpochAtSlot(vote.slot);
const nextRoot = vote.blockRoot;
for (let i = 0; i < forkChoice["balances"].length; i++) {
forkChoice["addLatestMessage"](i, nextEpoch, nextRoot, PayloadStatus.FULL);
// addLatestMessage's second arg is `nextSlot: Slot`, not an epoch — pass vote.slot directly so
// the internal computeEpochAtSlot(nextSlot) lands on the correct vote epoch instead of
// computeEpochAtSlot(epoch)=0 making every vote look 3 epochs stale.
forkChoice["addLatestMessage"](i, vote.slot, nextRoot, PayloadStatus.FULL);
}
}
Loading