From 2fedcc06820aacd6a21a6e4c90fee0d625b89696 Mon Sep 17 00:00:00 2001 From: Alec Schaefer Date: Tue, 17 Dec 2024 10:01:04 -0500 Subject: [PATCH] [Audit - TOB-CELOL2-3] Add error checks to migration script (#283) * addresses audit comments * fix errors * addresses feedback * missed defer statements --- op-chain-ops/cmd/celo-migrate/ancients.go | 25 +++++++++++++------ op-chain-ops/cmd/celo-migrate/db.go | 11 +++++--- op-chain-ops/cmd/celo-migrate/main.go | 8 +++--- op-chain-ops/cmd/celo-migrate/non-ancients.go | 9 +++++-- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/op-chain-ops/cmd/celo-migrate/ancients.go b/op-chain-ops/cmd/celo-migrate/ancients.go index 183c81ed50de..06542af89c7e 100644 --- a/op-chain-ops/cmd/celo-migrate/ancients.go +++ b/op-chain-ops/cmd/celo-migrate/ancients.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "path/filepath" @@ -38,27 +39,31 @@ func NewChainFreezer(datadir string, namespace string, readonly bool) (*rawdb.Fr return rawdb.NewFreezer(datadir, namespace, readonly, freezerTableSize, chainFreezerNoSnappy) } -func migrateAncientsDb(ctx context.Context, oldDBPath, newDBPath string, batchSize, bufferSize uint64) (uint64, uint64, error) { +func migrateAncientsDb(ctx context.Context, oldDBPath, newDBPath string, batchSize, bufferSize uint64) (numAncientsNewBefore uint64, numAncientsNewAfter uint64, err error) { defer timer("ancients")() oldFreezer, err := NewChainFreezer(filepath.Join(oldDBPath, "ancient"), "", false) // Can't be readonly because we need the .meta files to be created if err != nil { return 0, 0, fmt.Errorf("failed to open old freezer: %w", err) } - defer oldFreezer.Close() + defer func() { + err = errors.Join(err, oldFreezer.Close()) + }() newFreezer, err := NewChainFreezer(filepath.Join(newDBPath, "ancient"), "", false) if err != nil { return 0, 0, fmt.Errorf("failed to open new freezer: %w", err) } - defer newFreezer.Close() + defer func() { + err = errors.Join(err, newFreezer.Close()) + }() numAncientsOld, err := oldFreezer.Ancients() if err != nil { return 0, 0, fmt.Errorf("failed to get number of ancients in old freezer: %w", err) } - numAncientsNewBefore, err := newFreezer.Ancients() + numAncientsNewBefore, err = newFreezer.Ancients() if err != nil { return 0, 0, fmt.Errorf("failed to get number of ancients in new freezer: %w", err) } @@ -84,11 +89,15 @@ func migrateAncientsDb(ctx context.Context, oldDBPath, newDBPath string, batchSi return 0, 0, fmt.Errorf("failed to migrate ancients: %w", err) } - numAncientsNewAfter, err := newFreezer.Ancients() + numAncientsNewAfter, err = newFreezer.Ancients() if err != nil { return 0, 0, fmt.Errorf("failed to get number of ancients in new freezer: %w", err) } + if numAncientsNewAfter != numAncientsOld { + return 0, 0, fmt.Errorf("failed to migrate all ancients from old to new db. Expected %d, got %d", numAncientsOld, numAncientsNewAfter) + } + log.Info("Ancient Block Migration Ended", "process", "ancients", "ancientsInOldDB", numAncientsOld, "ancientsInNewDB", numAncientsNewAfter, "migrated", numAncientsNewAfter-numAncientsNewBefore) return numAncientsNewBefore, numAncientsNewAfter, nil } @@ -214,14 +223,16 @@ func writeAncientBlocks(ctx context.Context, freezer *rawdb.Freezer, in <-chan R } // getStrayAncientBlocks returns a list of ancient block numbers / hashes that somehow were not removed from leveldb -func getStrayAncientBlocks(dbPath string) ([]*rawdb.NumberHash, error) { +func getStrayAncientBlocks(dbPath string) (blocks []*rawdb.NumberHash, err error) { defer timer("getStrayAncientBlocks")() db, err := openDB(dbPath, true) if err != nil { return nil, fmt.Errorf("failed to open database: %w", err) } - defer db.Close() + defer func() { + err = errors.Join(err, db.Close()) + }() numAncients, err := db.Ancients() if err != nil { diff --git a/op-chain-ops/cmd/celo-migrate/db.go b/op-chain-ops/cmd/celo-migrate/db.go index 180b9e541dd0..1449c417bf95 100644 --- a/op-chain-ops/cmd/celo-migrate/db.go +++ b/op-chain-ops/cmd/celo-migrate/db.go @@ -38,7 +38,8 @@ func headerKey(number uint64, hash common.Hash) []byte { // Opens a database with access to AncientsDb func openDB(chaindataPath string, readOnly bool) (ethdb.Database, error) { - if _, err := os.Stat(chaindataPath); errors.Is(err, os.ErrNotExist) { + // Will throw an error if the chaindataPath does not exist + if _, err := os.Stat(chaindataPath); err != nil { return nil, err } @@ -100,14 +101,16 @@ func removeBlocks(ldb ethdb.Database, numberHashes []*rawdb.NumberHash) error { return nil } -func getHeadHeader(dbpath string) (*types.Header, error) { +func getHeadHeader(dbpath string) (headHeader *types.Header, err error) { db, err := openDBWithoutFreezer(dbpath, true) if err != nil { return nil, fmt.Errorf("failed to open database at %q err: %w", dbpath, err) } - defer db.Close() + defer func() { + err = errors.Join(err, db.Close()) + }() - headHeader := rawdb.ReadHeadHeader(db) + headHeader = rawdb.ReadHeadHeader(db) if headHeader == nil { return nil, fmt.Errorf("head header not in database at: %s", dbpath) } diff --git a/op-chain-ops/cmd/celo-migrate/main.go b/op-chain-ops/cmd/celo-migrate/main.go index 632ec8dd1587..84f746fd22e0 100644 --- a/op-chain-ops/cmd/celo-migrate/main.go +++ b/op-chain-ops/cmd/celo-migrate/main.go @@ -84,7 +84,7 @@ var ( batchSizeFlag = &cli.Uint64Flag{ Name: "batch-size", Usage: "Batch size to use for block migration, larger batch sizes can speed up migration but require more memory. If increasing the batch size consider also increasing the memory-limit", - Value: 50000, // TODO(Alec) optimize default parameters + Value: 50000, } bufferSizeFlag = &cli.Uint64Flag{ Name: "buffer-size", @@ -315,14 +315,16 @@ func runPreMigration(opts preMigrationOptions) ([]*rawdb.NumberHash, uint64, err return strayAncientBlocks, numAncientsNewAfter, nil } -func runNonAncientMigration(newDBPath string, strayAncientBlocks []*rawdb.NumberHash, batchSize, numAncients uint64) error { +func runNonAncientMigration(newDBPath string, strayAncientBlocks []*rawdb.NumberHash, batchSize, numAncients uint64) (err error) { defer timer("non-ancient migration")() newDB, err := openDBWithoutFreezer(newDBPath, false) if err != nil { return fmt.Errorf("failed to open new database: %w", err) } - defer newDB.Close() + defer func() { + err = errors.Join(err, newDB.Close()) + }() // get the last block number hash := rawdb.ReadHeadHeaderHash(newDB) diff --git a/op-chain-ops/cmd/celo-migrate/non-ancients.go b/op-chain-ops/cmd/celo-migrate/non-ancients.go index 61704133b0d0..44843d2080cd 100644 --- a/op-chain-ops/cmd/celo-migrate/non-ancients.go +++ b/op-chain-ops/cmd/celo-migrate/non-ancients.go @@ -19,7 +19,10 @@ func copyDbExceptAncients(oldDbPath, newDbPath string) error { // Get rsync help output cmdHelp := exec.Command("rsync", "--help") - output, _ := cmdHelp.CombinedOutput() + output, err := cmdHelp.CombinedOutput() + if err != nil { + return fmt.Errorf("failed to get rsync help output: %w", err) + } // Convert output to string outputStr := string(output) @@ -113,7 +116,9 @@ func migrateNonAncientBlock(number uint64, hash common.Hash, newDB ethdb.Databas // write header and body batch := newDB.NewBatch() rawdb.WriteBodyRLP(batch, hash, number, newBody) - _ = batch.Put(headerKey(number, hash), newHeader) + if err := batch.Put(headerKey(number, hash), newHeader); err != nil { + return fmt.Errorf("failed to write header: block %d - %x: %w", number, hash, err) + } if err := batch.Write(); err != nil { return fmt.Errorf("failed to write header and body: block %d - %x: %w", number, hash, err) }