Skip to content

Replay fix for Validator Manager #743

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion contracts/validator-manager/ValidatorManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.

uint32 public constant ADDRESS_LENGTH = 20; // This is only used as a packed uint32
uint32 public constant NODE_ID_LENGTH = 20;
uint8 public constant BLS_PUBLIC_KEY_LENGTH = 48;
Expand Down Expand Up @@ -388,6 +388,12 @@ contract ValidatorManager is Initializable, OwnableUpgradeable, ACP99Manager {
weight: weight
})
);

// Ensure that this validation ID has not been used before to prevent replays
if ($._validationPeriods[validationID].status != ValidatorStatus.Unknown) {
revert InvalidValidatorStatus($._validationPeriods[validationID].status);
}

$._pendingRegisterValidationMessages[validationID] = registerL1ValidatorMessage;
$._registeredValidators[nodeID] = validationID;

Expand Down
52 changes: 52 additions & 0 deletions contracts/validator-manager/tests/StakingManagerTests.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -1281,6 +1281,58 @@ abstract contract StakingManagerTest is ValidatorManagerTest {
});
}

function testReplayValidatorRegistration() public virtual override {
bytes32 validationID = _registerDefaultValidator();
bytes memory setWeightMessage =
ValidatorMessages.packL1ValidatorWeightMessage(validationID, 1, 0);
bytes memory uptimeMessage = ValidatorMessages.packValidationUptimeMessage(
validationID, DEFAULT_COMPLETION_TIMESTAMP - DEFAULT_REGISTRATION_TIMESTAMP
);
_initiateValidatorRemoval({
validationID: validationID,
completionTimestamp: DEFAULT_COMPLETION_TIMESTAMP,
setWeightMessage: setWeightMessage,
includeUptime: true,
uptimeMessage: uptimeMessage,
force: false
});

uint256 expectedReward = rewardCalculator.calculateReward({
stakeAmount: _weightToValue(DEFAULT_WEIGHT),
validatorStartTime: DEFAULT_REGISTRATION_TIMESTAMP,
stakingStartTime: DEFAULT_REGISTRATION_TIMESTAMP,
stakingEndTime: DEFAULT_COMPLETION_TIMESTAMP,
uptimeSeconds: DEFAULT_COMPLETION_TIMESTAMP - DEFAULT_REGISTRATION_TIMESTAMP
});

address validatorOwner = address(this);

_completeEndValidationWithChecks({
validationID: validationID,
validatorOwner: validatorOwner,
expectedReward: expectedReward,
validatorWeight: DEFAULT_WEIGHT,
rewardRecipient: validatorOwner
});

vm.warp(DEFAULT_EXPIRY - 1);
_beforeSend(_weightToValue(DEFAULT_WEIGHT), address(this));

vm.expectRevert(
abi.encodeWithSelector(
ValidatorManager.InvalidValidatorStatus.selector, ValidatorStatus.Completed
)
);
_initiateValidatorRegistration({
nodeID: DEFAULT_NODE_ID,
blsPublicKey: DEFAULT_BLS_PUBLIC_KEY,
remainingBalanceOwner: DEFAULT_P_CHAIN_OWNER,
disableOwner: DEFAULT_P_CHAIN_OWNER,
registrationExpiry: DEFAULT_EXPIRY,
weight: DEFAULT_WEIGHT
});
}

function testCompleteEndValidationWithNonValidatorRewardRecipient() public virtual {
bytes32 validationID = _registerDefaultValidator();
bytes memory setWeightMessage =
Expand Down
70 changes: 55 additions & 15 deletions contracts/validator-manager/tests/ValidatorManagerTests.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@ import {
WarpMessage,
IWarpMessenger
} from "@avalabs/[email protected]/contracts/interfaces/IWarpMessenger.sol";
import {ACP99Manager, ConversionData, InitialValidator, PChainOwner} from "../ACP99Manager.sol";
import {
ACP99Manager,
ConversionData,
InitialValidator,
PChainOwner,
ValidatorStatus
} from "../ACP99Manager.sol";
import {OwnableUpgradeable} from
"@openzeppelin/[email protected]/access/OwnableUpgradeable.sol";

Expand Down Expand Up @@ -279,6 +285,48 @@ abstract contract ValidatorManagerTest is Test {
_completeValidatorRemoval(0);
}

function testReplayValidatorRegistration() public virtual {
bytes32 validationID = _registerDefaultValidator();
bytes memory setWeightMessage =
ValidatorMessages.packL1ValidatorWeightMessage(validationID, 1, 0);
bytes memory uptimeMessage;
_initiateValidatorRemoval({
validationID: validationID,
completionTimestamp: DEFAULT_COMPLETION_TIMESTAMP,
setWeightMessage: setWeightMessage,
includeUptime: false,
uptimeMessage: uptimeMessage,
force: false
});

bytes memory l1ValidatorRegistrationMessage =
ValidatorMessages.packL1ValidatorRegistrationMessage(validationID, false);

_mockGetPChainWarpMessage(l1ValidatorRegistrationMessage, true);

vm.expectEmit(true, true, true, true, address(validatorManager));
emit CompletedValidatorRemoval(validationID);

_completeValidatorRemoval(0);

vm.warp(DEFAULT_EXPIRY - 1);
_beforeSend(_weightToValue(DEFAULT_WEIGHT), address(this));

vm.expectRevert(
abi.encodeWithSelector(
ValidatorManager.InvalidValidatorStatus.selector, ValidatorStatus.Completed
)
);
_initiateValidatorRegistration({
nodeID: DEFAULT_NODE_ID,
blsPublicKey: DEFAULT_BLS_PUBLIC_KEY,
remainingBalanceOwner: DEFAULT_P_CHAIN_OWNER,
disableOwner: DEFAULT_P_CHAIN_OWNER,
registrationExpiry: DEFAULT_EXPIRY,
weight: DEFAULT_WEIGHT
});
}

function testCompleteInvalidatedValidation() public {
bytes32 validationID = _setUpInitializeValidatorRegistration(
DEFAULT_NODE_ID,
Expand Down Expand Up @@ -482,8 +530,9 @@ abstract contract ValidatorManagerTest is Test {
uint64 weight,
uint64 registrationExpiry,
bytes memory blsPublicKey
) internal returns (bytes32 validationID) {
(validationID,) = ValidatorMessages.packRegisterL1ValidatorMessage(
) internal returns (bytes32) {
(bytes32 validationID, bytes memory registerL1ValidatorMessage) = ValidatorMessages
.packRegisterL1ValidatorMessage(
Comment on lines +533 to +535
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 :)

ValidatorMessages.ValidationPeriod({
nodeID: nodeID,
subnetID: subnetID,
Expand All @@ -494,19 +543,8 @@ abstract contract ValidatorManagerTest is Test {
weight: weight
})
);

bytes20 fixedID = _fixedNodeID(nodeID);
(, bytes memory registerL1ValidatorMessage) = ValidatorMessages
.packRegisterL1ValidatorMessage(
ValidatorMessages.ValidationPeriod({
subnetID: subnetID,
nodeID: nodeID,
blsPublicKey: blsPublicKey,
registrationExpiry: registrationExpiry,
remainingBalanceOwner: DEFAULT_P_CHAIN_OWNER,
disableOwner: DEFAULT_P_CHAIN_OWNER,
weight: weight
})
);
vm.warp(registrationExpiry - 1);
_mockSendWarpMessage(registerL1ValidatorMessage, bytes32(0));

Expand All @@ -524,6 +562,8 @@ abstract contract ValidatorManagerTest is Test {
registrationExpiry: registrationExpiry,
weight: weight
});

return validationID;
}

function _registerValidator(
Expand Down
4 changes: 4 additions & 0 deletions tests/flows/validator-manager/poa_to_pos.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ func PoAMigrationToPoS(network *localnetwork.LocalNetwork) {
Expect(err).Should(BeNil())
Expect(validationID[:]).Should(Equal(poaValidationID[:]))

validator, err := validatorManager.GetValidator(&bind.CallOpts{}, validationID)
Expect(err).Should(BeNil())
Expect(validator.EndTime).Should(Equal(uint64(0)))

//
// Remove the PoA validator and re-register as a PoS validator
//
Expand Down
Loading