feat: improvements to big block tests #3493

smuu · 2024-05-17T15:09:52Z

Overview

This PR is for sanaz/big-block-test to debug the issues and to speed up the testing process.

smuu · 2024-05-17T15:10:45Z

test/e2e/testnet/node.go

 	err = n.Instance.Commit()
 	if err != nil {
 		return fmt.Errorf("committing instance: %w", err)
+	}

+	if err = n.Instance.AddFolder(nodeDir, remoteRootDir, "10001:10001"); err != nil {
+		return fmt.Errorf("copying over node %s directory: %w", n.Name, err)
 	}


Adding files after commit speeds up the set-up process

smuu · 2024-05-17T15:10:54Z

go.mod


 require (
 	cosmossdk.io/errors v1.0.1
 	cosmossdk.io/math v1.3.0
 	github.com/celestiaorg/blobstream-contracts/v3 v3.1.0
 	github.com/celestiaorg/go-square v1.0.1
 	github.com/celestiaorg/go-square/merkle v0.0.0-20240117232118-fd78256df076
-	github.com/celestiaorg/knuu v0.13.2
+	github.com/celestiaorg/knuu v0.13.3-rc.9


Using the latest fixes and improvements

smuu · 2024-05-17T15:11:16Z

test/e2e/testnet/testnet.go

-	// copy over the keyring directory to the txsim instance
-	err = txsim.Instance.AddFolder(txsimKeyringDir, txsimRootDir, "10001:10001")
+	err = txsim.Instance.Commit()
 	if err != nil {
 		log.Err(err).
-			Str("directory", txsimKeyringDir).
 			Str("name", name).
-			Msg("error adding keyring dir to txsim")
+			Msg("error committing txsim")
 		return err
 	}
-	err = txsim.Instance.Commit()
+
+	// copy over the keyring directory to the txsim instance
+	err = txsim.Instance.AddFolder(txsimKeyringDir, txsimRootDir, "10001:10001")
 	if err != nil {
 		log.Err(err).
+			Str("directory", txsimKeyringDir).
 			Str("name", name).
-			Msg("error committing txsim")
+			Msg("error adding keyring dir to txsim")
 		return err
 	}


Adding files after commit speeds up the set-up process

test/e2e/testnet/testnet.go

smuu · 2024-05-17T15:14:11Z

test/e2e/testnet/testnet.go

+	// start genesis nodes asynchronously
 	for _, node := range genesisNodes {
-		err := node.Start()
+		err := node.Instance.StartWithoutWait()
 		if err != nil {
 			return fmt.Errorf("node %s failed to start: %w", node.Name, err)
 		}
 	}
+	// wait for instances to be running
 	for _, node := range genesisNodes {
-		client, err := node.Client()
+		err := node.Instance.WaitInstanceIsRunning()
 		if err != nil {
-			return fmt.Errorf("failed to initialize node %s: %w", node.Name, err)
+			return fmt.Errorf("node %s failed to start: %w", node.Name, err)
 		}
+	}


We can start all nodes in an async way and then wait for all nodes to be started.
I contrast to start and wait for them one-by-one.

Absolutely, it is very useful!

rootulp · 2024-05-17T18:09:09Z

@smuu can you please provide a PR description? Did you mean to tag @staheri14 for review b/c this PR merges to her branch.

staheri14 · 2024-05-17T18:43:17Z

Thanks @smuu for the PR!

@smuu can you please provide a PR description? Did you mean to tag @staheri14 for review b/c this PR merges to her branch.

AFAIK, The purpose of this PR is to provide fixes for some issues that occur when using the latest release candidate of knuu for the big block tests (and e2e tests in general). That is why it is based off celestiaorg:sanaz/big-block-test .

rach-id

🚀

Left some comments to understand more.

Also, if we want these changes, maybe it makes sense to make an official knuu release instead of depending on an RC

rach-id · 2024-05-18T09:22:24Z

test/e2e/testnet/node.go

+	if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+		return 0, err
+	}
+
+	return 0, fmt.Errorf("error getting height: %w", err)


[nit]
What's the difference between the two returns? one is adding the string "error getting height:", but does it matter? is it being parsed somewhere?

rach-id · 2024-05-18T09:39:02Z

test/e2e/testnet/test_helpers.go

+	resultData, ok := result["result"].(map[string]interface{})
+	if !ok {
+		return 0, fmt.Errorf("error getting result from status")
+	}
+	syncInfo, ok := resultData["sync_info"].(map[string]interface{})
+	if !ok {
+		return 0, fmt.Errorf("error getting sync info from status")
+	}
+	latestBlockHeight, ok := syncInfo["latest_block_height"].(string)
+	if !ok {
+		return 0, fmt.Errorf("error getting latest block height from sync info")
+	}
+	latestBlockHeightInt, err := strconv.ParseInt(latestBlockHeight, 10, 64)
+	if err != nil {
+		return 0, fmt.Errorf("error converting latest block height to int: %w", err)
+	}


Instead of these gymnastics, why not parse the whole response to ResultStatus from https://github.com/celestiaorg/celestia-core/blob/a281e871c70f4c8bd4f6ab7c7e39e28c715d65b2/rpc/core/types/responses.go#L123-L128, and get the field you want from it.

rach-id · 2024-05-18T09:40:54Z

test/e2e/testnet/test_helpers.go

+type JSONRPCError struct {
+	Code    int
+	Message string
+	Data    string
+}
+
+func (e *JSONRPCError) Error() string {
+	return fmt.Sprintf("JSONRPC Error - Code: %d, Message: %s, Data: %s", e.Code, e.Message, e.Data)
+}
+
+func getStatus(executor *knuu.Executor, app *knuu.Instance) (string, error) {
+	nodeIP, err := app.GetIP()
+	if err != nil {
+		return "", fmt.Errorf("error getting node ip: %w", err)
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+	status, err := executor.ExecuteCommandWithContext(ctx, "wget", "-q", "-O", "-", fmt.Sprintf("%s:26657/status", nodeIP))
+	if err != nil {
+		return "", fmt.Errorf("error executing command: %w", err)
+	}
+	return status, nil
+}
+
+func latestBlockHeightFromStatus(status string) (int64, error) {
+	var result map[string]interface{}
+	err := json.Unmarshal([]byte(status), &result)
+	if err != nil {
+		return 0, fmt.Errorf("error unmarshalling status: %w", err)
+	}
+
+	if errorField, ok := result["error"]; ok {
+		errorData, ok := errorField.(map[string]interface{})
+		if !ok {
+			return 0, fmt.Errorf("error field exists but is not a map[string]interface{}")
+		}
+		jsonError := &JSONRPCError{}
+		if errorCode, ok := errorData["code"].(float64); ok {
+			jsonError.Code = int(errorCode)
+		}
+		if errorMessage, ok := errorData["message"].(string); ok {
+			jsonError.Message = errorMessage
+		}
+		if errorData, ok := errorData["data"].(string); ok {
+			jsonError.Data = errorData
+		}
+		return 0, jsonError
+	}


Similar, instead of defining these here and doing all of the heavy lifting, you could use these existing types: https://github.com/celestiaorg/celestia-core/blob/a281e871c70f4c8bd4f6ab7c7e39e28c715d65b2/rpc/jsonrpc/types/types.go#L140-L159
And, you could simply parse the response from the command using:

recv := new(types.RPCResponse) err = json.Unmarshal(status, recv)

rach-id · 2024-05-18T09:42:53Z

test/e2e/testnet/testnet.go

-	err = txsim.Instance.Commit()
+
+	// copy over the keyring directory to the txsim instance
+	err = txsim.Instance.AddFolder(txsimKeyringDir, txsimRootDir, "10001:10001")


[microscopic nit]
it would be nice to define some constants for UID and GID and explain in their docs that these are the permissions given in the Celestia-app docker images

…u/improvements-to-big-block-tests

Signed-off-by: Smuu <[email protected]>

ninabarbakadze

we're making some of these improvements in my pr already #3487

…u/improvements-to-big-block-tests

Signed-off-by: Smuu <[email protected]>

smuu · 2024-05-22T13:11:46Z

test/e2e/benchmark/benchmark.go

-	log.Println("Reading blockchain")
-	blockchain, err := testnode.ReadBlockHeights(context.Background(),
-		b.Node(0).AddressRPC(), 1, 5)


Before, the chain was only read until height 5. As the txsim instances start after the validators, the first blocks do not contain any txs. Now, we are reading the whole blockchain.

Thanks Samuel, it was intentional to reduce the height to 5 as otherwise it would take a long time to read the blockchain specially for large size blocks, and would fail from one run to another. But if it works for the entire blockchain, that would be even better, thanks!

smuu · 2024-05-22T13:12:49Z

test/e2e/testnet/testnet.go

 	for _, node := range genesisNodes {
-		err := node.Start()
+		err := node.StartAsync()


In order to speed up the start-up phase, we can start all validator instances async and then wait for them being started.

smuu · 2024-05-22T13:13:31Z

test/e2e/testnet/testnet.go

-	for _, node := range t.nodes {
-		if node.Instance.IsInState(knuu.Started) {
-			if err := node.Instance.Stop(); err != nil {
-				log.Err(err).
-					Str("name", node.Name).
-					Msg("node  failed to stop")
-				continue
-			}
-			if err := node.Instance.WaitInstanceIsStopped(); err != nil {
-				log.Err(err).
-					Str("name", node.Name).
-					Msg("node  failed to stop")
-				continue
-			}
-		}
-		if node.Instance.IsInState(knuu.Started, knuu.Stopped) {
-			err := node.Instance.Destroy()
-			if err != nil {
-				log.Err(err).
-					Str("name", node.Name).
-					Msg("node  failed to cleanup")
-			}


There is no need to stop the instance before destroying it; we can simply destroy it.

ninabarbakadze · 2024-05-22T16:07:56Z

we're making some of these improvements in my pr already #3487

ignore this, i reverted those improvements.

staheri14 · 2024-05-22T18:36:38Z

@smuu I apologize for any confusion. I did not intend to unilaterally close the PR. My goal was to merge the changes in this branch into sanaz/big-block-test as you requested. I'm not sure how the PR ended up getting closed.

Closes #3480 This PR backports many optimizations introduced in the big block tests via #3493 to the `main` branch. Will be ready for review after merging #3514 --------- Co-authored-by: Evan Forbes <[email protected]>

Closes #3480 This PR backports many optimizations introduced in the big block tests via celestiaorg/celestia-app#3493 to the `main` branch. Will be ready for review after merging celestiaorg/celestia-app#3514 --------- Co-authored-by: Evan Forbes <[email protected]>

feat: improvements to big block tests

a375802

Signed-off-by: Smuu <[email protected]>

smuu requested a review from a team as a code owner May 17, 2024 15:09

smuu requested review from ramin and rootulp and removed request for a team May 17, 2024 15:09

celestia-bot requested a review from a team May 17, 2024 15:10

github-actions bot added the external label May 17, 2024

smuu commented May 17, 2024

View reviewed changes

rootulp removed request for ramin and rootulp May 17, 2024 18:08

rootulp assigned smuu May 17, 2024

rach-id reviewed May 18, 2024

View reviewed changes

smuu requested a review from staheri14 May 21, 2024 10:22

smuu added 4 commits May 21, 2024 13:00

Merge remote-tracking branch 'upstream/sanaz/big-block-test' into smu…

2ac8972

…u/improvements-to-big-block-tests

feat: use released version

3782895

Signed-off-by: Smuu <[email protected]>

feat: start txsim clients async

8387d6d

Signed-off-by: Smuu <[email protected]>

feat: start node async but port foreward properly

8cb5b14

Signed-off-by: Smuu <[email protected]>

ninabarbakadze reviewed May 21, 2024

View reviewed changes

celestia-bot requested review from a team and ninabarbakadze and removed request for a team May 21, 2024 13:13

evan-forbes added the WS: Big Blonks 🔭 Improving consensus critical gossiping protocols label May 21, 2024

Merge remote-tracking branch 'upstream/sanaz/big-block-test' into smu…

89b5a77

…u/improvements-to-big-block-tests

celestia-bot requested a review from a team May 22, 2024 06:34

smuu marked this pull request as draft May 22, 2024 06:34

smuu added 3 commits May 22, 2024 08:54

fix: read full blockchain

afb2f92

Signed-off-by: Smuu <[email protected]>

fix: start async

4ef632e

Signed-off-by: Smuu <[email protected]>

doc: add to output how long txs are submitted

d010d6d

Signed-off-by: Smuu <[email protected]>

smuu added 5 commits May 22, 2024 09:35

doc: let test runner know that reading the blockchain might take a while

0a8b3fa

Signed-off-by: Smuu <[email protected]>

feat: read blocks with retry

18e7c65

Signed-off-by: Smuu <[email protected]>

feat: simply destroy instances without stopping before

92c60b4

Signed-off-by: Smuu <[email protected]>

feat: dont check if node is running

3fccde4

Signed-off-by: Smuu <[email protected]>

feat: use port foreward feat instead of executor

2c47988

Signed-off-by: Smuu <[email protected]>

smuu mentioned this pull request May 22, 2024

Investigating large e2e network test failures due to deployment issues #3488

Open

smuu commented May 22, 2024

View reviewed changes

staheri14 merged commit 3842cf1 into celestiaorg:sanaz/big-block-test May 22, 2024
31 of 33 checks passed

staheri14 mentioned this pull request May 29, 2024

test: backports knuu optimizations from 3493 #3515

Merged

smuu deleted the smuu/improvements-to-big-block-tests branch June 3, 2024 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improvements to big block tests #3493

feat: improvements to big block tests #3493

smuu commented May 17, 2024 •

edited

Loading

smuu May 17, 2024

smuu May 17, 2024

smuu May 17, 2024

smuu May 17, 2024

staheri14 May 22, 2024

rootulp commented May 17, 2024

staheri14 commented May 17, 2024 •

edited

Loading

rach-id left a comment •

edited

Loading

rach-id May 18, 2024

rach-id May 18, 2024

rach-id May 18, 2024

rach-id May 18, 2024

ninabarbakadze left a comment

smuu May 22, 2024

staheri14 May 22, 2024

smuu May 22, 2024

smuu May 22, 2024 •

edited

Loading

ninabarbakadze commented May 22, 2024

staheri14 commented May 22, 2024

feat: improvements to big block tests #3493

feat: improvements to big block tests #3493

Conversation

smuu commented May 17, 2024 • edited Loading

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp commented May 17, 2024

staheri14 commented May 17, 2024 • edited Loading

rach-id left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninabarbakadze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smuu May 22, 2024 • edited Loading

Choose a reason for hiding this comment

ninabarbakadze commented May 22, 2024

staheri14 commented May 22, 2024

smuu commented May 17, 2024 •

edited

Loading

staheri14 commented May 17, 2024 •

edited

Loading

rach-id left a comment •

edited

Loading

smuu May 22, 2024 •

edited

Loading