From 0223e5a24b69291e67e40673b3cee8f448102189 Mon Sep 17 00:00:00 2001 From: Bartek Tofel Date: Wed, 24 Apr 2024 15:39:51 +0200 Subject: [PATCH] fix possible nil pointer, remove block range timeout, mutex for error appending --- integration-tests/actions/seth/actions.go | 53 ------------------- .../actions/seth/keeper_helpers.go | 24 +++++---- integration-tests/actions/seth/refund.go | 19 ------- .../testreporters/keeper_benchmark.go | 5 +- .../testsetups/keeper_benchmark.go | 15 +----- integration-tests/utils/seth.go | 16 +++++- 6 files changed, 33 insertions(+), 99 deletions(-) diff --git a/integration-tests/actions/seth/actions.go b/integration-tests/actions/seth/actions.go index 8b9b4b56102..c4636ebf139 100644 --- a/integration-tests/actions/seth/actions.go +++ b/integration-tests/actions/seth/actions.go @@ -3,13 +3,9 @@ package actions_seth import ( "context" "crypto/ecdsa" - "encoding/hex" - "encoding/json" "fmt" "math" "math/big" - "os" - "path/filepath" "strings" "testing" "time" @@ -550,55 +546,6 @@ func TeardownRemoteSuite( "Environment is left running so you can try manually!") } - // This is a failsafe, we should never use ephemeral keys on live networks - if !client.Cfg.IsSimulatedNetwork() { - if err := ReturnFundFromEphemeralKeys(l, client); err != nil { - l.Error().Err(err).Str("Namespace", namespace). - Msg("Error attempting to return funds from ephemeral keys to root key") - - pkStrings := []string{} - for _, pk := range client.PrivateKeys { - privateKeyBytes := pk.D.Bytes() - privateKeyBytes32 := make([]byte, 32) - copy(privateKeyBytes32[32-len(privateKeyBytes):], privateKeyBytes) - privateKeyHex := hex.EncodeToString(privateKeyBytes32) - pkStrings = append(pkStrings, "0x"+privateKeyHex) - } - - privateKeyJson, jsonErr := json.Marshal(pkStrings) - if jsonErr != nil { - l.Error(). - Err(jsonErr). - Msg("Error marshalling private keys to JSON. Funds are left in ephemeral keys") - - return err - } - - fileName := "ephemeral_addresses_private_keys.json" - writeErr := os.WriteFile(fileName, privateKeyJson, 0600) - if writeErr != nil { - l.Error(). - Err(writeErr). - Msg("Error writing ephemeral addresses private keys to file. Funds are left in ephemeral keys") - - return err - } - absolutePath, pathErr := filepath.Abs(fileName) - if pathErr != nil { - l.Error(). - Err(pathErr). - Str("FileName", fileName). - Msg("Error getting absolute path of file with private keys. Try looking for it yourself.") - - return err - } - - l.Info(). - Str("Filepath", absolutePath). - Msg("Private keys for ephemeral addresses are saved in the file. You can use them to return funds manually.") - } - } - return err } diff --git a/integration-tests/actions/seth/keeper_helpers.go b/integration-tests/actions/seth/keeper_helpers.go index 9309522228d..2b40d20cf9e 100644 --- a/integration-tests/actions/seth/keeper_helpers.go +++ b/integration-tests/actions/seth/keeper_helpers.go @@ -11,6 +11,8 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/types" + "github.com/google/uuid" + "github.com/pkg/errors" "github.com/smartcontractkit/seth" "github.com/stretchr/testify/require" @@ -224,7 +226,7 @@ func RegisterUpkeepContracts(t *testing.T, client *seth.Client, linkToken contra numberOfContracts, upkeepAddresses, checkData, isLogTrigger, isMercury) } -// RegisterUpkeepContractsWithCheckData concurrently registers a set of upkeep contracts with the given check data. It requires at least 1 ephemeral key to be present in Seth config. +// RegisterUpkeepContractsWithCheckData concurrently registers a set of upkeep contracts with the given check data. It requires at least 1 extra private key (epehemeral or static) to be present in Seth config. func RegisterUpkeepContractsWithCheckData(t *testing.T, client *seth.Client, linkToken contracts.LinkToken, linkFunds *big.Int, upkeepGasLimit uint32, registry contracts.KeeperRegistry, registrar contracts.KeeperRegistrar, numberOfContracts int, upkeepAddresses []string, checkData [][]byte, isLogTrigger bool, isMercury bool) []*big.Int { l := logging.GetTestLogger(t) registrationTxHashes := make([]common.Hash, 0) @@ -253,14 +255,12 @@ func RegisterUpkeepContractsWithCheckData(t *testing.T, client *seth.Client, lin var wgProcesses sync.WaitGroup wgProcesses.Add(len(upkeepAddresses)) - deplymentErrors := []error{} - deploymentCh := make(chan result, numberOfContracts) - // used mostly for logging/visibility and upkeep name creation atomicCounter := atomic.Uint64{} var registerUpkeepFn = func(channel chan result, keyNum int, config config) { atomicCounter.Add(1) + uuid := uuid.New().String() req, err := registrar.EncodeRegisterRequest( fmt.Sprintf("upkeep_%d", atomicCounter.Load()), @@ -277,34 +277,40 @@ func RegisterUpkeepContractsWithCheckData(t *testing.T, client *seth.Client, lin ) if err != nil { - channel <- result{err: err} + channel <- result{err: errors.Wrapf(err, "[uuid: %s] Failed to encode register request for upkeep %s", uuid, config.address)} return } balance, err := linkToken.BalanceOf(context.Background(), client.Addresses[keyNum].Hex()) if err != nil { - channel <- result{err: fmt.Errorf("Failed to get LINK balance of %s: %w", client.Addresses[keyNum].Hex(), err)} + channel <- result{err: errors.Wrapf(err, "[uuid: %s]Failed to get LINK balance of %s", uuid, client.Addresses[keyNum].Hex())} return } // not stricly necessary, but helps us to avoid an errorless revert if there is not enough LINK if balance.Cmp(linkFunds) < 0 { - channel <- result{err: fmt.Errorf("Not enough LINK balance for %s. Has: %s. Needs: %s", client.Addresses[keyNum].Hex(), balance.String(), linkFunds.String())} + channel <- result{err: fmt.Errorf("[uuid: %s] Not enough LINK balance for %s. Has: %s. Needs: %s", uuid, client.Addresses[keyNum].Hex(), balance.String(), linkFunds.String())} return } tx, err := linkToken.TransferAndCallFromKey(registrar.Address(), linkFunds, req, keyNum) - channel <- result{tx: tx, err: err} + channel <- result{tx: tx, err: errors.Wrapf(err, "[uuid: %s] Failed to register upkeep %s", uuid, config.address)} } + deploymentCh := make(chan result, numberOfContracts) + deplymentErrors := []error{} + errMut := sync.Mutex{} + // listen in the background until all registrations are done (it won't fail on the first error) go func() { defer l.Debug().Msg("Finished listening to results of registering upkeeps") for r := range deploymentCh { if r.err != nil { + errMut.Lock() l.Error().Err(r.err).Msg("Failed to register upkeep") deplymentErrors = append(deplymentErrors, r.err) wgProcesses.Done() + errMut.Unlock() continue } @@ -344,7 +350,7 @@ func RegisterUpkeepContractsWithCheckData(t *testing.T, client *seth.Client, lin require.Equal(t, 0, len(deplymentErrors), "Failed to register some upkeeps") - // Fetch the upkeep IDs + // Fetch the upkeep IDs (this could be moved to registerUpkeepFn to speed it up a bit) for _, txHash := range registrationTxHashes { receipt, err := client.Client.TransactionReceipt(context.Background(), txHash) require.NoError(t, err, "Registration tx should be completed") diff --git a/integration-tests/actions/seth/refund.go b/integration-tests/actions/seth/refund.go index 380dd44969a..15927f52227 100644 --- a/integration-tests/actions/seth/refund.go +++ b/integration-tests/actions/seth/refund.go @@ -17,7 +17,6 @@ import ( "github.com/pkg/errors" "github.com/rs/zerolog" "github.com/smartcontractkit/seth" - "golang.org/x/sync/errgroup" "github.com/smartcontractkit/chainlink-testing-framework/blockchain" @@ -368,21 +367,3 @@ func sendAllFundsIfPossible(log zerolog.Logger, sethClient *seth.Client, fromPri return nil } - -// ReturnFundFromEphemeralKeys returns funds from all ephemeral keys to the root key wallet -// using a variefy of retry strategies. -func ReturnFundFromEphemeralKeys(l zerolog.Logger, client *seth.Client) error { - err := client.NonceManager.UpdateNonces() - if err != nil { - return err - } - eg := errgroup.Group{} - for i := 1; i < len(client.Addresses); i++ { - i := i - eg.Go(func() error { - return sendAllFundsIfPossible(l, client, client.PrivateKeys[i]) - }) - } - - return eg.Wait() -} diff --git a/integration-tests/testreporters/keeper_benchmark.go b/integration-tests/testreporters/keeper_benchmark.go index b878ff67a31..6d6221fac89 100644 --- a/integration-tests/testreporters/keeper_benchmark.go +++ b/integration-tests/testreporters/keeper_benchmark.go @@ -223,8 +223,9 @@ func (k *KeeperBenchmarkTestReporter) WriteReport(folderLocation string) error { k.Summary.Metrics.TotalTimesPerformed = totalPerformed k.Summary.Metrics.TotalStaleReports = totalStaleReports k.Summary.Metrics.PercentStale = pctStale - k.Summary.Metrics.AverageActualPerformsPerBlock = float64(totalPerformed) / float64(k.Summary.TestInputs["BlockRange"].(int64)) - + if k.Summary.TestInputs["BlockRange"] != nil { + k.Summary.Metrics.AverageActualPerformsPerBlock = float64(totalPerformed) / float64(k.Summary.TestInputs["BlockRange"].(int64)) + } // TODO: Set test expectations /* Expect(int64(pctWithinSLA)).Should(BeNumerically(">=", int64(80)), "Expected PercentWithinSLA to be greater than or equal to 80, but got %f", pctWithinSLA) Expect(int64(pctReverted)).Should(BeNumerically("<=", int64(10)), "Expected PercentRevert to be less than or equal to 10, but got %f", pctReverted) diff --git a/integration-tests/testsetups/keeper_benchmark.go b/integration-tests/testsetups/keeper_benchmark.go index c2c24fac1e3..37420ff9e4d 100644 --- a/integration-tests/testsetups/keeper_benchmark.go +++ b/integration-tests/testsetups/keeper_benchmark.go @@ -252,21 +252,14 @@ func (k *KeeperBenchmarkTest) Run() { } } - effectiveBlockRange := inputs.Upkeeps.BlockRange + inputs.UpkeepSLA k.log.Info().Msgf("Waiting for %d blocks for all upkeeps to be performed", inputs.Upkeeps.BlockRange+inputs.UpkeepSLA) - timeout := effectiveBlockRange * 3 * int64(time.Second) - timeout = int64(float64(timeout) * 1.1) - timeoutDuration := time.Duration(timeout) - ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(timeoutDuration)) - defer cancel() - errgroup, errCtx := errgroup.WithContext(ctx) + errgroup, errCtx := errgroup.WithContext(context.Background()) var startedObservations = atomic.Int32{} var finishedObservations = atomic.Int32{} for rIndex := range k.keeperRegistries { - // headerCh := make(chan *types.Header, len(k.upkeepIDs[rIndex])) for index, upkeepID := range k.upkeepIDs[rIndex] { upkeepIDCopy := upkeepID registryIndex := rIndex @@ -275,7 +268,6 @@ func (k *KeeperBenchmarkTest) Run() { startedObservations.Add(1) k.log.Info().Str("UpkeepID", upkeepIDCopy.String()).Msg("Starting upkeep observation") - // doneCh := make(chan struct{}) confirmer := contracts.NewKeeperConsumerBenchmarkkUpkeepObserver( k.keeperConsumerContracts[registryIndex], k.keeperRegistries[registryIndex], @@ -286,7 +278,6 @@ func (k *KeeperBenchmarkTest) Run() { upkeepIndex, inputs.Upkeeps.FirstEligibleBuffer, k.log, - // doneCh, ) k.log.Debug().Str("UpkeepID", upkeepIDCopy.String()).Msg("Subscribing to new headers for upkeep observation") @@ -304,10 +295,6 @@ func (k *KeeperBenchmarkTest) Run() { k.log.Error().Err(errCtx.Err()).Str("UpkeepID", upkeepIDCopy.String()).Msg("Stopping obervations due to error in one of the goroutines") sub.Unsubscribe() return nil - case <-ctx.Done(): // timeout, abandon ship! - k.log.Error().Str("UpkeepID", upkeepIDCopy.String()).Msg("Stopping obervations due to timeout") - sub.Unsubscribe() - return fmt.Errorf("failed to observe desired block range for upkeep %s before timeout", upkeepIDCopy.String()) case header := <-headerCh: // new block, check if upkeep was performed finished, headerErr := confirmer.ReceiveHeader(header) if headerErr != nil { diff --git a/integration-tests/utils/seth.go b/integration-tests/utils/seth.go index ef9b331a447..cc5f1c60485 100644 --- a/integration-tests/utils/seth.go +++ b/integration-tests/utils/seth.go @@ -67,13 +67,25 @@ func MustReplaceSimulatedNetworkUrlWithK8(l zerolog.Logger, network blockchain.E return network } - if _, ok := testEnvironment.URLs["Simulated Geth"]; !ok { + networkKeys := []string{"Simulated Geth", "Simulated-Geth"} + var keyToUse string + + for _, key := range networkKeys { + _, ok := testEnvironment.URLs[key] + if ok { + keyToUse = key + break + } + } + + if keyToUse == "" { for k := range testEnvironment.URLs { l.Info().Str("Network", k).Msg("Available networks") } panic("no network settings for Simulated Geth") } - network.URLs = testEnvironment.URLs["Simulated Geth"] + + network.URLs = testEnvironment.URLs[keyToUse] return network }