fix(fabricx): dynamically fetch namespace version for PP deployment - #2294
fix(fabricx): dynamically fetch namespace version for PP deployment#2294SurbhiAgarwal1 wants to merge 11 commits into
Conversation
7cd8203 to
0230bc9
Compare
There was a problem hiding this comment.
Thanks for taking this on — fetching the real namespace version is the right fix for #2256, and the code reads cleanly.
My main concern is that the version can still silently end up as 0, which is exactly the bug being fixed:
- Two silent fallbacks to 0 — a failed type assertion and a swallowed fetch error (
deployer.go:126,:130). Either one reproduces #2256 with no diagnostics. - The test doesn't cover the fix — it never calls
deployPublicParametersRaw, so reverting the change leaves it green. - A second source of truth for
NsVersion—GetNamespacePolicies().Versionhere vsGetState("_meta", ns).Versioneverywhere else in the stack.
Details inline. 1 and 2 feel like blockers; 3 is worth a decision.
| func (s *deployerService) deployPublicParametersRaw(tmsID token.TMSID, ppRaw []byte) error { | ||
| tx, err := s.createPublicParametersTx(ppRaw, tmsID.Namespace) | ||
| var nsVersion uint64 | ||
| if fetcher, ok := s.ppFetcher.(interface { |
There was a problem hiding this comment.
The fix only fires if s.ppFetcher happens to satisfy this anonymous interface — there's no else and no log. ppFetcher is typed fabric.NetworkPublicParamsFetcher, and that interface exists precisely so other implementations can be plugged in (e.g. fabric.Driver.defaultPublicParamsFetcher, fabricx/driver.go:145, fabricx/network.go:44). The moment anything other than *pp.PublicParametersService is wired here, nsVersion silently stays 0 and #2256 is back with zero diagnostics.
Since NewTMSDeployerService already takes the concrete *pp.PublicParametersService, could we either type the field concretely, or add FetchNamespaceVersion to NetworkPublicParamsFetcher and drop the assertion? Then the compiler guarantees it.
| FetchNamespaceVersion(network cdriver.Network, channel cdriver.Channel, namespace cdriver.Namespace) (uint64, error) | ||
| }); ok { | ||
| ver, err := fetcher.FetchNamespaceVersion(tmsID.Network, tmsID.Channel, tmsID.Namespace) | ||
| if err != nil { |
There was a problem hiding this comment.
This downgrades a real failure to a warning and then submits with NsVersion: 0. On a transient query-service failure (gRPC timeout, committer restart, query service not yet reachable) against a chain whose policy version is >= 1, the committer invalidates the tx and the operator sees an opaque submit/finality error instead of the root cause.
I don't think the backward-compat fallback is needed: a namespace at its initial policy version already reports 0, so there's nothing to preserve. Suggest wrapping and returning the error instead.
| keyTranslator: &keys.Translator{}, | ||
| } | ||
|
|
||
| tx, err := s.createPublicParametersTx([]byte("raw-pp"), "test-ns", 2) |
There was a problem hiding this comment.
This test passes the literal 2 and then asserts NsVersion == 2, so it only proves that a parameter is copied into a struct field. mockFetcherWithVersion is assigned to ppFetcher but never consulted, because deployPublicParametersRaw — where the type assertion, the version plumbing and the error fallback all live — is never invoked.
Concretely: reverting deployPublicParametersRaw to pass 0 leaves this test green, so it can't catch a regression of #2256.
Could we drive deployPublicParametersRaw through a stub Submitter and assert NsVersion on the captured tx, plus cases for (a) FetchNamespaceVersion returning an error and (b) a fetcher that doesn't implement the method?
| } | ||
| } | ||
|
|
||
| return 0, nil |
There was a problem hiding this comment.
Falling out of the loop returns (0, nil), which makes "this namespace has no registered policy" indistinguishable from "version 0". If the TMS namespace was never registered on the chain (config typo, wrong channel, policy registration not yet committed), the deployer treats it as a legitimate version 0, builds and submits the tx, and it's rejected — with nothing anywhere pointing at the missing policy.
Suggest returning a distinguishable result (an error, or a found bool) and logging it, so a misconfigured namespace surfaces at deploy time.
| if err != nil { | ||
| return 0, errors.Wrapf(err, "failed getting query service") | ||
| } | ||
| policies, err := qs.GetNamespacePolicies() |
There was a problem hiding this comment.
This introduces a second source of truth for the same field. Everywhere else in this stack a fabricx TxNamespace.NsVersion is derived from queryService.GetState("_meta", ns).Version (FSC platform/fabricx/core/vault/vault.go, rwSetWrapper.Bytes() -> marshal.go:148); here it comes from GetNamespacePolicies() -> PolicyItem.Version. If those two counters ever diverge, PP deployment txs get invalidated again while ordinary token txs keep committing, which would be very hard to trace.
qs.GetState("_meta", namespace) is also a targeted single-key lookup rather than pulling every namespace's policy over gRPC, and it reuses the pattern already used in Fetch just above. Worth considering?
Nit while you're here: the if policies != nil guard below is dead — generated protobuf getters are nil-receiver-safe, so policies.GetPolicies() is already safe to range over.
|
Hi @AkramBitar, I've addressed all your feedback:
Thanks for the thorough review! |
Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
- Store ppFetcher as *pp.PublicParametersService (concrete type) so the
compiler guarantees FetchNamespaceVersion is always available — removes
the type assertion that silently fell back to NsVersion: 0
- Return error from FetchNamespaceVersion instead of swallowing it;
a transient query failure or misconfigured namespace now surfaces
immediately rather than submitting a doomed NsVersion: 0 tx
- Use qs.GetState('_meta', namespace) instead of GetNamespacePolicies()
to derive the version — same key and protowire varint encoding used by
the fabricx vault marshaller (single source of truth); also a targeted
single-key lookup rather than fetching all namespace policies over gRPC
- Return a distinguishable error when the namespace has no _meta entry
so a misconfigured namespace is caught at deploy time
- Remove dead 'if policies != nil' guard (protobuf getters are nil-safe)
- Rewrite deployer_test.go to drive deployPublicParametersRaw through a
stub Submitter and assert NsVersion on the captured tx; add cases for
FetchNamespaceVersion returning an error
Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
If a namespace policy update commits between the version fetch and the submit, the committer rejects the tx with a stale NsVersion. A single retry re-fetches the current version and resubmits, making the deployer robust to policy updates that land during a rolling rollout. Also adds a test asserting the retry re-fetches the version and uses the refreshed value in the retried transaction. Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
…ssing (LFDT-Panurus#2256) Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
…sient CI failures Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
… v1.62.2 Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
…pport target Go version Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
….24+) Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
46b776d to
2ee36a4
Compare
Signed-off-by: Surbhi Agarwal <SurbhiAgarwal1@users.noreply.github.com>
3 participants
Summary
This PR fixes a bug in
token/services/network/fabricx/tms/deployer.gowhere the public-parameters deployment transaction was hardcodingNsVersion: 0. This caused deployment transactions to be invalidated by the committer on chains where the token namespace endorsement policy had been updated at least once (e.g., after adding a new endorsing organization).Fix Details
PublicParametersServiceto implement a newFetchNamespaceVersionfunction which dynamically queries the current namespace version.deployPublicParametersRawto query and fetch the namespace's current version before building the deployment transaction.nsVersionintocreatePublicParametersTxinstead of hardcoding0.0if the version cannot be fetched, ensuring backward compatibility.deployer_test.goto ensureNsVersionis properly populated dynamically.Fixes #2256