From b961f81cd069faa285d25dca16c422041e1d2c76 Mon Sep 17 00:00:00 2001 From: Joel Smith Date: Fri, 22 Nov 2024 11:07:11 -0600 Subject: [PATCH] feat: add storage leak test (#222) * feat: add storage leak test * cleanup test * add e2e to workflow (cherry picked from commit 64c5cd5a9fb98a6abab5e467159a55497693b68d) # Conflicts: # .github/workflows/packet-forward-middleware.yml # middleware/packet-forward-middleware/Makefile --- .../workflows/packet-forward-middleware.yml | 109 ++++++++ middleware/packet-forward-middleware/Makefile | 25 ++ .../e2e/storage_leak_test.go | 260 ++++++++++++++++++ .../packetforward/ibc_middleware.go | 2 +- 4 files changed, 395 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/packet-forward-middleware.yml create mode 100644 middleware/packet-forward-middleware/e2e/storage_leak_test.go diff --git a/.github/workflows/packet-forward-middleware.yml b/.github/workflows/packet-forward-middleware.yml new file mode 100644 index 00000000..0fa85517 --- /dev/null +++ b/.github/workflows/packet-forward-middleware.yml @@ -0,0 +1,109 @@ +name: packet-forward-middleware +on: + pull_request: + paths: + - "middleware/packet-forward-middleware/**" + - ".github/workflows/packet-forward-middleware.yml" + +env: + LINT_VERSION: v1.57.1 + GO_VERSION: 1.22.1 + WORKING_DIRECTORY: middleware/packet-forward-middleware + + DOCKER_TAG: pfm:local + TAR_PATH: /tmp/pfm-docker-image.tar + IMAGE_NAME: pfm-docker-image + +jobs: + golangci: + name: Linter + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: golangci-lint + uses: golangci/golangci-lint-action@v2 + with: + version: ${{ env.LINT_VERSION }} + working-directory: ${{ env.WORKING_DIRECTORY }} + args: --timeout=5m + + test: + runs-on: ubuntu-latest + name: test + steps: + - name: Install Go + uses: actions/setup-go@v4 + with: + go-version: ${{ env.GO_VERSION }} + + - uses: actions/checkout@v4 + + - name: Test + run: go test ./... + working-directory: ${{ env.WORKING_DIRECTORY }} + + build-docker: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Setup Go ${{ env.GO_VERSION }} + uses: actions/setup-go@v4 + with: + go-version: ${{ env.GO_VERSION }} + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + + - name: Build and export + uses: docker/build-push-action@v5 + with: + context: ${{ env.WORKING_DIRECTORY }} + tags: ${{ env.DOCKER_TAG }} + outputs: type=docker,dest=${{ env.TAR_PATH }} + + - name: Upload artifact + uses: actions/upload-artifact@v3 + with: + name: ${{ env.IMAGE_NAME }} + path: ${{ env.TAR_PATH }} + + e2e-tests: + needs: build-docker + runs-on: ubuntu-latest + defaults: + run: + working-directory: ${{ env.WORKING_DIRECTORY }} + strategy: + matrix: + test: + - "ictest-forward" + - "ictest-timeout" + - "ictest-storage-leak" + - "ictest-upgrade" + - "ictest-nonrefundable" + fail-fast: false + + steps: + - name: Set up Go ${{ env.GO_VERSION }} + uses: actions/setup-go@v4 + with: + go-version: ${{ env.GO_VERSION }} + + - uses: actions/checkout@v4 + + - name: Download Tarball Artifact + uses: actions/download-artifact@v3 + with: + name: ${{ env.IMAGE_NAME }} + path: /tmp + + - name: Load Docker Image + run: | + docker image load -i ${{ env.TAR_PATH }} + docker image load -i testing/previous_images/pfm_7_0_1.tar + docker image ls -a + + - name: Run Test + run: make ${{ matrix.test }} diff --git a/middleware/packet-forward-middleware/Makefile b/middleware/packet-forward-middleware/Makefile index a431a5cd..fe4e3ede 100644 --- a/middleware/packet-forward-middleware/Makefile +++ b/middleware/packet-forward-middleware/Makefile @@ -211,6 +211,31 @@ endif .PHONY: run-tests test test-all $(TEST_TARGETS) ############################################################################### +<<<<<<< HEAD +======= +### e2e interchain test ### +############################################################################### + +local-image: + docker build . -t pfm:local + +ictest-forward: + cd e2e && go test -race -v -timeout 15m -run TestPacketForwardMiddleware . + +ictest-timeout: + cd e2e && go test -race -v -timeout 15m -run TestTimeoutOnForward . + +ictest-storage-leak: + cd e2e && go test -race -v -timeout 15m -run TestStorageLeak . + +ictest-upgrade: + cd e2e && go test -race -v -timeout 15m -run TestPFMUpgrade . + +ictest-nonrefundable: + cd e2e && go test -race -v -timeout 15m -run TestNonRefundable . + +############################################################################### +>>>>>>> 64c5cd5 (feat: add storage leak test (#222)) ### Linting ### ############################################################################### diff --git a/middleware/packet-forward-middleware/e2e/storage_leak_test.go b/middleware/packet-forward-middleware/e2e/storage_leak_test.go new file mode 100644 index 00000000..5306bec7 --- /dev/null +++ b/middleware/packet-forward-middleware/e2e/storage_leak_test.go @@ -0,0 +1,260 @@ +package e2e + +import ( + "context" + "encoding/json" + "testing" + "time" + + "cosmossdk.io/math" + transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" + chantypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" + "github.com/strangelove-ventures/interchaintest/v8" + "github.com/strangelove-ventures/interchaintest/v8/chain/cosmos" + "github.com/strangelove-ventures/interchaintest/v8/ibc" + "github.com/strangelove-ventures/interchaintest/v8/relayer" + "github.com/strangelove-ventures/interchaintest/v8/testreporter" + "github.com/strangelove-ventures/interchaintest/v8/testutil" + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" +) + +type PFMExport struct { + AppState struct { + PacketForwardMiddleware struct { + InFlightPackets map[string]interface{} `json:"in_flight_packets"` + } `json:"packetfowardmiddleware"` + } `json:"app_state"` +} + +// TestStorageLeak verifies that that the PFM module does not retain any in-flight packets after a timeout occurs +func TestStorageLeak(t *testing.T) { + if testing.Short() { + t.Skip("skipping in short mode") + } + + var ( + ctx = context.Background() + client, network = interchaintest.DockerSetup(t) + rep = testreporter.NewNopReporter() + eRep = rep.RelayerExecReporter(t) + chainIdA, chainIdB, chainIdC = "chain-a", "chain-b", "chain-c" + ) + + vals := 1 + fullNodes := 0 + + baseCfg := DefaultConfig + + baseCfg.ChainID = chainIdA + configA := baseCfg + + baseCfg.ChainID = chainIdB + configB := baseCfg + + baseCfg.ChainID = chainIdC + configC := baseCfg + + cf := interchaintest.NewBuiltinChainFactory(zaptest.NewLogger(t), []*interchaintest.ChainSpec{ + {Name: "pfm", ChainConfig: configA, NumFullNodes: &fullNodes, NumValidators: &vals}, + {Name: "pfm", ChainConfig: configB, NumFullNodes: &fullNodes, NumValidators: &vals}, + {Name: "pfm", ChainConfig: configC, NumFullNodes: &fullNodes, NumValidators: &vals}, + }) + + chains, err := cf.Chains(t.Name()) + require.NoError(t, err) + + chainA, chainB, chainC := chains[0].(*cosmos.CosmosChain), chains[1].(*cosmos.CosmosChain), chains[2].(*cosmos.CosmosChain) + + r := interchaintest.NewBuiltinRelayerFactory( + ibc.CosmosRly, + zaptest.NewLogger(t), + relayer.DockerImage(&DefaultRelayer), + relayer.StartupFlags("--processor", "events", "--block-history", "100"), + ).Build(t, client, network) + + const pathAB = "ab" + const pathBC = "bc" + + ic := interchaintest.NewInterchain(). + AddChain(chainA). + AddChain(chainB). + AddChain(chainC). + AddRelayer(r, "relayer"). + AddLink(interchaintest.InterchainLink{ + Chain1: chainA, + Chain2: chainB, + Relayer: r, + Path: pathAB, + }). + AddLink(interchaintest.InterchainLink{ + Chain1: chainB, + Chain2: chainC, + Relayer: r, + Path: pathBC, + }) + + require.NoError(t, ic.Build(ctx, eRep, interchaintest.InterchainBuildOptions{ + TestName: t.Name(), + Client: client, + NetworkID: network, + SkipPathCreation: false, + })) + + t.Cleanup(func() { + _ = ic.Close() + }) + + // Start the relayer on only the path between chainA<>chainB so that the initial transfer succeeds + err = r.StartRelayer(ctx, eRep, pathAB) + require.NoError(t, err) + + t.Cleanup( + func() { + err := r.StopRelayer(ctx, eRep) + if err != nil { + t.Logf("an error occured while stopping the relayer: %s", err) + } + }, + ) + + // Fund user accounts with initial balances and get the transfer channel information between each set of chains + initBal := math.NewInt(10_000_000_000) + users := interchaintest.GetAndFundTestUsers(t, ctx, t.Name(), initBal, chainA, chainB, chainC) + + abChan, err := ibc.GetTransferChannel(ctx, r, eRep, chainIdA, chainIdB) + require.NoError(t, err) + + baChan := abChan.Counterparty + + cbChan, err := ibc.GetTransferChannel(ctx, r, eRep, chainIdC, chainIdB) + require.NoError(t, err) + + bcChan := cbChan.Counterparty + + userA, userB, userC := users[0], users[1], users[2] + + // Compose the prefixed denoms and ibc denom for asserting balances + firstHopDenom := transfertypes.GetPrefixedDenom(baChan.PortID, baChan.ChannelID, chainA.Config().Denom) + secondHopDenom := transfertypes.GetPrefixedDenom(cbChan.PortID, cbChan.ChannelID, firstHopDenom) + + firstHopDenomTrace := transfertypes.ParseDenomTrace(firstHopDenom) + secondHopDenomTrace := transfertypes.ParseDenomTrace(secondHopDenom) + + firstHopIBCDenom := firstHopDenomTrace.IBCDenom() + secondHopIBCDenom := secondHopDenomTrace.IBCDenom() + + firstHopEscrowAccount := transfertypes.GetEscrowAddress(abChan.PortID, abChan.ChannelID).String() + secondHopEscrowAccount := transfertypes.GetEscrowAddress(bcChan.PortID, bcChan.ChannelID).String() + + zeroBal := math.ZeroInt() + transferAmount := math.NewInt(100_000) + + // Attempt to send packet from Chain A->Chain B->Chain C->Chain D + transfer := ibc.WalletAmount{ + Address: userB.FormattedAddress(), + Denom: chainA.Config().Denom, + Amount: transferAmount, + } + + retries := uint8(10) + + firstHopMetadata := &PacketMetadata{ + Forward: &ForwardMetadata{ + Receiver: userC.FormattedAddress(), + Channel: bcChan.ChannelID, + Port: bcChan.PortID, + Retries: &retries, + Timeout: time.Second * 1, // Set low timeout for forward from chainB<>chainC + }, + } + + memo, err := json.Marshal(firstHopMetadata) + require.NoError(t, err) + + opts := ibc.TransferOptions{ + Memo: string(memo), + } + + chainBHeight, err := chainB.Height(ctx) + require.NoError(t, err) + + transferTx, err := chainA.SendIBCTransfer(ctx, abChan.ChannelID, userA.KeyName(), transfer, opts) + require.NoError(t, err) + + // Poll for MsgRecvPacket on chainB + _, err = cosmos.PollForMessage[*chantypes.MsgRecvPacket](ctx, chainB, cosmos.DefaultEncoding().InterfaceRegistry, chainBHeight, chainBHeight+20, nil) + require.NoError(t, err) + + // Stop the relayer and wait for the timeout to happen on chainC + err = r.StopRelayer(ctx, eRep) + require.NoError(t, err) + + time.Sleep(time.Second * 11) + + // Restart the relayer + err = r.StartRelayer(ctx, eRep, pathAB, pathBC) + require.NoError(t, err) + + chainAHeight, err := chainA.Height(ctx) + require.NoError(t, err) + + chainBHeight, err = chainB.Height(ctx) + require.NoError(t, err) + + // Poll for the MsgTimeout on chainB and the MsgAck on chainA + _, err = cosmos.PollForMessage[*chantypes.MsgTimeout](ctx, chainB, chainB.Config().EncodingConfig.InterfaceRegistry, chainBHeight, chainBHeight+20, nil) + require.NoError(t, err) + + _, err = testutil.PollForAck(ctx, chainA, chainAHeight, chainAHeight+50, transferTx.Packet) + require.NoError(t, err) + + err = testutil.WaitForBlocks(ctx, 1, chainA) + require.NoError(t, err) + + // Assert balances to ensure that the funds are still on the original sending chain + chainABalance, err := chainA.GetBalance(ctx, userA.FormattedAddress(), chainA.Config().Denom) + require.NoError(t, err) + + chainBBalance, err := chainB.GetBalance(ctx, userB.FormattedAddress(), firstHopIBCDenom) + require.NoError(t, err) + + chainCBalance, err := chainC.GetBalance(ctx, userC.FormattedAddress(), secondHopIBCDenom) + require.NoError(t, err) + + require.True(t, chainABalance.Equal(initBal)) + require.True(t, chainBBalance.Equal(zeroBal)) + require.True(t, chainCBalance.Equal(zeroBal)) + + firstHopEscrowBalance, err := chainA.GetBalance(ctx, firstHopEscrowAccount, chainA.Config().Denom) + require.NoError(t, err) + + secondHopEscrowBalance, err := chainB.GetBalance(ctx, secondHopEscrowAccount, firstHopIBCDenom) + require.NoError(t, err) + + require.True(t, firstHopEscrowBalance.Equal(zeroBal)) + require.True(t, secondHopEscrowBalance.Equal(zeroBal)) + + // Wait for blocks + err = testutil.WaitForBlocks(ctx, 10, chainA, chainB, chainC) + require.NoError(t, err) + + // loop through chain a -> chain d + cosmosChains := []*cosmos.CosmosChain{chainA, chainB, chainC} + for i := 0; i < len(cosmosChains); i++ { + chain := cosmosChains[i] + chain.StopAllNodes(ctx) + + // get exported packet forward middleware state + stdOut, _, err := chain.GetNode().ExecBin(ctx, "export", "--modules-to-export=packetfowardmiddleware") + require.NoError(t, err) + + chain.StartAllNodes(ctx) + + // validate that there are no in-flight packets + var result PFMExport + err = json.Unmarshal(stdOut, &result) + require.NoError(t, err) + require.Len(t, result.AppState.PacketForwardMiddleware.InFlightPackets, 0) + } +} diff --git a/middleware/packet-forward-middleware/packetforward/ibc_middleware.go b/middleware/packet-forward-middleware/packetforward/ibc_middleware.go index 3e2083ae..82507e7e 100644 --- a/middleware/packet-forward-middleware/packetforward/ibc_middleware.go +++ b/middleware/packet-forward-middleware/packetforward/ibc_middleware.go @@ -363,8 +363,8 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Pac inFlightPacket, err := im.keeper.TimeoutShouldRetry(ctx, packet) if inFlightPacket != nil { + im.keeper.RemoveInFlightPacket(ctx, packet) if err != nil { - im.keeper.RemoveInFlightPacket(ctx, packet) // this is a forwarded packet, so override handling to avoid refund from being processed on this chain. // WriteAcknowledgement with proxied ack to return success/fail to previous chain. return im.keeper.WriteAcknowledgementForForwardedPacket(ctx, packet, data, inFlightPacket, newErrorAcknowledgement(err))