Mf/remove jailed stake from global active stake #533

mattfoley8 · 2023-05-19T14:29:47Z

This PR removes any jailed stake from our GlobalActiveStakeAmountNanos value. If a validator is jailed, we do not want to consider their stake in the total active stake across validators used in determining if we have a 2/3rd consensus on votes. This PR makes the following changes:

Renames GlobalStakeAmountNanos to GlobalActiveStakeAmountNanos. Note the addition of the keyword Active. This value describes the total active stake across validators, excluding any jailed validators' stake.
When a user stakes with a validator, we only add the additional stake to the GlobalActiveStakeAmountNanos if the validator isn't jailed.
When a user unstakes from a validator, we only subtract the decreased stake from the GlobalActiveStakeAmountNanos if the validator isn't jailed.
When a validator is jailed, we subtract their TotalStakeAmountNanos from the GlobalActiveStakeAmountNanos.
When a validator unjails himself, we add back their TotalStakeAmountNanos to the GlobalActiveStakeAmountNanos.
When a validator unregisters, we only subtract their now unstaked TotalStakeAmountNanos from the GlobalActiveStakeAmountNanos it they weren't jailed.

lazynina · 2023-05-19T15:29:06Z

lib/block_view_stake.go

-	// Restore the PrevGlobalStakeAmountNanos.
-	bav._setGlobalStakeAmountNanos(operationData.PrevGlobalStakeAmountNanos)
+	// Restore the PrevGlobalActiveStakeAmountNanos if the PrevValidatorEntry was active.
+	if prevValidatorEntry.Status() == ValidatorStatusActive {


we could just check if PrevGlobalActiveStakeAmountNanos is non-nil. I generally prefer that as opposed to relying on a different field. The prevs generally means "revert to this state when disconnecting".

lazynina · 2023-05-19T15:29:34Z

lib/block_view_stake.go

-	// Restore the PrevGlobalStakeAmountNanos.
-	bav._setGlobalStakeAmountNanos(operationData.PrevGlobalStakeAmountNanos)
+	// Restore the PrevGlobalActiveStakeAmountNanos if the PrevValidatorEntry was active.
+	if prevValidatorEntry.Status() == ValidatorStatusActive {


same as above.

lib/block_view_stake.go

lazynina · 2023-05-19T15:32:29Z

lib/block_view_types.go

+		if prevGlobalActiveStakeAmountNanos, err := VariableDecodeUint256(rr); err == nil {
+			op.PrevGlobalActiveStakeAmountNanos = prevGlobalActiveStakeAmountNanos


we could just write if op.PrevGlobalActiveStakeAmountNanos, err := VariableDecodeUint256(rr); err != nil here instead.

lazynina · 2023-05-19T15:34:42Z

lib/block_view_validator.go

 	// Set the CurrentValidatorEntry.
 	bav._setValidatorEntryMappings(currentValidatorEntry)

+	// Increase the GlobalActiveStakeAmountNanos.


IsValidUnjailValidatorMetadata ensures that the validator is indeed jailed, correct?

That's correct. See link here. It also validates that sufficient epochs have elapsed for the validator to be unjailed.

lib/block_view_validator.go

diamondhands0 · 2023-05-21T20:52:08Z

lib/block_view_stake.go

-		return 0, 0, nil, errors.Wrapf(err, "_connectStake: error adding StakeAmountNanos to GlobalStakeAmountNanos: ")
+	// Increase the GlobalActiveStakeAmountNanos if the validator is active.
+	var prevGlobalActiveStakeAmountNanos *uint256.Int
+	if currentValidatorEntry.Status() == ValidatorStatusActive {


I'm sure you've thougth about this, but do we want to make it so that you can't stake to a jailed validator? You would just add an else if block here to error if the status is jailed.

If you did this, I'd also add an else for "Unknown validator status" error.

I thought about it more, and I think what you're doing here is more elegant. If we were to disallow staking to jailed validators, we'd break symmetry with Unstake, which I think adds complexity.

diamondhands0 · 2023-05-21T20:57:43Z

lib/block_view_types.go

-		} else {
-			return errors.Wrapf(err, "UtxoOperation.Decode: Problem reading PrevGlobalStakeAmountNanos: ")
+		// PrevGlobalActiveStakeAmountNanos
+		if op.PrevGlobalActiveStakeAmountNanos, err = VariableDecodeUint256(rr); err != nil {


Nit: No need to fix this here, but I tend to prefer breaking assignments out of if statements like the following. I know it's not "Go-like" but I find it easier to read and less error-prone:

// PrevGlobalActiveStakeAmountNanos op.PrevGlobalActiveStakeAmountNanos, err = VariableDecodeUint256(rr) if err != nil { return errors.Wrapf(err, "UtxoOperation.Decode: Problem reading PrevGlobalActiveStakeAmountNanos: ") }

diamondhands0 · 2023-05-21T21:00:33Z

lib/block_view_validator.go

+	// Restore the PrevGlobalActiveStakeAmountNanos.
+	prevGlobalActiveStakeAmountNanos := operationData.PrevGlobalActiveStakeAmountNanos
+	if prevGlobalActiveStakeAmountNanos == nil {
+		return errors.New("_disconnectUnjailValidator: PrevGlobalActiveStakeAmountNanos is nil")


Nit: Whenever something should never happen, like I think is the case here, I like to add "This should never happen" or "sanity-check failed" to indicate that in the logs for whoever ends up having to debug it.

Let's add this^

lazynina · 2023-05-22T14:37:52Z

lib/block_view.go


-	// Global stake across all validators
-	GlobalStakeAmountNanos *uint256.Int
+	// Global active stake across all validators


can we expound upon this comment? Something like The global active stake is the sum of all DESO staked to validators who are not jailed.

lazynina · 2023-05-22T14:41:34Z

lib/block_view_validator.go

+	// Restore the PrevGlobalActiveStakeAmountNanos.
+	prevGlobalActiveStakeAmountNanos := operationData.PrevGlobalActiveStakeAmountNanos
+	if prevGlobalActiveStakeAmountNanos == nil {
+		return errors.New("_disconnectUnjailValidator: PrevGlobalActiveStakeAmountNanos is nil")


Let's add this^

* Diamondhands/pos txn types review (#530) * POS fixes and comments from diamondhands * Mf/pos txn types review (#531) * Add comments. * Addres review comments. * Add first batch of sanity check utils. * Add second batch of txn sanity checks. * Rename encode uint256 funcs in comments. * Nuke RegisteredAtBlockHeight. * Address review feedback. * Fix a few more typos. * Update some comments --------- Co-authored-by: diamondhands <diamondhands@bitcloutdev.com> --------- Co-authored-by: Matt Foley <100429827+mattfoley8@users.noreply.github.com> * Exclude zero-stake validators from TopValidatorsByStake. (#535) * Mf/remove jailed stake from global active stake (#533) * Remove jailed stake from global active stake. * Write to the UtxoView if no errors in jailing. * Add additional sanity checks. * PR review feedback. * Remove FIXME comment. --------- Co-authored-by: diamondhands0 <81935176+diamondhands0@users.noreply.github.com>

mattfoley8 added 2 commits May 18, 2023 17:09

Remove jailed stake from global active stake.

fb564a6

Write to the UtxoView if no errors in jailing.

ac9c062

mattfoley8 requested review from diamondhands0 and lazynina May 19, 2023 14:29

mattfoley8 self-assigned this May 19, 2023

mattfoley8 requested a review from a team as a code owner May 19, 2023 14:29

lazynina reviewed May 19, 2023

View reviewed changes

Add additional sanity checks.

7d59e65

diamondhands0 reviewed May 21, 2023

View reviewed changes

lazynina reviewed May 22, 2023

View reviewed changes

PR review feedback.

12b5915

lazynina approved these changes May 22, 2023

View reviewed changes

mattfoley8 merged commit ae9032c into feature/pos-txn-types May 22, 2023

mattfoley8 deleted the mf/remove-jailed-stake-from-global-active-stake branch May 22, 2023 17:02

		if prevGlobalActiveStakeAmountNanos, err := VariableDecodeUint256(rr); err == nil {
		op.PrevGlobalActiveStakeAmountNanos = prevGlobalActiveStakeAmountNanos

Mf/remove jailed stake from global active stake #533

Mf/remove jailed stake from global active stake #533

Uh oh!

Conversation

mattfoley8 commented May 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants