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

Fixes: rebalance decrease amount calculation, validator_lamport_balance initialization, tweaks to keeper #72

Merged
merged 9 commits into from
Aug 7, 2024

Conversation

ebatsell
Copy link
Collaborator

@ebatsell ebatsell commented Aug 5, 2024

Changes:

  • Fixes rebalance decrease: the stake pool's validator_list lamport amount can get out of alignment with the associated stake account's active lamports, resulting in more lamports being withdrawn from validators than is possible. The problem is fixed by passing along the true active stake amount into the decrease_validator_stake calculation
  • Unblocks unstaking for stake deposits by fixing how validator_lamport_balances are initialized. Currently, all validator_lamport_balances start at 0, meaning all current lamports are seen as stake deposits and tried to be unstaked until the cap is hit. However, we can load each value in a rebalance instruction when there are no activating or deactivating lamports and then continue with the working process. Adds a sentinel value and checks to skip modifying it when sentinel is present.
  • Will deploy on test program sssh4zkKhX8jXTNQz1xDHyGpygzgu2UhcRcUvZihBjP before deploying on mainnet program

@ebatsell ebatsell force-pushed the evan/rebalance-validator-list branch from 16a0751 to 1e703e2 Compare August 5, 2024 22:52
Copy link
Contributor

@CoachChuckFF CoachChuckFF left a comment

Choose a reason for hiding this comment

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

A couple of questions needed for clarification

@@ -245,6 +252,7 @@ impl UnstakeState {
// either to the target or to the previous balance before the deposit, whichever is lower in terms of total lamports unstaked
if current_lamports > state.validator_lamport_balances[index]
&& self.stake_deposit_unstake_total < self.stake_deposit_unstake_cap
&& state.validator_lamport_balances[index] != LAMPORT_BALANCE_DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the same check for increase_stake_calculation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the increase_stake_calculation algo here doesn't interact with this field so we're safe

Copy link
Contributor

Choose a reason for hiding this comment

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

10-4

@@ -967,15 +967,6 @@ impl StewardState {
RebalanceType::None
};

msg!("Reserve lamports after adjustment: {}", reserve_lamports);
Copy link
Contributor

Choose a reason for hiding this comment

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

Too much CU?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was causing local tests to fail since it was calling msg! outside of context, + think we're okay to remove these now since we have the emits

Copy link
Contributor

Choose a reason for hiding this comment

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

10-4

self.validator_lamport_balances[index] = self.validator_lamport_balances[index]
.checked_add(amount)
.ok_or(StewardError::ArithmeticError)?;
if self.validator_lamport_balances[index] != LAMPORT_BALANCE_DEFAULT {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this same check for Decrease?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, good catch

Copy link
Contributor

@CoachChuckFF CoachChuckFF left a comment

Choose a reason for hiding this comment

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

lgtm

@ebatsell ebatsell changed the title Fixes: rebalance decrease amount calculation, validator_lamport_balance initialization Fixes: rebalance decrease amount calculation, validator_lamport_balance initialization, tweaks to keeper Aug 7, 2024
@ebatsell ebatsell merged commit de7103b into master Aug 7, 2024
4 checks passed
@ebatsell ebatsell deleted the evan/rebalance-validator-list branch August 7, 2024 20:26
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.

2 participants