Nonce Lock - #140
Open
jannikluhn wants to merge 5 commits into
Open
Conversation
TotalSuccessfulIdentityRegistration, TotalDecryptionKeysReceived and TotalFailedRPCCalls are only ever incremented, so Counter is the correct type. Series names are unchanged, so existing queries keep working, and rate() over them is now legitimate rather than an accident.
Prometheus counters carry _total as a suffix, not a prefix, and the
plural belongs on the thing being counted:
shutter_api_total_successful_identities_registration
-> shutter_api_successful_identity_registrations_total
shutter_api_total_decryption_keys_received
-> shutter_api_decryption_keys_received_total
shutter_api_total_failed_rpc_calls
-> shutter_api_failed_rpc_calls_total
Go identifiers drop their now-redundant Total prefix to match. Existing
series keep their history under the old names but stop being written to,
so any dashboard or alert rule querying them needs updating.
The signer pays gas for every identity registration, but the service never queried its balance, so an account draining to empty was only visible once registrations started failing. At that point it surfaced as failed_rpc_calls_total from the transaction send sites, indistinguishable from an RPC outage. shutter_api_signer_balance_ether is published by a new BalancePoller running as a service alongside the metrics server, so it is gated on METRICS_ENABLED. It reads once at startup and then every 60s, each read bounded by a 10s timeout so a hung endpoint cannot stall the loop. A failed read logs, increments failed_rpc_calls_total and leaves the gauge alone; it never returns an error, because a transient RPC failure must not bring the service down through the error group. The metric is a GaugeVec with no labels rather than a plain Gauge. A plain Gauge is registered holding 0, so a restart while the RPC endpoint was down would publish 0 ether and fire the low-balance alert this metric exists to raise. With no value set the series is simply absent, and the exposed series is otherwise identical. Balance is reported in ether rather than wei so that alert thresholds are readable; the conversion goes through big.Float, as an integer quotient would truncate everything below 1 ether to zero. Known gap: a reading that stops being refreshed keeps its last value indefinitely, so a dead poller looks healthy. Detection relies on failed_rpc_calls_total, which now also moves for background polls and can therefore rise with no traffic. Co-Authored-By: Claude <noreply@anthropic.com>
successful_identity_registrations_total counts submissions, which says nothing about whether a transaction was mined. These three cover what happens after the response goes out. transactions_resolved_total is labelled by terminal state, and is finer grained than what the transaction manager reports to its callers. A revert is the contract's verdict rather than the manager's, so callers read it off the receipt, but a revert rate is still worth alerting on. Rejected and abandoned are one error to a caller that only wants to know nothing will be mined, and two series here. Every status series is created at registration so that rate() over a state that has not happened yet reads as zero instead of returning no data. submission_timeouts_total counts requests answered with an error while their transaction was still on its way to the node. Giving up waiting does not stop the transaction, so each one is a client told its registration failed that may have landed on chain anyway, and a candidate for reconciliation rather than a plain failure. pending_transactions is a gauge because it is not monotonic. A value that climbs and does not fall means registrations are being accepted but not mined, and that every later registration is queued behind them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contract bindings resolve a nil TransactOpts.Nonce by querying PendingNonceAt, so two registrations handled concurrently read the same nonce and sign two transactions for it. Whichever the node accepts second replaces the first, and one client is handed a transaction hash that will never be mined. A semaphore held across the submitting call closes that window: the next request only reads the nonce once the node has accepted the previous transaction. Nonce assignment and submission both happen inside the generated binding, with no seam between them, so the whole Register call has to be the critical section. The gas lookups it also performs do not need protecting but cannot be separated out. golang.org/x/sync/semaphore rather than a sync.Mutex because Acquire takes a context, so a request whose client has already gone away stops waiting instead of going on to send a transaction nobody will read the response to. The handlers pass ctx.Request.Context() rather than the gin.Context they receive, because gin.Context's Done channel is nil unless ContextWithFallback is set, and Acquire would silently never observe cancellation. The release is deferred rather than explicit because gin.Recovery would otherwise swallow a panic from the bindings and leave the semaphore held forever, wedging every later submission. Cancellation applies only while queued. Once Register is in flight the send completes, since aborting it would leave the nonce state unknown. go.mod also loses github.com/joho/godotenv, which go mod tidy dropped while promoting x/sync to a direct dependency. Nothing imports it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closed
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
1 participant
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.
Closes #133
Closes #121