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

Replay fix for Validator Manager #743

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

geoff-vball
Copy link
Contributor

@geoff-vball geoff-vball commented Mar 18, 2025

Why this should be merged

Fixes an issue where:

  1. Validator added
  2. Validator active
  3. Validator removed before expiry of original message.
  4. Validator is re-added with original initiateValidatorRegistration message.
  5. Validator is set to active with original completeValidatorRegistration message signed by p-chain

This could cause a validator to be marked as active in the contract, but inactive on the p-chain. Such a validator would not be able to accrue any uptime, so in most cases would be ineligible for rewards. We want to avoid potential de-syncs from with the p-chain.

Also aligns MAXIMUM_REGISTRATION_EXPIRY_LENGTH to be equal to the value on the p-chain.

How this works

Since we keep validator state indefinitely, we just check if the status is Unknown before initiating a registration.

How this was tested

How is this documented

Comment on lines +533 to +535
) internal returns (bytes32) {
(bytes32 validationID, bytes memory registerL1ValidatorMessage) = ValidatorMessages
.packRegisterL1ValidatorMessage(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated fix that simplified this code :)

@geoff-vball geoff-vball marked this pull request as ready for review March 18, 2025 20:53
@geoff-vball geoff-vball requested a review from a team as a code owner March 18, 2025 20:54
Copy link
Contributor

@bernard-avalabs bernard-avalabs left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Just a comment about the expiry length change.

@@ -94,7 +94,7 @@ contract ValidatorManager is Initializable, OwnableUpgradeable, ACP99Manager {
0xe92546d698950ddd38910d2e15ed1d923cd0a7b3dde9e2a6a3f380565559cb00;

uint8 public constant MAXIMUM_CHURN_PERCENTAGE_LIMIT = 20;
uint64 public constant MAXIMUM_REGISTRATION_EXPIRY_LENGTH = 2 days;
uint64 public constant MAXIMUM_REGISTRATION_EXPIRY_LENGTH = 1 days;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change required for this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required, no, but aligns with the value the p-chain uses.

@geoff-vball geoff-vball marked this pull request as draft April 8, 2025 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog 🗄️
Development

Successfully merging this pull request may close these issues.

2 participants