feat(evmstaking): implement epoched staking #96

Narangde · 2024-09-09T15:21:22Z

Implemention of epoched staking in evmstaking module.

With epoched staking, the messages are handled in 2 steps: queueing & processing. The messages from EL related to validator set are not executed immediately, but every epoch.

Queueing messages

When events related to validator set (CreateValidator, Deposit, Withdraw, Redelegate, and Unjail) are emitted from EL, the messages are queued in k.MessageQueue.

Processing messages

In EndBlock of evmstaking module, it is checked every block if the next epoch is started or not. If the next epoch is started, it executes all queued messages and return the updated validator set.

Epoch info

The epoch is managed in epochs keeper, so it needs to be injected to evmstaking keeper. The epoch_identifier for epoched staking is added to params of evmstaking.

issue: none

client/x/epochs/module/module.proto

scripts/buf.gen.gogo.yaml

Narangde · 2024-09-16T07:08:44Z

client/x/evmengine/types/expected_keepers.go

-	ProcessDeposit(ctx context.Context, ev *bindings.IPTokenStakingDeposit) error
-	ProcessWithdraw(ctx context.Context, ev *bindings.IPTokenStakingWithdraw) error


These two are not used in evmengine module

Narangde · 2024-09-17T09:37:18Z

@zsystm With this implementation, I fixed some tests of evmstaking module. Please review this 4c344d2.

zsystm · 2024-09-18T01:39:14Z

@Narangde
Since I’m on vacation today, I won’t be able to review the updated test code. I’ll review it tomorrow.

client/x/evmstaking/types/params.proto

client/x/evmstaking/types/evmstaking.proto

client/x/evmstaking/keeper/abci.go

ezreal1997 · 2024-09-18T14:05:47Z

client/x/evmstaking/keeper/abci.go

+
+	if isNextEpoch {
+		// process all queued messages
+		if err := k.ProcessAllMsgs(ctx); err != nil {


what if we have a lot msgs? any risk of block timeout here?

Epoch blocks usually take longer due to backlog calculations, e.g. Osmosis's daily epoch block takes up to a few minutes at worst. We should set settings to allow epoch blocks to exceed any timeout

Right. we need to allow longer block time for epoch block.

makes sense👍🏻

How are we thinking about setting a longer timeout for epoch blocks? I think the current timeout in config is set for all blocks

I'm not sure we can set longer timeout for epoch block, but if possible, it could be good. Why don't we make improvement in another PR?

We should definitely increase the timeout. I would imagine a lot of operations need to be processed during the epoch block. Can be in another PR

client/x/evmstaking/keeper/epochs.go

zsystm

comments for mocking usage

client/x/evmstaking/keeper/abci_test.go

Narangde · 2024-09-22T07:29:38Z

I added validation of epoch duration for epoched staking in a6c4fd5.

This is because if the unbonding period is shorter than the epoch duration, the unbonding is mature, but the staking keeper's EndBlocker may not process completeUnbonding and fail to send coins from the account to the module.

edisonz0718 · 2024-09-23T03:57:12Z

I added validation of epoch duration for epoched staking in a6c4fd5.

This is because if the unbonding period is shorter than the epoch duration, the unbonding is mature, but the staking keeper's EndBlocker may not process completeUnbonding and fail to send coins from the account to the module.

Could you elaborate this more? Why completeUnbonding is not processed if unbonding period is shorter than the epoch duration? I thought withdrawals (full or partial) should happen regardlessly.

jdubpark · 2024-09-23T19:47:16Z

FWIW, the current design implies that withdrawing a stake doesn't immediately lower the validator power, and only starts to unbond (for 21 days) at the start of each epoch.

jdubpark · 2024-09-21T04:14:57Z

client/x/evmstaking/keeper/msg_queue.go

+
+		if err := k.processMsg(ctx, &qMsg); err != nil {
+			log.Warn(ctx, "Failed to process queued message", err, "tx_id", string(qMsg.TxId))
+			return errors.Wrap(err, "process queued message")


I think we need to continue here, otherwise all queued messages after the error will not get processed. The same issue happened before while the evmstaking module processed events (incl. invalid ones) from the staking contract.

Also, we need to consider what it means for a queued message to fail. If a reward minting fails, why did it fail, and are there any side effects we need to resolve (e.g. bank module balance increasing)? In other words, gracefully handling failed messages in an epoch.

I agree that we continue processing messages. I just returned error here because it is used for our test codes. I will make it continue processing messages.

Also totally agree that we need to handle failed messages. This is also the part that I had the most trouble with. Since the case where a message fails can be quite complex to consider right now, I'm thinking of storing failed messages separately rather than handling them right away, so that we can handle them later appropriately for example via upgrade. What do you think?

jdubpark · 2024-09-23T19:44:37Z

client/x/evmstaking/keeper/msg_queue.go

+			return errors.Wrap(err, "process CreateValidator msg")
+		}
+	case *stype.MsgDelegate:
+		if err := k.ProcessDepositMsg(ctx, unwrappedMsg); err != nil {


In the future, maybe we can add an improvement where deposits are handled per block but only go into effect at the end of each epoch.

Narangde · 2024-09-23T23:29:45Z

Could you elaborate this more? Why completeUnbonding is not processed if unbonding period is shorter than the epoch duration? I thought withdrawals (full or partial) should happen regardlessly.

Because completeUnbonding is called in staking keeper's EndBlocker, which is called every epoch.
I will give an example of this. Let's assume the unbonding period is 10s and epoch duration is 60s. And epoch starts at time t.

t: epoch starts
t+10 withdraw tx submitted and queued
t+60 withdraw tx executed when the next epoch starts. Here, the unbonding complete time is t+70.
t+70 unbonding is completed, but completeUnbonding is not called yet because epoch is not passed, leading to failure in submit coins from account to module.

edisonz0718 · 2024-10-04T23:00:50Z

client/x/evmstaking/keeper/keeper.go

@@ -144,7 +149,7 @@ func (k Keeper) ProcessStakingEvents(ctx context.Context, height uint64, logs []
 				continue
 			}
 			ev.StakeAmount.Div(ev.StakeAmount, gwei)
-			if err = k.ProcessCreateValidator(ctx, ev); err != nil {
+			if err = k.HandleCreateValidatorEvent(ctx, ev); err != nil {


Can we combine all these events to a single enqueue since all of them share the same msg type

Yes, all types of events are converted to types.QueuedMessage and enqueue to a single queue. Did you mean to combine all Handle*Event to a single func?

I don't think we can combine all Handle*Event function to a single function. Because each event has different fields. If we want to make a single function, we need to move the logics for handling events (converting each event to QueuedMessage and enqueuing) out to ProcessStakingEvents. If I misunderstand your comment, please correct me.

edisonz0718 · 2024-10-07T22:13:46Z

client/x/evmstaking/keeper/keeper.go

@@ -144,7 +149,7 @@ func (k Keeper) ProcessStakingEvents(ctx context.Context, height uint64, logs []
 				continue
 			}
 			ev.StakeAmount.Div(ev.StakeAmount, gwei)
-			if err = k.ProcessCreateValidator(ctx, ev); err != nil {
+			if err = k.HandleCreateValidatorEvent(ctx, ev); err != nil {


edisonz0718 · 2024-10-07T22:21:52Z

client/x/evmstaking/keeper/withdraw.go

@@ -268,7 +270,7 @@ func (k Keeper) ProcessWithdraw(ctx context.Context, ev *bindings.IPTokenStaking

 	amountCoin, _ := IPTokenToBondCoin(ev.Amount)

-	log.Debug(ctx, "Processing EVM staking withdraw",
+	log.Info(ctx, "EVM staking withdraw detected",


I think we should keep this as debug. I'm thinking all tx level information are more for debugging purpose.

For other events, the log level in Info when the event is detected, thus I changed this to Info as well for code consistency. I think Debug is enough when the event is detected. How about we changing all log for event detection to Debug level?

@edisonz0718, could you check this comment? As you said, it seems like to be better that we use Debug for all tx log as done in 36a50a2

edisonz0718 · 2024-10-07T22:26:37Z

client/x/evmstaking/keeper/abci.go

-			delEvmAddr,
-			entry.amount.Uint64(),
-		))
+		partialWithdrawals, err := k.ExpectedPartialWithdrawals(ctx)


I think the partial withdrawal should still happen per block.

You're right.

edisonz0718 · 2024-10-08T00:01:25Z

lib/netconf/local/genesis.json

-        "min_partial_withdrawal_amount": 100000000
-      }
+        "min_partial_withdrawal_amount": 100000000,
+        "epoch_identifier": "minute"


How do we add epoch duration in genesis

Only epoch identifier is needed in evmstaking module. Epoch info such as epoch duration is in epochs module, and it can be added in genesis.json for now. There is no way to add epoch info as cosmos-sdk do.

It will be great to add configuration in genesis.json to change the number of blocks per epoch. It helps to perform tests in reasonable amount of time.

The epoch duration can be set in epochs module. For local or mininet test, we can use shorter epoch by setting shorter epoch. emvstaking module depends on epochs module

If we can set the duration in genesis.json, it will make setting different networks easier. Otherwise, different networks need to manage different binaries.

I think it's ok if each network has different genesis.json file (to be exact, different epoch identifier for epoch staking).

If we add duration in evmstaking param (and add to genesis.json), we don't need epochs module in evmstaking module and we don't need epochs module in Story for now. evmstaking module can be independent with epochs module. The reason why I have added and used epochs module for epoch staking is that I wanna make epochs module manage epoch stuffs in our app for modularity. Do you think that it better than current design that evmstaking module have independent epoch info for epoch staking?

Yeah I think we should add epoch info we use for evmstaking to epochs module's genesis file. I guess what you meant is that it should be in another PR?

Ah, I got it. I may misunderstand your comment.
Sure! I will add it in the following PR.

Done in #181

limengformal · 2024-10-08T01:12:38Z

lib/netconf/local/genesis.json

+        "min_partial_withdrawal_amount": 100000000,
+        "epoch_identifier": "minute"
+      },
+      "epoch_number": "0"


Is this the starting number of epoch?

It is the epoch number currently used in evmstaking module for epoch staking. This value is used for checking the next epoch is started in epochs module by comparing epoch number.

limengformal · 2024-10-08T01:13:32Z

lib/netconf/local/genesis.json

-        "min_partial_withdrawal_amount": 100000000
-      }
+        "min_partial_withdrawal_amount": 100000000,
+        "epoch_identifier": "minute"


It will be great to add configuration in genesis.json to change the number of blocks per epoch. It helps to perform tests in reasonable amount of time.

ezreal1997 · 2024-10-08T20:40:35Z

client/x/evmstaking/keeper/abci.go

-		if err != nil {
-			return nil, errors.Wrap(err, "map delegator pubkey to evm address")
+		// init message queue
+		if err := k.MessageQueue.Initialize(ctx); err != nil {


will all processed msgs pruned after re-init the MessageQueue?

Do we initialize every epoch? I thought there is a max amount that one epoch can process and the rest will flow to the next epoch

In the current PR, there is no limit in size of queue, that is, process all queued msgs.
With limitation, we should not initialize queue as you said.

limengformal · 2024-10-09T19:21:00Z

client/x/evmstaking/keeper/abci.go

+						unbondedEntries = append(unbondedEntries, UnbondedEntry{
+							validatorAddress: dvPair.ValidatorAddress,
+							delegatorAddress: dvPair.DelegatorAddress,
+							amount:           amt.Amount,
+						})


I thought we have a limit on the number of operations per unbonding period or epoch. Do we know where that is enforced?

As commented here, no limit in this PR. Will do in the following PR.

limengformal · 2024-10-09T19:23:06Z

client/x/evmstaking/keeper/abci.go

+
+	if isNextEpoch {
+		// process all queued messages
+		if err := k.ProcessAllMsgs(ctx); err != nil {


We should definitely increase the timeout. I would imagine a lot of operations need to be processed during the epoch block. Can be in another PR

limengformal · 2024-10-09T19:25:37Z

client/x/evmstaking/keeper/abci.go

-		if err != nil {
-			return nil, errors.Wrap(err, "map delegator pubkey to evm address")
+		// init message queue
+		if err := k.MessageQueue.Initialize(ctx); err != nil {


Do we initialize every epoch? I thought there is a max amount that one epoch can process and the rest will flow to the next epoch

edisonz0718 · 2024-10-09T22:41:46Z

Based on the comments, there are still two changes to be made:

We need a maximum length for the message queue. Can set it to 40,000 for now. Also, we only process X requests from the queue per epoch. We can set it to 4096 first.
Increase the timeout for the epoch block.

You can create issues and add them in a separate PR if it's a bigger change.

0xHansLee · 2024-10-10T07:44:02Z

Based on the comments, there are still two changes to be made:

We need a maximum length for the message queue. Can set it to 40,000 for now. Also, we only process X requests from the queue per epoch. We can set it to 4096 first.
Increase the timeout for the epoch block.
You can create issues and add them in a separate PR if it's a bigger change.

Thank you for your clarification. I will make another PR for the above things.

github-actions · 2024-10-10T09:00:40Z

Binary uploaded successfully 🎉

📦 Version Name: 0.10.1-unstable-91a4806
📦 Download Source: AWS S3

This reverts commit 91a4806.

LeoHChen reviewed Sep 9, 2024

View reviewed changes

client/x/epochs/module/module.proto Outdated Show resolved Hide resolved

Narangde force-pushed the feat/implement-epoched-staking branch from 6d2c56c to 1954660 Compare September 16, 2024 07:03

Narangde commented Sep 16, 2024

View reviewed changes

scripts/buf.gen.gogo.yaml Show resolved Hide resolved

Narangde commented Sep 16, 2024

View reviewed changes

Narangde force-pushed the feat/implement-epoched-staking branch from 1954660 to 17ce356 Compare September 17, 2024 08:15

Narangde changed the title ~~feat(wip): implement epoched staking~~ feat(evmstaking): implement epoched staking Sep 17, 2024

Narangde requested a review from zsystm September 17, 2024 09:37

Narangde marked this pull request as ready for review September 17, 2024 09:37

Narangde requested review from AndyBoWu, jdubpark, edisonz0718, ezreal1997, limengformal and leeren as code owners September 17, 2024 09:37

edisonz0718 reviewed Sep 18, 2024

View reviewed changes

client/x/evmstaking/types/params.proto Outdated Show resolved Hide resolved

client/x/evmstaking/types/evmstaking.proto Outdated Show resolved Hide resolved

client/x/evmstaking/keeper/abci.go Show resolved Hide resolved

client/x/evmstaking/keeper/abci.go Outdated Show resolved Hide resolved

Narangde force-pushed the feat/implement-epoched-staking branch 2 times, most recently from d04622b to 4e25327 Compare September 18, 2024 09:31

ezreal1997 reviewed Sep 18, 2024

View reviewed changes

zsystm reviewed Sep 19, 2024

View reviewed changes

client/x/evmstaking/keeper/abci_test.go Outdated Show resolved Hide resolved

Narangde force-pushed the feat/implement-epoched-staking branch from 99c399e to e3589d7 Compare September 21, 2024 08:54

edisonz0718 closed this Sep 23, 2024

edisonz0718 reopened this Sep 23, 2024

jdubpark reviewed Sep 23, 2024

View reviewed changes

Narangde force-pushed the feat/implement-epoched-staking branch from a6c4fd5 to b4dc947 Compare September 28, 2024 10:12

edisonz0718 reviewed Oct 4, 2024

View reviewed changes

edisonz0718 reviewed Oct 7, 2024

View reviewed changes

edisonz0718 reviewed Oct 8, 2024

View reviewed changes

limengformal reviewed Oct 8, 2024

View reviewed changes

0xHansLee force-pushed the feat/implement-epoched-staking branch 2 times, most recently from 823ad83 to 22d3af8 Compare October 8, 2024 06:16

ezreal1997 reviewed Oct 8, 2024

View reviewed changes

edisonz0718 approved these changes Oct 9, 2024

View reviewed changes

0xHansLee force-pushed the feat/implement-epoched-staking branch from 22d3af8 to 44d0595 Compare October 9, 2024 09:12

limengformal reviewed Oct 9, 2024

View reviewed changes

limengformal approved these changes Oct 9, 2024

View reviewed changes

0xHansLee force-pushed the feat/implement-epoched-staking branch from 36a50a2 to 9c976f4 Compare October 10, 2024 08:05

Narangde added 10 commits October 10, 2024 17:53

feat(evmstaking): implement epoched staking

1528198

fix(evmstaking): fix tests with implementation of epoched staking

05a1e16

perf(evmstaking): improve processing queued msg

c7db4e3

fix(evmstaking): fix tests by adding epoch identifier

d78d924

test(evmstaking): add tests for epoch identifier param

c2286a5

chore(evmstaking): change log level of withdraw

3bfbd16

feat(evmstaking): add validation of epoch duration

aba6e15

chore(evmstaking): continue processing msg when failed

4d7fac5

chore(evmstaking): process mature unbonding every epoch

9c975bf

feat(evmstaking): move partial withdraw out of epoch

71fbbde

0xHansLee force-pushed the feat/implement-epoched-staking branch from 81e0517 to 71fbbde Compare October 10, 2024 08:54

0xHansLee merged commit 91a4806 into main Oct 10, 2024
6 checks passed

0xHansLee deleted the feat/implement-epoched-staking branch October 10, 2024 08:57

0xHansLee added a commit that referenced this pull request Oct 13, 2024

Revert "feat(evmstaking): implement epoch staking (#96)"

2c5f43d

This reverts commit 91a4806.

0xHansLee restored the feat/implement-epoched-staking branch October 13, 2024 07:46

		ProcessDeposit(ctx context.Context, ev *bindings.IPTokenStakingDeposit) error
		ProcessWithdraw(ctx context.Context, ev *bindings.IPTokenStakingWithdraw) error

feat(evmstaking): implement epoched staking #96

feat(evmstaking): implement epoched staking #96

Conversation

Narangde commented Sep 9, 2024 • edited Loading

Queueing messages

Processing messages

Epoch info

Choose a reason for hiding this comment

Narangde commented Sep 17, 2024

zsystm commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsystm left a comment

Choose a reason for hiding this comment

Narangde commented Sep 22, 2024

edisonz0718 commented Sep 23, 2024 • edited Loading

jdubpark commented Sep 23, 2024

Choose a reason for hiding this comment

Narangde Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narangde commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xHansLee Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edisonz0718 commented Oct 9, 2024 • edited Loading

0xHansLee commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

Binary uploaded successfully 🎉

Narangde commented Sep 9, 2024 •

edited

Loading

edisonz0718 commented Sep 23, 2024 •

edited

Loading

Narangde Sep 28, 2024 •

edited

Loading

0xHansLee Oct 9, 2024 •

edited

Loading

edisonz0718 commented Oct 9, 2024 •

edited

Loading