Report a lagging name registration as pending, not Completed - #59
Open
requilence wants to merge 1 commit into
Open
requilence wants to merge 1 commit into
requilence wants to merge 1 commit into
Conversation
any-ns-node could tell the payment node that a name registration was Completed
while the name was not written to the name-service cache at all:
- cache.UpdateInCache returned nil when the ENS registry answered with a zero
owner address, so the caller believed the cache had been updated
- anynsaarpc.GetOperation published status.OperationState before that check, so
Completed was reported even when the cache update did nothing
The registry is read through a different RPC provider than the one that gives us
the user operation receipt, so it can lag behind for a fraction of a second right
after the registration is mined. That short lag turned into a permanent failure:
IsNameAvailable is cache-only, so the name kept reading as available forever, and
Completed is terminal for the payment node, so nothing ever retried.
- cache: add ErrNameNotRegistered and return it whenever the registry has no
owner for the name, instead of nil (zero address) or a raw error ("not found")
- anynsaarpc: report PendingOrNotFound instead of Completed when the cache was
not updated, so the payment node keeps polling
- anynsaarpc: retry the cache update 3 times, 1s apart, honouring ctx
cancellation. this only shortens the wait - correctness comes from the re-poll
- anynsrpc: a name missing from the registry is still "available", do not turn
it into an RPC error on the direct-read path
- contracts: pass ctx to the registry CallOpts, it was silently dropped
Operations with no row in Mongo are still reported as Completed without a cache
write. That path has no FullName, so a registration cannot be told apart from a
funding operation there; it now logs a warning.
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes GO-7482.
Summary
any-ns-nodecould report a name registration asCompletedto the payment node while the name was never written to the name-service cache. BecauseIsNameAvailableis cache-only, the name then read as available to every client forever, and becauseCompletedis terminal on the payment-node side, nothing ever retried. Users were charged and the name was permanently unusable.Two bugs combined:
cache.UpdateInCachereturnednil(success) when the ENS registry answered with a zero owner address, so the caller believed the cache had been updated.anynsaarpc.GetOperationpublishedstatus.OperationStatebefore that check, soCompletedwas reported even when the cache update did nothing.The trigger is a cross-provider race. The ERC-4337 receipt is polled through the Alchemy bundler, while the registry read goes to a different provider (
ConnectToRegistryContract()→ethclient.Dial(config.GethUrl)) at latest block. In the observed incident on nsnode1 the two reads were 99 ms apart and the registry provider had not caught up:Replaying the identical operation 48 days later returned the real owner and wrote the cache row with
name_expiresexactly 12 months after the original attempt, proving the name had been on chain the entire time. A sub-second provider lag became a permanent failure purely because the failure branch returnednil.Changes:
cache— add an exportedErrNameNotRegisteredsentinel and return it whenever the registry has no owner for the name: both on the zero-address read and on the"not found"error path, which previously returned a raw error. Both spellings now mean one thing to callers.anynsaarpc— when the cache update fails with that sentinel, reportnsp.OperationState_PendingOrNotFoundinstead ofCompleted, so the payment node keeps polling. Every other error keeps the existingfailed to update in cachebehaviour.anynsaarpc— retry the cache update 3 times, 1 s apart, honouringctxcancellation. This is a latency optimization only. The correctness mechanism is the payment node's re-poll, which handles a lag of any length; the retry just avoids waiting a whole poll interval for the common sub-second case.anynsrpc— a name missing from the registry is still available. Without this,IsNameAvailableon thereadFromCache: falsepath would turn every free name into an RPC error.contracts—GetOwnerForNamehashbuiltbind.CallOpts{}with a nilContext, so go-ethereum fell back tocontext.Background()and the caller's ctx was silently discarded. Pre-existing, but the retry is what makes it matter: without it a hung provider is unbounded and cancellation is honoured only between attempts.The retry deliberately lives in
anynsaarpc.GetOperationand not inUpdateInCache.UpdateInCache's other caller isanynsrpc.IsNameAvailable, where on thereadFromCache: falsepath every free name — the common answer during signup — would otherwise cost 3 registry RPCs and 2 s of in-request sleep.BatchIsNameAvailableloops sequentially, so a 10-name batch would go from sub-second to ~20 s, and there is no server-side DRPC handler timeout or client deadline, so it would hang rather than error. Production isreadFromCache: truetoday, but that flag is a candidate for another fix, so this must not be left as a landmine.The retry knobs are constants, not config. A missing YAML key would leave the count at
0, the loop would never run,addrwould stay zero and every registration would report "not registered" — a total cache-population outage from an absent key. The delay is avarrather than aconstonly so tests can shrink it.Why
PendingOrNotFoundand not an errorBoth options avoid the
Completedlatch, but they are not equivalent. Verified againstany-pp-nodeat77a4f78rather than reasoning from the enum alone:periodic/periodic_common.go:225-241processNSOperationPendingpersistsresp.OperationStateonly in theErrorbranch.Completedgoes terminal,Erroris stored asError, and anything else —PendingOrNotFoundincluded — falls through, returnsnil, and the stored op state staysPending. The item stays inStatusPendingNSOperationandProcessNSOperationre-handlesPendingon every tick. SoPendingOrNotFoundis never stored and cannot latch.processItems→increaseRetryCountOnError(periodic/periodic.go:76-107, and the v2 twin atperiodic/periodic_v2.go:116-152), bumpingOpsRetryCount. AtmaxRetries = 5the subscription/user is forced todb.StatusErrorwith a Sentry capture and a Slack alert — and once out ofStatusPendingNSOperationit is no longer picked up bysearchPendingNS. That is a different terminal latch needing manual recovery, reached after only five polls of a condition that is expected and self-healing.So the error path would trade a silent permanent failure for a noisy one.
PendingOrNotFoundgives unbounded, cost-free polling and keeps the operation id in the response. Genuine infrastructure failures (Mongo write errors,GetAdditionalNameInfofailures) still return an error, which is exactly where the retry-count and alert escalation belongs.Known remaining gap
GetOperationstill reportsCompletedwith no cache write when the operation has no row in Mongo (anynsaarpc.go:124,143).db.GetOperationreturnsmongo.ErrNoDocumentsverbatim, so on a restored backup or a wiped collection the whole cache block is skipped — byte-for-byte the GO-7482 symptom.This is deliberately not forced to a non-
Completedstate: some operations legitimately have no Mongo row (AdminFundUserAccount), and with the row absent there is noFullNameto tell a registration from a funding operation. It now emits alog.Warnso the case is at least visible.Test plan
go build ./...— cleango test -count=1 -p 1 ./...— all packages passcache: zero-address owner returnsErrNameNotRegisteredand writes nothing to Mongocache: the registry"not found"path returns the same sentinelcache: happy path still writes the cache row (existing tests)anynsaarpc: registry never catches up →PendingOrNotFound, no error,UpdateInCachecalled exactlyupdateCacheRetryCounttimesanynsaarpc: registry catches up on the second try →Completedanynsaarpc: closed context stops the retry after one attemptanynsaarpc: a non-sentinel cache failure is still an erroranynsrpc: a name missing from the registry reportsAvailable: truewith no errorNote: plain
go test ./...(packages in parallel) is flaky on this repo and on a cleanmain— every package fixture connects tomongodb://localhost:27017and drops the sameany-ns-testdatabase, so parallel packages wipe each other's data. Which test loses the race varies per run.-p 1is green on both. Unrelated to this change; listed as a follow-up.Follow-ups (not in this PR)
any-pp-nodeshould handlePendingOrNotFoundexplicitly. This fix's safety rests on an untested property of the consumer:processNSOperationPendinghappens not to persist the state, andProcessNSOperation's switch has nocasefor it — it would fall todefault: "NS operation state not supported". A future "record the state we got" cleanup there would silently convert this fix into a massStatusErrorincident. Recommend an explicitcase OperationState_PendingOrNotFound: return nilplus a test on the pp-node side.AddrRegistry, orGethUrlpointed at the wrong chain, now yields unbounded silent polling: no Sentry, no Slack, no age check, and the user sits inStatusPendingNSOperation— a paid subscription that never activates, with no alarm. Pre-fix that produced "wrongly Completed"; post-fix it is "nothing works, quietly". Stating the tradeoff plainly: this PR removes a silent data-corruption bug and replaces its worst case with a silent stall. The escalation belongs on the pp-node side.IsNameAvailablefall back to the contract on a cache miss — wider blast radius, needs its own design decision.go test ./....anynsaarpc.go:405CreateUserOperationbuildsoutand thenreturn nil, nil, discarding it — callers get a nil response with no error.ns.cache.name(cache.go:49 // TODO: index it) allows a concurrent double-insert. Pre-existing and not widened by this change.