From 810ce7ccd4afbb3335682f40263e428b7f7f3a82 Mon Sep 17 00:00:00 2001 From: Christian Lohr Date: Fri, 5 Jul 2024 17:45:32 +0200 Subject: [PATCH 1/2] feat: remove retry limit from erc20 transfers --- x/evm/keeper/attest_submit_logic_call.go | 79 ++++----- x/evm/keeper/attest_submit_logic_call_test.go | 153 ++++++++++++++++++ x/evm/keeper/smart_contract_deployment.go | 18 ++- x/skyway/keeper/grpc_query.go | 6 + x/skyway/types/expected_keepers.go | 1 + 5 files changed, 214 insertions(+), 43 deletions(-) create mode 100644 x/evm/keeper/attest_submit_logic_call_test.go diff --git a/x/evm/keeper/attest_submit_logic_call.go b/x/evm/keeper/attest_submit_logic_call.go index e7f10829..3764d312 100644 --- a/x/evm/keeper/attest_submit_logic_call.go +++ b/x/evm/keeper/attest_submit_logic_call.go @@ -100,10 +100,41 @@ func (a *submitLogicCallAttester) attemptRetry(ctx sdk.Context, proof *types.Sma types.SmartContractExecutionMessageType.With(fmt.Sprintf("%T", a.action)), ) + // Check if failed message was an ERC20 transfer. If so, override any + // max retries and keep going. + // Update deployment accordingly if exists + // Sets existing transfer state to FAILAED and removes it from the cache. + var deployment *types.SmartContractDeployment + var failedTransfer *types.SmartContractDeployment_ERC20Transfer + var smartContractID uint64 + if a.k.deploymentCache.Has(ctx, a.chainReferenceID, a.msgID) { + smartContractID = a.k.deploymentCache.Get(ctx, a.chainReferenceID, a.msgID) + deployment, _ = a.k.getSmartContractDeploymentByContractID(ctx, smartContractID, a.chainReferenceID) + if deployment == nil { + err := fmt.Errorf("no matching deployment found for contract ID %v on chain %v", smartContractID, a.chainReferenceID) + a.logger.WithError(err).Error(err.Error()) + return err + } + + for i, v := range deployment.Erc20Transfers { + if v.GetMsgID() == a.msgID { + if v.GetStatus() != types.SmartContractDeployment_ERC20Transfer_PENDING { + a.logger.WithFields("transfer-status", v.GetStatus()).Error("Unexpected status of failed message") + } + deployment.Erc20Transfers[i].Status = types.SmartContractDeployment_ERC20Transfer_FAIL + failedTransfer = &deployment.Erc20Transfers[i] + } + } + + a.k.deploymentCache.Delete(ctx, a.chainReferenceID, a.msgID) + } + // Retry message if eligible + // Must be less than cMaxSubmitLogicCallRetries + // ERC20 transfer do not have a limit var newMsgID uint64 slc := a.action - if slc.Retries < cMaxSubmitLogicCallRetries { + if slc.Retries < cMaxSubmitLogicCallRetries || failedTransfer != nil { slc.Retries++ a.logger.Info("retrying failed SubmitLogicCall message", "message-id", a.msgID, @@ -128,52 +159,22 @@ func (a *submitLogicCallAttester) attemptRetry(ctx sdk.Context, proof *types.Sma "chain-reference-id", a.chainReferenceID) } - // Update deployment accordingly if exists - // Sets existing transfer state to FAILAED and removes it from the cache. // If retry is happening, creates a new transfer record on the // deployment and adds it to the cache. - var deployment *types.SmartContractDeployment - if a.k.deploymentCache.Has(ctx, a.chainReferenceID, a.msgID) { - smartContractID := a.k.deploymentCache.Get(ctx, a.chainReferenceID, a.msgID) - deployment, _ = a.k.getSmartContractDeploymentByContractID(ctx, smartContractID, a.chainReferenceID) - if deployment == nil { - err := fmt.Errorf("no matching deployment found for contract ID %v on chain %v", smartContractID, a.chainReferenceID) - a.logger.WithError(err).Error(err.Error()) - return err - } - - var failedTransfer types.SmartContractDeployment_ERC20Transfer - for i, v := range deployment.Erc20Transfers { - if v.GetMsgID() == a.msgID { - if v.GetStatus() != types.SmartContractDeployment_ERC20Transfer_PENDING { - a.logger.WithFields("transfer-status", v.GetStatus()).Error("Unexpected status of failed message") - } - deployment.Erc20Transfers[i].Status = types.SmartContractDeployment_ERC20Transfer_FAIL - failedTransfer = deployment.Erc20Transfers[i] - } - } - - if newMsgID != 0 { - deployment.Erc20Transfers = append(deployment.Erc20Transfers, types.SmartContractDeployment_ERC20Transfer{ - Denom: failedTransfer.Denom, - Erc20: failedTransfer.Erc20, - MsgID: newMsgID, - Status: types.SmartContractDeployment_ERC20Transfer_PENDING, - }) - - defer func() { - if err == nil { - a.k.deploymentCache.Add(ctx, a.chainReferenceID, smartContractID, newMsgID) - } - }() - } + if newMsgID != 0 && failedTransfer != nil { + deployment.Erc20Transfers = append(deployment.Erc20Transfers, types.SmartContractDeployment_ERC20Transfer{ + Denom: failedTransfer.Denom, + Erc20: failedTransfer.Erc20, + MsgID: newMsgID, + Status: types.SmartContractDeployment_ERC20Transfer_PENDING, + }) if err := a.k.updateSmartContractDeployment(ctx, smartContractID, a.chainReferenceID, deployment); err != nil { a.logger.WithError(err).Error("Failed to update smart contract deployment.") return err } - a.k.deploymentCache.Delete(ctx, a.chainReferenceID, a.msgID) + a.k.deploymentCache.Add(ctx, a.chainReferenceID, smartContractID, newMsgID) } return nil diff --git a/x/evm/keeper/attest_submit_logic_call_test.go b/x/evm/keeper/attest_submit_logic_call_test.go new file mode 100644 index 00000000..3ef95988 --- /dev/null +++ b/x/evm/keeper/attest_submit_logic_call_test.go @@ -0,0 +1,153 @@ +package keeper + +import ( + codectypes "github.com/cosmos/cosmos-sdk/codec/types" + sdk "github.com/cosmos/cosmos-sdk/types" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + consensusmocks "github.com/palomachain/paloma/x/consensus/keeper/consensus/mocks" + consensustypes "github.com/palomachain/paloma/x/consensus/types" + "github.com/palomachain/paloma/x/evm/types" + evmmocks "github.com/palomachain/paloma/x/evm/types/mocks" + "github.com/stretchr/testify/mock" +) + +var _ = Describe("attest submit logic call", func() { + var k *Keeper + var ctx sdk.Context + var q *consensusmocks.Queuer + var msg *consensustypes.QueuedSignedMessage + var consensuskeeper *evmmocks.ConsensusKeeper + var evidence []*consensustypes.Evidence + var retries uint32 + + testChain := &types.AddChainProposal{ + ChainReferenceID: "eth-main", + Title: "Test Title", + Description: "Test description", + BlockHeight: uint64(123), + BlockHashAtHeight: "0x1234", + } + + BeforeEach(func() { + var ms mockedServices + k, ms, ctx = NewEvmKeeper(GinkgoT()) + consensuskeeper = ms.ConsensusKeeper + q = consensusmocks.NewQueuer(GinkgoT()) + + snapshot := createSnapshot(testChain) + ms.ValsetKeeper.On("GetCurrentSnapshot", mock.Anything).Return(snapshot, nil) + + q.On("ChainInfo").Return("", "eth-main") + q.On("Remove", mock.Anything, uint64(123)).Return(nil) + ms.SkywayKeeper.On("GetLastObservedSkywayNonce", mock.Anything, mock.Anything). + Return(uint64(100), nil).Maybe() + + err := setupTestChainSupport(ctx, consensuskeeper, testChain, k) + Expect(err).To(BeNil()) + }) + + JustBeforeEach(func() { + consensusMsg, err := codectypes.NewAnyWithValue(&types.Message{ + Action: &types.Message_SubmitLogicCall{ + SubmitLogicCall: &types.SubmitLogicCall{ + Retries: retries, + }, + }, + }) + Expect(err).To(BeNil()) + + msg = &consensustypes.QueuedSignedMessage{ + Id: 123, + Msg: consensusMsg, + Evidence: evidence, + } + }) + + Context("attesting with proof error", func() { + BeforeEach(func() { + proof, _ := codectypes.NewAnyWithValue( + &types.SmartContractExecutionErrorProof{ + ErrorMessage: "an error", + }) + evidence = []*consensustypes.Evidence{{ + ValAddress: sdk.ValAddress("addr1"), + Proof: proof, + }, { + ValAddress: sdk.ValAddress("addr2"), + Proof: proof, + }} + }) + + JustBeforeEach(func() { + Expect(k.attestRouter(ctx, q, msg)).To(Succeed()) + }) + + Context("attesting with 0 retries", func() { + BeforeEach(func() { + retries = 0 + consensuskeeper.On("PutMessageInQueue", + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(uint64(10), nil).Once() + }) + + It("should retry the deployment", func() { + // Should be called once on setup and again on retry + consensuskeeper.AssertNumberOfCalls(GinkgoT(), "PutMessageInQueue", 2) + }) + + It("should keep the deployment", func() { + val, _ := k.getSmartContractDeploymentByContractID(ctx, 1, + testChain.GetChainReferenceID()) + Expect(val).ToNot(BeNil()) + }) + + It("should increase retries on the smart contract deployment", func() { + cm, _ := msg.ConsensusMsg(k.cdc) + action := cm.(*types.Message).Action.(*types.Message_SubmitLogicCall) + Expect(action.SubmitLogicCall.Retries).To(BeNumerically("==", 1)) + }) + }) + + Context("attesting after retry limit", func() { + BeforeEach(func() { + retries = 2 + }) + + Context("with regular SLC", func() { + It("should not put message back into the queue", func() { + // Should be called only once on setup + consensuskeeper.AssertNumberOfCalls(GinkgoT(), "PutMessageInQueue", 1) + }) + }) + + Context("with ERC20 transfer", func() { + BeforeEach(func() { + k.deploymentCache.Add(ctx, testChain.GetChainReferenceID(), 1, 123) + dep, _ := k.getSmartContractDeploymentByContractID(ctx, 1, testChain.GetChainReferenceID()) + dep.Erc20Transfers = append(dep.Erc20Transfers, types.SmartContractDeployment_ERC20Transfer{ + Denom: "test-denom", + Erc20: "test-denom", + MsgID: 123, + Status: types.SmartContractDeployment_ERC20Transfer_FAIL, + }) + k.updateSmartContractDeployment(ctx, 1, testChain.ChainReferenceID, dep) + consensuskeeper.On("PutMessageInQueue", + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(uint64(10), nil).Once() + }) + + It("should put message back into the queue", func() { + // Should be called only once on setup + consensuskeeper.AssertNumberOfCalls(GinkgoT(), "PutMessageInQueue", 2) + }) + }) + }) + }) +}) diff --git a/x/evm/keeper/smart_contract_deployment.go b/x/evm/keeper/smart_contract_deployment.go index 6e05fd53..298779fa 100644 --- a/x/evm/keeper/smart_contract_deployment.go +++ b/x/evm/keeper/smart_contract_deployment.go @@ -55,10 +55,23 @@ func (k Keeper) HasAnySmartContractDeployment(ctx context.Context, chainReferenc func (k Keeper) DeleteSmartContractDeploymentByContractID(ctx context.Context, smartContractID uint64, chainReferenceID string) { sdkCtx := sdk.UnwrapSDKContext(ctx) - _, key := k.getSmartContractDeploymentByContractID(sdkCtx, smartContractID, chainReferenceID) + c, key := k.getSmartContractDeploymentByContractID(sdkCtx, smartContractID, chainReferenceID) if key == nil { return } + lkup := make(map[string]bool) + for _, v := range c.Erc20Transfers { + if ok, fnd := lkup[v.GetErc20()]; fnd && ok { + continue + } + lkup[v.GetErc20()] = v.GetStatus() == types.SmartContractDeployment_ERC20Transfer_OK + } + for key, v := range lkup { + if !v { + liblog.FromSDKLogger(k.Logger(ctx)).WithFields("erc20", k).Error("cannot delete smart contract deployment due to pending erc20 transfer", "erc20", key) + return + } + } k.Logger(ctx).Info("removing a smart contract deployment", "smart-contract-id", smartContractID, "chain-reference-id", chainReferenceID) k.provideSmartContractDeploymentStore(sdkCtx).Delete(key) } @@ -348,9 +361,6 @@ func (k Keeper) tryDeployingSmartContractToAllChains(ctx context.Context, smartC for _, chainInfo := range chainInfos { k.Logger(ctx).Info("trying to deploy smart contract to EVM chain", "smart-contract-id", smartContract.GetId(), "chain-reference-id", chainInfo.GetChainReferenceID()) if k.HasAnySmartContractDeployment(ctx, chainInfo.GetChainReferenceID()) { - // TODO: Only wait if the status is IN_FLIGHT - // TODO: We probably want to still delete the deployment in case of error AS LONG as we haven't sent a move ownership message - // we are already deploying to this chain. Lets wait it out. continue } if chainInfo.GetActiveSmartContractID() >= smartContract.GetId() { diff --git a/x/skyway/keeper/grpc_query.go b/x/skyway/keeper/grpc_query.go index 18f5da8b..1edcb0f3 100644 --- a/x/skyway/keeper/grpc_query.go +++ b/x/skyway/keeper/grpc_query.go @@ -77,6 +77,12 @@ func (k Keeper) OutgoingTxBatches( ) (*types.QueryOutgoingTxBatchesResponse, error) { var batches []types.OutgoingTxBatch + if k.evmKeeper.HasAnySmartContractDeployment(c, req.ChainReferenceId) { + // Ongoing smart contract deployment, don't give out batches to relay + // in order to avoid nonce increase on old compass + return &types.QueryOutgoingTxBatchesResponse{Batches: batches}, nil + } + // Check for pending valset messages on the queue queue := consensustypes.Queue(evmtypes.ConsensusTurnstoneMessage, consensustypes.ChainTypeEVM, req.ChainReferenceId) valsetMessagesOnQueue, err := k.consensusKeeper.GetPendingValsetUpdates(c, queue) diff --git a/x/skyway/types/expected_keepers.go b/x/skyway/types/expected_keepers.go index e678f070..52e02c18 100644 --- a/x/skyway/types/expected_keepers.go +++ b/x/skyway/types/expected_keepers.go @@ -75,4 +75,5 @@ type EVMKeeper interface { PickValidatorForMessage(ctx context.Context, chainReferenceID string, requirements *xchain.JobRequirements) (string, error) GetEthAddressByValidator(ctx context.Context, validator sdk.ValAddress, chainReferenceId string) (ethAddress *EthAddress, found bool, err error) GetValidatorAddressByEthAddress(ctx context.Context, ethAddr EthAddress, chainReferenceId string) (valAddr sdk.ValAddress, found bool, err error) + HasAnySmartContractDeployment(ctx context.Context, chainReferenceID string) (found bool) } From 442e0500205878713214467a7dea3bb3ae9d7f8e Mon Sep 17 00:00:00 2001 From: Christian Lohr Date: Fri, 5 Jul 2024 18:03:12 +0200 Subject: [PATCH 2/2] chore: avoid map iteration --- x/evm/keeper/smart_contract_deployment.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/evm/keeper/smart_contract_deployment.go b/x/evm/keeper/smart_contract_deployment.go index 298779fa..0625ca70 100644 --- a/x/evm/keeper/smart_contract_deployment.go +++ b/x/evm/keeper/smart_contract_deployment.go @@ -66,8 +66,8 @@ func (k Keeper) DeleteSmartContractDeploymentByContractID(ctx context.Context, s } lkup[v.GetErc20()] = v.GetStatus() == types.SmartContractDeployment_ERC20Transfer_OK } - for key, v := range lkup { - if !v { + for _, v := range c.Erc20Transfers { + if !lkup[v.GetErc20()] { liblog.FromSDKLogger(k.Logger(ctx)).WithFields("erc20", k).Error("cannot delete smart contract deployment due to pending erc20 transfer", "erc20", key) return }