test: consistent appHash between commits #3513
Conversation
ninabarbakadze
requested review from
staheri14 and
rach-id
and removed request for
a team
WalkthroughWalkthroughThe recent changes introduce new tests and modifications aimed at enhancing the non-determinism testing between different versions of the application. Key updates include the addition of functions for generating deterministic namespaces and keyrings, adjustments to test setups to include application versions, and new methods for managing genesis settings. These changes aim to ensure the consistency of application hashes and to prepare for handling potential non-determinism issues. Changes
Sequence Diagram(s)N/A Assessment against linked issues
Possibly related issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Additional comments not posted (6)
Warning Review ran into problemsProblems (1)
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| appVersion uint64, | ||
| ) [][]byte { | ||
| t.Helper() | ||
| txs := make([][]byte, len(accounts)) | ||
| opts := DefaultTxOpts() | ||
| for i, acc := range accounts { | ||
| signer, err := user.NewSigner(kr, enc, chainid, appconsts.LatestVersion, user.NewAccount(acc, accInfos[i].AccountNum, accInfos[i].Sequence)) | ||
| signer, err := user.NewSigner(kr, enc, chainid, appVersion, user.NewAccount(acc, accInfos[i].AccountNum, accInfos[i].Sequence)) |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification of the ManyMultiBlobTx function calls shows that not all instances include the appVersion parameter. Specifically, the calls in app/test/process_proposal_test.go and app/test/prepare_proposal_test.go are missing the appVersion parameter.
app/test/process_proposal_test.goapp/test/prepare_proposal_test.go
These instances need to be updated to include the appVersion parameter to ensure consistency and correctness.
Analysis chain
The addition of the appVersion parameter in ManyMultiBlobTx ensures that the blob transactions generated by this function are compatible with the specified app version, enhancing the accuracy and relevance of the tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `ManyMultiBlobTx` include the `appVersion` parameter.
# Test: Search for the function usage. Expect: Only occurances with the `appVersion` parameter.
rg --type go $'ManyMultiBlobTx'
Length of output: 517
Script:
#!/bin/bash
# Extract lines around the calls to ManyMultiBlobTx to verify the presence of the appVersion parameter.
# Extract lines from app/test/process_proposal_test.go
rg --context 5 'ManyMultiBlobTx' app/test/process_proposal_test.go
# Extract lines from app/test/non_determinism_test.go
rg --context 5 'ManyMultiBlobTx' app/test/non_determinism_test.go
# Extract lines from app/test/prepare_proposal_test.go
rg --context 5 'ManyMultiBlobTx' app/test/prepare_proposal_test.go
Length of output: 1920
app/test/non_determinism_test.go
Outdated
| numBlobTxs, numNormalTxs = 5, 5 | ||
| ) | ||
|
|
||
| expectedAppHash := []byte{100, 237, 125, 126, 116, 10, 189, 82, 156, 116, 176, 136, 169, 92, 185, 12, 72, 134, 254, 175, 234, 13, 159, 90, 139, 192, 190, 248, 67, 9, 32, 217} |
There was a problem hiding this comment.
[nit][optional since we have this a lot in the repo]
how was this hash generated? would be nice to have a comment where this value came from and how to generate it
There was a problem hiding this comment.
good point i'll link the executable from v1.x in the comments
There was a problem hiding this comment.
I'll link the file once this pr is merged as a follow-up #3522
ninabarbakadze
changed the title
I agree but I think first merge this with the basic structure and then we can further elaborate |
There was a problem hiding this comment.
I would have constructed this test a little differently. I would use a single user.Signer that has the keyring with all accounts. Then I would have something like.
txs := []struct {
sdkMsgs []sdk.Msg
author string
blobs []blob.Blob
txOptions []user.TxOption
}{
// add a mixture of blob txs and non blob txs with various signers
}
// you may want to break this out over multiple blocks
txBytes := make([]byte, len(txs))
for idx, tx := range txs {
if len(tx.sdkMsgs) > 0 {
txBytes[idx] = signer.CreateTx(tx.sdkMsgs, tx.txOptions)
} else {
txBytes[idx] = signer.CreatePayForBlob(tx.author, tx.blobs, tx.txOptions)
}
}
// run prepare proposal and process proposal (make sure data hashes are deterministic)
// run `BeginBlock`, `DeliverTx`, and `EndBlock` with the txBytes
// run `Commit` and get the app hash. Compare with the static one
app/test/non_determinism_test.go
Outdated
|
|
||
| // TestNonDeterminismBetweenMainAndV1 executes a set of different transactions, | ||
| // produces an app hash and compares it with the app hash produced by v1.x | ||
| func TestNonDeterminismBetweenMainAndV1(t *testing.T) { |
There was a problem hiding this comment.
This shouldn't necessary be between v1 and main. It should be comparing the app hash generated from this commit compared to the app hash generated from the previous commit
There was a problem hiding this comment.
agree that I think we can rename this function since the commitment should be the same across all branches if I'm understanding this comment correctly.
There was a problem hiding this comment.
[optional] slight tangent: the test could be renamed something like TestConsistentAppHash because it is asserting some expected app hash matches some calculated app hash. I think the goal is to identify cases of non-determinism but just because this test passes, it doesn't mean we prevented all potential cases of non-determinism.
There was a problem hiding this comment.
i agree with all of your points, i only wanted to underline that the hardcoded apphash was generated on v1.x because technically we could do it between any branches but practically the logic for generating apphash lives on v1.x.
test/util/testnode/utils.go
Outdated
| kr, addresses := NewKeyring(accounts...) | ||
| genAccounts := make([]authtypes.GenesisAccount, len(accounts)) | ||
| genBalances := make([]banktypes.Balance, len(accounts)) | ||
| func FundKeyringAccounts(addresses []sdk.AccAddress) ([]banktypes.Balance, []authtypes.GenesisAccount) { |
There was a problem hiding this comment.
[informational] note this modification is technically Go API breaking. We technically don't need to preserve Go API compatability because this is targeting main so FYI but no change needed
There was a problem hiding this comment.
good catch, do we need test! as a commit prefix?
There was a problem hiding this comment.
I would prefer if we add this as a completely new function called FundAddresses since it's quite a different function
There was a problem hiding this comment.
I'll revert this change completely. I thought that decoupling those functions made sense but we don't really use them elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
app/test/consistent_apphash_test.go (1)
38-134: Consider adding more detailed comments explaining the test steps inTestConsistentAppHash.While the function is well-structured, adding comments explaining each block of logic (e.g., keyring setup, transaction creation, block simulation) would enhance readability and maintainability, especially for new contributors.
test/util/test_app.go
Outdated
| } | ||
|
|
||
| // ApplyGenesisState sets genesis on initialized testApp with the provided arguments. | ||
| func ApplyGenesisState(testApp *app.App, pubKeys []cryptotypes.PubKey, balance int64, cparams *tmproto.ConsensusParams) (keyring.Keyring, []genesis.Account, error) { |
There was a problem hiding this comment.
why not?
| func ApplyGenesisState(testApp *app.App, pubKeys []cryptotypes.PubKey, balance int64, cparams *tmproto.ConsensusParams) (keyring.Keyring, []genesis.Account, error) { | |
| func ApplyGenesisState(testApp *app.App, gen *genesis.Genesis) error { |
The way you currently have it is not applying genesis but creating a genesis.
There was a problem hiding this comment.
preferably i'd have a function that lets you customise genesis without having to do the setup yourself. i'm happy to change the function signature to correctly reflect the behaviour. we also have a ticket refactoring test_app so we'll spend more time properly designing this process.
There was a problem hiding this comment.
technically this method is applying genesis on already initialised testapp
| func AddDeterministicValidatorToGenesis(g *genesis.Genesis) error { | ||
| // hardcoded keys for deterministic account creation | ||
| mnemo := "body world north giggle crop reduce height copper damp next verify orphan lens loan adjust inform utility theory now ranch motion opinion crowd fun" | ||
| consensusKey := ed25519.GenPrivKeyFromSecret([]byte("12345678901234567890123456389012")) | ||
| networkKey := ed25519.GenPrivKeyFromSecret([]byte("12345678901234567890123456786012")) |
There was a problem hiding this comment.
If you look at test/util/common.go, they already have a bunch of keys set up that you can use
There was a problem hiding this comment.
I don't think those keys are deterministic
test/util/testnode/utils.go
Outdated
| kr, addresses := NewKeyring(accounts...) | ||
| genAccounts := make([]authtypes.GenesisAccount, len(accounts)) | ||
| genBalances := make([]banktypes.Balance, len(accounts)) | ||
| func FundKeyringAccounts(addresses []sdk.AccAddress) ([]banktypes.Balance, []authtypes.GenesisAccount) { |
There was a problem hiding this comment.
I would prefer if we add this as a completely new function called FundAddresses since it's quite a different function
Co-authored-by: Rootul P <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
test/util/test_app.go (1)
208-248: Clarify the use of hardcoded secrets inAddDeterministicValidatorToGenesis.While the use of hardcoded secrets is suitable for deterministic testing environments, it's important to clearly document this to avoid confusion or misuse. Consider adding a comment explaining that these secrets are intended only for testing.
| // deterministicKeyRing returns a deterministic keyring and a list of deterministic public keys | ||
| func deterministicKeyRing(cdc codec.Codec) (keyring.Keyring, []types.PubKey) { | ||
| mnemonics := []string{ | ||
| "great myself congress genuine scale muscle view uncover pipe miracle sausage broccoli lonely swap table foam brand turtle comic gorilla firm mad grunt hazard", | ||
| "cheap job month trigger flush cactus chest juice dolphin people limit crunch curious secret object beach shield snake hunt group sketch cousin puppy fox", | ||
| "oil suffer bamboo one better attack exist dolphin relief enforce cat asset raccoon lava regret found love certain plunge grocery accuse goat together kiss", | ||
| "giraffe busy subject doll jump drama sea daring again club spend toe mind organ real liar permit refuse change opinion donkey job cricket speed", | ||
| "fee vapor thing fish fan memory negative raven cram win quantum ozone job mirror shoot sting quiz black apart funny sort cancel friend curtain", | ||
| "skin beef review pilot tooth act any alarm there only kick uniform ticket material cereal radar ethics list unlock method coral smooth street frequent", | ||
| "ecology scout core guard load oil school effort near alcohol fancy save cereal owner enforce impact sand husband trophy solve amount fish festival sell", | ||
| "used describe angle twin amateur pyramid bitter pool fluid wing erode rival wife federal curious drink battle put elbow mandate another token reveal tone", | ||
| "reason fork target chimney lift typical fine divorce mixture web robot kiwi traffic stove miss crane welcome camp bless fuel october riot pluck ordinary", | ||
| "undo logic mobile modify master force donor rose crumble forget plate job canal waste turn damp sure point deposit hazard quantum car annual churn", | ||
| "charge subway treat loop donate place loan want grief leg message siren joy road exclude match empty enforce vote meadow enlist vintage wool involve", | ||
| } | ||
| kb := keyring.NewInMemory(cdc) | ||
| pubKeys := make([]types.PubKey, len(mnemonics)) | ||
| for idx, mnemonic := range mnemonics { | ||
| rec, err := kb.NewAccount(fmt.Sprintf("account-%d", idx), mnemonic, "", "", hd.Secp256k1) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| pubKey, err := rec.GetPubKey() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| pubKeys[idx] = pubKey | ||
| } | ||
| return kb, pubKeys | ||
| } |
There was a problem hiding this comment.
Replace panic with proper error handling in deterministicKeyRing.
Using panic for error handling in test setups can lead to abrupt test terminations that are hard to debug. Consider returning an error from the function and using require.NoError in the test to handle these errors gracefully.
- panic(err)
+ return nil, errCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // deterministicKeyRing returns a deterministic keyring and a list of deterministic public keys | |
| func deterministicKeyRing(cdc codec.Codec) (keyring.Keyring, []types.PubKey) { | |
| mnemonics := []string{ | |
| "great myself congress genuine scale muscle view uncover pipe miracle sausage broccoli lonely swap table foam brand turtle comic gorilla firm mad grunt hazard", | |
| "cheap job month trigger flush cactus chest juice dolphin people limit crunch curious secret object beach shield snake hunt group sketch cousin puppy fox", | |
| "oil suffer bamboo one better attack exist dolphin relief enforce cat asset raccoon lava regret found love certain plunge grocery accuse goat together kiss", | |
| "giraffe busy subject doll jump drama sea daring again club spend toe mind organ real liar permit refuse change opinion donkey job cricket speed", | |
| "fee vapor thing fish fan memory negative raven cram win quantum ozone job mirror shoot sting quiz black apart funny sort cancel friend curtain", | |
| "skin beef review pilot tooth act any alarm there only kick uniform ticket material cereal radar ethics list unlock method coral smooth street frequent", | |
| "ecology scout core guard load oil school effort near alcohol fancy save cereal owner enforce impact sand husband trophy solve amount fish festival sell", | |
| "used describe angle twin amateur pyramid bitter pool fluid wing erode rival wife federal curious drink battle put elbow mandate another token reveal tone", | |
| "reason fork target chimney lift typical fine divorce mixture web robot kiwi traffic stove miss crane welcome camp bless fuel october riot pluck ordinary", | |
| "undo logic mobile modify master force donor rose crumble forget plate job canal waste turn damp sure point deposit hazard quantum car annual churn", | |
| "charge subway treat loop donate place loan want grief leg message siren joy road exclude match empty enforce vote meadow enlist vintage wool involve", | |
| } | |
| kb := keyring.NewInMemory(cdc) | |
| pubKeys := make([]types.PubKey, len(mnemonics)) | |
| for idx, mnemonic := range mnemonics { | |
| rec, err := kb.NewAccount(fmt.Sprintf("account-%d", idx), mnemonic, "", "", hd.Secp256k1) | |
| if err != nil { | |
| panic(err) | |
| } | |
| pubKey, err := rec.GetPubKey() | |
| if err != nil { | |
| panic(err) | |
| } | |
| pubKeys[idx] = pubKey | |
| } | |
| return kb, pubKeys | |
| } | |
| // deterministicKeyRing returns a deterministic keyring and a list of deterministic public keys | |
| func deterministicKeyRing(cdc codec.Codec) (keyring.Keyring, []types.PubKey, error) { | |
| mnemonics := []string{ | |
| "great myself congress genuine scale muscle view uncover pipe miracle sausage broccoli lonely swap table foam brand turtle comic gorilla firm mad grunt hazard", | |
| "cheap job month trigger flush cactus chest juice dolphin people limit crunch curious secret object beach shield snake hunt group sketch cousin puppy fox", | |
| "oil suffer bamboo one better attack exist dolphin relief enforce cat asset raccoon lava regret found love certain plunge grocery accuse goat together kiss", | |
| "giraffe busy subject doll jump drama sea daring again club spend toe mind organ real liar permit refuse change opinion donkey job cricket speed", | |
| "fee vapor thing fish fan memory negative raven cram win quantum ozone job mirror shoot sting quiz black apart funny sort cancel friend curtain", | |
| "skin beef review pilot tooth act any alarm there only kick uniform ticket material cereal radar ethics list unlock method coral smooth street frequent", | |
| "ecology scout core guard load oil school effort near alcohol fancy save cereal owner enforce impact sand husband trophy solve amount fish festival sell", | |
| "used describe angle twin amateur pyramid bitter pool fluid wing erode rival wife federal curious drink battle put elbow mandate another token reveal tone", | |
| "reason fork target chimney lift typical fine divorce mixture web robot kiwi traffic stove miss crane welcome camp bless fuel october riot pluck ordinary", | |
| "undo logic mobile modify master force donor rose crumble forget plate job canal waste turn damp sure point deposit hazard quantum car annual churn", | |
| "charge subway treat loop donate place loan want grief leg message siren joy road exclude match empty enforce vote meadow enlist vintage wool involve", | |
| } | |
| kb := keyring.NewInMemory(cdc) | |
| pubKeys := make([]types.PubKey, len(mnemonics)) | |
| for idx, mnemonic := range mnemonics { | |
| rec, err := kb.NewAccount(fmt.Sprintf("account-%d", idx), mnemonic, "", "", hd.Secp256k1) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| pubKey, err := rec.GetPubKey() | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| pubKeys[idx] = pubKey | |
| } | |
| return kb, pubKeys, nil | |
| } |
Overview
Fixes #2414
This test is split up in a few parts:
Verifying the AppHash:
If you want to make sure that the expected AppHash is correct, you can run the non determinism test on v1.x that generates it.
Follow-ups:
#3509
#3540