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

v2.0: runtime: use leader schedule epoch to serve sol_get_epoch_stake (backport of #3279) #3312

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Oct 25, 2024

Problem

The bank epoch stakes cache stores stakes according to the leader schedule epoch, which means the epoch stake for a given epoch N actually corresponds to the N + 1 epoch in the epoch stakes cache.

The sol_get_epoch_stake syscall is designed to vend to on-chain programs the epoch stakes for the current epoch, but the existing implementation is returning the epoch stakes for leader schedule epoch N, when it actually should return the epoch stakes for leader schedule epoch N + 1.

For example, if you activate stake in epoch N, it becomes active in epoch N + 1, but the existing bank implementation for serving the sol_get_epoch_stakes syscall actually wouldn't return this new stake until epoch N + 2.

Summary of Changes

Use Bank's current_epoch_stakes method - which fetches the leader schedule epoch bank.epoch() + 1 - to serve the epoch stake information for sol_get_epoch_stakes, properly returning the current epoch stake.


This is an automatic backport of pull request #3279 done by Mergify.

@mergify mergify bot requested a review from a team as a code owner October 25, 2024 13:20
@mergify mergify bot added the conflicts label Oct 25, 2024

This comment was marked as resolved.

@buffalojoec
Copy link

buffalojoec commented Oct 25, 2024

As specified in the backport instructions, I've only included the change, and not the additional tests or the BPF program tests. Lmk if anyone wants those added as well.

Case for backport: fixes a bug in the SVM API for accessing the epoch stakes cache, done through the sol_get_epoch_stake syscall. Small change, significant bugfix.

@bw-solana
Copy link

I'm probably missing something, but couldn't this break consensus? Does it need a feature gate?

@jstarry
Copy link

jstarry commented Oct 28, 2024

I'm probably missing something, but couldn't this break consensus? Does it need a feature gate?

This change is already behind the enable_get_epoch_stake_syscall feature gate which hasn't been activated yet. So this bug fix is fixing unactivated code in case that wasn't clear.

@bw-solana
Copy link

This change is already behind the enable_get_epoch_stake_syscall feature gate which hasn't been activated yet. So this bug fix is fixing unactivated code in case that wasn't clear.

Ah, I knew I was missing something. Thanks Justin!

Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

LGTM

@buffalojoec buffalojoec merged commit 6d74d13 into v2.0 Oct 29, 2024
39 checks passed
@buffalojoec buffalojoec deleted the mergify/bp/v2.0/pr-3279 branch October 29, 2024 06:36
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.

3 participants