Fork manager handlers precedence (#1878)

forkmanager/fork.go

@@ -4,22 +4,29 @@ import "github.com/0xPolygon/polygon-edge/helper/common"
 
 const InitialFork = "initialfork"
 
-// HandlerDesc gives description for the handler
-// eq: "extra", "proposer_calculator", etc
+// HandlerDesc gives description for the handler eq: "extra", "proposer_calculator", etc
 type HandlerDesc string
 
+// HandlerContainer keeps id of a handler and actual handler
+type HandlerContainer struct {
+	// ID represents an auto-incremented identifier for a handler
+	ID uint
+	// Handler represents an actual event handler
+	Handler interface{}
+}
+
 // Fork structure defines one fork
 type Fork struct {
-	// name of the fork
+	// Name is name of the fork
 	Name string
-	// after the fork is activated, `FromBlockNumber` shows from which block is enabled
+	// FromBlockNumber indicates the block from which fork becomes enabled
 	FromBlockNumber uint64
-	// fork consensus parameters
+	// Params are fork consensus parameters
 	Params *ForkParams
-	// this value is false if fork is registered but not activated
+	// IsActive is false if fork is registered but not activated
 	IsActive bool
-	// map of all handlers registered for this fork
-	Handlers map[HandlerDesc]interface{}
+	// Handlers is a map of all handlers registered for this fork
+	Handlers map[HandlerDesc]HandlerContainer
 }
 
 // ForkParams hard-coded fork params
@@ -59,16 +66,18 @@ func (fp *ForkParams) Copy() *ForkParams {
 
 // forkHandler defines one custom handler
 type forkHandler struct {
-	// Handler should be active from block `FromBlockNumber``
-	FromBlockNumber uint64
-	// instance of some structure, function etc
-	Handler interface{}
+	// id - if two handlers start from the same block number, the one with the greater ID should take precedence.
+	id uint
+	// fromBlockNumber defines block number after handler should be active
+	fromBlockNumber uint64
+	// handler represents an actual event handler - instance of some structure, function etc
+	handler interface{}
 }
 
 // forkParamsBlock encapsulates block and actual fork params
 type forkParamsBlock struct {
-	// Params should be active from block `FromBlockNumber``
-	FromBlockNumber uint64
-	// pointer to fork params
-	Params *ForkParams
+	// fromBlockNumber defines block number after params should be active
+	fromBlockNumber uint64
+	// params is a pointer to fork params
+	params *ForkParams
 }

forkmanager/fork_manager.go

@@ -18,6 +18,8 @@ type forkManager struct {
 	forkMap     map[string]*Fork
 	handlersMap map[HandlerDesc][]forkHandler
 	params      []forkParamsBlock
+
+	handlerIDCnt uint
 }
 
 // GeInstance returns fork manager singleton instance. Thread safe
@@ -51,7 +53,7 @@ func (fm *forkManager) RegisterFork(name string, forkParams *ForkParams) {
 		FromBlockNumber: 0,
 		IsActive:        false,
 		Params:          forkParams,
-		Handlers:        map[HandlerDesc]interface{}{},
+		Handlers:        map[HandlerDesc]HandlerContainer{},
 	}
 }
 
@@ -65,7 +67,12 @@ func (fm *forkManager) RegisterHandler(forkName string, handlerName HandlerDesc,
 		return fmt.Errorf("fork does not exist: %s", forkName)
 	}
 
-	fork.Handlers[handlerName] = handler
+	fm.handlerIDCnt++
+
+	fork.Handlers[handlerName] = HandlerContainer{
+		ID:      fm.handlerIDCnt,
+		Handler: handler,
+	}
 
 	return nil
 }
@@ -114,8 +121,8 @@ func (fm *forkManager) DeactivateFork(forkName string) error {
 
 	fork.IsActive = false
 
-	for forkHandlerName := range fork.Handlers {
-		fm.removeHandler(forkHandlerName, fork.FromBlockNumber)
+	for handlerName, handlerCont := range fork.Handlers {
+		fm.removeHandler(handlerName, fork.FromBlockNumber, handlerCont.ID)
 	}
 
 	fm.removeParams(fork.FromBlockNumber)
@@ -135,13 +142,13 @@ func (fm *forkManager) GetHandler(name HandlerDesc, blockNumber uint64) interfac
 
 	// binary search to find the latest handler defined for a specific block
 	pos := sort.Search(len(handlers), func(i int) bool {
-		return handlers[i].FromBlockNumber > blockNumber
+		return handlers[i].fromBlockNumber > blockNumber
 	}) - 1
 	if pos < 0 {
 		return nil
 	}
 
-	return handlers[pos].Handler
+	return handlers[pos].handler
 }
 
 // GetParams retrieves chain.ForkParams for a block number
@@ -151,13 +158,13 @@ func (fm *forkManager) GetParams(blockNumber uint64) *ForkParams {
 
 	// binary search to find the desired *chain.ForkParams
 	pos := sort.Search(len(fm.params), func(i int) bool {
-		return fm.params[i].FromBlockNumber > blockNumber
+		return fm.params[i].fromBlockNumber > blockNumber
 	}) - 1
 	if pos < 0 {
 		return nil
 	}
 
-	return fm.params[pos].Params
+	return fm.params[pos].params
 }
 
 // IsForkRegistered checks if fork is registered
@@ -200,47 +207,47 @@ func (fm *forkManager) GetForkBlock(name string) (uint64, error) {
 	return fork.FromBlockNumber, nil
 }
 
-func (fm *forkManager) addHandler(handlerName HandlerDesc, blockNumber uint64, handler interface{}) {
+func (fm *forkManager) addHandler(handlerName HandlerDesc, blockNumber uint64, handlerCont HandlerContainer) {
 	if handlers, exists := fm.handlersMap[handlerName]; !exists {
 		fm.handlersMap[handlerName] = []forkHandler{
 			{
-				FromBlockNumber: blockNumber,
-				Handler:         handler,
+				id:              handlerCont.ID,
+				fromBlockNumber: blockNumber,
+				handler:         handlerCont.Handler,
 			},
 		}
 	} else {
 		// keep everything in sorted order
 		index := sort.Search(len(handlers), func(i int) bool {
-			return handlers[i].FromBlockNumber >= blockNumber
-		})
-		// replace existing handler if on the same block as current one
-		if index < len(handlers) && handlers[index].FromBlockNumber == blockNumber {
-			handlers[index].Handler = handler
+			h := handlers[i]
 
-			return
-		}
+			return h.fromBlockNumber > blockNumber || (h.fromBlockNumber == blockNumber && h.id >= handlerCont.ID)
+		})
 
 		handlers = append(handlers, forkHandler{})
 		copy(handlers[index+1:], handlers[index:])
 		handlers[index] = forkHandler{
-			FromBlockNumber: blockNumber,
-			Handler:         handler,
+			id:              handlerCont.ID,
+			fromBlockNumber: blockNumber,
+			handler:         handlerCont.Handler,
 		}
 		fm.handlersMap[handlerName] = handlers
 	}
 }
 
-func (fm *forkManager) removeHandler(handlerName HandlerDesc, blockNumber uint64) {
+func (fm *forkManager) removeHandler(handlerName HandlerDesc, blockNumber uint64, id uint) {
 	handlers, exists := fm.handlersMap[handlerName]
 	if !exists {
 		return
 	}
 
 	index := sort.Search(len(handlers), func(i int) bool {
-		return handlers[i].FromBlockNumber >= blockNumber
+		h := handlers[i]
+
+		return h.fromBlockNumber > blockNumber || (h.fromBlockNumber == blockNumber && h.id >= id)
 	})
 
-	if index < len(handlers) && handlers[index].FromBlockNumber == blockNumber {
+	if index < len(handlers) && handlers[index].fromBlockNumber == blockNumber && handlers[index].id == id {
 		copy(handlers[index:], handlers[index+1:])
 		handlers[len(handlers)-1] = forkHandler{}
 		fm.handlersMap[handlerName] = handlers[:len(handlers)-1]
@@ -252,14 +259,14 @@ func (fm *forkManager) addParams(blockNumber uint64, params *ForkParams) {
 		return
 	}
 
-	item := forkParamsBlock{FromBlockNumber: blockNumber, Params: params}
+	item := forkParamsBlock{fromBlockNumber: blockNumber, params: params}
 
 	if len(fm.params) == 0 {
 		fm.params = append(fm.params, item)
 	} else {
 		// keep everything in sorted order
 		index := sort.Search(len(fm.params), func(i int) bool {
-			return fm.params[i].FromBlockNumber >= blockNumber
+			return fm.params[i].fromBlockNumber >= blockNumber
 		})
 
 		fm.params = append(fm.params, forkParamsBlock{})
@@ -268,22 +275,22 @@ func (fm *forkManager) addParams(blockNumber uint64, params *ForkParams) {
 
 		if index > 0 {
 			// copy all nil parameters from previous
-			copyParams(item.Params, fm.params[index-1].Params)
+			copyParams(item.params, fm.params[index-1].params)
 		}
 
 		// update parameters for next
 		for i := index; i < len(fm.params)-1; i++ {
-			copyParams(fm.params[i+1].Params, fm.params[i].Params)
+			copyParams(fm.params[i+1].params, fm.params[i].params)
 		}
 	}
 }
 
 func (fm *forkManager) removeParams(blockNumber uint64) {
 	index := sort.Search(len(fm.params), func(i int) bool {
-		return fm.params[i].FromBlockNumber >= blockNumber
+		return fm.params[i].fromBlockNumber >= blockNumber
 	})
 
-	if index < len(fm.params) && fm.params[index].FromBlockNumber == blockNumber {
+	if index < len(fm.params) && fm.params[index].fromBlockNumber == blockNumber {
 		copy(fm.params[index:], fm.params[index+1:])
 		fm.params[len(fm.params)-1] = forkParamsBlock{}
 		fm.params = fm.params[:len(fm.params)-1]

forkmanager/fork_manager_test.go

@@ -54,6 +54,17 @@ func TestForkManager(t *testing.T) {
 	assert.Equal(t, 2, handlersACnt)
 	assert.Equal(t, 3, handlersBCnt)
 
+	// double deactivate should be same as single deactivate
+	assert.NoError(t, forkManager.DeactivateFork(ForkA))
+	assert.NoError(t, forkManager.DeactivateFork(ForkA))
+	assert.Equal(t, 1, len(forkManager.handlersMap[HandlerA]))
+	assert.Equal(t, 2, len(forkManager.handlersMap[HandlerB]))
+
+	// activate fork again
+	assert.NoError(t, forkManager.ActivateFork(ForkA, 0))
+	assert.Equal(t, handlersACnt, len(forkManager.handlersMap[HandlerA]))
+	assert.Equal(t, handlersBCnt, len(forkManager.handlersMap[HandlerB]))
+
 	t.Run("activate not registered fork", func(t *testing.T) {
 		t.Parallel()
 
@@ -70,6 +81,12 @@ func TestForkManager(t *testing.T) {
 		assert.Equal(t, handlersBCnt, len(forkManager.handlersMap[HandlerB]))
 	})
 
+	t.Run("deactivate not registered fork", func(t *testing.T) {
+		t.Parallel()
+
+		assert.Error(t, forkManager.DeactivateFork(ForkE))
+	})
+
 	t.Run("is fork enabled", func(t *testing.T) {
 		t.Parallel()
 
@@ -274,3 +291,64 @@ func TestForkManager_HandlerReplacement(t *testing.T) {
 		assert.Equal(t, "ADH", execute(HandlerA, i+10))
 	}
 }
+
+func TestForkManager_HandlerPrecedence(t *testing.T) {
+	t.Parallel()
+
+	forkManager := &forkManager{
+		forkMap:     map[string]*Fork{},
+		handlersMap: map[HandlerDesc][]forkHandler{},
+	}
+
+	execute := func(name HandlerDesc, block uint64) string {
+		//nolint:forcetypeassert
+		return forkManager.GetHandler(name, block).(func() string)()
+	}
+
+	forkManager.RegisterFork(ForkA, nil)
+	forkManager.RegisterFork(ForkB, nil)
+	forkManager.RegisterFork(ForkC, nil)
+	forkManager.RegisterFork(ForkD, nil)
+	forkManager.RegisterFork(ForkE, nil)
+
+	assert.NoError(t, forkManager.RegisterHandler(ForkA, HandlerA, func() string { return "A" }))
+	assert.NoError(t, forkManager.RegisterHandler(ForkB, HandlerA, func() string { return "B" }))
+	assert.NoError(t, forkManager.RegisterHandler(ForkC, HandlerA, func() string { return "C" }))
+	assert.NoError(t, forkManager.RegisterHandler(ForkD, HandlerA, func() string { return "D" }))
+	assert.NoError(t, forkManager.RegisterHandler(ForkE, HandlerA, func() string { return "E" }))
+
+	assert.NoError(t, forkManager.ActivateFork(ForkE, 10))
+	assert.NoError(t, forkManager.ActivateFork(ForkC, 2))
+	assert.NoError(t, forkManager.ActivateFork(ForkA, 2))
+	assert.NoError(t, forkManager.ActivateFork(ForkB, 2))
+	assert.NoError(t, forkManager.ActivateFork(ForkD, 2))
+
+	assert.Equal(t, "D", execute(HandlerA, 2))
+	assert.NoError(t, forkManager.DeactivateFork(ForkD))
+	assert.Equal(t, "C", execute(HandlerA, 2))
+	assert.NoError(t, forkManager.DeactivateFork(ForkC))
+	assert.Equal(t, "B", execute(HandlerA, 2))
+	assert.NoError(t, forkManager.DeactivateFork(ForkB))
+	assert.Equal(t, "A", execute(HandlerA, 3))
+	assert.NoError(t, forkManager.DeactivateFork(ForkA))
+	assert.Nil(t, forkManager.GetHandler(HandlerA, 0))
+	assert.Equal(t, "E", execute(HandlerA, 11))
+
+	assert.NoError(t, forkManager.ActivateFork(ForkA, 0))
+	assert.NoError(t, forkManager.ActivateFork(ForkB, 0))
+	assert.NoError(t, forkManager.ActivateFork(ForkC, 0))
+
+	assert.Equal(t, "C", execute(HandlerA, 2))
+	assert.NoError(t, forkManager.DeactivateFork(ForkC))
+	assert.Equal(t, "B", execute(HandlerA, 1))
+	assert.NoError(t, forkManager.DeactivateFork(ForkB))
+	assert.Equal(t, "A", execute(HandlerA, 0))
+	assert.NoError(t, forkManager.DeactivateFork(ForkA))
+
+	assert.NoError(t, forkManager.ActivateFork(ForkB, 0))
+	assert.NoError(t, forkManager.ActivateFork(ForkC, 0))
+	assert.Equal(t, "C", execute(HandlerA, 0))
+	assert.NoError(t, forkManager.DeactivateFork(ForkC))
+	assert.Equal(t, "B", execute(HandlerA, 0))
+	assert.NoError(t, forkManager.DeactivateFork(ForkB))
+}