Skip to content

Withdraw inactive stake before unstaking #583

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Withdraw inactive stake before unstaking #583

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
13 changes: 10 additions & 3 deletions cli/maintainer/src/maintenance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,8 +1015,15 @@ impl SolidoState {
// of Lamports, we don't bother withdrawing. We try to do this
// so we don't pay more for fees than the amount that we'll
// withdraw. Or if we have stake to remove from unstake accounts.
if expected_difference_stake > SolidoState::MINIMUM_WITHDRAW_AMOUNT
|| removed_unstake > Lamports(0)
// If we're dealing with an inactive validator, we try to withraw
// all inactive stake.
let minimum_withdraw_amount = if validator.entry.active {
SolidoState::MINIMUM_WITHDRAW_AMOUNT
} else {
Lamports(0)
};

if expected_difference_stake > minimum_withdraw_amount || removed_unstake > Lamports(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will not pass if validator is inactive

if expected_difference_stake > minimum_withdraw_amount

minimum_withdraw_amount would be 0 also as expected_difference_stake, so 0 > 0 is false.
Take a look here
We should recompute effective stake balance after stake merging

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would expected_difference_stake be zero?

if validator is inactive

Let’s distinguish two things. There is the .active field on Validator. And there are the stake accounts. The case that was failing happens when the active field is false, but there is active stake in the stake accounts, which the maintainer tries to deactivate. So in this case, current_stake_balance definitely greater than zero. effective_stake_balance is the difference between the recorded balance in stake accounts (which will be greater than zero) and unstake accounts (which will still be zero). If there is excess stake to withdraw, then expected_difference_stake is going to be greater than zero.

Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you test this commit? I did. After stakes are merged the effective stake balance does not change by itself. It is changed only in WithdrawInactiveStake, but this is what we try to call here. Merging two stakes does not account additional rent exempt amount

{
// The balance of this validator is not up to date, try to update it.
let mut stake_account_addrs = Vec::new();
Expand Down Expand Up @@ -1610,11 +1617,11 @@ pub fn try_perform_maintenance(
// as possible.
.or_else(|| state.try_merge_on_all_stakes())
.or_else(|| state.try_update_exchange_rate())
.or_else(|| state.try_update_stake_account_balance())
.or_else(|| state.try_unstake_from_inactive_validator())
// Collecting validator fees goes after updating the exchange rate,
// because it may be rejected if the exchange rate is outdated.
// Same for updating the validator balance.
.or_else(|| state.try_update_stake_account_balance())
.or_else(|| state.try_deactivate_validator_if_commission_exceeds_max())
.or_else(|| state.try_stake_deposit())
.or_else(|| state.try_unstake_from_active_validators())
Expand Down