From 36ae03d37da69f8b717fac1c16b5966d7b327226 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 00:15:02 +0530 Subject: [PATCH 01/35] fix(evm): bump the evm module to fabric-smart-client v0.17.0 Needed to build this branch locally; already open separately as #2228 and will drop out here once that merges and this branch rebases. Signed-off-by: atharrva01 --- x/token/services/network/evm/go.mod | 2 +- x/token/services/network/evm/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x/token/services/network/evm/go.mod b/x/token/services/network/evm/go.mod index 3ad025b39a..4c3b0c46d1 100644 --- a/x/token/services/network/evm/go.mod +++ b/x/token/services/network/evm/go.mod @@ -8,7 +8,7 @@ require ( github.com/IBM/mathlib v0.3.0 github.com/LFDT-Panurus/panurus v0.0.0-00010101000000-000000000000 github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.1 - github.com/hyperledger-labs/fabric-smart-client v0.16.0 + github.com/hyperledger-labs/fabric-smart-client v0.17.0 github.com/stretchr/testify v1.11.1 go.opentelemetry.io/otel/trace v1.44.0 golang.org/x/crypto v0.54.0 diff --git a/x/token/services/network/evm/go.sum b/x/token/services/network/evm/go.sum index 9b2951c17f..37742fef6c 100644 --- a/x/token/services/network/evm/go.sum +++ b/x/token/services/network/evm/go.sum @@ -86,8 +86,8 @@ github.com/hashicorp/go-uuid v1.0.3 h1:2gKiV6YVmrJ1i2CKKa9obLvRieoRGviZFL26PcT/C github.com/hashicorp/go-uuid v1.0.3/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k= github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= -github.com/hyperledger-labs/fabric-smart-client v0.16.0 h1:JZtM2pd174Wo3rOJJIEN7dgvjpsp+K2J2lqCifVulJc= -github.com/hyperledger-labs/fabric-smart-client v0.16.0/go.mod h1:Xfm18BI6WuGU3IG++j8AyiThC74ZBDjd+VI5zoH9edw= +github.com/hyperledger-labs/fabric-smart-client v0.17.0 h1:6BKrkd0PFuDM0GMjL5brVl+wF3pnxj+G390XNCbWjRo= +github.com/hyperledger-labs/fabric-smart-client v0.17.0/go.mod h1:Xfm18BI6WuGU3IG++j8AyiThC74ZBDjd+VI5zoH9edw= github.com/hyperledger/fabric-amcl v0.0.0-20230602173724-9e02669dceb2 h1:B1Nt8hKb//KvgGRprk0h1t4lCnwhE9/ryb1WqfZbV+M= github.com/hyperledger/fabric-amcl v0.0.0-20230602173724-9e02669dceb2/go.mod h1:X+DIyUsaTmalOpmpQfIvFZjKHQedrURQ5t4YqquX7lE= github.com/hyperledger/fabric-lib-go v1.1.5-0.20260708100132-163bcc919208 h1:qA49XOMwyNPxggVyW+HSDAg7I3GdtlGWDTbh40/rwZk= From 488eb50a7d8588be565d4a8f5b612bead0bb8bc6 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 00:15:06 +0530 Subject: [PATCH 02/35] fix(evm): stop a persistent read failure from resolving as Invalid The finality watcher polled StatusByAnchor until either the anchor appeared or the timeout expired, and on timeout it always reported Invalid. If the chain was unreachable for the whole window, every poll errored and got skipped, so the loop reached the timeout having never actually observed the ledger, and reported Invalid anyway. The shared ttx listener maps Invalid straight to a deleted transaction, so a connectivity outage on the reading side could make a transaction that actually committed look failed, and its tokens would be dropped from local bookkeeping. The watcher now tracks whether any poll in the window actually reached the chain, valid or not. Only then does an absent anchor at the timeout mean Invalid. If every attempt errored, it reports OnError instead, the same signal the interface already defines for "the finality event could not be delivered." The transaction stays Pending rather than being marked deleted, and the driver's existing recovery sweep picks it up again later with a fresh read. Signed-off-by: atharrva01 --- .../services/network/evm/finality/manager.go | 19 ++++- .../network/evm/finality/manager_test.go | 82 ++++++++++++++++--- 2 files changed, 88 insertions(+), 13 deletions(-) diff --git a/x/token/services/network/evm/finality/manager.go b/x/token/services/network/evm/finality/manager.go index e6bc78094e..7b544f7dc6 100644 --- a/x/token/services/network/evm/finality/manager.go +++ b/x/token/services/network/evm/finality/manager.go @@ -185,6 +185,12 @@ func (m *Manager) watch(anchor [32]byte, anchorID string, listener driver.Finali ticker := time.NewTicker(m.pollInterval) defer ticker.Stop() + // observed becomes true the moment any poll actually reaches the chain, whether or not the anchor + // was found. It is what separates "checked repeatedly and it is genuinely absent" from "the chain + // was unreachable for the whole window": only the first is evidence the transaction is invalid, and + // conflating them would let a persistent connectivity failure masquerade as a revert. + observed := false + for { select { case <-ticker.C: @@ -192,14 +198,23 @@ func (m *Manager) watch(anchor [32]byte, anchorID string, listener driver.Finali if err != nil { continue // a transient read failure is not a verdict; keep polling until the timeout } + observed = true if code == driver.Valid { listener.OnStatus(ctx, anchorID, code, message, hash) return } case <-ctx.Done(): - // The anchor never appeared. A reverted apply emits nothing, so this is the only failure - // signal available by anchor. + if !observed { + // Every read attempt in the window errored: the chain was never actually reached, so + // there is no evidence the anchor is invalid, only that it could not be observed. + listener.OnError(context.Background(), anchorID, + errors.New("finality: could not reach the chain before the timeout")) + + return + } + // The anchor never appeared, and at least one read genuinely confirmed its absence. A + // reverted apply emits nothing, so this is the only failure signal available by anchor. listener.OnStatus(context.Background(), anchorID, driver.Invalid, "finality timeout", nil) return diff --git a/x/token/services/network/evm/finality/manager_test.go b/x/token/services/network/evm/finality/manager_test.go index fd1fde7225..5f4ebc4982 100644 --- a/x/token/services/network/evm/finality/manager_test.go +++ b/x/token/services/network/evm/finality/manager_test.go @@ -42,14 +42,18 @@ func (s *stubState) apply(hash []byte) { s.mu.Unlock() } -// recordingListener captures the single notification a listener is allowed to receive. +// recordingListener captures the single notification a listener is allowed to receive, through +// whichever of OnStatus/OnError actually fires. type recordingListener struct { - mu sync.Mutex - done chan struct{} - status int - message string - trHash []byte - notified int + mu sync.Mutex + done chan struct{} + status int + message string + trHash []byte + err error + notified int + statusCalled bool + errorCalled bool } func newRecordingListener() *recordingListener { @@ -59,6 +63,7 @@ func newRecordingListener() *recordingListener { func (l *recordingListener) OnStatus(_ context.Context, _ string, status int, message string, trHash []byte) { l.mu.Lock() l.status, l.message, l.trHash = status, message, trHash + l.statusCalled = true l.notified++ first := l.notified == 1 l.mu.Unlock() @@ -67,7 +72,17 @@ func (l *recordingListener) OnStatus(_ context.Context, _ string, status int, me } } -func (l *recordingListener) OnError(context.Context, string, error) {} +func (l *recordingListener) OnError(_ context.Context, _ string, err error) { + l.mu.Lock() + l.err = err + l.errorCalled = true + l.notified++ + first := l.notified == 1 + l.mu.Unlock() + if first { + close(l.done) + } +} func (l *recordingListener) wait(t *testing.T) { t.Helper() @@ -243,14 +258,59 @@ func TestAddListenerRejectsNil(t *testing.T) { require.Error(t, m.AddListener(t.Context(), anchor(0x01), "anchor-1", nil)) } -// TestTransientReadFailureDoesNotResolve checks that a flaky node does not produce a verdict: the -// manager keeps polling rather than reporting a status it cannot support. +// TestTransientReadFailureDoesNotResolve checks that a flaky node does not produce a verdict before +// the timeout: the manager keeps polling rather than reporting a status it cannot support. func TestTransientReadFailureDoesNotResolve(t *testing.T) { + state := &stubState{err: errors.New("temporarily unavailable")} + m := fastManager(&mock.EVMClient{}, state, 500*time.Millisecond) + listener := newRecordingListener() + + require.NoError(t, m.AddListener(t.Context(), anchor(0x01), "anchor-1", listener)) + select { + case <-listener.done: + t.Fatal("listener notified before the timeout on a read failure alone") + case <-time.After(100 * time.Millisecond): + } +} + +// TestPersistentReadFailureReportsErrorNotInvalid is the fix for a real bug: a chain the manager can +// never reach used to resolve as Invalid at the timeout, which the shared ttx listener maps straight +// to a deleted transaction. Nothing was ever actually observed here, so there is no evidence the +// anchor is invalid, only that it could not be checked. That must surface as OnError, not a verdict. +func TestPersistentReadFailureReportsErrorNotInvalid(t *testing.T) { state := &stubState{err: errors.New("temporarily unavailable")} m := fastManager(&mock.EVMClient{}, state, 100*time.Millisecond) listener := newRecordingListener() require.NoError(t, m.AddListener(t.Context(), anchor(0x01), "anchor-1", listener)) listener.wait(t) - assert.Equal(t, driver.Invalid, listener.status, "a persistent read failure resolves only at the timeout") + + listener.mu.Lock() + defer listener.mu.Unlock() + assert.True(t, listener.errorCalled, "an unreachable chain must report OnError, not a verdict") + assert.False(t, listener.statusCalled, "OnStatus must not fire when nothing was ever observed") + require.Error(t, listener.err) +} + +// TestOneCleanMissThenPersistentFailureStillResolvesInvalid pins the boundary of the fix above: once a +// single read has actually reached the chain and found nothing, a later run of read failures does not +// erase that evidence. The transaction still resolves Invalid at the timeout, as design §7.4 intends. +func TestOneCleanMissThenPersistentFailureStillResolvesInvalid(t *testing.T) { + state := &stubState{} + m := fastManager(&mock.EVMClient{}, state, 200*time.Millisecond) + listener := newRecordingListener() + + require.NoError(t, m.AddListener(t.Context(), anchor(0x01), "anchor-1", listener)) + // Let at least one clean poll land (5ms interval) before the chain goes unreachable for the rest + // of the window. + time.Sleep(20 * time.Millisecond) + state.mu.Lock() + state.err = errors.New("node down") + state.mu.Unlock() + + listener.wait(t) + listener.mu.Lock() + defer listener.mu.Unlock() + assert.True(t, listener.statusCalled) + assert.Equal(t, driver.Invalid, listener.status) } From 3f3f76f093f219ab2b7068a5dd4a27546d0a0a7b Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 00:20:35 +0530 Subject: [PATCH 03/35] fix(evm): raise the default finality timeout above real PoS finality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default block tag is finalized, and reading at that tag has a real time-to-finality of roughly 13 minutes (design §7.2). The default finality timeout was 5 minutes, so a deployment running on defaults alone would time out on every single transaction, valid or not, and report it Invalid before the chain could ever finalize it. The design already documents this exact constraint (§7.5: "any deployment must configure finality.timeout above ... the chain's finality"), but nothing enforced it. DefaultFinalityTimeout is now 20 minutes, with real margin over the ~13 minute floor rather than sitting at its edge. Validate also now rejects a finalized-tag configuration whose timeout is shorter than that floor, so a deployer who explicitly sets an unsafe combination gets a startup error instead of every transaction silently failing later. The floor applies only to the finalized tag; safe and latest resolve on their own, faster schedules and are not validated here. Signed-off-by: atharrva01 --- x/token/services/network/evm/config.go | 18 +++++++- x/token/services/network/evm/config_test.go | 51 +++++++++++++++++---- 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/x/token/services/network/evm/config.go b/x/token/services/network/evm/config.go index 10227804b1..1717690576 100644 --- a/x/token/services/network/evm/config.go +++ b/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 @@ -199,6 +208,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 } diff --git a/x/token/services/network/evm/config_test.go b/x/token/services/network/evm/config_test.go index 4056c34019..adef13c138 100644 --- a/x/token/services/network/evm/config_test.go +++ b/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 }, @@ -231,6 +237,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)) From 88c71ab1b1b29042baf40e70a713a12f843b1b92 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 00:26:05 +0530 Subject: [PATCH 04/35] fix(evm): retry a public-parameters reload that failed to apply The watcher recorded a new version as seen before calling the handler that actually applies it, and the handler had no way to report failure at all (UpdateHandler returned nothing). If applying a version failed for any reason, the watcher had already moved past it: the next poll only looks at what changed since the last seen version, so a failed reload was silently never retried, and the node kept serving stale public parameters with nothing left to notice the gap. UpdateHandler now returns an error, and the watcher only advances past a version once its handler actually succeeds; a failure is logged and the same version is retried on the next poll. applyPublicParams collects and returns the combined error of every TMS that failed to update, so a partial failure is visible to the watcher rather than swallowed. Retrying the whole batch is safe: updating a TMS with parameters it already holds is a no-op, so a TMS that already succeeded is not disturbed by covering it again. Covered at the watcher, where the actual defect lived: a new test drives a handler that fails twice then succeeds and asserts the same version is retried rather than skipped, and that seen only advances on the eventual success. Signed-off-by: atharrva01 --- x/token/services/network/evm/driver.go | 17 +++- x/token/services/network/evm/pp/watcher.go | 20 ++++- .../services/network/evm/pp/watcher_test.go | 79 ++++++++++++++++++- 3 files changed, 106 insertions(+), 10 deletions(-) diff --git a/x/token/services/network/evm/driver.go b/x/token/services/network/evm/driver.go index fc9be6d39e..1d786e86bb 100644 --- a/x/token/services/network/evm/driver.go +++ b/x/token/services/network/evm/driver.go @@ -208,8 +208,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 +224,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,13 +244,17 @@ 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 diff --git a/x/token/services/network/evm/pp/watcher.go b/x/token/services/network/evm/pp/watcher.go index d88c230db0..07a3af0a31 100644 --- a/x/token/services/network/evm/pp/watcher.go +++ b/x/token/services/network/evm/pp/watcher.go @@ -27,7 +27,12 @@ const DefaultWatchInterval = time.Second // UpdateHandler is called when the on-chain public parameters have changed, with the new parameters // and the version they were stored at. It is called from the watcher's own goroutine, one call at a // time, so a handler that blocks delays the next poll rather than racing with it. -type UpdateHandler func(ctx context.Context, raw []byte, version uint64) +// +// A non-nil return means this version was not fully applied. The watcher does not advance past a +// version its handler failed on, so the same version is retried on the next poll instead of being +// silently treated as handled: nothing else will ever ask the chain about a version once the watcher +// has moved on from it. +type UpdateHandler func(ctx context.Context, raw []byte, version uint64) error // Watcher notices when a TMS's public parameters change on chain and hands the new ones to a handler. // @@ -160,11 +165,20 @@ func (w *Watcher) poll(ctx context.Context) { return } - // Record what was actually read rather than what the version poll reported. They can differ if + // The handler runs before seen is advanced, and seen only advances once it succeeds. Advancing + // first would mean a failed reload is treated as handled anyway: the next poll only looks at what + // changed since seen, so a version it never actually applied would simply never be asked about + // again. + if err := w.handler(ctx, raw, actual); err != nil { + logger.Warnf("failed to apply public parameters version %d, will retry: %v", actual, err) + + return + } + + // Record what was actually applied rather than what the version poll reported. They can differ if // another update landed in between, and the parameters are the thing that matters. w.mu.Lock() w.seen = actual w.mu.Unlock() logger.Infof("public parameters updated to version %d", actual) - w.handler(ctx, raw, actual) } diff --git a/x/token/services/network/evm/pp/watcher_test.go b/x/token/services/network/evm/pp/watcher_test.go index 149a1f052f..2b9246b0a7 100644 --- a/x/token/services/network/evm/pp/watcher_test.go +++ b/x/token/services/network/evm/pp/watcher_test.go @@ -70,10 +70,12 @@ func newWatcherHarness(t *testing.T, state *chainState) (*Watcher, *[]update, *s var mu sync.Mutex got := make([]update, 0, 4) w, err := NewWatcher(evmClient, tokenState, "latest", 10*time.Millisecond, - func(_ context.Context, raw []byte, version uint64) { + func(_ context.Context, raw []byte, version uint64) error { mu.Lock() defer mu.Unlock() got = append(got, update{raw: string(raw), version: version}) + + return nil }) require.NoError(t, err) @@ -197,10 +199,12 @@ func TestWatcherSurvivesAFailedRead(t *testing.T) { var mu sync.Mutex seen := 0 w, err := NewWatcher(evmClient, tokenState, "latest", 5*time.Millisecond, - func(context.Context, []byte, uint64) { + func(context.Context, []byte, uint64) error { mu.Lock() defer mu.Unlock() seen++ + + return nil }) require.NoError(t, err) @@ -215,6 +219,73 @@ func TestWatcherSurvivesAFailedRead(t *testing.T) { }, 2*time.Second, 5*time.Millisecond, "the watcher must keep polling after a failed read") } +// TestWatcherRetriesAFailedHandler is the fix for a real bug: seen used to advance before the handler +// ran, so a handler that failed to apply a version was never asked about it again. The chain looked +// "seen" forever, and the node kept serving stale parameters with nothing left to notice the gap. +func TestWatcherRetriesAFailedHandler(t *testing.T) { + state := &chainState{} + state.set("params-v0", 0) + + w, attempts, mu := newFailingWatcherHarness(t, state, 3) + w.Start(context.Background()) + defer w.Stop() + + require.Eventually(t, func() bool { return baselineSet(w) }, time.Second, 5*time.Millisecond) + state.set("params-v1", 1) + + require.Eventually(t, func() bool { + mu.Lock() + defer mu.Unlock() + + return *attempts >= 3 + }, 2*time.Second, 5*time.Millisecond, "a failed handler must be retried on the same version, not skipped") + + w.mu.Lock() + seen, hasSeen := w.seen, w.hasSeen + w.mu.Unlock() + assert.True(t, hasSeen) + assert.Equal(t, uint64(1), seen, "seen only advances once the handler actually succeeds") +} + +// newFailingWatcherHarness is newWatcherHarness's counterpart for a handler that fails its first +// succeedAfter-1 calls and succeeds from then on, so a test can assert on the retry, not just the +// eventual success. +func newFailingWatcherHarness(t *testing.T, state *chainState, succeedAfter int) (*Watcher, *int, *sync.Mutex) { + t.Helper() + tokenState, err := client.HexToAddress("0x5FbDB2315678afecb367f032d93F642f64180aa3") + require.NoError(t, err) + + evmClient := &mock.EVMClient{} + evmClient.CallStub = func(_ context.Context, _ client.Address, data []byte, _ string) ([]byte, error) { + raw, version := state.get() + switch string(data) { + case string(abi.MethodID("getPublicParameters()")): + return abiBytesFor(raw), nil + case string(abi.MethodID("getPublicParamsVersion()")): + return abiUint64For(version), nil + } + + return nil, nil + } + + var mu sync.Mutex + attempts := 0 + w, err := NewWatcher(evmClient, tokenState, "latest", 5*time.Millisecond, + func(context.Context, []byte, uint64) error { + mu.Lock() + defer mu.Unlock() + attempts++ + if attempts < succeedAfter { + return assert.AnError + } + + return nil + }) + require.NoError(t, err) + + return w, &attempts, &mu +} + // TestWatcherStopIsIdempotentAndBlocks checks a stopped watcher will not call the handler again, so a // driver shutting down does not race a reload against a torn-down TMS provider. func TestWatcherStopIsIdempotentAndBlocks(t *testing.T) { @@ -241,14 +312,14 @@ func TestNewWatcherValidatesItsInput(t *testing.T) { tokenState, err := client.HexToAddress("0x5FbDB2315678afecb367f032d93F642f64180aa3") require.NoError(t, err) - _, err = NewWatcher(nil, tokenState, "latest", time.Second, func(context.Context, []byte, uint64) {}) + _, err = NewWatcher(nil, tokenState, "latest", time.Second, func(context.Context, []byte, uint64) error { return nil }) require.Error(t, err) _, err = NewWatcher(&mock.EVMClient{}, tokenState, "latest", time.Second, nil) require.Error(t, err) // a non-positive interval falls back to the default rather than spinning - w, err := NewWatcher(&mock.EVMClient{}, tokenState, "latest", 0, func(context.Context, []byte, uint64) {}) + w, err := NewWatcher(&mock.EVMClient{}, tokenState, "latest", 0, func(context.Context, []byte, uint64) error { return nil }) require.NoError(t, err) assert.Equal(t, DefaultWatchInterval, w.interval) } From 219cadf8d44e0fb50be1522151d268d082371e9a Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 00:33:37 +0530 Subject: [PATCH 05/35] fix(evm): retry a torn read between public-parameters bytes and version ChainProvider.PublicParams read the parameter bytes and the version as two separate, unsynchronised calls. An endorsed setup delta landing between them could tear the pair: bytes from before the update, version from after, or the reverse. The contract checks both fields together against what it currently holds and reverts StalePublicParams on a mismatch, so a torn read here did not corrupt state, but it turned a purely local race in this function into a doomed, gas-spending transaction the contract was always going to reject. The version is now read before and after the bytes, and the whole read is retried if it moved: version and bytes only ever change together, in the same transaction, so two matching reads bracketing the bytes read is proof nothing landed in between. The retry is bounded (three attempts) so a pathological chain that never settles fails with an error instead of spinning. TestWatcherSurvivesAFailedRead needed a related fix: it modelled the version as the raw RPC call count, which does not hold once PublicParams reads the version twice per attempt, two call counts a few lines apart would themselves look torn. Rewritten to use the same stable chain-state double the other watcher tests already use. Signed-off-by: atharrva01 --- x/token/services/network/evm/pp/provider.go | 56 +++++++++++++---- .../services/network/evm/pp/provider_test.go | 62 +++++++++++++++++++ .../services/network/evm/pp/watcher_test.go | 29 ++++++--- 3 files changed, 127 insertions(+), 20 deletions(-) diff --git a/x/token/services/network/evm/pp/provider.go b/x/token/services/network/evm/pp/provider.go index 22107a92a6..02f3546461 100644 --- a/x/token/services/network/evm/pp/provider.go +++ b/x/token/services/network/evm/pp/provider.go @@ -19,6 +19,12 @@ import ( // stored parameters. const getPublicParametersMethod = "getPublicParameters()" // #nosec G101 -- ABI method signature +// maxPublicParamsReadAttempts bounds the retry PublicParams performs when it detects a torn read. A +// public-parameters update is a rare, isolated event (design §3.5), so a bracket almost always comes +// back clean on the first attempt; the bound exists only so a chain updating on every single block +// cannot spin this forever. +const maxPublicParamsReadAttempts = 3 + // ChainProvider supplies the public parameters an endorser binds a StateDelta to, reading both the // bytes and the version from the contract. // @@ -55,22 +61,48 @@ func NewChainProvider(evmClient client.EVMClient, tokenState client.Address, blo } // PublicParams returns the parameters currently stored on chain and their version. +// +// There is no single getter for both (getPublicParameters and getPublicParamsVersion are separate +// calls), so an endorsed setup delta landing between them could tear the pair: bytes from before the +// update paired with the version from after, or the reverse. The contract would catch this at apply +// time, since it checks both fields together and reverts StalePublicParams unless they match exactly +// what it currently holds, but that turns a purely local race in this function into a doomed, +// gas-spending transaction rather than nothing happening at all. +// +// The version is read before and after the bytes, and the whole read is retried if it moved: version +// and bytes only ever change together, in the same transaction (design §3.5), so two matching reads +// bracketing the bytes read is proof nothing landed in between. func (p *ChainProvider) PublicParams(ctx context.Context) ([]byte, uint64, error) { - raw, err := p.client.Call(ctx, p.tokenState, abi.MethodID(getPublicParametersMethod), p.blockTag) - if err != nil { - return nil, 0, errors.Wrap(err, "failed to read the public parameters") - } - params, err := abi.DecodeBytes(raw) - if err != nil { - return nil, 0, errors.Wrap(err, "failed to decode the public parameters") - } + var lastVersion uint64 + for range maxPublicParamsReadAttempts { + before, err := p.versions.Sync(ctx) + if err != nil { + return nil, 0, err + } + + raw, err := p.client.Call(ctx, p.tokenState, abi.MethodID(getPublicParametersMethod), p.blockTag) + if err != nil { + return nil, 0, errors.Wrap(err, "failed to read the public parameters") + } + params, err := abi.DecodeBytes(raw) + if err != nil { + return nil, 0, errors.Wrap(err, "failed to decode the public parameters") + } - version, err := p.versions.Sync(ctx) - if err != nil { - return nil, 0, err + after, err := p.versions.Sync(ctx) + if err != nil { + return nil, 0, err + } + if after == before { + return params, after, nil + } + lastVersion = after } - return params, version, nil + return nil, 0, errors.Errorf( + "evm pp: public parameters changed on every read attempt (%d), last observed version %d", + maxPublicParamsReadAttempts, lastVersion, + ) } // Invalidate drops the cached version. PublicParams re-reads the version on every call, so there is diff --git a/x/token/services/network/evm/pp/provider_test.go b/x/token/services/network/evm/pp/provider_test.go index 00fb671d36..e1a0118a7b 100644 --- a/x/token/services/network/evm/pp/provider_test.go +++ b/x/token/services/network/evm/pp/provider_test.go @@ -67,6 +67,68 @@ func TestPublicParamsFollowsAnUpdate(t *testing.T) { assert.EqualValues(t, 1, version, "the version must be the one those parameters are stored at") } +// TestPublicParamsRetriesATornRead is the fix for a real bug: the bytes and the version were read as +// two separate calls with nothing bracketing them, so an update landing in between produced an +// internally inconsistent pair. The contract would reject it (it checks both fields together), so the +// practical cost was a doomed, gas-spending transaction rather than data corruption, but it was a +// deterministic gap in this function alone, closeable without touching the contract. +func TestPublicParamsRetriesATornRead(t *testing.T) { + tokenState, err := client.HexToAddress("0x5FbDB2315678afecb367f032d93F642f64180aa3") + require.NoError(t, err) + + // The version read returns 0, 1, 1, 1: the first attempt's bracket (before=0, after=1) straddles + // an update and must be retried; the second attempt's (before=1, after=1) does not. + versionReads := []uint64{0, 1, 1, 1} + calls := 0 + evmClient := &mock.EVMClient{} + evmClient.CallStub = func(_ context.Context, _ client.Address, data []byte, _ string) ([]byte, error) { + switch string(data) { + case string(abi.MethodID("getPublicParameters()")): + return abiBytesFor([]byte("params")), nil + case string(abi.MethodID("getPublicParamsVersion()")): + v := versionReads[calls] + calls++ + + return abiUint64For(v), nil + } + + return nil, nil + } + + provider := NewChainProvider(evmClient, tokenState, "latest") + raw, version, err := provider.PublicParams(context.Background()) + require.NoError(t, err) + assert.Equal(t, "params", string(raw)) + assert.EqualValues(t, 1, version, "the torn first attempt must be discarded, not returned") + assert.Equal(t, 4, calls, "a torn bracket must be retried, not accepted") +} + +// TestPublicParamsGivesUpAfterRepeatedTears checks the retry is bounded: a chain whose version never +// settles between two reads must not spin PublicParams forever. +func TestPublicParamsGivesUpAfterRepeatedTears(t *testing.T) { + tokenState, err := client.HexToAddress("0x5FbDB2315678afecb367f032d93F642f64180aa3") + require.NoError(t, err) + + var version uint64 + evmClient := &mock.EVMClient{} + evmClient.CallStub = func(_ context.Context, _ client.Address, data []byte, _ string) ([]byte, error) { + switch string(data) { + case string(abi.MethodID("getPublicParameters()")): + return abiBytesFor([]byte("params")), nil + case string(abi.MethodID("getPublicParamsVersion()")): + version++ + + return abiUint64For(version), nil + } + + return nil, nil + } + + provider := NewChainProvider(evmClient, tokenState, "latest") + _, _, err = provider.PublicParams(context.Background()) + require.Error(t, err) +} + // TestPublicParamsIsConsistentAcrossRepeatedReads checks the pair stays matched over several updates, // since a version that lagged by one would still look right on the first read after each change. func TestPublicParamsIsConsistentAcrossRepeatedReads(t *testing.T) { diff --git a/x/token/services/network/evm/pp/watcher_test.go b/x/token/services/network/evm/pp/watcher_test.go index 2b9246b0a7..0eb537c8f4 100644 --- a/x/token/services/network/evm/pp/watcher_test.go +++ b/x/token/services/network/evm/pp/watcher_test.go @@ -178,22 +178,32 @@ func TestWatcherReportsEachUpdateOnce(t *testing.T) { // one bad read would leave the node on stale parameters forever, which is far worse than a late // notice. func TestWatcherSurvivesAFailedRead(t *testing.T) { + state := &chainState{} + state.set("params-v0", 0) + tokenState, err := client.HexToAddress("0x5FbDB2315678afecb367f032d93F642f64180aa3") require.NoError(t, err) + // Only the version read errors, and only for the first four calls; everything after that, and + // every read of the bytes, falls through to the stable chain state below. Returning the raw call + // count as the version (the original shape of this test) does not work once PublicParams reads the + // version twice per attempt to detect a torn read: two different call counts a few lines apart + // would themselves look torn and the test would never get past that. var calls atomic.Int64 evmClient := &mock.EVMClient{} evmClient.CallStub = func(_ context.Context, _ client.Address, data []byte, _ string) ([]byte, error) { - n := calls.Add(1) - if string(data) == string(abi.MethodID("getPublicParamsVersion()")) { - if n <= 4 { - return nil, assert.AnError - } - - return abiUint64For(uint64(n)), nil + if string(data) == string(abi.MethodID("getPublicParamsVersion()")) && calls.Add(1) <= 4 { + return nil, assert.AnError + } + raw, version := state.get() + switch string(data) { + case string(abi.MethodID("getPublicParameters()")): + return abiBytesFor(raw), nil + case string(abi.MethodID("getPublicParamsVersion()")): + return abiUint64For(version), nil } - return abiBytesFor([]byte("params")), nil + return nil, nil } var mu sync.Mutex @@ -211,6 +221,9 @@ func TestWatcherSurvivesAFailedRead(t *testing.T) { w.Start(context.Background()) defer w.Stop() + require.Eventually(t, func() bool { return baselineSet(w) }, time.Second, 5*time.Millisecond) + state.set("params-v1", 1) + require.Eventually(t, func() bool { mu.Lock() defer mu.Unlock() From 80cd8553c94d4e286a4c9c8ed0be81e45c692bc2 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 00:39:43 +0530 Subject: [PATCH 06/35] fix(evm): make SetupPublicParams reach the endorsement factory SetupPublicParams checked only Network.endorsement, the field tests inject a stub into directly. In production nothing ever sets that field: the driver wires the per-TMS endorsement factory through endorsementFor instead, keyed by an already-built management service. SetupPublicParams never has one of those to hand it, that is the entire reason it takes a bare TMSID rather than a management service, so it could never reach the factory at all. Every call failed with "no endorsement service configured" on any real deployment, and first-time setup of a namespace, the one thing this method exists to make possible, could not work. Nothing caught this: the shared ppsetup view exercises the real production path, but the EVM integration suite bootstraps and updates parameters through its own harness-side submitter instead, bypassing this method entirely, so the gap was invisible to every existing test. Network now also carries endorsementForID, a TMSID-keyed counterpart to endorsementFor, and SetupPublicParams resolves through that instead. The driver installs it in installEndorsement next to the existing factory: both ultimately call the same per-TMS ServiceFactory.ForTMS, one entered from a management service, the other from the id alone. New tests cover what nothing did before: SetupPublicParams resolving through the id-based factory and reaching it with the requested TMS, and a failed endorsement collection not broadcasting. Signed-off-by: atharrva01 --- x/token/services/network/evm/driver.go | 5 ++ x/token/services/network/evm/network.go | 41 +++++++++++++--- x/token/services/network/evm/network_test.go | 51 ++++++++++++++++++++ 3 files changed, 89 insertions(+), 8 deletions(-) diff --git a/x/token/services/network/evm/driver.go b/x/token/services/network/evm/driver.go index 1d786e86bb..33e42d2935 100644 --- a/x/token/services/network/evm/driver.go +++ b/x/token/services/network/evm/driver.go @@ -300,6 +300,11 @@ 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 diff --git a/x/token/services/network/evm/network.go b/x/token/services/network/evm/network.go index abf9d880dd..98e365de66 100644 --- a/x/token/services/network/evm/network.go +++ b/x/token/services/network/evm/network.go @@ -44,11 +44,15 @@ type Network struct { // endorsementFor resolves the per-TMS endorsement service. It is preferred over the field above, // which stays for tests that inject a stub directly. endorsementFor func(tms *token2.ManagementService) (EndorsementService, error) - submitter *Submitter - reader *contractReader - finality *finality.Manager - tokenState client.Address - membership driver.LocalMembership + // endorsementForID is endorsementFor's counterpart for callers that only have a TMSID, not an + // already-built management service: SetupPublicParams needs this, since first-time setup of a + // namespace runs before a management service can exist for it to have one at all. + endorsementForID func(tmsID token2.TMSID) (EndorsementService, error) + submitter *Submitter + reader *contractReader + finality *finality.Manager + tokenState client.Address + membership driver.LocalMembership // startRecovery starts the per-TMS transaction recovery sweep. It is installed by the driver, // which owns the stores, and runs when a namespace binds rather than when the network is built. startRecovery func(ns string) error @@ -217,6 +221,26 @@ func (n *Network) SetEndorsementFactory(f func(tms *token2.ManagementService) (E n.endorsementFor = f } +// endorserForID is endorserFor's counterpart for a bare TMS id: the one injected directly if there is +// one, otherwise the per-TMS service the factory builds. SetupPublicParams uses this rather than +// endorserFor because it may run before a management service exists for the TMS at all. +func (n *Network) endorserForID(tmsID token2.TMSID) (EndorsementService, error) { + if n.endorsement != nil { + return n.endorsement, nil + } + if n.endorsementForID == nil { + return nil, errors.New("evm network: no endorsement service configured") + } + + return n.endorsementForID(tmsID) +} + +// SetEndorsementFactoryByID installs the resolver endorserForID uses. It is SetEndorsementFactory's +// counterpart for the id-only path. +func (n *Network) SetEndorsementFactoryByID(f func(tmsID token2.TMSID) (EndorsementService, error)) { + n.endorsementForID = f +} + // SetRecoveryStarter installs the hook that starts transaction recovery for a namespace. Like the // endorsement factory it is set by the driver after construction, because the stores it sweeps // belong to the driver rather than to the network. @@ -280,12 +304,13 @@ func (n *Network) SetupPublicParams( signer view.Identity, txID driver.TxID, ) (driver.Envelope, error) { - if n.endorsement == nil { - return nil, errors.New("evm network: no endorsement service configured") + endorser, err := n.endorserForID(tmsID) + if err != nil { + return nil, err } anchor := n.ComputeTxID(&txID) - result, err := n.endorsement.Endorse(context, &endorsement.EndorseRequest{ + result, err := endorser.Endorse(context, &endorsement.EndorseRequest{ TokenRequest: publicParamsRaw, TMSID: tmsID, Anchor: anchor, diff --git a/x/token/services/network/evm/network_test.go b/x/token/services/network/evm/network_test.go index c7ff1d9631..95d1c38dae 100644 --- a/x/token/services/network/evm/network_test.go +++ b/x/token/services/network/evm/network_test.go @@ -267,6 +267,57 @@ func TestRequestApprovalWithoutEndorsementService(t *testing.T) { require.Error(t, err) } +// --- SetupPublicParams ----------------------------------------------------------------------------- + +// TestSetupPublicParamsWithoutEndorsementService checks the same failure mode RequestApproval has: +// nothing configured means a clear error, not a nil dereference further down. +func TestSetupPublicParamsWithoutEndorsementService(t *testing.T) { + n := testNetwork(t, nil, nil) + _, err := n.SetupPublicParams(nil, token2.TMSID{Network: "evm", Namespace: "token"}, []byte("pp"), nil, driverTxID()) + require.Error(t, err) +} + +// TestSetupPublicParamsResolvesThroughTheIDBasedFactory is the fix for a real bug: SetupPublicParams +// checked only the endorsement field tests inject directly, never the per-TMS factory the driver +// actually installs in production (SetEndorsementFactory, keyed by a management service). Since +// SetupPublicParams only ever has a bare TMSID, not a management service, it could never reach that +// factory, and the call failed with "no endorsement service configured" on every real deployment +// regardless of how correctly everything else was wired: first-time setup of a namespace, the one +// thing this method exists to make possible, could not work at all. +func TestSetupPublicParamsResolvesThroughTheIDBasedFactory(t *testing.T) { + n := testNetwork(t, nil, nil) + tmsID := token2.TMSID{Network: "evm", Namespace: "token"} + stub := &stubEndorser{result: &endorsement.Result{Anchor: "anchor", Endorsements: [][]byte{{0x01}}}} + + var seenID token2.TMSID + n.SetEndorsementFactoryByID(func(id token2.TMSID) (EndorsementService, error) { + seenID = id + + return stub, nil + }) + + env, err := n.SetupPublicParams(nil, tmsID, []byte("pp"), nil, driverTxID()) + require.NoError(t, err) + assert.NotNil(t, env) + assert.Equal(t, tmsID, seenID, "the factory must be asked to resolve exactly the TMS being set up") + require.NotNil(t, stub.seen) + assert.Equal(t, tmsID, stub.seen.TMSID) +} + +// TestSetupPublicParamsDoesNotTouchTheChain mirrors TestRequestApprovalDoesNotTouchTheChain: a failed +// endorsement collection must not broadcast anything. +func TestSetupPublicParamsDoesNotTouchTheChain(t *testing.T) { + evm := &mock.EVMClient{} + n := testNetwork(t, evm, nil) + n.SetEndorsementFactoryByID(func(token2.TMSID) (EndorsementService, error) { + return &stubEndorser{err: errors.New("no quorum")}, nil + }) + + _, err := n.SetupPublicParams(nil, token2.TMSID{Network: "evm", Namespace: "token"}, []byte("pp"), nil, driverTxID()) + require.Error(t, err) + assert.Zero(t, evm.SendRawTransactionCallCount(), "a failed setup must not broadcast") +} + // --- Broadcast ----------------------------------------------------------------------------------- func TestBroadcastRejectsBadInput(t *testing.T) { From f32ddcdc46466da48f0ba723e09d23b4cd0573a2 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 00:45:48 +0530 Subject: [PATCH 07/35] fix(evm): close the race between a failed broadcast and a concurrent nonce allocation NonceManager split allocation and recovery into two separate critical sections: Next handed out a nonce and released the lock, and a failed Submit later called Reset, which walked the whole sequence back to whatever eth_getTransactionCount(pending) currently showed. That call only reflects transactions the node has actually seen. A different, concurrent Submit that had already allocated a higher nonce but not yet reached SendRawTransaction was invisible to it, and Reset would hand that same nonce to a third caller, producing two transactions racing for one nonce. Every path that called Reset already treated its own failure as certain proof the transaction never reached the chain: gas estimation and fee suggestion are read-only, signing is local, and a rejected broadcast is documented as never judged by the chain. So walking back to the chain's view was never actually necessary, it just happened to be how the recovery was implemented, and that implementation was what raced. NonceManager.Next and Reset are replaced by WithNonce, which holds the lock for the whole allocate-and-use step. The sequence advances only if the callback succeeds; on failure the nonce is simply left where it was, with no round trip to the chain, and nothing else could have been mid allocation while the callback ran. Submitter.Submit now runs its entire body inside that callback. New tests cover the failure path directly (a failed attempt does not advance the sequence and needs no re-sync) and drive many goroutines with a mix of successes and failures to check every successful attempt still gets a distinct nonce. Signed-off-by: atharrva01 --- x/token/services/network/evm/nonce.go | 42 +++---- x/token/services/network/evm/nonce_test.go | 107 ++++++++++++++---- x/token/services/network/evm/submitter.go | 79 ++++++------- .../services/network/evm/submitter_test.go | 26 +++-- 4 files changed, 161 insertions(+), 93 deletions(-) diff --git a/x/token/services/network/evm/nonce.go b/x/token/services/network/evm/nonce.go index 60e06f3d03..bbc95a91e8 100644 --- a/x/token/services/network/evm/nonce.go +++ b/x/token/services/network/evm/nonce.go @@ -16,11 +16,11 @@ import ( ) // NonceManager hands out the submitter account's Ethereum transaction nonces. Ethereum requires them -// to be strictly sequential per account, so concurrent broadcasts must not read the same value: the -// manager serializes allocation and tracks the next nonce locally rather than asking the node each -// time, which would hand the same nonce to two callers racing between the query and the send. +// to be strictly sequential per account, so two broadcasts must never collide: WithNonce holds a lock +// across the whole allocate-and-use step, not just the allocation, so nothing else can be mid-flight +// when a failed attempt needs the sequence walked back. // -// The chain is the source of truth, so there is nothing to persist. On a fresh start (or after a +// The chain is the only source of truth, so nothing here is persisted. On a fresh start (or after a // restart) the first allocation recovers from eth_getTransactionCount at the pending tag, which // already accounts for transactions still in the mempool. That makes a node restart transparent, at // the cost of one round trip. @@ -38,35 +38,37 @@ func NewNonceManager(evmClient client.EVMClient, submitter client.Address) *Nonc return &NonceManager{client: evmClient, submitter: submitter} } -// Next returns the nonce to use for the next transaction and advances the counter. The caller owns -// the returned nonce: if it does not end up broadcasting, it should Reset so the sequence does not -// develop a gap that stalls every later transaction. -func (n *NonceManager) Next(ctx context.Context) (uint64, error) { +// WithNonce allocates the next nonce and runs use with it, holding the manager's lock for the whole +// step. The sequence advances only if use succeeds; on failure the nonce is left exactly where it +// was, so the next call gets the same value, with no round trip to the chain needed to know that is +// safe. +// +// It is safe because of what use's contract already is, not despite it: every caller in this driver +// treats a failure as proof the transaction never reached the chain (gas estimation and fee +// suggestion are read-only, signing is local, and a rejected broadcast is documented as never judged +// by the chain), and while use runs, no other call can be allocating a nonce for this account at all. +// A nonce handed out twice used to happen exactly in the gap between those two things: one call's +// failure re-derived the whole sequence from the chain's current mempool view, which does not yet +// include a nonce a different, still in-flight call already holds and has not broadcast yet. +func (n *NonceManager) WithNonce(ctx context.Context, use func(nonce uint64) error) error { n.mu.Lock() defer n.mu.Unlock() if !n.initialized { pending, err := n.client.PendingNonceAt(ctx, n.submitter) if err != nil { - return 0, errors.Wrapf(err, "failed to recover the nonce for submitter [%s]", n.submitter) + return errors.Wrapf(err, "failed to recover the nonce for submitter [%s]", n.submitter) } n.next = pending n.initialized = true } - nonce := n.next + if err := use(n.next); err != nil { + return err + } n.next++ - return nonce, nil -} - -// Reset drops the cached counter so the next allocation re-reads from the node. It is the recovery -// path for a failed broadcast: rather than guess whether the node saw the transaction, re-derive the -// sequence from the pending nonce, which is authoritative. -func (n *NonceManager) Reset() { - n.mu.Lock() - n.initialized = false - n.mu.Unlock() + return nil } // Cached returns the next nonce the manager would hand out and whether it has been initialized, diff --git a/x/token/services/network/evm/nonce_test.go b/x/token/services/network/evm/nonce_test.go index 1a154bfc03..7ba34e3cf9 100644 --- a/x/token/services/network/evm/nonce_test.go +++ b/x/token/services/network/evm/nonce_test.go @@ -25,7 +25,12 @@ func TestNonceRecoversFromChain(t *testing.T) { evm.PendingNonceAtReturns(42, nil) n := NewNonceManager(evm, client.Address{}) - got, err := n.Next(t.Context()) + var got uint64 + err := n.WithNonce(t.Context(), func(nonce uint64) error { + got = nonce + + return nil + }) require.NoError(t, err) assert.Equal(t, uint64(42), got, "the first nonce must come from the chain, not from zero") } @@ -39,31 +44,46 @@ func TestNonceIsSequentialWithoutRefetching(t *testing.T) { n := NewNonceManager(evm, client.Address{}) for want := uint64(7); want < 12; want++ { - got, err := n.Next(t.Context()) + err := n.WithNonce(t.Context(), func(nonce uint64) error { + assert.Equal(t, want, nonce) + + return nil + }) require.NoError(t, err) - assert.Equal(t, want, got) } assert.Equal(t, 1, evm.PendingNonceAtCallCount(), "the node is consulted once, not per transaction") } -// TestNonceResetReRecovers checks the failed-broadcast path: after a reset the sequence is -// re-derived from the node, which is authoritative about what it has actually seen. -func TestNonceResetReRecovers(t *testing.T) { +// TestNonceIsNotAdvancedOnFailure is the fix for a real bug. The manager used to have a separate +// Reset that walked the whole sequence back and re-derived it from the chain's current mempool view, +// which does not include a nonce a different, still in-flight call already holds and has not +// broadcast yet: one call's failure could hand that nonce to somebody else. WithNonce closes the gap +// by holding the lock for the whole allocate-and-use step, so a failed attempt simply leaves the +// nonce where it was: nothing else could have raced in while use ran, so reusing the same value on +// the next call needs no round trip to the chain to be safe. +func TestNonceIsNotAdvancedOnFailure(t *testing.T) { evm := &mock.EVMClient{} - evm.PendingNonceAtReturnsOnCall(0, 5, nil) - evm.PendingNonceAtReturnsOnCall(1, 9, nil) + evm.PendingNonceAtReturns(5, nil) n := NewNonceManager(evm, client.Address{}) - first, err := n.Next(t.Context()) - require.NoError(t, err) - assert.Equal(t, uint64(5), first) + err := n.WithNonce(t.Context(), func(nonce uint64) error { + assert.Equal(t, uint64(5), nonce) + + return assert.AnError + }) + require.Error(t, err) - n.Reset() + err = n.WithNonce(t.Context(), func(nonce uint64) error { + assert.Equal(t, uint64(5), nonce, "a failed attempt must not consume the nonce") - second, err := n.Next(t.Context()) + return nil + }) require.NoError(t, err) - assert.Equal(t, uint64(9), second, "after a reset the nonce must come from the node again") - assert.Equal(t, 2, evm.PendingNonceAtCallCount()) + assert.Equal(t, 1, evm.PendingNonceAtCallCount(), "recovering from a failure needs no round trip to the chain") + + next, initialized := n.Cached() + assert.True(t, initialized) + assert.Equal(t, uint64(6), next, "the sequence advances only past the attempt that actually succeeded") } func TestNonceSurfacesRecoveryFailure(t *testing.T) { @@ -71,8 +91,14 @@ func TestNonceSurfacesRecoveryFailure(t *testing.T) { evm.PendingNonceAtReturns(0, errors.New("node unavailable")) n := NewNonceManager(evm, client.Address{}) - _, err := n.Next(t.Context()) + called := false + err := n.WithNonce(t.Context(), func(uint64) error { + called = true + + return nil + }) require.Error(t, err) + assert.False(t, called, "use must not run when the nonce could not be recovered") _, initialized := n.Cached() assert.False(t, initialized, "a failed recovery must not leave the manager initialized") @@ -93,16 +119,51 @@ func TestNonceConcurrentAllocationIsUnique(t *testing.T) { ) for range callers { wg.Go(func() { - nonce, err := n.Next(t.Context()) - if err != nil { - return - } - mu.Lock() - got[nonce] = struct{}{} - mu.Unlock() + err := n.WithNonce(t.Context(), func(nonce uint64) error { + mu.Lock() + got[nonce] = struct{}{} + mu.Unlock() + + return nil + }) + assert.NoError(t, err) }) } wg.Wait() assert.Len(t, got, callers, "every concurrent caller must receive a distinct nonce") } + +// TestNonceHandlesConcurrentFailuresWithoutDuplicating drives many goroutines against one manager, +// roughly half of them failing, and checks every nonce a goroutine actually succeeded with is unique. +// This is the scenario the old Reset-based design got wrong: a failure from one caller must not be +// able to hand a still in-flight caller's nonce to somebody else. +func TestNonceHandlesConcurrentFailuresWithoutDuplicating(t *testing.T) { + evm := &mock.EVMClient{} + evm.PendingNonceAtReturns(0, nil) + n := NewNonceManager(evm, client.Address{}) + + const callers = 200 + var ( + wg sync.WaitGroup + mu sync.Mutex + got = make(map[uint64]struct{}, callers) + ) + for i := range callers { + wg.Go(func() { + _ = n.WithNonce(t.Context(), func(nonce uint64) error { + if i%2 == 0 { + return assert.AnError + } + mu.Lock() + got[nonce] = struct{}{} + mu.Unlock() + + return nil + }) + }) + } + wg.Wait() + + assert.Len(t, got, callers/2, "every successful attempt must have consumed a distinct nonce") +} diff --git a/x/token/services/network/evm/submitter.go b/x/token/services/network/evm/submitter.go index 6b9b09d71c..6a11df24d5 100644 --- a/x/token/services/network/evm/submitter.go +++ b/x/token/services/network/evm/submitter.go @@ -71,8 +71,10 @@ func NewSubmitter( func (s *Submitter) Address() client.Address { return s.address } // Submit encodes applyStateDelta(delta, endorsements), signs it and broadcasts it, returning the raw -// transaction and its hash. On any failure after a nonce was allocated it resets the sequence, so a -// transaction that never reached the node does not leave a gap that would stall every later one. +// transaction and its hash. The whole nonce-relevant sequence runs under the nonce manager's lock, so +// a failure here (the delta reverts on estimation, the node rejects the broadcast) can never race a +// different transaction's allocation: the nonce this attempt held is simply not consumed, and the next +// caller, whether that is a retry of this same transaction or somebody else's, gets it instead. func (s *Submitter) Submit( ctx context.Context, delta *statedelta.StateDelta, @@ -87,48 +89,47 @@ func (s *Submitter) Submit( data := abi.EncodeApplyStateDelta(delta, endorsements) - nonce, err := s.nonces.Next(ctx) - if err != nil { - return nil, client.Hash{}, err - } - defer func() { - if err != nil { - // The nonce was allocated but never consumed on chain; re-derive the sequence. - s.nonces.Reset() + err = s.nonces.WithNonce(ctx, func(nonce uint64) error { + gasLimit, gasErr := s.gasLimit(ctx, data) + if gasErr != nil { + return gasErr + } + fees, feeErr := s.client.SuggestGasFees(ctx) + if feeErr != nil { + return errors.Wrapf(ErrNetworkUnavailable, "submitter: failed to get gas fees: %v", feeErr) } - }() - gasLimit, err := s.gasLimit(ctx, data) - if err != nil { - return nil, client.Hash{}, err - } - fees, err := s.client.SuggestGasFees(ctx) - if err != nil { - return nil, client.Hash{}, errors.Wrapf(ErrNetworkUnavailable, "submitter: failed to get gas fees: %v", err) - } + tx := &client.DynamicFeeTx{ + ChainID: s.chainID, + Nonce: nonce, + MaxPriorityFeePerGas: fees.MaxPriorityFeePerGas, + MaxFeePerGas: fees.MaxFeePerGas, + Gas: gasLimit, + To: &s.tokenState, + Value: big.NewInt(0), // applyStateDelta is not payable + Data: data, + } - tx := &client.DynamicFeeTx{ - ChainID: s.chainID, - Nonce: nonce, - MaxPriorityFeePerGas: fees.MaxPriorityFeePerGas, - MaxFeePerGas: fees.MaxFeePerGas, - Gas: gasLimit, - To: &s.tokenState, - Value: big.NewInt(0), // applyStateDelta is not payable - Data: data, - } + signed, signErr := client.SignTx(tx, s.key) + if signErr != nil { + return errors.Wrap(signErr, "submitter: failed to sign the transaction") + } - rawTx, err = client.SignTx(tx, s.key) - if err != nil { - return nil, client.Hash{}, errors.Wrap(err, "submitter: failed to sign the transaction") - } - txHash, err = s.client.SendRawTransaction(ctx, rawTx) + hash, sendErr := s.client.SendRawTransaction(ctx, signed) + if sendErr != nil { + // A transaction the node refused to accept was never judged by the chain, so this is the + // transient class: the delta is still valid and the caller should retry rather than + // re-derive it. A node that executed it and rejected it comes back through gasLimit instead. + return errors.Wrapf( + ErrNetworkUnavailable, "submitter: failed to broadcast the transaction: %v", sendErr) + } + + rawTx, txHash = signed, hash + + return nil + }) if err != nil { - // A transaction the node refused to accept was never judged by the chain, so this is the - // transient class: the delta is still valid and the caller should retry rather than re-derive - // it. A node that executed it and rejected it comes back through gasLimit instead. - return nil, client.Hash{}, errors.Wrapf( - ErrNetworkUnavailable, "submitter: failed to broadcast the transaction: %v", err) + return nil, client.Hash{}, err } return rawTx, txHash, nil diff --git a/x/token/services/network/evm/submitter_test.go b/x/token/services/network/evm/submitter_test.go index f33e2f7be4..1e4b41eef0 100644 --- a/x/token/services/network/evm/submitter_test.go +++ b/x/token/services/network/evm/submitter_test.go @@ -115,10 +115,12 @@ func TestSubmitFixedGasStrategy(t *testing.T) { assert.Zero(t, evm.EstimateGasCallCount(), "a fixed limit must not consult the node") } -// TestSubmitResetsNonceOnFailure is the production trap: a nonce allocated for a transaction that -// never reached the node would leave a gap, and every later transaction would sit unmineable behind -// it. A failed submit must put the sequence back under the node's authority. -func TestSubmitResetsNonceOnFailure(t *testing.T) { +// TestSubmitDoesNotAdvanceTheNonceOnFailure is the production trap: a nonce allocated for a +// transaction that never reached the node must not be lost, or every later transaction sits +// unmineable behind the gap. Submit runs the whole attempt under the nonce manager's lock, so a +// failure simply leaves the nonce exactly where it was, ready to be reused without asking the chain +// again. +func TestSubmitDoesNotAdvanceTheNonceOnFailure(t *testing.T) { evm := readySubmitterClient() evm.SendRawTransactionReturns(client.Hash{}, errors.New("broadcast rejected")) s := testSubmitter(t, evm, estimateGas()) @@ -126,8 +128,9 @@ func TestSubmitResetsNonceOnFailure(t *testing.T) { _, _, err := s.Submit(t.Context(), testDelta(), [][]byte{make([]byte, 65)}) require.Error(t, err) - _, initialized := s.nonces.Cached() - assert.False(t, initialized, "a failed broadcast must reset the nonce sequence") + next, initialized := s.nonces.Cached() + assert.True(t, initialized, "the manager still knows its sequence; there was nothing to lose") + assert.Equal(t, uint64(3), next, "the nonce this attempt held must still be the next one handed out") } // TestSubmitClassifiesARevertedEstimate covers the verdict the shared fungible bodies actually read. @@ -189,17 +192,18 @@ func TestSubmitClassifiesARevertedEstimate(t *testing.T) { require.NotErrorIs(t, err, ErrTransactionReverted) }) - // The nonce reset is what makes the retry advice usable: a rejected transaction that consumed a - // nonce locally would leave every later one stuck behind a gap. - t.Run("a rejected transaction leaves no nonce gap", func(t *testing.T) { + // Not consuming the nonce is what makes the retry advice usable: a rejected transaction that + // consumed a nonce locally would leave every later one stuck behind a gap. + t.Run("a rejected transaction leaves its nonce ready to reuse", func(t *testing.T) { evm := readySubmitterClient() evm.EstimateGasReturns(0, reverted) s := testSubmitter(t, evm, estimateGas()) _, _, err := s.Submit(t.Context(), testDelta(), [][]byte{make([]byte, 65)}) require.Error(t, err) - _, initialized := s.nonces.Cached() - assert.False(t, initialized, "a rejected transaction must put the sequence back under the node") + next, initialized := s.nonces.Cached() + assert.True(t, initialized) + assert.Equal(t, uint64(3), next, "the rejected attempt's nonce must still be next in line") }) } From 55a9c8f5ccd4750fe821728ca6feadce7b039a6a Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 01:13:21 +0530 Subject: [PATCH 08/35] fix(evm): refuse a second network's endorser registration instead of discarding it silently Driver.New runs once per (network, channel) the node is configured for, and installEndorsement, called from it, builds a fresh per-network ServiceFactory and tries to register this node's endorsement responder every time. That registration used a sync.Once scoped to the whole Driver, not to a network, so on a node configured to endorse for more than one EVM network, only the first network's registration ever ran. Every later network's factory, key and allowlist were silently discarded, with nothing logged to say so. The reason a straight per-network fix is not possible: FSC routes an incoming session to a responder by the initiating view's Go type alone, with no notion of "this responder, but only for network X". Only one Responder can ever be registered for endorsement.Initiator across a process's lifetime, so whichever network's factory happens to win the race is baked into it permanently, EIP-712 domain, chain client and all. Routing a second network's requests through it would not fail cleanly, it would validate against the right TMS but sign and read against the wrong chain. registerEndorser now tracks which network it registered for and refuses, loudly, when a different network also wants to endorse: an operator gets a clear error naming both networks instead of a request that silently never gets answered. The same network registering twice (a network rebuilt over a node's life) and a network that never wanted to endorse in the first place both remain unaffected. Signed-off-by: atharrva01 --- x/token/services/network/evm/driver.go | 95 +++++++++----- x/token/services/network/evm/driver_test.go | 134 ++++++++++++++++++++ 2 files changed, 194 insertions(+), 35 deletions(-) diff --git a/x/token/services/network/evm/driver.go b/x/token/services/network/evm/driver.go index 33e42d2935..9a733301d1 100644 --- a/x/token/services/network/evm/driver.go +++ b/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) @@ -260,7 +263,7 @@ func (d *Driver) applyPublicParams(ctx context.Context, tmsIDs []token2.TMSID, r // 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") @@ -309,7 +312,7 @@ func (d *Driver) installEndorsement(n *Network, config *Config, evmClient client // 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 } @@ -338,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. diff --git a/x/token/services/network/evm/driver_test.go b/x/token/services/network/evm/driver_test.go index df66cdc2d3..58df8aeca4 100644 --- a/x/token/services/network/evm/driver_test.go +++ b/x/token/services/network/evm/driver_test.go @@ -7,14 +7,20 @@ SPDX-License-Identifier: Apache-2.0 package evm import ( + "context" "testing" token2 "github.com/LFDT-Panurus/panurus/token" "github.com/LFDT-Panurus/panurus/token/services/config" "github.com/LFDT-Panurus/panurus/token/services/network/driver" "github.com/LFDT-Panurus/panurus/x/token/services/network/evm/client/mock" + "github.com/LFDT-Panurus/panurus/x/token/services/network/evm/eip712" + "github.com/LFDT-Panurus/panurus/x/token/services/network/evm/endorsement" + "github.com/LFDT-Panurus/panurus/x/token/services/network/evm/pp" "github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors" + svcview "github.com/hyperledger-labs/fabric-smart-client/platform/view/services/view" + "github.com/hyperledger-labs/fabric-smart-client/platform/view/view" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -111,3 +117,131 @@ func TestNetworkRejectsMalformedTransactionID(t *testing.T) { err = n.AddFinalityListener("token", "not-a-valid-anchor", nil) require.Error(t, err) } + +// --- registerEndorser ------------------------------------------------------------------------------ + +// fakeViewRegistry records every RegisterResponder call, so a test can tell whether a second +// registration attempt actually reached FSC or was refused before getting there. +type fakeViewRegistry struct { + calls int +} + +func (f *fakeViewRegistry) RegisterResponder(view.View, any) error { + f.calls++ + + return nil +} + +// fakeViewManager satisfies endorsement.ViewManager without ever running a view; registerEndorser +// itself never calls InitiateView, only Service.Endorse does. +type fakeViewManager struct{} + +func (fakeViewManager) InitiateView(context.Context, view.View) (any, error) { return nil, nil } + +// fakeIdentityProvider resolves every name to an identity carrying the same bytes, so +// config.AllowedRequesters can turn the allowlist's names into identities the way d.resolveIdentity +// does in production. +type fakeIdentityProvider struct{} + +func (fakeIdentityProvider) Identity(name string) view.Identity { return view.Identity(name) } +func (fakeIdentityProvider) DefaultIdentity() view.Identity { return view.Identity("default") } + +var _ svcview.IdentityProvider = fakeIdentityProvider{} + +// endorserConfig returns a configuration for a node that endorses, backed by the well-known test key +// keystore_test.go already writes, whose address matches validConfig's single endorser entry. +func endorserConfig(t *testing.T) *Config { + t.Helper() + c := validConfig() + c.Endorser = EndorserConfig{ + Enabled: true, + Keystore: writeKey(t, testKeyHex), + Address: testKeyAddress, + } + c.Endorsement.Allowlist = []string{"endorser-1"} + + return c +} + +// testServiceFactory builds a real *endorsement.ServiceFactory over config, the same shape +// installEndorsement assembles, so registerEndorser is exercised with the collaborator it actually +// gets in production. +func testServiceFactory(t *testing.T, config *Config) *endorsement.ServiceFactory { + t.Helper() + registry, err := config.EndorserRegistry(func(name string) (view.Identity, error) { + return view.Identity(name), nil + }) + require.NoError(t, err) + tokenState, err := config.TokenStateAddress() + require.NoError(t, err) + evmClient := &mock.EVMClient{} + + factory, err := endorsement.NewServiceFactory(endorsement.FactoryConfig{ + Registry: registry, + Threshold: int(config.Endorsement.Threshold), + Domain: eip712.Domain{ChainID: config.ChainIDBig(), VerifyingContract: tokenState}, + Client: evmClient, + TokenState: tokenState, + BlockTag: config.Finality.BlockTag, + PublicParams: pp.NewChainProvider(evmClient, tokenState, config.Finality.BlockTag), + ViewManager: fakeViewManager{}, + TMS: func(token2.TMSID) (*token2.ManagementService, error) { + return nil, errors.New("not needed for this test") + }, + }) + require.NoError(t, err) + + return factory +} + +// TestRegisterEndorserRefusesASecondNetwork is the fix for a real bug. FSC registers a responder by +// the initiating view's Go type alone, so only one Responder can ever be registered for +// endorsement.Initiator across this node's whole process lifetime. Before this fix a sync.Once +// silently discarded every network after the first one that reached registerEndorser, with no signal +// at all that a second, differently configured network's endorsement requests would never be +// answered, or worse, would be answered through the wrong network's chain client and EIP-712 domain. +func TestRegisterEndorserRefusesASecondNetwork(t *testing.T) { + registry := &fakeViewRegistry{} + d := &Driver{viewRegistry: registry, identities: fakeIdentityProvider{}} + config := endorserConfig(t) + factory := testServiceFactory(t, config) + + d.registerEndorser("network-a:", factory, config) + assert.Equal(t, 1, registry.calls, "the first network must register") + assert.Equal(t, "network-a:", d.registeredFor) + + d.registerEndorser("network-b:", factory, config) + assert.Equal(t, 1, registry.calls, "a second, different network must not overwrite the registration") + assert.Equal(t, "network-a:", d.registeredFor, "the first network's registration must stand") +} + +// TestRegisterEndorserIsIdempotentForTheSameNetwork checks Driver.New being called twice for the same +// network (a network rebuilt over a node's life) does not trip the second-network refusal. +func TestRegisterEndorserIsIdempotentForTheSameNetwork(t *testing.T) { + registry := &fakeViewRegistry{} + d := &Driver{viewRegistry: registry, identities: fakeIdentityProvider{}} + config := endorserConfig(t) + factory := testServiceFactory(t, config) + + d.registerEndorser("network-a:", factory, config) + d.registerEndorser("network-a:", factory, config) + + assert.Equal(t, 1, registry.calls, "registering the same network twice must not re-register") +} + +// TestRegisterEndorserSkipsANonEndorsingNetwork checks a network that is not configured to endorse +// never touches the registration state, so it cannot trigger the second-network refusal for a network +// that never wanted to endorse in the first place. +func TestRegisterEndorserSkipsANonEndorsingNetwork(t *testing.T) { + registry := &fakeViewRegistry{} + d := &Driver{viewRegistry: registry, identities: fakeIdentityProvider{}} + endorsing := endorserConfig(t) + factory := testServiceFactory(t, endorsing) + d.registerEndorser("network-a:", factory, endorsing) + + notEndorsing := validConfig() // Endorser.Enabled defaults to false + d.registerEndorser("network-b:", factory, notEndorsing) + + assert.Equal(t, 1, registry.calls) + assert.Equal(t, "network-a:", d.registeredFor) +} From 505dc2636ac14e86c005c499c9bcefacd2be0377 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 01:18:15 +0530 Subject: [PATCH 09/35] fix(evm): catch an endorsing node with no allowlist at startup, not at registration Two comments, on the Allowlist field and on Authorizer itself, promised that an empty allowlist would default to "the TMS network's nodes, resolved at config load in Week 5". That resolution was never built. Authorizer.NewAuthorizer is deliberately fail-closed and rejects an empty allowlist outright, which is the right call for authorization, but nothing upstream of it ever supplied the promised default, so an operator who left Allowlist unset trusting the documented behavior got a node that came up looking healthy and silently never registered as an endorser, the failure logged as an error easy to miss during wiring rather than surfaced as the startup failure it should have been. Validate now rejects an endorser.enabled configuration with no allowlist, matching this file's own stated philosophy that a bad configuration should be a startup error, not a surprise later. Both comments are corrected to describe the actual, intentional fail-closed behavior instead of a fallback that does not exist. The integration harness is unaffected: it already builds the allowlist itself from every node in the TMS rather than relying on the driver to do it, which is what the documented default was supposed to be doing. Signed-off-by: atharrva01 --- x/token/services/network/evm/config.go | 13 +++++++++++-- x/token/services/network/evm/config_test.go | 3 +++ .../services/network/evm/endorsement/authorize.go | 5 +++-- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/x/token/services/network/evm/config.go b/x/token/services/network/evm/config.go index 1717690576..2953cf5f3f 100644 --- a/x/token/services/network/evm/config.go +++ b/x/token/services/network/evm/config.go @@ -131,8 +131,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"` @@ -278,6 +280,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 diff --git a/x/token/services/network/evm/config_test.go b/x/token/services/network/evm/config_test.go index adef13c138..5763113cd3 100644 --- a/x/token/services/network/evm/config_test.go +++ b/x/token/services/network/evm/config_test.go @@ -218,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) { diff --git a/x/token/services/network/evm/endorsement/authorize.go b/x/token/services/network/evm/endorsement/authorize.go index eed54fc869..7dfbf619c0 100644 --- a/x/token/services/network/evm/endorsement/authorize.go +++ b/x/token/services/network/evm/endorsement/authorize.go @@ -13,8 +13,9 @@ import ( // Authorizer decides whether an FSC identity may request endorsement. It is the EVM analog of the // Fabric responder's MSP/ACL creator check: EVM has no MSP, so authorization is membership in an -// allowlist of FSC identities configured per TMS (design §6.2, §15.7). The default policy (the TMS -// network's nodes) is resolved at config load in Week 5; here the allowlist is supplied explicitly. +// allowlist of FSC identities configured per TMS (design §6.2, §15.7). There is no automatic default; +// the allowlist is always supplied explicitly, and Config.Validate refuses to let an endorsing node +// start without one, rather than resolving an empty one to "the TMS network's nodes" here. // // It is fail-closed: an empty allowlist is rejected at construction, and an unknown or empty caller // is denied, so a misconfiguration cannot silently authorize everyone. From b59d821f66ee7581dc7228f0271df3b667e2867c Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 01:24:35 +0530 Subject: [PATCH 10/35] fix(evm): refuse to broadcast an envelope whose anchor and delta disagree Broadcast checked only that an envelope carried a delta at all, never that the envelope's own anchor and the anchor baked into that delta actually named the same transaction. Under the normal flow they always agree, since RequestApproval and SetupPublicParams both derive the envelope's anchor and the delta's anchor from the same value, but Broadcast has no way to know how the envelope it was actually handed was built. A mismatch here is not just a theoretical validation gap. The chain only ever looks at the delta's anchor: that is what applyStateDelta checks for replay, what the digest covers, what StateCommitted is emitted for. The local side tracks the transaction by the envelope's anchor instead, finality listeners and the ttx store are keyed on it. If the two ever diverged, the transaction would apply and commit on chain under one anchor while everything local kept waiting on a different one, and after the finality timeout wrongly report a transaction that actually succeeded as failed, the same failure shape HIGH #1 fixed, just reachable through a construction bug instead of a chain-read one. Broadcast now parses the envelope's anchor and compares it against the delta's before spending any gas, and refuses if they disagree. Signed-off-by: atharrva01 --- x/token/services/network/evm/network.go | 19 ++++++ x/token/services/network/evm/network_test.go | 68 ++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/x/token/services/network/evm/network.go b/x/token/services/network/evm/network.go index 98e365de66..7cd1442df3 100644 --- a/x/token/services/network/evm/network.go +++ b/x/token/services/network/evm/network.go @@ -248,6 +248,16 @@ func (n *Network) SetRecoveryStarter(f func(ns string) error) { n.startRecovery // Broadcast assembles the endorsed envelope into a signed transaction, sends it, and records the // resulting raw transaction and hash back into the envelope so the caller can follow its finality. +// +// It checks that the envelope's anchor and its delta's own anchor agree before spending any gas. +// Under the normal flow they always do, since both are derived from the same anchor at the point an +// Envelope is built (RequestApproval, SetupPublicParams), but Broadcast has no way to know how the +// envelope it was actually handed got here, and a mismatch here is not hypothetical: it would mean +// the local bookkeeping that tracks this transaction (finality listeners, the ttx store) is keyed on +// one anchor while the chain applies and emits StateCommitted for a different one. A finality wait on +// the anchor Broadcast was told about would then watch an anchor the chain never touches, and after +// the timeout wrongly report a transaction that actually succeeded as failed, the same failure shape +// as a persistent read error, just from a construction bug instead of a chain-connectivity one. func (n *Network) Broadcast(ctx context.Context, blob any) error { env, ok := blob.(*Envelope) if !ok { @@ -259,6 +269,15 @@ func (n *Network) Broadcast(ctx context.Context, blob any) error { if env.Delta == nil { return errors.Errorf("evm network: envelope [%s] carries no state delta", env.Anchor) } + anchor, err := keys.AnchorFromTxID(env.Anchor) + if err != nil { + return errors.Wrapf(err, "evm network: envelope carries an invalid anchor [%s]", env.Anchor) + } + if env.Delta.Anchor != anchor { + return errors.Errorf( + "evm network: envelope anchor [%s] does not match its delta's anchor [%x]; refusing to broadcast", + env.Anchor, env.Delta.Anchor) + } rawTx, txHash, err := n.submitter.Submit(ctx, env.Delta, env.Endorsements) if err != nil { diff --git a/x/token/services/network/evm/network_test.go b/x/token/services/network/evm/network_test.go index 95d1c38dae..8ad21f43b4 100644 --- a/x/token/services/network/evm/network_test.go +++ b/x/token/services/network/evm/network_test.go @@ -338,6 +338,74 @@ func TestBroadcastRejectsBadInput(t *testing.T) { }) } +// networkWithSubmitter returns a Network wired with a real submitter over a ready mock client, for +// Broadcast tests that need to get past the submitter/delta checks to exercise what follows them. +func networkWithSubmitter(t *testing.T, evm *mock.EVMClient) *Network { + t.Helper() + c := validConfig() + c.applyDefaults() + require.NoError(t, c.Validate()) + n, err := NewNetwork("evm-net", c, evm, nil, testSubmitter(t, evm, estimateGas()), nil) + require.NoError(t, err) + + return n +} + +// TestBroadcastRejectsMismatchedAnchor is the fix for a real gap: nothing checked that an envelope's +// anchor and its delta's own anchor actually agreed before spending gas on it. Under the normal flow +// they always do, since RequestApproval derives both from the same anchor, but Broadcast has no way +// to know how the envelope it was actually handed was built, and the local finality tracking is keyed +// on the envelope's anchor while the chain applies and emits StateCommitted for the delta's: a +// mismatch here would mean waiting on an anchor the chain never touches. +func TestBroadcastRejectsMismatchedAnchor(t *testing.T) { + evm := readySubmitterClient() + n := networkWithSubmitter(t, evm) + + elsewhere, err := keys.AnchorFromTxID(anchorHex(0xEE)) + require.NoError(t, err) + delta := testDelta() + delta.Anchor = elsewhere + + err = n.Broadcast(t.Context(), &Envelope{ + Anchor: anchorHex(0x01), + Delta: delta, + Endorsements: [][]byte{make([]byte, 65)}, + }) + require.Error(t, err) + assert.Zero(t, evm.SendRawTransactionCallCount(), "a mismatched envelope must not be broadcast") +} + +// TestBroadcastRejectsAnUnparsableAnchor checks the envelope's own anchor is validated before it is +// compared against anything, rather than producing a confusing failure further down. +func TestBroadcastRejectsAnUnparsableAnchor(t *testing.T) { + evm := readySubmitterClient() + n := networkWithSubmitter(t, evm) + + err := n.Broadcast(t.Context(), &Envelope{ + Anchor: "not-hex", + Delta: testDelta(), + Endorsements: [][]byte{make([]byte, 65)}, + }) + require.Error(t, err) + assert.Zero(t, evm.SendRawTransactionCallCount()) +} + +// TestBroadcastAcceptsAConsistentEnvelope is the positive case: an envelope whose anchor and delta +// agree, as every normal caller produces, must still broadcast. +func TestBroadcastAcceptsAConsistentEnvelope(t *testing.T) { + evm := readySubmitterClient() + n := networkWithSubmitter(t, evm) + + delta := testDelta() + err := n.Broadcast(t.Context(), &Envelope{ + Anchor: hex.EncodeToString(delta.Anchor[:]), + Delta: delta, + Endorsements: [][]byte{make([]byte, 65)}, + }) + require.NoError(t, err) + assert.Equal(t, 1, evm.SendRawTransactionCallCount()) +} + // --- queries ------------------------------------------------------------------------------------- func TestQueryTokens(t *testing.T) { From 01e2a0bd16696a3fbed3e898863a14363ee0bb89 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 14:24:41 +0530 Subject: [PATCH 11/35] docs(evm): correct the blockTag field comment to list all three accepted tags FinalityConfig.BlockTag's own comment said state is read at finalized or safe, but Validate has accepted latest as a third legal value since it was introduced for the local, instant-mining test harness. The comment now names all three and repeats, next to the field itself, what BlockTagLatest's own doc comment already says: it carries no reorg protection and is only appropriate for a local chain. No behavior changes; latest was already accepted before this commit. Signed-off-by: atharrva01 --- x/token/services/network/evm/config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/token/services/network/evm/config.go b/x/token/services/network/evm/config.go index 2953cf5f3f..df01748fdd 100644 --- a/x/token/services/network/evm/config.go +++ b/x/token/services/network/evm/config.go @@ -80,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"` From 5f2f9315ab221931947dd0e4bfc086419c525d45 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 14:51:38 +0530 Subject: [PATCH 12/35] test(evm): fuzz the anchor decoder AnchorFromTxID hex-decodes a token-request anchor, and every caller hands it attacker-influenced input before anything about it has been checked: an endorser decodes a request's own anchor before it has validated that request (endorsement/delta.go), a node decodes an input's TxId while translating a transfer action (statedelta/ translator.go), and a node decodes an envelope's anchor straight off the wire (network.go). AGENTS.md asks for a FuzzXxx on exactly this shape of function, and this one had none while its sibling parsers (eip712.RecoverAddress, eip712.NewSignerFromBytes, the envelope and ABI decoders) already do. The function itself already handles malformed input cleanly, hex decode errors and length checks both return proper errors rather than panicking, confirmed by a million-plus fuzz executions with no failures. This closes the coverage gap the rule asks for rather than fixing a live bug. Wired into nightly-fuzz.yml so it runs under extended -fuzztime, not just its seed corpus. Signed-off-by: atharrva01 --- .github/workflows/nightly-fuzz.yml | 4 +++ .../services/network/evm/keys/fuzz_test.go | 33 +++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 x/token/services/network/evm/keys/fuzz_test.go diff --git a/.github/workflows/nightly-fuzz.yml b/.github/workflows/nightly-fuzz.yml index b2cf043200..05e32c488c 100644 --- a/.github/workflows/nightly-fuzz.yml +++ b/.github/workflows/nightly-fuzz.yml @@ -120,6 +120,10 @@ jobs: dir: ./x/token/services/network/evm pkg: . func: FuzzComputeAnchor + - name: evm-anchor-from-txid + dir: ./x/token/services/network/evm + pkg: ./keys + func: FuzzAnchorFromTxID - name: evm-abi-decode-bytes dir: ./x/token/services/network/evm pkg: ./abi diff --git a/x/token/services/network/evm/keys/fuzz_test.go b/x/token/services/network/evm/keys/fuzz_test.go new file mode 100644 index 0000000000..3baa9f0b54 --- /dev/null +++ b/x/token/services/network/evm/keys/fuzz_test.go @@ -0,0 +1,33 @@ +/* +Copyright IBM Corp. All Rights Reserved. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package keys + +import ( + "encoding/hex" + "testing" +) + +// FuzzAnchorFromTxID fuzzes the anchor decoder. The string comes from a token request's TxId, which +// an endorser decodes before it has validated the request that carries it (endorsement/delta.go), and +// which a node also decodes straight off an envelope it received (network.go). Neither caller trusts +// its input yet, so the property is that decoding either fails cleanly or succeeds; it must never +// panic on malformed, truncated, or oversized hex. +func FuzzAnchorFromTxID(f *testing.F) { + a := anchor(0x11) + valid := hex.EncodeToString(a[:]) + + f.Add(valid) // a well-formed anchor + f.Add("") // empty + f.Add("zz") // non-hex + f.Add(valid[:AnchorLength]) // truncated, still valid hex + f.Add(valid + "ab") // over-long, still valid hex + f.Add("0x" + valid) // 0x-prefixed, not accepted by hex.DecodeString + + f.Fuzz(func(t *testing.T, txID string) { + _, _ = AnchorFromTxID(txID) + }) +} From 4406c8d2c6e640bfda34a7ed8c26edf256117e73 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 15:11:10 +0530 Subject: [PATCH 13/35] fix(evm): stop a null eth_getBlockByNumber result from reading as a zero base fee baseFee unmarshaled the node's response into a plain struct value, not a pointer. Unmarshaling a JSON null into a non-pointer target is a documented no-op in encoding/json, so a node returning result: null for eth_getBlockByNumber("latest") left the struct at its zero value, identical to the legitimate case the empty-string check exists for: a pre-London or zero-fee chain that simply has no baseFeePerGas field. Both were read as a real base fee of zero. That zero fed straight into SuggestGasFees's maxFee = baseFee*2 + tip, which every Submit call uses to price its transaction, so a transient or malformed response from the node silently produced an underpriced transaction instead of surfacing as an error. head is now a pointer, the same pattern GetTransactionReceipt and IsPending already use to tell a null result apart from a present one with an empty field. Signed-off-by: atharrva01 --- x/token/services/network/evm/client/jsonrpc.go | 10 ++++++++-- x/token/services/network/evm/client/jsonrpc_test.go | 13 +++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/x/token/services/network/evm/client/jsonrpc.go b/x/token/services/network/evm/client/jsonrpc.go index 9e67a2dcba..2e4dfcc5c7 100644 --- a/x/token/services/network/evm/client/jsonrpc.go +++ b/x/token/services/network/evm/client/jsonrpc.go @@ -218,14 +218,20 @@ func (c *JSONRPCClient) SuggestGasFees(ctx context.Context) (GasFees, error) { } // baseFee reads the base fee of the latest block. A chain with no base fee at all (a pre-London or -// zero-fee configuration) reports none, which is a base fee of zero rather than an error. +// zero-fee configuration) reports the field absent, which is a base fee of zero rather than an error. +// A null result, by contrast, means the node did not return a block at all and is an error: head is a +// pointer so that case is distinguishable, since unmarshaling JSON null into a non-pointer target is a +// silent no-op that would otherwise be indistinguishable from the legitimate absent-field case. func (c *JSONRPCClient) baseFee(ctx context.Context) (*big.Int, error) { - var head struct { + var head *struct { BaseFeePerGas string `json:"baseFeePerGas"` } if err := c.call(ctx, "eth_getBlockByNumber", &head, "latest", false); err != nil { return nil, err } + if head == nil { + return nil, errors.New("evm client: eth_getBlockByNumber(\"latest\") returned no block") + } if head.BaseFeePerGas == "" { return new(big.Int), nil } diff --git a/x/token/services/network/evm/client/jsonrpc_test.go b/x/token/services/network/evm/client/jsonrpc_test.go index bf8965e593..b0afd48b53 100644 --- a/x/token/services/network/evm/client/jsonrpc_test.go +++ b/x/token/services/network/evm/client/jsonrpc_test.go @@ -273,6 +273,19 @@ func TestSuggestGasFeesOnAZeroFeeChain(t *testing.T) { assert.Equal(t, big.NewInt(0), fees.MaxFeePerGas) } +// TestSuggestGasFeesErrorsOnANullBlock covers a node that returns no block at all for "latest" (a +// gateway hiccup or a malformed response), which must not be treated as the pre-London zero-base-fee +// case: both look like an empty BaseFeePerGas field unless the two are told apart. +func TestSuggestGasFeesErrorsOnANullBlock(t *testing.T) { + c, _ := newTestServer(t, map[string]string{ + "eth_maxPriorityFeePerGas": `"0x1"`, + "eth_getBlockByNumber": `null`, + }) + + _, err := c.SuggestGasFees(context.Background()) + require.Error(t, err) +} + // TestSuggestGasFeesSurfacesRealFailures checks the fallback is only for an unimplemented method: a // node that implements neither is a failure rather than a silent zero fee. func TestSuggestGasFeesSurfacesRealFailures(t *testing.T) { From f5c111e9fc54ec9ca50ff26077c3096d6b1747b5 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 15:13:12 +0530 Subject: [PATCH 14/35] fix(evm): classify eth_call reverts the same way eth_estimateGas already does Call went through the generic call() wrapper and returned every JSON-RPC error the same way, while EstimateGas explicitly classifies a revert as ErrExecutionReverted so a caller can tell a permanent rejection from a node that simply failed to answer. Both eth_call and eth_estimateGas can revert against a real node; nothing in the EVMClient interface said only one of them would. No caller is affected today: every method Call is currently used for (getToken, getPublicParameters, getTransferMetadata, getTokenRequestHash, getPublicParamsVersion) is a plain storage read with no revert condition in the Solidity source. This closes the gap in the interface itself before the first Call against a method that can revert has to rediscover it. Signed-off-by: atharrva01 --- .../services/network/evm/client/jsonrpc.go | 15 +++++++++-- .../network/evm/client/jsonrpc_test.go | 26 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/x/token/services/network/evm/client/jsonrpc.go b/x/token/services/network/evm/client/jsonrpc.go index 2e4dfcc5c7..d70e30a95d 100644 --- a/x/token/services/network/evm/client/jsonrpc.go +++ b/x/token/services/network/evm/client/jsonrpc.go @@ -72,14 +72,25 @@ func (c *JSONRPCClient) Ping(ctx context.Context) error { return err } -// Call performs a read-only contract call at the given block tag. +// Call performs a read-only contract call at the given block tag. A revert is classified the same way +// EstimateGas classifies one (ErrExecutionReverted), so a caller that later adds a Call against a +// method capable of reverting does not have to rediscover this distinction: eth_call and +// eth_estimateGas fail the same way against the same node. func (c *JSONRPCClient) Call(ctx context.Context, to Address, data []byte, blockTag string) ([]byte, error) { if blockTag == "" { blockTag = BlockTagFinalized } arg := map[string]any{"to": to.Hex(), "data": encodeHexBytes(data)} var out string - if err := c.call(ctx, "eth_call", &out, arg, blockTag); err != nil { + rpcErr, err := c.invoke(ctx, "eth_call", &out, arg, blockTag) + if rpcErr != nil { + if isReverted(rpcErr) { + return nil, errors.Wrapf(ErrExecutionReverted, "eth_call failed: %s", rpcErr.Message) + } + + return nil, errors.Wrap(rpcErr, "eth_call failed") + } + if err != nil { return nil, err } diff --git a/x/token/services/network/evm/client/jsonrpc_test.go b/x/token/services/network/evm/client/jsonrpc_test.go index b0afd48b53..ccc75f0489 100644 --- a/x/token/services/network/evm/client/jsonrpc_test.go +++ b/x/token/services/network/evm/client/jsonrpc_test.go @@ -120,6 +120,32 @@ func TestCallSurfacesRPCError(t *testing.T) { assert.Contains(t, err.Error(), "method not found") } +// TestCallClassifiesReverts mirrors TestEstimateGasClassifiesReverts: eth_call can revert against a +// real node exactly as eth_estimateGas can, and a caller needs the same distinction between "the chain +// rejected this" and "the node failed to answer" for either one. +func TestCallClassifiesReverts(t *testing.T) { + for _, tc := range []struct { + name string + code int + message string + reverted bool + }{ + {name: "geth wording", code: -32000, message: "execution reverted", reverted: true}, + {name: "besu wording", code: -32000, message: "Execution reverted", reverted: true}, + {name: "another server error", code: -32000, message: "header not found", reverted: false}, + {name: "method not found", code: -32601, message: "method not found", reverted: false}, + } { + t.Run(tc.name, func(t *testing.T) { + c := newErrorServer(t, tc.code, tc.message) + + _, err := c.Call(context.Background(), Address{}, nil, "latest") + require.Error(t, err) + assert.Equal(t, tc.reverted, errors.Is(err, ErrExecutionReverted)) + assert.Contains(t, err.Error(), tc.message) + }) + } +} + func TestPendingNonceAt(t *testing.T) { c, calls := newTestServer(t, map[string]string{"eth_getTransactionCount": `"0x2a"`}) From 360892a3d35aa01cb6f8b9dc9346835701bb4ea4 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 15:16:05 +0530 Subject: [PATCH 15/35] fix(evm): parse an uppercase 0X hex prefix the same way everywhere decodeHex (types.go, used by HexToAddress/HexToHash) tolerated both 0x and 0X, but decodeHexBytes, parseHexUint and parseHexBig in jsonrpc.go, which decode every JSON-RPC response quantity and data field, only stripped a lowercase 0x. The two paths disagreed on what counts as a hex prefix for the same syntax in the same package. Every real node emits lowercase 0x, so this never produced a wrong value, only an avoidable inconsistency; a 0X-prefixed response failed with a parse error rather than being misread. Extracted the prefix stripping decodeHex already had into a shared trimHexPrefix and pointed all four parsers at it, so there is one rule instead of two copies that can drift. Signed-off-by: atharrva01 --- x/token/services/network/evm/client/jsonrpc.go | 12 ++++++------ .../services/network/evm/client/jsonrpc_test.go | 14 ++++++++++++++ x/token/services/network/evm/client/types.go | 14 ++++++++++---- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/x/token/services/network/evm/client/jsonrpc.go b/x/token/services/network/evm/client/jsonrpc.go index d70e30a95d..77892ba40a 100644 --- a/x/token/services/network/evm/client/jsonrpc.go +++ b/x/token/services/network/evm/client/jsonrpc.go @@ -517,9 +517,9 @@ func encodeHexBytes(b []byte) string { return "0x" + hex.EncodeToString(b) } -// decodeHexBytes decodes 0x-prefixed hex data, tolerating an empty or bare "0x" value. +// decodeHexBytes decodes 0x/0X-prefixed hex data, tolerating an empty or bare prefix value. func decodeHexBytes(s string) ([]byte, error) { - s = strings.TrimPrefix(strings.TrimSpace(s), "0x") + s = trimHexPrefix(s) if s == "" { return nil, nil } @@ -531,9 +531,9 @@ func decodeHexBytes(s string) ([]byte, error) { return b, nil } -// parseHexUint parses a 0x-prefixed hex quantity into a uint64. +// parseHexUint parses a 0x/0X-prefixed hex quantity into a uint64. func parseHexUint(s string) (uint64, error) { - trimmed := strings.TrimPrefix(strings.TrimSpace(s), "0x") + trimmed := trimHexPrefix(s) if trimmed == "" { return 0, errors.Errorf("empty hex quantity") } @@ -545,9 +545,9 @@ func parseHexUint(s string) (uint64, error) { return v, nil } -// parseHexBig parses a 0x-prefixed hex quantity into a big.Int. +// parseHexBig parses a 0x/0X-prefixed hex quantity into a big.Int. func parseHexBig(s string) (*big.Int, error) { - trimmed := strings.TrimPrefix(strings.TrimSpace(s), "0x") + trimmed := trimHexPrefix(s) if trimmed == "" { return nil, errors.Errorf("empty hex quantity") } diff --git a/x/token/services/network/evm/client/jsonrpc_test.go b/x/token/services/network/evm/client/jsonrpc_test.go index ccc75f0489..f787bd4732 100644 --- a/x/token/services/network/evm/client/jsonrpc_test.go +++ b/x/token/services/network/evm/client/jsonrpc_test.go @@ -501,4 +501,18 @@ func TestHexHelpers(t *testing.T) { _, err = decodeHexBytes("0xodd") require.Error(t, err) }) + + t.Run("an uppercase 0X prefix is tolerated exactly like decodeHex accepts one", func(t *testing.T) { + got, err := parseHexUint("0X2a") + require.NoError(t, err) + assert.Equal(t, uint64(42), got) + + gotBig, err := parseHexBig("0X2a") + require.NoError(t, err) + assert.Zero(t, gotBig.Cmp(big.NewInt(42))) + + gotBytes, err := decodeHexBytes("0Xdead") + require.NoError(t, err) + assert.Equal(t, []byte{0xde, 0xad}, gotBytes) + }) } diff --git a/x/token/services/network/evm/client/types.go b/x/token/services/network/evm/client/types.go index 4e36b0d3fc..19417d6047 100644 --- a/x/token/services/network/evm/client/types.go +++ b/x/token/services/network/evm/client/types.go @@ -167,11 +167,17 @@ func (h *Hash) UnmarshalJSON(data []byte) error { return h.UnmarshalText([]byte(s)) } -// decodeHex decodes a hex string, tolerating an optional 0x/0X prefix and surrounding whitespace. -func decodeHex(s string) ([]byte, error) { +// trimHexPrefix strips surrounding whitespace and an optional 0x/0X prefix. It is the one place that +// decides what counts as a prefix, shared by every hex parser in this package so a JSON-RPC quantity +// and an address/hash literal are never held to different rules for the same syntax. +func trimHexPrefix(s string) string { s = strings.TrimSpace(s) s = strings.TrimPrefix(s, "0x") - s = strings.TrimPrefix(s, "0X") - return hex.DecodeString(s) + return strings.TrimPrefix(s, "0X") +} + +// decodeHex decodes a hex string, tolerating an optional 0x/0X prefix and surrounding whitespace. +func decodeHex(s string) ([]byte, error) { + return hex.DecodeString(trimHexPrefix(s)) } From bcb57efe49ac88df1772e7d7298dbd71e029e64f Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 15:19:27 +0530 Subject: [PATCH 16/35] test(evm): fuzz the eth_getLogs and eth_getTransactionReceipt decoders jsonLog.toLog and jsonReceipt.toReceipt turn a response body from whatever node the driver is pointed at into driver types, and are called from GetLogs (log scanning for anchor resolution) and GetTransactionReceipt (finality). Every sub-parser they call (HexToAddress, HexToHash, decodeHexBytes, parseHexUint) already fails safely and most are already fuzzed individually, but the assembly functions themselves had no direct coverage, unlike the ABI decoders and the envelope wire decoder, which do. No panic found in either fuzz target. Wired both into nightly-fuzz.yml so they run under extended -fuzztime rather than only their seed corpus. Signed-off-by: atharrva01 --- .github/workflows/nightly-fuzz.yml | 8 ++++ .../services/network/evm/client/fuzz_test.go | 46 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/.github/workflows/nightly-fuzz.yml b/.github/workflows/nightly-fuzz.yml index 05e32c488c..90e09ec885 100644 --- a/.github/workflows/nightly-fuzz.yml +++ b/.github/workflows/nightly-fuzz.yml @@ -152,6 +152,14 @@ jobs: dir: ./x/token/services/network/evm pkg: ./client func: FuzzBytesToAddress + - name: evm-json-log-to-log + dir: ./x/token/services/network/evm + pkg: ./client + func: FuzzJSONLogToLog + - name: evm-json-receipt-to-receipt + dir: ./x/token/services/network/evm + pkg: ./client + func: FuzzJSONReceiptToReceipt steps: - name: Checkout code diff --git a/x/token/services/network/evm/client/fuzz_test.go b/x/token/services/network/evm/client/fuzz_test.go index 790d109f41..81faa27b7b 100644 --- a/x/token/services/network/evm/client/fuzz_test.go +++ b/x/token/services/network/evm/client/fuzz_test.go @@ -8,6 +8,7 @@ package client import ( "bytes" + "encoding/json" "strings" "testing" ) @@ -93,3 +94,48 @@ func FuzzBytesToAddress(f *testing.F) { } }) } + +// FuzzJSONLogToLog fuzzes the eth_getLogs record decoder. It is the point where a response body from +// whatever node the driver was pointed at (a compromised, buggy, or simply mismatched one, per the +// design's own framing of alternative EVM backends) turns into a driver type, so the property is that +// a malformed record is rejected rather than panicking the caller. +func FuzzJSONLogToLog(f *testing.F) { + f.Add( + `{"address":"0x5FbDB2315678afecb367f032d93F642f64180aa3",` + + `"topics":["0x853f272fffc6efc284fc16a254decca742d2347e05703e501c59968f78f81ffa"],"data":"0xabcd",` + + `"transactionHash":"0x853f272fffc6efc284fc16a254decca742d2347e05703e501c59968f78f81ffa","blockNumber":"0x1"}`, + ) + f.Add(`{}`) + f.Add(`{"topics":[""]}`) + f.Add(`{"topics":["not hex"]}`) + f.Add(`{"data":"not hex"}`) + f.Add(`{"blockNumber":"not hex"}`) + f.Add(`not json at all`) + + f.Fuzz(func(t *testing.T, raw string) { + var j jsonLog + if err := json.Unmarshal([]byte(raw), &j); err != nil { + return + } + _, _ = j.toLog() + }) +} + +// FuzzJSONReceiptToReceipt fuzzes the eth_getTransactionReceipt record decoder, the same node-supplied +// boundary as FuzzJSONLogToLog, including the nested logs it decodes through jsonLog.toLog. +func FuzzJSONReceiptToReceipt(f *testing.F) { + f.Add(`{"transactionHash":"0x853f272fffc6efc284fc16a254decca742d2347e05703e501c59968f78f81ffa","blockNumber":"0x1","status":"0x1","logs":[]}`) + f.Add(`{}`) + f.Add(`{"status":"not hex"}`) + f.Add(`{"blockNumber":null,"status":"0x0"}`) + f.Add(`{"logs":[{"data":"not hex"}]}`) + f.Add(`not json at all`) + + f.Fuzz(func(t *testing.T, raw string) { + var j jsonReceipt + if err := json.Unmarshal([]byte(raw), &j); err != nil { + return + } + _, _ = j.toReceipt() + }) +} From d3c0c0e36e79969d52a3c402e2dff0546f625515 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 15:37:04 +0530 Subject: [PATCH 17/35] docs(evm): correct the endorsement allowlist's per-TMS claim Authorizer's doc comment said the allowlist is configured per TMS. Traced end to end, it isn't: configNetworkResolver.ConfigFor loads the EVM Config of whichever TMS declares the network first and that single Config, allowlist included, is what every TMS on the network gets checked against (Responder.factoryFor resolves a factory per TMS, but Authorize runs before that, off the one Authorizer the node was built with). The connection fields in Config really are shared across a network's TMS; the endorsement policy fields ride along with them by accident of being in the same struct, not by design. Comment now says what the code actually does and names the reason, so the gap between two TMS wanting different requester sets is a known, documented limitation instead of a silent surprise for the next person who reads only this file. Signed-off-by: atharrva01 --- .../services/network/evm/endorsement/authorize.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/x/token/services/network/evm/endorsement/authorize.go b/x/token/services/network/evm/endorsement/authorize.go index 7dfbf619c0..56d42cbb3f 100644 --- a/x/token/services/network/evm/endorsement/authorize.go +++ b/x/token/services/network/evm/endorsement/authorize.go @@ -13,9 +13,18 @@ import ( // Authorizer decides whether an FSC identity may request endorsement. It is the EVM analog of the // Fabric responder's MSP/ACL creator check: EVM has no MSP, so authorization is membership in an -// allowlist of FSC identities configured per TMS (design §6.2, §15.7). There is no automatic default; -// the allowlist is always supplied explicitly, and Config.Validate refuses to let an endorsing node -// start without one, rather than resolving an empty one to "the TMS network's nodes" here. +// allowlist of FSC identities (design §6.2, §15.7). There is no automatic default; the allowlist is +// always supplied explicitly, and Config.Validate refuses to let an endorsing node start without one, +// rather than resolving an empty one to "the TMS network's nodes" here. +// +// The design describes this allowlist as configured per TMS, but in the current wiring it is really +// per node: configNetworkResolver.ConfigFor loads the EVM configuration of whichever TMS declares it +// first for a network/channel, on the reasoning that connection fields (endpoint, chain id) are +// genuinely shared - and Endorsement.Allowlist rides along as part of that same Config, so every TMS a +// Responder resolves a factory for (Responder.factoryFor) is checked against the first TMS's list, not +// its own. Two TMS on one network cannot be given different requester sets today; giving Authorizer +// its own per-TMS lookup would need Config's connection fields and its endorsement policy fields +// (allowlist, threshold, endorser settings) to stop being loaded as one struct. // // It is fail-closed: an empty allowlist is rejected at construction, and an unknown or empty caller // is denied, so a misconfiguration cannot silently authorize everyone. From 9e07e60d5d4002c96f495eb12ab6720d4ced3a97 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 15:59:48 +0530 Subject: [PATCH 18/35] fix(evm): revert isSpent/areTokensSpent on a graph-hiding clone instead of lying Both functions resolve spent status through tokenMarker[tokenID], the content-bound marker recorded at output creation. A graph-hiding driver never populates that marker: translator.go leaves OutputToken .SNMarker at its zero value for graph-hiding outputs, since that mode spends by serial number instead, and graph-hiding spends only ever write serialUsed, never snSpent. So on a graph-hiding clone tokenMarker[tokenID] is always 0x0, snSpent[0x0] is never set, and isSpent/areTokensSpent always answer false, including for a token that was in fact spent via serialUsed. On-chain enforcement in applyStateDelta was never affected: it branches on graphHiding directly and checks serialUsed, not these query functions. But any caller of the public ABI (a wallet, an explorer, a monitoring tool) that asks isSpent against a graph-hiding TMS got a plausible-looking wrong answer instead of an error. Both functions now revert with UnsupportedForGraphHiding on that clone; isSerialUsed is the correct query for it and already existed. Signed-off-by: atharrva01 --- .../network/evm/contracts/src/TokenState.sol | 8 ++++++++ .../network/evm/contracts/test/TokenState.t.sol | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/x/token/services/network/evm/contracts/src/TokenState.sol b/x/token/services/network/evm/contracts/src/TokenState.sol index 9c448a821a..822ef1d998 100644 --- a/x/token/services/network/evm/contracts/src/TokenState.sol +++ b/x/token/services/network/evm/contracts/src/TokenState.sol @@ -71,6 +71,7 @@ contract TokenState { error MetadataKeyOccupied(bytes32 key); error MalformedSetupDelta(); error MalformedTransferDelta(); + error UnsupportedForGraphHiding(); event StateCommitted(bytes32 indexed anchor, bool success, string message); event PublicParametersUpdated(bytes32 indexed paramsHash, uint64 version); @@ -192,12 +193,19 @@ contract TokenState { } /// @notice Graph-revealing spent status by token id, resolved through the content-bound marker. + /// @dev Reverts on a graph-hiding clone: that mode never records a marker (translator.go leaves + /// SNMarker zero for it), so tokenMarker[tokenID] would always resolve to snSpent[0x0], + /// which is never set here - a caller would silently get "not spent" for a token that was + /// in fact spent via serialUsed. Use isSerialUsed for a graph-hiding clone instead. function isSpent(bytes32 tokenID) external view returns (bool) { + if (graphHiding) revert UnsupportedForGraphHiding(); return snSpent[tokenMarker[tokenID]]; } /// @notice Graph-revealing spent status for a batch of token ids, aligned with the input. + /// @dev Reverts on a graph-hiding clone, for the same reason as isSpent. function areTokensSpent(bytes32[] calldata tokenIDs) external view returns (bool[] memory out) { + if (graphHiding) revert UnsupportedForGraphHiding(); out = new bool[](tokenIDs.length); for (uint256 i = 0; i < tokenIDs.length; i++) { out[i] = snSpent[tokenMarker[tokenIDs[i]]]; diff --git a/x/token/services/network/evm/contracts/test/TokenState.t.sol b/x/token/services/network/evm/contracts/test/TokenState.t.sol index d0bbc85a8b..a11d681d8a 100644 --- a/x/token/services/network/evm/contracts/test/TokenState.t.sol +++ b/x/token/services/network/evm/contracts/test/TokenState.t.sol @@ -344,6 +344,20 @@ contract TokenStateTest is Test { gh.applyStateDelta(d2, _signFor(gh, d2)); } + function test_GraphHiding_IsSpentQueriesRevert() public { + TokenState gh = TokenState(Clones.clone(address(impl))); + gh.initialize(address(verifier), address(this), pp0, true); + + bytes32[] memory ids = new bytes32[](1); + ids[0] = keccak256("whatever"); + + vm.expectRevert(TokenState.UnsupportedForGraphHiding.selector); + gh.isSpent(ids[0]); + + vm.expectRevert(TokenState.UnsupportedForGraphHiding.selector); + gh.areTokensSpent(ids); + } + // --- lifecycle guards -------------------------------------------------------------------------- function test_DoubleInitialize_Reverts() public { From edcd66219bd83ec1c32412519375914cc1f3e967 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:22:35 +0530 Subject: [PATCH 19/35] fix(evm): make the eth_getLogs/eth_getTransactionReceipt fuzz properties actually check something FuzzJSONLogToLog and FuzzJSONReceiptToReceipt, added earlier this session in bcb57efe4 right after two real bugs in this same decoding file, claimed in their own doc comment that malformed input is rejected rather than panicking the caller, but the fuzz bodies discarded the result entirely (_, _ = j.toLog()). That only proves no panic; it gives no signal at all if a future change drops one of the inner error checks and starts returning a zero-valued field instead of an error. Confirmed the gap was real before fixing it: temporarily dropped the error check on HexToAddress inside toLog and reran the fuzzer, which took under 15 seconds to find the exact class of bug the finding described, undetected by the old property. Reverted the injection immediately after confirming it. Both properties now re-derive every field of a successful decode directly from the raw JSON strings via the same already-fuzzed sub-parsers (HexToAddress, HexToHash, decodeHexBytes, parseHexUint) and require an exact match, through a shared assertLogMatchesRaw helper. This checks that toLog/toReceipt actually wire each field through its parser and propagate that parser's error, without reimplementing hex parsing itself. Signed-off-by: atharrva01 --- .../services/network/evm/client/fuzz_test.go | 70 ++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/x/token/services/network/evm/client/fuzz_test.go b/x/token/services/network/evm/client/fuzz_test.go index 81faa27b7b..4099e8d441 100644 --- a/x/token/services/network/evm/client/fuzz_test.go +++ b/x/token/services/network/evm/client/fuzz_test.go @@ -117,10 +117,49 @@ func FuzzJSONLogToLog(f *testing.F) { if err := json.Unmarshal([]byte(raw), &j); err != nil { return } - _, _ = j.toLog() + log, err := j.toLog() + if err != nil { + return + } + assertLogMatchesRaw(t, log, j) }) } +// assertLogMatchesRaw re-derives every field of log directly from j's raw strings and requires an +// exact match. toLog is glue over the already-fuzzed HexToAddress/HexToHash/decodeHexBytes/ +// parseHexUint, so this does not re-implement hex parsing; it proves toLog actually wires each field +// through its parser and propagates that parser's error, rather than, say, silently substituting a +// zero value for a field it failed to parse. +func assertLogMatchesRaw(t *testing.T, log Log, j jsonLog) { + t.Helper() + + addr, err := HexToAddress(j.Address) + if err != nil || log.Address != addr { + t.Fatalf("toLog succeeded but Address does not match parsing j.Address directly (err=%v)", err) + } + txHash, err := HexToHash(j.TxHash) + if err != nil || log.TxHash != txHash { + t.Fatalf("toLog succeeded but TxHash does not match parsing j.TxHash directly (err=%v)", err) + } + data, err := decodeHexBytes(j.Data) + if err != nil || !bytes.Equal(log.Data, data) { + t.Fatalf("toLog succeeded but Data does not match parsing j.Data directly (err=%v)", err) + } + blockNumber, err := parseHexUint(j.BlockNumber) + if err != nil || log.BlockNumber != blockNumber { + t.Fatalf("toLog succeeded but BlockNumber does not match parsing j.BlockNumber directly (err=%v)", err) + } + if len(log.Topics) != len(j.Topics) { + t.Fatalf("toLog succeeded but topic count changed: got %d, raw had %d", len(log.Topics), len(j.Topics)) + } + for i, raw := range j.Topics { + h, err := HexToHash(raw) + if err != nil || log.Topics[i] != h { + t.Fatalf("toLog succeeded but topic %d does not match parsing it directly (err=%v)", i, err) + } + } +} + // FuzzJSONReceiptToReceipt fuzzes the eth_getTransactionReceipt record decoder, the same node-supplied // boundary as FuzzJSONLogToLog, including the nested logs it decodes through jsonLog.toLog. func FuzzJSONReceiptToReceipt(f *testing.F) { @@ -136,6 +175,33 @@ func FuzzJSONReceiptToReceipt(f *testing.F) { if err := json.Unmarshal([]byte(raw), &j); err != nil { return } - _, _ = j.toReceipt() + receipt, err := j.toReceipt() + if err != nil { + return + } + + txHash, err := HexToHash(j.TxHash) + if err != nil || receipt.TxHash != txHash { + t.Fatalf("toReceipt succeeded but TxHash does not match parsing j.TxHash directly (err=%v)", err) + } + status, err := parseHexUint(j.Status) + if err != nil || receipt.Status != status { + t.Fatalf("toReceipt succeeded but Status does not match parsing j.Status directly (err=%v)", err) + } + if (j.BlockNumber == nil) != (receipt.BlockNumber == nil) { + t.Fatalf("toReceipt succeeded but BlockNumber presence does not match the raw field") + } + if j.BlockNumber != nil { + bn, err := parseHexUint(*j.BlockNumber) + if err != nil || *receipt.BlockNumber != bn { + t.Fatalf("toReceipt succeeded but BlockNumber does not match parsing it directly (err=%v)", err) + } + } + if len(receipt.Logs) != len(j.Logs) { + t.Fatalf("toReceipt succeeded but log count changed: got %d, raw had %d", len(receipt.Logs), len(j.Logs)) + } + for i := range j.Logs { + assertLogMatchesRaw(t, receipt.Logs[i], j.Logs[i]) + } }) } From 411d3d6b513a49b2a383f64f626a0dbe0e8b8642 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:22:41 +0530 Subject: [PATCH 20/35] fix(evm): make FuzzDecodeUint64 check the overflow property it claims to The doc comment said the interesting case is a value that does not fit in 64 bits, which must be rejected rather than silently truncated, but the fuzz body discarded the result (_, _ = DecodeUint64(ret)), unlike its siblings FuzzDecodeBytes and FuzzDecodeBoolArray in the same file, which do check a real post-condition. A regression narrowing DecodeUint64's high-byte check (an off-by-one on the loop bound, for instance) would pass this fuzz target undetected. On a successful decode, the property now independently re-checks that every byte outside the low 8 is zero (via bytes.Equal against a zero-filled slice, not the same byte-loop DecodeUint64 uses) and that the decoded value matches reading the low 8 bytes directly. Signed-off-by: atharrva01 --- x/token/services/network/evm/abi/fuzz_test.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/x/token/services/network/evm/abi/fuzz_test.go b/x/token/services/network/evm/abi/fuzz_test.go index a05b5e0201..fc7e97d37c 100644 --- a/x/token/services/network/evm/abi/fuzz_test.go +++ b/x/token/services/network/evm/abi/fuzz_test.go @@ -102,7 +102,21 @@ func FuzzDecodeUint64(f *testing.F) { f.Add(make([]byte, wordLength*2)) f.Fuzz(func(t *testing.T, ret []byte) { - _, _ = DecodeUint64(ret) + got, err := DecodeUint64(ret) + if err != nil { + return + } + if len(ret) < wordLength { + t.Fatalf("decoded a uint64 from a %d byte response, shorter than one word", len(ret)) + } + // The value must actually fit: every byte outside the low 8 has to be zero, checked here by an + // independent comparison rather than the same byte-loop DecodeUint64 itself uses. + if !bytes.Equal(ret[:wordLength-8], make([]byte, wordLength-8)) { + t.Fatalf("accepted a value whose high bytes are not zero: it does not fit in a uint64") + } + if want := binary.BigEndian.Uint64(ret[wordLength-8 : wordLength]); got != want { + t.Fatalf("decoded %d, but the low 8 bytes actually encode %d", got, want) + } }) } From b9d2dba9a5b1d95dfb9109bb83c957c2b6a1df90 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:22:51 +0530 Subject: [PATCH 21/35] fix(evm): drop a tautological check from FuzzRecoverAddress The fuzz body re-called checkSignatureFormat on a signature RecoverAddress had already accepted. RecoverAddress calls that exact function itself first and returns before recovery on any format failure, so by the time the fuzz body reached this line the answer was already guaranteed nil - a pure function asked the same question of the same input a second time, which can never fail and verified nothing. The doc comment overclaimed too: it read as if this target proves Go's format rules match the EndorsementVerifier contract's, but that cross -check happens elsewhere (TestRecoverRejectsMalformed, EndorsementVerifier.t.sol's format-rejection cases, and the Go<->Solidity golden fixture in TestGoldenFixtureEndorsement / GoEndorsement.t.sol). Removed the dead check and rewrote the comment to describe what this target actually verifies: no panic, and any accepted signature recovers a real, non-zero address. Signed-off-by: atharrva01 --- .../services/network/evm/eip712/fuzz_test.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/x/token/services/network/evm/eip712/fuzz_test.go b/x/token/services/network/evm/eip712/fuzz_test.go index bf369111d5..a790fb35db 100644 --- a/x/token/services/network/evm/eip712/fuzz_test.go +++ b/x/token/services/network/evm/eip712/fuzz_test.go @@ -18,10 +18,14 @@ import ( // endorser over an FSC session, and the initiator recovers a signer from it before it has any reason // to trust the peer. A panic here is reachable by any node that can open a session. // -// The property is that RecoverAddress either returns an address or an error, never panics, and never -// accepts a signature the EndorsementVerifier contract would reject. The second half matters as much -// as the first: a signature accepted here but rejected on chain would let an initiator assemble a -// quorum that cannot be applied. +// The property this fuzz target checks is narrower than "never accepts a signature the contract would +// reject": RecoverAddress calls checkSignatureFormat itself and returns before recovery on failure, so +// re-checking format here on an accepted signature would only be asking the same pure function the +// same question twice and could never fail. What this actually verifies is that RecoverAddress never +// panics and, whenever it does accept a signature, recovers a real, non-zero address from it. That Go's +// format rules match the EndorsementVerifier contract's is a real property, proven independently by +// TestRecoverRejectsMalformed (signer_test.go) and the on-chain EndorsementVerifier.t.sol cases, plus +// the golden-fixture cross-check in TestGoldenFixtureEndorsement / GoEndorsement.t.sol. func FuzzRecoverAddress(f *testing.F) { signer, err := NewSignerFromBytes(testScalar(1)) if err != nil { @@ -49,12 +53,6 @@ func FuzzRecoverAddress(f *testing.F) { if err != nil { return } - - // Anything accepted must have passed the contract's own format rules, since the two have to - // agree for an endorsement to be applyable. - if err := checkSignatureFormat(sig); err != nil { - t.Fatalf("accepted a signature the contract would reject: %v", err) - } if address == (client.Address{}) { t.Fatal("recovered the zero address without an error") } From 9ccd89a8d3ee29616aa77c1a22c16b691b046cc6 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:40:02 +0530 Subject: [PATCH 22/35] fix(evm): reject a setup delta carrying metadata, matching the contract Validate's IsSetup branch checked SpentRefs and Outputs are empty and SetupParameters is present, but never checked MetadataKeys/ MetadataVals. TokenState.sol's _applySetup reverts MalformedSetupDelta on non-empty metadataKeys too, so this was an asymmetry between what Go refuses to sign and what the contract refuses to apply. Translator itself never reaches this path (writeSetup already blocks mixing setup with prior metadata), but Validate is also the safety net for a StateDelta assembled directly rather than through Translator - nwo/setup.go builds one by hand, and only happens not to trigger this today because it leaves metadata nil. Validate's own doc comment says it exists so endorsers fail fast rather than sign a malformed delta; this closes the one field it was missing. Signed-off-by: atharrva01 --- x/token/services/network/evm/statedelta/types.go | 4 ++-- .../services/network/evm/statedelta/types_test.go | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/x/token/services/network/evm/statedelta/types.go b/x/token/services/network/evm/statedelta/types.go index ca192a0a40..cba4565a9a 100644 --- a/x/token/services/network/evm/statedelta/types.go +++ b/x/token/services/network/evm/statedelta/types.go @@ -86,8 +86,8 @@ func (d *StateDelta) Validate() error { } } if d.IsSetup { - if len(d.SpentRefs) != 0 || len(d.Outputs) != 0 { - return errors.Errorf("setup delta must carry no spent refs or outputs") + if len(d.SpentRefs) != 0 || len(d.Outputs) != 0 || len(d.MetadataKeys) != 0 { + return errors.Errorf("setup delta must carry no spent refs, outputs, or metadata") } if len(d.SetupParameters) == 0 { return errors.Errorf("setup delta must carry the new public parameters") diff --git a/x/token/services/network/evm/statedelta/types_test.go b/x/token/services/network/evm/statedelta/types_test.go index 1db228de99..cc5ea5811b 100644 --- a/x/token/services/network/evm/statedelta/types_test.go +++ b/x/token/services/network/evm/statedelta/types_test.go @@ -44,6 +44,18 @@ func TestStateDeltaValidate(t *testing.T) { assert.Error(t, d.Validate()) }) + t.Run("setup delta with metadata", func(t *testing.T) { + // TokenState.sol's _applySetup reverts MalformedSetupDelta on non-empty metadataKeys too; + // Validate must refuse this before an endorser ever signs it, not leave it to the contract. + d := &StateDelta{ + IsSetup: true, + SetupParameters: []byte("pp"), + MetadataKeys: [][32]byte{{0x01}}, + MetadataVals: [][]byte{[]byte("v")}, + } + assert.Error(t, d.Validate()) + }) + t.Run("non-setup delta smuggling setup parameters", func(t *testing.T) { // SetupParameters is digest-covered: endorsers would sign bytes the contract ignores. d := &StateDelta{SpentRefs: [][32]byte{{0x01}}, SetupParameters: []byte("pp")} From 60babbf70b9a253dffb67ef9223239ab66ad3f79 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:40:09 +0530 Subject: [PATCH 23/35] fix(evm): read IsGraphHiding once per action, not once per output writeIssue and writeTransfer both called action.IsGraphHiding() fresh inside their per-output loop. It is an interface method, not a guaranteed-pure field, so nothing enforced that it answers the same way on every call within one action; if it ever didn't, one action's outputs could split across both marker styles, with whichever outputs saw the wrong answer silently losing their real SNMarker. The Fabric reference translator this package mirrors reads it once, before its own output loop, specifically to rule this out. Neither shipped driver can trigger this today (fabtoken and zkatdlog/nogh both hardcode IsGraphHiding to a constant false), so this is a latent divergence from the audited-safe reference pattern rather than a live bug, confirmed via a counterfeiter mock returning different values on successive calls in the new regression tests. Cached the value once at the top of each function instead. Signed-off-by: atharrva01 --- .../network/evm/statedelta/translator.go | 11 ++++-- .../network/evm/statedelta/translator_test.go | 34 +++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/x/token/services/network/evm/statedelta/translator.go b/x/token/services/network/evm/statedelta/translator.go index 03e576bcc1..e899801819 100644 --- a/x/token/services/network/evm/statedelta/translator.go +++ b/x/token/services/network/evm/statedelta/translator.go @@ -162,8 +162,13 @@ func (t *Translator) writeIssue(action translator.IssueAction) error { if err != nil { return errors.Wrapf(err, "failed to get issue outputs") } + // Read once, outside the loop: every output of one action shares one graph-hiding mode, and a + // per-output re-query would let an action whose IsGraphHiding answer isn't perfectly idempotent + // (an interface method, not a guaranteed-pure field) split its own outputs across both marker + // styles. Matches the Fabric reference translator's own cache-before-the-loop pattern. + graphHiding := action.IsGraphHiding() for i, output := range outputs { - t.appendOutput(t.counter+uint64(i), output, action.IsGraphHiding()) // #nosec G115 -- i is a small slice index + t.appendOutput(t.counter+uint64(i), output, graphHiding) // #nosec G115 -- i is a small slice index } t.counter += uint64(len(outputs)) @@ -183,6 +188,8 @@ func (t *Translator) writeTransfer(action translator.TransferAction) error { } // Redeem outputs are skipped but their slot still consumes an output index, exactly as the // Fabric translator enumerates them. + // Read once, outside the loop: see the matching comment in writeIssue. + graphHiding := action.IsGraphHiding() for i := range action.NumOutputs() { if action.IsRedeemAt(i) { continue @@ -191,7 +198,7 @@ func (t *Translator) writeTransfer(action translator.TransferAction) error { if err != nil { return errors.Wrapf(err, "failed to serialize transfer output at index %d", i) } - t.appendOutput(t.counter+uint64(i), output, action.IsGraphHiding()) // #nosec G115 -- i is a small output index + t.appendOutput(t.counter+uint64(i), output, graphHiding) // #nosec G115 -- i is a small output index } t.counter += uint64(action.NumOutputs()) // #nosec G115 -- output counts are small diff --git a/x/token/services/network/evm/statedelta/translator_test.go b/x/token/services/network/evm/statedelta/translator_test.go index 308d5eb1fe..bea0c9ce2f 100644 --- a/x/token/services/network/evm/statedelta/translator_test.go +++ b/x/token/services/network/evm/statedelta/translator_test.go @@ -103,6 +103,40 @@ func TestIssueMapping(t *testing.T) { assert.False(t, d.IsSetup) } +// TestIssueGraphHidingReadOncePerAction guards against re-querying IsGraphHiding per output: if it +// isn't idempotent across calls (nothing in the interface guarantees a pure getter), a per-output +// re-query would let one action's outputs split across both marker styles, silently losing the real +// SNMarker for whichever outputs saw the "wrong" answer. +func TestIssueGraphHidingReadOncePerAction(t *testing.T) { + a := issueAction([][]byte{[]byte("out-0"), []byte("out-1")}, nil) + a.IsGraphHidingReturnsOnCall(0, false) + a.IsGraphHidingReturnsOnCall(1, true) + + tr := NewTranslator(testAnchor(0x44), testPP, 0) + require.NoError(t, tr.Write(context.Background(), a)) + d := finish(t, tr) + + require.Len(t, d.Outputs, 2) + assert.Equal(t, d.Outputs[0].SNMarker == [32]byte{}, d.Outputs[1].SNMarker == [32]byte{}, + "both outputs of one action must agree on graph-hiding mode") +} + +// TestTransferGraphHidingReadOncePerAction is the transfer-side counterpart of +// TestIssueGraphHidingReadOncePerAction. +func TestTransferGraphHidingReadOncePerAction(t *testing.T) { + a := transferAction(nil, nil, [][]byte{[]byte("keep-0"), []byte("keep-1")}, nil, nil) + a.IsGraphHidingReturnsOnCall(0, false) + a.IsGraphHidingReturnsOnCall(1, true) + + tr := NewTranslator(testAnchor(0x55), testPP, 0) + require.NoError(t, tr.Write(context.Background(), a)) + d := finish(t, tr) + + require.Len(t, d.Outputs, 2) + assert.Equal(t, d.Outputs[0].SNMarker == [32]byte{}, d.Outputs[1].SNMarker == [32]byte{}, + "both outputs of one action must agree on graph-hiding mode") +} + // TestTransferMapping covers the content-bound spend refs and the redeem slot semantics: a redeem // output is skipped but its index is consumed, exactly as the Fabric translator enumerates outputs. func TestTransferMapping(t *testing.T) { From 6517bb40503fc44a9dc938e0b257b67bfdffced1 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:44:37 +0530 Subject: [PATCH 24/35] test(evm): exercise a real setup action through the endorsement pipeline Nothing in this package's unit tests drove Responder.endorse with a SetupAction before this, and the integration harness's own way of bumping public parameters mid-test (nwo.SetupUpdater) hand-assembles and signs a StateDelta directly rather than calling into Responder, DeltaFactory, or Translator.writeSetup at all. So the production code path a real endorser actually runs for an administrative PP update had zero coverage anywhere in the repo - a bug in Authorize, Build, or writeSetup specific to the setup shape could ship undetected while every other test stayed green. TestResponderEndorsesASetupAction drives one through Handle end to end and confirms the signature recovers to the endorser over the independently-recomputed digest, the same no-blind-sign check TestResponderSignsWhatItBuilds already does for issue actions. It passed on the first run: this closes a real coverage gap rather than a live bug in the pipeline itself. This does not touch nwo.SetupUpdater or the integration harness's bypass, which is a separate, larger question about whether integration tests should route PP updates through real endorser sessions instead. Signed-off-by: atharrva01 --- .../network/evm/endorsement/responder_test.go | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/x/token/services/network/evm/endorsement/responder_test.go b/x/token/services/network/evm/endorsement/responder_test.go index 9675ff76f7..2a4e05bc48 100644 --- a/x/token/services/network/evm/endorsement/responder_test.go +++ b/x/token/services/network/evm/endorsement/responder_test.go @@ -241,5 +241,37 @@ func TestResponderSurfacesPublicParamsFailure(t *testing.T) { assert.Empty(t, resp.Signature) } +// fakeSetup is a minimal SetupAction (the SDK ships no counterfeiter fake for it), mirroring +// statedelta's own test double for the same interface. +type fakeSetup struct { + params []byte +} + +func (f *fakeSetup) GetSetupParameters() ([]byte, error) { return f.params, nil } + +// TestResponderEndorsesASetupAction drives a public-parameters update through the real endorsement +// pipeline: authorize, validate, translate via Translator.writeSetup, sign. Nothing else in this +// package's unit tests exercises a setup request this way, and the integration harness's own way of +// bumping public parameters mid-test (nwo.SetupUpdater) hand-assembles and signs a StateDelta directly +// rather than going through Responder/DeltaFactory - so this is that pipeline's first real exercise +// against a genuine setup action. +func TestResponderEndorsesASetupAction(t *testing.T) { + newPP := []byte("new-public-parameters") + actions := []any{&fakeSetup{params: newPP}} + meta := map[string][]byte{common.TokenRequestToSign: []byte(trsMessage)} + signer := newSigner(t, 1) + r := newResponder(t, &fakeValidator{actions: actions, meta: meta}, &fakePP{raw: []byte(testPPRaw), version: testPPVer}, signer) + + req := validRequest() + resp := r.Handle(context.Background(), view.Identity(testCaller), req) + require.NoError(t, resp.Error()) + require.NotEmpty(t, resp.Signature) + + digest := recomputeDigest(t, req.Anchor, actions, meta) + got, err := eip712.RecoverAddress(digest, resp.Signature) + require.NoError(t, err) + assert.Equal(t, signer.Address(), got, "endorser must have signed the setup delta it built") +} + // compile-time check that the concrete signer satisfies the injected interface. var _ EndorserSigner = (*eip712.Signer)(nil) From 12afcffbb74e2576b839efd2e12881fa8d202c1a Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:47:14 +0530 Subject: [PATCH 25/35] fix(evm): reject a zero TokenState address at setup-updater construction NewSetupUpdater validated Client, Submitter, ChainID and EndorserKeys but not TokenState, so a caller that wires one up before Deploy has populated the real contract address (the real integration caller does exactly this, building config from a Deployment struct that starts zero-valued) got a constructor that succeeded silently. The failure only surfaced later, less clearly, inside buildDelta's first getPublicParameters call against the zero address. Also fixed a test bug this introduced: TestNewSetupUpdaterValidatesItsInput's base() config left TokenState zero by default, so without giving it a real address first, every other case in that table (no client, no submitter, ...) would have started failing on the new check before ever reaching the field it was actually named for. Signed-off-by: atharrva01 --- x/token/services/network/evm/nwo/setup.go | 3 +++ x/token/services/network/evm/nwo/setup_test.go | 15 ++++++++++----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/x/token/services/network/evm/nwo/setup.go b/x/token/services/network/evm/nwo/setup.go index 89da0883c1..7458949644 100644 --- a/x/token/services/network/evm/nwo/setup.go +++ b/x/token/services/network/evm/nwo/setup.go @@ -69,6 +69,9 @@ func NewSetupUpdater(config SetupUpdaterConfig) (*SetupUpdater, error) { if config.Client == nil { return nil, errors.New("evm nwo: setup updater needs a client") } + if config.TokenState == (client.Address{}) { + return nil, errors.New("evm nwo: setup updater needs the deployed TokenState address") + } if config.Submitter == nil { return nil, errors.New("evm nwo: setup updater needs a submitter to broadcast with") } diff --git a/x/token/services/network/evm/nwo/setup_test.go b/x/token/services/network/evm/nwo/setup_test.go index e8efad125e..f332d2c7b1 100644 --- a/x/token/services/network/evm/nwo/setup_test.go +++ b/x/token/services/network/evm/nwo/setup_test.go @@ -190,9 +190,13 @@ func TestUpdateRejectsEmptyParameters(t *testing.T) { } func TestNewSetupUpdaterValidatesItsInput(t *testing.T) { + validTokenState, err := client.HexToAddress("0x5FbDB2315678afecb367f032d93F642f64180aa3") + require.NoError(t, err) + base := func() SetupUpdaterConfig { return SetupUpdaterConfig{ Client: &mock.EVMClient{}, + TokenState: validTokenState, ChainID: big.NewInt(testChainID), EndorserKeys: [][]byte{testKey(1)}, Submitter: &evm.Submitter{}, @@ -200,11 +204,12 @@ func TestNewSetupUpdaterValidatesItsInput(t *testing.T) { } for name, broken := range map[string]func(*SetupUpdaterConfig){ - "no client": func(c *SetupUpdaterConfig) { c.Client = nil }, - "no submitter": func(c *SetupUpdaterConfig) { c.Submitter = nil }, - "no chain id": func(c *SetupUpdaterConfig) { c.ChainID = nil }, - "no endorsers": func(c *SetupUpdaterConfig) { c.EndorserKeys = nil }, - "unusable key": func(c *SetupUpdaterConfig) { c.EndorserKeys = [][]byte{{0x01}} }, + "no client": func(c *SetupUpdaterConfig) { c.Client = nil }, + "no token state": func(c *SetupUpdaterConfig) { c.TokenState = client.Address{} }, + "no submitter": func(c *SetupUpdaterConfig) { c.Submitter = nil }, + "no chain id": func(c *SetupUpdaterConfig) { c.ChainID = nil }, + "no endorsers": func(c *SetupUpdaterConfig) { c.EndorserKeys = nil }, + "unusable key": func(c *SetupUpdaterConfig) { c.EndorserKeys = [][]byte{{0x01}} }, } { t.Run(name, func(t *testing.T) { config := base() From 03d00b4a57c3c3997cef165fb61c442e6674a4da Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 16:47:22 +0530 Subject: [PATCH 26/35] docs(evm): clarify DeploySpec.Threshold's zero means invalid, not all SetupUpdaterConfig.Threshold, in this same package, documents zero as 'use every endorser' and NewSetupUpdater implements exactly that. DeploySpec.Threshold carried no such note, and the one concrete Backend, ForgeBackend.Deploy (integration/nwo/token/evm/deploy.go), treats zero as a hard validation error instead. Both config types describe the same authority per this package's own doc.go framing, so a reader forming an expectation from one about the other would be wrong. No behavior change; documents what Deploy already enforces and names why deploy-time and update-time thresholds differ on purpose. Signed-off-by: atharrva01 --- x/token/services/network/evm/nwo/backend.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/x/token/services/network/evm/nwo/backend.go b/x/token/services/network/evm/nwo/backend.go index bb2ce4b5df..f541ba8c0d 100644 --- a/x/token/services/network/evm/nwo/backend.go +++ b/x/token/services/network/evm/nwo/backend.go @@ -23,8 +23,12 @@ type TMSRef struct { // and the initial public parameters (version 0). Endorsers are Ethereum addresses here; the address // to FSC identity binding the driver's config carries is assembled by the harness alongside this. type DeploySpec struct { - TMS TMSRef - Endorsers []client.Address + TMS TMSRef + Endorsers []client.Address + // Threshold is the quorum size the EndorsementVerifier is constructed with, baked into the + // contract for its whole life. Unlike SetupUpdaterConfig.Threshold in this same package, zero is + // not "use every endorser" here: a concrete Backend must reject it, since a deploy-time threshold + // is a security parameter that has to be stated explicitly, not defaulted. Threshold uint GraphHiding bool PublicParams []byte From 0e0b1e7a7244dcc598b8d33e6c60fa6432378e02 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sat, 15 Aug 2026 22:37:56 +0530 Subject: [PATCH 27/35] chore(evm): gitignore the bug-hunt scratch prompt Mirrors the plan.md precedent: a working file for the round-3 hunt that should never land in a PR diff. Signed-off-by: atharrva01 --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index d5d06091f9..9d0cac0111 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ cmd/token_validation_service/out/ coverage.out /.codex/ plan.md +x/BUG_HUNT_PROMPT.md # The EVM integration suites generate a node binary tree and a network under out/ and testdata/. # Both hold real Go files, so a stray "git add -A" commits them and a linter then reports on them. From bee6fbd0dcd7b8c755bdc6d2b09d6e63c7620df8 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sun, 16 Aug 2026 03:01:22 +0530 Subject: [PATCH 28/35] fix(evm): bound the size of a StateDelta before it is validated or hashed An endorser's reply carries the StateDelta it built, and the initiator JSON-decodes and EIP-712-hashes it before it has any reason to trust the endorser's signature over it: bind() only rejects a mismatched delta after eip712.Digest has already hashed every field. StateDelta.Validate had no upper bound on the number of outputs, spent refs or metadata entries, or on any variable-length field, so a single dishonest registered endorser could force real CPU and memory cost per request with an oversized, self-consistent but wrong delta. Validate now rejects a delta whose entry counts or variable-length fields exceed generous fixed bounds before doing any per-element work, closing the gap for every caller of Validate, not just the endorsement path. Signed-off-by: atharrva01 --- .../services/network/evm/statedelta/types.go | 39 +++++++++++++++ .../network/evm/statedelta/types_test.go | 47 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/x/token/services/network/evm/statedelta/types.go b/x/token/services/network/evm/statedelta/types.go index cba4565a9a..92df00a8bc 100644 --- a/x/token/services/network/evm/statedelta/types.go +++ b/x/token/services/network/evm/statedelta/types.go @@ -12,6 +12,19 @@ import ( "github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors" ) +// Size bounds Validate enforces on a StateDelta. They exist so that a delta received from an +// untrusted party (an endorser's reply, over the wire, before its signature has been checked - see +// endorsement.Initiator) cannot force unbounded decoding and EIP-712 hashing work on the receiver. +// They are set generously above anything a real translated token request produces, not as a business +// rule on transaction shape. +const ( + // maxDeltaEntries bounds SpentRefs, Outputs and the metadata key/value pairs, each independently. + maxDeltaEntries = 4096 + // maxFieldBytes bounds each variable-length byte field: one output's TokenData, one metadata + // value, or SetupParameters. + maxFieldBytes = 1 << 20 // 1 MiB +) + // OutputToken is a newly created token. It carries two keys (both derived off-chain; the contract // treats them as opaque): the addressable id and the content-bound marker. type OutputToken struct { @@ -76,7 +89,33 @@ type StateDelta struct { // - MetadataKeys are strictly ascending (the frozen §4.4 canonicalization). Unsorted keys mean the // emitting translator is broken (endorsers would produce different bytes and signatures would // not assemble); duplicate keys would make the on-chain write order ambiguous. +// +// It also enforces the size bounds above before doing any per-element work, so a delta built from +// untrusted bytes is rejected before it can force unbounded comparison or hashing work downstream. func (d *StateDelta) Validate() error { + if len(d.SpentRefs) > maxDeltaEntries { + return errors.Errorf("too many spent refs: %d exceeds the %d limit", len(d.SpentRefs), maxDeltaEntries) + } + if len(d.Outputs) > maxDeltaEntries { + return errors.Errorf("too many outputs: %d exceeds the %d limit", len(d.Outputs), maxDeltaEntries) + } + for i, out := range d.Outputs { + if len(out.TokenData) > maxFieldBytes { + return errors.Errorf("output %d token data too large: %d bytes exceeds the %d limit", i, len(out.TokenData), maxFieldBytes) + } + } + if len(d.MetadataKeys) > maxDeltaEntries { + return errors.Errorf("too many metadata entries: %d exceeds the %d limit", len(d.MetadataKeys), maxDeltaEntries) + } + for i, val := range d.MetadataVals { + if len(val) > maxFieldBytes { + return errors.Errorf("metadata value %d too large: %d bytes exceeds the %d limit", i, len(val), maxFieldBytes) + } + } + if len(d.SetupParameters) > maxFieldBytes { + return errors.Errorf("setup parameters too large: %d bytes exceeds the %d limit", len(d.SetupParameters), maxFieldBytes) + } + if len(d.MetadataKeys) != len(d.MetadataVals) { return errors.Errorf("metadata keys/values length mismatch: %d != %d", len(d.MetadataKeys), len(d.MetadataVals)) } diff --git a/x/token/services/network/evm/statedelta/types_test.go b/x/token/services/network/evm/statedelta/types_test.go index cc5ea5811b..9ad871e82b 100644 --- a/x/token/services/network/evm/statedelta/types_test.go +++ b/x/token/services/network/evm/statedelta/types_test.go @@ -85,4 +85,51 @@ func TestStateDeltaValidate(t *testing.T) { } assert.Error(t, d.Validate()) }) + + // These are the regression tests for the DoS finding: a delta from an untrusted source (an + // endorser's reply, before its signature is checked) must be rejected before Validate or the + // EIP-712 digest does unbounded per-element work over it. + t.Run("too many outputs", func(t *testing.T) { + d := &StateDelta{Outputs: make([]OutputToken, maxDeltaEntries+1)} + assert.ErrorContains(t, d.Validate(), "too many outputs") + }) + + t.Run("too many spent refs", func(t *testing.T) { + d := &StateDelta{SpentRefs: make([][32]byte, maxDeltaEntries+1)} + assert.ErrorContains(t, d.Validate(), "too many spent refs") + }) + + t.Run("too many metadata entries", func(t *testing.T) { + d := &StateDelta{ + MetadataKeys: make([][32]byte, maxDeltaEntries+1), + MetadataVals: make([][]byte, maxDeltaEntries+1), + } + assert.ErrorContains(t, d.Validate(), "too many metadata entries") + }) + + t.Run("output token data too large", func(t *testing.T) { + d := &StateDelta{Outputs: []OutputToken{{TokenData: make([]byte, maxFieldBytes+1)}}} + assert.ErrorContains(t, d.Validate(), "token data too large") + }) + + t.Run("metadata value too large", func(t *testing.T) { + d := &StateDelta{ + MetadataKeys: [][32]byte{{0x01}}, + MetadataVals: [][]byte{make([]byte, maxFieldBytes+1)}, + } + assert.ErrorContains(t, d.Validate(), "metadata value 0 too large") + }) + + t.Run("setup parameters too large", func(t *testing.T) { + d := &StateDelta{IsSetup: true, SetupParameters: make([]byte, maxFieldBytes+1)} + assert.ErrorContains(t, d.Validate(), "setup parameters too large") + }) + + t.Run("at the bound is still valid", func(t *testing.T) { + d := &StateDelta{ + SpentRefs: make([][32]byte, maxDeltaEntries), + Outputs: []OutputToken{{TokenData: make([]byte, maxFieldBytes)}}, + } + require.NoError(t, d.Validate()) + }) } From 46bab47fbc8a22252a9e73f9d45c2244718496c2 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sun, 16 Aug 2026 03:04:11 +0530 Subject: [PATCH 29/35] fix(evm): reject a private-key scalar that overflows the curve order NewSignerFromBytes and LoadKey both went through secp256k1.PrivKeyFromBytes, which silently reduces an out-of-range scalar modulo the curve order instead of erroring - the overflow flag ModNScalar.SetByteSlice returns was discarded. A key file whose bytes are at or above the order would load as a different, unrelated key with no error, catchable only via LoadKeyForAddress's separate address check, and not at all on the documented empty-expected skip path. Both now go through eip712.DecodePrivateKeyScalar, which checks the overflow flag itself before accepting the scalar. Signed-off-by: atharrva01 --- x/token/services/network/evm/eip712/signer.go | 24 +++++++++++++++---- .../network/evm/eip712/signer_test.go | 6 +++++ x/token/services/network/evm/keystore.go | 6 ++--- x/token/services/network/evm/keystore_test.go | 8 +++++++ 4 files changed, 37 insertions(+), 7 deletions(-) diff --git a/x/token/services/network/evm/eip712/signer.go b/x/token/services/network/evm/eip712/signer.go index 6e51b69556..5c6fc7ea0b 100644 --- a/x/token/services/network/evm/eip712/signer.go +++ b/x/token/services/network/evm/eip712/signer.go @@ -55,19 +55,35 @@ func NewSigner(key *secp256k1.PrivateKey) *Signer { } // NewSignerFromBytes returns a Signer over a raw 32-byte private-key scalar. It rejects scalars of -// the wrong length and the zero scalar (whose public key is the point at infinity). +// the wrong length, the zero scalar (whose public key is the point at infinity), and a scalar at or +// above the curve order: secp256k1.PrivKeyFromBytes would silently reduce such a value modulo the +// order rather than erroring, producing a signer for a different key than the raw bytes represent. func NewSignerFromBytes(raw []byte) (*Signer, error) { if len(raw) != PrivateKeyLength { return nil, errors.Errorf("invalid private key length: expected %d bytes, got %d", PrivateKeyLength, len(raw)) } - key := secp256k1.PrivKeyFromBytes(raw) - if key.Key.IsZero() { - return nil, errors.New("invalid private key: zero scalar") + key, err := DecodePrivateKeyScalar(raw) + if err != nil { + return nil, err } return NewSigner(key), nil } +// DecodePrivateKeyScalar parses raw as a private-key scalar, rejecting the zero scalar and anything +// that overflows the curve order instead of silently reducing it modulo the order. +func DecodePrivateKeyScalar(raw []byte) (*secp256k1.PrivateKey, error) { + var scalar secp256k1.ModNScalar + if overflow := scalar.SetByteSlice(raw); overflow { + return nil, errors.New("invalid private key: scalar is not less than the curve order") + } + if scalar.IsZero() { + return nil, errors.New("invalid private key: zero scalar") + } + + return secp256k1.NewPrivateKey(&scalar), nil +} + // Address returns the Ethereum address of the signer, as registered in the EndorsementVerifier. func (s *Signer) Address() client.Address { return s.address diff --git a/x/token/services/network/evm/eip712/signer_test.go b/x/token/services/network/evm/eip712/signer_test.go index c8d99f1a9d..4ceae4cafb 100644 --- a/x/token/services/network/evm/eip712/signer_test.go +++ b/x/token/services/network/evm/eip712/signer_test.go @@ -154,6 +154,12 @@ func TestNewSignerFromBytesRejectsInvalid(t *testing.T) { _, err = NewSignerFromBytes(make([]byte, PrivateKeyLength)) require.Error(t, err, "zero scalar must be rejected") + + // A scalar at or above the curve order must be rejected rather than silently reduced modulo the + // order into a different, unrelated key. + overflowing := bytes.Repeat([]byte{0xff}, PrivateKeyLength) + _, err = NewSignerFromBytes(overflowing) + require.Error(t, err, "a scalar exceeding the curve order must be rejected") } // TestGoldenFixtureEndorsement pins the fixture endorsement (statedelta_digest_fixture.json): the diff --git a/x/token/services/network/evm/keystore.go b/x/token/services/network/evm/keystore.go index e52441fd02..e50f020433 100644 --- a/x/token/services/network/evm/keystore.go +++ b/x/token/services/network/evm/keystore.go @@ -45,9 +45,9 @@ func LoadKey(path string) (*secp256k1.PrivateKey, error) { return nil, errors.Wrapf(err, "evm keystore: invalid key material at [%s]", path) } - key := secp256k1.PrivKeyFromBytes(scalar) - if key.Key.IsZero() { - return nil, errors.Errorf("evm keystore: the key at [%s] is the zero scalar", path) + key, err := eip712.DecodePrivateKeyScalar(scalar) + if err != nil { + return nil, errors.Wrapf(err, "evm keystore: the key at [%s] is invalid: %v", path, err) } return key, nil diff --git a/x/token/services/network/evm/keystore_test.go b/x/token/services/network/evm/keystore_test.go index f58d0665b1..5e71002520 100644 --- a/x/token/services/network/evm/keystore_test.go +++ b/x/token/services/network/evm/keystore_test.go @@ -74,6 +74,14 @@ func TestLoadKeyRejectsBadMaterial(t *testing.T) { _, err := LoadKey(writeKey(t, "00000000000000000000000000000000000000000000000000000000000000"+"00")) require.Error(t, err, "the zero scalar has no valid public key") }) + + t.Run("scalar exceeds the curve order", func(t *testing.T) { + // Must be rejected outright, not silently reduced modulo the curve order into a different, + // unrelated key that the rest of the file's hex bytes gave no hint of. + overflowing := "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + _, err := LoadKey(writeKey(t, overflowing)) + require.Error(t, err, "a scalar at or above the curve order must be rejected") + }) } // TestLoadKeyForAddressCatchesMismatch is the configuration guard: pairing a key with the wrong From 8baea77f3df23838f87418d8dc5df44ebed8e8af Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sun, 16 Aug 2026 03:06:17 +0530 Subject: [PATCH 30/35] fix(evm): recognize and discard a reorged-out log The wire and domain Log types had no field for eth_getLogs's removed flag, so a log the node reports as belonging to a block a reorg has undone was indistinguishable from a canonical one. TxHashByAnchor could report a transaction hash, or trip its own duplicate-commit guard, off a log that no longer reflects the chain. jsonLog/Log now carry Removed, decoded from the wire, and TxHashByAnchor drops removed logs before deciding whether the anchor was applied. Signed-off-by: atharrva01 --- .../services/network/evm/client/evmclient.go | 4 +++ .../services/network/evm/client/jsonrpc.go | 5 +++- .../network/evm/client/jsonrpc_test.go | 18 +++++++++++ x/token/services/network/evm/finality/logs.go | 5 ++++ .../network/evm/finality/logs_test.go | 30 +++++++++++++++++++ 5 files changed, 61 insertions(+), 1 deletion(-) diff --git a/x/token/services/network/evm/client/evmclient.go b/x/token/services/network/evm/client/evmclient.go index 4f74caa0c1..08a550b851 100644 --- a/x/token/services/network/evm/client/evmclient.go +++ b/x/token/services/network/evm/client/evmclient.go @@ -43,6 +43,10 @@ type Log struct { Data []byte TxHash Hash BlockNumber uint64 + // Removed is true when the node is reporting that a reorg has undone the block this log was + // mined in. A caller treating a log as evidence of a committed, permanent effect must not trust + // one with Removed set. + Removed bool } // LogFilter selects logs by contract address, block range and indexed topics. diff --git a/x/token/services/network/evm/client/jsonrpc.go b/x/token/services/network/evm/client/jsonrpc.go index 77892ba40a..d78ab48818 100644 --- a/x/token/services/network/evm/client/jsonrpc.go +++ b/x/token/services/network/evm/client/jsonrpc.go @@ -430,6 +430,7 @@ type jsonLog struct { Data string `json:"data"` TxHash string `json:"transactionHash"` BlockNumber string `json:"blockNumber"` + Removed bool `json:"removed"` } func (j *jsonLog) toLog() (Log, error) { @@ -458,7 +459,9 @@ func (j *jsonLog) toLog() (Log, error) { return Log{}, errors.Wrap(err, "invalid log block number") } - return Log{Address: addr, Topics: topics, Data: data, TxHash: txHash, BlockNumber: blockNumber}, nil + return Log{ + Address: addr, Topics: topics, Data: data, TxHash: txHash, BlockNumber: blockNumber, Removed: j.Removed, + }, nil } type jsonReceipt struct { diff --git a/x/token/services/network/evm/client/jsonrpc_test.go b/x/token/services/network/evm/client/jsonrpc_test.go index f787bd4732..76c185316b 100644 --- a/x/token/services/network/evm/client/jsonrpc_test.go +++ b/x/token/services/network/evm/client/jsonrpc_test.go @@ -440,6 +440,24 @@ func TestGetLogs(t *testing.T) { assert.Equal(t, "0x64", arg["toBlock"]) } +// TestGetLogsCapturesRemoved checks a log the node marks as reorged-out decodes with Removed set, +// rather than being indistinguishable from a canonical one. +func TestGetLogsCapturesRemoved(t *testing.T) { + c, _ := newTestServer(t, map[string]string{ + "eth_getLogs": `[{ + "address":"0x5FbDB2315678afecb367f032d93F642f64180aa3", + "topics":["0x1111111111111111111111111111111111111111111111111111111111111111"], + "data":"0x","transactionHash":"0x853f272fffc6efc284fc16a254decca742d2347e05703e501c59968f78f81ffa", + "blockNumber":"0x2","removed":true + }]`, + }) + + logs, err := c.GetLogs(context.Background(), LogFilter{}) + require.NoError(t, err) + require.Len(t, logs, 1) + assert.True(t, logs[0].Removed, "a reorged-out log must be reported as such") +} + // TestGetLogsWildcardTopic checks that an empty inner slice becomes a null (match-any) position, // which is the eth_getLogs convention. func TestGetLogsWildcardTopic(t *testing.T) { diff --git a/x/token/services/network/evm/finality/logs.go b/x/token/services/network/evm/finality/logs.go index 1d84a6c03a..9392ff9f4f 100644 --- a/x/token/services/network/evm/finality/logs.go +++ b/x/token/services/network/evm/finality/logs.go @@ -8,6 +8,7 @@ package finality import ( "context" + "slices" "github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors" @@ -52,6 +53,10 @@ func (m *Manager) TxHashByAnchor(ctx context.Context, anchor [32]byte) (client.H if err != nil { return client.Hash{}, false, errors.Wrap(err, "finality: failed to search for the commit event") } + + // A log the node marks removed was mined in a block a reorg has since undone: it is no longer + // part of the canonical chain and must not be treated as evidence the anchor was applied. + logs = slices.DeleteFunc(logs, func(l client.Log) bool { return l.Removed }) if len(logs) == 0 { return client.Hash{}, false, nil } diff --git a/x/token/services/network/evm/finality/logs_test.go b/x/token/services/network/evm/finality/logs_test.go index 6c823efd6e..761ed575ce 100644 --- a/x/token/services/network/evm/finality/logs_test.go +++ b/x/token/services/network/evm/finality/logs_test.go @@ -104,6 +104,36 @@ func TestTxHashByAnchorRejectsDuplicates(t *testing.T) { assert.Contains(t, err.Error(), "replay guard") } +// TestTxHashByAnchorIgnoresARemovedLog is the reorg regression test: a log the node reports as +// removed was mined in a block that is no longer canonical and must not be trusted as evidence the +// anchor was applied, nor counted against the replay-guard duplicate check. +func TestTxHashByAnchorIgnoresARemovedLog(t *testing.T) { + want, err := client.HexToHash("0x853f272fffc6efc284fc16a254decca742d2347e05703e501c59968f78f81ffa") + require.NoError(t, err) + + t.Run("a removed log alone is not found", func(t *testing.T) { + evm := &mock.EVMClient{} + evm.GetLogsReturns([]client.Log{{TxHash: want, BlockNumber: 12, Removed: true}}, nil) + + _, found, err := logManager(evm).TxHashByAnchor(t.Context(), anchor(0x01)) + require.NoError(t, err) + assert.False(t, found, "a reorged-out log is not evidence the anchor was applied") + }) + + t.Run("a removed log alongside the real one does not trip the duplicate guard", func(t *testing.T) { + evm := &mock.EVMClient{} + evm.GetLogsReturns([]client.Log{ + {TxHash: want, BlockNumber: 12}, + {TxHash: want, BlockNumber: 7, Removed: true}, + }, nil) + + got, found, err := logManager(evm).TxHashByAnchor(t.Context(), anchor(0x01)) + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, want, got) + }) +} + func TestTxHashByAnchorSurfacesQueryFailure(t *testing.T) { evm := &mock.EVMClient{} evm.GetLogsReturns(nil, errors.New("node unavailable")) From 44a67a08f4d84dd4cb8482b7349111e09afffe96 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sun, 16 Aug 2026 03:12:18 +0530 Subject: [PATCH 31/35] fix(evm): honour the configured block tag in the log-based finality search TxHashByAnchor always searched up to "latest" regardless of the operator's configured reorg-safety tag, unlike every other reader in the module (contractReader, ChainProvider, VersionKeeper all take and use one). An operator who set finalized/safe everywhere else still got an unpinned, head-relative log search here, with no way to configure otherwise. LogFilter gains ToBlockTag, sent as the upper bound instead of the numeric ToBlock when set; finality.Manager now takes and threads through the same blockTag its network.go caller already reads off Config.Finality for the reader and the version keeper. Signed-off-by: atharrva01 --- .../services/network/evm/client/evmclient.go | 13 ++++++++---- .../services/network/evm/client/jsonrpc.go | 5 ++++- x/token/services/network/evm/finality/logs.go | 13 +++++++----- .../network/evm/finality/logs_test.go | 21 ++++++++++++++++--- .../services/network/evm/finality/manager.go | 8 +++++-- .../network/evm/finality/manager_test.go | 2 +- x/token/services/network/evm/network.go | 7 +++++-- 7 files changed, 51 insertions(+), 18 deletions(-) diff --git a/x/token/services/network/evm/client/evmclient.go b/x/token/services/network/evm/client/evmclient.go index 08a550b851..b63d01c64f 100644 --- a/x/token/services/network/evm/client/evmclient.go +++ b/x/token/services/network/evm/client/evmclient.go @@ -53,14 +53,19 @@ type Log struct { // Topics follows the eth_getLogs convention: position i lists the acceptable values for topic i; // an empty inner slice matches any value at that position. // -// ToBlock == 0 means "latest": a range ending at genesis is never a useful query, and searching up to -// the head is what a caller looking for an event actually wants, so the zero value is spent on the -// common case rather than requiring a separate round trip to read the current block number. +// The upper end of the range is ToBlockTag when set, otherwise ToBlock, with ToBlock == 0 meaning +// "latest": a range ending at genesis is never a useful query, and searching up to the head is what a +// caller looking for an event actually wants, so the zero value is spent on the common case rather +// than requiring a separate round trip to read the current block number. ToBlockTag lets a caller that +// cares about reorg safety search only up to its configured tag (e.g. "finalized") instead. type LogFilter struct { Address Address FromBlock uint64 ToBlock uint64 - Topics [][]Hash + // ToBlockTag, when non-empty, is sent as the upper bound instead of ToBlock (e.g. BlockTagFinalized + // or BlockTagSafe). + ToBlockTag string + Topics [][]Hash } // GasFees carries the EIP-1559 fee parameters suggested by the node. diff --git a/x/token/services/network/evm/client/jsonrpc.go b/x/token/services/network/evm/client/jsonrpc.go index d78ab48818..8c6dc19484 100644 --- a/x/token/services/network/evm/client/jsonrpc.go +++ b/x/token/services/network/evm/client/jsonrpc.go @@ -113,7 +113,10 @@ func (c *JSONRPCClient) GetLogs(ctx context.Context, q LogFilter) ([]Log, error) topics = append(topics, values) } toBlock := any(encodeHexUint(q.ToBlock)) - if q.ToBlock == 0 { + switch { + case q.ToBlockTag != "": + toBlock = q.ToBlockTag + case q.ToBlock == 0: toBlock = BlockTagLatest } arg := map[string]any{ diff --git a/x/token/services/network/evm/finality/logs.go b/x/token/services/network/evm/finality/logs.go index 9392ff9f4f..f204790c84 100644 --- a/x/token/services/network/evm/finality/logs.go +++ b/x/token/services/network/evm/finality/logs.go @@ -43,12 +43,15 @@ func (m *Manager) TxHashByAnchor(ctx context.Context, anchor [32]byte) (client.H return client.Hash{}, false, errors.New("finality: no token state address configured for log lookups") } - // ToBlock is left at zero, which the filter reads as "latest": the anchor may have been applied at - // any point up to the head, and this avoids a second round trip just to learn the block number. + // ToBlockTag mirrors the configured tag every other reader in this module uses (finalized/safe by + // default in production), so a search here cannot see a block state reads elsewhere are configured + // not to trust yet. Left empty it reads as "latest": the anchor may have been applied at any point + // up to the head, and this avoids a second round trip just to learn the block number. logs, err := m.client.GetLogs(ctx, client.LogFilter{ - Address: m.tokenState, - FromBlock: m.fromBlock, - Topics: [][]client.Hash{{StateCommittedTopic()}, {anchor}}, + Address: m.tokenState, + FromBlock: m.fromBlock, + ToBlockTag: m.blockTag, + Topics: [][]client.Hash{{StateCommittedTopic()}, {anchor}}, }) if err != nil { return client.Hash{}, false, errors.Wrap(err, "finality: failed to search for the commit event") diff --git a/x/token/services/network/evm/finality/logs_test.go b/x/token/services/network/evm/finality/logs_test.go index 761ed575ce..c7e8a09206 100644 --- a/x/token/services/network/evm/finality/logs_test.go +++ b/x/token/services/network/evm/finality/logs_test.go @@ -27,7 +27,7 @@ func testAddress(low byte) client.Address { } func logManager(evm client.EVMClient) *Manager { - return NewManager(evm, &stubState{}, testAddress(0xAA), 0, 5*time.Millisecond, time.Second) + return NewManager(evm, &stubState{}, testAddress(0xAA), 0, "", 5*time.Millisecond, time.Second) } // TestStateCommittedTopic pins topic 0 against the value any Ethereum tooling computes @@ -63,7 +63,7 @@ func TestTxHashByAnchorFiltersCorrectly(t *testing.T) { evm := &mock.EVMClient{} evm.GetLogsReturns(nil, nil) - m := NewManager(evm, &stubState{}, testAddress(0xAA), 100, 5*time.Millisecond, time.Second) + m := NewManager(evm, &stubState{}, testAddress(0xAA), 100, "", 5*time.Millisecond, time.Second) _, _, err := m.TxHashByAnchor(t.Context(), anchor(0x42)) require.NoError(t, err) @@ -81,6 +81,21 @@ func TestTxHashByAnchorFiltersCorrectly(t *testing.T) { assert.Equal(t, client.Hash(anchor(0x42)), filter.Topics[1][0], "the anchor is the indexed topic") } +// TestTxHashByAnchorHonoursTheConfiguredBlockTag is the regression test for the finding that this +// search always reached the chain head regardless of the operator's configured reorg-safety tag, +// unlike every other reader in the module. A non-empty blockTag must reach the filter as ToBlockTag. +func TestTxHashByAnchorHonoursTheConfiguredBlockTag(t *testing.T) { + evm := &mock.EVMClient{} + evm.GetLogsReturns(nil, nil) + + m := NewManager(evm, &stubState{}, testAddress(0xAA), 0, client.BlockTagFinalized, 5*time.Millisecond, time.Second) + _, _, err := m.TxHashByAnchor(t.Context(), anchor(0x01)) + require.NoError(t, err) + + _, filter := evm.GetLogsArgsForCall(0) + assert.Equal(t, client.BlockTagFinalized, filter.ToBlockTag) +} + // TestTxHashByAnchorNotFound covers the ordinary case for a recipient still waiting: no event yet. // It must not be an error, since the transaction may simply not have been applied. func TestTxHashByAnchorNotFound(t *testing.T) { @@ -146,7 +161,7 @@ func TestTxHashByAnchorSurfacesQueryFailure(t *testing.T) { // querying the zero address, which would match nothing and look like "not committed yet". func TestTxHashByAnchorNeedsTheContract(t *testing.T) { evm := &mock.EVMClient{} - m := NewManager(evm, &stubState{}, client.Address{}, 0, 5*time.Millisecond, time.Second) + m := NewManager(evm, &stubState{}, client.Address{}, 0, "", 5*time.Millisecond, time.Second) _, _, err := m.TxHashByAnchor(t.Context(), anchor(0x01)) require.Error(t, err) diff --git a/x/token/services/network/evm/finality/manager.go b/x/token/services/network/evm/finality/manager.go index 7b544f7dc6..01c01a18c7 100644 --- a/x/token/services/network/evm/finality/manager.go +++ b/x/token/services/network/evm/finality/manager.go @@ -48,6 +48,7 @@ type Manager struct { state StateReader tokenState client.Address fromBlock uint64 + blockTag string pollInterval time.Duration timeout time.Duration @@ -57,13 +58,15 @@ type Manager struct { // NewManager returns a finality manager. Non-positive intervals fall back to sane defaults so a // partially filled configuration cannot produce a busy loop. -// The tokenState address and fromBlock are only needed by the log-based lookup (TxHashByAnchor); the -// status paths work without them. +// The tokenState address, fromBlock and blockTag are only needed by the log-based lookup +// (TxHashByAnchor); the status paths work without them. An empty blockTag searches up to the chain +// head, matching client.LogFilter's own default. func NewManager( evmClient client.EVMClient, state StateReader, tokenState client.Address, fromBlock uint64, + blockTag string, pollInterval, timeout time.Duration, ) *Manager { if pollInterval <= 0 { @@ -78,6 +81,7 @@ func NewManager( state: state, tokenState: tokenState, fromBlock: fromBlock, + blockTag: blockTag, pollInterval: pollInterval, timeout: timeout, pending: map[string]struct{}{}, diff --git a/x/token/services/network/evm/finality/manager_test.go b/x/token/services/network/evm/finality/manager_test.go index 5f4ebc4982..2c14296a76 100644 --- a/x/token/services/network/evm/finality/manager_test.go +++ b/x/token/services/network/evm/finality/manager_test.go @@ -101,7 +101,7 @@ func anchor(low byte) [32]byte { } func fastManager(evm client.EVMClient, state StateReader, timeout time.Duration) *Manager { - return NewManager(evm, state, testAddress(0xAA), 0, 5*time.Millisecond, timeout) + return NewManager(evm, state, testAddress(0xAA), 0, "", 5*time.Millisecond, timeout) } // --- resolution by eth transaction hash ------------------------------------------------------------ diff --git a/x/token/services/network/evm/network.go b/x/token/services/network/evm/network.go index 7cd1442df3..6448e2a8f5 100644 --- a/x/token/services/network/evm/network.go +++ b/x/token/services/network/evm/network.go @@ -91,8 +91,11 @@ func NewNetwork( submitter: submitter, reader: reader, membership: membership, - finality: finality.NewManager(evmClient, reader, tokenState, config.Finality.FromBlock, config.Finality.PollInterval, config.Finality.Timeout), - tokenState: tokenState, + finality: finality.NewManager( + evmClient, reader, tokenState, config.Finality.FromBlock, config.Finality.BlockTag, + config.Finality.PollInterval, config.Finality.Timeout, + ), + tokenState: tokenState, }, nil } From 4de0b76b8bbd0596bdf5368a08c02701f4bb41fb Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sun, 16 Aug 2026 03:24:05 +0530 Subject: [PATCH 32/35] fix(evm): propagate a broken endorser registration instead of only logging it registerEndorser swallowed every failure past the second-network and empty-allowlist checks (an unusable signing key, allowlist resolution, authorizer or responder construction, view-registry registration) as a log line. installEndorsement and Driver.New always returned success regardless, so a node explicitly configured with endorser.enabled came up looking healthy while never answering an endorsement request - discoverable only once a quorum it was needed for timed out, with nothing connecting the timeout back to the startup log. registerEndorser now returns an error that installEndorsement and New propagate, the same contract a broken submitter key already gets in newSubmitter. registeredFor is still only set once every step succeeds, so this does not change the existing retry-on-later-call or refuse-a-second-network behavior. Signed-off-by: atharrva01 --- x/token/services/network/evm/driver.go | 40 +++++++++++---------- x/token/services/network/evm/driver_test.go | 19 ++++++++++ 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/x/token/services/network/evm/driver.go b/x/token/services/network/evm/driver.go index 9a733301d1..0bfbe9a6f8 100644 --- a/x/token/services/network/evm/driver.go +++ b/x/token/services/network/evm/driver.go @@ -312,7 +312,9 @@ func (d *Driver) installEndorsement(n *Network, config *Config, evmClient client // 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(network+":"+channel, factory, config) + if err := d.registerEndorser(network+":"+channel, factory, config); err != nil { + return errors.Wrap(err, "evm: failed to register this node as an endorser") + } return nil } @@ -350,11 +352,19 @@ func (d *Driver) newSubmitter(config *Config, evmClient client.EVMClient) (*Subm // 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. // +// Because a broken endorser configuration answers no requests and looks identical to network trouble +// from the outside, discoverable only once a quorum times out, every failure past that check is +// returned to the caller instead of only logged: a node explicitly configured with endorser.enabled +// must not start up looking healthy while unable to fulfil that role, the same contract newSubmitter +// already holds for a configured-but-broken submitter key. registeredFor is set only once every step +// succeeds, so a failed attempt does not permanently lock the node out of registering on a later, +// successful call for the same network. +// // 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(networkKey string, factory *endorsement.ServiceFactory, config *Config) { +func (d *Driver) registerEndorser(networkKey string, factory *endorsement.ServiceFactory, config *Config) error { if d.viewRegistry == nil || !config.Endorser.Enabled { - return + return nil } d.registerMu.Lock() @@ -367,40 +377,32 @@ func (d *Driver) registerEndorser(networkKey string, factory *endorsement.Servic networkKey, d.registeredFor, networkKey) } - return + return nil } 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 + return errors.Wrap(err, "this node is configured as an endorser but its key is unusable") } allowed, err := config.AllowedRequesters(d.resolveIdentity) if err != nil { - logger.Errorf("failed to resolve the endorsement allowlist: %v", err) - - return + return errors.Wrap(err, "failed to resolve the endorsement allowlist") } authorizer, err := endorsement.NewAuthorizer(allowed) if err != nil { - logger.Errorf("failed to build the endorsement allowlist: %v", err) - - return + return errors.Wrap(err, "failed to build the endorsement allowlist") } responder, err := factory.NewResponder(authorizer, signer, d.resolveTMS) if err != nil { - logger.Errorf("failed to build the endorsement responder: %v", err) - - return + return errors.Wrap(err, "failed to build the endorsement responder") } if err := endorsement.RegisterEndorser(d.viewRegistry, responder); err != nil { - logger.Errorf("failed to register the endorsement responder: %v", err) - - return + return errors.Wrap(err, "failed to register the endorsement responder") } d.registeredFor = networkKey logger.Infof("registered as the endorser for [%s] with address %s", networkKey, signer.Address()) + + return nil } // resolveIdentity turns a configured node name into the identity that node speaks with. diff --git a/x/token/services/network/evm/driver_test.go b/x/token/services/network/evm/driver_test.go index 58df8aeca4..cec410f236 100644 --- a/x/token/services/network/evm/driver_test.go +++ b/x/token/services/network/evm/driver_test.go @@ -245,3 +245,22 @@ func TestRegisterEndorserSkipsANonEndorsingNetwork(t *testing.T) { assert.Equal(t, 1, registry.calls) assert.Equal(t, "network-a:", d.registeredFor) } + +// TestRegisterEndorserReturnsAnErrorForABrokenKey is the regression test for the finding that a node +// explicitly configured as an endorser, but whose signing key cannot be loaded, used to register +// nothing and only log the failure: nothing told the caller registration never happened, so +// installEndorsement and Driver.New both reported success regardless. A broken endorser answers no +// requests, which looks identical to ordinary network trouble from the outside and was otherwise +// discoverable only once a quorum it was needed for timed out. +func TestRegisterEndorserReturnsAnErrorForABrokenKey(t *testing.T) { + registry := &fakeViewRegistry{} + d := &Driver{viewRegistry: registry, identities: fakeIdentityProvider{}} + config := endorserConfig(t) + config.Endorser.Keystore = "" // unusable: LoadKey rejects an empty path + factory := testServiceFactory(t, config) + + err := d.registerEndorser("network-a:", factory, config) + require.Error(t, err) + assert.Zero(t, registry.calls, "a broken key must not reach the view registry") + assert.Empty(t, d.registeredFor, "a failed attempt must not mark the network as registered") +} From 020df64c98bf6c122a5c8a3f2d1705c71dd5205c Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sun, 16 Aug 2026 13:13:54 +0530 Subject: [PATCH 33/35] test(evm): reproduce the multi-TMS cross-contamination bug live Deferred finding #3 (configNetworkResolver.ConfigFor picks one Config for the first TMS to declare an EVM network/channel; Provider memoizes one *Network per (network, channel) with the namespace left out of the key) was confirmed by reading the code, not by running it. This drives the actual Driver.New/Network.Connect/QueryTokens/Broadcast entry points with two TMS on one network, each with its own TokenState address, and observes live that TMS B's reads, its submitter, and its EIP-712 signing domain all silently target TMS A's TokenState. Kept as the regression test for whenever Config gets split into network-shared and per-TMS parts: it currently asserts the contamination and should flip to asserting isolation once that lands. Signed-off-by: atharrva01 --- .../evm/multitms_cross_contamination_test.go | 292 ++++++++++++++++++ 1 file changed, 292 insertions(+) create mode 100644 x/token/services/network/evm/multitms_cross_contamination_test.go diff --git a/x/token/services/network/evm/multitms_cross_contamination_test.go b/x/token/services/network/evm/multitms_cross_contamination_test.go new file mode 100644 index 0000000000..40514681f7 --- /dev/null +++ b/x/token/services/network/evm/multitms_cross_contamination_test.go @@ -0,0 +1,292 @@ +/* +Copyright IBM Corp. All Rights Reserved. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package evm + +// Round 4, target #4: multi-TMS cross-contamination, built and observed live. +// +// This is the regression test for deferred finding #3 (BUG_HUNT_PROMPT.md's "known, deliberately +// deferred" list): configNetworkResolver.ConfigFor (driver.go:479-492) picks one *Config for the +// first TMS to declare an EVM network/channel, and token/services/network/network.go's Provider +// memoizes one *Network per (network, channel) with the namespace NOT part of the memoization key +// (network.go:344-357, 365, 428-448 in the repo root module). Every other TMS sharing that +// network/channel is therefore routed, silently, through the first TMS's Contracts.TokenState, +// EndorsementVerifier/EIP-712 domain, Submitter and Gas policy. +// +// KEEP THIS TEST. It is not a throwaway: it is meant to start failing (RED) the moment the +// eventual fix splits Config into network-shared vs. per-TMS parts, at which point it should be +// updated to assert isolation instead of contamination. + +import ( + "math/big" + "reflect" + "testing" + "unsafe" + + token2 "github.com/LFDT-Panurus/panurus/token" + "github.com/LFDT-Panurus/panurus/token/services/config" + "github.com/LFDT-Panurus/panurus/token/token" + "github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LFDT-Panurus/panurus/x/token/services/network/evm/client" + "github.com/LFDT-Panurus/panurus/x/token/services/network/evm/client/mock" + "github.com/LFDT-Panurus/panurus/x/token/services/network/evm/eip712" +) + +// crossTMSResolver simulates configNetworkResolver's real behavior (driver.go:431-497) for two TMS +// sharing one network/channel, each carrying its own EVM configuration with a distinct TokenState. +// +// ConfigFor mirrors configNetworkResolver.ConfigFor's loop exactly: it walks the TMS in declared +// order and returns the FIRST one's *Config for the network/channel -- there is no way for it to +// take a TMS/namespace as input at all, which is the root of the bug. configForCalls counts how many +// times it was asked, so the test can prove Driver.New resolves configuration exactly once for the +// pair, not once per TMS. +type crossTMSResolver struct { + network, channel string + // order lists the TMS ids in the order configNetworkResolver.ConfigFor would encounter them + // while walking config.Service.Configurations(); order[0] is "the first TMS to declare it". + order []token2.TMSID + configs map[token2.TMSID]*Config + configForCalls int +} + +func (r *crossTMSResolver) IsEVMNetwork(network, channel string) bool { + return network == r.network && channel == r.channel +} + +func (r *crossTMSResolver) TMSIDsFor(network, channel string) []token2.TMSID { + if !r.IsEVMNetwork(network, channel) { + return nil + } + + return append([]token2.TMSID(nil), r.order...) +} + +func (r *crossTMSResolver) ConfigFor(network, channel string) (*Config, error) { + r.configForCalls++ + if !r.IsEVMNetwork(network, channel) { + return nil, errors.Errorf("no evm configuration for [%s:%s]", network, channel) + } + + return r.configs[r.order[0]], nil +} + +func (r *crossTMSResolver) ConfigurationFor(tmsID token2.TMSID) (*config.Configuration, error) { + return nil, errors.Errorf("no configuration for [%s]", tmsID) +} + +// driverNewWithClient reproduces Driver.New's body (driver.go:144-180) line for line, with exactly +// one substitution: the real client.NewJSONRPCClient(config.Endpoint, nil) call is replaced with an +// already-constructed client.EVMClient (a *mock.EVMClient in this test). +// +// New has no seam to inject a client -- it always dials config.Endpoint for real -- and this test +// needs to observe every chain-facing call the constructed Network, Submitter and endorsement +// factory make, which a live TCP dial cannot give it. Every other line -- resolving configuration +// through the resolver, newSubmitter, NewNetwork, installEndorsement, watchPublicParams, the +// recovery-starter wiring -- is copied unchanged from New, and it is exactly that part the test +// exercises; only the transport, which the cross-TMS routing bug has nothing to do with, differs. +func driverNewWithClient(d *Driver, network, channel string, evmClient client.EVMClient) (*Network, error) { + if !d.resolver.IsEVMNetwork(network, channel) { + return nil, errors.Errorf("evm: no evm network configuration for [%s:%s]", network, channel) + } + config, err := d.resolver.ConfigFor(network, channel) + if err != nil { + return nil, err + } + submitter, err := d.newSubmitter(config, evmClient) + if err != nil { + return nil, err + } + n, err := NewNetwork(network, config, evmClient, nil, submitter, d.membership) + if err != nil { + return nil, err + } + if err := d.installEndorsement(n, config, evmClient, network, channel); err != nil { + return nil, err + } + d.watchPublicParams(network, channel, config, evmClient) + n.SetRecoveryStarter(func(ns string) error { + return d.startRecovery(token2.TMSID{Network: network, Channel: channel, Namespace: ns}, n) + }) + + return n, nil +} + +// extractDomain reads the live, unexported eip712.Domain field off a *endorsement.Service returned +// through the EndorsementService interface, via reflection. This is test-only introspection -- no +// production code is touched or needs to be -- used because endorsement.Service.domain has no +// exported accessor and the point of this test is to observe the actual object the driver built for +// TMS B, not to re-derive what it "should" contain. +func extractDomain(t *testing.T, svc EndorsementService) eip712.Domain { + t.Helper() + v := reflect.ValueOf(svc) + require.Equal(t, reflect.Pointer, v.Kind(), "expected a pointer-backed *endorsement.Service") + v = v.Elem() + f := v.FieldByName("domain") + require.True(t, f.IsValid(), "endorsement.Service is expected to carry an unexported 'domain' field") + f = reflect.NewAt(f.Type(), unsafe.Pointer(f.UnsafeAddr())).Elem() + domain, ok := f.Interface().(eip712.Domain) + require.True(t, ok, "domain field was not an eip712.Domain") + + return domain +} + +// TestMultiTMSCrossContamination_Live builds two TMS on one EVM network/channel, each with its own, +// different TokenState clone, drives them through the actual production entry points (Driver.New +// once, Network.Connect per TMS -- confirmed below by call-counting rather than assumed), and +// observes -- live, on the constructed objects, not by re-reading source -- which TokenState address +// ends up backing TMS B's reads, its submitter, and its EIP-712 signing domain. +func TestMultiTMSCrossContamination_Live(t *testing.T) { + const network = "evm-net" + const channel = "" + + tmsA := token2.TMSID{Network: network, Channel: channel, Namespace: "tms-a"} + tmsB := token2.TMSID{Network: network, Channel: channel, Namespace: "tms-b"} + + // Two TMS, two genuinely different TokenState clones. If Config were split per TMS the way the + // eventual fix needs to, these would never be conflated. + addrA, err := client.HexToAddress("0x" + repeat("aa", 20)) + require.NoError(t, err) + addrB, err := client.HexToAddress("0x" + repeat("bb", 20)) + require.NoError(t, err) + + configA := validConfig() + configA.Contracts.TokenState = addrA.Hex() + configA.Submitter = SubmitterConfig{Keystore: writeKey(t, testKeyHex), Address: testKeyAddress} + configA.applyDefaults() + require.NoError(t, configA.Validate()) + + configB := validConfig() + configB.Contracts.TokenState = addrB.Hex() + // Give B a materially different endorsement policy too (a config for a totally independent + // deployment, not a typo of A's), so nothing about this test depends on A and B being + // accidentally similar. + configB.Endorsement.Endorsers = []EndorserBinding{ + {Address: "0x9e5f4552091a69125d5dfcb7b8c2659029395bde", FSCIdentity: "endorser-b"}, + } + configB.applyDefaults() + require.NoError(t, configB.Validate()) + + resolver := &crossTMSResolver{ + network: network, + channel: channel, + order: []token2.TMSID{tmsA, tmsB}, // tms-a declares the network first + configs: map[token2.TMSID]*Config{tmsA: configA, tmsB: configB}, + } + + evmClient := &mock.EVMClient{} + evmClient.ChainIDReturns(big.NewInt(testChainID), nil) + + d := &Driver{ + resolver: resolver, + identities: fakeIdentityProvider{}, + viewManager: fakeViewManager{}, + } + + // --- Step 1: the real per-(network,channel) entry point -------------------------------------- + // + // token/services/network/network.go's Provider memoizes one *Network per (network,channel) via + // lazy.NewProviderWithKeyMapper(key, ms.newNetwork), keyed on netId{network,channel} alone (see + // Provider.networks / netId / key() in that file) -- the TMS/namespace is not part of the key. So + // networkProvider.newNetwork calls d.New(network, channel) exactly ONCE for this pair, no matter + // how many TMS share it, and the resulting *Network is reused by every one of them. + // + // Reproduce that here and confirm it: Driver.New must resolve configuration exactly once. + n, err := driverNewWithClient(d, network, channel, evmClient) + require.NoError(t, err) + require.NotNil(t, n) + assert.Equal(t, 1, resolver.configForCalls, + "Driver.New must resolve the network's configuration exactly once: in production this call "+ + "happens once per (network,channel), memoized, regardless of how many TMS share it -- there "+ + "is no per-TMS resolution to observe here because none exists") + + // --- Step 2: the real per-TMS entry point ----------------------------------------------------- + // + // Provider.Connect walks every configured TMS and, for each, calls GetNetwork(tmsID.Network, + // tmsID.Channel).Connect(tmsID.Namespace) -- GetNetwork returns the SAME memoized *Network for + // both TMS A and TMS B, so Connect is what runs per TMS, on one shared object. Network.Connect + // (network.go:140-164) only checks reachability/chain id and starts recovery; it does not + // rebuild, re-scope or namespace anything about the reader, submitter or endorsement factory. + _, err = n.Connect(tmsA.Namespace) + require.NoError(t, err) + _, err = n.Connect(tmsB.Namespace) + require.NoError(t, err) + + // --- Observation 1: reads -------------------------------------------------------------------- + // + // QueryTokens takes a namespace argument (network.go:359) but never uses it to pick a + // TokenState: it reads through n.reader, built once in NewNetwork from whichever config ConfigFor + // resolved. Ask for TMS B's namespace and see whose contract actually gets called. + evmClient.CallReturns(abiBytes([]byte("owned-by-a")), nil) + out, err := n.QueryTokens(t.Context(), tmsB.Namespace, []*token.ID{{TxId: anchorHex(0x01), Index: 0}}) + require.NoError(t, err) + require.Len(t, out, 1) + assert.Equal(t, []byte("owned-by-a"), out[0]) + + require.Equal(t, 1, evmClient.CallCallCount()) + _, calledTo, _, _ := evmClient.CallArgsForCall(0) + assert.Equal(t, addrA, calledTo, + "QueryTokens for the tms-b namespace silently reads through TMS A's TokenState clone") + assert.NotEqual(t, addrB, calledTo, "it must not be tms-b's own configured TokenState") + + // --- Observation 2: the submitter -------------------------------------------------------------- + // + // There is exactly one Submitter for the whole Network, built once in Driver.New/newSubmitter + // from the same single config. Broadcast (network.go:264) does not even take a TMS/namespace + // argument, so there is structurally no way for it to route TMS B's transaction anywhere but + // through that one submitter, targeting whatever TokenState it was built with. + evmClient.PendingNonceAtReturns(3, nil) + evmClient.EstimateGasReturns(100_000, nil) + evmClient.SuggestGasFeesReturns(client.GasFees{ + MaxFeePerGas: big.NewInt(20_000_000_000), + MaxPriorityFeePerGas: big.NewInt(1_000_000_000), + }, nil) + evmClient.SendRawTransactionReturns(client.Hash{}, nil) + + env := &Envelope{ + Anchor: anchorHex(0xA1), + Delta: testDelta(), + Endorsements: [][]byte{make([]byte, 65)}, + } + require.NoError(t, n.Broadcast(t.Context(), env)) // this stands in for TMS B's own broadcast + + require.Equal(t, 1, evmClient.EstimateGasCallCount()) + _, estimateMsg := evmClient.EstimateGasArgsForCall(0) + require.NotNil(t, estimateMsg.To) + assert.Equal(t, addrA, *estimateMsg.To, + "the shared submitter targets TMS A's TokenState for a transaction that has nothing to do with TMS A") + assert.NotEqual(t, addrB, *estimateMsg.To) + + // --- Observation 3: the EIP-712 domain ---------------------------------------------------------- + // + // installEndorsement (driver.go:266-320) builds ONE endorsement.ServiceFactory carrying ONE + // eip712.Domain{VerifyingContract: } (driver.go:277-285), from the + // exact same config object NewNetwork used. ServiceFactory.ForTMS (endorsement/esp.go:104-141) + // only varies the resolved RequestValidator per TMS id; domain, tokenState, client, registry and + // threshold are all single fields on the factory, reused unchanged for every TMS it ever builds a + // Service for. Get the actual *endorsement.Service the driver built for TMS B and read its live + // domain rather than re-deriving what it "should" be. + svcB, err := n.endorsementForID(tmsB) + require.NoError(t, err) + domainB := extractDomain(t, svcB) + assert.Equal(t, addrA, domainB.VerifyingContract, + "TMS B's endorsement service signs and verifies EIP-712 digests against TMS A's TokenState address") + assert.NotEqual(t, addrB, domainB.VerifyingContract) + assert.Equal(t, configA.ChainIDBig().String(), domainB.ChainID.String()) +} + +// repeat returns s repeated n times, a tiny local helper so the two test addresses above are +// visibly-constructed, easy-to-eyeball hex strings instead of opaque literals. +func repeat(s string, n int) string { + out := make([]byte, 0, len(s)*n) + for range n { + out = append(out, s...) + } + + return string(out) +} From 202ad4611b385e9481743bc4bf7cfadb996e5e64 Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Fri, 14 Aug 2026 23:07:22 +0530 Subject: [PATCH 34/35] lint(evm): waive containedctx where an interface forces the field #2180 enabled containedctx and fixed the ttx occurrence. The evm module is a separate Go module and was not linted in that pass, so make lint has been failing on it since. Neither field can be dropped. Ledger.ctx exists because driver.GetStateFnc passes no context to GetState, and fakeContext.ctx exists because it implements view.Context, whose Context() method has to return one. Signed-off-by: atharrva01 --- x/token/services/network/evm/endorsement/fakes_test.go | 4 +++- x/token/services/network/evm/endorsement/ledger.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/x/token/services/network/evm/endorsement/fakes_test.go b/x/token/services/network/evm/endorsement/fakes_test.go index f3dda25e0c..7e94cc8c04 100644 --- a/x/token/services/network/evm/endorsement/fakes_test.go +++ b/x/token/services/network/evm/endorsement/fakes_test.go @@ -69,7 +69,9 @@ func (s *pipeSession) Close() {} // own session via Session. Everything the endorsement views do not touch panics, so an unexpected // dependency surfaces loudly rather than silently. type fakeContext struct { - ctx context.Context + // view.Context is an interface whose Context() method returns one, so an implementation of it has + // to hold a context. + ctx context.Context //nolint:containedctx me view.Identity sessions map[string]view.Session // keyed by party UniqueID, for GetSession (initiator side) own view.Session // for Session() (responder side) diff --git a/x/token/services/network/evm/endorsement/ledger.go b/x/token/services/network/evm/endorsement/ledger.go index f358722d21..c2d9c107bf 100644 --- a/x/token/services/network/evm/endorsement/ledger.go +++ b/x/token/services/network/evm/endorsement/ledger.go @@ -34,7 +34,9 @@ const getTokenMethod = "getToken(bytes32)" // #nosec G101 -- ABI method signatur // Ledger satisfies token.Ledger, so it can be passed straight to // Validator.UnmarshallAndVerifyWithMetadata. type Ledger struct { - ctx context.Context + // GetState takes no context (driver.GetStateFnc has none), so the one to read the chain with is + // captured here. A Ledger is built per request and used only for it, so it stays short-lived. + ctx context.Context //nolint:containedctx client client.EVMClient tokenState client.Address blockTag string From fe2611ae3dd7720a426a7aaee94bc71b015d0aeb Mon Sep 17 00:00:00 2001 From: atharrva01 Date: Sun, 16 Aug 2026 13:29:30 +0530 Subject: [PATCH 35/35] fix(evm): check registerEndorser's error in the existing test calls Making registerEndorser return an error (the earlier commit in this PR, "propagate a broken endorser registration instead of only logging it") turned every unchecked call to it in driver_test.go into an errcheck violation. golangci-lint's max-same-issues default (3) only surfaced the first three in CI, hiding the rest until those were fixed - checked every call site in the file, not just the reported ones, to avoid a second round trip. Signed-off-by: atharrva01 --- x/token/services/network/evm/driver_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/x/token/services/network/evm/driver_test.go b/x/token/services/network/evm/driver_test.go index cec410f236..9767178de7 100644 --- a/x/token/services/network/evm/driver_test.go +++ b/x/token/services/network/evm/driver_test.go @@ -206,11 +206,12 @@ func TestRegisterEndorserRefusesASecondNetwork(t *testing.T) { config := endorserConfig(t) factory := testServiceFactory(t, config) - d.registerEndorser("network-a:", factory, config) + require.NoError(t, d.registerEndorser("network-a:", factory, config)) assert.Equal(t, 1, registry.calls, "the first network must register") assert.Equal(t, "network-a:", d.registeredFor) - d.registerEndorser("network-b:", factory, config) + require.NoError(t, d.registerEndorser("network-b:", factory, config), + "a second network is refused loudly via a log line, not an error - see registerEndorser's doc comment") assert.Equal(t, 1, registry.calls, "a second, different network must not overwrite the registration") assert.Equal(t, "network-a:", d.registeredFor, "the first network's registration must stand") } @@ -223,8 +224,8 @@ func TestRegisterEndorserIsIdempotentForTheSameNetwork(t *testing.T) { config := endorserConfig(t) factory := testServiceFactory(t, config) - d.registerEndorser("network-a:", factory, config) - d.registerEndorser("network-a:", factory, config) + require.NoError(t, d.registerEndorser("network-a:", factory, config)) + require.NoError(t, d.registerEndorser("network-a:", factory, config)) assert.Equal(t, 1, registry.calls, "registering the same network twice must not re-register") } @@ -237,10 +238,10 @@ func TestRegisterEndorserSkipsANonEndorsingNetwork(t *testing.T) { d := &Driver{viewRegistry: registry, identities: fakeIdentityProvider{}} endorsing := endorserConfig(t) factory := testServiceFactory(t, endorsing) - d.registerEndorser("network-a:", factory, endorsing) + require.NoError(t, d.registerEndorser("network-a:", factory, endorsing)) notEndorsing := validConfig() // Endorser.Enabled defaults to false - d.registerEndorser("network-b:", factory, notEndorsing) + require.NoError(t, d.registerEndorser("network-b:", factory, notEndorsing)) assert.Equal(t, 1, registry.calls) assert.Equal(t, "network-a:", d.registeredFor)