add example staking manager #1223

ceyonur · 2024-06-26T14:31:08Z

Why this should be merged

How this works

How this was tested

How is this documented

ceyonur · 2024-07-15T19:00:20Z

contracts/contracts/interfaces/staking-manager.md

+- expirationTime (uint64): The time when this register message will be expired
+- ed25519 signature (bytes): The signature of the message signed by the node's BLS key
+
+The messageID of the request can be crafted with `sha256(subnetID, nodeID, amount, expirationTime, signature)`. This should be same with the one that will be crafted for P-chain.


This is important

ceyonur · 2024-07-15T19:02:47Z

contracts/contracts/interfaces/staking-manager.md

+The contract should verify these:
+
+- (Optional) the staking amount is greater than the minimum staking amount
+- (Optional) The node is not already registered as a validator. This will be handled by the P-Chain though.


This will be handled by P-Chain anyway, but I think we have the info to check this in contract

ceyonur · 2024-07-15T19:03:14Z

contracts/contracts/interfaces/staking-manager.md

+
+- (Optional) the staking amount is greater than the minimum staking amount
+- (Optional) The node is not already registered as a validator. This will be handled by the P-Chain though.
+- (Optional) Expiry timestamp is not in the past


This seems reasonable but again subnet chain and p-chain might not be in sync.

ceyonur · 2024-07-15T19:13:17Z

contracts/contracts/interfaces/staking-manager.md

+
+1- If a validator becomes ineligible permanetly due to low uptime, a warp message should be sent to the manager contract.
+2- Once a validator is removed, a warp message that indicates the uptime is enough will be sent to the contract.
+3- A message will be aggregated upon a request from the manager contract (e.g via an event), then the message will be sent to the manager contract.


I feel like if the manager contract can signal for aggregation of a message which target a uptime specified by the contract; it can implement many different use cases. I.e contract can ask about >%80 uptime message to be signed and if so it can use it to rewarding. In another case it might ask for <%50 uptime and if signed can slash. A simply dynamic request for >%x itself is very helpful for contracts to determine their min uptime.

Pseudocode :

constant threshold = 50 function receiveRegisterMessageInvalid(uint32 messageIndex) external { ... if (validator.redeemedAt != 0) { uint256 reward = calculateReward( validator.weight, validator.startedAt, validator.redeemedAt ); uptimeID = calculateID(validationID, threshold, reward) lockedRewards[uptimeID] = {validator.rewardAddress, reward} emit UptimeRequest(uptimeID, threshold, validationID, validator.nodeID) ... } function receiveUptime(uint32 messageIndex) external{ (WarpMessage memory warpMessage, bool success) = WARP_MESSENGER.getVerifiedWarpMessage(messageIndex); require(success, "ExampleValidatorManager: invalid warp message"); uptimeMessage = decode(warpMessage) uptimeID = uptimeMessage.ID reward = lockedRewards[uptimeID] delete lockedRewards[uptimeID] stakingToken.transfer(reward.address, reward.amount) ... }

The relayer/aggregator should listen UptimeRequest events and then aggregate signatures based on parameters. The validators should only be willing to sign the message if they think the uptime is above the specified threshold.

I prefer this flexible alternative, do we plan to support only >= and < operators?

In most of the cases I don't think we even need to support "<". Most of the time it will be enough to check if a validator has enough uptime. However I also don't think it would be too complicated to support "<". I think these both can cover almost all use cases.

contracts/contracts/ExampleStakingManager.sol

darioush · 2024-07-10T20:14:52Z

contracts/contracts/ExampleStakingManager.sol

+
+  uint256 public minStakingDuration; // Minimun duration for staking in seconds.
+  uint256 public minStakingAmount; // Minimum amount for staking.
+  bool public stakingEnabled; // Start time of the staking period.s


nit: seems this comment is not correct?

yup corrected

darioush · 2024-07-10T20:31:53Z

contracts/contracts/ExampleStakingManager.sol

+    }
+
+    uint256 stakingTime = finishedAt - startedAt;
+    return (stakingTime * rewardRate * amount) / (1 ether); // Assuming rewardRate is scaled appropriately


nit: should we add the comment about the scale to the comment by the definition?

I don't think this reward calculation is even something we would use in production.

darioush · 2024-07-10T23:25:30Z

contracts/contracts/ExampleStakingManager.sol

+
+  // TODO: add partial withdraw + increase stake
+
+  // TODO: add delegation


what is the tx flow for delegation? does it make sense to batch updating the delegations to a single SetSubnetValidatorWeightTx?

darioush · 2024-07-15T19:56:09Z

contracts/contracts/interfaces/staking-manager.md

+
+### receiveRemoveValidator
+
+Once the `SetSubnetValidatorWeightTx` is accepted, a warp message `InvalidValidatorRegisterMessage` will be relayed to the contract with `messageID` of the request. The message represents a messageID will not possibly become a valid validator. This essentialy means the message was not valid in the first place, or the validator has removed from the set. The InvalidValidatorRegisterMessage can either be a result of `removeValidator` from the contract or it can be originated directly from P-chain via `ExitValidatorSetTx`. In either case this ultimately tells the contract that a validator was evicted. See related ACP-77 section [here](https://github.com/avalanche-foundation/ACPs/tree/main/ACPs/77-reinventing-subnets#exitvalidatorsettx).


nit: "will not possibly become become a valid validator"
this is referring to the future right? since it's possible it was a validator in the past. (eg evicted).

yes. I think following sentence reflects that, but if you think it's not clear lmk.
This essentialy means the message was not valid in the first place, or the validator has removed from the set.

darioush · 2024-07-15T19:58:04Z

contracts/contracts/interfaces/staking-manager.md

+
+Rewards should not be unlocked or sent to the staker at this point. This is because we need to first perform a uptime check to determine the rewards.
+
+TODO: should we give a partial reward in case the validator got removed from P-chain (balance drained)? Possibly by timestamping the `InvalidValidatorRegisterMessage`.


this seems important to support

yes. We have discussed about ExitValidatorSetTx. It won't result into a signable InvalidValidatorRegisterMessage, rather it will mark the validator inactive and not actually remove from the set. I think we can stop increasing uptime once they got inactive.

darioush · 2024-07-15T19:58:26Z

contracts/contracts/interfaces/staking-manager.md

+
+TODO: should we give a partial reward in case the validator got removed from P-chain (balance drained)? Possibly by timestamping the `InvalidValidatorRegisterMessage`.
+
+TODO: should we add a cooldown period for validator removal/readd?


what is the benefit of requiring this cooldown period?

I was thinking if it would be DoSable if someone removes and readds itself too quickly. but I think that should be good. We should however still need to add a cooldown for partial redeeming.

darioush · 2024-07-15T20:01:05Z

contracts/contracts/interfaces/staking-manager.md

+The contract should verify these in `receiveRemoveValidator`:
+
+- Verified warp message
+- The typecheck on the warp message (== InvalidValidatorRegisterMessage)


nit: I dislike the name of this message InvalidValidatorRegisterMessage, mainly because "Invalid....Message" seems weird as part of an expected flow.

Should we rename it to ValidatorUnregistered or ValidatorNotRegistered (or perhaps Validation)?
Not sure if it is too late for this

yea i don't like it very much either. I'm not quite sure how to reflect "the message was invalid" + "the message won't be valid anymore. Maybe DefunctMessage?

Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

Signed-off-by: Ceyhun Onur <ceyhun.onur@avalabs.org>

…to validator-manager

geoff-vball · 2024-07-19T16:25:10Z

contracts/contracts/interfaces/staking-manager.md

+- (?) The nodeID in the pending request is not a validator already
+- (?) Message is from P-Chain
+
+TODO: we might not want last 3 checks (with ?) to minimize reverts. The contract should be able to handle these cases without reverting. This is because the timing of the messages is not guaranteed and the contract should be able to handle out-of-order messages.


@ceyonur

If there isn't a pending request for this message ID, doesn't that mean we've been sent something invalid?

The nodeID in the pending request being a validator already seems fine, we may send multiple requests to register.

How would we handle a message not from the P-Chain here? Shouldn't that revert, as a malicious subnet could send a similar message?

1- I guess yes, sending a msg from another subnet should be mostly covered before msg arrives here. But still we'd assume that it's not possible + we assume those pending requests get never deleted. Otherwise one can replay those messages.
2- in theory we can send many requests, but P-chain won't accept them if there is already a validator with that nodeID. We should definetly let multiple requests to be sent in case there is no validator (to prevent blocking that nodeID), but I was not %100 sure if we should let those if there is a known validator for that nodeID. Mostly to prevent replay attacks.
3- as told in 1 I was assuming the message would not possibly arrive at this point/not even signed.

Looking at this again, it now seems that we should probably add all possible checks.

Co-authored-by: Ceyhun Onur <ceyhun.onur@avalabs.org>

Signed-off-by: Jonathan Oppenheimer <jonathan.oppenheimer@avalabs.org> Co-authored-by: Austin Larson <78000745+alarso16@users.noreply.github.com> Co-authored-by: Ceyhun Onur <ceyhun.onur@avalabs.org>

ceyonur added 4 commits June 26, 2024 17:30

add example staking manager

f6387bb

add remove validator flow

f8310e8

add IStakingManager interface

5957b99

add staking-manager description

dbd09df

ceyonur commented Jul 15, 2024

View reviewed changes

add headers

c9dae3e

darioush reviewed Jul 15, 2024

View reviewed changes

ceyonur and others added 4 commits July 16, 2024 11:37

Update contracts/contracts/ExampleStakingManager.sol

7b8057c

Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>

Update ExampleStakingManager.sol

1f732b7

Signed-off-by: Ceyhun Onur <ceyhun.onur@avalabs.org>

add delegation, increase weight and exit validator set

1609534

Merge branch 'validator-manager' of github.com:ava-labs/subnet-evm in…

2883a23

…to validator-manager

geoff-vball reviewed Jul 19, 2024

View reviewed changes

ceyonur closed this Sep 16, 2024

JonathanOppenheimer pushed a commit that referenced this pull request Oct 8, 2025

Remove duplicate cancun checks (#1223)

2730a58

Co-authored-by: Ceyhun Onur <ceyhun.onur@avalabs.org>


		// TODO: add partial withdraw + increase stake

		// TODO: add delegation


		### receiveRemoveValidator

		Once the `SetSubnetValidatorWeightTx` is accepted, a warp message `InvalidValidatorRegisterMessage` will be relayed to the contract with `messageID` of the request. The message represents a messageID will not possibly become a valid validator. This essentialy means the message was not valid in the first place, or the validator has removed from the set. The InvalidValidatorRegisterMessage can either be a result of `removeValidator` from the contract or it can be originated directly from P-chain via `ExitValidatorSetTx`. In either case this ultimately tells the contract that a validator was evicted. See related ACP-77 section [here](https://github.com/avalanche-foundation/ACPs/tree/main/ACPs/77-reinventing-subnets#exitvalidatorsettx).


		Rewards should not be unlocked or sent to the staker at this point. This is because we need to first perform a uptime check to determine the rewards.

		TODO: should we give a partial reward in case the validator got removed from P-chain (balance drained)? Possibly by timestamping the `InvalidValidatorRegisterMessage`.


		TODO: should we give a partial reward in case the validator got removed from P-chain (balance drained)? Possibly by timestamping the `InvalidValidatorRegisterMessage`.

		TODO: should we add a cooldown period for validator removal/readd?

add example staking manager #1223

add example staking manager #1223

Uh oh!

Conversation

ceyonur commented Jun 26, 2024

Why this should be merged

How this works

How this was tested

How is this documented

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!

ceyonur Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

geoff-vball Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

ceyonur Jul 15, 2024 •

edited

Loading

geoff-vball Jul 19, 2024 •

edited

Loading