Add transaction manager - #138
Open
jannikluhn wants to merge 6 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>
Every registration is signed by the same key, so all of them compete for one nonce sequence. Left to the generated bindings, a nil TransactOpts.Nonce makes each call resolve its own nonce, so two concurrent requests sign two transactions with the same one and only one of them can be mined. Under any real load the API was losing registrations to that race. The Manager closes the window structurally rather than with locks: Send only queues a request, and a single goroutine owns nonce assignment, submission and everything in flight. That goroutine also polls for receipts, so the package holds no mutexes. Polling is only observation, so a submission delaying a receipt check by a few hundred milliseconds costs nothing next to what sharing the nonce sequence would. A request is watched until it is mined, and if something else mines its nonce first it is carried over to a free one rather than failed. A transaction that goes RebroadcastAfter without being mined is sent again, priced to outbid the version it replaces, which doubles as gas bumping. Recomputing from the node's suggestion alone would not do: geth demands a bump on the tip as well as the fee cap, and the tip oracle does not move when the base fee does, so a transaction stranded below a risen base fee could never be replaced and would wedge the whole queue behind it. MaxFeeCapPercent bounds how far that escalation goes. Only the lowest-nonce transaction is resubmitted, and only one per poll. Transactions are mined in nonce order, so repricing the others cannot make them move until it does. Known limits: pending state is in memory and lost on restart, so transactions still in the pool are adopted by nonce rather than resubmitted. An account with no gas money is watched indefinitely and wedges registration, visible in pending_transactions and signer_balance_ether, and clears once it is refilled. A transaction sent from this key by anything other than this process will collide with ours. Carrying a request over abandons a transaction that cannot be proven dead, so if the old one is still included the request lands twice: the time registry reverts the second with AlreadyRegistered, the event registry has no such error and registers the identity twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both registration endpoints signed and sent their own transaction with a nil nonce, so concurrent registrations raced for the same one. They now hand a closure to a single process-wide manager, which owns the nonce sequence of the signing key. One manager for the whole process is the point: it only works if every registration goes through the same one. Submission is no longer synchronous, so the endpoints wait for the first event on the request's channel to get a hash to answer with. awaitSubmission bounds that wait at five seconds, which has to cover assigning a nonce, reading gas prices and broadcasting, plus the wait behind any submissions already queued in front of it. It does not cover waiting for a block, since the response carries a hash rather than a receipt. The bound is generous rather than tight because giving up does not cancel anything: the transaction is still sent, so a client told its registration failed may find it on chain regardless, which costs far more than waiting a little longer. A full queue is the one failure reported to the client as such, with 503, because "overloaded, come back later" is the only one a client can act on. The event endpoint's receipt poller becomes recordEventRegistration, following the same channel instead of polling TransactionReceipt on a one-second ticker. It also now covers the case where the request gave up waiting or its insert failed, so an identity registered on chain is no longer one the API has no record of. The signer address comes from the manager rather than being derived from the config key at each call site, which also removes ChainID from the per-request path: it is read once at startup instead. ChainID and TransactionReceipt leave EthClientInterface, since nothing in the usecase layer reads the chain directly any more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
12 tasks
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
Adds a transaction manager that assigns nonces, watches transactions, reports on their inclusion, and if necessary resends transactions if they get stuck or lost.