Skip to content

Commit ecccfb8

Browse files
0xNilesh0xNilesh
authored
fix: accept provably unreachable PENDING ballots in RevertStuckInbound (#344)
A PENDING ballot whose every eligible voter has already voted can never receive another vote, so it is terminal in fact; the admin hatch now opens for it. REJECTED stays refused (F-2026-18801).
1 parent aa5ad0e commit ecccfb8

4 files changed

Lines changed: 409 additions & 9 deletions

File tree

‎test/integration/uexecutor/revert_stuck_inbound_test.go‎

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,252 @@ func seedBallot(t *testing.T, chainApp *app.ChainApp, ctx sdk.Context, inbound *
8989
}))
9090
}
9191

92+
// seedPendingBallotWithVotes stores a PENDING ballot carrying a real
93+
// eligible-voter list and per-voter vote slots, which seedBallot deliberately
94+
// leaves empty. The F-2026-18147 scenarios all turn on whether any eligible
95+
// voter still holds a NOT_YET_VOTED slot, so they need the populated shape.
96+
//
97+
// The voter strings are never resolved against the staking set on this path —
98+
// RevertStuckInbound only reads Status/EligibleVoters/Votes off the ballot.
99+
func seedPendingBallotWithVotes(
100+
t *testing.T,
101+
chainApp *app.ChainApp,
102+
ctx sdk.Context,
103+
inbound *uexecutortypes.Inbound,
104+
status uvalidatortypes.BallotStatus,
105+
voters []string,
106+
votes []uvalidatortypes.VoteResult,
107+
threshold int64,
108+
) {
109+
t.Helper()
110+
require.Len(t, votes, len(voters), "each eligible voter needs exactly one vote slot")
111+
ballotKey, err := uexecutortypes.GetInboundBallotKey(*inbound)
112+
require.NoError(t, err)
113+
require.NoError(t, chainApp.UvalidatorKeeper.Ballots.Set(ctx, ballotKey, uvalidatortypes.Ballot{
114+
Id: ballotKey,
115+
BallotType: uvalidatortypes.BallotObservationType_BALLOT_OBSERVATION_TYPE_INBOUND_TX,
116+
EligibleVoters: voters,
117+
Votes: votes,
118+
VotingThreshold: threshold,
119+
Status: status,
120+
BlockHeightCreated: 1,
121+
BlockHeightExpiry: 100_000_000,
122+
}))
123+
require.NoError(t, chainApp.UvalidatorKeeper.ActiveBallotIDs.Set(ctx, ballotKey))
124+
}
125+
126+
// threeVoters is the eligible-voter list shared by the F-2026-18147 scenarios.
127+
func threeVoters() []string {
128+
return []string{"cosmosvaloper1aaa", "cosmosvaloper1bbb", "cosmosvaloper1ccc"}
129+
}
130+
131+
// TestRevertStuckInbound_PendingUnreachable_ThresholdMet_CreatesRevertOutbound
132+
// is the headline F-2026-18147 case.
133+
//
134+
// RecomputeBallotQuorum preserves the votes of still-eligible voters, lowers the
135+
// threshold, and returns PENDING without ever calling CheckIfFinalizingVote. The
136+
// shape reproduced here is what that leaves behind in the worst case: every
137+
// eligible voter has voted YES and the preserved YES count already clears the
138+
// recomputed threshold, so the ballot *should* have passed — but Ballot.AddVote
139+
// rejects repeat votes, so no further vote can ever be cast and nothing will
140+
// move it off PENDING. Natural expiry is 100M blocks away.
141+
//
142+
// Before this fix the admin hatch required EXPIRED, and recompute only expires a
143+
// ballot at zero eligible voters, so the deposit was stranded permanently.
144+
func TestRevertStuckInbound_PendingUnreachable_ThresholdMet_CreatesRevertOutbound(t *testing.T) {
145+
chainApp, ctx, inbound, admin := setupRevertStuckInbound(t)
146+
seedPendingBallotWithVotes(t, chainApp, ctx, inbound,
147+
uvalidatortypes.BallotStatus_BALLOT_STATUS_PENDING,
148+
threeVoters(),
149+
[]uvalidatortypes.VoteResult{
150+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
151+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
152+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
153+
},
154+
2, // YES (3) already clears the recomputed threshold
155+
)
156+
157+
ms := uexecutorkeeper.NewMsgServerImpl(chainApp.UexecutorKeeper)
158+
resp, err := ms.RevertStuckInbound(sdk.WrapSDKContext(ctx), &uexecutortypes.MsgRevertStuckInbound{
159+
Signer: admin,
160+
Inbound: inbound,
161+
})
162+
require.NoError(t, err, "an unreachable PENDING ballot must be revertible")
163+
require.NotEmpty(t, resp.UtxId)
164+
require.NotEmpty(t, resp.OutboundId)
165+
166+
// --- UTX assertions ---
167+
utx, _, err := chainApp.UexecutorKeeper.GetUniversalTx(ctx, resp.UtxId)
168+
require.NoError(t, err)
169+
require.Equal(t, uexecutortypes.GetInboundUniversalTxKey(*inbound), utx.Id)
170+
require.NotNil(t, utx.InboundTx)
171+
require.Equal(t, inbound.TxHash, utx.InboundTx.TxHash)
172+
173+
require.Len(t, utx.PcTx, 1)
174+
require.Equal(t, "FAILED", utx.PcTx[0].Status)
175+
require.Contains(t, utx.PcTx[0].ErrorMsg, "unreachable",
176+
"the audit trail must record WHY the hatch opened, not the expired wording")
177+
178+
// --- Revert outbound assertions ---
179+
require.Len(t, utx.OutboundTx, 1)
180+
ob := utx.OutboundTx[0]
181+
require.Equal(t, resp.OutboundId, ob.Id)
182+
require.Equal(t, uexecutortypes.TxType_INBOUND_REVERT, ob.TxType)
183+
require.Equal(t, uexecutortypes.Status_PENDING, ob.OutboundStatus)
184+
require.Equal(t, inbound.SourceChain, ob.DestinationChain)
185+
require.Equal(t, inbound.RevertInstructions.FundRecipient, ob.Recipient)
186+
require.Equal(t, inbound.Amount, ob.Amount)
187+
require.Equal(t, inbound.AssetAddr, ob.ExternalAssetAddr)
188+
189+
// --- PendingOutbounds index: the refund is actually queued for TSS signing ---
190+
pending, err := chainApp.UexecutorKeeper.PendingOutbounds.Get(ctx, ob.Id)
191+
require.NoError(t, err, "revert outbound must be indexed in PendingOutbounds for UV pickup")
192+
require.Equal(t, ob.Id, pending.OutboundId)
193+
require.Equal(t, utx.Id, pending.UniversalTxId)
194+
}
195+
196+
// TestRevertStuckInbound_PendingUnreachable_BelowThreshold_Accepted covers the
197+
// second stuck shape: every eligible voter has voted, but the YES count never
198+
// reached the threshold and the NO count never reached it either, so
199+
// IsFinalizingVote fires for neither branch. Reachable without any recompute at
200+
// all — 3 voters, threshold 3, one dissenting FAILURE vote.
201+
//
202+
// Unreachability, not vote arithmetic, is the predicate; both shapes qualify.
203+
func TestRevertStuckInbound_PendingUnreachable_BelowThreshold_Accepted(t *testing.T) {
204+
chainApp, ctx, inbound, admin := setupRevertStuckInbound(t)
205+
seedPendingBallotWithVotes(t, chainApp, ctx, inbound,
206+
uvalidatortypes.BallotStatus_BALLOT_STATUS_PENDING,
207+
threeVoters(),
208+
[]uvalidatortypes.VoteResult{
209+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
210+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
211+
uvalidatortypes.VoteResult_VOTE_RESULT_FAILURE,
212+
},
213+
3, // YES (2) < 3, NO (1) < 3 → neither branch of IsFinalizingVote fires
214+
)
215+
216+
ms := uexecutorkeeper.NewMsgServerImpl(chainApp.UexecutorKeeper)
217+
resp, err := ms.RevertStuckInbound(sdk.WrapSDKContext(ctx), &uexecutortypes.MsgRevertStuckInbound{
218+
Signer: admin,
219+
Inbound: inbound,
220+
})
221+
require.NoError(t, err, "a fully-voted PENDING ballot below threshold is equally unreachable")
222+
223+
utx, _, err := chainApp.UexecutorKeeper.GetUniversalTx(ctx, resp.UtxId)
224+
require.NoError(t, err)
225+
require.Len(t, utx.OutboundTx, 1)
226+
require.Equal(t, uexecutortypes.TxType_INBOUND_REVERT, utx.OutboundTx[0].TxType)
227+
228+
_, err = chainApp.UexecutorKeeper.PendingOutbounds.Get(ctx, utx.OutboundTx[0].Id)
229+
require.NoError(t, err, "revert outbound must be queued for UV pickup")
230+
}
231+
232+
// TestRevertStuckInbound_PendingWithUnvotedVoter_Refused is the guard against
233+
// widening the hatch too far.
234+
//
235+
// This ballot is deliberately the most tempting possible refusal: the YES votes
236+
// already clear the threshold, so it *looks* exactly like the headline case. It
237+
// is not — one eligible voter still holds a NOT_YET_VOTED slot, so a single
238+
// normal VoteOnBallot finalizes it through the proper VoteInbound pipeline,
239+
// which mints and executes rather than refunding. Admin revert must not race
240+
// that. This is also the shape Hacken's no-code workaround produces: add an
241+
// eligible UV, recompute, and the new voter arrives NOT_YET_VOTED.
242+
func TestRevertStuckInbound_PendingWithUnvotedVoter_Refused(t *testing.T) {
243+
chainApp, ctx, inbound, admin := setupRevertStuckInbound(t)
244+
seedPendingBallotWithVotes(t, chainApp, ctx, inbound,
245+
uvalidatortypes.BallotStatus_BALLOT_STATUS_PENDING,
246+
threeVoters(),
247+
[]uvalidatortypes.VoteResult{
248+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
249+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
250+
uvalidatortypes.VoteResult_VOTE_RESULT_NOT_YET_VOTED,
251+
},
252+
2, // YES (2) already meets threshold — still refused, it can finalize normally
253+
)
254+
255+
ms := uexecutorkeeper.NewMsgServerImpl(chainApp.UexecutorKeeper)
256+
_, err := ms.RevertStuckInbound(sdk.WrapSDKContext(ctx), &uexecutortypes.MsgRevertStuckInbound{
257+
Signer: admin,
258+
Inbound: inbound,
259+
})
260+
require.Error(t, err, "a PENDING ballot with an unvoted eligible voter can still finalize; admin revert must refuse it")
261+
require.Contains(t, err.Error(), "admin revert requires EXPIRED")
262+
263+
// The refusal must be total: no UTX, so no revert outbound can be signed.
264+
utxKey := uexecutortypes.GetInboundUniversalTxKey(*inbound)
265+
has, hErr := chainApp.UexecutorKeeper.HasUniversalTx(ctx, utxKey)
266+
require.NoError(t, hErr)
267+
require.False(t, has, "a refused revert must not leave a UniversalTx behind")
268+
}
269+
270+
// TestRevertStuckInbound_RejectedBallot_FullyVoted_StillRefused re-pins the
271+
// F-2026-18801 refusal against the new predicate.
272+
//
273+
// A REJECTED ballot is fully voted by construction, so the "every eligible voter
274+
// has voted" test on its own would let it through. It must not: REJECTED means a
275+
// supermajority affirmatively voted the observation invalid, and refunding would
276+
// pay out of the TSS vault against a deposit the validator set concluded never
277+
// happened. PENDING-unreachable is the opposite case — nobody can act at all.
278+
// The status guard in IsUnreachablePending is what keeps them apart.
279+
func TestRevertStuckInbound_RejectedBallot_FullyVoted_StillRefused(t *testing.T) {
280+
chainApp, ctx, inbound, admin := setupRevertStuckInbound(t)
281+
seedPendingBallotWithVotes(t, chainApp, ctx, inbound,
282+
uvalidatortypes.BallotStatus_BALLOT_STATUS_REJECTED,
283+
threeVoters(),
284+
[]uvalidatortypes.VoteResult{
285+
uvalidatortypes.VoteResult_VOTE_RESULT_FAILURE,
286+
uvalidatortypes.VoteResult_VOTE_RESULT_FAILURE,
287+
uvalidatortypes.VoteResult_VOTE_RESULT_FAILURE,
288+
},
289+
2,
290+
)
291+
292+
ms := uexecutorkeeper.NewMsgServerImpl(chainApp.UexecutorKeeper)
293+
_, err := ms.RevertStuckInbound(sdk.WrapSDKContext(ctx), &uexecutortypes.MsgRevertStuckInbound{
294+
Signer: admin,
295+
Inbound: inbound,
296+
})
297+
require.Error(t, err, "REJECTED stays refused however its vote slots are filled (F-2026-18801)")
298+
require.Contains(t, err.Error(), "admin revert requires EXPIRED")
299+
300+
utxKey := uexecutortypes.GetInboundUniversalTxKey(*inbound)
301+
has, hErr := chainApp.UexecutorKeeper.HasUniversalTx(ctx, utxKey)
302+
require.NoError(t, hErr)
303+
require.False(t, has, "a refused revert must not leave a UniversalTx behind")
304+
}
305+
306+
// TestRevertStuckInbound_ExpiredBallot_FullyVoted_StillAccepted keeps the
307+
// original precondition intact under the new switch: EXPIRED is accepted on its
308+
// status alone, and still records the expired wording rather than the
309+
// unreachable-pending wording.
310+
func TestRevertStuckInbound_ExpiredBallot_FullyVoted_StillAccepted(t *testing.T) {
311+
chainApp, ctx, inbound, admin := setupRevertStuckInbound(t)
312+
seedPendingBallotWithVotes(t, chainApp, ctx, inbound,
313+
uvalidatortypes.BallotStatus_BALLOT_STATUS_EXPIRED,
314+
threeVoters(),
315+
[]uvalidatortypes.VoteResult{
316+
uvalidatortypes.VoteResult_VOTE_RESULT_SUCCESS,
317+
uvalidatortypes.VoteResult_VOTE_RESULT_NOT_YET_VOTED,
318+
uvalidatortypes.VoteResult_VOTE_RESULT_NOT_YET_VOTED,
319+
},
320+
3,
321+
)
322+
323+
ms := uexecutorkeeper.NewMsgServerImpl(chainApp.UexecutorKeeper)
324+
resp, err := ms.RevertStuckInbound(sdk.WrapSDKContext(ctx), &uexecutortypes.MsgRevertStuckInbound{
325+
Signer: admin,
326+
Inbound: inbound,
327+
})
328+
require.NoError(t, err)
329+
330+
utx, _, err := chainApp.UexecutorKeeper.GetUniversalTx(ctx, resp.UtxId)
331+
require.NoError(t, err)
332+
require.Len(t, utx.PcTx, 1)
333+
require.Contains(t, utx.PcTx[0].ErrorMsg, "expired")
334+
require.Len(t, utx.OutboundTx, 1)
335+
require.Equal(t, uexecutortypes.TxType_INBOUND_REVERT, utx.OutboundTx[0].TxType)
336+
}
337+
92338
func TestRevertStuckInbound_HappyPath_ExpiredBallot_CreatesRevertOutbound(t *testing.T) {
93339
chainApp, ctx, inbound, admin := setupRevertStuckInbound(t)
94340
seedBallot(t, chainApp, ctx, inbound, uvalidatortypes.BallotStatus_BALLOT_STATUS_EXPIRED)

‎x/uexecutor/keeper/admin_revert.go‎

Lines changed: 64 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,59 @@ import (
1313
)
1414

1515
// RevertStuckInbound creates an INBOUND_REVERT outbound for an inbound whose
16-
// ballot has expired without finalizing. The revert outbound enters the normal
16+
// ballot can no longer finalize. The revert outbound enters the normal
1717
// PendingOutbounds flow; UVs sign it via TSS and broadcast it to the source
1818
// chain, refunding the user.
1919
//
20-
// Strict precondition: the ballot for the supplied inbound must be in EXPIRED
21-
// state. Admin must run MsgRecomputeBallotQuorum first to drive a stuck ballot
22-
// to EXPIRED if it isn't already (recompute auto-expires when no eligible
23-
// voters remain).
20+
// Precondition: the ballot for the supplied inbound must be either
21+
//
22+
// - EXPIRED, or
23+
// - PENDING but provably unreachable - every eligible voter has already voted
24+
// (Ballot.IsUnreachablePending).
25+
//
26+
// The second case exists because RecomputeBallotQuorum can leave a ballot
27+
// permanently stuck (F-2026-18147). It preserves the votes of still-eligible
28+
// voters, lowers the threshold, and returns PENDING without ever calling
29+
// CheckIfFinalizingVote. If the preserved votes already fill every slot there is
30+
// no vote left to cast - Ballot.AddVote rejects repeat votes - so nothing can
31+
// move the ballot off PENDING and the deposit sits in the source gateway
32+
// forever. Such a ballot is terminal in fact whatever its stored status says, so
33+
// the hatch treats it as terminal too. This holds for both stuck shapes: YES
34+
// already at or above the recomputed threshold (should have passed, never will)
35+
// and YES below it (can never reach it).
36+
//
37+
// The deliberate limits of that widening:
38+
//
39+
// - A PENDING ballot with an unvoted eligible voter is still refused. It can
40+
// finalize normally, and reverting would race a legitimate vote.
41+
// - A PENDING ballot with no eligible voters at all is refused too. Recompute
42+
// rebuilds the voter list from the live UV set, so it either gains real
43+
// voters or auto-expires; a shipped path already resolves it.
44+
// - Fixing this inside RecomputeBallotQuorum by calling CheckIfFinalizingVote
45+
// was rejected. That marks the ballot PASSED without running VoteInbound's
46+
// post-finalization pipeline, so no UniversalTx is ever built
47+
// (msg_vote_inbound.go builds one only when that specific vote finalizes)
48+
// and BallotHooks returns early on PASSED without minting or executing. The
49+
// funds would stay stuck AND the ballot would no longer be PENDING, so
50+
// recompute could not be retried - strictly worse than leaving it alone.
51+
//
52+
// This route reverts rather than executes: the user is refunded on the source
53+
// chain instead of receiving bridged funds on Push. For a ballot whose YES votes
54+
// met the threshold that is the less generous of the two resolutions, and it is
55+
// the deliberate trade for a change that stays inside the module that owns
56+
// inbound execution.
57+
//
58+
// The ballot record itself is left untouched. The HasUniversalTx guard below is
59+
// the idempotency barrier, and mutating ballot status from x/uexecutor would
60+
// fire the uvalidator terminal hook and re-enter inbound routing for an inbound
61+
// this call is already resolving.
62+
//
63+
// REJECTED stays refused, deliberately and not by omission (F-2026-18801): a
64+
// supermajority affirmatively voted that the observation is invalid, so a revert
65+
// outbound would pay real funds out of the TSS-controlled vault against a
66+
// deposit the validator set concluded never happened. PENDING-unreachable is the
67+
// opposite situation - nobody can act at all - which is why it is accepted while
68+
// REJECTED is not.
2469
//
2570
// REJECTED is refused deliberately, not by omission (F-2026-18801). The two
2671
// terminal-failure statuses mean opposite things:
@@ -65,9 +110,17 @@ func (k Keeper) RevertStuckInbound(ctx context.Context, inbound types.Inbound) (
65110
return "", "", errors.Wrap(sdkErrors.ErrNotFound, fmt.Sprintf("ballot for inbound not found (key=%s): %s", ballotKey, err))
66111
}
67112

68-
if ballot.Status != uvalidatortypes.BallotStatus_BALLOT_STATUS_EXPIRED {
113+
var revertReason string
114+
switch {
115+
case ballot.Status == uvalidatortypes.BallotStatus_BALLOT_STATUS_EXPIRED:
116+
revertReason = "admin revert: stuck ballot expired"
117+
case ballot.IsUnreachablePending():
118+
revertReason = "admin revert: pending ballot unreachable, every eligible voter has already voted"
119+
default:
69120
return "", "", errors.Wrap(sdkErrors.ErrInvalidRequest,
70-
fmt.Sprintf("ballot %s status is %s; admin revert requires EXPIRED (use MsgRecomputeBallotQuorum to drive a stuck pending ballot to EXPIRED)",
121+
fmt.Sprintf("ballot %s status is %s; admin revert requires EXPIRED, or PENDING with every eligible voter already voted (no further vote can be cast). "+
122+
"MsgRecomputeBallotQuorum rebuilds the eligible-voter set from the live UV set and marks the ballot EXPIRED only when zero eligible voters remain, "+
123+
"so a pending ballot that still has an unvoted eligible voter has to be finalized by that voter through the normal vote flow",
71124
ballotKey, ballot.Status.String()))
72125
}
73126

@@ -84,7 +137,7 @@ func (k Keeper) RevertStuckInbound(ctx context.Context, inbound types.Inbound) (
84137
InboundTx: &inbound,
85138
PcTx: []*types.PCTx{{
86139
Status: "FAILED",
87-
ErrorMsg: "admin revert: stuck ballot expired",
140+
ErrorMsg: revertReason,
88141
}},
89142
}
90143
if cErr := k.CreateUniversalTx(ctx, universalTxKey, utx); cErr != nil {
@@ -107,7 +160,7 @@ func (k Keeper) RevertStuckInbound(ctx context.Context, inbound types.Inbound) (
107160
)
108161
}
109162

110-
if attachErr := k.attachOutboundsToUtx(sdkCtx, universalTxKey, []*types.OutboundTx{revertOutbound}, "admin revert: stuck ballot expired"); attachErr != nil {
163+
if attachErr := k.attachOutboundsToUtx(sdkCtx, universalTxKey, []*types.OutboundTx{revertOutbound}, revertReason); attachErr != nil {
111164
return "", "", fmt.Errorf("failed to attach revert outbound: %w", attachErr)
112165
}
113166

@@ -118,6 +171,8 @@ func (k Keeper) RevertStuckInbound(ctx context.Context, inbound types.Inbound) (
118171
"source_chain", inbound.SourceChain,
119172
"recipient", revertOutbound.Recipient,
120173
"amount", revertOutbound.Amount,
174+
"ballot_status", ballot.Status.String(),
175+
"reason", revertReason,
121176
)
122177

123178
return universalTxKey, revertOutbound.Id, nil

0 commit comments

Comments
 (0)