reviews

Author: ceyonur

miner/worker.go

@@ -149,7 +149,7 @@ func (w *worker) commitNewWork(predicateContext *precompileconfig.PredicateConte
 		chainExtra = params.GetExtra(w.chainConfig)
 	)
 
-	timestamp, timestampMS, err := customheader.GetNextTimestamp(parent, chainExtra, tstart)
+	timestamp, timestampMS, err := customheader.GetNextTimestamp(chainExtra, parent, tstart)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get next timestamp: %w", err)
 	}

plugin/evm/customheader/min_delay_excess.go

@@ -77,7 +77,7 @@ func VerifyMinDelayExcess(
 		}
 	default:
 		// Prior to Granite there was no expected min delay excess.
-		// TODO (ceyonur): this can be removed after Granite is activated.
+		// TODO Cleanup (ceyonur): this can be removed after Granite is activated.
 		if customtypes.GetHeaderExtra(header).MinDelayExcess != nil {
 			return fmt.Errorf("%w: %s", errRemoteMinDelayExcessSet, header.Hash())
 		}

plugin/evm/customheader/time.go

@@ -30,32 +30,28 @@ var (
 
 // GetNextTimestamp calculates the timestamp (in seconds and milliseconds) for the header based on the parent's timestamp and the current time.
 // First return value is the timestamp in seconds, second return value is the timestamp in milliseconds.
-func GetNextTimestamp(parent *types.Header, config *extras.ChainConfig, now time.Time) (uint64, uint64, error) {
+func GetNextTimestamp(config *extras.ChainConfig, parent *types.Header, now time.Time) (uint64, uint64, error) {
 	var (
 		timestamp   = uint64(now.Unix())
 		timestampMS = uint64(now.UnixMilli())
 	)
-	// Note: in order to support asynchronous block production, blocks are allowed to have
-	// the same timestamp as their parent. This allows more than one block to be produced
-	// per second.
-	parentExtra := customtypes.GetHeaderExtra(parent)
-	if parent.Time >= timestamp || (parentExtra.TimeMilliseconds != nil && *parentExtra.TimeMilliseconds >= timestampMS) {
-		// In pre-Granite, blocks are allowed to have the same timestamp as their parent.
-		// In Granite, there is a minimum delay enforced, and if the timestamp is the same as the parent,
-		// the block will be rejected.
-		// The block builder should have already waited enough time to meet the minimum delay.
-		// This is to early-exit from the block building.
-		if config.IsGranite(timestamp) {
+	if parent.Time < timestamp {
+		return timestamp, timestampMS, nil
+	}
+	// In Granite, there is a minimum delay enforced, and if the timestamp is the same as the parent,
+	// the block will be rejected.
+	// The block builder should have already waited enough time to meet the minimum delay.
+	// This is to early-exit from the block building.
+	if config.IsGranite(timestamp) {
+		if uint64(customtypes.BlockTime(parent).UnixMilli()) >= timestampMS {
 			return 0, 0, ErrGraniteClockBehindParent
 		}
-		// Add the delay to the parent timestamp with seconds precision.
-		timestamp = parent.Time
-		// Actually we don't need to set timestampMS, because it will be not be set if this is not
-		// Granite, but setting here for consistency.
-		timestampMS = parent.Time * 1000
+		return timestamp, timestampMS, nil
 	}
-
-	return timestamp, timestampMS, nil
+	// In pre-Granite, blocks are allowed to have the same timestamp as their parent
+	// Actually we don't need to return modified timestampMS, because it will be not be set if this is not
+	// Granite, but setting here for consistency.
+	return parent.Time, parent.Time * 1000, nil
 }
 
 // VerifyTime verifies that the header's Time and TimeMilliseconds fields are

plugin/evm/customheader/time_test.go

@@ -359,14 +359,23 @@ func TestGetNextTimestamp(t *testing.T) {
 			now:         now,
 			expectedErr: ErrGraniteClockBehindParent,
 		},
+		{
+			name:           "current_timesec_equals_parent_time_with_different_milliseconds_granite",
+			parent:         generateHeader(nowSeconds, utils.NewUint64(nowMillis-1000)),
+			extraConfig:    extras.TestGraniteChainConfig,
+			now:            now,
+			expectedErr:    nil,
+			expectedSec:    nowSeconds,
+			expectedMillis: nowMillis,
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			if test.extraConfig == nil {
 				test.extraConfig = extras.TestChainConfig
 			}
-			sec, millis, err := GetNextTimestamp(test.parent, test.extraConfig, test.now)
+			sec, millis, err := GetNextTimestamp(test.extraConfig, test.parent, test.now)
 			require.ErrorIs(t, err, test.expectedErr)
 			require.Equal(t, test.expectedSec, sec)
 			require.Equal(t, test.expectedMillis, millis)