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

stake-pool: Add tolerance for stake accounts at minimum #3839

Merged
merged 8 commits into from
Dec 5, 2022

Conversation

joncinque
Copy link
Contributor

Problem

As the value of stake pool tokens increase, it could become impossible to reduce a stake account to its minimum value due to rounding.

Solution

This is a very tricky problem, but to give at least some cushion, round up to the lamports-per-pool-token value.

While working on this, I refactored the BigVec search to go through a closure rather than a function pointer, and it reduced compute unit consumption so much that the max pool size has increased by 50%!

joncinque added a commit to joncinque/solana-program-library that referenced this pull request Nov 29, 2022
joncinque added a commit that referenced this pull request Nov 29, 2022
* stake-pool: Refresh blockhash more often on update tests

* Bump down max pool size, updated correctly in #3839

* Move withdraw test to another file

* Refactor withdraw tests more

* Get more blockhashes during `update_stake_pool_balance`
@bennofs
Copy link

bennofs commented Dec 2, 2022

While doing the final review on this, I found one more case that is not handled yet:

  • there is an active, non-preferred validator with much stake
  • the preferred validator has only slightly more stake than min (less than lamports per pool token more)

Then the code forces you to withdraw from the preferred validator, which is impossible since even burning 1 token is too much to withdraw from that validator. All the other withdraw options are also not available because we still have the other, non-preferred validator forcing us to withdraw from an active validator.

@bennofs
Copy link

bennofs commented Dec 2, 2022

Here is a test case which fails and demonstrates the issue:

#[tokio::test]
async fn success_with_small_preferred_withdraw() {
    let (
        mut context,
        stake_pool_accounts,
        validator_stake,
        deposit_info,
        user_transfer_authority,
        user_stake_recipient,
        tokens_to_burn,
    ) = setup_for_withdraw(spl_token::id()).await;

    // make pool tokens very valuable, so it isn't possible to exactly get down to the minimum
    transfer(
        &mut context.banks_client,
        &context.payer,
        &context.last_blockhash,
        &stake_pool_accounts.reserve_stake.pubkey(),
        deposit_info.stake_lamports * 5, // each pool token is worth more than one lamport
    )
    .await;
    stake_pool_accounts
        .update_all(
            &mut context.banks_client,
            &context.payer,
            &context.last_blockhash,
            &[validator_stake.vote.pubkey()],
            false,
        )
        .await;

    let preferred_validator = simple_add_validator_to_pool(
        &mut context.banks_client,
        &context.payer,
        &context.last_blockhash,
        &stake_pool_accounts,
        None,
    )
    .await;

    stake_pool_accounts
        .set_preferred_validator(
            &mut context.banks_client,
            &context.payer,
            &context.last_blockhash,
            instruction::PreferredValidatorType::Withdraw,
            Some(preferred_validator.vote.pubkey()),
        )
        .await;

    println!("{}",  get_account(&mut context.banks_client, &preferred_validator.stake_account).await.lamports);


    // add a tiny bit of stake, less than lamports per pool token to preferred validator
    let preferred_deposit = simple_deposit_stake(
        &mut context.banks_client,
        &context.payer,
        &context.last_blockhash,
        &stake_pool_accounts,
        &preferred_validator,
        2_000_000_000 + 1,
    )
    .await
    .unwrap();

    println!("{}",  get_account(&mut context.banks_client, &preferred_validator.stake_account).await.lamports);
    let rent = context.banks_client.get_rent().await.unwrap();
    let rent_exempt = rent.minimum_balance(std::mem::size_of::<stake::state::StakeState>());
    // decrease a little stake, not all
    let error = stake_pool_accounts
        .decrease_validator_stake(
            &mut context.banks_client,
            &context.payer,
            &context.last_blockhash,
            &preferred_validator.stake_account,
            &preferred_validator.transient_stake_account,
            2_000_000_000 + rent_exempt,
            preferred_validator.transient_stake_seed,
        )
        .await;
    assert!(error.is_none());   
    println!("{}",  get_account(&mut context.banks_client, &preferred_validator.stake_account).await.lamports);

    // warp forward to deactivation
    let first_normal_slot = context.genesis_config().epoch_schedule.first_normal_slot;
    let slots_per_epoch = context.genesis_config().epoch_schedule.slots_per_epoch;
    context
        .warp_to_slot(first_normal_slot + slots_per_epoch)
        .unwrap();

    // update to merge deactivated stake into reserve
    stake_pool_accounts
        .update_all(
            &mut context.banks_client,
            &context.payer,
            &context.last_blockhash,
            &[validator_stake.vote.pubkey(), preferred_validator.vote.pubkey()],
            false,
        )
        .await;

    // withdraw from preferred fails
    let new_authority = Pubkey::new_unique();
    let error = stake_pool_accounts
        .withdraw_stake(
            &mut context.banks_client,
            &context.payer,
            &context.last_blockhash,
            &user_stake_recipient.pubkey(),
            &user_transfer_authority,
            &deposit_info.pool_account.pubkey(),
            &preferred_validator.stake_account,
            &new_authority,
            1,
        )
        .await;
    assert!(error.is_some());

    // preferred is empty, withdrawing from non-preferred works
    let new_authority = Pubkey::new_unique();
    let error = stake_pool_accounts
        .withdraw_stake(
            &mut context.banks_client,
            &context.payer,
            &context.last_blockhash,
            &user_stake_recipient.pubkey(),
            &user_transfer_authority,
            &deposit_info.pool_account.pubkey(),
            &validator_stake.stake_account,
            &new_authority,
            tokens_to_burn / 6,
        )
        .await;
    assert!(error.is_none());
}

// `stake_rent + minimum_delegation + lamports_per_pool_token * 2 - 2`.
// We give an extra grace on this check of two lamports, which should be
// reasonable. At worst, it just means that a withdrawer is losing out
// on two lamports.
Copy link

Choose a reason for hiding this comment

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

I am curious, I think there is something I am missing here. We are comparing against the actual lamports in the stake account here, so a tolerance of +lamports_per_token should always be enough.

For example, let's assume we only reduce the stake account to minimum_stake_lamports + (lamports_per_token - 1). But then split_from_lamports = minimum_stake_lamports +(lamports_per_token - 1) already, and we can always find a token burn amount so that we are within +lamports_per_token of that. No need to double, as far as I understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I just wrote the test poorly during my first pass. No need for the limit, thanks for taking the time to notice this!

@joncinque
Copy link
Contributor Author

While doing the final review on this, I found one more case that is not handled yet:

Thanks for noticing this and providing the test case, I've put in the test and the fix.

With that, everything's been addressed from your most recent review.

Copy link

@bennofs bennofs left a comment

Choose a reason for hiding this comment

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

Looks good to me now

@joncinque joncinque merged commit da37530 into solana-labs:master Dec 5, 2022
@joncinque joncinque deleted the sp-tol branch December 5, 2022 11:23
HaoranYi pushed a commit to HaoranYi/solana-program-library that referenced this pull request Jul 19, 2023
…#3853)

* stake-pool: Refresh blockhash more often on update tests

* Bump down max pool size, updated correctly in solana-labs#3839

* Move withdraw test to another file

* Refactor withdraw tests more

* Get more blockhashes during `update_stake_pool_balance`
HaoranYi pushed a commit to HaoranYi/solana-program-library that referenced this pull request Jul 19, 2023
…3839)

* stake-pool: Add tolerance for stake accounts at minimum

* Use test-case to check more cases

* Add more tolerance on withdrawal

* Potentially fix test per solana-labs#3854

* Keep throwing solutions until CI passes

* Fix repeated transaction issue

* Fix preferred withdrawal tolerance too

* Remove doubled tolerance
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