Rpc: make inactive_stake consistent in getStakeActivation

[CriesofCarrots]

Problem

The RPC endpoint getStakeActivation only looks at the total stake balance for the deactivating and inactive cases. This means that if an active stake account receives a transfer, the amount is not reflected in the "inactive" stake, which is confusing.

Summary of Changes

Evaluate inactive_stake to the total lamports balance minus effective stake and rent-exempt reserve for all activation cases.

Fixes #35111

[t-nelson]

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?

[CriesofCarrots]

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.

[t-nelson]

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?

[xdzurman]

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.

[CriesofCarrots]

so deprecate this method and call it a day?

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).

[codecov]

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (7a95e4f) to head (51039cb).
Report is 94 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #35116       +/-   ##
===========================================
+ Coverage        0    81.6%    +81.6%     
===========================================
  Files           0      831      +831     
  Lines           0   224830   +224830     
===========================================
+ Hits            0   183551   +183551     
- Misses          0    41279    +41279     

[CriesofCarrots]

CI failure unrelated to this patch... er, I mean I needed to rebase. First commit is unchanged.

[willhickey]

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #35419. Mergify cannot evaluate rules on this PR. ⚠️