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.

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

Jump to bottom

fix: fix regression in getCurrentSlotAround #6407

Merged
merged 1 commit into from
Feb 7, 2024
Merged

fix: fix regression in getCurrentSlotAround #6407

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/validator/src/util/clock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export class Clock implements IClock {
*/
export function getCurrentSlotAround(config: ChainForkConfig, genesisTime: TimeSeconds): Slot {
const diffInSeconds = Date.now() / 1000 - genesisTime;
const slotsSinceGenesis = Math.floor(diffInSeconds / config.SECONDS_PER_SLOT);
const slotsSinceGenesis = Math.round(diffInSeconds / config.SECONDS_PER_SLOT);
Copy link
Contributor

@nazarhussain nazarhussain Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The tests data we have for this function suggests that for 12s slot window clock at 12.5 would still be zero slot. To align implementation with the tests data we have to use Math.floor here. Or else if that's not the case then we should change the tests data else it will be fail.

lodestar/packages/validator/test/unit/utils/clock.test.ts

Line 90 in 6a47911

{name: "should return next slot after 12.5s", delta: 12.5},

With changing back to round it will return nextSlot + 1 when the clock current time is around the slot boundary.

See this output
https://github.com/ChainSafe/lodestar/actions/runs/7652507863/job/20852823580#step:6:714

Copy link
Member

@nflaig nflaig Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But we are not applying Math.round on the delta, it is applied on the slots since genensis which is based on diff in seconds since genesis.

The goal of getCurrentSlotAround is clearly that it returns the closest slot, see PR that introduced this #2736.

Running those tests locally a few times I can see that those fail sometimes with Math.round and also with Math.floor

yarn vitest --run test/unit/utils/clock.test.ts -t getCurrentSlot
 FAIL  test/unit/utils/clock.test.ts > util / Clock > getCurrentSlot > 'should return next slot after 12.5s'
AssertionError: expected 8155649 to be 8155648 // Object.is equality

- Expected
+ Received

- 8155648
+ 8155649

When I use Math.floor the 12.5s test fails sometimes, and with Math.round the 11.5s test fails.

This is clearly an issue with the tests, and those need to be made more robust. A bit strange is that it only happens with vitest, with mocha those consistenly seem to pass. Weird timing issue I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the answer is to fix the test cases, not force the implementation to change for the buggy tests.
Merging this for now to fix the noticeable validator performance regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes test cases don't enforce implementation, but fixture data particularly guide what kind of behavior we expect form the implementation.

return GENESIS_SLOT + slotsSinceGenesis;
}

Expand Down
Loading