Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: update pending nonces when aborting a cctx through MsgAbortStuckCCTX #3230

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

fix: update pending nonces when aborting a cctx through MsgAbortStuckCCTX #3230

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
601488d
call save outbound from Abort stuck cctx
kingpinXD Nov 27, 2024
9a7cd75
add check for pending status
kingpinXD Nov 27, 2024
994e66a
add changelog
kingpinXD Nov 28, 2024
5eb1127
update usage for IsTerminalStatus function
kingpinXD Nov 28, 2024
091ec82
add test for removing non-existent tracker
kingpinXD Nov 28, 2024
7265af5
break SetCctxAndNonceToCctxAndInboundHashToCctx into separate functio…
kingpinXD Nov 28, 2024
6d4a4cf
update comments for SetCctxAndNonceToCctxAndInboundHashToCctx
kingpinXD Nov 28, 2024
c50a6ea
update comments for SetCctxAndNonceToCctxAndInboundHashToCctx
kingpinXD Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

* [3206](https://github.com/zeta-chain/node/pull/3206) - skip Solana unsupported transaction version to not block inbound observation
* [3184](https://github.com/zeta-chain/node/pull/3184) - zetaclient should not retry if inbound vote message validation fails
* [3230](https://github.com/zeta-chain/node/pull/3230) - update pending nonces when aborting a cctx through MsgAbortStuckCCTX

## v23.0.0

Expand Down
10 changes: 2 additions & 8 deletions e2e/utils/zetacore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func WaitCctxsMinedByInboundHash(
allFound := true
for j, cctx := range res.CrossChainTxs {
cctx := cctx
if !IsTerminalStatus(cctx.CctxStatus.Status) {
if !cctx.CctxStatus.Status.IsTerminalStatus() {
// prevent spamming logs
if i%20 == 0 {
logger.Info(
Expand Down Expand Up @@ -170,7 +170,7 @@ func WaitCCTXMinedByIndex(
}

cctx := res.CrossChainTx
if !IsTerminalStatus(cctx.CctxStatus.Status) {
if !cctx.CctxStatus.Status.IsTerminalStatus() {
// prevent spamming logs
if i%20 == 0 {
logger.Info(
Expand Down Expand Up @@ -299,12 +299,6 @@ func WaitCctxByInboundHash(
}
}

func IsTerminalStatus(status crosschaintypes.CctxStatus) bool {
return status == crosschaintypes.CctxStatus_OutboundMined ||
status == crosschaintypes.CctxStatus_Aborted ||
status == crosschaintypes.CctxStatus_Reverted
}

// WaitForBlockHeight waits until the block height reaches the given height
func WaitForBlockHeight(
ctx context.Context,
Expand Down
20 changes: 17 additions & 3 deletions x/crosschain/keeper/cctx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,29 @@ import (
)

// SetCctxAndNonceToCctxAndInboundHashToCctx does the following things in one function:
// 1. set the cctx in the store
// 2. set the mapping inboundHash -> cctxIndex , one inboundHash can be connected to multiple cctxindex
// 3. set the mapping nonce => cctx

// 1. Set the Nonce to Cctx mapping
// A new mapping between a nonce and a cctx index should be created only when we add a new outbound to an existing cctx.
// When adding a new outbound , the only two conditions are
// - The cctx is in CctxStatus_PendingOutbound , which means the first outbound has been added, and we need to set the nonce for that
// - The cctx is in CctxStatus_PendingRevert , which means the second outbound has been added, and we need to set the nonce for that

// 2. Set the cctx in the store

// 3. set the mapping inboundHash -> cctxIndex
// A new value is added to the mapping when a single inbound hash is connected to multiple cctx indexes

// 4. update the zeta accounting
// Zeta-accounting is updated aborted cctxs of cointtype zeta.When a cctx is aborted it means that `GetAbortedAmount`
//of zeta is locked and cannot be used.

kingpinXD marked this conversation as resolved.
Show resolved Hide resolved
func (k Keeper) SetCctxAndNonceToCctxAndInboundHashToCctx(
ctx sdk.Context,
cctx types.CrossChainTx,
tssPubkey string,
) {
// set mapping nonce => cctxIndex

if cctx.CctxStatus.Status == types.CctxStatus_PendingOutbound ||
cctx.CctxStatus.Status == types.CctxStatus_PendingRevert {
k.GetObserverKeeper().SetNonceToCctx(ctx, observerTypes.NonceToCctx{
Expand All @@ -34,6 +47,7 @@ func (k Keeper) SetCctxAndNonceToCctxAndInboundHashToCctx(
}

k.SetCrossChainTx(ctx, cctx)

// set mapping inboundHash -> cctxIndex
in, _ := k.GetInboundHashToCctx(ctx, cctx.InboundParams.ObservedHash)
in.InboundHash = cctx.InboundParams.ObservedHash
Expand Down
14 changes: 5 additions & 9 deletions x/crosschain/keeper/msg_server_abort_stuck_cctx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,15 @@ func (k msgServer) AbortStuckCCTX(
}

// check if the cctx is pending
isPending := cctx.CctxStatus.Status == types.CctxStatus_PendingOutbound ||
cctx.CctxStatus.Status == types.CctxStatus_PendingInbound ||
cctx.CctxStatus.Status == types.CctxStatus_PendingRevert
if !isPending {
if !cctx.CctxStatus.Status.IsPendingStatus() {
return nil, types.ErrStatusNotPending
}

cctx.CctxStatus = &types.Status{
Status: types.CctxStatus_Aborted,
StatusMessage: AbortMessage,
}
// update the status
cctx.CctxStatus.UpdateStatusAndErrorMessages(types.CctxStatus_Aborted, AbortMessage, "")

k.SetCrossChainTx(ctx, cctx)
// Save out outbound, we do not need to provide the tss-pubkey as NonceToCctx is not updated
k.SaveOutbound(ctx, &cctx, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Nonce cleanup needs to be implemented in abort stuck CCTX handler

The SaveOutbound implementation in msg_server_vote_outbound_tx.go shows that it handles nonce cleanup by calling RemoveFromPendingNonces, but this is specifically for the outbound voting flow. When aborting a stuck CCTX, we need explicit nonce cleanup to prevent nonce gaps.

  • Add RemoveFromPendingNonces call before SaveOutbound in msg_server_abort_stuck_cctx.go to properly clean up the pending nonce:
k.GetObserverKeeper().RemoveFromPendingNonces(ctx, cctx.GetCurrentOutboundParam().TssPubkey, cctx.GetCurrentOutboundParam().ReceiverChainId, int64(cctx.GetCurrentOutboundParam().TssNonce))
🔗 Analysis chain

Verify nonce removal from pending list.

The PR objectives mention using RemoveFromPendingNonces, but it's not explicitly called here. Let's verify if SaveOutbound handles this or if we need additional nonce cleanup.

Let me gather more information about the SaveOutbound implementation to understand how it handles nonces.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for nonce handling in SaveOutbound implementation
ast-grep --pattern 'func (k Keeper) SaveOutbound($_, $_, $_) {
  $$$
  RemoveFromPendingNonces($$$)
  $$$
}'

# Also check for any other nonce handling in the keeper
rg -A 5 "RemoveFromPendingNonces"

Length of output: 6219

Script:

#!/bin/bash
# Check SaveOutbound implementation
ast-grep --pattern 'func (k Keeper) SaveOutbound($_, $_, $_) {
  $$$
}'

# Check if there are any other methods in the abort stuck cctx file that handle nonces
rg -A 5 "nonce" x/crosschain/keeper/msg_server_abort_stuck_cctx.go

Length of output: 1252


return &types.MsgAbortStuckCCTXResponse{}, nil
}
55 changes: 51 additions & 4 deletions x/crosschain/keeper/msg_server_abort_stuck_cctx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/stretchr/testify/require"
observertypes "github.com/zeta-chain/node/x/observer/types"

keepertest "github.com/zeta-chain/node/testutil/keeper"
"github.com/zeta-chain/node/testutil/sample"
Expand All @@ -14,6 +15,7 @@ import (

func TestMsgServer_AbortStuckCCTX(t *testing.T) {
t.Run("can abort a cctx in pending inbound", func(t *testing.T) {
// Arrange
k, ctx, _, _ := keepertest.CrosschainKeeperWithMocks(t, keepertest.CrosschainMockOptions{
UseAuthorityMock: true,
})
Expand All @@ -28,24 +30,39 @@ func TestMsgServer_AbortStuckCCTX(t *testing.T) {
Status: crosschaintypes.CctxStatus_PendingInbound,
StatusMessage: "pending inbound",
}
cctx.GetCurrentOutboundParam().TssNonce = 1

k.SetCrossChainTx(ctx, *cctx)
k.GetObserverKeeper().SetPendingNonces(ctx, observertypes.PendingNonces{
NonceLow: 0,
NonceHigh: 10,
ChainId: cctx.GetCurrentOutboundParam().ReceiverChainId,
Tss: cctx.GetCurrentOutboundParam().TssPubkey,
})

// abort the cctx
msg := crosschaintypes.MsgAbortStuckCCTX{
Creator: admin,
CctxIndex: sample.GetCctxIndexFromString("cctx_index"),
}
keepertest.MockCheckAuthorization(&authorityMock.Mock, &msg, nil)
// Act
_, err := msgServer.AbortStuckCCTX(ctx, &msg)

// Assert
require.NoError(t, err)
cctxFound, found := k.GetCrossChainTx(ctx, sample.GetCctxIndexFromString("cctx_index"))
require.True(t, found)
require.Equal(t, crosschaintypes.CctxStatus_Aborted, cctxFound.CctxStatus.Status)
require.Equal(t, crosschainkeeper.AbortMessage, cctxFound.CctxStatus.StatusMessage)
require.Contains(t, cctxFound.CctxStatus.StatusMessage, crosschainkeeper.AbortMessage)
pendingNonces, found := k.GetObserverKeeper().
GetPendingNonces(ctx, cctx.GetCurrentOutboundParam().TssPubkey, cctx.GetCurrentOutboundParam().ReceiverChainId)
require.True(t, found)
require.Equal(t, pendingNonces.NonceLow, int64(cctx.GetCurrentOutboundParam().TssNonce+1))
})

t.Run("can abort a cctx in pending outbound", func(t *testing.T) {
// Arrange
k, ctx, _, _ := keepertest.CrosschainKeeperWithMocks(t, keepertest.CrosschainMockOptions{
UseAuthorityMock: true,
})
Expand All @@ -59,26 +76,42 @@ func TestMsgServer_AbortStuckCCTX(t *testing.T) {
Status: crosschaintypes.CctxStatus_PendingOutbound,
StatusMessage: "pending outbound",
}
cctx.GetCurrentOutboundParam().TssNonce = 1

k.SetCrossChainTx(ctx, *cctx)
k.GetObserverKeeper().SetPendingNonces(ctx, observertypes.PendingNonces{
NonceLow: 0,
NonceHigh: 10,
ChainId: cctx.GetCurrentOutboundParam().ReceiverChainId,
Tss: cctx.GetCurrentOutboundParam().TssPubkey,
})

// abort the cctx
msg := crosschaintypes.MsgAbortStuckCCTX{
Creator: admin,
CctxIndex: sample.GetCctxIndexFromString("cctx_index"),
}
keepertest.MockCheckAuthorization(&authorityMock.Mock, &msg, nil)

// Act
_, err := msgServer.AbortStuckCCTX(ctx, &msg)

// Assert
require.NoError(t, err)
cctxFound, found := k.GetCrossChainTx(ctx, sample.GetCctxIndexFromString("cctx_index"))
require.True(t, found)
require.Equal(t, crosschaintypes.CctxStatus_Aborted, cctxFound.CctxStatus.Status)
require.Equal(t, crosschainkeeper.AbortMessage, cctxFound.CctxStatus.StatusMessage)
require.Contains(t, cctxFound.CctxStatus.StatusMessage, crosschainkeeper.AbortMessage)
// ensure the last update timestamp is updated
require.Equal(t, cctxFound.CctxStatus.LastUpdateTimestamp, ctx.BlockTime().Unix())
pendingNonces, found := k.GetObserverKeeper().
GetPendingNonces(ctx, cctx.GetCurrentOutboundParam().TssPubkey, cctx.GetCurrentOutboundParam().ReceiverChainId)
require.True(t, found)
require.Equal(t, pendingNonces.NonceLow, int64(cctx.GetCurrentOutboundParam().TssNonce+1))

})

t.Run("can abort a cctx in pending revert", func(t *testing.T) {
// Arrange
k, ctx, _, _ := keepertest.CrosschainKeeperWithMocks(t, keepertest.CrosschainMockOptions{
UseAuthorityMock: true,
})
Expand All @@ -93,21 +126,35 @@ func TestMsgServer_AbortStuckCCTX(t *testing.T) {
Status: crosschaintypes.CctxStatus_PendingRevert,
StatusMessage: "pending revert",
}
cctx.GetCurrentOutboundParam().TssNonce = 1

k.SetCrossChainTx(ctx, *cctx)
k.GetObserverKeeper().SetPendingNonces(ctx, observertypes.PendingNonces{
NonceLow: 0,
NonceHigh: 10,
ChainId: cctx.GetCurrentOutboundParam().ReceiverChainId,
Tss: cctx.GetCurrentOutboundParam().TssPubkey,
})

// abort the cctx
msg := crosschaintypes.MsgAbortStuckCCTX{
Creator: admin,
CctxIndex: sample.GetCctxIndexFromString("cctx_index"),
}
keepertest.MockCheckAuthorization(&authorityMock.Mock, &msg, nil)
// Act
_, err := msgServer.AbortStuckCCTX(ctx, &msg)

// Assert
require.NoError(t, err)
cctxFound, found := k.GetCrossChainTx(ctx, sample.GetCctxIndexFromString("cctx_index"))
require.True(t, found)
require.Equal(t, crosschaintypes.CctxStatus_Aborted, cctxFound.CctxStatus.Status)
require.Equal(t, crosschainkeeper.AbortMessage, cctxFound.CctxStatus.StatusMessage)
require.Contains(t, cctxFound.CctxStatus.StatusMessage, crosschainkeeper.AbortMessage)
pendingNonces, found := k.GetObserverKeeper().
GetPendingNonces(ctx, cctx.GetCurrentOutboundParam().TssPubkey, cctx.GetCurrentOutboundParam().ReceiverChainId)
require.True(t, found)
require.Equal(t, pendingNonces.NonceLow, int64(cctx.GetCurrentOutboundParam().TssNonce+1))
})

t.Run("cannot abort a cctx in pending outbound if not admin", func(t *testing.T) {
Expand Down
36 changes: 23 additions & 13 deletions x/crosschain/keeper/outbound_tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,29 @@ func TestOutboundTrackerGet(t *testing.T) {
}
}
func TestOutboundTrackerRemove(t *testing.T) {
k, ctx, _, _ := keepertest.CrosschainKeeper(t)
items := createNOutboundTracker(k, ctx, 10)
for _, item := range items {
k.RemoveOutboundTrackerFromStore(ctx,
item.ChainId,
item.Nonce,
)
_, found := k.GetOutboundTracker(ctx,
item.ChainId,
item.Nonce,
)
require.False(t, found)
}
t.Run("Remove tracker if it exists", func(t *testing.T) {
keeper, ctx, _, _ := keepertest.CrosschainKeeper(t)
items := createNOutboundTracker(keeper, ctx, 10)
for _, item := range items {
keeper.RemoveOutboundTrackerFromStore(ctx,
item.ChainId,
item.Nonce,
)
_, found := keeper.GetOutboundTracker(ctx,
item.ChainId,
item.Nonce,
)
require.False(t, found)
}
})

t.Run("Do nothing if tracker doesn't exist", func(t *testing.T) {
keeper, ctx, _, _ := keepertest.CrosschainKeeper(t)
require.NotPanics(t, func() {
keeper.RemoveOutboundTrackerFromStore(ctx, 1, 1)
})
})

}

func TestOutboundTrackerGetAll(t *testing.T) {
Expand Down
8 changes: 8 additions & 0 deletions x/crosschain/types/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,11 @@ func stateTransitionMap() map[CctxStatus][]CctxStatus {
}
return stateTransitionMap
}

func (c CctxStatus) IsTerminalStatus() bool {
return c == CctxStatus_Aborted || c == CctxStatus_Reverted || c == CctxStatus_OutboundMined
}

func (c CctxStatus) IsPendingStatus() bool {
return c == CctxStatus_PendingInbound || c == CctxStatus_PendingOutbound || c == CctxStatus_PendingRevert
}
42 changes: 42 additions & 0 deletions x/crosschain/types/status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,45 @@ func TestStatus_ChangeStatus(t *testing.T) {
)
})
}

func TestCctxStatus_IsTerminalStatus(t *testing.T) {
tests := []struct {
name string
status types.CctxStatus
expected bool
}{
{"PendingInbound", types.CctxStatus_PendingInbound, false},
{"PendingOutbound", types.CctxStatus_PendingOutbound, false},
{"OutboundMined", types.CctxStatus_OutboundMined, true},
{"Reverted", types.CctxStatus_Reverted, true},
{"Aborted", types.CctxStatus_Aborted, true},
{"PendingRevert", types.CctxStatus_PendingRevert, false},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, tc.expected, tc.status.IsTerminalStatus())
})
}
}

func TestCctxStatus_IsPendingStatus(t *testing.T) {
tests := []struct {
name string
status types.CctxStatus
expected bool
}{
{"PendingInbound", types.CctxStatus_PendingInbound, true},
{"PendingOutbound", types.CctxStatus_PendingOutbound, true},
{"OutboundMined", types.CctxStatus_OutboundMined, false},
{"Reverted", types.CctxStatus_Reverted, false},
{"Aborted", types.CctxStatus_Aborted, false},
{"PendingRevert", types.CctxStatus_PendingRevert, true},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, tc.expected, tc.status.IsPendingStatus())
})
}
}
Loading