feat: verifiable downtime evidence with optimistic challenges and consumer pausing

[giunatale]

Closes #38

Downtime on a consumer chain is unprovable on the provider, but it is disprovable: a single validator signature for a claimed-missed height, sealed under a light-client-verified header, indicts the evidence source.
This PR builds the downtime pipeline around that asymmetry.

• Consumers track missed blocks over tumbling windows (x/slashing downtime
  handling becomes log-only) and report a per-window bitmap to the provider.
  Window parameters are provider-owned and distributed via consumer genesis
  and VSC packets, with staged activation
• The provider verifies and prices the infraction (validator's epoch fee
  share x missed fraction, converted via photon), then queues the slash
  behind a challenge window instead of executing it. DowntimeSlashFraction
  acts as a per-window ceiling (default 0.0001), repeated
  windows queue independently and can compound
• MsgChallengeConsumerDowntime lets anyone cancel a validator's pending
  slashes by proving a claimed-missed block was actually signed. A
  successful challenge refunds the withheld fee shares (escrowed in the
  consumer fee pool for the window's duration) and moves the consumer to a
  new CONSUMER_PHASE_PAUSED: no VSC packets, fee accrual stopped, resumable
  by governance (MsgResumeConsumer, with a forced snapshot resync), and
  auto-stopped after MaxPauseDuration.
• Inbound VSC packets are now authenticated by source port and a pinned
  provider chain id.

The first two commits are standalone fixes for 2 pre-existing bugs on main (export at a zero height panicked after any slash & the consumer stored the provider's client id instead of its own)

Full design and operational notes in docs/consumer-downtime.md.

[julienrbrt]

I have some tiny nits i'll share, but amazing work! so ACK

---

GLM 5.2 review

PR Review: feat: verifiable downtime evidence with optimistic challenges and consumer pausing (#63)

Overall assessment: approve with minor comments. This is a high-quality, well-reasoned PR that implements a subtle consensus-critical feature with rare attention to soundness and edge cases. The design document is excellent, and the implementation traces it faithfully. I verified the build compiles and all new and existing tests pass.

Scope

12 commits, 100 files, +18986/−1846. The first two commits (d25a408, a48a7e0) are standalone bug fixes against main; the rest build the downtime pipeline. New Go code is concentrated in:

• x/vaas/consumer/keeper/downtime.go — consumer-side tumbling-window detection
• x/vaas/provider/keeper/downtime*.go — evidence validation, slash pricing/execution, challenge verification, pruning
• x/vaas/provider/keeper/fees.go — fee exclusion + pool-as-escrow
• x/vaas/provider/keeper/consumer_lifecycle.go — PAUSED phase + auto-stop/resume
• x/vaas/consumer/ibc_module.go, relay.go — source-port + chain-id pinning

Soundness of the core design

The asymmetry the PR builds around (downtime is disprovable, not provable; only chain-sealed signatures are unforgeable) is correctly translated into code:

• Challenge soundness (verifySealedCommitSignature): the commit.Hash() == header.Header.LastCommitHash check is the load-bearing seal. The VoteSignBytes(chainID, int32(sigIdx)) uses the array index of the matching signature, which is what cometbft expects. Accepting both Commit and Nil flags is correct — Nil still proves liveness. Pubkey self-authentication via ed25519.PubKey(pubKey).Address() == valAddr correctly decouples from key-assignment state.
• Re-acceptance prevention: the DowntimeWindowFloors + AcceptedDowntimeWindows interaction is sound. The floor advances monotonically to the max pruned window end (guarded by key.K3() > floor), and the floor write precedes the record delete so a mid-prune failure cannot make a pruned window re-acceptable. Ancient windows hit the floor, live windows hit the retained records.
• BeginBlock ordering: the explicit TestBeginBlockOrdering_UnresponsiveStopCancelsPendingSlashBeforeSweep encodes the contract that SweepUnresponsiveConsumers / BeginBlockAutoStopPausedConsumers run before SweepPendingDowntimeSlashes. Same-block cancellation wins over execution.
• Resume atomicity: ResumeConsumerChain uses sendVSCPacketsToChainStrict (propagates errors) rather than the EndBlock wrapper (swallows them), with a clear explanation of why a silent queue-only "success" would corrupt the consumer's validator set under out-of-order IBC v2 delivery.

What I verified

• go vet clean on consumer and provider packages
• go test ./x/vaas/provider/keeper/ — pass (93s)
• go test ./x/vaas/consumer/... — pass
• Targeted run of all downtime/pause/challenge/resume/withheld-fee tests — pass

Minor comments / questions

These don't block merge but are worth considering.

1. Genesis import of StagedDowntimeParams panics on invalid input (x/vaas/consumer/keeper/genesis.go ~L105–128). The comment justifies this ("Halt InitChain on unusable staged params"), and GenesisState.Validate also rejects them, so an honest operator never hits it. But a state-export with a future schema change could halt InitChain. The defensive stance is defensible; flagging in case you prefer a log-and-skip.

2. recordWithheldFee's expired-but-not-swept branch (fees.go L337–354). When existing.ExpiresAt is in the past but the record hasn't been swept yet, the code overwrites amount (discarding existing.Amount) and sets a fresh expiry. This is consistent with the doc ("an expired-but-not-yet-swept record is replaced outright"), but the expired funds are still in the pool — the consumer kept them when the record expired. Re-escrowing a fresh amount against funds that were never moved is fine, but the validator loses the previously-escrowed claim. Intended? Given fee exclusion is a side effect of accusation (not a right), probably acceptable, but worth confirming this matches the intent.

3. liveEpochShare recomputes numBonded at pricing time (fees.go L141–151). DistributeConsumerFees and liveEpochShare both call GetBondedValidatorsByPower, but the consumer's numBonded can change between pricing (receipt of evidence in the current epoch) and distribution (epoch boundary). If a validator unbonds in between, the share recorded at distribution diverges from the share used to price. The doc acknowledges P resolves "live" for current-epoch windows; just noting the small window of inconsistency is inherent.

4. DowntimeEvidenceMaxAge + DowntimeChallengeWindow < trusting is checked against DefaultConsumerUnbondingPeriod only (ValidateInfractionParamsAgainst, params.go L207–220). The comment acknowledges this is "per-consumer deviations are operator guidance." Since the default is the only checked bound, a chain that configures longer consumer unbonding periods gets a stricter-than-needed constraint, and one with shorter is unprotected. The doc is honest about this; consider whether the operator guidance (section 9) is the right place vs. a runtime per-consumer check.

5. E2e test genesis patch note (e2e_setup_test.go): the comment about slash_fraction deserializing from nil to zero is an important footgun. Since InfractionParameters is "unmarshaled directly into InfractionParameters with no defaulting pass," any operator writing genesis by hand who omits slash_fraction silently gets zero-cap downtime slashes. Worth either (a) defaulting at genesis unmarshal time, or (b) a louder callout in the genesis documentation. The same applies to MinSignedPerWindow / SignedBlocksWindow at the consumer genesis.

6. findPendingDowntimeSlashContaining iterates pending slashes linearly (downtime_challenge.go L156–175). Bounded by the number of pending windows for a single (consumer, validator) pair — typically 1–2. Fine in practice; flagging only because a validator with a large backlog (e.g. long network partition) would make each challenge O(n) in pending windows.

Non-issues I checked and confirmed safe

• The BitmapSet bounds in the consumer's TrackMissedBlocks are safe: stale bitmaps are padded to (window+7)/8 before indexing.
• MaxMissed formula W − ceil(M·W) matches the doc and is used identically on consumer (close) and provider (threshold check).
• SlashTokens units work out: P (fee tokens) · M / C (photons/bond_token) → bond tokens; fraction = slashTokens / totalTokens is dimensionless and capped by SlashFraction ∈ [0,1].
• Chain-id pinning is pre-seeded at genesis from the trusted provider client state, closing the "first packet teaches the pin" window.
• PendingDowntimeSlashes keyed by (consumer, validator, window_end_height) correctly coexists with multiple windows per pair; deletion-on-last-execute correctly leaves the WithheldFeeRecord alive while any window is still pending.

Recommendation

Approve. The design is thoughtful, the implementation is careful and matches the doc, tests cover positive paths, negative controls, ordering contracts, and the full queue-then-execute lifecycle. The six minor items above are worth a follow-up issue or a quick reply, none are blockers.

[tbruyelle]

Incredible work, the design is very solid.

Me and my friend claude just found a couple of bugs that needs to be fixed IMO, see the comments.

[tbruyelle]

I've discovered later that the issue raised in this comment has been addressed in #65. I keep the comment because I spent too much time on it and it would be painful for me to remove it xD. In addition the issue still exists right after launch, as mentioned in the Note2.

I have doubts about this point being "out of scope". Indeed it looks quite easy to override a provider client id, there is no need to forge a light-client history, you can just create a legit one:

1. run a one-validator chain with a chain-id equivalent to the provider chain (e.g. `atomone-1). Let's call it the attacker chain.
2. create a new IBC client on the consumer chain for the attacker chain. While the chain-id is identical to the provider chain, the consensus state comes from the attacker's valset (surprisingly IBC has no notion of chain-id uniqueness).
3. register the counterparty (at this point, everything is legit)
4. send a VSC packet from the attacker chain, using a valset_update_id large to exceeds the existing one (e.g. 999999)
5. the consumer chain accepts it, chain-id passes the check as it corresponds to the pinned one (atomone-1), SetProviderClientID() is invoked with the client id of the attacker chain, overriding the legit one from the provider chain.

Afterwards SendEvidencePackets every downtime packets to the attacker chain (so they can be silently ignored: the real purpose of this attack), and the real provider's next VSC also fails because the valset update id stays below the highest registered one (999999).

A naive fix could be to simply reject the VSC packet when a client id is already registered and is different from the provided one:

// in OnRecvVSCPacketV2, once a provider client is established
if current, found := k.GetProviderClientID(ctx); found && current != consumerClientID {
    return errorsmod.Wrapf(types.ErrInvalidProviderClient,
        "packet arrived over client %s, expected established provider client %s",
        consumerClientID, current)
}

But this fix prevents the consumer chain from restoring an expired client. So let's just add a condition to the client status, the consumer chain rejects the override of an existing client id if the related client is still active:

current, found := k.GetProviderClientID(ctx)
switch {
case !found:
    k.SetProviderClientID(ctx, consumerClientID)
case current != consumerClientID:
    _, hasCounterparty := k.clientV2Keeper.GetClientCounterparty(ctx, current)
    if hasCounterparty && k.clientKeeper.GetClientStatus(ctx, current) == ibcexported.Active {
        return errorsmod.Wrapf(types.ErrInvalidProviderClient,
            "packet arrived over client %s but %s is already established and active",
            consumerClientID, current)
    }
    k.SetProviderClientID(ctx, consumerClientID)
}

---

Note1: before this PR, this valset injection was even easier, so this PR makes it harder but there's still some vulns that need to be fixed.

Note2: this doesn't fully close the issue, two windows remain where the established client can still be overrided:

• an expired or frozen light client: though this needs the relayer to stop refreshing for a whole trusting peruid.
• right after launch: this is the most reachable one, the attacker only has to beat the relayer's first VSC delivery.

[giunatale]

Confirmed, and the attack works as written. I reproduced the reasoning against the code
rather than only the docs: authenticateProviderChainID was the entire gate and it compares
the chain-id string, so a chain reporting atomone-1 passes it while carrying a different
valset, and SetProviderClientID then overwrote the pin unconditionally. The consequence you
name is the serious half. Evidence packets get addressed to the attacker's client and
disappear, and the large valset_update_id in the same packet strands the real provider
below the dedup watermark permanently. Both fail silently.

Fixed here rather than left to #65, so main is never in this state. I used a slightly
different discriminator than your patch: a registered IBC v2 counterparty instead of
client status.

pinned, found := k.GetProviderClientID(ctx)
if !found {
    // both genesis paths pin, so this is malformed genesis or corrupted state
    return errorsmod.Wrapf(types.ErrInvalidProviderClient, ...)
}
if pinned == consumerClientID {
    return nil
}
if _, hasCounterparty := k.clientV2Keeper.GetClientCounterparty(ctx, pinned); hasCounterparty {
    return errorsmod.Wrapf(types.ErrInvalidProviderClient, ...)
}
k.SetProviderClientID(ctx, consumerClientID)  // the unroutable genesis pin, replaced once

The reason is your own Note2 window (a). Counterparties cannot be unregistered, so a pin that
has one is routable for good and neither expiry nor a freeze reopens the override. A
status check would reopen it, since it lets a client that has gone non-Active be replaced.
The tradeoff is that recovering an expired pin becomes a governance MsgRecoverClient, which
substitutes fresh client state under the same client id and leaves the pin intact; that is
the intended and only re-key path. It also happens to be the mechanism #65 already carries,
so the two converge instead of competing.

Your Note2 window (b), the launch race, is real and survives this. The genesis client has no
counterparty, so the first routable client to deliver wins the pin. It is being handled in
#65, where the consumer can content-check the delivering client's consensus state against the
provider valset it already holds in genesis (ProviderInfo.InitialValSet) instead of
trusting whoever arrives first.

Two things fell out of this that are worth flagging:

• The threat-model section you commented on listed this under Out of scope, claiming it
  "requires a forged light-client history". That was wrong on both counts and is now
  corrected to describe the attack and what stops it.
• There was a test asserting the vulnerable behaviour as a requirement:
  TestOnRecvVSCPacketV2SameChainIdHealsClient, with "a same-chain-id client replacement must still be accepted". Removed, and its one still-valid claim (the chain-id pin survives
  a rejection) moved into the new override test.

Three tests cover it now: a routable pin cannot be overridden, an unroutable genesis pin is
adopted once, and no pin at all is rejected rather than established by the packet.

[tbruyelle]

There are three other places where paused consumer chains are excluded:

1. UpdateConsumer : maybe it's intended, maybe it's not ?
2. ValidatorConsensusKeyInUse (key_assignement.go:235): this one is more annoying because a paused consumer's assigned keys become invisible to the collision guard. Can potientially be exploited?
3. AssignConsumerKey (key_assignment.go:74): new keys to the paused consumer are rejected

[giunatale]

All three are real. They split into one safety bug and two policy calls. I checked each
against IsConsumerActive, which was {REGISTERED, INITIALIZED, LAUNCHED}, so PAUSED was
invisible to every caller.

2. ValidatorConsensusKeyInUse: yes, exploitable, and it was the worst of the four. The
guard runs from the AfterValidatorCreated staking hook and iterates
GetAllActiveConsumerIds, which skipped PAUSED. So while a consumer is paused, a new provider
validator can be created with a consensus key that already serves as some other validator's
assigned key on that consumer. A pause does not release assignments, since they are what the
resume snapshot is rebuilt from, so on resume ResumeConsumerChain queues an immediate
snapshot carrying two validators at one consensus address, and CreateConsumerValidators
on this branch has no dedup. That halts the consumer.

Fixed by making PAUSED part of IsConsumerActive, since the predicate means "still exists and
holds state" and a paused consumer does.

3. AssignConsumerKey: agreed, it should be allowed. A pause runs up to
MaxPauseDuration and resolves on someone else's governance decision, so a validator that
needs to replace a compromised consumer key should not have to wait for that. It follows from
the IsConsumerActive change.

That one had a consequence you didn't mention and I nearly missed. AssignConsumerKey
branches on phase == LAUNCHED to decide whether to schedule the old consumer address for
pruning or delete it outright. A paused consumer would have taken the delete branch, and a
paused consumer is precisely the one with downtime state in flight, since a pause is entered
by a successful challenge. Both the challenge lookup and the re-submission defence resolve an
accused consumer address through that mapping, so deleting it would strand them. That branch
now treats PAUSED like LAUNCHED.

1. UpdateConsumer: intended, and now explicit instead of incidental. An update can
rewrite the infraction parameters and initialization parameters that the challenge causing
the pause was judged under, and the resume path replays state built from them. Governance
resumes or removes first, then updates. It no longer relies on IsConsumerActive to express
that: there is an explicit PAUSED check with the reason, plus a test pinning the asymmetry so
nobody "tidies" it back into symmetry with key assignment later.

Not changed: fees.go also iterates GetAllActiveConsumerIds, but it immediately filters to
LAUNCHED, so it was and remains LAUNCHED-only.

[tbruyelle]

verifyClientMessage() doesnt check the client status. It incidentally returns an error for an expired client because the trusting period is checked, but returns no error for a frozen client.

Given that a client becomes frozen when there is equivocations, we cannot rely on the return value of VerifyClientMessage() to approve the header in that case. I suggest we gate this call to active clients only.

[giunatale]

Confirmed. verifyDowntimeChallengeHeader called lightClientModule.VerifyClientMessage
with no status check, and your reading of why is right: that path validates the header against
the trusted state, trust level and trusting period, so expiry surfaces only incidentally
through the trusting-period check and a freeze does not surface at all. Frozen is exactly the
state a client enters once the chain behind it is proven to have equivocated, and on this
branch nothing stops a consumer whose client is frozen, so its headers could keep cancelling
slashes and pausing it.

Gated on Active, with every other status failing closed rather than enumerating the bad
ones. Placed at the top of verifyDowntimeChallengeHeader rather than in
HandleChallengeConsumerDowntime on purpose: the gate belongs next to the call it is
compensating for, and there it cannot be bypassed by a second caller later. It also sits
deliberately outside the OverrideVerifyDowntimeChallengeHeaderForTest seam, because that
seam exists so unit tests need not fabricate a client store for header verification, not so
they can skip the status check. The gate is therefore enforced on every path, tests included.

Covered for Frozen, Expired and Unknown, each asserting the pending slash survives and the
consumer stays LAUNCHED.

[tbruyelle]

This return prevents a revert of the downtime params if the revert happens in the same window of the initial set.

1. Provider params A to B: a VSC packet carries B; consumer stages B (window still running under A).
2. Provider reverts B to A before the window closes. Another VSC packet carries A, but it's not staged since it's the same as the window.
3. closeWindow: applyStagedDowntimeParams writes B. Consumer now measures under B while the provider is on A.

What's missing is a remove of the potential staged downtime params in case the new ones are equal to window's ones.

if current.SignedBlocksWindow == p.SignedBlocksWindow && current.MinSignedPerWindow.Equal(p.MinSignedPerWindow) {
	// The incoming params match what is already active, so any pending stage
	// was reverted before it took effect -- drop it rather than let the next
	// window boundary activate a value the provider no longer uses.
	if err := k.StagedDowntimeParams.Remove(ctx); err != nil {
		panic(err)
	}
	return
}

[giunatale]

Confirmed, and the sequence reproduces exactly as you wrote it. StageDowntimeParams
compares the incoming params against the active ones, so a revert arriving before the window
closed looked like a no-op and returned without touching the stage, leaving the abandoned
value to activate at the boundary.

The consequence is the one this whole design cannot absorb: consumer and provider then
disagree on the parameters for the same window. The consumer closes it computing MaxMissed
from B while the provider checks the threshold echoed on the packet against A, so the same
window is measured one way and priced another.

Fixed essentially as you proposed. The equal-params branch now removes any pending stage
before returning, with a comment on why the branch is not simply a no-op. The test stages a
change mid-window, reverts it, closes the window, and asserts the consumer is still on the
original params; it fails without the Remove.

[giunatale]

@julienrbrt answering the six minor items from your review, since they are in the review body
rather than inline threads. Two led to changes, one is already handled in #65, three are
answers with no change.

1 · Genesis panics on invalid StagedDowntimeParams. No change. The scenario needs a
state export whose schema has drifted, and this module has no migration path by design: it is
pre-release with nothing deployed, so there is no earlier version of the schema to drift
from. GenesisState.Validate rejects the same input first, so InitChain is reached only by an
operator who bypassed validation. Halting is the right outcome there, because staged params
that cannot be parsed mean the next window boundary would activate something unusable, and a
consumer measuring downtime under unusable params is worse than a chain that will not start.

2 · recordWithheldFee dropping an expired claim. Intended, and your reasoning for why
it is acceptable is the reason it is correct. The escrow exists to make a validator whole if
it challenges successfully. Once the challenge window closes unchallenged, the accusation
stood and the claim is extinguished: the amount stays with the consumer and is released on the
next sweep. Carrying the old amount forward would re-escrow funds nobody can claim. Since
this needed asking, the code now says so rather than only the doc.

3 · liveEpochShare recomputing numBonded at pricing time. No change, this is
inherent. P for a current-epoch window has no recorded value to resolve, so it is priced
live and the bonded count can move before distribution. The alternative, pricing at
distribution time, would make a slash depend on state well after the infraction, which is
worse. The window is bounded by one epoch.

4 · ValidateInfractionParamsAgainst checking only DefaultConsumerUnbondingPeriod.
Already addressed in #65, which is where it belongs since that PR is what makes the client's
trusting period observable. Adoption there rejects a client whose TrustingPeriod is not
above DowntimeEvidenceMaxAge + DowntimeChallengeWindow, and a MinConsumerUnbondingPeriod
floor stops a consumer being registered with an unbonding period too short to support the
window. That turns the default-only compile-time bound into a real per-consumer runtime check.

5 · slash_fraction deserialising to zero. Half right, and the half that was right is now
fixed. A nil fraction is already rejected: GenesisState.Validate reaches
ValidateFraction, which refuses nil, and there is a test for the omitted-key case. Zero
was the actual hole, since it sits inside [0,1] and passes. double_sign.slash_fraction is
now required to be positive, because zero there removes the entire economic deterrent for
equivocation while the parameters still read as configured, and nobody selects that on purpose
by leaving a field at zero. downtime.slash_fraction still accepts zero, since jail without
slashing is a legitimate policy a chain may want.

I did not take the "default at genesis unmarshal time" option. Substituting a value the
operator did not write is worse than refusing the input: it would mean a genesis that reads
one way and runs another, which is the failure mode this whole PR is about.

6 · Linear scan in findPendingDowntimeSlashContaining. No change. It is bounded by the
pending windows for a single (consumer, validator) pair, which is one or two in practice, and
a long partition raises it only as far as the acceptance floor allows before older windows are
pruned. A challenge is a single transaction paying its own gas, so the cost lands on the
challenger.

On the verification you ran, the same three are clean here, along with make lint. The four
items from @tbruyelle's review are fixed in d7b4149 with 11 new tests, each written failing
first and each production guard mutation-checked. The e2e suites still need one run against
those fixes before this merges.