feat: liveness-based consumer removal, snapshot resync, and safe mode#59
Open
giunatale wants to merge 20 commits into
Open
feat: liveness-based consumer removal, snapshot resync, and safe mode#59giunatale wants to merge 20 commits into
giunatale wants to merge 20 commits into
Conversation
…moted IBC callbacks
…bound - SafeModeThreshold becomes a per-consumer initialization parameter (was a hardcoded const), validated > 0. - LivenessGraceFraction becomes a provider module parameter (was a hardcoded const), default 0.66; the sweep grace is unbonding * fraction. - consumer unbonding is bounded only by <= provider unbonding; the hardcoded multi-day floor is dropped (an impractically short unbonding is an operator concern, and short values are useful for test/dev chains). - clarify the remove-consumer CLI help: removal is governance-only. Accompanying unit tests are in the following commit.
Provider: block-clock sweep (including boundary and per-consumer partial removal), ack/counter tracking, snapshot resync, timeout/error-ack are log-only, param and query coverage, and the relaxed unbonding bound. Consumer: VSC staleness, snapshot-replace apply, and the safe-mode tx filter.
Short-unbonding Docker suite with fast blocks and a first-sync gate: exercises recover-before-grace, real safe mode (relayer paused -> VSC-stale consumer restricted, then recovers on resume), the liveness query, and the block-clock sweep to STOPPED under sustained IBC silence.
Add docs/consumer-liveness.md (liveness clock and sweep, snapshot resync, demoted IBC callbacks, consumer safe mode, params/bounds, operator query) and reconcile the lifecycle doc.
- sweep: run StopAndPrepareForConsumerRemoval in a cached context so a partial failure cannot commit the STOPPED phase without also scheduling removal (which would strand a consumer STOPPED-but-never-DELETED); a stop-error test now covers this. - fraction validation: require a fraction strictly in (0,1), rejecting exactly 1, so the liveness grace stays inside the unbonding/slashable window and the IBC trusting period stays below unbonding. - computeReplaceUpdates: read the current cross-chain validator set once, not twice. - GetConsumerParams: panic on a params read error instead of returning a zero-value struct, whose zero SafeModeThreshold would silently pin the consumer in safe mode and hide the corruption. - genesis export: document that the liveness clock is not exported and resets safely on import (no immediate sweep, no forced snapshot).
giunatale
requested review from
clockworkgr,
julienrbrt and
tbruyelle
as code owners
The long-unbonding IntegrationTestSuite was never green on this branch because create-consumer failed the new init-parameter validation, and once that was fixed two of its never-run liveness tests needed adjustment. - testdata/create_consumer.json: add a safe_mode_threshold (now a required init param, must be > 0) and bring vaas_timeout_period within [10m, 24h] (it was four weeks). Either omission made create-consumer fail at CheckTx, leaving the consumer uncreated and its genesis empty. - consumer registration now broadcasts in sync mode and asserts the tx code, so an init-param rejection fails immediately instead of surfacing ~90s later as an empty consumer genesis. - testLivenessTransientOutage: delegate at least one unit of voting power (tokens / powerReduction) so the validator's integer power actually changes and the snapshot resync is exercised; a smaller delegation truncated to +0 power. Also unpause the consumer via defer so a failure cannot leave the container paused and cascade into later tests. - remove testLivenessSafeMode: its debt-based bank-send restriction duplicates testConsumerDebtFlow, its gov-allowlist probe duplicates the ante unit tests, and real VSC-staleness safe mode is covered end-to-end by the short-unbonding LivenessIntegrationTestSuite. It also could not trigger debt because the preceding debt test leaves the fee pool funded.
…grace A consumer's SafeModeThreshold (a per-consumer init parameter) must be strictly shorter than the provider's liveness grace (unbonding * LivenessGraceFraction), so a lagging consumer enters safe mode -- and stops value-bearing work under a possibly-stale set -- before the provider's sweep removes it. Both values are cross-chain, but the provider holds each at create/update time (the threshold as an init param it ships to the consumer, the grace derived from its own unbonding and fraction), so the relationship is enforced provider-side rather than left to operator discipline, exactly as the consumer-unbonding bound already is. The unbonding and safe-mode checks are combined into a single validateConsumerInitParams that reads the provider unbonding once.
Export and re-import the per-consumer liveness state so a genesis export/import (a state-export restart) preserves it instead of resetting it: - provider ConsumerState gains last_ack_time and highest_sent / highest_acked vsc ids, exported for launched consumers and restored at InitGenesis. - consumer GenesisState gains last_vsc_recv_time, so the VSC-staleness clock survives a restart rather than falling back to the current block time. Previously these fields reset on import -- safe by fallback (no immediate sweep, no forced snapshot, never-stale until the first VSC) but not faithful. Genesis round-tripping is foundational state handling, not a version migration, so the liveness clock is now preserved end-to-end (covered by the provider and consumer genesis round-trip tests).
tbruyelle
reviewed
Jul 6, 2026
MsgRemoveConsumer requires the governance module as authority, but the CLI signed it with the caller's address, so the broadcast tx always failed ErrInvalidSigner. Removal goes through a gov proposal (tx gov submit-proposal with MsgRemoveConsumer); the dedicated command could only ever fail, so drop it.
ctx is already an sdk.Context, so sdk.UnwrapSDKContext(ctx).BlockTime() is just ctx.BlockTime().
MinVAASTimeoutPeriod (10m) floored VaasTimeoutPeriod; validation is now (0, MaxTimeoutDelta]. The floor guarded against a too-short timeout expiring packets before delivery, but post-redesign that is not dangerous -- an undelivered packet is superseded next epoch, a late one is dropped by the consumer's dedup, and a persistently unreachable consumer is removed by the liveness sweep. The floor only blocked exercising the timeout path in tests.
The consumer now emits EventTypeSnapshotResync when it applies a snapshot VSC packet (is_snapshot=true) -- not on ordinary diffs -- so a resync is observable by operators and assertable in e2e (rather than being indistinguishable from a resent diff). The main-suite testLivenessTransientOutage previously claimed to 'verify snapshot resync' and exercise the demoted timeout, but with a brief pause no packet times out and re-convergence could equally be a resent diff. Its doc and messages are corrected to claim only what it proves (re-converges after a transient outage); the snapshot path and demoted timeout are covered by unit tests (TestSnapshotResyncEmitsEvent, the TestSnapshot* set, TestTimeoutDoesNotRemove) and, end-to-end, by an upcoming forced-timeout test in the short-timer suite.
The main suite's transient-outage test cannot prove that the demoted OnTimeout no longer removes the consumer, nor that a behind consumer heals via a snapshot rather than a resent diff, because with a brief pause no packet ever times out. This adds testForcedTimeoutSnapshotResync to the short-timer suite. It sets a short provider vaas_timeout_period (20s, now allowed since the floor was dropped) and pauses the *relayer* -- not the consumer -- so the consumer keeps producing blocks and its clock advances past the timeout with packets undelivered. On unpause the relayer submits MsgTimeout (provider OnTimeout fires) and delivers a snapshot to the now-behind consumer. The test asserts all three: the consumer stays LAUNCHED, the provider logged a real timeout (so the LAUNCHED assertion is not vacuous), and the consumer logged a snapshot resync (healed via a snapshot, not a resent diff). The consumer now also logs "applied snapshot resync" alongside the EventTypeSnapshotResync event so the e2e can assert on it reliably (a container log grep, like the provider-timeout assertion) rather than via tx_search.
IntegrationTestSuite and LivenessIntegrationTestSuite duplicated ~29 near- identical Docker/relayer/exec helpers that differed only in constants (chain ids, ports, network, container names) and the genesis/config-toml patches applied at bring-up. Introduce baseTestSuite (embedded by both) parameterized by a baseSuiteConfig: the differing constants become config fields and the differing mutations become hook funcs (patchProviderGenesis, patchProviderConfigToml, patchConsumerConfigToml). The shared bring-up, ts-relayer wiring, and exec/query helpers move to the base; the liveness* duplicates are deleted; each suite keeps only its config and its own test methods. Behaviour is unchanged (identical ports, ids, genesis values, timeouts, sequencing) -- both suites pass. Also rename the main suite's queryConsumerPhase to queryProviderConsumerPhase (it queries the provider) and host it on the base.
Contributor
Author
|
@tbruyelle I should have addressed all your f/b so I marked everything as resolved but don't hesitate to let me know if you wanted something done differently or you have more f/b! |
julienrbrt
reviewed
Jul 7, 2026
| // receiving a VSC packet before its tx admission gate enters safe mode | ||
| // (only ibc.core and gov messages pass). Set well below the provider | ||
| // liveness grace period. | ||
| DefaultSafeModeThreshold = 3 * time.Hour |
Member
There was a problem hiding this comment.
The chain devs better run themselves a relayer then :D
Contributor
Author
There was a problem hiding this comment.
wdym? the safe mode allows still the situation to be rectified and to be exited. Can you elaborate?
tbruyelle
reviewed
Jul 8, 2026
Rewrite docs/consumer-liveness.md and the liveness test comments to describe the current design only, dropping narration of how things used to behave (it is confusing context for unreleased software): the "why this exists" framing becomes a present-tense design rationale, the IBC-callbacks section states they are log-only rather than "no longer remove", and the VaasTimeoutPeriod note drops the "lowered from four weeks" history. Also drop the mention of the short-lived vaas_timeout floor from a test comment (added and removed within this branch).
julienrbrt
approved these changes
Jul 8, 2026
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
3 participants
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 #36
The provider retired a consumer on the first failed VSC delivery, which could be a packet timeout, an error acknowledgement, or a local send failure. Two facts made that too severe:
timeout of
min(VaasTimeoutPeriod, MaxTimeoutDelta), and IBC v2 hard-capsMaxTimeoutDeltaat 24h — so a 24h outage of a single relayer or RPC wasenough to permanently retire a consumer.
changed. Simply dropping eager removal would, by itself, leave a departed
validator stranded in the consumer's active set — a validator that could
later unbond on the provider and validate the consumer with no slashable
stake.
This replaces the per-packet kill switch with a provider-local liveness clock, pairs it with a self-healing resync so lost diffs can't strand a stale validator, and adds a consumer-side safe mode for the window in which a consumer's set may be out of date.
What changed
lastAck(blocktime of each consumer's last successful VSC ack), seeded at
LAUNCHEDandrefreshed on every ack. Each
BeginBlock,SweepUnresponsiveConsumersstopsany consumer whose
lastAckis older thangrace = unbonding * LivenessGraceFraction. It runs on the provider's own block clock, so removalno longer depends on a live client or relayer.
highestAcked < highestSent) the provider sends an absolute snapshot (is_snapshot = true)of the valset instead of a diff; the consumer replaces its set and drops any validator
absent from the snapshot. This closes the lost-diff hole above.
send-failure paths no longer remove; the sweep is the single removal
authority. This also closes a latent gap where an expired client left a
consumer
LAUNCHEDforever.IsVSCStale, thresholdSafeModeThreshold), the consumer's tx admission gate restricts to/ibc.core.*and/cosmos.gov.*— refusing other kinds of txs under apossibly-stale set while keeping the recovery path (VSC recv, governance)
open. It never gates VSC recv or evidence, so recovery can't brick.