test: consistent app hash on v2 #3879

ninabarbakadze · 2024-09-16T15:48:53Z

Overview

coderabbitai · 2024-09-17T14:59:43Z

Walkthrough

The changes in the pull request enhance the testing framework for application hashes by introducing a structured approach to testing consistency across different versions of the Celestia application. A new appHashTest struct is created to encapsulate test case details, and existing tests are refactored into a table-driven format. Functions for encoding SDK messages are modularized, and the executeTxs function is updated to include application version handling. These modifications aim to improve the organization and maintainability of the test code.

Changes

Files	Change Summary
`app/test/consistent_apphash_test.go`	Introduced `appHashTest` struct and refactored `TestConsistentAppHash`. Added encoding functions for versions 1 and 2, and modified `executeTxs` to include `appVersion`. Created `createEncodedBlobTx` for transaction creation.

Assessment against linked issues

Objective	Addressed	Explanation
Consistent apphash test between v2 and main (Issue #3624)	✅
Refactor existing test to table-driven test (Issue #3624)	✅

Possibly related PRs

test: consistent appHash between commits #3513: Ensures consistent AppHash between commits, complementing the structured testing framework.
test: expand apphash test with all state machine msgs #3606: Expands the apphash test to include all state machine messages, enhancing testing capabilities.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
-- @coderabbitai generate unit testing code for this file.
-- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
-- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
-- @coderabbitai read src/utils.ts and generate unit testing code.
-- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
-- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

Outside diff range and nitpick comments (1)

app/test/consistent_apphash_test.go (1)

Line range hint 152-323: LGTM with a suggestion!

The encodedSdkMessagesV1 function is responsible for creating and encoding various SDK messages for v1 of the application. The function is well-structured, with a clear separation of messages for each block. It covers a wide range of SDK modules and creates messages with deterministic values.

The function uses appropriate naming conventions for variables and messages, making it easy to understand the purpose of each message.

However, the function is quite large and contains a lot of message creation logic. To improve readability and maintainability, consider refactoring the message creation logic into separate smaller functions for each module or message type. This will make the code more modular and easier to understand and maintain.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 228a32d and 54ff6d8.

Files selected for processing (1)

app/test/consistent_apphash_test.go (5 hunks)

Additional comments not posted (5)

app/test/consistent_apphash_test.go (5)

62-135: LGTM!

The TestConsistentAppHash function is well-structured and comprehensive. It covers testing the consistency of app hashes across different versions of the application. The test cases are defined in a clear and organized manner using the appHashTest struct. The test execution flow is logical and easy to follow.

The test cases cover both v1 and v2 of the application, ensuring that the app hashes remain consistent across different versions. The test also verifies the consistency of the data root.

Overall, the changes enhance the testing framework and provide a robust way to ensure the consistency of app hashes.

137-150: LGTM!

The getAccountsAndCreateSigner function is a helper utility that retrieves accounts from the keyring, queries their info, and creates a signer with those accounts. The function is well-named, and its purpose is clear. It takes the necessary parameters and returns the signer and account addresses.

The function does not contain any complex logic and serves as a useful utility for setting up the test environment.

325-338: LGTM!

The encodedSdkMessagesV2 function is responsible for creating and encoding SDK messages specific to v2 of the application. The function is small and focused, creating only the MsgTryUpgrade and MsgSignalVersion messages.

The function follows a similar structure to encodedSdkMessagesV1, using the processSdkMessages function to create encoded transactions. This consistency makes it easy to understand and maintain.

The function is well-named and its purpose is clear.

340-354: LGTM!

The createEncodedBlobTx function is responsible for creating, signing, and returning an encoded blob transaction. The function follows a clear and focused logic.

It takes the necessary parameters, creates a new blob using a fixed namespace and deterministic data, and constructs a blobTx struct with the required fields. It then uses the signer to create a PayForBlobs transaction with the blob and default transaction options.

The function uses appropriate naming conventions and structs to organize the data, making it easy to understand and maintain.

Line range hint 420-512: LGTM!

The executeTxs function is responsible for executing a set of transactions and returning the data hash and app hash. The function is well-structured and follows a clear sequence of steps.

It takes the necessary parameters, including the test app, encoded transactions, validators, last commit hash, and app version. It prepares a proposal, processes it, begins a block, delivers the transactions, ends the block, and commits the state using the appropriate functions of the test app.

The function handles both SDK transactions and blob transactions, performing necessary checks and validations throughout the process. It retrieves the app hash using the LastCommitID function of the test app and returns the data hash and app hash.

The function is well-organized and easy to follow, making it maintainable and understandable.

rootulp · 2024-09-17T15:24:12Z

app/test/consistent_apphash_test.go

+			name:    "execute sdk messages and blob tx on v2",
+			version: v2.Version,
+			encodedSdkMessages: func(t *testing.T, accountAddresses []sdk.AccAddress, genValidators []stakingtypes.Validator, testApp *app.App, signer *user.Signer, valSigner *user.Signer) ([][]byte, [][]byte, [][]byte) {
+				firstBlockEncodedTxs, secondBlockEncodedTxs, thirdBlockEncodedTxs := encodedSdkMessagesV1(t, accountAddresses, genValidators, testApp, signer, valSigner)


v2 removed x/blobstream so I think the blobstream transactions need to be removed the test case on v2. Alternatively if they aren't removed, maybe this test case should verify that x/blobstream messages are no-ops on v2.

I think those messages are supposed to be rejected on v2. I'll investigate.

It looks like the blobstream messages were never part of the v1 test suite anyway so I think doing it like this is fine

cmwaters

Looks good. Probably a good idea to add ICA and IBC messages as well to expand the surface area of the tests

cmwaters

Don't know why this was not included in my original review 🤷

cmwaters · 2024-09-19T09:44:35Z

app/test/consistent_apphash_test.go

@@ -388,7 +440,7 @@ func executeTxs(testApp *app.App, encodedBlobTx []byte, encodedSdkTxs [][]byte,
 	dataHash := resPrepareProposal.BlockData.Hash

 	header := tmproto.Header{
-		Version:        version.Consensus{App: 1},
+		Version:        version.Consensus{App: appVersion},


Suggested change

Version: version.Consensus{App: appVersion},

Version: version.Consensus{App: testApp.AppVersion()},

You could just do this which avoids needing the extra argument

ninabarbakadze · 2024-09-19T17:08:35Z

Looks good. Probably a good idea to add ICA and IBC messages as well to expand the surface area of the tests

I'm extending the test to include all IBC messages for all verisons

test: consistent app hash on v2

7f22afa

ninabarbakadze self-assigned this Sep 16, 2024

ninabarbakadze added 6 commits September 16, 2024 19:10

test: v2 generates apphash

ea51a2e

test: generates apphash

9bdc596

test: apphash matches

788c896

style: cleanup

a9e3561

Merge branch 'main' into nina/v2-apphash-test

60bdf44

style: delete print statements

54ff6d8

ninabarbakadze marked this pull request as ready for review September 17, 2024 14:59

ninabarbakadze requested a review from a team as a code owner September 17, 2024 14:59

ninabarbakadze requested review from rootulp and staheri14 and removed request for a team September 17, 2024 14:59

ninabarbakadze requested a review from cmwaters September 17, 2024 14:59

coderabbitai bot reviewed Sep 17, 2024

View reviewed changes

rootulp previously approved these changes Sep 17, 2024

View reviewed changes

cmwaters reviewed Sep 19, 2024

View reviewed changes

Merge branch 'main' into nina/v2-apphash-test

c9144af

celestia-bot requested review from a team and evan-forbes and removed request for a team September 19, 2024 17:32

ninabarbakadze marked this pull request as draft September 19, 2024 17:32

chore: expand test coverage

45180d4

ninabarbakadze dismissed rootulp’s stale review via 45180d4 September 21, 2024 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: consistent app hash on v2 #3879

test: consistent app hash on v2 #3879

ninabarbakadze commented Sep 16, 2024 •

edited

Loading

coderabbitai bot commented Sep 17, 2024 •

edited

Loading

Walkthrough

Changes

Assessment against linked issues

Possibly related PRs

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

coderabbitai bot left a comment

rootulp Sep 17, 2024

ninabarbakadze Sep 18, 2024

cmwaters Sep 19, 2024

cmwaters left a comment

cmwaters left a comment

cmwaters Sep 19, 2024

ninabarbakadze commented Sep 19, 2024

	Version: version.Consensus{App: appVersion},
	Version: version.Consensus{App: testApp.AppVersion()},

test: consistent app hash on v2 #3879

Are you sure you want to change the base?

test: consistent app hash on v2 #3879

Conversation

ninabarbakadze commented Sep 16, 2024 • edited Loading

Overview

coderabbitai bot commented Sep 17, 2024 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Possibly related PRs

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

rootulp Sep 17, 2024

Choose a reason for hiding this comment

ninabarbakadze Sep 18, 2024

Choose a reason for hiding this comment

cmwaters Sep 19, 2024

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

cmwaters Sep 19, 2024

Choose a reason for hiding this comment

ninabarbakadze commented Sep 19, 2024

ninabarbakadze commented Sep 16, 2024 •

edited

Loading

coderabbitai bot commented Sep 17, 2024 •

edited

Loading