op-deployer: Safety and validation improvements

op-deployer/pkg/deployer/artifacts/downloader.go

@@ -3,8 +3,10 @@ package artifacts
 import (
 	"archive/tar"
 	"bufio"
+	"bytes"
 	"compress/gzip"
 	"context"
+	"crypto/sha256"
 	"errors"
 	"fmt"
 	"io"
@@ -15,6 +17,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/ethereum/go-ethereum/common"
+
 	"github.com/ethereum-optimism/optimism/op-deployer/pkg/deployer/standard"
 
 	"github.com/ethereum/go-ethereum/log"
@@ -41,15 +45,50 @@ func LogProgressor(lgr log.Logger) DownloadProgressor {
 func Download(ctx context.Context, loc *Locator, progress DownloadProgressor) (foundry.StatDirFs, CleanupFunc, error) {
 	var u *url.URL
 	var err error
+	var checker integrityChecker
 	if loc.IsTag() {
 		u, err = standard.ArtifactsURLForTag(loc.Tag)
 		if err != nil {
 			return nil, nil, fmt.Errorf("failed to get standard artifacts URL for tag %s: %w", loc.Tag, err)
 		}
+
+		hash, err := standard.ArtifactsHashForTag(loc.Tag)
+		if err != nil {
+			return nil, nil, fmt.Errorf("failed to get standard artifacts hash for tag %s: %w", loc.Tag, err)
+		}
+
+		checker = &hashIntegrityChecker{hash: hash}
 	} else {
 		u = loc.URL
+		checker = &noopIntegrityChecker{}
 	}
 
+	return downloadURL(ctx, u, progress, checker)
+}
+
+type integrityChecker interface {
+	CheckIntegrity(data []byte) error
+}
+
+type hashIntegrityChecker struct {
+	hash common.Hash
+}
+
+func (h *hashIntegrityChecker) CheckIntegrity(data []byte) error {
+	hash := sha256.Sum256(data)
+	if hash != h.hash {
+		return fmt.Errorf("integrity check failed - expected: %x, got: %x", h.hash, hash)
+	}
+	return nil
+}
+
+type noopIntegrityChecker struct{}
+
+func (noopIntegrityChecker) CheckIntegrity(data []byte) error {
+	return nil
+}
+
+func downloadURL(ctx context.Context, u *url.URL, progress DownloadProgressor, checker integrityChecker) (foundry.StatDirFs, CleanupFunc, error) {
 	switch u.Scheme {
 	case "http", "https":
 		req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil)
@@ -78,7 +117,16 @@ func Download(ctx context.Context, loc *Locator, progress DownloadProgressor) (f
 			total:    resp.ContentLength,
 		}
 
-		gr, err := gzip.NewReader(pr)
+		data, err := io.ReadAll(pr)
+		if err != nil {
+			return nil, nil, fmt.Errorf("failed to read response body: %w", err)
+		}
+
+		if err := checker.CheckIntegrity(data); err != nil {
+			return nil, nil, fmt.Errorf("failed to check integrity: %w", err)
+		}
+
+		gr, err := gzip.NewReader(bytes.NewReader(data))
 		if err != nil {
 			return nil, nil, fmt.Errorf("failed to create gzip reader: %w", err)
 		}
@@ -111,7 +159,6 @@ type progressReader struct {
 }
 
 func (pr *progressReader) Read(p []byte) (int, error) {
-
 	n, err := pr.r.Read(p)
 	pr.curr += int64(n)
 	if pr.progress != nil && time.Since(pr.lastPrint) > 1*time.Second {

op-deployer/pkg/deployer/artifacts/downloader_test.go

@@ -9,10 +9,12 @@ import (
 	"os"
 	"testing"
 
+	"github.com/ethereum/go-ethereum/common"
+
 	"github.com/stretchr/testify/require"
 )
 
-func TestDownloadArtifacts(t *testing.T) {
+func TestDownloadArtifacts_MockArtifacts(t *testing.T) {
 	f, err := os.OpenFile("testdata/artifacts.tar.gz", os.O_RDONLY, 0o644)
 	require.NoError(t, err)
 	defer f.Close()
@@ -21,6 +23,9 @@ func TestDownloadArtifacts(t *testing.T) {
 		w.WriteHeader(http.StatusOK)
 		_, err := io.Copy(w, f)
 		require.NoError(t, err)
+		// Seek to beginning of file for next request
+		_, err = f.Seek(0, 0)
+		require.NoError(t, err)
 	}))
 	defer ts.Close()
 
@@ -31,14 +36,50 @@ func TestDownloadArtifacts(t *testing.T) {
 		URL: artifactsURL,
 	}
 
-	fs, cleanup, err := Download(ctx, loc, nil)
-	require.NoError(t, err)
-	require.NotNil(t, fs)
-	defer func() {
-		require.NoError(t, cleanup())
-	}()
+	t.Run("success", func(t *testing.T) {
+		fs, cleanup, err := Download(ctx, loc, nil)
+		require.NoError(t, err)
+		require.NotNil(t, fs)
+		defer func() {
+			require.NoError(t, cleanup())
+		}()
 
-	info, err := fs.Stat("WETH98.sol/WETH98.json")
-	require.NoError(t, err)
-	require.Greater(t, info.Size(), int64(0))
+		info, err := fs.Stat("WETH98.sol/WETH98.json")
+		require.NoError(t, err)
+		require.Greater(t, info.Size(), int64(0))
+	})
+
+	t.Run("bad integrity", func(t *testing.T) {
+		_, _, err := downloadURL(ctx, loc.URL, nil, &hashIntegrityChecker{
+			hash: common.Hash{'B', 'A', 'D'},
+		})
+		require.Error(t, err)
+		require.ErrorContains(t, err, "integrity check failed")
+	})
+
+	t.Run("ok integrity", func(t *testing.T) {
+		_, _, err := downloadURL(ctx, loc.URL, nil, &hashIntegrityChecker{
+			hash: common.HexToHash("0x0f814df0c4293aaaadd468ac37e6c92f0b40fd21df848076835cb2c21d2a516f"),
+		})
+		require.NoError(t, err)
+	})
+}
+
+func TestDownloadArtifacts_TaggedVersions(t *testing.T) {
+	tags := []string{
+		"op-contracts/v1.6.0",
+		"op-contracts/v1.7.0-beta.1+l2-contracts",
+	}
+	for _, tag := range tags {
+		t.Run(tag, func(t *testing.T) {
+			t.Parallel()
+
+			loc := MustNewLocatorFromTag(tag)
+			_, cleanup, err := Download(context.Background(), loc, nil)
+			t.Cleanup(func() {
+				require.NoError(t, cleanup())
+			})
+			require.NoError(t, err)
+		})
+	}
 }

op-deployer/pkg/deployer/broadcaster/gas_estimator.go

@@ -11,15 +11,20 @@ import (
 var (
 	// baseFeePadFactor = 50% as a divisor
 	baseFeePadFactor = big.NewInt(2)
-	// tipMulFactor = 20 as a multiplier
-	tipMulFactor = big.NewInt(20)
+	// tipMulFactor = 5 as a multiplier
+	tipMulFactor = big.NewInt(5)
 	// dummyBlobFee is a dummy value for the blob fee. Since this gas estimator will never
 	// post blobs, it's just set to 1.
 	dummyBlobFee = big.NewInt(1)
+	// maxTip is the maximum tip that can be suggested by this estimator.
+	maxTip = big.NewInt(50 * 1e9)
+	// minTip is the minimum tip that can be suggested by this estimator.
+	minTip = big.NewInt(1 * 1e9)
 )
 
 // DeployerGasPriceEstimator is a custom gas price estimator for use with op-deployer.
-// It pads the base fee by 50% and multiplies the suggested tip by 20.
+// It pads the base fee by 50% and multiplies the suggested tip by 5 up to a max of
+// 50 gwei.
 func DeployerGasPriceEstimator(ctx context.Context, client txmgr.ETHBackend) (*big.Int, *big.Int, *big.Int, error) {
 	chainHead, err := client.HeaderByNumber(ctx, nil)
 	if err != nil {
@@ -34,5 +39,14 @@ func DeployerGasPriceEstimator(ctx context.Context, client txmgr.ETHBackend) (*b
 	baseFeePad := new(big.Int).Div(chainHead.BaseFee, baseFeePadFactor)
 	paddedBaseFee := new(big.Int).Add(chainHead.BaseFee, baseFeePad)
 	paddedTip := new(big.Int).Mul(tip, tipMulFactor)
+
+	if paddedTip.Cmp(minTip) < 0 {
+		paddedTip.Set(minTip)
+	}
+
+	if paddedTip.Cmp(maxTip) > 0 {
+		paddedTip.Set(maxTip)
+	}
+
 	return paddedTip, paddedBaseFee, dummyBlobFee, nil
 }

op-deployer/pkg/deployer/integration_test/apply_test.go

@@ -100,21 +100,38 @@ func TestEndToEndApply(t *testing.T) {
 	l1Client, err := ethclient.Dial(rpcURL)
 	require.NoError(t, err)
 
-	depKey := new(deployerKey)
+	pk, err := crypto.HexToECDSA("ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80")
+	require.NoError(t, err)
+
 	l1ChainID := new(big.Int).SetUint64(defaultL1ChainID)
 	dk, err := devkeys.NewMnemonicDevKeys(devkeys.TestMnemonic)
 	require.NoError(t, err)
-	pk, err := dk.Secret(depKey)
-	require.NoError(t, err)
 
 	l2ChainID1 := uint256.NewInt(1)
 	l2ChainID2 := uint256.NewInt(2)
 
 	loc, _ := testutil.LocalArtifacts(t)
-	intent, st := newIntent(t, l1ChainID, dk, l2ChainID1, loc, loc)
-	cg := ethClientCodeGetter(ctx, l1Client)
 
-	t.Run("initial chain", func(t *testing.T) {
+	t.Run("two chains one after another", func(t *testing.T) {
+		intent, st := newIntent(t, l1ChainID, dk, l2ChainID1, loc, loc)
+		cg := ethClientCodeGetter(ctx, l1Client)
+
+		require.NoError(t, deployer.ApplyPipeline(
+			ctx,
+			deployer.ApplyPipelineOpts{
+				L1RPCUrl:           rpcURL,
+				DeployerPrivateKey: pk,
+				Intent:             intent,
+				State:              st,
+				Logger:             lgr,
+				StateWriter:        pipeline.NoopStateWriter(),
+			},
+		))
+
+		// create a new environment with wiped state to ensure we can continue using the
+		// state from the previous deployment
+		intent.Chains = append(intent.Chains, newChainIntent(t, dk, l1ChainID, l2ChainID2))
+
 		require.NoError(t, deployer.ApplyPipeline(
 			ctx,
 			deployer.ApplyPipelineOpts{
@@ -131,10 +148,12 @@ func TestEndToEndApply(t *testing.T) {
 		validateOPChainDeployment(t, cg, st, intent)
 	})
 
-	t.Run("subsequent chain", func(t *testing.T) {
-		// create a new environment with wiped state to ensure we can continue using the
-		// state from the previous deployment
-		intent.Chains = append(intent.Chains, newChainIntent(t, dk, l1ChainID, l2ChainID2))
+	t.Run("chain with tagged artifacts", func(t *testing.T) {
+		intent, st := newIntent(t, l1ChainID, dk, l2ChainID1, loc, loc)
+		cg := ethClientCodeGetter(ctx, l1Client)
+
+		intent.L1ContractsLocator = artifacts.DefaultL1ContractsLocator
+		intent.L2ContractsLocator = artifacts.DefaultL2ContractsLocator
 
 		require.NoError(t, deployer.ApplyPipeline(
 			ctx,
@@ -148,6 +167,7 @@ func TestEndToEndApply(t *testing.T) {
 			},
 		))
 
+		validateSuperchainDeployment(t, st, cg)
 		validateOPChainDeployment(t, cg, st, intent)
 	})
 }
@@ -245,9 +265,26 @@ func testApplyExistingOPCM(t *testing.T, l1ChainID uint64, forkRPCUrl string, ve
 		{"DelayedWETH", releases.DelayedWETH.ImplementationAddress, st.ImplementationsDeployment.DelayedWETHImplAddress},
 	}
 	for _, tt := range implTests {
-		t.Run(tt.name, func(t *testing.T) {
-			require.Equal(t, tt.expAddr, tt.actAddr)
-		})
+		require.Equal(t, tt.expAddr, tt.actAddr, "unexpected address for %s", tt.name)
+	}
+
+	superchain, err := standard.SuperchainFor(l1ChainIDBig.Uint64())
+	require.NoError(t, err)
+
+	managerOwner, err := standard.ManagerOwnerAddrFor(l1ChainIDBig.Uint64())
+	require.NoError(t, err)
+
+	superchainTests := []struct {
+		name    string
+		expAddr common.Address
+		actAddr common.Address
+	}{
+		{"ProxyAdmin", managerOwner, st.SuperchainDeployment.ProxyAdminAddress},
+		{"SuperchainConfig", common.Address(*superchain.Config.SuperchainConfigAddr), st.SuperchainDeployment.SuperchainConfigProxyAddress},
+		{"ProtocolVersions", common.Address(*superchain.Config.ProtocolVersionsAddr), st.SuperchainDeployment.ProtocolVersionsProxyAddress},
+	}
+	for _, tt := range superchainTests {
+		require.Equal(t, tt.expAddr, tt.actAddr, "unexpected address for %s", tt.name)
 	}
 
 	artifactsFSL2, cleanupL2, err := artifacts.Download(

op-deployer/pkg/deployer/pipeline/implementations.go

@@ -35,10 +35,12 @@ func DeployImplementations(env *Env, intent *state.Intent, st *state.State) erro
 	var err error
 	if intent.L1ContractsLocator.IsTag() && intent.DeploymentStrategy == state.DeploymentStrategyLive {
 		standardVersionsTOML, err = standard.L1VersionsDataFor(intent.L1ChainID)
-		if err != nil {
-			return fmt.Errorf("error getting standard versions TOML: %w", err)
+		if err == nil {
+			contractsRelease = intent.L1ContractsLocator.Tag
+		} else {
+			contractsRelease = "dev"
 		}
-		contractsRelease = intent.L1ContractsLocator.Tag
+
 	} else {
 		contractsRelease = "dev"
 	}