fix: arm safe mode for never-connected consumers and harden snapshot and photon fee edges - #66
Conversation
|
Branched from Note for reviewers: two statements in this PR's |
2b71a23 to
92ea64b
Compare
1ce861e to
39a5b5c
Compare
92ea64b to
021c4bd
Compare
39a5b5c to
3c074c0
Compare
| // fatal as an empty snapshot) is guarded rather than only the snapshot resync | ||
| // path. An empty-update heartbeat diff over a live set stays accepted: it | ||
| // leaves the set as it was, which is non-empty. | ||
| func (k Keeper) wouldEmptyValidatorSet(ctx sdk.Context, newChanges vaastypes.ValidatorSetChangePacketData) bool { |
There was a problem hiding this comment.
tbf, i do not see the use case of this. the validator set of a consumer and the provider does not diverge (as we do not support PSS), so this cannot trigger. I guess it is fine to leave it here but just noting.
There was a problem hiding this comment.
Same, I cannot think of case where the validator set would be empty. The provider side specifically ensures this doesn't happen. Are you defending against a malicous/buggy provider ?
If we remove that guard we need to update the doc as well https://github.com/allinbits/vaas/pull/66/changes#diff-eadc741663941dea689a5d9455f9aa364291ca593072e625a4c3c743d59157f6R82-R86
There was a problem hiding this comment.
Yes, a buggy provider is exactly what the guard is for. The one input it rejects is the one CometBFT cannot survive: an EndBlock that empties the validator set halts the consumer irrecoverably. With the guard the packet dies as an error ack, observable on the provider and costing nothing. A working provider never emits it, but the sender is another chain's code, so the consumer should not be the party that halts. Keeping it; the doc paragraph describes exactly this backstop role, so it stays too.
| } | ||
|
|
||
| providerClientID, ok := d.keeper.GetProviderClientID(ctx) | ||
| if !ok || !d.keeper.HasRoutableProviderClient(ctx) { |
There was a problem hiding this comment.
Can we add an e2e case for this? I do not believe we actually have any txs being sent from the consumer.
There was a problem hiding this comment.
A e2e test with a tx that uses bridged photons as fees would be nice too.
There was a problem hiding this comment.
Added in 72ea261. Enabling the policy takes genesis: the provider-authored consumer genesis always writes the flag off, and with no governance wired in the reference consumer app it cannot be flipped after launch, so the harness sets photon_fees_enabled in the consumer's genesis file the same way a real core-shard operator would when assembling the launch file, and starts the node with zero min-gas-prices, the matching launch config.
That means the whole main suite runs under the policy, not just the new test: from the moment the pin turns routable, every consumer tx is subject to it. The relayer exemption is exercised continuously, and the pre-existing subtests double as proof the policy does not disturb normal operation. The photon-off configuration stays covered by the liveness suite. On top of that, testPhotonFeeEnforcement proves the loop: a uatone-fee tx is rejected by the ante, an ICS-20 v2 transfer bridges uphoton in while enforcement is on, and a bank send paying voucher fees commits.
Writing it surfaced an ibc-go v10.2 wart worth knowing: channelv2 reads packet timeouts as unix seconds, while the transfer CLI's relative-timeout path emits nanoseconds (the v1 unit), so a plain tx ibc-transfer transfer against a v2 client always fails the 24h timeout cap. Absolute timeouts in seconds work; noted in the test.
There was a problem hiding this comment.
The v2 transfer timeout wart from the e2e work is filed upstream too: cosmos/ibc-go#9064
tbruyelle
left a comment
tbruyelle
left a comment
There was a problem hiding this comment.
A few comments, with the most important of which is: how do we transfer photons if we already need bridged photons to pays the fees ?
| // fatal as an empty snapshot) is guarded rather than only the snapshot resync | ||
| // path. An empty-update heartbeat diff over a live set stays accepted: it | ||
| // leaves the set as it was, which is non-empty. | ||
| func (k Keeper) wouldEmptyValidatorSet(ctx sdk.Context, newChanges vaastypes.ValidatorSetChangePacketData) bool { |
There was a problem hiding this comment.
Same, I cannot think of case where the validator set would be empty. The provider side specifically ensures this doesn't happen. Are you defending against a malicous/buggy provider ?
If we remove that guard we need to update the doc as well https://github.com/allinbits/vaas/pull/66/changes#diff-eadc741663941dea689a5d9455f9aa364291ca593072e625a4c3c743d59157f6R82-R86
| } | ||
|
|
||
| providerClientID, ok := d.keeper.GetProviderClientID(ctx) | ||
| if !ok || !d.keeper.HasRoutableProviderClient(ctx) { |
There was a problem hiding this comment.
A e2e test with a tx that uses bridged photons as fees would be nice too.
| return next(ctx, tx, simulate) | ||
| } | ||
|
|
||
| expected := ExpectedPhotonDenom(providerClientID) |
There was a problem hiding this comment.
I should have reported this in #57, but better late than never:
- this excludes bridged photons from another client, only photons bridged from the pinned client are accepted. Not a blocker but probably need to be documented.
- the empty-fee bypass is closed as soon as the client id is pinned. But how are we supposed to tranfer photons to pay fees if we need bridged photons to transfer them ?
There was a problem hiding this comment.
Both points were real, and the second was worse than stated: once the pin turned routable the ante required photon fees on every tx, including the relayer's MsgRecvPacket, which is the only way vouchers can arrive. Enabling the policy bricked the chain outright.
Fixed in 72ea261: transactions made up exclusively of infrastructure messages (/ibc.core.* and /cosmos.gov.*) are exempt from the policy. MsgSendPacket is carved out (it is the raw form of an outbound transfer, a user action), and one non-exempt message makes the whole tx enforced. What exempt txs pay stays node-local min-gas-prices policy: a tokenless core shard launches with zero prices and genesis-declared relayer and owner accounts, and raises prices once photon circulates. The new e2e proves the loop end to end: the voucher-delivering transfer arrives while enforcement is on.
On pinned-only vouchers: documented in 72ea261, and I think it should stay that way, likely permanently. Voucher denoms are path-dependent, so "photon via another client" cannot be verified locally; trusting another client by chain id would re-open what #65 closed, and the honest generic version degrades into a governance-maintained denom whitelist. One canonical path also keeps fees fungible.
A finding from working this: the reference consumer app wires no gov module at all (only the AppModuleBasic, a fork leftover for CLI codecs), so nothing can sign as the params authority and consumer params are genesis-frozen there, the photon flag included. Canonical gov would tally zero power anyway on the no-op staking keeper; embedders wanting post-genesis control need their own governance variant. Now stated in the ante and msg-filter docs.
There was a problem hiding this comment.
Sounds good.
One thing to remember is that an exempted tx in PhotonFeeDecorator doesnt mean it is exempted in DeductFeeDecorator. Validator's minimum-gas-price is supposed to be 0 at launch, but once raised, all txs, exempted or not, will have to pay what the validator has set in this setting. Maybe we should also override the DeductFeeDecorate.txFeeChecker in the consumer chain to make exemption more consistent. Wdyt ?
There was a problem hiding this comment.
Agreed and done in 93b1d28: the consumer wires an InfrastructureExemptTxFeeChecker into fee deduction, the SDK default plus the same infrastructure predicate skipping the CheckTx floor, so raising minimum-gas-prices no longer prices relayer traffic (an explicitly configured checker still wins). Two notes for the record: floors on a photon-only chain belong in the voucher denom, since a native-denom floor would reject the photon fees user txs must pay; and post-bootstrap this class of problem cannot hard-deadlock anyway, because vouchers already on the chain move by local bank send, so the relayer can be refilled without a relayed packet.
021c4bd to
295c3ae
Compare
cd1e24a to
69146fd
Compare
…k at the first wall-clock block Review round for PR #66. Photon-only fees deadlocked the chain the moment the policy activated: every transaction needed a photon-voucher fee, including the relayer's packet deliveries, which are the only way vouchers can ever arrive. The ante now exempts transactions made up exclusively of infrastructure messages (/ibc.core.* and /cosmos.gov.*, with the user-originating MsgSendPacket carved out); what they pay is left to node-local min-gas-prices, so a tokenless core shard launches with zero prices and genesis-declared relayer and owner accounts. The VSC staleness clock armed at NewChain InitGenesis with the genesis block time, which is the genesis file's creation time and may predate launch, burning safe-mode budget before the first block. It now arms in BeginBlock at the first block after genesis, the first one carrying wall-clock time; a restart genesis without the field arms the same way instead of staying unarmed forever. Docs are aligned with two facts the review surfaced: only vouchers from the pinned client are accepted (deliberate: any other path is either unverifiable or needs denom whitelisting), and the reference consumer app wires no governance module, so consumer params are genesis-frozen there and the photon flag cannot be flipped after launch. The liveness query doc now shows the REST path that exists on this branch instead of CLI syntax that lands with #74/#76. The main e2e suite now runs its consumer with photon fees enabled from genesis, and a new test proves the policy end to end: a native-fee tx is rejected, an ICS-20 v2 transfer bridges uphoton in while enforcement is on (the exemption at work), and a voucher-fee tx commits.
ea437e0 to
f45483a
Compare
…hoton fee edges A NewChain consumer now arms the VSC staleness clock at InitGenesis with the genesis block time, so a consumer whose relayer never materializes crosses SafeModeThreshold and enters restricted mode instead of running on its genesis validator set unrestricted forever (the unset clock falls back to the current block time and never reads stale). Restarts still restore the exported value, which is now always present for chains that launched as NewChain; PreVAAS chains stay unarmed until their first VSC, since the standalone staking keeper still runs the chain there. OnRecvVSCPacketV2 now rejects, with an error acknowledgement and before any state change, a packet whose application would leave the consumer with zero validators, the state in which the EndBlock flush halts the chain irrecoverably. Snapshots are judged by their own positive-power updates (they replace the set wholesale); diffs by overlaying the current cross-chain set with the accumulated pending changes and the packet's updates. Detecting the diff case reuses the existing accumulate helpers, so a diff removing every validator is guarded too, not only the snapshot resync path; empty heartbeat diffs over a live set keep flowing. PhotonFeeDecorator keeps deriving the expected voucher denom from the live provider client pin: the pin moves at most once, from the unroutable genesis client to the first VSC-delivering client, and ICS-20 vouchers can only arrive over that adopted client, so the derived denom tracks exactly what real vouchers carry (a static genesis-time param could never know the adopted client id). The decorator now splits its behavior at routability of the pin: before the pin is routable no voucher can exist, so it stays a full no-op -- rejecting the fee-less relayer traffic of that phase would block the first VSC from ever arriving -- and once routable it enforces that fees are non-empty and denominated in the voucher, closing the empty-fee bypass of the photon-only policy. Gas-estimation simulations are exempt from the non-empty requirement. Whether that policy applies at all is a new consumer param, photon_fees_enabled (field 8 of ConsumerParams, default false), so every node derives the decision from consensus state. The decorator has no CheckTx-only carve-out and therefore also rejects during FinalizeBlock, so a node-local switch would let two validators configured differently disagree on whether a transaction is valid and diverge on the app hash, with a validator joining after launch on the shipped default as a mundane trigger. Only the on/off decision lives in params; the expected voucher denom stays derived from the live pin, which no genesis-time value could name. A chain opts in through its consumer genesis or a governance MsgUpdateParams, which carries the flag through unchanged -- unlike the provider-owned downtime window fields, which that handler keeps preserving from the stored params. The reference consumer app wires the decorator unconditionally, immediately before fee deduction per the decorator's contract: insertion is app wiring and cannot depend on per-block state, so the decorator self-gates on the param and no-ops on chains that did not opt in. DisabledModulesDecorator and MsgFilterDecorator were already wired. The stale claim in docs/consumer-liveness.md that a fresh consumer is never stale before its first VSC is updated to the armed-at-genesis behavior. Four further corrections to that same doc, none behavioral: the headline packet-timeout claim now states the real 1h VaasTimeoutPeriod default instead of reading the 24h MaxTimeoutDelta ceiling as the effective timeout (the argument that a timeout is too weak a removal signal only gets stronger at an hour); the snapshot trigger lists all three conditions QueueVSCPackets tests rather than the acked/sent comparison alone, adding the locally queued packet and the empty stored consumer valset; the liveness query example uses the vaasprovider subcommand root; and last_ack_time is described as the query's one piece of stored state, with the other three fields derived from it. Tests: unit coverage for the genesis arming and the unarmed restart fallback, for empty-snapshot and set-emptying-diff rejection with state left untouched, and for the photon decorator across the param gate and both pin phases, including the simulation carve-out; for the param's default, its validity under either setting, the keeper accessor the decorator reads, its genesis round-trip and its governance toggle; and new app-level ante tests that run signed transactions through the full reference ante chain against real keepers, driving the policy from genesis params over routable and unroutable pins. Existing tests encoding the removed behaviors changed deliberately: TestSnapshotEmptyRemovesAll became TestEmptySnapshotRejected, the heartbeat-based debt and downtime-staging tests now seed a live validator set, and the photon ante table drops its empty-fee-passes case. No photon e2e is included: enforcing photon fees on the shared e2e consumer would break every other subtest's fee payments, and the harness has never exercised an ICS-20 v2 transfer relay to mint real vouchers; at the param's default the decorator no-ops, so the suites are unaffected by the wiring.
…k at the first wall-clock block Review round for PR #66. Photon-only fees deadlocked the chain the moment the policy activated: every transaction needed a photon-voucher fee, including the relayer's packet deliveries, which are the only way vouchers can ever arrive. The ante now exempts transactions made up exclusively of infrastructure messages (/ibc.core.* and /cosmos.gov.*, with the user-originating MsgSendPacket carved out); what they pay is left to node-local min-gas-prices, so a tokenless core shard launches with zero prices and genesis-declared relayer and owner accounts. The VSC staleness clock armed at NewChain InitGenesis with the genesis block time, which is the genesis file's creation time and may predate launch, burning safe-mode budget before the first block. It now arms in BeginBlock at the first block after genesis, the first one carrying wall-clock time; a restart genesis without the field arms the same way instead of staying unarmed forever. Docs are aligned with two facts the review surfaced: only vouchers from the pinned client are accepted (deliberate: any other path is either unverifiable or needs denom whitelisting), and the reference consumer app wires no governance module, so consumer params are genesis-frozen there and the photon flag cannot be flipped after launch. The liveness query doc now shows the REST path that exists on this branch instead of CLI syntax that lands with #74/#76. The main e2e suite now runs its consumer with photon fees enabled from genesis, and a new test proves the policy end to end: a native-fee tx is rejected, an ICS-20 v2 transfer bridges uphoton in while enforcement is on (the exemption at work), and a voucher-fee tx commits.
Per review: the photon decorator's exemption stopped at consensus, so a validator raising minimum-gas-prices would price the relayer traffic the exemption keeps flowing, one mempool at a time. The consumer now wires an InfrastructureExemptTxFeeChecker into fee deduction: it mirrors the SDK default checker, except a transaction made up exclusively of infrastructure messages skips the CheckTx floor. An explicitly configured checker still wins, and floors on a photon-only chain belong in the voucher denom, which the docs now say.
017268d to
93b1d28
Compare
3 participants
Four consumer-side hardening items.
Never-connected consumers now enter safe mode
LastVSCRecvTimewas only ever set on packet receipt, and the getter fell backto the current block time when unset, so a consumer that never received a
validator-set update was never considered stale and never entered safe mode.
The clock is now armed at the first block after genesis: the genesis block
itself carries the genesis file's
genesis_time, which may predate launch, soarming any earlier would burn safe-mode budget before the chain produced a
block.
SafeModeThresholdstaleness therefore accrues from launch. An explicittimestamp from a restart genesis still wins, a restart genesis without the
field arms the same way, and every accepted update re-arms the clock. (A
pre-VAAS standalone chain deliberately stays unarmed until its first update: a
live standalone economy must not enter restricted mode mid-changeover.) The
liveness documentation asserted the old behavior and was corrected.
A packet can no longer empty the validator set
A well-formed packet whose application would leave the consumer with an empty
validator set halts the chain at EndBlock.
OnRecvVSCPacketV2now rejects suchpackets with an error acknowledgement before any state write, for snapshots
(the packet's positive-power updates are the entire resulting set) and for
diffs (the current set overlaid with pending changes plus the packet,
reproducing exactly what EndBlock would apply). Rejection happens inside the
IBC cache context, so no state leaks; out-of-order stale packets still
deduplicate to a success acknowledgement first, preserving replay semantics.
Heartbeat diffs over a live set are unaffected.
Photon fee decorator: bypass closed, bootstrap and exemptions made explicit
The expected fee denom stays derived from the pinned provider client
(
ibc/SHA256("transfer/<clientID>/uphoton")): under client authentication thepin moves at most once at bootstrap, and IBC vouchers can only ever arrive over
the adopted, routable client, so the derived denom tracks exactly the denom
real vouchers carry, while a static genesis-time parameter could never name the
adopted client id. Vouchers bridged over any other path are deliberately not
accepted: they cannot be verified locally, and accepting them would take a
governance-maintained denom whitelist. The decorator godoc documents both.
Behavior splits on pin routability:
exist yet, so there is nothing to police, and rejecting the fee-less relayer
traffic of that phase would block the first validator-set update forever. The
message-filter decorator remains the traffic restriction for that phase.
AND the fee must be non-empty, closing the empty-fee bypass. Gas simulation
is exempt from the non-empty check only; a wrong denom still rejects in
simulation.
Transactions made up exclusively of infrastructure messages stay exempt while
enforcing:
/ibc.core.*and/cosmos.gov.*, with the user-originatingMsgSendPacketcarved out, and one non-exempt message makes the whole txenforced. The vouchers the policy demands can only arrive in relayer-submitted
packet deliveries, so pricing those in photon would deadlock the chain the
moment the policy activates. What exempt transactions pay stays node-local
min-gas-prices policy: a tokenless core shard launches with zero prices and
genesis-declared relayer and owner accounts, and raises prices once photon
circulates.
The decorator is wired, not shipped inert
PhotonFeeDecoratoris now always part of the reference consumer app's antechain, inserted immediately before fee deduction, and the on/off decision is a
consumer module parameter (
photon_fees_enabled, default false) rather thannode-local configuration.
That distinction is the point. The decorator rejects transactions in
FinalizeBlock, not only inCheckTx, so it is consensus-relevant: driving itfrom
app.tomlwould let two validators disagree on whether a transaction isvalid, diverging the app hash and halting the chain, and the trigger would be
as mundane as a validator joining after launch with the shipped default. As a
parameter it comes from state, so every node agrees, and it round-trips through
genesis. The provider-authored consumer genesis always writes it off; enabling
takes a hand-authored genesis or a later
MsgUpdateParamsfrom the paramsauthority, where the embedding app wires one. The reference consumer app wires
no governance, so there the genesis choice is final.
New app-level tests build the real consumer app once per parameter setting and
run fully signed transactions through the real ante chain against real keepers:
enabled + voucher denom accepted and deducted; enabled + wrong denom rejected
(from an unfunded account, proving the rejection is policy, not funds); enabled
exemption set is pinned by dedicated ante tests.
The main e2e suite runs its consumer with
photon_fees_enabledset in theconsumer genesis, the way a real core shard would enable it, so the whole suite
exercises the policy, and
testPhotonFeeEnforcementproves the loop end toend: a native-fee tx is rejected by the ante, an ICS-20 v2 transfer bridges
uphoton in while enforcement is on, and a bank send paying voucher fees
commits.
Testing
Unit + app-module suites green (including the app module's first ante tests);
golangci-lintclean; both Docker e2e suites pass: the main suite 17/17 withphoton fees enforced suite-wide, the liveness suite 5/5 including its safe-mode
scenarios under the BeginBlock-armed clock.