feat(sequencer): hardforking when rotating to sentinel #1455

mtsitrin · 2024-11-10T17:40:57Z

codecov · 2024-11-10T17:56:45Z

Codecov Report

Attention: Patch coverage is 73.41772% with 21 lines in your changes missing coverage. Please review.

Please upload report for BASE (mtsitrin/937-rollapp-hard-fork-hub-side@c25d2bc). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
x/sequencer/keeper/rotation.go	61.11%	9 Missing and 5 partials ⚠️
x/sequencer/keeper/fraud.go	75.00%	2 Missing and 2 partials ⚠️
x/sequencer/keeper/msg_server_create.go	0.00%	0 Missing and 1 partial ⚠️
x/sequencer/keeper/msg_server_update.go	0.00%	0 Missing and 1 partial ⚠️
x/sequencer/types/hooks.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@                            Coverage Diff                             @@
##             mtsitrin/937-rollapp-hard-fork-hub-side    #1455   +/-   ##
==========================================================================
  Coverage                                           ?   22.70%           
==========================================================================
  Files                                              ?      575           
  Lines                                              ?   124311           
  Branches                                           ?        0           
==========================================================================
  Hits                                               ?    28227           
  Misses                                             ?    92312           
  Partials                                           ?     3772

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

#1457) Co-authored-by: danwt <[email protected]>

danwt

TBH to me it's becoming a lot more complicated and coupling x/sequencer with hard fork

I'm not sure why you can't just trigger hard fork in AfterChooseNewProposer hook on rollapp keeper if after is sentinel

This is now drifting a lot from the original spec https://www.notion.so/dymension/Rollapp-Hard-Fork-Hub-POV-10aa4a51f86a80a6a1d2ca8fcca9e68b?pvs=4#c310c5683fff48b490a0a5e403aa5fc0

Which had a very clear decoupling, and was IMO, easy and simple

omritoptix · 2024-11-11T15:06:42Z

x/sequencer/keeper/proposer.go

+func (k Keeper) RecoverFromSentinelProposerIfNeeded(ctx sdk.Context, rollapp string) error {
 	proposer := k.GetProposer(ctx, rollapp)
-	before := proposer

 	// a valid proposer is already set so there's no need to do anything
 	if !proposer.Sentinel() {
 		return nil
 	}


I find this function name and inner condition rather confusing as it tries to encapsulate various use cases and do multiple things in one function which make it non reusable. and from some call site (like the kick) I find it cumbersome.

This function does:

check if new proposer is needed

set new proposer

set new successor

for (1) - we can check it outside the function based on the callsite
for (2) - I think this all it should do
for (3) - we have the setSuccessorForRollapp. isn't it the same? shouldn't we just call it from callsite?

it's clearer to just make this function do one thing (i.e recoverFromSentinel/UpdateProposer) and simply check the condition and other work outside this function, where necessary.

Currently this function is being called from 3 callsites:

create sequencer (need to check if current proposer is sentinel outside)

update sequencer (need to check if current proposer is sentinel and opted in before call)

kick (no need to check if current proposer is sentinel, as we're kicking)

omritoptix · 2024-11-11T15:10:31Z

x/sequencer/keeper/proposer.go

-	k.SetProposer(ctx, rollapp, successor.Address)
-	k.SetSuccessor(ctx, rollapp, types.SentinelSeqAddr)
-	if k.GetProposer(ctx, rollapp).Sentinel() {
+	// if successor is sentinel, we attempt to find a non sentinel successor


at what point will the successor be not sentinel, if we only reached this part if the proposer is sentinel?

yeah not sure
refactored

omritoptix · 2024-11-11T15:15:00Z

x/sequencer/keeper/proposer.go

+	k.SetProposer(ctx, rollapp, successor.Address)
+	k.SetSuccessor(ctx, rollapp, types.SentinelSeqAddr)
+
+	if !successor.Sentinel() {


when will it not be the case given the call sites? (i.e we only call it from create/update/kick)

omritoptix · 2024-11-11T15:21:33Z

x/sequencer/keeper/rotation.go

 	k.SetSuccessor(ctx, rollapp, types.SentinelSeqAddr)

+	// if proposer is sentinel, prepare new revision for the rollapp
+	if successor.Sentinel() {
+		err := k.rollappKeeper.HardForkToLatest(ctx, rollapp)


why do we need to hard fork here? can't we just hard fork when the new sequencer bonds? otherwise we gonna bump 2 revisions when the sequencer will come.

if we don't hard fork here than already in the RecoverFromHaltIfNeeded should cover it no?

I didn't plan on hard forking on RecoverFromHaltIfNeeded, just to notify x/rollapp so it could update the last state info with nextProposer.
in case of fraud proposal, RecoverFromHaltIfNeeded will be called later on, so hard fork already happend

omritoptix · 2024-11-11T15:22:52Z

x/sequencer/keeper/rotation.go

+
+// setSuccessorForRollapp will assign a successor. It won't replace an existing one.
+// It will prioritize non sentinel
+func (k Keeper) setSuccessorForRollapp(ctx sdk.Context, rollapp string) error {


this seems like a repeat of part of the RecoverFromHaltIfNeeded function.
also I suggest for standartization setSuccessorForRollapp -> chooseSuccessor

omritoptix · 2024-11-11T15:56:23Z

x/sequencer/keeper/rotation.go

+	successor := k.GetSuccessor(ctx, rollapp)
+	if !successor.Sentinel() {
+		// a valid successor is already set so there's no need to do anything
+		return nil
+	}


as to my other comment of the RecoverFromHaltIfNeeded would sugget to remove outside the function.

omritoptix · 2024-11-11T15:57:56Z

x/sequencer/keeper/rotation.go

+		return types.Sequencer{}, gerrc.ErrInternal.Wrap("seqs must at least include sentinel")
+	}
+	// slices package is recommended over sort package
+	slices.SortStableFunc(seqs, func(a, b types.Sequencer) int {


shouldn't we filter out the opted-out one?

it's filtered out in RollappPotentialProposers

mtsitrin · 2024-11-12T08:52:25Z

TBH to me it's becoming a lot more complicated and coupling x/sequencer with hard fork

I'm not sure why you can't just trigger hard fork in AfterChooseNewProposer hook on rollapp keeper if after is sentinel

This is now drifting a lot from the original spec https://www.notion.so/dymension/Rollapp-Hard-Fork-Hub-POV-10aa4a51f86a80a6a1d2ca8fcca9e68b?pvs=4#c310c5683fff48b490a0a5e403aa5fc0

Which had a very clear decoupling, and was IMO, easy and simple

pls be more specific
what I did was:

when rollapp left with no proposer, prepare a new revision.
it allows the L2 to sync to the expected revision and just wait for a new proposer
when new proposer bonds after halt, we only need to modify the last state info with nextProposer

we have the fraud proposal which leaves the rollapp with bumped revision and no proposer.
I've tried to have the other flows act similar

danwt · 2024-11-12T10:42:37Z

pls be more specific

Sorry I will try to be more specific

having another look

danwt

Left a few concrete comments.

But I do have reservations about the factoring and architecture here now. In my opinion, things are now less dry, and there are more unique paths through the code, which makes things more difficult

Ive tried to illustrate how I can see things working, which is closer with the original flow as described in the research. Maybe it's interesting for you. LMK

danwt · 2024-11-12T10:56:26Z

x/sequencer/keeper/fraud.go

+	err := k.TryUnbond(ctx, &proposer, proposer.TokensCoin())
+	if err != nil {
+		return errorsmod.Wrap(err, "try unbond")
 	}


why is it try unbond vs unbond?

I'll rethink and add comment on the main PR

danwt · 2024-11-12T10:59:20Z

x/sequencer/keeper/proposer.go

+// RecoverFromHalt will assign a new proposer to the rollapp.
+// It will choose a new proposer from the list of potential proposers.
+// The rollapp must
+func (k Keeper) RecoverFromHalt(ctx sdk.Context, rollapp string) error {


Why is it called recover from halt?
It's just ensuring that there is a proposer

// The rollapp must

Unfinished sentence

it's supposed to be called only when proposer == nil and successor != nil
so it's recover from halt

danwt · 2024-11-12T11:15:35Z

x/sequencer/keeper/msg_server_update.go

 	// maybe set as proposer if one is needed
-	if err := k.UpdateProposerIfNeeded(ctx, seq.RollappId); err != nil {
-		return nil, errorsmod.Wrap(err, "choose proposer")
+	proposer := k.GetProposer(ctx, seq.RollappId)
+	if proposer.Sentinel() {
+		if err := k.RecoverFromHalt(ctx, seq.RollappId); err != nil {
+			return nil, err
+		}
 	}


The original chooseProposer function did exactly this. It chose one if one is needed. Not sure why it's factored differently now

for clarity and readability. by @omritoptix 's suggestion
if one is needed. checked by the caller

danwt · 2024-11-12T11:20:17Z

x/sequencer/keeper/rotation.go

+// setSuccessorForRotatingRollapp will assign a successor to the rollapp.
+// It will prioritize non sentinel
+// called when a proposer has finished their notice period.
+func (k Keeper) setSuccessorForRotatingRollapp(ctx sdk.Context, rollapp string) error {


can't this just be chooseSuccessor

it's called only in rotation flow, so I thought it will be more clear

danwt · 2024-11-12T11:24:54Z

x/sequencer/keeper/msg_server_create.go

+	proposer := k.GetProposer(ctx, msg.RollappId)
+	if proposer.Sentinel() {
+		if err := k.RecoverFromHalt(ctx, msg.RollappId); err != nil {
+			return nil, err
+		}


ditto my other comment
This was already encapsulated in the old chooseProposer func, not sure why it's factored differently now

danwt · 2024-11-12T11:27:15Z

x/sequencer/keeper/bond.go

 	}
+	k.SetSequencer(ctx, *seq)


why add this?
The function is taking pointer to sequencer, it should be caller responsibility to set it

this function does different stuff (e.g refund)
seems wrong to complete the function without storing the changes on the seq object
wdyt?
I see now that I have duplicate call to k.SetSequencer(ctx, *seq) by the caller as well

hardforking when rotating to sentinel

9b8ce93

github-actions bot added the dym-internal label Nov 10, 2024

mtsitrin marked this pull request as ready for review November 10, 2024 18:28

mtsitrin requested a review from a team as a code owner November 10, 2024 18:28

mtsitrin linked an issue Nov 10, 2024 that may be closed by this pull request

force hard fork when going into "halt" #1430

Closed

mtsitrin requested review from omritoptix and danwt November 10, 2024 19:21

feat(hardfork): clearing rollapp's denom metadata in case of hard fork (

d22358c

#1457) Co-authored-by: danwt <[email protected]>

danwt reviewed Nov 11, 2024

View reviewed changes

omritoptix requested changes Nov 11, 2024

View reviewed changes

mtsitrin added 2 commits November 12, 2024 08:59

cleared the code abit

23aac93

renames

e14c0f5

linter

2b7e9f9

mtsitrin requested a review from omritoptix November 12, 2024 10:03

mtsitrin merged commit 93a59c8 into mtsitrin/937-rollapp-hard-fork-hub-side Nov 12, 2024
5 checks passed

mtsitrin deleted the mtsitrin/1430-force-hard-fork-when-going-into-halt branch November 12, 2024 11:29

mtsitrin mentioned this pull request Nov 12, 2024

feat(hard-fork): Implement rollapp hard fork #1354

Merged

danwt reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequencer): hardforking when rotating to sentinel #1455

feat(sequencer): hardforking when rotating to sentinel #1455

mtsitrin commented Nov 10, 2024

codecov bot commented Nov 10, 2024

danwt left a comment

omritoptix Nov 11, 2024

mtsitrin Nov 12, 2024

omritoptix Nov 11, 2024

mtsitrin Nov 12, 2024

omritoptix Nov 11, 2024

mtsitrin Nov 12, 2024

omritoptix Nov 11, 2024

mtsitrin Nov 11, 2024

omritoptix Nov 11, 2024

omritoptix Nov 11, 2024

omritoptix Nov 11, 2024

mtsitrin Nov 12, 2024

mtsitrin commented Nov 12, 2024

danwt commented Nov 12, 2024

danwt left a comment

danwt Nov 12, 2024

mtsitrin Nov 12, 2024

danwt Nov 12, 2024

mtsitrin Nov 12, 2024

danwt Nov 12, 2024

mtsitrin Nov 12, 2024

danwt Nov 12, 2024

mtsitrin Nov 12, 2024

danwt Nov 12, 2024

danwt Nov 12, 2024

mtsitrin Nov 12, 2024

feat(sequencer): hardforking when rotating to sentinel #1455

feat(sequencer): hardforking when rotating to sentinel #1455

Conversation

mtsitrin commented Nov 10, 2024

codecov bot commented Nov 10, 2024

Codecov Report

danwt left a 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

mtsitrin commented Nov 12, 2024

danwt commented Nov 12, 2024

danwt left a 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