fix: prevent the release paths from dropping coins the fullnode cannot resolve - #176
Merged
Merged
Conversation
…oins Three places took a set of coin ids, asked the fullnode to resolve them, and released whatever came back: get_latest_gas_objects(ids).into_values().flatten().collect() `flatten()` discards every coin the node could not resolve, with no retry. A discarded coin is never released, and it has already been taken out of the registry, so nothing can surface it again: absent from the free list, absent from every reservation, invisible to expire_coins, and below the 200x target_init_balance floor the periodic rescan selects on. It is still owned on chain and permanently unusable. The expiry sweep was the worst of the three and the least obvious. Its coins were never executed, so they are *guaranteed* live -- a NOT_FOUND there can only be a read failure, never a deleted coin. It is also the highest-volume path, and it reported success either way, because `count` was taken after the flatten, so ten-in/zero-out logged as ten released. num_expired_gas_coins had no increment site anywhere, so the one metric an operator would reach for was always zero. All three now share IotaClient::resolve_gas_coins, which retries on the existing budget and returns the ids that never resolved instead of dropping them. Callers decide what that means: - expiry: invariant_violation, safe because the ids come from the station's own registry and no request can drive it. - failed execution: warn plus a counter, deliberately NOT invariant_violation -- those ids are tx.gas_payment.objects, which is client-supplied until the payment is bound to the reservation. Upgrade it once the binding lands. - rescan re-read: a loud error, since run_once has no handle to metrics. New metric num_unreleased_gas_coins counts what could not be released, exported at zero from startup so an absent series never reads as "nothing was lost". num_expired_gas_coins is now incremented, and the expiry log reports released-of-expired rather than a number that agreed with itself. Also corrects smashed_coin_count, which was derived from updated_coins.len() and so counted every coin the node failed to resolve as one the validators legitimately consumed -- reporting a real loss as a routine smash. Only an executed transaction smashes, and then exactly one coin survives. This bounds the loss to the case where a coin stays unreadable past the retry budget, and makes that case loud instead of silent. It does not recover such a coin; that needs the durable pending-recovery queue described as Option D in SOLUTIONS.md.
Both found by adversarially reviewing the previous commit. Neither affects custody -- no coin is lost or double-released either way -- but both make the station report something untrue. 1. The expiry site raised invariant_violation, justified by a comment claiming its ids "come from the station's own registry, never from a caller, so no request can drive this counter". Registry provenance is not registry integrity. Until the payment is bound to the reservation, a caller can have a coin held by reservation A destroyed by paying with it under reservation B; A then references a coin the validators deleted, and when A expires that id can never resolve. One reserve/execute pair poisons a future expiry batch, and the sweep runs every second. So this is exactly the caller-drivable invariant the previous commit was careful to avoid on the failed-execution path, and missed here. Downgraded to a warning; the comment now records what has to land before it can be raised. 2. The failed-execution path counted every unresolved id as an unreleased coin while forcing the smash count to zero, so both metrics were wrong in the same scenario. If the transaction landed despite the error, smashing deleted all but one payment coin: nothing was lost, nothing is still owned, and the alert's documented remedy is a registry-wiping rescan for coins that do not exist. That branch cannot tell a smashed coin from a lost one, so it no longer claims either -- the ids go to the log with the ambiguity stated, and the counter is left to the expiry site, where the claim is usually true. The smash count is now derived rather than hardcoded. The retry added in the previous commit made it derivable: a transaction that never landed resolves every coin and yields 0, one that landed resolves the survivor once the lag clears and not the deleted ones, yielding the true count. Also corrects the metric help text, which asserted a full rescan as the remedy without qualification.
qrayven
force-pushed
the
fix/registry-inconsitency
branch
from
August 6, 2026 15:48
f82cf3e to
e3c0d69
Compare
qrayven
force-pushed
the
fix/release-paths-drop-coins
branch
from
August 6, 2026 15:49
f258d40 to
e73d17a
Compare
qrayven
marked this pull request as ready for review
August 6, 2026 15:50
itsyaasir
approved these changes
Aug 7, 2026
qrayven
added a commit
that referenced
this pull request
Aug 7, 2026
… coin (#174) * fix(metrics): make invariant_violation usable from the transaction path * fix(gas-station): release the non-oversized coins instead of none of them * fix: prevent the release paths from dropping coins the fullnode cannot resolve (#176) * fix(gas-station): stop the release paths from dropping unresolvable coins * fix(gas-station): correct two reporting defects in the release-path fix
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.
The problem
Three separate places take a set of coin ids, ask the fullnode to resolve them, and release whatever
comes back:
flatten()discards every coin the node could not resolve, with no retry. A discarded coin isnever released, and it has already been removed from the registry, so nothing can surface it again:
absent from the free list, absent from every reservation, invisible to
expire_coins, and below the200 × target_init_balancefloor the periodic rescan selects on. It is still owned by the sponsoron chain and permanently unusable by the station.
NOT_FOUNDmeans thereWhy the expiry sweep is the worst of the three
It is the one that looks least alarming and is in fact the most dangerous:
Noneis unambiguous. An expired reservation was never executed, so its coins were neverconsumed and are guaranteed still live on chain. There is no innocent reading — dropping one is
always a loss.
produces around 110 expiry releases.
Released N coins after expirationalways agreed with itself. Ten in, zero out logged cheerfullyat INFO. And nothing anywhere incremented
num_expired_gas_coins, so the one metric an operatorwould reach for was always zero.
The failed-execution release added its own misdirection: the smashed-coin count was derived from how
many coins came back, so every dropped coin was reported as one the validators legitimately consumed.
The only signal pointing at the problem argued against it.
The fix
All three sites now share
IotaClient::resolve_gas_coins, which retries on the existing budget andreturns the ids that never resolved instead of dropping them. Callers decide what that means,
and they decide differently on purpose:
warn!plus a newnum_unreleased_gas_coinscounter.warn!only, and deliberately no counter. That branch cannot tell acoin smashed by a landed transaction from a lost one, and counting them would fire an alert whose
documented remedy is a registry-wiping rescan, for coins that may not exist. The ambiguity is
stated in the log instead.
error!;run_oncehas no handle to the metrics registry.Three corrections ship alongside:
num_expired_gas_coinsis incremented at all, and the expiry log reports released of expiredrather than a number computed after the drop.
smashed_coin_countis derived rather than guessed. The retry makes it derivable on the failurepath too: a transaction that never landed resolves every coin and yields 0; one that landed
resolves the survivor once the lag clears and not the deleted ones, giving the true count.
num_unreleased_gas_coinsis exported at zero from startup, so an absent series never reads as"nothing was lost".
The second commit
f258d40corrects two reporting defects that an adversarial review of the first commit found.Neither affects custody — no coin is lost or double-released either way — but both made the station
report something untrue.
The expiry site raised
invariant_violation, justified by a comment claiming its ids "come fromthe station's own registry, never from a caller". Registry provenance is not registry integrity.
Until the payment is bound to the reservation, a caller can have a coin held by reservation A
destroyed by paying with it under reservation B; A then references a coin the validators deleted, and
when A expires that id can never resolve. One reserve/execute pair poisons a future expiry batch, and
the sweep runs every second. Downgraded to a warning, with the prerequisite recorded — it can be
raised once
fix/bind-reservation-to-gas-coinslands.The failed-execution path counted smashed coins as lost while zeroing the smash count, so both
metrics were wrong in the same scenario. It now claims neither.
What this does not do
A coin that stays unreadable past the retry budget is reported, not recovered. It is still not
released. That is the honest limit of this change: it converts a silent leak into a loud one and
bounds the window, but recovering such a coin needs the durable pending-recovery queue described as
Option D in SOLUTIONS.md, which is deliberately deferred until there is data on how
often the budget is actually exhausted.
Deliberately not an automatic rescan trigger. A full rescan wipes the registry and holds a
12-hour maintenance lock during which all reservations are refused — a disproportionate remedy for
one coin, and one whose firing rate a caller can influence.
Verification
cargo nextest run -j 1)Two things worth knowing about what the numbers do not cover:
execution, so the coins' versions never move and the read cannot lag. The 29 failure-path releases
prove the branch returns every coin when the read succeeds — nothing more. Forcing a
dispatched-then-errored transaction needs fault injection at the node boundary.
RunMode::Initlists every owned coin. It sleeps only when something is unresolved, so the common path pays
nothing, but a startup immediately after a large split can spend up to the full budget.