From 8ffc978743920621bb1539a5d5cf05d112f8d33a Mon Sep 17 00:00:00 2001 From: Tanmay Date: Wed, 31 Jul 2024 17:03:33 -0400 Subject: [PATCH 1/6] refactor tracker cleanup --- cmd/zetae2e/local/local.go | 8 ++ .../keeper/msg_server_vote_outbound_tx.go | 38 +++--- .../msg_server_vote_outbound_tx_test.go | 126 ++++++++++++++++-- x/crosschain/types/cctx.go | 4 + x/crosschain/types/cctx_test.go | 6 + 5 files changed, 156 insertions(+), 26 deletions(-) diff --git a/cmd/zetae2e/local/local.go b/cmd/zetae2e/local/local.go index 72d01e4584..337b529bd8 100644 --- a/cmd/zetae2e/local/local.go +++ b/cmd/zetae2e/local/local.go @@ -362,10 +362,18 @@ func localE2ETest(cmd *cobra.Command, _ []string) { logger.Print("❌ network report validation failed %v", err) os.Exit(1) } + checkTrackers(ctx, deployerRunner) os.Exit(0) } +func checkTrackers(ctx context.Context, deployRunner *runner.E2ERunner) { + // get all trackers + res, err := deployRunner.CctxClient.OutTxTrackerAll(ctx, &crosschaintypes.QueryAllOutboundTrackerRequest{}) + require.NoError(deployRunner, err) + require.Empty(deployRunner, res.OutboundTracker, "there should be no trackers") +} + // waitKeygenHeight waits for keygen height func waitKeygenHeight( ctx context.Context, diff --git a/x/crosschain/keeper/msg_server_vote_outbound_tx.go b/x/crosschain/keeper/msg_server_vote_outbound_tx.go index 61e1d9389e..000c148414 100644 --- a/x/crosschain/keeper/msg_server_vote_outbound_tx.go +++ b/x/crosschain/keeper/msg_server_vote_outbound_tx.go @@ -90,6 +90,11 @@ func (k msgServer) VoteOutbound( return &types.MsgVoteOutboundResponse{}, nil } + // Set the finalized ballot to the current outbound params. + // The two cases are possible + // 1. The outbound tx is successful, and this is the only ballot that is set + // 2. Revert TX is created, in which case the ballot for the revert TX would be set only when that ballot is finalized. + cctx.SetOutboundBallotIndex(ballotIndex) // If ballot is successful, the value received should be the out tx amount. err = cctx.AddOutbound(ctx, *msg, ballot.BallotStatus) if err != nil { @@ -101,11 +106,11 @@ func (k msgServer) VoteOutbound( err = k.ValidateOutboundObservers(ctx, &cctx, ballot.BallotStatus, msg.ValueReceived.String()) if err != nil { - k.SaveFailedOutbound(ctx, &cctx, err.Error(), ballotIndex) + k.SaveFailedOutbound(ctx, &cctx, err.Error()) return &types.MsgVoteOutboundResponse{}, nil } - k.SaveSuccessfulOutbound(ctx, &cctx, ballotIndex) + k.SaveSuccessfulOutbound(ctx, &cctx) return &types.MsgVoteOutboundResponse{}, nil } @@ -171,17 +176,17 @@ SaveFailedOutbound saves a failed outbound transaction.It does the following thi 2. Save the outbound */ -func (k Keeper) SaveFailedOutbound(ctx sdk.Context, cctx *types.CrossChainTx, errMessage string, ballotIndex string) { +func (k Keeper) SaveFailedOutbound(ctx sdk.Context, cctx *types.CrossChainTx, errMessage string) { cctx.SetAbort(errMessage) ctx.Logger().Error(errMessage) - k.SaveOutbound(ctx, cctx, ballotIndex) + k.SaveOutbound(ctx, cctx) } // SaveSuccessfulOutbound saves a successful outbound transaction. // This function does not set the CCTX status, therefore all successful outbound transactions need // to have their status set during processing -func (k Keeper) SaveSuccessfulOutbound(ctx sdk.Context, cctx *types.CrossChainTx, ballotIndex string) { - k.SaveOutbound(ctx, cctx, ballotIndex) +func (k Keeper) SaveSuccessfulOutbound(ctx sdk.Context, cctx *types.CrossChainTx) { + k.SaveOutbound(ctx, cctx) } /* @@ -195,18 +200,17 @@ SaveOutbound saves the outbound transaction.It does the following things in one 4. Set the cctx and nonce to cctx and inboundHash to cctx */ -func (k Keeper) SaveOutbound(ctx sdk.Context, cctx *types.CrossChainTx, ballotIndex string) { - receiverChain := cctx.GetCurrentOutboundParam().ReceiverChainId - tssPubkey := cctx.GetCurrentOutboundParam().TssPubkey - outTxTssNonce := cctx.GetCurrentOutboundParam().TssNonce - - cctx.GetCurrentOutboundParam().BallotIndex = ballotIndex - +func (k Keeper) SaveOutbound(ctx sdk.Context, cctx *types.CrossChainTx) { // #nosec G115 always in range - k.GetObserverKeeper().RemoveFromPendingNonces(ctx, tssPubkey, receiverChain, int64(outTxTssNonce)) - k.RemoveOutboundTrackerFromStore(ctx, receiverChain, outTxTssNonce) - ctx.Logger(). - Info("%s: Remove tracker %s: , Block Height : %d ", voteOutboundID, getOutboundTrackerIndex(receiverChain, outTxTssNonce), ctx.BlockHeight()) + for _, outboundParams := range cctx.OutboundParams { + k.GetObserverKeeper().RemoveFromPendingNonces(ctx, outboundParams.TssPubkey, outboundParams.ReceiverChainId, int64(outboundParams.TssNonce)) + k.RemoveOutboundTrackerFromStore(ctx, outboundParams.ReceiverChainId, outboundParams.TssNonce) + ctx.Logger().With( + "voteOutboundID", voteOutboundID, + "trackerIndex", getOutboundTrackerIndex(outboundParams.ReceiverChainId, outboundParams.TssNonce), + "blockHeight", ctx.BlockHeight(), + ).Info("Remove tracker") + } // This should set nonce to cctx only if a new revert is created. k.SetCctxAndNonceToCctxAndInboundHashToCctx(ctx, *cctx) diff --git a/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go b/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go index 92d39b2ab4..4c19461d44 100644 --- a/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go +++ b/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go @@ -153,7 +153,7 @@ func TestKeeper_VoteOutbound(t *testing.T) { keepertest.MockSaveOutbound(observerMock, ctx, cctx, tss) msgServer := keeper.NewMsgServerImpl(*k) - _, err := msgServer.VoteOutbound(ctx, &types.MsgVoteOutbound{ + msg := types.MsgVoteOutbound{ CctxHash: cctx.Index, OutboundTssNonce: cctx.GetCurrentOutboundParam().TssNonce, OutboundChain: cctx.GetCurrentOutboundParam().ReceiverChainId, @@ -165,11 +165,13 @@ func TestKeeper_VoteOutbound(t *testing.T) { ObservedOutboundEffectiveGasPrice: math.NewInt(21), ObservedOutboundGasUsed: 21, CoinType: cctx.InboundParams.CoinType, - }) + } + _, err := msgServer.VoteOutbound(ctx, &msg) require.NoError(t, err) c, found := k.GetCrossChainTx(ctx, cctx.Index) require.True(t, found) require.Equal(t, types.CctxStatus_OutboundMined, c.CctxStatus.Status) + require.Equal(t, msg.Digest(), c.GetCurrentOutboundParam().BallotIndex) }) t.Run("successfully vote on outbound tx, vote-type failed", func(t *testing.T) { @@ -209,7 +211,7 @@ func TestKeeper_VoteOutbound(t *testing.T) { keepertest.MockSaveOutboundNewRevertCreated(observerMock, ctx, cctx, tss) oldParamsLen := len(cctx.OutboundParams) msgServer := keeper.NewMsgServerImpl(*k) - _, err := msgServer.VoteOutbound(ctx, &types.MsgVoteOutbound{ + msg := types.MsgVoteOutbound{ CctxHash: cctx.Index, OutboundTssNonce: cctx.GetCurrentOutboundParam().TssNonce, OutboundChain: cctx.GetCurrentOutboundParam().ReceiverChainId, @@ -221,7 +223,8 @@ func TestKeeper_VoteOutbound(t *testing.T) { ObservedOutboundEffectiveGasPrice: math.NewInt(21), ObservedOutboundGasUsed: 21, CoinType: cctx.InboundParams.CoinType, - }) + } + _, err := msgServer.VoteOutbound(ctx, &msg) require.NoError(t, err) c, found := k.GetCrossChainTx(ctx, cctx.Index) require.True(t, found) @@ -229,6 +232,7 @@ func TestKeeper_VoteOutbound(t *testing.T) { require.Equal(t, oldParamsLen+1, len(c.OutboundParams)) require.Equal(t, types.TxFinalizationStatus_Executed, c.OutboundParams[oldParamsLen-1].TxFinalizationStatus) require.Equal(t, types.TxFinalizationStatus_NotFinalized, cctx.GetCurrentOutboundParam().TxFinalizationStatus) + require.Equal(t, msg.Digest(), c.GetCurrentOutboundParam().BallotIndex) }) t.Run("unsuccessfully vote on outbound tx, vote-type failed", func(t *testing.T) { @@ -465,7 +469,7 @@ func TestKeeper_SaveFailedOutbound(t *testing.T) { HashList: nil, }) cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound - k.SaveFailedOutbound(ctx, cctx, sample.String(), sample.ZetaIndex(t)) + k.SaveFailedOutbound(ctx, cctx, sample.String()) require.Equal(t, cctx.CctxStatus.Status, types.CctxStatus_Aborted) _, found := k.GetOutboundTracker( ctx, @@ -474,6 +478,32 @@ func TestKeeper_SaveFailedOutbound(t *testing.T) { ) require.False(t, found) }) + + t.Run("successfully save failed outbound with multiple trackers", func(t *testing.T) { + k, ctx, _, _ := keepertest.CrosschainKeeper(t) + cctx := sample.CrossChainTx(t, "test") + for _, outboundParams := range cctx.OutboundParams { + k.SetOutboundTracker(ctx, types.OutboundTracker{ + Index: "", + ChainId: outboundParams.ReceiverChainId, + Nonce: outboundParams.TssNonce, + HashList: nil, + }) + } + cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound + k.SaveFailedOutbound(ctx, cctx, sample.String()) + require.Equal(t, cctx.CctxStatus.Status, types.CctxStatus_Aborted) + + for _, outboundParams := range cctx.OutboundParams { + _, found := k.GetOutboundTracker( + ctx, + outboundParams.ReceiverChainId, + outboundParams.TssNonce, + ) + require.False(t, found) + } + + }) } func TestKeeper_SaveSuccessfulOutbound(t *testing.T) { @@ -487,7 +517,7 @@ func TestKeeper_SaveSuccessfulOutbound(t *testing.T) { HashList: nil, }) cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound - k.SaveSuccessfulOutbound(ctx, cctx, sample.String()) + k.SaveSuccessfulOutbound(ctx, cctx) require.Equal(t, cctx.GetCurrentOutboundParam().BallotIndex, sample.String()) _, found := k.GetOutboundTracker( ctx, @@ -496,6 +526,31 @@ func TestKeeper_SaveSuccessfulOutbound(t *testing.T) { ) require.False(t, found) }) + + t.Run("successfully save successful outbound with multiple trackers", func(t *testing.T) { + k, ctx, _, _ := keepertest.CrosschainKeeper(t) + cctx := sample.CrossChainTx(t, "test") + for _, outboundParams := range cctx.OutboundParams { + k.SetOutboundTracker(ctx, types.OutboundTracker{ + Index: "", + ChainId: outboundParams.ReceiverChainId, + Nonce: outboundParams.TssNonce, + HashList: nil, + }) + } + cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound + k.SaveSuccessfulOutbound(ctx, cctx) + require.Equal(t, cctx.GetCurrentOutboundParam().BallotIndex, sample.String()) + + for _, outboundParams := range cctx.OutboundParams { + _, found := k.GetOutboundTracker( + ctx, + outboundParams.ReceiverChainId, + outboundParams.TssNonce, + ) + require.False(t, found) + } + }) } func TestKeeper_SaveOutbound(t *testing.T) { @@ -505,7 +560,6 @@ func TestKeeper_SaveOutbound(t *testing.T) { // setup state for crosschain and observer modules cctx := sample.CrossChainTx(t, "test") cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound - ballotIndex := sample.String() k.SetOutboundTracker(ctx, types.OutboundTracker{ Index: "", ChainId: cctx.GetCurrentOutboundParam().ReceiverChainId, @@ -524,8 +578,7 @@ func TestKeeper_SaveOutbound(t *testing.T) { }) // Save outbound and assert all values are successfully saved - k.SaveOutbound(ctx, cctx, ballotIndex) - require.Equal(t, cctx.GetCurrentOutboundParam().BallotIndex, ballotIndex) + k.SaveOutbound(ctx, cctx) _, found := k.GetOutboundTracker( ctx, cctx.GetCurrentOutboundParam().ReceiverChainId, @@ -550,6 +603,61 @@ func TestKeeper_SaveOutbound(t *testing.T) { ) require.True(t, found) }) + + t.Run("successfully save outbound with multiple trackers", func(t *testing.T) { + k, ctx, _, zk := keepertest.CrosschainKeeper(t) + + // setup state for crosschain and observer modules + cctx := sample.CrossChainTx(t, "test") + for _, outboundParams := range cctx.OutboundParams { + k.SetOutboundTracker(ctx, types.OutboundTracker{ + Index: "", + ChainId: outboundParams.ReceiverChainId, + Nonce: outboundParams.TssNonce, + HashList: nil, + }) + zk.ObserverKeeper.SetPendingNonces(ctx, observertypes.PendingNonces{ + NonceLow: int64(cctx.GetCurrentOutboundParam().TssNonce) - 1, + NonceHigh: int64(cctx.GetCurrentOutboundParam().TssNonce) + 1, + ChainId: outboundParams.ReceiverChainId, + Tss: outboundParams.TssPubkey, + }) + } + cctx.CctxStatus.Status = types.CctxStatus_PendingRevert + + tssPubkey := cctx.GetCurrentOutboundParam().TssPubkey + zk.ObserverKeeper.SetTSS(ctx, observertypes.TSS{ + TssPubkey: tssPubkey, + }) + + // Save outbound and assert all values are successfully saved + k.SaveOutbound(ctx, cctx) + + for _, outboundParams := range cctx.OutboundParams { + _, found := k.GetOutboundTracker( + ctx, + outboundParams.ReceiverChainId, + outboundParams.TssNonce, + ) + require.False(t, found) + pn, found := zk.ObserverKeeper.GetPendingNonces( + ctx, + outboundParams.TssPubkey, + outboundParams.ReceiverChainId, + ) + require.True(t, found) + require.GreaterOrEqual(t, pn.NonceLow, int64(outboundParams.TssNonce)+1) + require.GreaterOrEqual(t, pn.NonceHigh, int64(outboundParams.TssNonce)+1) + _, found = k.GetInboundHashToCctx(ctx, cctx.InboundParams.ObservedHash) + require.True(t, found) + } + ncctx, found := zk.ObserverKeeper.GetNonceToCctx(ctx, + tssPubkey, + cctx.GetCurrentOutboundParam().ReceiverChainId, + int64(cctx.GetCurrentOutboundParam().TssNonce)) + require.True(t, found) + require.Equal(t, cctx.Index, ncctx.CctxIndex) + }) } func TestKeeper_ValidateOutboundMessage(t *testing.T) { diff --git a/x/crosschain/types/cctx.go b/x/crosschain/types/cctx.go index 27b9b30968..88ab5c1b32 100644 --- a/x/crosschain/types/cctx.go +++ b/x/crosschain/types/cctx.go @@ -164,6 +164,10 @@ func (m CrossChainTx) GetCCTXIndexBytes() ([32]byte, error) { return sendHash, nil } +func (m CrossChainTx) SetOutboundBallotIndex(index string) { + m.GetCurrentOutboundParam().BallotIndex = index +} + func GetCctxIndexFromBytes(sendHash [32]byte) string { return fmt.Sprintf("0x%s", hex.EncodeToString(sendHash[:])) } diff --git a/x/crosschain/types/cctx_test.go b/x/crosschain/types/cctx_test.go index eecdf0cdf0..57d09ed188 100644 --- a/x/crosschain/types/cctx_test.go +++ b/x/crosschain/types/cctx_test.go @@ -16,6 +16,12 @@ import ( observertypes "github.com/zeta-chain/zetacore/x/observer/types" ) +func TestCrossChainTx_SetOutboundBallot(t *testing.T) { + cctx := sample.CrossChainTx(t, "test") + ballotIndex := sample.ZetaIndex(t) + cctx.SetOutboundBallotIndex(ballotIndex) + require.Equal(t, ballotIndex, cctx.GetCurrentOutboundParam().BallotIndex) +} func TestCrossChainTx_GetCCTXIndexBytes(t *testing.T) { cctx := sample.CrossChainTx(t, "sample") indexBytes, err := cctx.GetCCTXIndexBytes() From 309358e1bb9b5091793055b4679c6b1802f136bb Mon Sep 17 00:00:00 2001 From: Tanmay Date: Wed, 31 Jul 2024 23:32:38 -0400 Subject: [PATCH 2/6] add changelog --- changelog.md | 4 ++++ .../zetacored_tx_fungible_update-gateway-contract.md | 2 +- x/crosschain/keeper/msg_server_vote_outbound_tx.go | 11 ++--------- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/changelog.md b/changelog.md index ae097c3163..1b149729c0 100644 --- a/changelog.md +++ b/changelog.md @@ -6,6 +6,10 @@ * [2578](https://github.com/zeta-chain/node/pull/2578) - Add Gateway address in protocol contract list +### Refactor + +* [2615](https://github.com/zeta-chain/node/pull/2615) - Refactor cleanup of outbound trackers + ## v19.0.0 ### Breaking Changes diff --git a/docs/cli/zetacored/zetacored_tx_fungible_update-gateway-contract.md b/docs/cli/zetacored/zetacored_tx_fungible_update-gateway-contract.md index d75877c440..0b4217c65b 100644 --- a/docs/cli/zetacored/zetacored_tx_fungible_update-gateway-contract.md +++ b/docs/cli/zetacored/zetacored_tx_fungible_update-gateway-contract.md @@ -3,7 +3,7 @@ Broadcast message UpdateGatewayContract to update the gateway contract address ``` -zetacored tx fungible update-gateway-contract [contract-address] [flags] +zetacored tx fungible update-gateway-contract [contract-address] [flags] ``` ### Options diff --git a/x/crosschain/keeper/msg_server_vote_outbound_tx.go b/x/crosschain/keeper/msg_server_vote_outbound_tx.go index 000c148414..3da780ccc1 100644 --- a/x/crosschain/keeper/msg_server_vote_outbound_tx.go +++ b/x/crosschain/keeper/msg_server_vote_outbound_tx.go @@ -91,9 +91,6 @@ func (k msgServer) VoteOutbound( } // Set the finalized ballot to the current outbound params. - // The two cases are possible - // 1. The outbound tx is successful, and this is the only ballot that is set - // 2. Revert TX is created, in which case the ballot for the revert TX would be set only when that ballot is finalized. cctx.SetOutboundBallotIndex(ballotIndex) // If ballot is successful, the value received should be the out tx amount. err = cctx.AddOutbound(ctx, *msg, ballot.BallotStatus) @@ -203,13 +200,9 @@ SaveOutbound saves the outbound transaction.It does the following things in one func (k Keeper) SaveOutbound(ctx sdk.Context, cctx *types.CrossChainTx) { // #nosec G115 always in range for _, outboundParams := range cctx.OutboundParams { - k.GetObserverKeeper().RemoveFromPendingNonces(ctx, outboundParams.TssPubkey, outboundParams.ReceiverChainId, int64(outboundParams.TssNonce)) + k.GetObserverKeeper(). + RemoveFromPendingNonces(ctx, outboundParams.TssPubkey, outboundParams.ReceiverChainId, int64(outboundParams.TssNonce)) k.RemoveOutboundTrackerFromStore(ctx, outboundParams.ReceiverChainId, outboundParams.TssNonce) - ctx.Logger().With( - "voteOutboundID", voteOutboundID, - "trackerIndex", getOutboundTrackerIndex(outboundParams.ReceiverChainId, outboundParams.TssNonce), - "blockHeight", ctx.BlockHeight(), - ).Info("Remove tracker") } // This should set nonce to cctx only if a new revert is created. From bd00a6f60619ba108d9f03addf14f1d319c5ebb8 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Thu, 1 Aug 2024 12:51:55 -0400 Subject: [PATCH 3/6] add extra tests for pending nonces --- x/observer/keeper/pending_nonces_test.go | 53 +++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/x/observer/keeper/pending_nonces_test.go b/x/observer/keeper/pending_nonces_test.go index 6eaaa3abf3..22faf24fa7 100644 --- a/x/observer/keeper/pending_nonces_test.go +++ b/x/observer/keeper/pending_nonces_test.go @@ -6,9 +6,10 @@ import ( "github.com/cosmos/cosmos-sdk/types/query" "github.com/stretchr/testify/require" - + "github.com/zeta-chain/zetacore/pkg/chains" keepertest "github.com/zeta-chain/zetacore/testutil/keeper" "github.com/zeta-chain/zetacore/testutil/sample" + "github.com/zeta-chain/zetacore/x/observer/types" ) func TestKeeper_PendingNoncesAll(t *testing.T) { @@ -136,4 +137,54 @@ func TestKeeper_RemoveFromPendingNonces(t *testing.T) { } require.True(t, nonceUpdated) }) + + t.Run("test removal within range only using fixed value", func(t *testing.T) { + k, ctx, _, _ := keepertest.ObserverKeeper(t) + + tss := sample.Tss() + // make nonces and pubkey deterministic for test + chainIDS := []int64{chains.GoerliLocalnet.ChainId, chains.BitcoinTestnet.ChainId, chains.BscTestnet.ChainId} + pn := make([]types.PendingNonces, len(chainIDS)) + + for idx, chainID := range chainIDS { + pn[idx] = types.PendingNonces{ + ChainId: chainID, + NonceLow: 1, + NonceHigh: 10, + Tss: tss.TssPubkey, + } + } + for _, pendingNonce := range pn { + k.SetPendingNonces(ctx, pendingNonce) + } + + // remove from pending nonces + k.RemoveFromPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId, 1) + pnGoerli, found := k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) + require.True(t, found) + require.Equal(t, int64(2), pnGoerli.NonceLow) + require.Equal(t, int64(10), pnGoerli.NonceHigh) + + // try removing lower than nonceLow, this might be triggered if we try to remove a previously removed nonce + k.RemoveFromPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId, 1) + pnGoerli, found = k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) + require.True(t, found) + require.Equal(t, int64(2), pnGoerli.NonceLow) + require.Equal(t, int64(10), pnGoerli.NonceHigh) + + // try removing higher than nonceHigh + k.RemoveFromPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId, 11) + pnGoerli, found = k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) + require.True(t, found) + require.Equal(t, int64(2), pnGoerli.NonceLow) + require.Equal(t, int64(10), pnGoerli.NonceHigh) + + //pending nonces for other chains should not be affected by removal + for _, chainID := range chainIDS[1:] { + pn, found := k.GetPendingNonces(ctx, tss.TssPubkey, chainID) + require.True(t, found) + require.Equal(t, int64(1), pn.NonceLow) + require.Equal(t, int64(10), pn.NonceHigh) + } + }) } From 22f14acd3f5e32aeb370866cb6f3ab2576865fe5 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Thu, 1 Aug 2024 16:04:38 -0400 Subject: [PATCH 4/6] add more checks for ballot index --- cmd/zetae2e/local/local.go | 12 +++--- testutil/keeper/crosschain.go | 7 +++- .../msg_server_vote_outbound_tx_test.go | 37 ++++++++++++------- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/cmd/zetae2e/local/local.go b/cmd/zetae2e/local/local.go index 337b529bd8..f083f9c3a4 100644 --- a/cmd/zetae2e/local/local.go +++ b/cmd/zetae2e/local/local.go @@ -351,7 +351,7 @@ func localE2ETest(cmd *cobra.Command, _ []string) { if testTSSMigration { runTSSMigrationTest(deployerRunner, logger, verbose, conf) } - + checkTrackers(deployerRunner) // print and validate report networkReport, err := deployerRunner.GenerateNetworkReport() if err != nil { @@ -362,16 +362,18 @@ func localE2ETest(cmd *cobra.Command, _ []string) { logger.Print("❌ network report validation failed %v", err) os.Exit(1) } - checkTrackers(ctx, deployerRunner) os.Exit(0) } -func checkTrackers(ctx context.Context, deployRunner *runner.E2ERunner) { +func checkTrackers(deployRunner *runner.E2ERunner) { // get all trackers - res, err := deployRunner.CctxClient.OutTxTrackerAll(ctx, &crosschaintypes.QueryAllOutboundTrackerRequest{}) + res, err := deployRunner.CctxClient.OutTxTrackerAll( + deployRunner.Ctx, + &crosschaintypes.QueryAllOutboundTrackerRequest{}, + ) require.NoError(deployRunner, err) - require.Empty(deployRunner, res.OutboundTracker, "there should be no trackers") + require.Empty(deployRunner, res.OutboundTracker, "there should be no trackers at the end of the test") } // waitKeygenHeight waits for keygen height diff --git a/testutil/keeper/crosschain.go b/testutil/keeper/crosschain.go index 6f350aa5ed..d12e4e37f9 100644 --- a/testutil/keeper/crosschain.go +++ b/testutil/keeper/crosschain.go @@ -395,10 +395,11 @@ func MockSaveOutbound( ctx sdk.Context, cctx *types.CrossChainTx, tss observertypes.TSS, + expectedNumberOfOutboundParams int, ) { m.On("RemoveFromPendingNonces", ctx, tss.TssPubkey, cctx.GetCurrentOutboundParam().ReceiverChainId, mock.Anything). - Return().Once() + Return().Times(expectedNumberOfOutboundParams) m.On("GetTSS", ctx).Return(observertypes.TSS{}, true) } @@ -407,10 +408,12 @@ func MockSaveOutboundNewRevertCreated( ctx sdk.Context, cctx *types.CrossChainTx, tss observertypes.TSS, + expectedNumberOfOutboundParams int, ) { m.On("RemoveFromPendingNonces", ctx, tss.TssPubkey, cctx.GetCurrentOutboundParam().ReceiverChainId, mock.Anything). - Return().Once() + Return().Times(expectedNumberOfOutboundParams) + m.On("GetTSS", ctx).Return(observertypes.TSS{}, true) m.On("SetNonceToCctx", mock.Anything, mock.Anything).Return().Once() } diff --git a/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go b/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go index 4c19461d44..0ac0d817ba 100644 --- a/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go +++ b/x/crosschain/keeper/msg_server_vote_outbound_tx_test.go @@ -150,7 +150,8 @@ func TestKeeper_VoteOutbound(t *testing.T) { keepertest.MockGetOutbound(observerMock, ctx) // Successfully mock SaveSuccessfulOutbound - keepertest.MockSaveOutbound(observerMock, ctx, cctx, tss) + expectedNumberOfOutboundParams := 1 + keepertest.MockSaveOutbound(observerMock, ctx, cctx, tss, expectedNumberOfOutboundParams) msgServer := keeper.NewMsgServerImpl(*k) msg := types.MsgVoteOutbound{ @@ -172,6 +173,7 @@ func TestKeeper_VoteOutbound(t *testing.T) { require.True(t, found) require.Equal(t, types.CctxStatus_OutboundMined, c.CctxStatus.Status) require.Equal(t, msg.Digest(), c.GetCurrentOutboundParam().BallotIndex) + require.Len(t, c.OutboundParams, expectedNumberOfOutboundParams) }) t.Run("successfully vote on outbound tx, vote-type failed", func(t *testing.T) { @@ -208,7 +210,8 @@ func TestKeeper_VoteOutbound(t *testing.T) { _ = keepertest.MockUpdateNonce(observerMock, senderChain) //Successfully mock SaveOutbound - keepertest.MockSaveOutboundNewRevertCreated(observerMock, ctx, cctx, tss) + expectedNumberOfOutboundParams := 2 + keepertest.MockSaveOutboundNewRevertCreated(observerMock, ctx, cctx, tss, expectedNumberOfOutboundParams) oldParamsLen := len(cctx.OutboundParams) msgServer := keeper.NewMsgServerImpl(*k) msg := types.MsgVoteOutbound{ @@ -229,10 +232,11 @@ func TestKeeper_VoteOutbound(t *testing.T) { c, found := k.GetCrossChainTx(ctx, cctx.Index) require.True(t, found) require.Equal(t, types.CctxStatus_PendingRevert, c.CctxStatus.Status) - require.Equal(t, oldParamsLen+1, len(c.OutboundParams)) + require.Len(t, c.OutboundParams, expectedNumberOfOutboundParams) require.Equal(t, types.TxFinalizationStatus_Executed, c.OutboundParams[oldParamsLen-1].TxFinalizationStatus) require.Equal(t, types.TxFinalizationStatus_NotFinalized, cctx.GetCurrentOutboundParam().TxFinalizationStatus) - require.Equal(t, msg.Digest(), c.GetCurrentOutboundParam().BallotIndex) + require.Equal(t, msg.Digest(), c.OutboundParams[0].BallotIndex) + require.Equal(t, "", c.GetCurrentOutboundParam().BallotIndex) }) t.Run("unsuccessfully vote on outbound tx, vote-type failed", func(t *testing.T) { @@ -271,10 +275,11 @@ func TestKeeper_VoteOutbound(t *testing.T) { keepertest.MockGetSupportedChainFromChainID(observerMock, senderChain) //Successfully mock SaveOutBound - keepertest.MockSaveOutbound(observerMock, ctx, cctx, tss) + expectedNumberOfOutboundParams := 2 + keepertest.MockSaveOutbound(observerMock, ctx, cctx, tss, expectedNumberOfOutboundParams) oldParamsLen := len(cctx.OutboundParams) msgServer := keeper.NewMsgServerImpl(*k) - _, err := msgServer.VoteOutbound(ctx, &types.MsgVoteOutbound{ + msg := types.MsgVoteOutbound{ CctxHash: cctx.Index, OutboundTssNonce: cctx.GetCurrentOutboundParam().TssNonce, OutboundChain: cctx.GetCurrentOutboundParam().ReceiverChainId, @@ -286,12 +291,15 @@ func TestKeeper_VoteOutbound(t *testing.T) { ObservedOutboundEffectiveGasPrice: math.NewInt(21), ObservedOutboundGasUsed: 21, CoinType: cctx.InboundParams.CoinType, - }) + } + _, err := msgServer.VoteOutbound(ctx, &msg) require.NoError(t, err) c, found := k.GetCrossChainTx(ctx, cctx.Index) require.True(t, found) require.Equal(t, types.CctxStatus_Aborted, c.CctxStatus.Status) - require.Equal(t, oldParamsLen+1, len(c.OutboundParams)) + require.Len(t, c.OutboundParams, expectedNumberOfOutboundParams) + require.Equal(t, msg.Digest(), c.OutboundParams[0].BallotIndex) + require.Equal(t, "", c.GetCurrentOutboundParam().BallotIndex) // The message processing fails during the creation of the revert tx // So the original outbound tx is executed and the revert tx is not finalized. // The cctx status is Aborted @@ -337,10 +345,11 @@ func TestKeeper_VoteOutbound(t *testing.T) { keepertest.MockGetSupportedChainFromChainID(observerMock, senderChain) //Successfully mock SaveFailedOutbound - keepertest.MockSaveOutbound(observerMock, ctx, cctx, tss) + expectedNumberOfOutboundParams := 1 + keepertest.MockSaveOutbound(observerMock, ctx, cctx, tss, expectedNumberOfOutboundParams) msgServer := keeper.NewMsgServerImpl(*k) - _, err := msgServer.VoteOutbound(ctx, &types.MsgVoteOutbound{ + msg := types.MsgVoteOutbound{ CctxHash: cctx.Index, OutboundTssNonce: cctx.GetCurrentOutboundParam().TssNonce, OutboundChain: cctx.GetCurrentOutboundParam().ReceiverChainId, @@ -352,12 +361,15 @@ func TestKeeper_VoteOutbound(t *testing.T) { ObservedOutboundEffectiveGasPrice: math.NewInt(21), ObservedOutboundGasUsed: 21, CoinType: cctx.InboundParams.CoinType, - }) + } + _, err := msgServer.VoteOutbound(ctx, &msg) require.NoError(t, err) c, found := k.GetCrossChainTx(ctx, cctx.Index) require.True(t, found) + require.Equal(t, msg.Digest(), c.GetCurrentOutboundParam().BallotIndex) // Status would be CctxStatus_PendingRevert if process outbound did not fail require.Equal(t, types.CctxStatus_Aborted, c.CctxStatus.Status) + require.Len(t, c.OutboundParams, expectedNumberOfOutboundParams) }) t.Run("fail to finalize outbound if not a finalizing vote", func(t *testing.T) { @@ -518,7 +530,7 @@ func TestKeeper_SaveSuccessfulOutbound(t *testing.T) { }) cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound k.SaveSuccessfulOutbound(ctx, cctx) - require.Equal(t, cctx.GetCurrentOutboundParam().BallotIndex, sample.String()) + _, found := k.GetOutboundTracker( ctx, cctx.GetCurrentOutboundParam().ReceiverChainId, @@ -540,7 +552,6 @@ func TestKeeper_SaveSuccessfulOutbound(t *testing.T) { } cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound k.SaveSuccessfulOutbound(ctx, cctx) - require.Equal(t, cctx.GetCurrentOutboundParam().BallotIndex, sample.String()) for _, outboundParams := range cctx.OutboundParams { _, found := k.GetOutboundTracker( From 36daf8e290c4ad910d585da3e9d762e80c323624 Mon Sep 17 00:00:00 2001 From: Tanmay Date: Fri, 2 Aug 2024 12:13:46 -0400 Subject: [PATCH 5/6] resolve comments --- cmd/zetae2e/local/local.go | 13 ++-------- e2e/runner/trackers.go | 17 ++++++++++++++ x/crosschain/types/cctx.go | 1 + x/observer/keeper/pending_nonces_test.go | 30 ++++++++++++------------ 4 files changed, 35 insertions(+), 26 deletions(-) create mode 100644 e2e/runner/trackers.go diff --git a/cmd/zetae2e/local/local.go b/cmd/zetae2e/local/local.go index f083f9c3a4..c57cd41c80 100644 --- a/cmd/zetae2e/local/local.go +++ b/cmd/zetae2e/local/local.go @@ -351,7 +351,8 @@ func localE2ETest(cmd *cobra.Command, _ []string) { if testTSSMigration { runTSSMigrationTest(deployerRunner, logger, verbose, conf) } - checkTrackers(deployerRunner) + // Verify that there are no trackers left over after tests complete + deployerRunner.EnsureNoTrackers() // print and validate report networkReport, err := deployerRunner.GenerateNetworkReport() if err != nil { @@ -366,16 +367,6 @@ func localE2ETest(cmd *cobra.Command, _ []string) { os.Exit(0) } -func checkTrackers(deployRunner *runner.E2ERunner) { - // get all trackers - res, err := deployRunner.CctxClient.OutTxTrackerAll( - deployRunner.Ctx, - &crosschaintypes.QueryAllOutboundTrackerRequest{}, - ) - require.NoError(deployRunner, err) - require.Empty(deployRunner, res.OutboundTracker, "there should be no trackers at the end of the test") -} - // waitKeygenHeight waits for keygen height func waitKeygenHeight( ctx context.Context, diff --git a/e2e/runner/trackers.go b/e2e/runner/trackers.go new file mode 100644 index 0000000000..0959086a3d --- /dev/null +++ b/e2e/runner/trackers.go @@ -0,0 +1,17 @@ +package runner + +import ( + "github.com/stretchr/testify/require" + crosschaintypes "github.com/zeta-chain/zetacore/x/crosschain/types" +) + +// EnsureNoTrackers ensures that there are no trackers left on zetacore +func (r *E2ERunner) EnsureNoTrackers() { + // get all trackers + res, err := r.CctxClient.OutTxTrackerAll( + r.Ctx, + &crosschaintypes.QueryAllOutboundTrackerRequest{}, + ) + require.NoError(r, err) + require.Empty(r, res.OutboundTracker, "there should be no trackers at the end of the test") +} diff --git a/x/crosschain/types/cctx.go b/x/crosschain/types/cctx.go index 88ab5c1b32..6952a5092f 100644 --- a/x/crosschain/types/cctx.go +++ b/x/crosschain/types/cctx.go @@ -164,6 +164,7 @@ func (m CrossChainTx) GetCCTXIndexBytes() ([32]byte, error) { return sendHash, nil } +// SetOutboundBallotIndex sets the outbound ballot index for the most recent outbound. func (m CrossChainTx) SetOutboundBallotIndex(index string) { m.GetCurrentOutboundParam().BallotIndex = index } diff --git a/x/observer/keeper/pending_nonces_test.go b/x/observer/keeper/pending_nonces_test.go index 22faf24fa7..a92155584c 100644 --- a/x/observer/keeper/pending_nonces_test.go +++ b/x/observer/keeper/pending_nonces_test.go @@ -144,47 +144,47 @@ func TestKeeper_RemoveFromPendingNonces(t *testing.T) { tss := sample.Tss() // make nonces and pubkey deterministic for test chainIDS := []int64{chains.GoerliLocalnet.ChainId, chains.BitcoinTestnet.ChainId, chains.BscTestnet.ChainId} - pn := make([]types.PendingNonces, len(chainIDS)) + pendingNonces := make([]types.PendingNonces, len(chainIDS)) for idx, chainID := range chainIDS { - pn[idx] = types.PendingNonces{ + pendingNonces[idx] = types.PendingNonces{ ChainId: chainID, NonceLow: 1, NonceHigh: 10, Tss: tss.TssPubkey, } } - for _, pendingNonce := range pn { + for _, pendingNonce := range pendingNonces { k.SetPendingNonces(ctx, pendingNonce) } // remove from pending nonces k.RemoveFromPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId, 1) - pnGoerli, found := k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) + actualPendingNoncesGoerli, found := k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) require.True(t, found) - require.Equal(t, int64(2), pnGoerli.NonceLow) - require.Equal(t, int64(10), pnGoerli.NonceHigh) + require.Equal(t, int64(2), actualPendingNoncesGoerli.NonceLow) + require.Equal(t, int64(10), actualPendingNoncesGoerli.NonceHigh) // try removing lower than nonceLow, this might be triggered if we try to remove a previously removed nonce k.RemoveFromPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId, 1) - pnGoerli, found = k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) + actualPendingNoncesGoerli, found = k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) require.True(t, found) - require.Equal(t, int64(2), pnGoerli.NonceLow) - require.Equal(t, int64(10), pnGoerli.NonceHigh) + require.Equal(t, int64(2), actualPendingNoncesGoerli.NonceLow) + require.Equal(t, int64(10), actualPendingNoncesGoerli.NonceHigh) // try removing higher than nonceHigh k.RemoveFromPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId, 11) - pnGoerli, found = k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) + actualPendingNoncesGoerli, found = k.GetPendingNonces(ctx, tss.TssPubkey, chains.GoerliLocalnet.ChainId) require.True(t, found) - require.Equal(t, int64(2), pnGoerli.NonceLow) - require.Equal(t, int64(10), pnGoerli.NonceHigh) + require.Equal(t, int64(2), actualPendingNoncesGoerli.NonceLow) + require.Equal(t, int64(10), actualPendingNoncesGoerli.NonceHigh) //pending nonces for other chains should not be affected by removal for _, chainID := range chainIDS[1:] { - pn, found := k.GetPendingNonces(ctx, tss.TssPubkey, chainID) + pendingNonces, found := k.GetPendingNonces(ctx, tss.TssPubkey, chainID) require.True(t, found) - require.Equal(t, int64(1), pn.NonceLow) - require.Equal(t, int64(10), pn.NonceHigh) + require.Equal(t, int64(1), pendingNonces.NonceLow) + require.Equal(t, int64(10), pendingNonces.NonceHigh) } }) } From 2e72ced4e19a26f375292bd1fa2580a6a0da2aae Mon Sep 17 00:00:00 2001 From: Tanmay Date: Tue, 6 Aug 2024 14:00:06 -0400 Subject: [PATCH 6/6] rebase develop --- e2e/runner/trackers.go | 1 + 1 file changed, 1 insertion(+) diff --git a/e2e/runner/trackers.go b/e2e/runner/trackers.go index 0959086a3d..2ff27180ed 100644 --- a/e2e/runner/trackers.go +++ b/e2e/runner/trackers.go @@ -2,6 +2,7 @@ package runner import ( "github.com/stretchr/testify/require" + crosschaintypes "github.com/zeta-chain/zetacore/x/crosschain/types" )