core/txpool: add validation and comprehensive tests for SetGasPrice (#1876)

This commit adds robust input validation to the SetGasPrice method and
implements comprehensive table-driven tests to ensure correct behavior.

Changes in core/txpool/txpool.go:
- Add nil gas price validation with graceful error handling
- Add validation to reject negative gas prices
- Add validation to reject gas prices exceeding 1000 GWei maximum
- Return detailed error messages for each validation failure
- Log warnings when invalid gas prices are rejected

Changes in eth/api_miner.go:
- Update MinerAPI.SetGasPrice to check error return from txPool.SetGasPrice
- Return false when validation fails (when SetGasPrice returns error)
- Return true when validation succeeds (when SetGasPrice returns nil)

New tests in core/txpool/txpool_test.go:
- Implement TestSetGasPrice using table-driven test pattern
- Test 4 invalid cases: nil, negative, exceed max+1, exceed 10000 GWei
- Test 7 valid cases: 0, 1 wei, 1 GWei, 100 GWei, 500 GWei, max-1, max
- Each test case includes expected error value for precise validation
- All 11 test cases verify both error returns and gas price state
- Tests use isolated pool instances to ensure independence

core/txpool/txpool.go

@@ -215,6 +215,8 @@ var DefaultConfig = Config{
 	Lifetime: 3 * time.Hour,
 }
 
+var defaultMaxPrice = big.NewInt(1000 * params.GWei)
+
 // sanitize checks the provided user configurations and changes anything that's
 // unreasonable or unworkable.
 func (config *Config) sanitize() Config {
@@ -466,11 +468,26 @@ func (pool *TxPool) GasPrice() *big.Int {
 }
 
 // SetGasPrice updates the minimum price required by the transaction pool for a
-// new transaction, and drops all transactions below this threshold.
-func (pool *TxPool) SetGasPrice(price *big.Int) {
+// new transaction, and drops all transactions below this threshold. Negative
+// gas prices and prices exceeding 1000 GWei are considered invalid and will be
+// rejected without updating the threshold.
+func (pool *TxPool) SetGasPrice(price *big.Int) error {
 	pool.mu.Lock()
 	defer pool.mu.Unlock()
 
+	if price == nil {
+		log.Warn("Reject nil gas price")
+		return errors.New("reject nil gas price")
+	}
+	if price.Sign() < 0 {
+		log.Warn("Reject invalid gas price", "price", price)
+		return fmt.Errorf("reject negative gas price: %v", price)
+	}
+	if price.Cmp(defaultMaxPrice) > 0 {
+		log.Warn("Reject invalid gas price", "price", price, "max", defaultMaxPrice)
+		return fmt.Errorf("reject too high gas price: %v, maximum: %v", price, defaultMaxPrice)
+	}
+
 	old := pool.gasPrice
 	pool.gasPrice = price
 	// if the min miner fee increased, remove transactions below the new threshold
@@ -484,6 +501,7 @@ func (pool *TxPool) SetGasPrice(price *big.Int) {
 	}
 
 	log.Info("Transaction pool price threshold updated", "price", price)
+	return nil
 }
 
 // Nonce returns the next nonce of an account, with all transactions executable

core/txpool/txpool_test.go

@@ -2693,3 +2693,114 @@ func BenchmarkMultiAccountBatchInsert(b *testing.B) {
 		pool.AddRemotesSync([]*types.Transaction{tx})
 	}
 }
+
+// TestSetGasPrice tests the SetGasPrice validation logic using table-driven tests
+func TestSetGasPrice(t *testing.T) {
+	testCases := []struct {
+		name        string
+		price       *big.Int
+		wantErr     error
+		description string
+	}{
+		// Invalid cases - should be rejected
+		{
+			name:        "nil gas price",
+			price:       nil,
+			wantErr:     errors.New("reject nil gas price"),
+			description: "nil pointer should be rejected gracefully",
+		},
+		{
+			name:        "negative gas price",
+			price:       big.NewInt(-1),
+			wantErr:     fmt.Errorf("reject negative gas price: %v", big.NewInt(-1)),
+			description: "negative value should be rejected",
+		},
+		{
+			name:        "exceeds maximum by 1",
+			price:       new(big.Int).Add(defaultMaxPrice, big.NewInt(1)),
+			wantErr:     fmt.Errorf("reject too high gas price: %v, maximum: %v", new(big.Int).Add(defaultMaxPrice, big.NewInt(1)), defaultMaxPrice),
+			description: "value exceeding 1000 GWei should be rejected",
+		},
+		{
+			name:        "exceeds maximum significantly",
+			price:       big.NewInt(10000 * params.GWei),
+			wantErr:     fmt.Errorf("reject too high gas price: %v, maximum: %v", big.NewInt(10000*params.GWei), defaultMaxPrice),
+			description: "value far exceeding maximum should be rejected",
+		},
+		// Valid cases - should be accepted
+		{
+			name:        "zero gas price",
+			price:       big.NewInt(0),
+			wantErr:     nil,
+			description: "zero is valid as it's not negative",
+		},
+		{
+			name:        "minimum positive value",
+			price:       big.NewInt(1),
+			wantErr:     nil,
+			description: "minimum positive value should be accepted",
+		},
+		{
+			name:        "1 GWei",
+			price:       big.NewInt(params.GWei),
+			wantErr:     nil,
+			description: "1 GWei should be accepted",
+		},
+		{
+			name:        "100 GWei",
+			price:       big.NewInt(100 * params.GWei),
+			wantErr:     nil,
+			description: "100 GWei should be accepted",
+		},
+		{
+			name:        "500 GWei",
+			price:       big.NewInt(500 * params.GWei),
+			wantErr:     nil,
+			description: "500 GWei should be accepted",
+		},
+		{
+			name:        "just below maximum",
+			price:       new(big.Int).Sub(defaultMaxPrice, big.NewInt(1)),
+			wantErr:     nil,
+			description: "value just below maximum should be accepted",
+		},
+		{
+			name:        "exactly at maximum",
+			price:       defaultMaxPrice,
+			wantErr:     nil,
+			description: "exactly 1000 GWei should be accepted",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Create a fresh pool instance for each test case to ensure isolation
+			pool, _ := setupPool()
+			defer pool.Stop()
+
+			oldPrice := pool.GasPrice()
+			haveErr := pool.SetGasPrice(tc.price)
+			newPrice := pool.GasPrice()
+
+			if tc.wantErr != nil {
+				// Invalid case: should return error and price should remain unchanged
+				if haveErr == nil {
+					t.Errorf("%s: Expected error %q, got nil", tc.description, tc.wantErr)
+				} else if haveErr.Error() != tc.wantErr.Error() {
+					t.Errorf("%s: Expected error %q, got %q", tc.description, tc.wantErr, haveErr)
+				}
+				if newPrice.Cmp(oldPrice) != 0 {
+					t.Errorf("%s: Gas price should not change. Expected %v, got %v", tc.description, oldPrice, newPrice)
+				}
+			} else {
+				// Valid case: should return nil error and price should be updated
+				if haveErr != nil {
+					t.Errorf("%s: Expected no error, got: %v", tc.description, haveErr)
+				}
+				if newPrice.Cmp(tc.price) != 0 {
+					t.Errorf("%s: Expected gas price to be set to %v, got %v", tc.description, tc.price, newPrice)
+				}
+			}
+		})
+	}
+}

eth/api_miner.go

@@ -97,8 +97,8 @@ func (api *MinerAPI) SetGasPrice(gasPrice hexutil.Big) bool {
 	api.e.gasPrice = (*big.Int)(&gasPrice)
 	api.e.lock.Unlock()
 
-	api.e.txPool.SetGasPrice((*big.Int)(&gasPrice))
-	return true
+	err := api.e.txPool.SetGasPrice((*big.Int)(&gasPrice))
+	return err == nil
 }
 
 // SetEtherbase sets the etherbase of the miner