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

ACP-77: Deactivate SoVs without sufficient fees #3412

Merged
merged 282 commits into from
Nov 5, 2024

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Sep 24, 2024

Why this should be merged

ACP-77 introduces a new type of validator: the SubnetOnlyValidator.

This PR introduces logic to disable this new validator type when a validator no longer has sufficient balance to pay additional fees.

How this works

  1. Block timestamps can not be advanced further than all SoVs have sufficient balance.
  2. SoVs that run out of funds due to the timestamp increase are evicted prior to processing transactions
  3. After processing transactions, SoVs that can not cover at least 1 seconds worth of fees are removed. (This prevents the chain timestamp from being frozen)

How this was tested

Added unit tests.

Need to be documented in RELEASES.md?

There are no user facing changes.

@@ -565,5 +569,70 @@ func (v *verifier) processStandardTxs(txs []*txs.Tx, feeCalculator fee.Calculato
}
}

// After processing all the transactions, deactivate any SoVs that might not
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "might not" mean in this context? Is it nondeterministic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is deterministic. However, this check is not accurate.

Specifically, it's possible for another block to be issued at the same timestamp. This means that a later block technically could be issued with the same timestamp as this block and the balance of this SoV could be increased (for example).

Ideally this SoV would not have been marked as inactive in this case, because it was always able to pay fees for the time it was in the validator set. But we can't know for certain at this point that this will or will not happen, so we take the conservative approach here and disable the SoV.

// If the validator has exactly the right amount of fee for the next
// second we should not remove them here.
if sov.EndAccumulatedFee >= potentialAccruedFees {
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think break is correct.

This is iterating over sovs (which means that sov.EndAccumulatedFee is monotonically increasing). Once we find the first SoV that has sov.EndAccumulatedFee >= potentialAccruedFees, we know that all following SoVs will have sov.EndAccumulatedFee >= potentialAccruedFees.

Technically a continue would still work here - but it would mean that we are iterating over SoVs that do not need to be iterated over.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I was missing that the sov's are ordered in that way. I think a comment explaining that any following sov's will also pass this check would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Base automatically changed from implement-acp-77-sov-validators-state to master November 5, 2024 16:39
vms/platformvm/block/executor/verifier.go Show resolved Hide resolved
vms/platformvm/block/executor/verifier.go Show resolved Hide resolved
vms/platformvm/txs/executor/state_changes.go Outdated Show resolved Hide resolved
vms/platformvm/txs/executor/state_changes_test.go Outdated Show resolved Hide resolved
vms/platformvm/state/chain_time_helpers.go Show resolved Hide resolved
@StephenButtolph StephenButtolph added this pull request to the merge queue Nov 5, 2024
Merged via the queue into master with commit 9185ae7 Nov 5, 2024
23 checks passed
@StephenButtolph StephenButtolph deleted the implement-acp-77-deactivation branch November 5, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants