fix: let MsgRemoveConsumer erase a pre-launch consumer and release its chain id - #78
Conversation
| // signs to abandon a chain it no longer intends to launch; governance signs | ||
| // when the owner key is lost, which would otherwise pin the consumer -- and its | ||
| // chain id -- in place forever. | ||
| message MsgRetireConsumer { |
There was a problem hiding this comment.
I understand the reason of this PR but I do not like the new message. Why cannot we simply tweak MsgRemoveConsumer instead?
There was a problem hiding this comment.
Folded in b15867f. MsgRemoveConsumer is now the one removal message: pre-launch it erases immediately under the owner-or-gov admission (fee pool refunded, chain id released), launched or paused it keeps the gov-only stop with deferred erasure, and the event carries the phase at removal to tell the arms apart. The authority field became signer, retire-consumer's CLI became remove-consumer, and #76 and #67 were updated accordingly.
c8dbbcb to
58b4ae9
Compare
…umer PR #78 now removes pre-launch consumers through MsgRemoveConsumer and emits the remove event on both arms, so vaas_retire_consumer has no user anywhere.
…umer PR #78 now removes pre-launch consumers through MsgRemoveConsumer and emits the remove event on both arms, so vaas_retire_consumer has no user anywhere.
PR #78 merged MsgRetireConsumer into MsgRemoveConsumer per review: one message whose effect depends on the consumer's phase. The lifecycle, runbook, fee-pool, events, and queries pages now describe that shape, and the remove event documents its new consumer_phase attribute.
PR #78 merged MsgRetireConsumer into MsgRemoveConsumer per review: one message whose effect depends on the consumer's phase. The lifecycle, runbook, fee-pool, events, and queries pages now describe that shape, and the remove event documents its new consumer_phase attribute.
giunatale
changed the title
| // punished for is already outside the slashable window. Keeping the id past | ||
| // this point would reserve that chain id for good, since DELETED is | ||
| // terminal. | ||
| k.DeleteConsumerChainId(ctx, consumerId) |
There was a problem hiding this comment.
Unless I am wrong this makes BeginBlockRemoveConsumers delete as well the chainID of a previously launched chain.
There was a problem hiding this comment.
It does, deliberately: releasing at deletion on both routes is half the point of the PR. Without it a consumer removed by the liveness sweep can never re-register its own chain id, since DELETED is terminal. The safety argument is in the comment at this line: by deletion time the client mapping is already removed, so no inbound packet can be attributed to the chain (the provider resolves packets by destination client), evidence, downtime and fee distribution all require LAUNCHED, and on this route a full unbonding period has passed since STOPPED, so nothing is still slashable. Releasing earlier, at STOPPED, would be unsafe precisely because the client mapping still exists there. If you see a concrete path this rationale misses, that is exactly what I want to know.
| phase := k.GetConsumerPhase(ctx, consumerId) | ||
| if phase != types.CONSUMER_PHASE_STOPPED { | ||
| return fmt.Errorf("cannot delete non-stopped chain: %d", consumerId) | ||
| if phase != types.CONSUMER_PHASE_STOPPED && !k.IsConsumerPrelaunched(ctx, consumerId) { |
There was a problem hiding this comment.
When BeginBlockRemoveConsumers calls this https://github.com/allinbits/vaas/blob/c9df1f2/x/vaas/provider/keeper/consumer_lifecycle.go#L382-L388 it immediately errors.
There was a problem hiding this comment.
It does not error there: the condition is a conjunction, phase != STOPPED && !IsConsumerPrelaunched, so a STOPPED consumer fails the first clause and passes the gate. BeginBlockRemoveConsumers only dequeues consumers that StopAndPrepareForConsumerRemoval put in STOPPED, and TestBeginBlockRemoveConsumers_DeletesEligible drives exactly that route through the real queue. That said, the negated composition invited the misread, so 5ff01bc names the three deletable phases positively, the same readability fix the UpdateConsumer gate got on #63.
There was a problem hiding this comment.
yeah, well it feels weird to even call it then.
There was a problem hiding this comment.
Deliberate: the callers' checks are semantic gates (retire is prelaunch-only, the removal queue holds STOPPED by construction), while this one is the teardown's own contract. DeleteConsumerChain is the destructive, irreversible half (pool sweep, state erasure, chain-id release), so it enforces the union of legal entry phases itself rather than trusting caller discipline, and a future third caller cannot erase a launched consumer by accident. That is the same keeper-level-guard posture the rest of the module takes. I added a sentence at the check saying exactly this (0f5cfb9).
PR #78 merged MsgRetireConsumer into MsgRemoveConsumer per review: one message whose effect depends on the consumer's phase. The lifecycle, runbook, fee-pool, events, and queries pages now describe that shape, and the remove event documents its new consumer_phase attribute.
…umer PR #78 now removes pre-launch consumers through MsgRemoveConsumer and emits the remove event on both arms, so vaas_retire_consumer has no user anywhere.
Creating a consumer is permissionless and, until now, nothing could undo it before launch: MsgRemoveConsumer accepts only a launched or paused consumer, MsgUpdateConsumer is owner-only, and DeleteConsumerChain deliberately kept the chain-id entry that ChainIdInUse consults with no phase filter. So a single transaction reserved any chain-id string for good, a consumer whose owner key was lost sat in the registered phase forever with state nobody could prune, and a consumer the liveness sweep removed could never re-register under its own chain id. MsgRetireConsumer erases a consumer that is still registered or initialized. Its signer may be the consumer owner or the gov authority, the same owner-or-gov admission MsgFundConsumerFeePool and MsgWithdrawConsumerFeePool use: the owner arm lets whoever registered a chain abandon it, and the gov arm is the remedy when the owner key is lost. MsgRemoveConsumer keeps its gov-only, launched-or-paused scope, because a live chain must stop and wait out the unbonding period before its state can go; a chain no validator ever validated has nothing to unbond, so retirement runs the same teardown immediately. It reuses DeleteConsumerChain instead of repeating part of it, so a funded fee pool is still paid back to its depositors pro rata, and it drops the spawn-time queue entry an initialized consumer holds. DeleteConsumerChain now also releases the chain id, on both routes into it. That point is safe and an earlier one is not: the client mapping is removed in the same call, so no inbound packet can be attributed to the consumer any more (the provider resolves packets by destination client), and evidence, downtime accusations and fee distribution all require the launched phase; on the stop-then-remove route a full unbonding period has additionally passed since the stop, so nothing the chain did remains punishable. The tombstone still keeps owner, metadata, initialization parameters and phase so a removed consumer stays visible to explorers, and genesis now requires a deleted consumer's chain id to be empty, which is what makes the release survive a state-export restart. Two assumptions the pre-launch path broke are fixed alongside: DeleteConsumerClientId panicked on a consumer that never had a client adopted, and the genesis export/import path plus the consumer-chain queries assumed every consumer record carries a chain id.
Fee-pool funding is permissionless and withdrawal is depositor-gated with a gov-only arm while the consumer is launched or paused; neither uses the owner-or-gov admission this handler does.
Per review, one message removes a consumer whatever its phase, instead of a separate MsgRetireConsumer. A consumer that has not launched is erased immediately by RetireConsumerChain under the owner-or-gov admission, its fee pool refunded and its chain id released; a launched or paused consumer keeps today's gov-only stop with erasure deferred by the unbonding period; stopped and deleted are rejected. The message's authority field becomes signer, since it no longer always holds the gov account, and the one event carries the phase at removal so indexers can tell the immediate arm from the deferred one. The retire-consumer CLI command becomes remove-consumer.
The stopped-or-prelaunched admission read as a negated composition that review found easy to misparse; the three phases it admits are now named positively, same as the UpdateConsumer gate.
84edb8f to
5ff01bc
Compare
The review stumbled here twice: the callers gate their own phases, and this check is the teardown's, independent of caller discipline.
23a41ea to
0f5cfb9
Compare
2 participants
Closes two gaps in the consumer lifecycle: a consumer that never launched could
not be terminated, and a chain id was reserved forever by the first transaction
that used it.
What was wrong
MsgRemoveConsumerrequires LAUNCHED or PAUSED,MsgUpdateConsumerisowner-only with no authority path, and the chain-id uniqueness check iterates
every registered chain id with no phase filter — while
DeleteConsumerChaindeliberately kept the chain-id entry and the function that would remove it had no
caller. Since consumer creation is permissionless with no deposit, that means:
one transaction permanently reserves any chain-id string with no governance
remedy; an owner who loses their key before launch strands a consumer in
REGISTERED forever with state nothing can prune; and a consumer removed by the
liveness sweep can never re-register under its own chain id.
What this changes
MsgRemoveConsumernow covers the whole lifecycle, with an effect thatdepends on the consumer's phase. Pre-launch (REGISTERED or INITIALIZED) it
erases the consumer immediately: no validator ever validated the chain, so no
unbonding delay applies, and either the consumer's owner or the governance
authority may sign — which covers the lost-key case. Launched or paused it
keeps today's governance-only stop with erasure deferred by the unbonding
period. The message's authority field becomes
signer, since it no longeralways holds the governance account, and the remove event carries the phase at
removal so indexers can tell the immediate arm from the deferred one. The
pre-launch arm reuses the full deletion teardown rather than reimplementing a
partial one, and additionally drops the launch-queue entry an INITIALIZED
consumer holds. A funded fee pool is swept to its depositors pro rata on the
way out, with truncation dust to the community pool, so removing a pre-launch
consumer never strands deposits. The
remove-consumerCLI command submits themessage directly; the post-launch arm remains a governance proposal.
Chain ids are released at deletion, immediately before the phase becomes
DELETED, on both routes into it. That point is deliberate: the client mapping is
already gone by then and the provider authenticates inbound packets by
destination client, evidence and fee paths all require LAUNCHED, and nothing
anywhere reverse-maps a chain id to a consumer — so a later registration of the
same chain id cannot inherit anything. Releasing at STOPPED would not have been
safe, because the client mapping still exists there and a look-alike could get
the old consumer's still-pinned client adopted by chain-id matching.
Releasing the chain id turned out not to be self-contained: the DELETED
tombstone depends on the chain-id row, and dropping it naively would have made
provider genesis export panic after the first deletion, made import panic
on a legitimately exported genesis, and broken the main consumer-list query
entirely. All four sites are fixed, and genesis now requires a deleted consumer's
chain id to be empty so the release survives an export/restart. A latent panic in
the client-id deletion path for a consumer that never adopted a client — hidden
because every existing test set one first — is fixed too.
Testing
Owner erases REGISTERED and INITIALIZED; governance erases a lost-key consumer;
launched and paused reject the owner and stop under governance with the chain id
still reserved; STOPPED and DELETED are rejected for both signer arms; a
stranger is rejected and state is untouched; a funded pool is returned to
depositors; the chain id is re-registrable afterwards and is not released
while the consumer is non-terminal; and the stop-then-remove route is seeded
with fully lived state so the teardown assertion is non-vacuous.
Mutation-checked: removing the chain-id release fails four tests, keeping the
launch-queue entry fails the INITIALIZED case.