fix(core): resolve the first usable public parameters, not the first retrievable

[AkramBitar]

Fixes #2281

Problem

Old behaviour — stops at the first source that returns bytes, not at the first usable one:

1. Try options       -> got bytes -> pass to driver -> driver rejects (wrong version)
                                                      |
                                                      v
                                                 STOP. Return error.
                                                 Storage / config / ledger never tried.

newTMS (token/core/tms.go) resolved the public parameters and instantiated the token service as two independent steps: loadPublicParams walked the four sources (caller options -> local storage -> local configuration -> network fetcher) and returned the bytes of the first one that had any, and a single NewTokenService call then decided pass or fail for the whole TMS.

So the walk moved on only when a source returned no bytes — never when a source returned bytes the driver rejects (driver name/version skew such as invalid identifier, expecting [dlog.v1], got [dlog.v2], an unparsable container, an out-of-range curve id, ...). A retrievable but unusable copy permanently shadowed every lower-priority source, including the authoritative one on the ledger.

Result: a stale or corrupted local copy permanently blocks the authoritative copy on the ledger. During a public parameters upgrade — where the on-chain parameters are updated before the nodes are — the affected TMS cannot be instantiated at all, and cannot recover without manual configuration or database intervention.

Diagnosis was equally hard: only one source ever reached the driver, and the retrieval failures of the earlier ones were Warnf'd and dropped, so nothing identified which source supplied the offending bytes.

Fix

New behaviour — retrieve and try each source in order, first accepted wins:

1. Try options       -> got bytes -> driver rejects  -> skip, try next
2. Try storage       -> got bytes -> driver rejects  -> skip, try next
3. Try config file   -> got bytes -> driver rejects  -> skip, try next
4. Try ledger        -> got bytes -> driver ACCEPTS  -> use these, done

If all 4 produced bytes, none usable  -> error "failed to instantiate token service"
                                         (deliberately NOT ErrTMSNotFound)
If no source produced any bytes       -> ErrTMSNotFound ("TMS not set up yet")

For each source, newTMS retrieves the public parameters and immediately tries to instantiate the token service with them, inside the loop. The first source the driver accepts wins. A source that yields nothing, fails to yield, or yields public parameters the driver rejects falls through to the next one. loadPublicParams is gone.

• Named sources (ppSource{name, retrieve}: options, storage, configuration, fetcher) so every failure is attributable; the returned error aggregates each source's failure, tagged with the source name.
• ErrTMSNotFound semantics preserved — returned if and only if no source produced any bytes ("TMS not set up yet"), never when a source produced unusable bytes. It is now decided by an explicit count of the sources that produced bytes. The callers that branch on it (token/services/network/fabric/endorsement/fsc) are unaffected.
• No duplicate work: public parameters byte-identical to a blob a higher-priority source already tried are not deserialized twice, and the fetcher is still never called when a higher-priority source succeeds.
• newTokenService converts a panic raised while deserializing stale or attacker-controlled public parameters into an error, and logs the stack at the recover site. Without it, one panicking blob would still shadow the authoritative public parameters — the same failure mode.
• Walking every source reaches two paths a short-circuiting source used to hide: ppFromConfig dereferenced a nil driver.Configuration returned with a nil error (the pre-existing test even carried a "To avoid panic in ppFromConfig" comment), and ppFromStorage a nil storage. Both are now reported as errors.

Bonus fix (commit 2) — the Update path

This is not main's behaviour — the two are not the same. On main, Update always won on source 1: options is first in the priority order and an update always carries public parameters, so main never consulted storage on this path, never fell back, and was never destructive (service.Done() sits behind if err == nil). The fallback described below was introduced by commit 1 of this PR, which routed Update through the new walk. Commit 2 removes it. It is a regression caught in review, not a pre-existing bug.

After commit 1: Update(newPP) -> driver rejects newPP -> falls through to local storage -> builds a service from an older copy -> reports success, having already unloaded the service that was running. Reached from the ledger setup listener (token/services/network/fabric/network.go), that turned a node which merely needed a binary upgrade into a node silently running on superseded public parameters, with ManagementServiceProvider.Update also shutting down the selector manager and clearing its cache on the way. On the update path the fallback cannot even reach the ledger, because the service options an update is built from carry no PublicParamsFetcher — so it could only ever land on a stale local copy, the opposite of what the walk is for.

After commit 2: Update(newPP) -> driver rejects newPP -> fails, and leaves the running service untouched. Update resolves from source 1 alone (updatePublicParamsSources) and accepts only what it was given. No fallback.

Also in commit 2:

• the panic's stack is kept. newTokenService turns a panic into an error and the walk moves on, so the stack was lost and nothing downstream could say where a driver panicked. It is now logged at the recover site.
• ErrTMSNotFound is decided by a count, not inferred from len(instantiationErrs). The previous form relied on "a duplicate implies an earlier failed attempt", an invariant nothing enforces.
• the write to opts.PublicParams is gone. Every caller passes a pointer to its own by-value copy, so the documented post-condition was unobservable.

Tests

TestTMSProviderPublicParamsFallback in token/core/tms_test.go, 9 sub-cases:

• fallback to a usable source when options / storage / configuration hold unusable public parameters;
• a driver panic on one source does not abort the walk;
• identical public parameters held by two sources are tried once;
• all sources unusable -> error names every source and is not an ErrTMSNotFound;
• no source holds any public parameters -> ErrTMSNotFound, and every source's reason is reported;
• a nil configuration returned with a nil error does not panic;
• a failed resolution is not cached, so a later call succeeds once the network serves usable public parameters.

These sub-cases were verified to fail against the pre-fix newTMS (four fail outright, the panic case aborts the run) and to pass after it.

For the Update path: TestTMSProviderUpdateRejectsUnusablePublicParams (the update fails, and never reads storage or queries the network) and TestTMSProviderUpdateKeepsTheCachedServiceOnFailure (the running service is neither unloaded nor evicted). These replace TestTMSProviderUpdateFallback, which asserted the behaviour commit 1 had introduced.

TestTMSProviderConcurrentGet covers 32 concurrent resolvers, asserting the TMS is instantiated once and the network queried once.

Verification

• go test ./token/core/ passes, also under -race.
• golangci-lint run token/core/... -> 0 issues; make checks -> exit 0.
• go test ./token/... shows one unrelated pre-existing failure, TestTranslatePath in token/services/identity/config: it asserts the checkout path contains "panurus" and the local worktree is named differently. It fails identically on unmodified main.

Docs

docs/public_parameters.md gains a "Resolution Order" section under Discovery and Fetching, documenting the four sources, the retrieve-and-try-in-order semantics, and the error/ErrTMSNotFound contract — plus a "Resolution vs. update" subsection stating that the walk does not apply to Update, and a note that source 4 requires a configured PublicParamsFetcher.

Known limitation (not addressed here)

GetTokenManagerService holds the provider-wide write lock across the whole walk, which may now include the ledger fetch, so a hung fetch blocks TMS lookups for every namespace on the node. This is pre-existing — the fetcher was always reachable under that lock — but this change makes reaching it more likely. Fixing it means per-key creation locking, which changes when the post-init callback can run concurrently, so it is tracked separately in #2306 rather than folded in here.

[adecaro]

Review: does this fully address #2281?

@adecaro

Approve — the fix does exactly what #2281 asks: newTMS now walks every public-parameters source and tries to instantiate with each one instead of stopping at the first source that merely retrieved bytes.

Checked out 2281_newtms_first_usable_pp at fab0226e and verified:

$ go build ./...                                          # clean
$ go vet ./token/core/                                     # clean
$ gofmt -l -s token/core/tms.go token/core/tms_test.go     # clean
$ go test -count=1 ./token/core/                           # ok, coverage: 96.8% of statements
$ go test -race -count=1 ./token/core/...                  # ok (all subpackages)
$ golangci-lint run ./token/core/...                        # 0 issues in changed files

I also grepped every caller of TMSProvider.Update/newTMS/ErrTMSNotFound across the repo (token/services/network/fabric/endorsement/fsc/service.go, responder.go) to confirm none of them pattern-match on the old error text or rely on newTMS mutating opts.PublicParams — none do, and opts is passed by value into the exported methods anyway, so that mutation was never externally visible even pre-PR.

Reverting token/core/tms.go to bdcd99d (pre-PR) and re-running TestTMSProviderPublicParamsFallback fails four of its subtests, and one panics uncaught (panic: invalid curve id, since base has no recover()), confirming the tests pin the bug rather than the diff. I also checked out the intermediate commit 2c78ccf2f (commit 1 only, without commit 2's Update-specific restriction) and confirmed TestTMSProviderUpdateRejectsUnusablePublicParams fails there — commit 1 alone would let Update silently fall back to stale storage; commit 2 exists specifically to close that. Both commits are load-bearing and correctly ordered.

checks, utest, itest (base is main, so the full matrix ran), bench-check, cgo-check, CodeQL, and DCO are all green at fab0226e.

What it fixes

#	Finding	Verified
2281.1	newTMS committed to the first source that returned bytes, even if the driver then rejected them, instead of trying the next source	✅ traced in newTMS (tms.go:279-346): retrieval and instantiation now happen per-source inside the same loop iteration, and a rejection continues the walk
2281.2	A stale/malformed local copy (storage or config) could shadow the authoritative network fetcher	✅ TestTMSProviderPublicParamsFallback/unusable_storage_public_params_fall_back_to_the_fetcher + my own revert test above
2281.3	Failures from skipped sources were not surfaced, making the eventual error unhelpful	✅ retrievalErrs/instantiationErrs are aggregated via errors.Join and every source's failure is present in the final error (TestTMSProviderPublicParamsSourceErrors)

All three rows check out; no gaps.

Non-blocking

• Design note: GetTokenManagerService/Update hold the single provider-wide m.lock (not one per TMS key) for the entire newTMS walk. Before this PR that walk normally did one retrieval; now, in the recovery scenario this fix targets, it can do several driver deserializations plus a network fetch, all while every other network/channel/namespace key is blocked. Not a regression from this PR (the coarse lock predates it) and the trade-off is reasonable for a rare path — flagging in case a future PR wants per-key lock granularity if this walk gets exercised more than occasionally.
• Nice catch, not a finding: the PR incidentally fixes a pre-existing typo in ppFromStorage's error string ("no public parames found" → "no public params found"), and the test suite (priority matrix, concurrency, dedup-by-digest, Update keeping the cached service on a rejected update) goes well beyond what #2281 asked for.

Recommendation

Nothing to change before merge. The Design note above is worth a one-line mention in docs/public_parameters.md's existing "Resolution vs. update" section if you want to preempt the question later, but it's not blocking.

[adecaro]

LGTM