Analyze if removing a consumer chain on IBC timeout/"ack error" is not too severe

[tbruyelle]

vaas/x/vaas/provider/keeper/relay.go

Lines 37 to 48 in e7a3153

	func (k Keeper) OnTimeoutPacketV2(ctx sdk.Context, sourceClientID string) error {
	consumerId, found := k.GetClientIdToConsumerId(ctx, sourceClientID)
	if !found {
	k.Logger(ctx).Error("packet timeout, unknown client:", "clientID", sourceClientID)
	return errorsmod.Wrapf(
	providertypes.ErrInvalidConsumerClient,
	"timeout on unknown client %s", sourceClientID,
	)
	}
	k.Logger(ctx).Info("packet timeout, deleting the consumer:", "consumerId", consumerId, "clientId", sourceClientID)
	return k.StopAndPrepareForConsumerRemoval(ctx, consumerId)
	}

When the provider chain receives a packet timeout, it stops and removes the related consumer chain.

Given that a packet timeout can happen if the consumer chain is just not responsive (or just the RPC targeted by the relayer, if there's a single one), I wonder if this is not too severe.

Summoning @clockworkgr since it's related to relayer behavior.

[clockworkgr]

It depends what the timeoutTimestamp/timeoutHeight for the packet is. Whether it's severe or not largely depends on that. If the packets generated by the provider have a long enough timeout to allow for flaky connections/relays then no

[tbruyelle]

@clockworkgr Makes sense yes I agree.

Does the relayer have a retry mechanism for the RecvPacket in case of the first attempt fails ?
Does he handle a list of different RPCs in case one of them is down ?

[clockworkgr]

@clockworkgr Makes sense yes I agree.

Does the relayer have a retry mechanism for the RecvPacket in case of the first attempt fails ?
Does he handle a list of different RPCs in case one of them is down ?

Each polling cycle queries unreceived packets so yeah it retries by default.

Only uses single RPCs though

[tbruyelle]

Only uses single RPCs though

is it the case for hermes for example ? We know how dangerous it can be to rely on a single RPC, it can be poisonned.

[giunatale]

I think the concern is valid. Moreover there's even an additional thing to consider that wasn't addressed so far: the effective timeout isn't the configured one.

SendVSCPacketsToChain (in provider/keeper/relay.go) sets timeoutTimestamp from min(VaasTimeoutPeriod, channeltypesv2.MaxTimeoutDelta). VaasTimeoutPeriod defaults to 4 weeks, but IBC v2 hard-caps MaxTimeoutDelta at 24h, so every VSC packet actually expires 24h after it's sent regardless of the param. The "4 weeks to ensure the channel doesn't close" comment on the default is misleading — the real margin for a flaky consumer/relayer is 24h, and with a single relayer RPC (as noted above) a >24h outage of just that RPC is enough. And matter of fact a timeout higher than channeltypesv2.MaxTimeoutDelta would also be rejected.

The teardown that follows is also irreversible: timeout, error-ack, and send-failure all funnel into StopAndPrepareForConsumerRemoval, and there's no path back from STOPPED to LAUNCHED, only forward to DELETED after the unbonding period. So a single 24h liveness blip permanently retires the consumer.

Also, worth flagging as a side note: a real 28-day timeout would be unsafe, because it exceeds the TrustingPeriod (and the unbonding period). The 24h cap is what keeps removal inside the slashable window. So the param value is both inert and, if it ever applied, essentially wrong (unless is set to be lower than channeltypesv2.MaxTimeoutDelta, which I think should be a hard cap for that param if we want to keep it).

[tbruyelle]

Interesting, so removing a consumer chain on timeout is definitily too severe. We need to think of an other approach, maybe when removing only when there is multiple timeouts in a row. But that said that also allows a longer valset discrepency between the provider and the consumer, which could lead to some attack opportunities.

This also raises a bug in the recently merged EvidencePacket (#39): there's no cap of the timeout like for the VSCPacket, EvidencePacket takes the timeout directly from the VaasTimeoutPeriod param, so with 4 weeks they are always rejected, never removed from the queue and retried every block. This needs to be fixed (cc @julienrbrt )

[tbruyelle]

Asked Claude to write a plan to include a grace period instead of immediate consumer remove.

This raised an important concern regarding the need for a resync of the valset. Indeed, since the VSCPackets are just diffs, if a consumer misses one, the provider has to base the diff of the next one on the latest acknowledged instead of the latest sent.

Not part of this plan but maybe during this grace period only core and Vaas IBC packets should be allowed. This is to prevent misbehavior during valset discrepency.

Sorry for the long read but if you think this is the way to go I can create a PR.

Plan: graceful consumer handling on IBC timeout

Context

When the provider receives an IBC packet timeout (or an error-ack, or a local send
failure) for a VSC packet to a consumer, it immediately and irreversibly removes
the consumer chain — OnTimeoutPacketV2, OnAcknowledgementPacketV2, and the
send-failure branch of SendVSCPacketsToChain all call
StopAndPrepareForConsumerRemoval (x/vaas/provider/keeper/relay.go:30,47,245), which
moves the consumer to STOPPED with no path back to LAUNCHED, only forward to
DELETED after the unbonding period.

This is too severe (issue #36):

1. A timeout only means "no delivery within the packet timeout" — a transient consumer
   outage, or the single relayer RPC being down/poisoned. It is not evidence of
   misbehaviour.
2. The effective timeout is always 24h, never the configured value:
   SendVSCPacketsToChain uses min(GetVAASTimeoutPeriod, channeltypesv2.MaxTimeoutDelta)
   and IBC v2 hard-caps MaxTimeoutDelta at 24h (relay.go:207). So
   DefaultVAASTimeoutPeriod = 4 weeks (x/vaas/types/shared_params.go:13) is inert
   and its "ensure channel doesn't close" comment is misleading. A 24h blip of one RPC
   permanently retires a consumer.

A second, related problem surfaces as a consequence of the fix. VSC packets are
diffs, and the provider advances its record of the consumer's set optimistically at
queue time (SetConsumerValSet(next) in ComputeConsumerNextValSet,
x/vaas/provider/keeper/validator_set_update.go:193) whether or not the packet is
delivered. The consumer dedups by ValsetUpdateId and applies diffs
(x/vaas/consumer/keeper/relay.go:24,55). So a lost packet's change is never re-sent:
e.g. a validator that left during the outage is removed from the provider's record but
the only "remove" packet was the one that timed out → the consumer keeps that validator
forever, attackable with no slashable stake. Today this is masked because the consumer is
deleted on the first timeout; once we stop removing eagerly, it becomes a live exploit
window. So "don't remove on timeout" is only correct if paired with a resync mechanism.

Intended outcome: a transient outage no longer kills a consumer; the consumer
self-heals to the correct validator set when connectivity returns; a consumer is removed
only after a sustained outage that approaches the slashable-stake window; and the
VaasTimeoutPeriod param is made honest.

Decisions (confirmed with the user)

• Grace period is derived (no new param, no proto change) from the provider
  unbonding period.
• Error-acks share the same time-based grace as timeouts/send-failures.
• Resync = "last-acked baseline" (diff against the last acknowledged set), not strict
  +1 ordering.

Already resolved — no action

The issue's last comment flags a missing timeout cap on EvidencePacket. That was fixed in
PR #52 (b07ccaa): x/vaas/consumer/keeper/evidence_packet.go:71 already uses
min(GetVAASTimeoutPeriod, MaxTimeoutDelta). Nothing to do; mention in the PR description.

---

Change A — replace immediate removal with a derived liveness grace

Idea: track per-consumer liveness (time of the last successful VSC acknowledgement).
On timeout / error-ack / send-failure, do not remove — log and let the next epoch
retry. Remove only when the consumer has had no successful ack for longer than a derived
grace period.

• New state (provider keeper Schema, x/vaas/provider/keeper/keeper.go): a
  per-consumer ConsumerLastLivenessTime map. Mirror the existing byte-time storage
  pattern used by ConsumerRemovalTime (keeper.go + Set/Get/DeleteConsumerRemovalTime).
  Add a prefix in x/vaas/provider/types/keys.go. Delete it in DeleteConsumerChain
  (consumer_lifecycle.go:404 block).

  • Lazy init (avoids a migration): the getter returns ctx.BlockTime() when no entry
    exists, so existing launched consumers are treated as "live as of now" at the upgrade
    block instead of being mass-removed. Initialize explicitly at launch
    (SetConsumerPhase(...LAUNCHED), consumer_lifecycle.go:256).

• Update liveness on success — OnAcknowledgementPacketV2 (relay.go:22): when
  ackError == "", set ConsumerLastLivenessTime = ctx.BlockTime() for the consumer
  (look up via GetClientIdToConsumerId). (This is also where resync advances the baseline
  — see Change B.)

• Derived grace period — new helper on the provider keeper, e.g.
  LivenessGracePeriod(ctx) = CalculateTrustPeriod(providerUnbondingTime, GraceFraction),
  reusing CalculateTrustPeriod (x/vaas/types/shared_params.go:59) and
  k.stakingKeeper.UnbondingTime(ctx) (the slashable-stake window — same source
  StopAndPrepareForConsumerRemoval already uses for deletion scheduling). Introduce a
  code constant GraceFraction (recommend "0.33" ≈ ⅓ of unbonding ≈ ~7 days at the
  21-day default — far more lenient than 24h, comfortably inside the slashable window).
  Flag the fraction for review — it's the one tunable judgment call.

• Grace check helper — shouldRemoveForUnresponsiveness(ctx, consumerId) returns true
  when ctx.BlockTime().Sub(lastLiveness) > LivenessGracePeriod(ctx).

• Rewire the three removal sites (relay.go) to call the grace check instead of
  removing unconditionally:

  • OnTimeoutPacketV2 (:37) — log; remove only if grace exceeded.
  • OnAcknowledgementPacketV2 error branch (:29) — same (per the user's choice).
  • SendVSCPacketsToChain non-ErrClientNotActive send failure (:238-249) — mirror the
    lenient ErrClientNotActive path (log, leave data, return nil); rely on the grace sweep.

• Mandatory periodic sweep — timeouts/acks are relayer-driven, so a fully dead
  relayer fires no callback and a callback-only grace check would never trigger removal.
  Add a sweep over GetAllLaunchedConsumerIds that removes any consumer past its grace.
  Wire it into the existing provider BeginBlock (x/vaas/provider/module.go:146, next to
  BeginBlockRemoveConsumers) or EndBlockVSU. This is the load-bearing safety addition.

Change B — last-acked-baseline resync (self-healing diffs)

Idea: compute each epoch's VSC diff against the last acknowledged consumer set
instead of the optimistically-advanced one. Because validator updates carry absolute power
and the consumer applies them idempotently, a packet computed against the acked baseline is
a complete delta from a state the consumer is known to have — so a lost packet's change is
automatically re-sent every epoch until an ack confirms receipt. Consumer code is
unchanged; wire format and the global VSC-ID counter are unchanged.

Key correctness invariant (removes the need for contiguity tracking): because every
diff is computed against the current baseline (not against the previous packet), an ack
for VSC id N proves the consumer applied a full delta from the baseline to set@N, i.e.
it is now at set@N. So advancing the baseline to the highest acked id is correct even
if earlier ids were never acked or acks arrive out of order. (Verified against the
consumer's dedup + absolute-power apply in x/vaas/consumer/keeper/relay.go and
validators.go.)

• New state (provider keeper Schema):

  • AckedConsumerValSet — per-consumer baseline set (mirror the ConsumerValidators
    map shape). Lazy-init: when absent, fall back to the current ConsumerValSet so
    behaviour at the upgrade block matches today until the first ack lands.
  • SentVSCValSet — snapshot of the intended set per in-flight (consumerId, vscId),
    used to resolve the baseline when an ack arrives. Bounded window (keep only the
    most recent ~N in-flight ids; drop older — safe because the baseline only moves
    forward and a recovering relayer delivers recent packets). Pruned on ack/timeout and in
    DeleteConsumerChain.

• Diff computation — ComputeConsumerNextValSet
  (validator_set_update.go:180): keep SetConsumerValSet(next) (the optimistic record is
  still used elsewhere), but compute the outgoing updates as
  DiffValidators(AckedConsumerValSet, next) instead of
  DiffValidators(currentConsumerValSet, next), and snapshot next into SentVSCValSet
  keyed by the epoch's VSC id.

• Advance baseline on ack — in OnAcknowledgementPacketV2 success branch, decode the
  acked VSC id from the payload and, if it exceeds the last acked id, set
  AckedConsumerValSet = SentVSCValSet[id] and prune snapshots <= id. This requires
  threading the payload (already available in the IBC callback
  x/vaas/provider/ibc_module.go:162 OnAcknowledgementPacket) into
  OnAcknowledgementPacketV2 — extend its signature to take the decoded id (or the
  payload). On timeout (OnTimeoutPacket/OnTimeoutPacketV2), prune that id's snapshot;
  baseline is unchanged (the next epoch's diff-from-baseline heals the gap).

• Stop deleting on send is not required — pending packets can still be deleted after
  send as today (relay.go:259); the baseline + snapshots are the resync state, not the
  pending queue.

[giunatale]

I took your plan @tbruyelle and some work I already did on analysis/planning and came up with an implementation that should address this (see #59). Documentation is available in docs/consumer-liveness.md as a starting point.