fix(evm): ten bugs from a self-review pass over the network driver

x/token/services/network/evm/config.go

@@ -27,7 +27,16 @@ const (
 	// DefaultPollInterval is how often finality polls a transaction's status.
 	DefaultPollInterval = 2 * time.Second
 	// DefaultFinalityTimeout bounds how long a transaction is awaited before it is treated as failed.
-	DefaultFinalityTimeout = 5 * time.Minute
+	// It carries real margin over MinFinalizedTagTimeout (design §7.2, §7.5) so a deployment running
+	// on defaults alone is not sitting at the edge of normal PoS finalization variance.
+	DefaultFinalityTimeout = 20 * time.Minute
+	// MinFinalizedTagTimeout is the floor Validate enforces on Finality.Timeout when BlockTag is
+	// finalized. Real time-to-finality on a PoS chain is ~13 minutes (design §7.2); a shorter timeout
+	// cannot ever see a transaction finalize and condemns it regardless of whether it succeeded (design
+	// §7.5: "any deployment must configure finality.timeout above ... the chain's finality"). It bounds
+	// only the chain's own lag; a deployment that also delays broadcasting a signed transaction needs
+	// additional headroom on top of this, which Validate has no way to know and cannot enforce.
+	MinFinalizedTagTimeout = 13 * time.Minute
 	// DefaultGasMultiplier scales the node's gas estimate to absorb small state changes between
 	// estimation and execution.
 	DefaultGasMultiplier = 1.2
@@ -71,7 +80,8 @@ type ContractsConfig struct {
 
 // FinalityConfig controls how transaction finality is observed.
 type FinalityConfig struct {
-	// BlockTag is the tag state is read at (finalized or safe).
+	// BlockTag is the tag state is read at: finalized (default, no reorg risk), safe, or latest (no
+	// reorg protection at all; only appropriate for a local, instant-mining chain).
 	BlockTag string `yaml:"blockTag"`
 	// PollInterval is the delay between status polls.
 	PollInterval time.Duration `yaml:"pollInterval"`
@@ -122,8 +132,10 @@ type EndorsementConfig struct {
 	// Threshold is the number of distinct endorser signatures a transaction needs. It must match the
 	// threshold the EndorsementVerifier was constructed with.
 	Threshold uint `yaml:"threshold"`
-	// Allowlist is the FSC identities permitted to request endorsement. Empty means the policy is
-	// resolved from the TMS network's nodes at wiring time.
+	// Allowlist is the FSC identities permitted to request endorsement. It is fail-closed: when this
+	// node is configured to endorse, an empty allowlist is a validation error rather than a default
+	// that resolves to anyone. There is no automatic "the TMS network's nodes" fallback; every
+	// permitted requester has to be named.
 	Allowlist []string `yaml:"allowlist"`
 	// Endorsers binds each endorser's Ethereum address to its FSC identity.
 	Endorsers []EndorserBinding `yaml:"endorsers"`
@@ -199,6 +211,13 @@ func (c *Config) Validate() error {
 	default:
 		return errors.Errorf("evm config: unsupported finality blockTag [%s]", c.Finality.BlockTag)
 	}
+	if c.Finality.BlockTag == client.BlockTagFinalized && c.Finality.Timeout < MinFinalizedTagTimeout {
+		return errors.Errorf(
+			"evm config: finality.timeout [%s] is shorter than the finalized tag's own time-to-finality "+
+				"(~%s); it would condemn every transaction regardless of whether it succeeded",
+			c.Finality.Timeout, MinFinalizedTagTimeout,
+		)
+	}
 	if err := c.validateGas(); err != nil {
 		return err
 	}
@@ -262,6 +281,13 @@ func (c *Config) validateEndorsement() error {
 		if _, err := client.HexToAddress(c.Endorser.Address); err != nil {
 			return errors.Wrap(err, "evm config: invalid endorser address")
 		}
+		// The authorizer is deliberately fail-closed: it refuses to build from an empty allowlist
+		// rather than default to trusting everyone. Catching that here means a node left this way
+		// fails at startup rather than coming up looking healthy and silently never registering as an
+		// endorser, which is where this used to surface, as an error log easy to miss during wiring.
+		if len(c.Endorsement.Allowlist) == 0 {
+			return errors.New("evm config: endorsement.allowlist is required when endorser.enabled is set")
+		}
 	}
 
 	return nil

x/token/services/network/evm/config_test.go

@@ -15,6 +15,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/yaml.v3"
+
+	"github.com/LFDT-Panurus/panurus/x/token/services/network/evm/client"
 )
 
 // yamlConfiguration is a Configuration backed by a real YAML document, so the tests exercise the
@@ -92,7 +94,7 @@ services:
       finality:
         blockTag: finalized
         pollInterval: 2s
-        timeout: 5m
+        timeout: 15m
       gas:
         strategy: estimate
         multiplier: 1.5
@@ -123,7 +125,7 @@ func TestLoadConfigFullDocument(t *testing.T) {
 	assert.Equal(t, int64(31337), c.ChainIDBig().Int64())
 	assert.Equal(t, "finalized", c.Finality.BlockTag)
 	assert.Equal(t, 2*time.Second, c.Finality.PollInterval)
-	assert.Equal(t, 5*time.Minute, c.Finality.Timeout)
+	assert.Equal(t, 15*time.Minute, c.Finality.Timeout)
 	assert.InEpsilon(t, 1.5, c.Gas.Multiplier, 1e-9)
 	assert.True(t, c.Endorser.Enabled)
 	assert.Equal(t, uint(2), c.Endorsement.Threshold)
@@ -190,13 +192,17 @@ func TestConfigValidationRejectsBadDocuments(t *testing.T) {
 	}
 
 	cases := map[string]func(*Config){
-		"empty endpoint":          func(c *Config) { c.Endpoint = "" },
-		"zero chain id":           func(c *Config) { c.ChainID = 0 },
-		"negative chain id":       func(c *Config) { c.ChainID = -1 },
-		"missing token state":     func(c *Config) { c.Contracts.TokenState = "" },
-		"malformed token state":   func(c *Config) { c.Contracts.TokenState = "0xdeadbeef" },
-		"malformed verifier":      func(c *Config) { c.Contracts.EndorsementVerifier = "not-an-address" },
-		"unsupported block tag":   func(c *Config) { c.Finality.BlockTag = "pending" },
+		"empty endpoint":        func(c *Config) { c.Endpoint = "" },
+		"zero chain id":         func(c *Config) { c.ChainID = 0 },
+		"negative chain id":     func(c *Config) { c.ChainID = -1 },
+		"missing token state":   func(c *Config) { c.Contracts.TokenState = "" },
+		"malformed token state": func(c *Config) { c.Contracts.TokenState = "0xdeadbeef" },
+		"malformed verifier":    func(c *Config) { c.Contracts.EndorsementVerifier = "not-an-address" },
+		"unsupported block tag": func(c *Config) { c.Finality.BlockTag = "pending" },
+		"finalized timeout shorter than real finality": func(c *Config) {
+			c.Finality.BlockTag = client.BlockTagFinalized
+			c.Finality.Timeout = MinFinalizedTagTimeout - time.Second
+		},
 		"unknown gas strategy":    func(c *Config) { c.Gas.Strategy = "guess" },
 		"multiplier below one":    func(c *Config) { c.Gas.Multiplier = 0.5 },
 		"fixed gas without limit": func(c *Config) { c.Gas.Strategy = GasStrategyFixed; c.Gas.Limit = 0 },
@@ -212,6 +218,9 @@ func TestConfigValidationRejectsBadDocuments(t *testing.T) {
 		},
 		"enabled endorser without address": func(c *Config) { c.Endorser.Address = "" },
 		"enabled endorser bad address":     func(c *Config) { c.Endorser.Address = "nope" },
+		"enabled endorser without allowlist": func(c *Config) {
+			c.Endorsement.Allowlist = nil
+		},
 	}
 	for name, mutate := range cases {
 		t.Run(name, func(t *testing.T) {
@@ -231,6 +240,33 @@ func TestFixedGasStrategyIsValid(t *testing.T) {
 	require.NoError(t, c.Validate())
 }
 
+// TestFinalizedTagRequiresLongEnoughTimeout pins the fix for the bug where the shipped default paired
+// the finalized tag with a timeout shorter than real PoS finality: a deployment running on defaults
+// alone would time out and report every transaction Invalid, whether or not it actually succeeded.
+// The floor applies only to the finalized tag; safe and latest resolve on their own faster schedules,
+// so a short timeout there is a legitimate choice, not a misconfiguration Validate can detect.
+func TestFinalizedTagRequiresLongEnoughTimeout(t *testing.T) {
+	c, err := LoadConfig(newYAMLConfiguration(t, fullConfigYAML))
+	require.NoError(t, err)
+
+	c.Finality.BlockTag = client.BlockTagFinalized
+	c.Finality.Timeout = MinFinalizedTagTimeout - time.Second
+	require.Error(t, c.Validate())
+
+	c.Finality.Timeout = MinFinalizedTagTimeout
+	require.NoError(t, c.Validate(), "the floor itself must be accepted")
+
+	c.Finality.BlockTag = client.BlockTagLatest
+	c.Finality.Timeout = time.Second
+	require.NoError(t, c.Validate(), "the floor must not apply to a tag it was not measured for")
+}
+
+// TestDefaultFinalityTimeoutClearsItsOwnFloor checks the shipped default is internally consistent: it
+// must satisfy MinFinalizedTagTimeout, since DefaultBlockTag is finalized.
+func TestDefaultFinalityTimeoutClearsItsOwnFloor(t *testing.T) {
+	assert.GreaterOrEqual(t, DefaultFinalityTimeout, MinFinalizedTagTimeout)
+}
+
 // TestThresholdEqualToSetSizeIsValid checks the boundary: an N-of-N policy is legitimate.
 func TestThresholdEqualToSetSizeIsValid(t *testing.T) {
 	c, err := LoadConfig(newYAMLConfiguration(t, fullConfigYAML))

x/token/services/network/evm/driver.go

@@ -81,8 +81,11 @@ type Driver struct {
 	auditStores     auditdb.StoreServiceManager
 	metricsProvider metrics.Provider
 	recoveryTracer  trace.Tracer
-	// registerOnce guards the responder registration, which is per node rather than per network.
-	registerOnce sync.Once
+	// registerMu guards registeredFor: this node can register only one FSC-level endorsement responder
+	// for its whole lifetime (see registerEndorser), so a second, different network trying to claim it
+	// needs to be detected rather than silently discarded.
+	registerMu    sync.Mutex
+	registeredFor string
 	// watchers keeps one public-parameters watcher per network, so building a network twice does not
 	// leave two pollers on one contract.
 	watchersMu sync.Mutex
@@ -162,7 +165,7 @@ func (d *Driver) New(network, channel string) (driver.Network, error) {
 	if err != nil {
 		return nil, err
 	}
-	if err := d.installEndorsement(n, config, evmClient); err != nil {
+	if err := d.installEndorsement(n, config, evmClient, network, channel); err != nil {
 		return nil, err
 	}
 	d.watchPublicParams(network, channel, config, evmClient)
@@ -208,8 +211,8 @@ func (d *Driver) watchPublicParams(network, channel string, config *Config, evmC
 
 	watcher, err := pp.NewWatcher(
 		evmClient, tokenState, config.Finality.BlockTag, config.Finality.PollInterval,
-		func(ctx context.Context, raw []byte, version uint64) {
-			d.applyPublicParams(ctx, tmsIDs, raw, version)
+		func(ctx context.Context, raw []byte, version uint64) error {
+			return d.applyPublicParams(ctx, tmsIDs, raw, version)
 		},
 	)
 	if err != nil {
@@ -224,10 +227,17 @@ func (d *Driver) watchPublicParams(network, channel string, config *Config, evmC
 // applyPublicParams reloads every TMS on the network with the new parameters and persists them. A
 // failure for one TMS does not stop the others: they are independent, and a node serving stale
 // parameters for one is better than for all of them.
-func (d *Driver) applyPublicParams(ctx context.Context, tmsIDs []token2.TMSID, raw []byte, version uint64) {
+//
+// It returns the combined error of every TMS that failed, if any, so the watcher knows this version
+// was not fully applied and retries it rather than treating it as handled. Retrying is safe: Update is
+// a no-op when the parameters it is given already match the TMS's current ones, so a TMS that already
+// succeeded is not disturbed by a retry covering the whole batch.
+func (d *Driver) applyPublicParams(ctx context.Context, tmsIDs []token2.TMSID, raw []byte, version uint64) error {
+	var errs []error
 	for _, tmsID := range tmsIDs {
 		if err := d.tmsProvider.Update(tmsID, raw); err != nil {
 			logger.Warnf("failed to update tms [%s] to public parameters version %d: %v", tmsID, version, err)
+			errs = append(errs, errors.Wrapf(err, "tms [%s]", tmsID))
 
 			continue
 		}
@@ -237,19 +247,23 @@ func (d *Driver) applyPublicParams(ctx context.Context, tmsIDs []token2.TMSID, r
 		service, err := d.tokensManager.ServiceByTMSId(tmsID)
 		if err != nil {
 			logger.Warnf("failed to get the token store for [%s]: %v", tmsID, err)
+			errs = append(errs, errors.Wrapf(err, "tms [%s]", tmsID))
 
 			continue
 		}
 		if err := service.StorePublicParams(ctx, raw); err != nil {
 			logger.Warnf("failed to store public parameters for [%s]: %v", tmsID, err)
+			errs = append(errs, errors.Wrapf(err, "tms [%s]", tmsID))
 		}
 	}
+
+	return errors.Join(errs...)
 }
 
 // installEndorsement builds the endorsement seam for this network and hands it to the network. The
 // service itself is per TMS, because it needs that TMS's validator, so what is installed is a factory
 // that resolves one when the network is given a TMS to approve for.
-func (d *Driver) installEndorsement(n *Network, config *Config, evmClient client.EVMClient) error {
+func (d *Driver) installEndorsement(n *Network, config *Config, evmClient client.EVMClient, network, channel string) error {
 	if d.viewManager == nil {
 		logger.Debugf("no view manager available; this node cannot collect endorsements")
 
@@ -289,11 +303,16 @@ func (d *Driver) installEndorsement(n *Network, config *Config, evmClient client
 		// one is evicted and rebuilt whenever public parameters change.
 		return factory.ForTMS(tms.ID())
 	})
+	// SetupPublicParams goes through this instead: it may run before any management service exists for
+	// the TMS (first-time setup), so it only ever has the id, never the wrapper above requires.
+	n.SetEndorsementFactoryByID(func(tmsID token2.TMSID) (EndorsementService, error) {
+		return factory.ForTMS(tmsID)
+	})
 
 	// Registration happens now, not on the first approval. An endorser node answers requests without
 	// ever making one, so registering lazily on the approval path would mean it never registers at
 	// all and every request to it times out.
-	d.registerEndorser(factory, config)
+	d.registerEndorser(network+":"+channel, factory, config)
 
 	return nil
 }
@@ -322,44 +341,66 @@ func (d *Driver) newSubmitter(config *Config, evmClient client.EVMClient) (*Subm
 // registerEndorser registers this node's responder so it can answer requests. A node that does not
 // endorse has no key and registers nothing.
 //
+// The registration is per node, not per network, because FSC routes an incoming session to a
+// responder by the initiating view's Go type alone: it has no notion of "this responder, but only for
+// network X". So only one Responder can ever be registered for endorsement.Initiator across this
+// process's whole lifetime, and whichever network happens to build it first has its factory, EIP-712
+// domain and chain client baked into it permanently. A second, differently configured network trying
+// to endorse through the same registration would validate against the right TMS but sign and read
+// against the wrong chain, so it is refused loudly here instead of silently discarded: an operator who
+// configures two endorsing networks on one node needs to see why the second one never answers.
+//
 // The TMS is resolved when a request arrives rather than now: resolving one here would ask the token
 // layer for a service that is still being built through this very driver.
-func (d *Driver) registerEndorser(factory *endorsement.ServiceFactory, config *Config) {
-	d.registerOnce.Do(func() {
-		if d.viewRegistry == nil || !config.Endorser.Enabled {
-			return
-		}
-		signer, err := config.EndorserSigner()
-		if err != nil || signer == nil {
-			logger.Errorf("this node is configured as an endorser but its key is unusable: %v", err)
+func (d *Driver) registerEndorser(networkKey string, factory *endorsement.ServiceFactory, config *Config) {
+	if d.viewRegistry == nil || !config.Endorser.Enabled {
+		return
+	}
 
-			return
+	d.registerMu.Lock()
+	defer d.registerMu.Unlock()
+	if d.registeredFor != "" {
+		if d.registeredFor != networkKey {
+			logger.Errorf(
+				"[%s] is configured to endorse, but this node is already registered as the endorser for [%s]; "+
+					"one node can endorse for only one EVM network at a time, [%s] will not answer endorsement requests",
+				networkKey, d.registeredFor, networkKey)
 		}
-		allowed, err := config.AllowedRequesters(d.resolveIdentity)
-		if err != nil {
-			logger.Errorf("failed to resolve the endorsement allowlist: %v", err)
 
-			return
-		}
-		authorizer, err := endorsement.NewAuthorizer(allowed)
-		if err != nil {
-			logger.Errorf("failed to build the endorsement allowlist: %v", err)
+		return
+	}
 
-			return
-		}
-		responder, err := factory.NewResponder(authorizer, signer, d.resolveTMS)
-		if err != nil {
-			logger.Errorf("failed to build the endorsement responder: %v", err)
+	signer, err := config.EndorserSigner()
+	if err != nil || signer == nil {
+		logger.Errorf("this node is configured as an endorser but its key is unusable: %v", err)
 
-			return
-		}
-		if err := endorsement.RegisterEndorser(d.viewRegistry, responder); err != nil {
-			logger.Errorf("failed to register the endorsement responder: %v", err)
+		return
+	}
+	allowed, err := config.AllowedRequesters(d.resolveIdentity)
+	if err != nil {
+		logger.Errorf("failed to resolve the endorsement allowlist: %v", err)
 
-			return
-		}
-		logger.Infof("registered as an endorser with address %s", signer.Address())
-	})
+		return
+	}
+	authorizer, err := endorsement.NewAuthorizer(allowed)
+	if err != nil {
+		logger.Errorf("failed to build the endorsement allowlist: %v", err)
+
+		return
+	}
+	responder, err := factory.NewResponder(authorizer, signer, d.resolveTMS)
+	if err != nil {
+		logger.Errorf("failed to build the endorsement responder: %v", err)
+
+		return
+	}
+	if err := endorsement.RegisterEndorser(d.viewRegistry, responder); err != nil {
+		logger.Errorf("failed to register the endorsement responder: %v", err)
+
+		return
+	}
+	d.registeredFor = networkKey
+	logger.Infof("registered as the endorser for [%s] with address %s", networkKey, signer.Address())
 }
 
 // resolveIdentity turns a configured node name into the identity that node speaks with.