Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Conversation

mvines
Copy link
Contributor

@mvines mvines commented Mar 25, 2022

There's no mechanism to cleanup stake delegated to abandoned vote accounts. As a motivating example, 9% of the devnet stake has now been abandoned for almost 100 epochs. Currently the only path to remove that 9% dead weight is by a network restart.

This PR proposes a new stake program instruction that permits anybody to deactivate stake delegated to a vote account that has not landed a vote in the last 5 (10) epochs. There's no reward for performing this common good so there should be no motivation to bot this instruction as soon as a stake account becomes eligible for public destaking.

The benefit of this approach over some form of automatic destaking by the runtime is the lack of complexity. The stake program changes here are very minimal and thus easy to inspect and get comfortable with (or not!).

TODO:

  • Decide if 10 epochs is the right magic number, which is ~three or more weeks in human time. This feels long enough for an operator to resolve any temporary censorship attempts by another validator/staker that's attempting to force them off the network. Update: I thought about it and 5 epochs seems like plenty of buffer, which is approximately 15 days of delinquency.
  • Add tests if this approach is accepted
  • Cli support
  • Vote accounts that have never voted but have managed to accumulate active stake somehow are currently excluded from this instruction. Decide if this is acceptable. The alternative would be to permit such stake to be deactivated immediately. Update: Stake delegated to a vote account that has never voted may also be destaked by anybody
  • Consider requiring a second vote account that voted in each of the last 5 (10) epochs be provided to the instruction. This second vote account acts as proof to the stake program that the chain has been active over the last 5 (10) epoch, and prevents a very unlikely vector where a stake account could be destaked if the attacker is able to warp the chain ahead by 5 (10) epochs (perhaps on a mainnet restart somehow). This might be overkill. Update: Added

@mvines mvines force-pushed the DeactivateDelinquent branch from 627b151 to 3ddb9db Compare March 25, 2022 16:26
@mvines mvines added the CI Pull Request is ready to enter CI label Mar 25, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Mar 25, 2022
@mvines mvines force-pushed the DeactivateDelinquent branch from 3ddb9db to 589f816 Compare March 25, 2022 21:35
@leoluk
Copy link
Contributor

leoluk commented Mar 26, 2022

Big 👍 on this - jailing has been working very well on Cosmos networks.

However, jailing on Cosmos works slightly differently - you don't lose the delegations, but your node just gets marked as "jailed" and all stakes gets slashed a small % (optionally). You can then unjail yourself once the issue is resolved without losing the delegations. Actually deactivating stake is in some ways a harsher punishment since you won't easily get those delegations back.

Decide if 10 epochs is the right magic number, which is ~three or more weeks in human time. This feels long enough for an operator to resolve any temporary censorship attempts by another validator/staker that's attempting to force them off the network.

What's the current threshold for a successful censorship attempt? Unless it's very low, we could even go a much more aggressive threshold, like 2-3 epochs - this is still plenty of time. It's a good way to further incentivize higher-quality validator operations.

Did we previously use the deliquency status for anything critical?

Related issue:

@mvines
Copy link
Contributor Author

mvines commented Mar 28, 2022

What's the current threshold for a successful censorship attempt? Unless it's very low, we could even go a much more aggressive threshold, like 2-3 epochs - this is still plenty of time. It's a good way to further incentivize higher-quality validator operations.

I feel the threshold would be pretty high, but only have intuition. To prevent a validator from voting at least once in multiple epochs seems like it would require a large percentage of the stake to reject all vote transactions and all blocks containing vote transactions from the victim. To be fully effective, I think 2/3rds of the stake would need be participating at which point it doesn't matter anyway.

Did we previously use the deliquency status for anything critical?

Beyond obviously computing epoch rewards, no.

@mvines mvines force-pushed the DeactivateDelinquent branch 2 times, most recently from 39f4c5f to b4091a3 Compare March 29, 2022 03:45
@mvines mvines changed the title Proposal: Add StakeInstruction::DeactivateDelinquent Add StakeInstruction::DeactivateDelinquent Mar 29, 2022
@mvines mvines force-pushed the DeactivateDelinquent branch from b4091a3 to a536ad3 Compare March 29, 2022 16:56
@mvines mvines marked this pull request as ready for review March 29, 2022 16:56
@mvines mvines force-pushed the DeactivateDelinquent branch 3 times, most recently from 4c646ee to f5fff3e Compare March 30, 2022 03:41
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #23932 (f5fff3e) into master (2da4e3e) will decrease coverage by 0.0%.
The diff coverage is 70.7%.

❗ Current head f5fff3e differs from pull request most recent head 3b30bff. Consider uploading reports for the commit 3b30bff to get more accurate results

@@            Coverage Diff            @@
##           master   #23932     +/-   ##
=========================================
- Coverage    81.8%    81.7%   -0.1%     
=========================================
  Files         581      585      +4     
  Lines      158312   159583   +1271     
=========================================
+ Hits       129518   130431    +913     
- Misses      28794    29152    +358     

@mvines
Copy link
Contributor Author

mvines commented Mar 30, 2022

I think this is ready for review, but no rush as I’m OOO for the next week or so.

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

cursory look at the logic looks good. i think we can improve the error reporting so clients can be more helpful with failures

@mvines mvines added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 4, 2022
@mvines mvines force-pushed the DeactivateDelinquent branch from f5fff3e to 3bb0ae2 Compare April 12, 2022 20:12
@mvines
Copy link
Contributor Author

mvines commented Apr 12, 2022

cursory look at the logic looks good. i think we can improve the error reporting so clients can be more helpful with failures

Added some additional StakeErrors in a couple places instead of using more general InstructionErrors

@CriesofCarrots
Copy link
Contributor

Shoot, I forgot to review while you were gone. Will look a little later this afternoon!

@mvines mvines force-pushed the DeactivateDelinquent branch 4 times, most recently from 06717e0 to 733eaae Compare April 12, 2022 22:06
@mvines
Copy link
Contributor Author

mvines commented Apr 13, 2022

CI is now happy here other than the unrelated downstream-projects timeout

Copy link
Contributor

@CriesofCarrots CriesofCarrots 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 nitty comments on the runtime side of things. Looking at cli now

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

looking good. great test coverage!

seems like the cli command that we really want is one that takes the delinquent and reference vote accounts, then looks up the delinquent voter's stake accounts and deactivates them all.

@mvines
Copy link
Contributor Author

mvines commented Apr 13, 2022

seems like the cli command that we really want is one that takes the delinquent and reference vote accounts, then looks up the delinquent voter's stake accounts and deactivates them all.

Yeah you're right. I figured that could be bashed though since hopefully this isn't a feature that's used regularly

@mvines mvines force-pushed the DeactivateDelinquent branch from 733eaae to 3b30bff Compare April 13, 2022 16:02
@mvines
Copy link
Contributor Author

mvines commented Apr 13, 2022

✅ Review feedback applied

@mvines mvines force-pushed the DeactivateDelinquent branch from 3b30bff to ff23ca8 Compare April 13, 2022 21:14
@mvines mvines force-pushed the DeactivateDelinquent branch from ff23ca8 to 9a0ce8f Compare April 13, 2022 21:15
Copy link
Contributor

@CriesofCarrots CriesofCarrots 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!

@mvines
Copy link
Contributor Author

mvines commented Apr 13, 2022

are you good here as well @t-nelson?

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm! :shipit:

@mvines mvines merged commit 57ff737 into solana-labs:master Apr 14, 2022
@mvines mvines deleted the DeactivateDelinquent branch April 14, 2022 05:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants