Merge 4abf2e5afd into 23800122b3

This PR is purely for improved readability; I was doing work involving
the file and think this may help others who are trying to understand
what's going on.

1. `snapshot.Tree.Rebuild()` now returns a function that blocks until
regeneration is complete, allowing `Tree.waitBuild()` to be removed
entirely as all it did was search for the `done` channel behind this new
function.
2. Its usage inside `New()` is also simplified by (a) only waiting if
`!AsyncBuild`; and (b) avoiding the double negative of `if !NoBuild`.

---------

Co-authored-by: Martin HS <martin@swende.se>

accounts/abi/topics.go

@@ -42,7 +42,7 @@ func MakeTopics(query ...[]interface{}) ([][]common.Hash, error) {
 			case common.Address:
 				copy(topic[common.HashLength-common.AddressLength:], rule[:])
 			case *big.Int:
-				copy(topic[:], math.U256Bytes(rule))
+				copy(topic[:], math.U256Bytes(new(big.Int).Set(rule)))
 			case bool:
 				if rule {
 					topic[common.HashLength-1] = 1

accounts/abi/topics_test.go

@@ -149,6 +149,23 @@ func TestMakeTopics(t *testing.T) {
 			}
 		})
 	}
+
+	t.Run("does not mutate big.Int", func(t *testing.T) {
+		t.Parallel()
+		want := [][]common.Hash{{common.HexToHash("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")}}
+
+		in := big.NewInt(-1)
+		got, err := MakeTopics([]interface{}{in})
+		if err != nil {
+			t.Fatalf("makeTopics() error = %v", err)
+		}
+		if !reflect.DeepEqual(got, want) {
+			t.Fatalf("makeTopics() = %v, want %v", got, want)
+		}
+		if orig := big.NewInt(-1); in.Cmp(orig) != 0 {
+			t.Fatalf("makeTopics() mutated an input parameter from %v to %v", orig, in)
+		}
+	})
 }
 
 type args struct {

cmd/geth/chaincmd.go

@@ -282,14 +282,13 @@ func importChain(ctx *cli.Context) error {
 	if ctx.Args().Len() < 1 {
 		utils.Fatalf("This command requires an argument.")
 	}
-	// Start metrics export if enabled
-	utils.SetupMetrics(ctx)
-	// Start system runtime metrics collection
-	go metrics.CollectProcessMetrics(3 * time.Second)
-
-	stack, _ := makeConfigNode(ctx)
+	stack, cfg := makeConfigNode(ctx)
 	defer stack.Close()
 
+	// Start metrics export if enabled
+	utils.SetupMetrics(&cfg.Metrics)
+	go metrics.CollectProcessMetrics(3 * time.Second)
+
 	chain, db := utils.MakeChain(ctx, stack, false)
 	defer db.Close()
 

cmd/geth/config.go

@@ -25,6 +25,7 @@ import (
 	"runtime"
 	"slices"
 	"strings"
+	"time"
 	"unicode"
 
 	"github.com/ethereum/go-ethereum/accounts"
@@ -150,7 +151,7 @@ func loadBaseConfig(ctx *cli.Context) gethConfig {
 	// Load config file.
 	if file := ctx.String(configFileFlag.Name); file != "" {
 		if err := loadConfig(file, &cfg); err != nil {
-			utils.Fatalf("%v", err)
+			utils.Fatalf("failed to load config: %v", err)
 		}
 	}
 
@@ -192,6 +193,10 @@ func makeFullNode(ctx *cli.Context) *node.Node {
 		cfg.Eth.OverrideVerkle = &v
 	}
 
+	// Start metrics export if enabled
+	utils.SetupMetrics(&cfg.Metrics)
+	go metrics.CollectProcessMetrics(3 * time.Second)
+
 	backend, eth := utils.RegisterEthService(stack, &cfg.Eth)
 
 	// Create gauge with geth system and build information

cmd/geth/main.go

@@ -34,7 +34,6 @@ import (
 	"github.com/ethereum/go-ethereum/internal/debug"
 	"github.com/ethereum/go-ethereum/internal/flags"
 	"github.com/ethereum/go-ethereum/log"
-	"github.com/ethereum/go-ethereum/metrics"
 	"github.com/ethereum/go-ethereum/node"
 	"go.uber.org/automaxprocs/maxprocs"
 
@@ -325,12 +324,6 @@ func prepare(ctx *cli.Context) {
 			ctx.Set(utils.CacheFlag.Name, strconv.Itoa(4096))
 		}
 	}
-
-	// Start metrics export if enabled
-	utils.SetupMetrics(ctx)
-
-	// Start system runtime metrics collection
-	go metrics.CollectProcessMetrics(3 * time.Second)
 }
 
 // geth is the main entry point into the system if no special subcommand is run.

cmd/utils/flags.go

@@ -1969,64 +1969,60 @@ func RegisterFullSyncTester(stack *node.Node, eth *eth.Ethereum, target common.H
 	log.Info("Registered full-sync tester", "hash", target)
 }
 
-func SetupMetrics(ctx *cli.Context) {
-	if metrics.Enabled {
-		log.Info("Enabling metrics collection")
+func SetupMetrics(cfg *metrics.Config) {
+	if !cfg.Enabled {
+		return
+	}
+	log.Info("Enabling metrics collection")
+	var (
+		enableExport   = cfg.EnableInfluxDB
+		enableExportV2 = cfg.EnableInfluxDBV2
+	)
+	if enableExport && enableExportV2 {
+		Fatalf("Flags %v can't be used at the same time", strings.Join([]string{MetricsEnableInfluxDBFlag.Name, MetricsEnableInfluxDBV2Flag.Name}, ", "))
+	}
+	if enableExport || enableExportV2 {
+		v1FlagIsSet := cfg.InfluxDBUsername != "" || cfg.InfluxDBPassword != ""
+		v2FlagIsSet := cfg.InfluxDBToken != "" || cfg.InfluxDBOrganization != "" || cfg.InfluxDBBucket != ""
 
-		var (
-			enableExport   = ctx.Bool(MetricsEnableInfluxDBFlag.Name)
-			enableExportV2 = ctx.Bool(MetricsEnableInfluxDBV2Flag.Name)
-		)
-
-		if enableExport || enableExportV2 {
-			CheckExclusive(ctx, MetricsEnableInfluxDBFlag, MetricsEnableInfluxDBV2Flag)
-
-			v1FlagIsSet := ctx.IsSet(MetricsInfluxDBUsernameFlag.Name) ||
-				ctx.IsSet(MetricsInfluxDBPasswordFlag.Name)
-
-			v2FlagIsSet := ctx.IsSet(MetricsInfluxDBTokenFlag.Name) ||
-				ctx.IsSet(MetricsInfluxDBOrganizationFlag.Name) ||
-				ctx.IsSet(MetricsInfluxDBBucketFlag.Name)
-
-			if enableExport && v2FlagIsSet {
-				Fatalf("Flags --influxdb.metrics.organization, --influxdb.metrics.token, --influxdb.metrics.bucket are only available for influxdb-v2")
-			} else if enableExportV2 && v1FlagIsSet {
-				Fatalf("Flags --influxdb.metrics.username, --influxdb.metrics.password are only available for influxdb-v1")
-			}
+		if enableExport && v2FlagIsSet {
+			Fatalf("Flags --influxdb.metrics.organization, --influxdb.metrics.token, --influxdb.metrics.bucket are only available for influxdb-v2")
+		} else if enableExportV2 && v1FlagIsSet {
+			Fatalf("Flags --influxdb.metrics.username, --influxdb.metrics.password are only available for influxdb-v1")
 		}
+	}
 
-		var (
-			endpoint = ctx.String(MetricsInfluxDBEndpointFlag.Name)
-			database = ctx.String(MetricsInfluxDBDatabaseFlag.Name)
-			username = ctx.String(MetricsInfluxDBUsernameFlag.Name)
-			password = ctx.String(MetricsInfluxDBPasswordFlag.Name)
+	var (
+		endpoint = cfg.InfluxDBEndpoint
+		database = cfg.InfluxDBDatabase
+		username = cfg.InfluxDBUsername
+		password = cfg.InfluxDBPassword
 
-			token        = ctx.String(MetricsInfluxDBTokenFlag.Name)
-			bucket       = ctx.String(MetricsInfluxDBBucketFlag.Name)
-			organization = ctx.String(MetricsInfluxDBOrganizationFlag.Name)
-		)
+		token        = cfg.InfluxDBToken
+		bucket       = cfg.InfluxDBBucket
+		organization = cfg.InfluxDBOrganization
+	)
 
-		if enableExport {
-			tagsMap := SplitTagsFlag(ctx.String(MetricsInfluxDBTagsFlag.Name))
+	if enableExport {
+		tagsMap := SplitTagsFlag(cfg.InfluxDBTags)
 
-			log.Info("Enabling metrics export to InfluxDB")
+		log.Info("Enabling metrics export to InfluxDB")
 
-			go influxdb.InfluxDBWithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, database, username, password, "geth.", tagsMap)
-		} else if enableExportV2 {
-			tagsMap := SplitTagsFlag(ctx.String(MetricsInfluxDBTagsFlag.Name))
+		go influxdb.InfluxDBWithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, database, username, password, "geth.", tagsMap)
+	} else if enableExportV2 {
+		tagsMap := SplitTagsFlag(cfg.InfluxDBTags)
 
-			log.Info("Enabling metrics export to InfluxDB (v2)")
+		log.Info("Enabling metrics export to InfluxDB (v2)")
 
-			go influxdb.InfluxDBV2WithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, token, bucket, organization, "geth.", tagsMap)
-		}
+		go influxdb.InfluxDBV2WithTags(metrics.DefaultRegistry, 10*time.Second, endpoint, token, bucket, organization, "geth.", tagsMap)
+	}
 
-		if ctx.IsSet(MetricsHTTPFlag.Name) {
-			address := net.JoinHostPort(ctx.String(MetricsHTTPFlag.Name), fmt.Sprintf("%d", ctx.Int(MetricsPortFlag.Name)))
-			log.Info("Enabling stand-alone metrics HTTP endpoint", "address", address)
-			exp.Setup(address)
-		} else if ctx.IsSet(MetricsPortFlag.Name) {
-			log.Warn(fmt.Sprintf("--%s specified without --%s, metrics server will not start.", MetricsPortFlag.Name, MetricsHTTPFlag.Name))
-		}
+	if cfg.HTTP != "" {
+		address := net.JoinHostPort(cfg.HTTP, fmt.Sprintf("%d", cfg.Port))
+		log.Info("Enabling stand-alone metrics HTTP endpoint", "address", address)
+		exp.Setup(address)
+	} else if cfg.HTTP == "" && cfg.Port != 0 {
+		log.Warn(fmt.Sprintf("--%s specified without --%s, metrics server will not start.", MetricsPortFlag.Name, MetricsHTTPFlag.Name))
 	}
 }
 

core/state/snapshot/snapshot.go

@@ -206,47 +206,24 @@ func New(config Config, diskdb ethdb.KeyValueStore, triedb *triedb.Database, roo
 		log.Warn("Snapshot maintenance disabled (syncing)")
 		return snap, nil
 	}
-	// Create the building waiter iff the background generation is allowed
-	if !config.NoBuild && !config.AsyncBuild {
-		defer snap.waitBuild()
-	}
 	if err != nil {
 		log.Warn("Failed to load snapshot", "err", err)
-		if !config.NoBuild {
-			snap.Rebuild(root)
-			return snap, nil
+		if config.NoBuild {
+			return nil, err
 		}
-		return nil, err // Bail out the error, don't rebuild automatically.
+		wait := snap.Rebuild(root)
+		if !config.AsyncBuild {
+			wait()
+		}
+		return snap, nil
 	}
 	// Existing snapshot loaded, seed all the layers
-	for head != nil {
+	for ; head != nil; head = head.Parent() {
 		snap.layers[head.Root()] = head
-		head = head.Parent()
 	}
 	return snap, nil
 }
 
-// waitBuild blocks until the snapshot finishes rebuilding. This method is meant
-// to be used by tests to ensure we're testing what we believe we are.
-func (t *Tree) waitBuild() {
-	// Find the rebuild termination channel
-	var done chan struct{}
-
-	t.lock.RLock()
-	for _, layer := range t.layers {
-		if layer, ok := layer.(*diskLayer); ok {
-			done = layer.genPending
-			break
-		}
-	}
-	t.lock.RUnlock()
-
-	// Wait until the snapshot is generated
-	if done != nil {
-		<-done
-	}
-}
-
 // Disable interrupts any pending snapshot generator, deletes all the snapshot
 // layers in memory and marks snapshots disabled globally. In order to resume
 // the snapshot functionality, the caller must invoke Rebuild.
@@ -688,8 +665,9 @@ func (t *Tree) Journal(root common.Hash) (common.Hash, error) {
 
 // Rebuild wipes all available snapshot data from the persistent database and
 // discard all caches and diff layers. Afterwards, it starts a new snapshot
-// generator with the given root hash.
-func (t *Tree) Rebuild(root common.Hash) {
+// generator with the given root hash. The returned function blocks until
+// regeneration is complete.
+func (t *Tree) Rebuild(root common.Hash) (wait func()) {
 	t.lock.Lock()
 	defer t.lock.Unlock()
 
@@ -721,9 +699,11 @@ func (t *Tree) Rebuild(root common.Hash) {
 	// Start generating a new snapshot from scratch on a background thread. The
 	// generator will run a wiper first if there's not one running right now.
 	log.Info("Rebuilding state snapshot")
+	disk := generateSnapshot(t.diskdb, t.triedb, t.config.CacheSize, root)
 	t.layers = map[common.Hash]snapshot{
-		root: generateSnapshot(t.diskdb, t.triedb, t.config.CacheSize, root),
+		root: disk,
 	}
+	return func() { <-disk.genPending }
 }
 
 // AccountIterator creates a new account iterator for the specified root hash and

eth/tracers/logger/logger.go

@@ -363,26 +363,35 @@ func (t *mdLogger) OnEnter(depth int, typ byte, from common.Address, to common.A
 	if depth != 0 {
 		return
 	}
-	create := vm.OpCode(typ) == vm.CREATE
-	if !create {
-		fmt.Fprintf(t.out, "From: `%v`\nTo: `%v`\nData: `%#x`\nGas: `%d`\nValue `%v` wei\n",
-			from.String(), to.String(),
-			input, gas, value)
+	if create := vm.OpCode(typ) == vm.CREATE; !create {
+		fmt.Fprintf(t.out, "Pre-execution info:\n"+
+			"  - from: `%v`\n"+
+			"  - to: `%v`\n"+
+			"  - data: `%#x`\n"+
+			"  - gas: `%d`\n"+
+			"  - value: `%v` wei\n",
+			from.String(), to.String(), input, gas, value)
 	} else {
-		fmt.Fprintf(t.out, "From: `%v`\nCreate at: `%v`\nData: `%#x`\nGas: `%d`\nValue `%v` wei\n",
-			from.String(), to.String(),
-			input, gas, value)
+		fmt.Fprintf(t.out, "Pre-execution info:\n"+
+			"  - from: `%v`\n"+
+			"  - create: `%v`\n"+
+			"  - data: `%#x`\n"+
+			"  - gas: `%d`\n"+
+			"  - value: `%v` wei\n",
+			from.String(), to.String(), input, gas, value)
 	}
-
 	fmt.Fprintf(t.out, `
-|  Pc   |      Op     | Cost |   Stack   |   RStack  |  Refund |
-|-------|-------------|------|-----------|-----------|---------|
+|  Pc   |      Op     | Cost |   Refund  |   Stack   |
+|-------|-------------|------|-----------|-----------|
 `)
 }
 
 func (t *mdLogger) OnExit(depth int, output []byte, gasUsed uint64, err error, reverted bool) {
 	if depth == 0 {
-		fmt.Fprintf(t.out, "\nOutput: `%#x`\nConsumed gas: `%d`\nError: `%v`\n",
+		fmt.Fprintf(t.out, "\nPost-execution info:\n"+
+			"  - output: `%#x`\n"+
+			"  - consumed gas: `%d`\n"+
+			"  - error: `%v`\n",
 			output, gasUsed, err)
 	}
 }
@@ -390,7 +399,8 @@ func (t *mdLogger) OnExit(depth int, output []byte, gasUsed uint64, err error, r
 // OnOpcode also tracks SLOAD/SSTORE ops to track storage change.
 func (t *mdLogger) OnOpcode(pc uint64, op byte, gas, cost uint64, scope tracing.OpContext, rData []byte, depth int, err error) {
 	stack := scope.StackData()
-	fmt.Fprintf(t.out, "| %4d  | %10v  |  %3d |", pc, vm.OpCode(op).String(), cost)
+	fmt.Fprintf(t.out, "| %4d  | %10v  |  %3d |%10v |", pc, vm.OpCode(op).String(),
+		cost, t.env.StateDB.GetRefund())
 
 	if !t.cfg.DisableStack {
 		// format stack
@@ -401,7 +411,6 @@ func (t *mdLogger) OnOpcode(pc uint64, op byte, gas, cost uint64, scope tracing.
 		b := fmt.Sprintf("[%v]", strings.Join(a, ","))
 		fmt.Fprintf(t.out, "%10v |", b)
 	}
-	fmt.Fprintf(t.out, "%10v |", t.env.StateDB.GetRefund())
 	fmt.Fprintln(t.out, "")
 	if err != nil {
 		fmt.Fprintf(t.out, "Error: %v\n", err)

eth/tracers/logger/logger_json.go

@@ -71,7 +71,7 @@ func NewJSONLogger(cfg *Config, writer io.Writer) *tracing.Hooks {
 	l.hooks = &tracing.Hooks{
 		OnTxStart:         l.OnTxStart,
 		OnSystemCallStart: l.onSystemCallStart,
-		OnExit:            l.OnEnd,
+		OnExit:            l.OnExit,
 		OnOpcode:          l.OnOpcode,
 		OnFault:           l.OnFault,
 	}
@@ -152,13 +152,6 @@ func (l *jsonLogger) OnEnter(depth int, typ byte, from common.Address, to common
 	l.encoder.Encode(frame)
 }
 
-func (l *jsonLogger) OnEnd(depth int, output []byte, gasUsed uint64, err error, reverted bool) {
-	if depth > 0 {
-		return
-	}
-	l.OnExit(depth, output, gasUsed, err, false)
-}
-
 func (l *jsonLogger) OnExit(depth int, output []byte, gasUsed uint64, err error, reverted bool) {
 	type endLog struct {
 		Output  string              `json:"output"`

internal/ethapi/simulate.go

@@ -21,7 +21,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"maps"
 	"math/big"
 	"time"
 
@@ -186,7 +185,7 @@ func (sim *simulator) processBlock(ctx context.Context, block *simBlock, header,
 			Tracer:    tracer.Hooks(),
 		}
 	)
-	var tracingStateDB = vm.StateDB(sim.state)
+	tracingStateDB := vm.StateDB(sim.state)
 	if hooks := tracer.Hooks(); hooks != nil {
 		tracingStateDB = state.NewHookedState(sim.state, hooks)
 	}
@@ -289,7 +288,7 @@ func (sim *simulator) activePrecompiles(base *types.Header) vm.PrecompiledContra
 		isMerge = (base.Difficulty.Sign() == 0)
 		rules   = sim.chainConfig.Rules(base.Number, isMerge, base.Time)
 	)
-	return maps.Clone(vm.ActivePrecompiledContracts(rules))
+	return vm.ActivePrecompiledContracts(rules)
 }
 
 // sanitizeChain checks the chain integrity. Specifically it checks that

metrics/metrics.go

@@ -6,15 +6,16 @@
 package metrics
 
 import (
+	"flag"
+	"io"
 	"os"
 	"runtime/metrics"
 	"runtime/pprof"
 	"strconv"
-	"strings"
 	"syscall"
 	"time"
 
-	"github.com/ethereum/go-ethereum/log"
+	"github.com/naoina/toml"
 )
 
 // Enabled is checked by the constructor functions for all of the
@@ -24,34 +25,63 @@ import (
 // for less cluttered pprof profiles.
 var Enabled = false
 
-// enablerFlags is the CLI flag names to use to enable metrics collections.
-var enablerFlags = []string{"metrics"}
-
-// enablerEnvVars is the env var names to use to enable metrics collections.
-var enablerEnvVars = []string{"GETH_METRICS"}
-
 // init enables or disables the metrics system. Since we need this to run before
 // any other code gets to create meters and timers, we'll actually do an ugly hack
 // and peek into the command line args for the metrics flag.
 func init() {
-	for _, enabler := range enablerEnvVars {
-		if val, found := syscall.Getenv(enabler); found && !Enabled {
-			if enable, _ := strconv.ParseBool(val); enable { // ignore error, flag parser will choke on it later
-				log.Info("Enabling metrics collection")
-				Enabled = true
-			}
+	if val, found := syscall.Getenv("GETH_METRICS"); found && !Enabled {
+		if enable, _ := strconv.ParseBool(val); enable { // ignore error, flag parser will choke on it later
+			Enabled = true
 		}
 	}
-	for _, arg := range os.Args {
-		flag := strings.TrimLeft(arg, "-")
 
-		for _, enabler := range enablerFlags {
-			if !Enabled && flag == enabler {
-				log.Info("Enabling metrics collection")
-				Enabled = true
-			}
+	fs := flag.NewFlagSet("", flag.ContinueOnError)
+	fs.SetOutput(io.Discard)
+	fs.Bool("metrics", false, "")
+	fs.String("config", "", "")
+
+	// The flag package will quit parsing immediately if it encounters a flag that
+	// was not declared to it ahead of time. We could be fancy and try to look
+	// through the args to find the ones we care about, but then we'd need to
+	// handle the various ways that flags can be defined -- which was the point of
+	// using the flags package in the first place! So instead, let's chop off the
+	// first element in the args list each time a parse fails. This way we will
+	// eventually parse every arg and get the ones we care about.
+	for i := range os.Args[1:] {
+		if err := fs.Parse(os.Args[i+1:]); err == nil {
+			break
 		}
 	}
+
+	// Now visit the flags we defined which are present in the args and see if we
+	// should enable metrics.
+	fs.Visit(func(f *flag.Flag) {
+		g, ok := f.Value.(flag.Getter)
+		if !ok {
+			// Don't expect this to fail, but skip over just in case it does.
+			return
+		}
+		switch f.Name {
+		case "metrics":
+			Enabled = g.Get().(bool)
+		case "config":
+			data, err := os.ReadFile(g.Get().(string))
+			if err != nil {
+				return
+			}
+			var cfg map[string]map[string]any
+			if err := toml.Unmarshal(data, &cfg); err != nil {
+				return
+			}
+			// A config file is definitely being used and was parsed correct. Let's
+			// try to peek inside and see if metrics are listed as enabled.
+			if m, ok := cfg["Metrics"]; ok {
+				if v, ok := m["Enabled"].(bool); ok && v {
+					Enabled = true
+				}
+			}
+		}
+	})
 }
 
 var threadCreateProfile = pprof.Lookup("threadcreate")

signer/core/apitypes/types.go

@@ -676,7 +676,7 @@ func (typedData *TypedData) EncodePrimitiveValue(encType string, encValue interf
 		if err != nil {
 			return nil, err
 		}
-		return math.U256Bytes(b), nil
+		return math.U256Bytes(new(big.Int).Set(b)), nil
 	}
 	return nil, fmt.Errorf("unrecognized type '%s'", encType)
 }