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

Rpc: make inactive_stake consistent in getStakeActivation #35116

Closed
wants to merge 4 commits into from
Closed

Rpc: make inactive_stake consistent in getStakeActivation #35116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a58f52
Make inactive_stake consistent
Feb 6, 2024
c0563e4
Add rpc_deprecated_v1_18 module
Feb 7, 2024
18bd63c
Move get_stake_activation to deprecated list
Feb 7, 2024
51039cb
Fix typo
Feb 7, 2024
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
74 changes: 41 additions & 33 deletions rpc/src/rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1786,16 +1786,10 @@ impl JsonRpcRequestProcessor {
} else {
StakeActivationState::Inactive
};
let inactive_stake = match stake_activation_state {
StakeActivationState::Activating => activating,
StakeActivationState::Active => 0,
StakeActivationState::Deactivating => stake_account
.lamports()
.saturating_sub(effective + rent_exempt_reserve),
StakeActivationState::Inactive => {
stake_account.lamports().saturating_sub(rent_exempt_reserve)
}
};
let inactive_stake = stake_account
.lamports()
Copy link
Contributor

Choose a reason for hiding this comment

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

mmm... won't this be misleading in a situation where lamports are deposited while a delegation is incurring a multi-epoch warmup/cooldown? since those lamports are never going to be active without first deactivating the delegation.

i think considering lamports() for anything purporting to be "stake" is probably wrong

it seems like what we really want to communicate here is something like withrawable_balance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talk about this a little on the issue that was created: #35111 (comment)
In one sense, it is misleading, but I actually don't think it's incorrect to say that the additional lamports are inactive even though they will not ever be activated (without first deactivating).

The stake: deactivating case already reports extra lamports as inactive, so there is an inconsistency here that ought to be fixed, regardless. I suppose another way to do that would be to update the deactivating case to calculate the inactive from the Delegation amount instead of the account balance. But that seems less helpful to me.

All that said, since we no longer support past epochs for this method (and since we don't return the actual activating/deactivating amounts), users can figure out everything this method returns by inspecting the stake account.

Copy link
Contributor

Choose a reason for hiding this comment

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

I talk about this a little on the issue that was created: #35111 (comment)

eh... my gut says "reporter is holding it wrong". i'm not a huge fan of reinforcing more wrong conceptual interpretations. we have too many already. we screwed ourselves by synonymizing stake and delegation.

In one sense, it is misleading, but I actually don't think it's incorrect to say that the additional lamports are inactive even though they will not ever be activated (without first deactivating).

i don't disagree that excess lamports in a stake account are inactive. i just don't agree that they are "stake" in the first place.

The stake: deactivating case already reports extra lamports as inactive, so there is an inconsistency here that ought to be fixed, regardless. I suppose another way to do that would be to update the deactivating case to calculate the inactive from the Delegation amount instead of the account balance. But that seems less helpful to me.

yeah this is kinda what i was getting at here:

i think considering lamports() for anything purporting to be "stake" is probably wrong

removing lamports() from any of these calculations seems like it would only improve the accuracy of the response. i'd rather answer "why don't my excess lamports show up in the getStakeActivation?" and correct the wrong expectation than further muddy it

All that said, since we no longer support past epochs for this method (and since we don't return the actual activating/deactivating amounts), users can figure out everything this method returns by inspecting the stake account.

so deprecate this method and call it a day? :trollface:

Copy link

@xdzurman xdzurman 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.

i don't disagree that excess lamports in a stake account are inactive. i just don't agree that they are "stake" in the first place.

  1. Why would they not be stake? They are funds on a stake account.
  2. Why would you consider it stake, if the funds were sent to an inactive/deactivating stake account (they would be shown in inactive_stake) but not if the funds were sent to an active/activating stake account (they're hidden)?

yeah this is kinda what i was getting at here:

As carrot guy said, that would be far less helpful and confusing to handle in my opinion too

it seems like what we really want to communicate here is something like withrawable_balance?

I guess that would kinda fix the issue, but it would create even more confusion as to what is considered stake, what is not, what is withdrawable, whether withdrawable === inactive in some cases and not in others, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so deprecate this method and call it a day? :trollface:

Yes, I'm actually fine with that, but I would like to fix the inconsistency as well. My preferred fix is the one in this PR, so I will add a deprecation commit (although deprecation communication mostly happens on the docs side).

.saturating_sub(effective)
.saturating_sub(rent_exempt_reserve);
Ok(RpcStakeActivation {
state: stake_activation_state,
active: effective,
Expand Down Expand Up @@ -2982,14 +2976,6 @@ pub mod rpc_accounts {
block: Slot,
) -> Result<RpcBlockCommitment<BlockCommitmentArray>>;

#[rpc(meta, name = "getStakeActivation")]
fn get_stake_activation(
&self,
meta: Self::Metadata,
pubkey_str: String,
config: Option<RpcEpochConfig>,
) -> Result<RpcStakeActivation>;

// SPL Token-specific RPC endpoints
// See https://github.com/solana-labs/solana-program-library/releases/tag/token-v2.0.0 for
// program details
Expand Down Expand Up @@ -3062,20 +3048,6 @@ pub mod rpc_accounts {
Ok(meta.get_block_commitment(block))
}

fn get_stake_activation(
&self,
meta: Self::Metadata,
pubkey_str: String,
config: Option<RpcEpochConfig>,
) -> Result<RpcStakeActivation> {
debug!(
"get_stake_activation rpc request received: {:?}",
pubkey_str
);
let pubkey = verify_pubkey(&pubkey_str)?;
meta.get_stake_activation(&pubkey, config)
}

fn get_token_account_balance(
&self,
meta: Self::Metadata,
Expand Down Expand Up @@ -4082,7 +4054,43 @@ fn rpc_perf_sample_from_perf_sample(slot: u64, sample: PerfSample) -> RpcPerfSam
}
}

// RPC methods deprecated in v1.8
pub mod rpc_deprecated_v1_18 {
use super::*;
#[rpc]
pub trait DeprecatedV1_18 {
type Metadata;

// DEPRECATED
#[rpc(meta, name = "getStakeActivation")]
fn get_stake_activation(
&self,
meta: Self::Metadata,
pubkey_str: String,
config: Option<RpcEpochConfig>,
) -> Result<RpcStakeActivation>;
}

pub struct DeprecatedV1_18Impl;
impl DeprecatedV1_18 for DeprecatedV1_18Impl {
type Metadata = JsonRpcRequestProcessor;

fn get_stake_activation(
&self,
meta: Self::Metadata,
pubkey_str: String,
config: Option<RpcEpochConfig>,
) -> Result<RpcStakeActivation> {
debug!(
"get_stake_activation rpc request received: {:?}",
pubkey_str
);
let pubkey = verify_pubkey(&pubkey_str)?;
meta.get_stake_activation(&pubkey, config)
}
}
}

// RPC methods deprecated in v1.9
pub mod rpc_deprecated_v1_9 {
#![allow(deprecated)]
use super::*;
Expand Down
6 changes: 4 additions & 2 deletions rpc/src/rpc_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ use {
max_slots::MaxSlots,
optimistically_confirmed_bank_tracker::OptimisticallyConfirmedBank,
rpc::{
rpc_accounts::*, rpc_accounts_scan::*, rpc_bank::*, rpc_deprecated_v1_7::*,
rpc_deprecated_v1_9::*, rpc_full::*, rpc_minimal::*, rpc_obsolete_v1_7::*, *,
rpc_accounts::*, rpc_accounts_scan::*, rpc_bank::*, rpc_deprecated_v1_18::*,
rpc_deprecated_v1_7::*, rpc_deprecated_v1_9::*, rpc_full::*, rpc_minimal::*,
rpc_obsolete_v1_7::*, *,
},
rpc_cache::LargestAccountsCache,
rpc_health::*,
Expand Down Expand Up @@ -510,6 +511,7 @@ impl JsonRpcService {
io.extend_with(rpc_full::FullImpl.to_delegate());
io.extend_with(rpc_deprecated_v1_7::DeprecatedV1_7Impl.to_delegate());
io.extend_with(rpc_deprecated_v1_9::DeprecatedV1_9Impl.to_delegate());
io.extend_with(rpc_deprecated_v1_18::DeprecatedV1_18Impl.to_delegate());
}
if obsolete_v1_7_api {
io.extend_with(rpc_obsolete_v1_7::ObsoleteV1_7Impl.to_delegate());
Expand Down
Loading