fix: make the consumer and provider genesis round-trips restart-safe by giunatale · Pull Request #68 · allinbits/vaas

giunatale · 2026-07-30T16:01:41Z

Fixes three export/restart correctness bugs, removes two vestigial per-block
maps, adds the missing restart runbook, and proves the consumer round-trip end
to end.

The bugs

The consumer's packet-dedup watermark was not exported. VSC packets are
out of order and deduplicated via HighestValsetUpdateID; a restarted
consumer came back with the watermark at zero and would re-apply stale
updates. The watermark is now a genesis field, exported and restored.
The provider's stored per-consumer validator set was not exported. After
a provider restart the stored set was empty, and diffing the live bonded set
against an empty set emits only additions — a validator that unbonded during
the outage would never get its power-0 removal and would keep consensus power
on the consumer indefinitely. Instead of exporting the set,
QueueVSCPackets now treats an empty stored set for a launched consumer as
must-snapshot: the first post-restart epoch sends a full snapshot, which
reconciles the consumer regardless of what it held. (The same path covers a
consumer's very first epoch.)
consumer export emitted null validator pubkeys. CometBFT's genesis
validation dereferences each pubkey on reload, so export-then-start was a
broken round-trip. The export now carries every validator's consensus
pubkey.

Also removed: two write-only per-block maps (provider ValsetUpdateBlockHeight
and consumer HeightValsetUpdateIDs) whose proto fields are kept reserved.

The consumer's in-debt flag survives a restart too

The consumer's in-debt flag gates its own transaction admission alongside the
VSC-staleness clock, but only the clock was round-tripped: a debt-gated consumer
that restarted from a state export came back ungated. Both arms are now
exported. The provider's copy of the flag is exported as well, rather than
relying on it being recomputed after import — the recomputation only runs at an
epoch boundary and only for launched consumers, so a provider restart while a
consumer was paused-and-in-debt, followed by a resume, would stamp "not in debt"
into the forced snapshot and clear the gate the consumer had just correctly
restored.

Genesis validation of withheld-fee records now checks the amount like every
sibling validator does: an omitted amount produced a nil big integer that
validated cleanly and then panicked during fee distribution, and a negative one
inflated the funds the pool believed it had.

Runbook

docs/genesis-restart-runbook.md documents the export/import contract of both
modules, what is deliberately re-derived after restart, and the halt/upgrade
procedure — including one operational constraint discovered while testing this
PR end to end: advancing an IBC client past a restart requires the relayer to
fetch validator sets at pre-restart heights from the restarted chain, so an
export-based restart with a fresh data dir stalls packet flow from the
restarted chain (after a consumer restart the ack-driven liveness clock on the
provider is affected too). The runbook documents the two escape hatches (keep
the pre-restart block store queryable, or governance client recovery).

Testing

Unit: full export -> reload round-trips for both modules, must-snapshot on an
empty stored set, exported pubkeys survive CometBFT's reload validation.
A new Docker e2e subtest restarts the real consumer from its own export:
quiesces the relayer, stops the container, asserts the exported genesis
carries non-null pubkeys and the exact committed watermark, restarts a fresh
container from the export on the same network identity, and asserts blocks
resume, the consumer stays LAUNCHED on the provider, and validator-set
updates converge with every post-restart update id strictly above the
restored watermark. Full main e2e suite green with it.

giunatale · 2026-07-30T16:02:20Z

Branched from giunatale/docs/refresh (#67) (hard dependency: its restart runbook slots into the refreshed documentation set). Opened against that branch; will retarget to main once it lands.

julienrbrt · 2026-08-06T09:01:37Z

+	consumertypes "github.com/allinbits/vaas/x/vaas/consumer/types"
+)
+
+// TestGetValidatorSetCarriesPubKeysAndReloads is the M6 property: the exported


let's simplify this. what is M6 (except a tv channel).

silly leftover from in-dev references. I'll fix, thanks for noticing!

Fixed in 85366ff: the codename is gone and the test is slimmed per the ask (one validator proves the property, and ValidateAndComplete's error return needs no NotPanics wrapper). A sweep found two more in-dev references (M3) hiding in the provider restart test; the same commit removes them.

tbruyelle

Good fixes. I've added a bunch of comments that needs your attention.

Some of them are not related to the changes of this PR, but are related to genesis round-trips, so I think they should be fixed here if you consider them valid.

tbruyelle · 2026-09-01T08:05:27Z

 		}
-		if len(gs.HeightToValsetUpdateId) != 0 {
-			return errorsmod.Wrap(vaastypes.ErrInvalidGenesis, "HeightToValsetUpdateId must be nil for new chain")
-		}


What about raising an error if gs.ConsumerInDebt is true ?

Added for new chains in 8061569: the flag is provider-relayed state, so a chain that never launched cannot carry it. A restart genesis keeps carrying it deliberately: restoring preserves the debt gate until the next VSC re-asserts it.

tbruyelle · 2026-09-01T08:06:49Z

+	// Restore the debt flag on a restart (see ExportGenesis): the other arm of
+	// the tx-admission gate LastVSCRecvTime drives. Without this a debt-gated
+	// consumer comes back ungated until the next VSC packet re-asserts the
+	// flag. False is the absent case and matches a fresh keeper exactly, since
+	// IsConsumerInDebt reads an unset flag as not in debt.
+	if state.ConsumerInDebt {
+		k.SetConsumerInDebt(ctx, true)
+	}
+
+	// Restore the out-of-order dedup watermark on a restart (see ExportGenesis
+	// and OnRecvVSCPacketV2). Without this a state-export restart resets the
+	// watermark to found=false, so a stale diff VSC still sitting in IBC state
+	// would be accepted and applied over a newer set. A watermark of 0 is the
+	// absent case (new chain / no VSC applied yet); VSC ids are strictly
+	// positive, so leaving it unset there matches a fresh keeper exactly.
+	if state.HighestValsetUpdateId != 0 {
+		if err := k.SetHighestValsetUpdateID(ctx, state.HighestValsetUpdateId); err != nil {
+			panic(fmt.Errorf("init: set highest valset update id: %w", err))
+		}
+	}


For consistency, I would move these 2 conditions in the "restart" branch (the else of the state.NewChain).

Moved in 8061569.

tbruyelle · 2026-09-01T08:10:15Z


 	clientID, ok := k.GetProviderClientID(ctx)
 	if !ok {
 		return types.DefaultGenesisState()


This is not related to this PR, but while we're fixing genesis round trips, is it OK to discard the real parameters in case there is no client ID?

Valid, and already fixed on #65 as part of the owner-declaration rework: the export there keeps the params, the chain-id pin, and the safe-mode clock, with the client id simply exported empty, so an unpinned chain restarts still waiting to be pinned. It reaches this branch through the merge order (#65 lands before #67/#68); duplicating it here would only manufacture a conflict.

tbruyelle · 2026-09-01T08:38:02Z

-		for _, h2v := range state.HeightToValsetUpdateId {
-			k.SetHeightValsetUpdateID(ctx, h2v.Height, h2v.ValsetUpdateId)
-		}
 		k.SetProviderClientID(ctx, state.ProviderClientId)


Not in this PR but it might be worth fixing it: state.ProviderClientID must exist as a real IBC client or else the chain is pinned to a dead id.

Let's check if the client exists before:

// The exporting chain had a working provider client, so a restart genesis // naming one that does not exist here means the VAAS and IBC genesis // fragments came from different exports (or were hand-edited). Fail at // InitChain rather than pinning a dead id: authenticateProviderChainID // would then reject every inbound packet and the consumer would run on // its genesis valset forever, with no error on either side. // // Safe to look up: app.go orders ibchost before ibcconsumertypes, so the // client store is populated by the time this runs. if _, found := k.clientKeeper.GetClientState(ctx, state.ProviderClientId); !found { panic(fmt.Errorf( "init: genesis pins provider client %q, which does not exist in the IBC client store", state.ProviderClientId, )) } k.SetProviderClientID(ctx, state.ProviderClientId)

Added in 8061569, essentially as you wrote it (the premises check out: the app initializes ibc before vaasconsumer, and the keeper already holds the client keeper). A new test drives the panic.

tbruyelle · 2026-09-01T09:36:58Z

 		if floor, ok := floorByPair[pk]; ok && e.WindowStartHeight <= floor {
 			return fmt.Errorf(
 				"accepted downtime window for consumer %d validator %x starts at or below the pair's pruned floor: start %d, floor %d",
 				e.ConsumerId, e.ProviderConsAddr, e.WindowStartHeight, floor)


This error can occur with an exported genesis because of the current logic in PruneAcceptedDowntimeWindows:

Claude output:
Line 163 raises the floor to the pruned window's end height, and nothing checks whether a retained record sits below that:

if errors.Is(err, collections.ErrNotFound) || key.K3() > floor { k.DowntimeWindowFloors.Set(ctx, pairKey, key.K3()) // K3 = window END }

Pruning is ordered by AcceptedAt, not by height. Accepted windows for a pair are disjoint but arrive in any order — HandleConsumerDowntime supports that deliberately, because IBC v2 delivery is unordered and relaying is permissionless.

So an earlier-accepted-but-higher window prunes first:

t=0 accept [300,400] AcceptedAt = 0 t=10 accept [100,200] (disjoint, ok) AcceptedAt = 10 t=horizon prune [300,400] → floor = 400 [100,200] retained (still inside the horizon)

Two things break:

The export becomes unimportable. ExportGenesis emits the retained record alongside the floor, and validateAcceptedDowntimeWindows (provider/types/genesis.go:368) rejects its own output:

accepted downtime window for consumer 0 validator ... starts at or below
the pair's pruned floor: start 100, floor 400

Runtime over-rejection. The accept path rejects any new evidence with WindowStartHeight <= floor, so heights 200–400 — never accepted, never pruned — are now permanently unusable for that pair.

Confirmed, exactly as analyzed: pruning is eligible by AcceptedAt while windows arrive in any height order, so the floor could vault a retained lower window. Fixed in 8c01248: the sweep now prunes lowest-first per pair, holding an expired window until every lower window of its pair is pruned; a held window stays guarded by its own retained record, so nothing becomes re-acceptable early. The new test reproduces your timeline and asserts the interleaved state export round-trips.

tbruyelle · 2026-09-01T11:31:25Z

+		if err := sdk.ValidateDenom(r.Amount.Denom); err != nil {
+			return fmt.Errorf("withheld fee record: invalid denom %q (consumer=%d, validator=%x): %w",
+				r.Amount.Denom, r.ConsumerId, r.ProviderConsAddr, err)
+		}


Three things here:

First, we should also check if the denom match the consumer's fee denom. In case of mismatch a panic will happen in BeginBlock (x/vaas/provider/keeper/fees.go:348), which will halt the chain.

This check cannot be handled here, because you don't have access to k.feeDenom. It needs to go in InitGenesis.

My agent also suggests a "belt-and-braces" against that potential panic in x/vaas/provider/keeper/fees.go:348:

if existing.Amount.Denom == amount.Denom && !existing.Amount.Amount.IsNil() { amount = amount.Add(existing.Amount) } else { k.Logger(ctx).Error("discarding withheld fee record with unexpected denom", ...) }

Secondly, r.ExpiresAt is not validated.

Lastly, the 2 checks from lines 306-313 on Amount sdk.Coin can one-lined using sdk.Coin.Validate().

All three in 8061569, with two notes. The denom check lives in InitGenesis as you say (validation cannot see the wired denom) and fails at InitChain; I left fees.go without the belt-and-braces, since runtime records always carry the wired denom, the genesis guard closes the only injection path, and silently discarding escrow seems worse than the module's usual panic-on-corruption posture. Coin.Validate replaced the negative/denom pair, but the nil check stays separate: Validate panics on a nil amount rather than returning. ExpiresAt now rejects zero.

tbruyelle · 2026-09-01T12:02:03Z

+		// leaving it with consensus power on the consumer indefinitely. A
+		// snapshot reconciles the consumer's set regardless. This also covers
+		// a consumer's very first epoch, where a snapshot is equivalent to the
+		// all-additions diff it would otherwise produce.


This also covers a consumer's very first epoch, where a snapshot is equivalent to the all-additions diff it would otherwise produce.

It does not: LaunchConsumer already populates the consumer valset.

Right: LaunchConsumer populates it via ComputeConsumerNextValSet. Sentence dropped in 8061569.

tbruyelle · 2026-09-01T12:03:35Z

-// longer reachable.
+// EndBlockTrackValsetUpdates prunes per-consumer key-assignment entries that
+// are no longer reachable.
 func (k Keeper) EndBlockTrackValsetUpdates(ctx sdk.Context) {


The function name no longer matches its behaviour.

Renamed to EndBlockPruneKeyAssignments in 8061569.

tbruyelle · 2026-09-01T12:05:40Z

+- every consumer's `ConsumerState`: phase, owner, metadata, init params,
+  client id, consumer genesis, pending VSC packets, removal / pause-expiration
+  times, the liveness clock (`LastAckTime`, `HighestSentVscId`,
+  `HighestAckedVscId`), the previous consumer valset hash (the hash client


previous consumer valset hash

No such field exists.

Gone in 8061569: that was the July field the #65 rework removed.

- consumer: add highest_valset_update_id to the genesis proto and export/restore it, so a restarted consumer keeps deduplicating VSC packets by their update id instead of re-applying stale ones. - provider: treat an empty stored ConsumerValSet for a LAUNCHED consumer as must-snapshot, so the next epoch re-establishes the set after a restart rather than sending an empty diff. - consumer app export: set PubKey on each exported GenesisValidator so the set round-trips through genesis. - consumer: add consumer_in_debt to the genesis proto and export/restore it, mirroring last_vsc_recv_time. Both are arms of the same tx-admission gate and IsConsumerInDebt reads an unset flag as "not in debt", so a debt-gated consumer used to come back admitting ordinary transactions until the next VSC packet re-asserted the flag. - provider: add in_debt to ConsumerState and export/restore it too. Only the per-epoch fee distribution rewrites the flag, and only for LAUNCHED consumers, so it is not re-derivable at import the way the code comment and the runbook claimed: a PAUSED consumer resumed before its first post-restart distribution would be sent an immediate snapshot clearing a debt it still owes. - provider: validate the withheld-fee genesis amount (nil, negative, bad denom) like every sibling validator does. A genesis omitting the amount subfield used to pass Validate and panic later on a nil big.Int the first time the record was read for payment or escrow accounting. - drop the two vestigial per-block maps (ValsetUpdateBlockHeight, HeightValsetUpdateIDs) now that nothing reads them; their proto fields stay reserved. - add docs/genesis-restart-runbook.md describing the export/import round-trip. - e2e: restart the consumer from its exported genesis in TestVAAS, checking the exported valset pubkeys and dedup watermark and that VSC flow resumes. - test: cover the consumer's new-chain provider-chain-id pin, which nothing exercised -- the existing test covers only the restart branch.

in_debt sat at 18 only to leave room for a sibling branch's field 17, which that branch has since removed entirely. ConsumerState's numbering now runs 1 through 17 with nothing skipped and nothing reserved.

…g gaps The two vestigial maps this branch removed (ValsetUpdateBlockHeight, HeightValsetUpdateIDs) were dead ICS-fork bookkeeping, not part of the standalone-changeover machinery, so nothing will ever reintroduce their fields. Pre-release, a removed number is simply freed: the reserved markers are gone and every later field in both GenesisState messages shifts down by one, leaving the provider at a dense 1..16 and the consumer at 1..13.

The M6 and M3 labels were internal planning references that mean nothing to a reader of this repository. The export test also slims down per review: one validator proves the property, and ValidateAndComplete's error return needs no NotPanics wrapper.

Windows are accepted in any height order (delivery is unordered, relaying permissionless) but aged out by acceptance time, so an expired higher window could prune while a lower one was retained, vaulting the scalar floor over the retained record. That made the pair's own export unimportable (genesis validation rejects a record at or below the floor) and permanently rejected the never-accused heights in between. The sweep now holds an expired window until every lower window of its pair has been pruned; a held window stays guarded by its own retained record, so nothing becomes re-acceptable early.

A restart genesis pinning a provider client that does not exist in the IBC client store fails at InitChain instead of pinning a dead id that would silently reject every inbound packet. A new chain cannot start in debt: the flag is provider-relayed state, so genesis validation rejects it (a restart legitimately carries it). The debt-flag and dedup watermark restores move into the restart branch they belong to. A withheld fee record in a denom the module does not charge fails at InitChain, since accumulating it onto a runtime withhold would panic sdk.Coin.Add in BeginBlock, and its validation now also rejects a zero expiry and leans on sdk.Coin.Validate (the nil-amount check stays separate: Validate panics on nil). EndBlockTrackValsetUpdates is named for what it still does, EndBlockPruneKeyAssignments, and two stale sentences go: the snapshot comment's first-epoch claim (LaunchConsumer populates the valset) and the runbook's reference to a field that does not exist.

giunatale requested review from clockworkgr, julienrbrt and tbruyelle as code owners July 30, 2026 16:01

giunatale force-pushed the giunatale/docs/refresh branch from c6f2086 to cf9e63f Compare July 31, 2026 18:40

giunatale force-pushed the giunatale/fix/genesis-roundtrip branch 2 times, most recently from dda585e to 1c22145 Compare July 31, 2026 19:01

julienrbrt reviewed Aug 6, 2026

View reviewed changes

giunatale force-pushed the giunatale/fix/genesis-roundtrip branch from 5119148 to 4fcaf09 Compare August 26, 2026 10:04

giunatale force-pushed the giunatale/docs/refresh branch from cf9e63f to fcda155 Compare August 26, 2026 14:38

giunatale force-pushed the giunatale/fix/genesis-roundtrip branch from 4fcaf09 to 22b9165 Compare August 26, 2026 15:01

giunatale force-pushed the giunatale/docs/refresh branch from aa375e2 to 680ba62 Compare August 27, 2026 13:59

giunatale force-pushed the giunatale/fix/genesis-roundtrip branch from 22b9165 to dd081af Compare August 27, 2026 13:59

tbruyelle requested changes Sep 1, 2026

View reviewed changes

giunatale added 6 commits September 2, 2026 15:06

renumber the consumer state in-debt field to keep the sequence dense

2b99bf1

in_debt sat at 18 only to leave room for a sibling branch's field 17, which that branch has since removed entirely. ConsumerState's numbering now runs 1 through 17 with nothing skipped and nothing reserved.

giunatale force-pushed the giunatale/fix/genesis-roundtrip branch from b4b4bee to 8061569 Compare September 2, 2026 13:06

Uh oh!

Conversation

giunatale commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bugs

The consumer's in-debt flag survives a restart too

Runbook

Testing

Uh oh!

giunatale commented Jul 30, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbruyelle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbruyelle Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbruyelle Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giunatale commented Jul 30, 2026 •

edited

Loading

giunatale Aug 26, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading

tbruyelle Sep 1, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading

tbruyelle Sep 1, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading

giunatale Sep 2, 2026 •

edited

Loading