fix(relayer): harden sponsored registration and balance alerts - #671
Conversation
Style Guide AuditAudited 1 file(s) against the Sui Documentation Style Guide. 2 violation(s) found. All must be fixed before merge.
|
harrymove-ctrl
left a comment
There was a problem hiding this comment.
Request changes — 1 blocker, plus residual items from the #663 review
Reviewed against the blockers/majors recorded on the Alert gas task for #663. This PR closes most of them properly, and two fixes are exactly right: B2 (assertSponsoredRegisterTransactionKind re-serialises the returned kind and requires byte equality — the decode→re-encode round trip approach the review asked for) and M3 (alerting moved to walAddressBalanceFrost / suiAddressBalanceMist, which is what the register PTB can actually withdraw). The Rust side now parses the sidecar code field, which closes the pre-existing gap where every non-2xx collapsed into AppError::Internal and the journal was never cleared.
Blocker
The B1 recovery can register the same blob twice. See the inline comment on walrus-upload-journal.ts:492. Short version: the guard uses isSponsoredTransactionInvalidatedMessage, which also matches not_found — and not_found does not prove Enoki never submitted. Combined with a single un-retried getTransaction, a lost response on the execute call can be misread as "never landed", the journal gets cleared, and a fresh sponsorship registers the blob again: WAL paid twice plus an orphan Blob object. The repo already has both narrower tools this needs (isEnokiSponsoredTransactionExpired, and the ENOKI_INVALIDATED_* backoff triple whose own .env.example note covers "short Sui object visibility lag").
Majors still open
B3 is only half closed. The rollout gate protects the initial deploy, but should_reset_prepared_register deliberately excludes INVALID_PREPARED_REGISTER_TRANSACTION (asserted in the new test). So a rollback taken after DURABLE_ENOKI_REGISTER_ENABLED=true still hits the original wedge: old replicas return 409 with that code, no Rust arm matches it, classification falls through to Transient, and the same bytes burn all five attempts. The review offered two fixes and only the flag was taken — please also classify that code as permanent-or-clear-journal, or say explicitly in the rollout notes that rollback after phase 2 requires draining first.
M3 is fixed in the alerter but not on the dashboard. services/server/src/routes/admin_dashboard.rs is not in this PR. It still deserialises only sui_mist / wal_frost (address balance plus owned coins) and wallet_status() compares those against the same thresholds the alerter now applies to address balances. So the exact scenario M3 describes — WAL sitting in coin objects, zero address balance — now pages Slack while the dashboard renders ok. That dashboard is the first surface on-call opens when the alert fires, and #669 had just added the SUI column to it, so the two should move together. Suggest get_wallets read walAddressBalanceFrost / suiAddressBalanceMist and compute status from those (showing both spendable and total would be even better, and would also address Minor 6).
M1 unaddressed. Sponsored journals still carry no expiration: assertSponsoredRegisterTransaction does not require one, so maxEpoch stays undefined and the guard at :472-482 remains dead for this path. The B1 recovery now compensates for the invalidated case, so this is no longer "stuck forever" — but the code still degrades silently instead of asserting, which is what the review objected to. Worth either asserting the invariant or documenting that sponsored journals are intentionally unbounded and rely solely on the invalidation path.
M7 unaddressed and now actively wrong. docs/relayer/runbook-gas-pool.md:41-43 still says Enoki splits a gas coin on the selected pool wallet, and :105 still tells on-call to send SUI to the starved pool wallet. With the gate that guidance is now correct in phase 1 and wrong in phase 2 — it needs the conditional, not just a rewrite.
Minors
- Minor 12 is not actually fixed. See the inline comment on
:300— the predicate is meaningful now and properly tested, but the call site is still a tautology and thethrowis still unreachable. - M4 residual. The fixture at
sidecar-query-helpers.test.ts:173-174still setssponsorDigest: digest, so the test would keep passing if the two were swapped in production — the precise confusion this change introduces. A distinct sponsor-digest value costs one line. - The
not_foundpath has no test. The new rejection test only drives{"code":"expired"}. - M5 residual.
docs/reference/environment-variables.mdand.env.exampleare both good now, butscripts/sidecar/state.ts:77still publishesfallbackToDirectSignon the health surface and does not publishDURABLE_ENOKI_REGISTER_ENABLED. During a two-phase rollout the gate is the single most important piece of state to be able to read remotely; please add it. - Minor 7 residual.
WALLET_BALANCE_LOW_THRESHOLD_WAL/_SUI,SPONSOR_BALANCE_LOW_THRESHOLD_SUI,BALANCE_MONITOR_INTERVAL_SECSandWALLET_BALANCE_LOW_ALERT_DEDUP_SECSare still absent fromdocs/reference/environment-variables.md. - Minor 10 residual.
wallet_balance_low_dedup_key_formationstill drivesAlertDedupwith hand-written strings and never callsnotify_wallet_balance_low, so reverting the key would keep it green. Now that the key has three segments the gap is wider, not narrower. DurableUploadAdvance::Preparedis overloaded. The reset path returns it withregister_transaction: None, bypassing theis_some()check that the success path treats as an error. It works — jobs.rs persists and the next loop iteration re-prepares — but the variant now means two different things; a distinct variant or a comment on the enum would help. Note it also consumes one of the six loop slots, and overflowing returnsWalletJobError::Transient, costing one of the five job attempts.
Rollout note
With the gate off and ENOKI_API_KEY set — the state both staging and production are in — durableRegisterDirectSigningAllowed(true, false) returns true, so durable register direct-signs and pays gas from the uploader wallet, regardless of ENOKI_FALLBACK_TO_DIRECT_SIGN. Before #663 that same combination with the flag false returned 409 and refused. That makes phase 1 more permissive than pre-#663 for any deployment that sets the flag false, which is the documented production setting. Please state this in the rollout steps: during phase 1 uploader wallets fund their own register gas, so the new SUI alert is load-bearing for exactly that window.
| if (!isSponsoredTransactionInvalidatedMessage(errorMessage(error))) throw error; | ||
|
|
||
| // Enoki sponsorship handles are disposable. Only discard this one | ||
| // after a second digest lookup proves the journaled Sui transaction | ||
| // never landed; the Rust writer will then clear the prepared bytes | ||
| // and build a fresh sponsorship on its next checkpoint. | ||
| try { | ||
| const finalized = await client.getTransaction({ digest: prepared.digest, include }); | ||
| return assertExpectedDigest(finalized); | ||
| } catch (lookupError: any) { | ||
| if (lookupError?.code !== "NOT_FOUND") throw lookupError; | ||
| } | ||
| throw new NoSideEffectError( | ||
| `sponsored register transaction ${prepared.digest} was invalidated and is not on chain; rebuild sponsorship`, | ||
| ); |
There was a problem hiding this comment.
Blocker — this predicate is too broad, and one lookup cannot prove absence.
The direction is right and this is the fix B1 asked for, but both halves are weaker than the proof this path needs.
isSponsoredTransactionInvalidatedMessage matches four patterns, and they are not equivalent:
/sponsored transaction has expired/i || /"code"\s*:\s*"expired"/i // never submitted
/sponsored transaction not found/i || /"code"\s*:\s*"not_found"/i // ambiguousexpired implies Enoki never submitted. not_found does not — Enoki can return it for a sponsorship handle it already consumed.
Concrete sequence, all within existing timeouts:
executeSponsoredreaches Enoki, which submits to Sui.- Our HTTP response is lost (timeout).
callEnokiretries —ENOKI_TRANSIENT_MAX_ATTEMPTS=2.- Enoki reports the handle gone:
not_found. - We do one
getTransaction; the node has not indexed the transaction yet →NOT_FOUND. - We conclude "never landed" →
NoSideEffectError→ Rust clears the journal → fresh sponsorship, new digest. - The blob is registered twice: WAL paid twice, plus an orphan Blob object.
This file already assumes that lag — the success path immediately below retries getTransaction 8 times with escalating delay for exactly this reason. And the repo already has the two narrower tools:
isEnokiSponsoredTransactionExpiredinscripts/walrus-error-detection.tsmatches onlyexpiredand deliberately leavesnot_foundout.ENOKI_INVALIDATED_MAX_ATTEMPTS/_BASE_DELAY_MS/_MAX_DELAY_MS(4 / 1000 / 8000) exist for this hazard;.env.exampledescribes them as covering "Enoki-sponsored tx invalidation (expired) and short Sui object visibility lag".
mayHaveBeenSubmitted is also ignored here, while the maxEpoch branch ~15 lines up does honour it with an UNAVAILABLE ambiguous error.
Suggested replacement for the catch body (this also needs import { isEnokiSponsoredTransactionExpired } from "../../walrus-error-detection.js";):
| if (!isSponsoredTransactionInvalidatedMessage(errorMessage(error))) throw error; | |
| // Enoki sponsorship handles are disposable. Only discard this one | |
| // after a second digest lookup proves the journaled Sui transaction | |
| // never landed; the Rust writer will then clear the prepared bytes | |
| // and build a fresh sponsorship on its next checkpoint. | |
| try { | |
| const finalized = await client.getTransaction({ digest: prepared.digest, include }); | |
| return assertExpectedDigest(finalized); | |
| } catch (lookupError: any) { | |
| if (lookupError?.code !== "NOT_FOUND") throw lookupError; | |
| } | |
| throw new NoSideEffectError( | |
| `sponsored register transaction ${prepared.digest} was invalidated and is not on chain; rebuild sponsorship`, | |
| ); | |
| if (!isEnokiSponsoredTransactionExpired(errorMessage(error))) throw error; | |
| // `expired` is the only invalidation that proves Enoki never | |
| // submitted. Re-read with the same backoff the success path uses: | |
| // a stale read is precisely what would make this ambiguous. | |
| for (let attempt = 1; attempt <= 8; attempt += 1) { | |
| try { | |
| return assertExpectedDigest( | |
| await client.getTransaction({ digest: prepared.digest, include }), | |
| ); | |
| } catch (lookupError: any) { | |
| if (lookupError?.code !== "NOT_FOUND") throw lookupError; | |
| if (attempt < 8) { | |
| await new Promise((resolve) => setTimeout(resolve, attempt * 250)); | |
| } | |
| } | |
| } | |
| if (mayHaveBeenSubmitted) { | |
| throw Object.assign( | |
| new Error( | |
| `sponsored register transaction ${prepared.digest} was invalidated but remains ambiguous`, | |
| ), | |
| { code: "UNAVAILABLE" }, | |
| ); | |
| } | |
| throw new NoSideEffectError( | |
| `sponsored register transaction ${prepared.digest} expired and is not on chain; rebuild sponsorship`, | |
| ); |
If you would rather keep the broad matcher, the equivalent is to treat expired as reset-eligible and not_found as ambiguous, and to add a test for the not_found branch — right now only {"code":"expired"} is exercised.
| } | ||
|
|
||
| if (!durableRegisterDirectSigningAllowed(false, ENOKI_FALLBACK_TO_DIRECT_SIGN)) { | ||
| if (!durableRegisterDirectSigningAllowed( |
There was a problem hiding this comment.
Minor 12 from the #663 review is not actually fixed — the throw is still unreachable.
The function is genuinely better: both parameters are now used and the three combinations are tested. But this call site remains a tautology.
durableRegisterDirectSigningAllowed returns false only when durableSponsorshipEnabled && enokiConfigured, and that is exactly the branch that already returned early at :266. So on every path that reaches line 300, the predicate is true and throw new Error("durable register requires Enoki sponsorship") can never fire.
Either drop the block, or make it express the invariant you actually want to enforce — e.g. refuse to direct-sign when a key is configured and the operator has not made an explicit phase choice, rather than deriving the condition from the same two values the branch above already discriminated on.
Only treat Enoki `expired` as proof the sponsorship never submitted, and re-read the journaled digest with the same index backoff as the success path before clearing the journal. `not_found` stays ambiguous. Also reset incompatible prepared journals on mixed-version 409s, drive the admin dashboard from address balances, and document the phase-1 uploader-pays-gas window.
|
@harrymove-ctrl addressed the review on this push. Blocker. Recovery now uses B3. M3. Admin dashboard reads M1. Documented: sponsored journals are intentionally unbounded and rebuild only via the expired-sponsorship path. M7 + rollout. Runbook is now phase-conditional. Phase 1 with Also: dropped the tautological throw, distinct |
Treat Enoki not_found as ambiguous (digest lookup, then UNAVAILABLE) instead of a verify failure. Lock the expired-path index retry in tests, show spendable vs total balances on the admin dashboard, and keep the dashboard parseable against an older sidecar payload.
harrymove-ctrl
left a comment
There was a problem hiding this comment.
Approving
Reviewed 41c47d4d again against every item from the previous review. Each one verified in the code, not only in the description.
Blocker resolved, and factored better than what I suggested. The recovery path now shares one lookupPreparedRegisterTransaction helper with the success path (indexAttempts = 8), so both use identical index backoff instead of two copies. The classification is right: !invalidated rethrows untouched, a successful lookup returns the finalized transaction, !expired || mayHaveBeenSubmitted raises UNAVAILABLE and preserves the journal, and only a true expired with no submission marker raises NoSideEffectError. Tests cover the expired reset, not_found staying UNAVAILABLE, indexed after retry, and the ambiguous path.
B3. Confirmed the 409 is emitted at :705 inside the validatePreparedRegisterTransaction catch, before phaseCanSubmitSideEffect = true. Clearing the journal on that code therefore cannot discard a transaction that was already submitted.
M3. wallet_status now receives address balances, which matches the alerter exactly. parse_address_or_total falling back to the total when the field is absent keeps mixed version sidecar payloads working during rollout. The frontend relabels both columns and renders the total as a secondary line only when it differs from spendable.
M1, M7, the health surface, the env var table, the fixture sponsor digest, and the extracted dedup key all check out.
Two cosmetic items, not blocking
1. .admin-balance-total is referenced but never defined. It appears twice in AdminWalletBalances.tsx and in no stylesheet. index.css carries .admin-table-monospace and .dash-page .admin-status-badge, so this class falls through unstyled: the total renders at the same size, weight and color as the spendable figure, stacked inside a right aligned monospace cell. That produces two equally weighted numbers in precisely the case worth distinguishing, on the screen on call opens when the alert fires.
2. durableRegisterDirectSigningAllowed now has no production call site. Only the tests at :137 reference it. Dropping the unreachable throw was the right call, but the function outlived it, and its !durableSponsorshipEnabled || !enokiConfigured is the exact negation of the ENOKI_API_KEY && DURABLE_ENOKI_REGISTER_ENABLED condition inlined at :266. That leaves one decision expressed in two places, which is the shape that drifts later. Either delete the function together with its test, or use it at :266.
One correction to the PR description
The description says not_found is rethrown. The code does something better than that: not_found goes through the digest lookup and then raises UNAVAILABLE, which preserves the journal for a later retry rather than surfacing a raw Enoki error. Worth fixing the wording so a later reader does not infer the weaker behaviour from the description.
Summary
Follow-up hardening for #663 before promoting staging to main.
Sponsored durable registration
not_foundis ambiguous and does not clear the journal. The sidecar returnsNO_SIDE_EFFECT; Rust clears the prepared journal and creates a fresh sponsorship.INVALID_PREPARED_REGISTER_TRANSACTIONalso clears the journal so a current replica can rebuild instead of burning Transient retries.DURABLE_ENOKI_REGISTER_ENABLED(defaultfalse) for a two-phase rollout. Deploy all replicas with sponsorship disabled first, let old replicas drain, then enable it. Drain again before rolling the gate back.Proactive balance alerts
statususes the same address balances. Wire fieldssui/walare spendable;sui_total/wal_totalkeep combined totals.Rollout
DURABLE_ENOKI_REGISTER_ENABLED=false(the default).ENOKI_API_KEYis set, durable register still direct-signs and pays gas from the uploader wallet. The SUI address-balance alert is load-bearing for this window. Do not top up Enoki pool wallets expecting them to sponsor this path.DURABLE_ENOKI_REGISTER_ENABLED=trueand redeploy/restart.false.Validation
sidecar-query-helpers.test.ts(37/37)cargo fmt --checkgit diff --checkContext
Addresses the blocking findings documented in the Notion task Alert gas for the #663 / #669 promotion path, and Harry's review on this PR.