Add unit tests for rule validation

opera/marshal_test.go

@@ -1,6 +1,7 @@
 package opera
 
 import (
+	"fmt"
 	"math/big"
 	"testing"
 
@@ -38,6 +39,35 @@ func TestUpdateRules(t *testing.T) {
 	require.Error(err, "should fail on invalid rules")
 }
 
+func TestUpdateRules_ValidityCheckIsConductedIfCheckIsEnabledInUpdatedRuleSet(t *testing.T) {
+	for _, enabledBefore := range []bool{true, false} {
+		for _, enabledAfter := range []bool{true, false} {
+			for _, validUpdate := range []bool{true, false} {
+				t.Run(fmt.Sprintf("before=%t,after=%t,valid=%t", enabledBefore, enabledAfter, validUpdate), func(t *testing.T) {
+					require := require.New(t)
+
+					base := MainNetRules()
+					base.Upgrades.CheckRuleChanges = enabledBefore
+
+					maxParents := 1
+					if validUpdate {
+						maxParents = 5
+					}
+
+					update := fmt.Sprintf(`{"Dag":{"MaxParents":%d}, "Upgrades":{"CheckRuleChanges":%t}}`, maxParents, enabledAfter)
+
+					_, err := UpdateRules(base, []byte(update))
+					if enabledAfter && !validUpdate {
+						require.Error(err)
+					} else {
+						require.NoError(err)
+					}
+				})
+			}
+		}
+	}
+}
+
 func TestMainNetRulesRLP(t *testing.T) {
 	rules := MainNetRules()
 	require := require.New(t)

opera/rules.go

@@ -429,6 +429,15 @@ func (r Rules) Copy() Rules {
 	return cp
 }
 
+// Validate checks the rules for consistency and safety. Rules are considered safe if
+// they do not risk stalling the network or preventing future rule updates.
+//
+// Note: the validation is very liberal to allow a maximum flexibility in the rules.
+// The merely check for the most critical configuration errors that may lead to network
+// stalls or rule update issues. However, many valid configurations may still result
+// in undesirable network behavior. Rule-setters need to be aware of the implications
+// of their choices and should always test their rules in a controlled environment.
+// This validation is not a substitute for proper testing.
 func (r Rules) Validate() error {
 	return validate(r)
 }

opera/validate.go

@@ -13,7 +13,7 @@ func validate(rules Rules) error {
 		validateDagRules(rules.Dag),
 		validateEmitterRules(rules.Emitter),
 		validateEpochsRules(rules.Epochs),
-		validateBlockRules(rules.Blocks),
+		validateBlocksRules(rules.Blocks),
 		validateEconomyRules(rules.Economy),
 		validateUpgrades(rules.Upgrades),
 	)
@@ -72,14 +72,14 @@ func validateEpochsRules(rules EpochsRules) error {
 	return errors.Join(issues...)
 }
 
-func validateBlockRules(rules BlocksRules) error {
+func validateBlocksRules(rules BlocksRules) error {
 	var issues []error
 
 	if rules.MaxBlockGas < MinimumMaxBlockGas {
-		issues = append(issues, errors.New("MaxBlockGas is too low"))
+		issues = append(issues, errors.New("Blocks.MaxBlockGas is too low"))
 	}
 	if rules.MaxBlockGas > MaximumMaxBlockGas {
-		issues = append(issues, errors.New("MaxBlockGas is too high"))
+		issues = append(issues, errors.New("Blocks.MaxBlockGas is too high"))
 	}
 
 	// The empty-block skip period is not restricted. There are no too low or too high values.
@@ -140,7 +140,11 @@ func validateGasRules(rules GasRules) error {
 		issues = append(issues, errors.New("Gas.MaxEventGas is too low"))
 	}
 
-	if rules.MaxEventGas-rules.EventGas < upperBoundForRuleChangeGasCosts {
+	if rules.EventGas > rules.MaxEventGas {
+		issues = append(issues, errors.New("Gas.EventGas is too high"))
+	}
+
+	if rules.MaxEventGas < upperBoundForRuleChangeGasCosts+rules.EventGas {
 		issues = append(issues, errors.New("Gas.EventGas is too high"))
 	}