feat!: add chainbuilder tool #3886
Conversation
cmwaters
requested review from
rootulp and
evan-forbes
and removed request for
a team
WalkthroughWalkthroughThe pull request introduces changes primarily focused on updating the Changes
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
tools/chainbuilder/README.md (3)
13-13: Consider using "afterward" instead of "afterwards".In American English, "afterward" is the preferred variant. "Afterwards" is more commonly used in British English and other dialects.
-This will create a directory with the name `testnode-{chainID}`. All files will be populated and blocks generated based on specified input. You can run a validator on the file system afterwards by calling: +This will create a directory with the name `testnode-{chainID}`. All files will be populated and blocks generated based on specified input. You can run a validator on the file system afterward by calling:Tools
LanguageTool
[locale-violation] ~13-~13: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... can run a validator on the file system afterwards by calling: ``` celestia-appd start --...(AFTERWARDS_US)
25-25: Add a comma after "By default".Adding a comma after "By default" improves readability and adheres to common punctuation practices.
-`namespace` allows you to pick a custom v0 namespace. By default "test" will be chosen. +`namespace` allows you to pick a custom v0 namespace. By default, "test" will be chosen.Tools
LanguageTool
[uncategorized] ~25-~25: Did you mean: “By default,”?
Context: ...lows you to pick a custom v0 namespace. By default "test" will be chosen. This tool takes...(BY_DEFAULT_COMMA)
29-29: Remove the extra blank line at the end of the file.Multiple consecutive blank lines are unnecessary and do not adhere to common Markdown formatting practices. Remove the extra blank line to improve the file's formatting.
- -Tools
Markdownlint
29-29: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (10)
- go.mod (5 hunks)
- pkg/user/account.go (1 hunks)
- test/util/genesis/document.go (2 hunks)
- test/util/genesis/files.go (2 hunks)
- test/util/genesis/genesis.go (3 hunks)
- test/util/testnode/network.go (2 hunks)
- tools/chainbuilder/README.md (1 hunks)
- tools/chainbuilder/benchmark_test.go (1 hunks)
- tools/chainbuilder/integration_test.go (1 hunks)
- tools/chainbuilder/main.go (1 hunks)
Additional context used
LanguageTool
tools/chainbuilder/README.md
[locale-violation] ~13-~13: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... can run a validator on the file system afterwards by calling: ``` celestia-appd start --...(AFTERWARDS_US)
[uncategorized] ~25-~25: Did you mean: “By default,”?
Context: ...lows you to pick a custom v0 namespace. By default "test" will be chosen. This tool takes...(BY_DEFAULT_COMMA)
Markdownlint
tools/chainbuilder/README.md
29-29: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
9-9: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
15-15: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
Additional comments not posted (27)
tools/chainbuilder/benchmark_test.go (1)
11-26: LGTM!The benchmark test for the
Runfunction is well-structured and follows the standard Go benchmarking conventions. It sets up a reasonable configuration for the builder, executes theRunfunction in a loop, and checks for errors. The use of a temporary directory for each benchmark run ensures isolation and prevents interference between runs.This benchmark will be useful for measuring the performance of the
Runfunction under different configurations and can help identify performance bottlenecks if needed.tools/chainbuilder/README.md (1)
1-12: LGTM!The introduction and usage example are clear and well-written. The
go runcommand is the correct way to execute the tool.Tools
Markdownlint
9-9: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
test/util/genesis/files.go (5)
8-8: LGTM!The import statement for the
srvconfigpackage is valid and follows the standard naming convention.
21-74: Excellent work on refactoring theInitFilesfunction!The changes made to the function signature, return type, and internal logic significantly improve the function's interface and functionality. Here are the key improvements:
The addition of the
appCfgparameter allows the function to handle application configuration alongside the existing parameters, enhancing its role in the initialization process.Changing the return type to a single
errorsimplifies the error handling mechanism and makes the code more readable.Setting the root directory for
tmConfigusing the providedrootDirstreamlines the logic and eliminates the need for constructing a separatebasePath.Creating the configuration directory using
rootDiris consistent with the changes made to the function signature and improves code clarity.Writing configuration files for both the application (
app.toml) and Tendermint (config.toml) enhances the function's responsibility in setting up the necessary configuration files.Directly returning errors improves error handling and code readability.
Overall, these changes make the
InitFilesfunction more efficient, maintainable, and easier to use. Great job!
27-27: Good use offmt.Errorffor formatting the error message.Using
fmt.Errorfis a better practice compared to string concatenation when formatting error messages. The error message is clear and includes the relevant information about the missing validator index.
40-40: Excellent error wrapping usingfmt.Errorfand the%wverb.Wrapping the original error with
fmt.Errorfand the%wverb is a best practice in Go error handling. It allows the error to be propagated up the call stack while providing additional context about the operation that failed. The error message clearly indicates that the error occurred during the exporting of genesis and includes the original error.
69-72: Great addition of writing application and Tendermint configuration files.Writing the application configuration to a separate file (
app.toml) in theconfigdirectory is a good practice for maintaining a clear configuration structure and separating concerns. Using thesrvconfig.WriteConfigFilefunction ensures that the application configuration is written in the correct format and location.Similarly, writing the Tendermint configuration to
config.tomlusing theconfig.WriteConfigFilefunction is consistent with the Tendermint configuration management practices and ensures proper configuration setup.These additions enhance the
InitFilesfunction's responsibility in setting up the necessary configuration files for the application and Tendermint.pkg/user/account.go (1)
43-45: LGTM!The
AccountNumbermethod is a simple and straightforward addition to theAccountstruct. It provides a convenient way to access theaccountNumberfield without introducing any breaking changes or compatibility issues.The method is correctly defined as a value receiver and is exported, which aligns with the PR objectives of enhancing the functionality of the
Accounttype.No further changes are required.
test/util/testnode/network.go (2)
5-5: LGTM!The import statement for the
filepathpackage is valid and correctly formatted. This package is a standard Go library used for file path manipulation and is likely needed for the changes made in theNewNetworkfunction.
23-24: Looks good!The changes made in this code segment improve the organization and clarity of the test setup process:
The
baseDirpath is constructed by joining the temporary directory created for the test with a subdirectory named "testnode". This enhances the structure of generated files by specifying a dedicated location for the test node.The update to the
genesis.InitFilesfunction call, which now includesconfig.AppConfigas an argument, aligns with the updated requirements for initializing the genesis and validator files. This change suggests that the application configuration is now being handled during the initialization process.Overall, these modifications contribute to a cleaner and more organized test setup.
tools/chainbuilder/integration_test.go (1)
28-103: Excellent integration test for the chainbuilder tool!The
TestRunfunction provides comprehensive coverage for the chainbuilder tool's functionality and integration with the Celestia application. Here are some strengths of the test:
- Skips execution in short mode, which is a good practice for resource-intensive tests.
- Uses a reasonable number of blocks (10) for the chainbuilder configuration.
- Handles both first run and second run (with existing directory) scenarios.
- Properly sets up the Tendermint configuration, application database, and application instance.
- Loads the node key and creates a new Tendermint node, ensuring that it starts without errors.
- Creates a local client and verifies that the node's status is not nil.
- Asserts that the new node makes progress in the blockchain by checking the latest block height.
- Stops the node gracefully using
defer, ensuring proper cleanup.- Uses
requirefor assertions, which is a good practice for tests.- Uses
require.Eventuallyfor retrying the block height assertion, handling asynchronous behavior.- Uses reasonable timeouts and retry intervals for the
require.Eventuallycall.Overall, this test helps ensure that the chainbuilder tool can successfully create and manage a blockchain node, and it helps catch potential regressions or issues in the tool's behavior.
test/util/genesis/document.go (2)
77-77: LGTM!The change in the
GenesisTimefield assignment is correct and consistent with the new function signature.
27-27: Verify the function signature change in the codebase.The addition of the
genesisTimeparameter is a good change that enhances the flexibility of the function by allowing for the specification of a custom genesis time.Run the following script to verify the function usage:
Verification successful
Function signature change verified successfully
The
Documentfunction call intest/util/genesis/genesis.gohas been correctly updated to include the newgenesisTimeparameter. Theg.GenesisTimeargument is passed as the sixth parameter, aligning with the updated function signature. This confirms that the change has been consistently applied across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `Document` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'Document\('Length of output: 612
Script:
#!/bin/bash # Description: View the complete function call to Document in genesis.go # Search for the Document function call with more context rg --type go -C 10 'return Document\(' test/util/genesis/genesis.goLength of output: 343
test/util/genesis/genesis.go (3)
123-126: LGTM!The new
WithKeyringmethod provides a convenient way to set the keyring of theGenesisstruct directly, enhancing the flexibility of theGenesisconfiguration. The implementation looks good.
230-232: LGTM!The new
EncodingConfigmethod provides a convenient way to access the encoding configuration associated with theGenesisinstance. This addition may facilitate better handling of encoding-related tasks within the genesis context. The implementation looks good.
216-216: Verify the impact of the change in the exported state.The
Exportmethod now includesg.GenesisTimein its return values, which suggests a change in the exported state of theGenesisobject. This may affect how the genesis data is serialized or utilized downstream.Please review and ensure that this change is intended and handled correctly by the consumers of this method.
Run the following script to verify the usage of the
Exportmethod in the codebase and analyze the impact:Verification successful
Verification successful: GenesisTime inclusion is correctly implemented
The
Exportmethod now correctly includesg.GenesisTimein its return values by passing it to theDocumentfunction. This change appears to be intentional and properly implemented.The impact on other parts of the codebase seems minimal based on the usage patterns of the
Exportmethod. However, to ensure full compatibility, it's recommended to:
- Thoroughly test the components that use the
Exportmethod, particularly in:
- tools/chainbuilder/main.go
- test/util/test_app.go
- test/util/genesis/files.go
- test/e2e/testnet/testnet.go
- app/test/upgrade_test.go
Ensure that these components correctly handle the updated genesis document that now includes the genesis time.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `Export` method and analyze the impact of the change in the exported state. # Test: Search for the method usage. Expect: Occurrences of the method with the correct handling of the returned genesis time. rg --type go -A 10 $'\\(g \\*Genesis\\) Export\\('Length of output: 697
Script:
#!/bin/bash # Description: View the complete implementation of the Export method and search for its usages # View the complete Export method implementation echo "Complete Export method implementation:" rg --type go -A 30 '^\s*func \(g \*Genesis\) Export\(' test/util/genesis/genesis.go echo "\nUsages of the Export method:" # Search for usages of the Export method in the entire codebase rg --type go '\.Export\(\)'Length of output: 1324
go.mod (5)
14-14: Verify the usage and compatibility of the newcometbft-dbdependency.Please ensure that:
- The
github.com/cometbft/cometbft-dbpackage is actually used in the codebase to justify its addition.- This new dependency doesn't introduce any conflicts or compatibility issues with existing dependencies.
63-63: Verify the impact of updatingbackoffto v4.2.1.Please ensure that:
- The update to
github.com/cenkalti/backoff/v4 v4.2.1doesn't introduce any breaking changes in the codebase.- All usages of the
backoffpackage are compatible with the new version.
73-73: Clarify the purpose and necessity of the new Docker related dependencies.Several Docker related dependencies have been added:
github.com/containerd/continuitygit.luolix.top/distribution/referencegit.luolix.top/docker/dockergit.luolix.top/docker/go-connectionsgit.luolix.top/docker/go-unitsPlease provide clarification on:
- Why were these dependencies introduced? What functionality do they provide?
- Are all of these dependencies necessary? Can any of them be removed?
- The
+incompatiblesuffix ondocker/dockerversion suggests potential compatibility issues. Has this been checked and resolved?Also applies to: 89-92
178-179: Clarify the purpose and necessity of the newopencontainersrelated dependencies.Two
opencontainersrelated dependencies have been added:
github.com/opencontainers/go-digestgit.luolix.top/opencontainers/image-specPlease provide clarification on:
- Why were these dependencies introduced? What functionality do they provide?
- Are both of these dependencies necessary? Can any of them be removed?
211-211: Verify the impact of updatingbboltto v1.3.10.Please ensure that:
- The update to
go.etcd.io/bbolt v1.3.10doesn't introduce any breaking changes in the codebase.- All usages of the
bboltpackage are compatible with the new version.tools/chainbuilder/main.go (6)
47-98: LGTM!The
mainfunction sets up the CLI interface correctly using Cobra. The flags are defined with appropriate default values and descriptions. TheRunfunction is invoked with the correct arguments.
110-413: The core logic of theRunfunction is implemented correctly.The function correctly distinguishes between creating a new chain and extending an existing one. It sets up the necessary configurations, databases, and stores based on the provided arguments. The block generation logic is implemented correctly, with appropriate error handling. The usage of goroutines for concurrent processing is a good practice.
415-475: LGTM!The
generateSquareRoutinefunction correctly generates a blob, creates a transaction, builds a data square, and sends it to thedataChchannel. The logic is implemented correctly with appropriate error handling.
483-508: LGTM!The
persistDataRoutinefunction correctly saves the state and blocks to the database. It receives data from thedataChchannel, persists it using thestateStoreandblockStore, and logs progress messages appropriately.
100-108: LGTM!The
BuilderConfigstruct includes all the necessary fields to configure the chain builder. The field names are descriptive and follow the Go naming conventions. The field types are appropriate for their respective values.
477-481: LGTM!The
persistDatastruct includes all the necessary fields to persist the chain data. The field names are descriptive and follow the Go naming conventions. The field types are appropriate for their respective values.
tools/chainbuilder/main.go
Outdated
| ValidatorAddress: validatorAddr, | ||
| Timestamp: currentTime, | ||
| Signature: precommitVote.Signature, | ||
| } | ||
| commit = types.NewCommit(height, 0, blockID, []types.CommitSig{commitSig}) | ||
|
|
||
| var lastCommitInfo abci.LastCommitInfo | ||
| if height > 1 { | ||
| lastCommitInfo = abci.LastCommitInfo{ | ||
| Round: 0, | ||
| Votes: []abci.VoteInfo{ | ||
| { | ||
| Validator: abci.Validator{ | ||
| Address: validatorAddr, | ||
| Power: validatorPower, | ||
| }, | ||
| SignedLastBlock: true, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| beginBlockResp := simApp.BeginBlock(abci.RequestBeginBlock{ | ||
| Hash: block.Hash(), | ||
| Header: *block.Header.ToProto(), | ||
| LastCommitInfo: lastCommitInfo, | ||
| }) | ||
|
|
||
| deliverTxResponses := make([]*abci.ResponseDeliverTx, len(block.Data.Txs)) | ||
|
|
||
| for idx, tx := range block.Data.Txs { | ||
| blobTx, isBlobTx := types.UnmarshalBlobTx(tx) | ||
| if isBlobTx { | ||
| tx = blobTx.Tx | ||
| } | ||
| deliverTxResponse := simApp.DeliverTx(abci.RequestDeliverTx{ | ||
| Tx: tx, | ||
| }) | ||
| if deliverTxResponse.Code != abci.CodeTypeOK { | ||
| return fmt.Errorf("failed to deliver tx: %s", deliverTxResponse.Log) | ||
| } | ||
| deliverTxResponses[idx] = &deliverTxResponse | ||
| } | ||
|
|
||
| endBlockResp := simApp.EndBlock(abci.RequestEndBlock{ | ||
| Height: block.Height, | ||
| }) | ||
|
|
||
| commitResp := simApp.Commit() | ||
| state.LastBlockHeight = height | ||
| state.LastBlockID = blockID | ||
| state.LastBlockTime = block.Time | ||
| state.LastValidators = state.Validators | ||
| state.Validators = state.NextValidators | ||
| state.NextValidators = state.NextValidators.CopyIncrementProposerPriority(1) | ||
| state.AppHash = commitResp.Data | ||
| state.LastResultsHash = sm.ABCIResponsesResultsHash(&smproto.ABCIResponses{ | ||
| DeliverTxs: deliverTxResponses, | ||
| BeginBlock: &beginBlockResp, | ||
| EndBlock: &endBlockResp, | ||
| }) | ||
| currentTime = currentTime.Add(cfg.BlockInterval) | ||
| persistCh <- persistData{ | ||
| state: state.Copy(), | ||
| block: block, | ||
| seenCommit: &types.Commit{ | ||
| Height: commit.Height, | ||
| Round: commit.Round, | ||
| BlockID: commit.BlockID, | ||
| Signatures: []types.CommitSig{commitSig}, | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| close(dataCh) | ||
| close(persistCh) | ||
|
|
||
| var firstErr error | ||
| for i := 0; i < cap(errCh); i++ { | ||
| err := <-errCh | ||
| if err != nil && firstErr == nil { | ||
| firstErr = err | ||
| } | ||
| } | ||
|
|
||
| if err := blockDB.Close(); err != nil { | ||
| return fmt.Errorf("failed to close block database: %w", err) | ||
| } | ||
| if err := stateDB.Close(); err != nil { | ||
| return fmt.Errorf("failed to close state database: %w", err) | ||
| } | ||
| if err := appDB.Close(); err != nil { | ||
| return fmt.Errorf("failed to close application database: %w", err) | ||
| } | ||
|
|
||
| fmt.Println("Chain built successfully", state.LastBlockHeight) | ||
|
|
||
| return firstErr | ||
| } |
There was a problem hiding this comment.
Consider refactoring the Run function into smaller, more focused functions.
The Run function is quite large and handles multiple responsibilities. Consider breaking it down into smaller functions that each handle a specific task, such as setting up the configuration, initializing the genesis state, generating blocks, and persisting data. This will improve the readability and maintainability of the code.
celestia-bot
requested review from
a team,
staheri14 and
ninabarbakadze
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
tools/chainbuilder/README.md (2)
9-11: Specify the language for fenced code blocks.For better syntax highlighting and clarity, please specify the language for the fenced code blocks.
Apply this diff to specify the language:
-``` +```bash go run ./tools/chainbuilder```diff -``` +```bash celestia-appd start --home /path/to/testnode-{chainID}Also applies to: 15-17 <details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 9-9: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> --- `13-13`: **Fix minor grammar issues.** A couple of minor grammar improvements: - In American English, "afterward" is the preferred variant instead of "afterwards". - Add a comma after "By default". Apply this diff to fix the grammar issues: ```diff -This will create a directory with the name `testnode-{chainID}`. All files will be populated and blocks generated based on specified input. You can run a validator on the file system afterwards by calling: +This will create a directory with the name `testnode-{chainID}`. All files will be populated and blocks generated based on specified input. You can run a validator on the file system afterward by calling:-- `namespace` allows you to pick a custom v0 namespace. By default "test" will be chosen. +- `namespace` allows you to pick a custom v0 namespace. By default, "test" will be chosen.Also applies to: 25-25
Tools
LanguageTool
[locale-violation] ~13-~13: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... can run a validator on the file system afterwards by calling: ``` celestia-appd start --...(AFTERWARDS_US)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- tools/chainbuilder/README.md (1 hunks)
- tools/chainbuilder/integration_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/chainbuilder/integration_test.go
Additional context used
LanguageTool
tools/chainbuilder/README.md
[locale-violation] ~13-~13: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... can run a validator on the file system afterwards by calling: ``` celestia-appd start --...(AFTERWARDS_US)
[uncategorized] ~25-~25: Did you mean: “By default,”?
Context: ...lows you to pick a custom v0 namespace. By default "test" will be chosen. This tool takes...(BY_DEFAULT_COMMA)
Markdownlint
tools/chainbuilder/README.md
9-9: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
15-15: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
Additional comments not posted (1)
tools/chainbuilder/README.md (1)
1-27: LGTM!Overall, the README.md file provides a clear and informative documentation for the
chainbuildertool. It covers the usage, options, and performance aspects of the tool. With the minor improvements suggested above, the documentation will be even better.Tools
LanguageTool
[locale-violation] ~13-~13: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... can run a validator on the file system afterwards by calling: ``` celestia-appd start --...(AFTERWARDS_US)
[uncategorized] ~25-~25: Did you mean: “By default,”?
Context: ...lows you to pick a custom v0 namespace. By default "test" will be chosen. This tool takes...(BY_DEFAULT_COMMA)
Markdownlint
9-9: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
15-15: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
| This will create a directory with the name `testnode-{chainID}`. All files will be populated and blocks generated based on specified input. You can run a validator on the file system afterwards by calling: | ||
|
|
||
| ``` | ||
| celestia-appd start --home /path/to/testnode-{chainID} |
There was a problem hiding this comment.
[nit] It might be helpful to mention how the pruning option may need adjustment. When I ran this and pointed it to the generated directory by chainbuilder, I encountered this error:
celestia-appd start --home ./testnode-1YgtBH --force-no-bbr
Error: cannot enable state sync snapshots with 'everything' pruning setting: error in app.toml
I had to modify the pruning setting in app.toml to resolve the error. Up to you, but it might be worth mentioning in the documentation.
cmwaters
dismissed stale reviews from evan-forbes, staheri14, and rootulp
via
8bfa934
celestia-bot
requested review from
a team and
rach-id
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (1)
- tools/chainbuilder/main.go (1 hunks)
Additional context used
GitHub Check: lint / golangci-lint
tools/chainbuilder/main.go
[failure] 291-291:
S1038: should use fmt.Printf instead of fmt.Println(fmt.Sprintf(...)) (but don't forget the newline) (gosimple)
| validatorPower := state.Validators.Validators[0].VotingPower | ||
|
|
There was a problem hiding this comment.
Ensure Validators slice is not empty before accessing
The code assumes that state.Validators.Validators has at least one validator. Accessing index [0] without checking could cause a panic if the slice is empty. Consider adding a check to ensure the slice is not empty before accessing.
Add a check for empty validators:
+ if len(state.Validators.Validators) == 0 {
+ return fmt.Errorf("no validators found in state")
+ }
validatorPower := state.Validators.Validators[0].VotingPowerCommittable 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.
| validatorPower := state.Validators.Validators[0].VotingPower | |
| if len(state.Validators.Validators) == 0 { | |
| return fmt.Errorf("no validators found in state") | |
| } | |
| validatorPower := state.Validators.Validators[0].VotingPower |
tools/chainbuilder/main.go
Outdated
| fmt.Println(fmt.Sprintf("blocks cannot be generated into the future, stopping at height %d", | ||
| lastBlock.Height)) |
There was a problem hiding this comment.
Simplify the print statement by using fmt.Printf directly
Instead of nesting fmt.Sprintf within fmt.Println, you can use fmt.Printf to format and print the string in a single call.
Apply this diff to simplify the code:
- fmt.Println(fmt.Sprintf("blocks cannot be generated into the future, stopping at height %d",
- lastBlock.Height))
+ fmt.Printf("blocks cannot be generated into the future, stopping at height %d\n",
+ lastBlock.Height)Committable 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.
| fmt.Println(fmt.Sprintf("blocks cannot be generated into the future, stopping at height %d", | |
| lastBlock.Height)) | |
| fmt.Printf("blocks cannot be generated into the future, stopping at height %d\n", | |
| lastBlock.Height) |
Tools
GitHub Check: lint / golangci-lint
[failure] 291-291:
S1038: should use fmt.Printf instead of fmt.Println(fmt.Sprintf(...)) (but don't forget the newline) (gosimple)
| fee := float64(blobGas) * appconsts.DefaultMinGasPrice * 2 | ||
| tx, _, err := signer.CreatePayForBlobs(account.Name(), []*share.Blob{blob}, user.SetGasLimit(blobGas), user.SetFee(uint64(fee))) |
There was a problem hiding this comment.
Avoid loss of precision in fee calculation
The fee is calculated using floating-point arithmetic and then cast to uint64, which may result in loss of precision. It's better to perform fee calculations using integer arithmetic to prevent rounding errors.
Modify the fee calculation to use integer arithmetic:
- fee := float64(blobGas) * appconsts.DefaultMinGasPrice * 2
+ fee := uint64(float64(blobGas) * appconsts.DefaultMinGasPrice * 2)Ensure that the fee remains accurate after the conversion.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
tools/chainbuilder/main.go (2)
433-438: Passcontext.Contextas the first parameterIn Go conventions, the
context.Contextparameter should be the first parameter in function signatures. This enhances readability and consistency across the codebase.Apply this diff to reorder the parameters:
-func generateSquareRoutine( - ctx context.Context, - signer *user.Signer, - cfg BuilderConfig, - dataCh chan<- *tmproto.Data, -) error { +func generateSquareRoutine( + signer *user.Signer, + cfg BuilderConfig, + dataCh chan<- *tmproto.Data, + ctx context.Context, +) error {Update the function calls accordingly.
495-499: Consider adding a comment to explain thepersistDatastructAdding a comment above the
persistDatastruct can improve code readability by explaining its purpose.Apply this diff to add a comment:
+// persistData holds the state, block, and seenCommit needed for persistence. type persistData struct { state sm.State block *types.Block seenCommit *types.Commit }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- tools/chainbuilder/benchmark_test.go (1 hunks)
- tools/chainbuilder/integration_test.go (1 hunks)
- tools/chainbuilder/main.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/chainbuilder/benchmark_test.go
- tools/chainbuilder/integration_test.go
Additional comments not posted (3)
tools/chainbuilder/main.go (3)
260-261: EnsureValidatorsslice is not empty before accessing
454-455: Avoid loss of precision in fee calculation
119-120: Ensure time calculations account for block intervals accuratelyThe calculation of
startTimesubtracts the total expected time span of blocks from the current time. However, ifcfg.BlockIntervalorcfg.NumBlocksare large, this could result instartTimebeing in the distant past or even negative. Consider validating the resultingstartTimeor setting a reasonable minimum to prevent potential issues.Run the following script to check if
startTimeis in a valid range:Verification successful
Time calculations properly account for block intervals
Based on the executed scripts,
startTimeremains within a valid range even with largecfg.BlockIntervalandcfg.NumBlocksvalues. No issues detected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that startTime is not before Unix epoch time. # Test: Check if startTime is after January 1, 1970. # Expect: Start time should be a valid recent timestamp. # Replace with actual values if available NUM_BLOCKS=100 BLOCK_INTERVAL_SECONDS=1 START_TIME=$(date -d "@$(($(date +%s) - $NUM_BLOCKS * $BLOCK_INTERVAL_SECONDS))" +%s) if [ $START_TIME -lt 0 ]; then echo "startTime is before Unix epoch time. Please validate the calculation." else echo "startTime is valid." fiLength of output: 183
Script:
#!/bin/bash # Description: Verify that startTime remains valid with large NUM_BLOCKS and BLOCK_INTERVAL_SECONDS. # Test: Check if startTime is after January 1, 1970, with large parameters. # Expect: startTime should still be a valid recent timestamp or within a reasonable range. # Set large values NUM_BLOCKS=1000000 BLOCK_INTERVAL_SECONDS=60 # Get current timestamp CURRENT_TIMESTAMP=$(date +%s) # Calculate start time START_TIME=$((CURRENT_TIMESTAMP - NUM_BLOCKS * BLOCK_INTERVAL_SECONDS)) # Check if start time is before Unix epoch if [ $START_TIME -lt 0 ]; then echo "startTime is before Unix epoch time. Please validate the calculation." else # Optionally, check if startTime is too far in the past # For example, not older than a certain threshold, say 10 years TEN_YEARS_SECONDS=$((10 * 365 * 24 * 60 * 60)) if [ $((CURRENT_TIMESTAMP - START_TIME)) -gt $TEN_YEARS_SECONDS ]; then echo "startTime is more than 10 years in the past. Please validate the calculation." else echo "startTime is valid." fi fiLength of output: 254
| return fmt.Errorf("failed to sign precommit vote (%s): %w", precommitVote.String(), err) | ||
| } |
There was a problem hiding this comment.
Avoid exposing sensitive information in error messages
The error message includes the string representation of precommitVote, which may contain sensitive information. It's safer to omit or sanitize such details in error logs to prevent potential information leaks.
Apply this diff to remove sensitive information from the error message:
- return fmt.Errorf("failed to sign precommit vote (%s): %w", precommitVote.String(), err)
+ return fmt.Errorf("failed to sign precommit vote: %w", err)Committable 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.
| return fmt.Errorf("failed to sign precommit vote (%s): %w", precommitVote.String(), err) | |
| } | |
| return fmt.Errorf("failed to sign precommit vote: %w", err) |
| fmt.Println("Creating chain from scratch with Chain ID:", gen.ChainID) | ||
| } else { | ||
| cfgPath := filepath.Join(cfg.ExistingDir, "config/config.toml") | ||
| if _, err := os.Stat(cfgPath); os.IsNotExist(err) { | ||
| return fmt.Errorf("config file for existing chain not found at %s", cfgPath) | ||
| } | ||
| fmt.Println("Loading chain from existing directory:", cfg.ExistingDir) | ||
| tmCfg.SetRoot(cfg.ExistingDir) |
There was a problem hiding this comment.
Consider using a structured logging library instead of fmt.Println
The code uses fmt.Println for logging informational messages. For better logging practices, consider using a structured logging library like Go's standard log package or third-party libraries such as logrus or zap. This allows for log levels, formatted output, and easier maintenance.
Example using the standard log package:
- fmt.Println("Creating chain from scratch with Chain ID:", gen.ChainID)
+ log.Printf("Creating chain from scratch with Chain ID: %s\n", gen.ChainID)Repeat this change for other fmt.Println calls to ensure consistent logging throughout the application.
Committable suggestion was skipped due to low confidence.
| numBlocks, _ := cmd.Flags().GetInt("num-blocks") | ||
| blockSize, _ := cmd.Flags().GetInt("block-size") | ||
| blockInterval, _ := cmd.Flags().GetDuration("block-interval") | ||
| existingDir, _ := cmd.Flags().GetString("existing-dir") | ||
| namespaceStr, _ := cmd.Flags().GetString("namespace") | ||
| upToTime, _ := cmd.Flags().GetBool("up-to-now") | ||
| appVersion, _ := cmd.Flags().GetUint64("app-version") |
There was a problem hiding this comment.
Handle errors when parsing command-line flags
The current implementation ignores errors returned by cmd.Flags().Get* methods. This could lead to unexpected behavior if a flag cannot be parsed correctly. It's important to check and handle these errors to ensure robustness.
Apply this diff to handle the errors:
- numBlocks, _ := cmd.Flags().GetInt("num-blocks")
- blockSize, _ := cmd.Flags().GetInt("block-size")
- blockInterval, _ := cmd.Flags().GetDuration("block-interval")
- existingDir, _ := cmd.Flags().GetString("existing-dir")
- namespaceStr, _ := cmd.Flags().GetString("namespace")
- upToTime, _ := cmd.Flags().GetBool("up-to-now")
- appVersion, _ := cmd.Flags().GetUint64("app-version")
+ numBlocks, err := cmd.Flags().GetInt("num-blocks")
+ if err != nil {
+ return fmt.Errorf("failed to get 'num-blocks' flag: %w", err)
+ }
+ blockSize, err := cmd.Flags().GetInt("block-size")
+ if err != nil {
+ return fmt.Errorf("failed to get 'block-size' flag: %w", err)
+ }
+ blockInterval, err := cmd.Flags().GetDuration("block-interval")
+ if err != nil {
+ return fmt.Errorf("failed to get 'block-interval' flag: %w", err)
+ }
+ existingDir, err := cmd.Flags().GetString("existing-dir")
+ if err != nil {
+ return fmt.Errorf("failed to get 'existing-dir' flag: %w", err)
+ }
+ namespaceStr, err := cmd.Flags().GetString("namespace")
+ if err != nil {
+ return fmt.Errorf("failed to get 'namespace' flag: %w", err)
+ }
+ upToTime, err := cmd.Flags().GetBool("up-to-now")
+ if err != nil {
+ return fmt.Errorf("failed to get 'up-to-now' flag: %w", err)
+ }
+ appVersion, err := cmd.Flags().GetUint64("app-version")
+ if err != nil {
+ return fmt.Errorf("failed to get 'app-version' flag: %w", err)
+ }Committable 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.
| numBlocks, _ := cmd.Flags().GetInt("num-blocks") | |
| blockSize, _ := cmd.Flags().GetInt("block-size") | |
| blockInterval, _ := cmd.Flags().GetDuration("block-interval") | |
| existingDir, _ := cmd.Flags().GetString("existing-dir") | |
| namespaceStr, _ := cmd.Flags().GetString("namespace") | |
| upToTime, _ := cmd.Flags().GetBool("up-to-now") | |
| appVersion, _ := cmd.Flags().GetUint64("app-version") | |
| numBlocks, err := cmd.Flags().GetInt("num-blocks") | |
| if err != nil { | |
| return fmt.Errorf("failed to get 'num-blocks' flag: %w", err) | |
| } | |
| blockSize, err := cmd.Flags().GetInt("block-size") | |
| if err != nil { | |
| return fmt.Errorf("failed to get 'block-size' flag: %w", err) | |
| } | |
| blockInterval, err := cmd.Flags().GetDuration("block-interval") | |
| if err != nil { | |
| return fmt.Errorf("failed to get 'block-interval' flag: %w", err) | |
| } | |
| existingDir, err := cmd.Flags().GetString("existing-dir") | |
| if err != nil { | |
| return fmt.Errorf("failed to get 'existing-dir' flag: %w", err) | |
| } | |
| namespaceStr, err := cmd.Flags().GetString("namespace") | |
| if err != nil { | |
| return fmt.Errorf("failed to get 'namespace' flag: %w", err) | |
| } | |
| upToTime, err := cmd.Flags().GetBool("up-to-now") | |
| if err != nil { | |
| return fmt.Errorf("failed to get 'up-to-now' flag: %w", err) | |
| } | |
| appVersion, err := cmd.Flags().GetUint64("app-version") | |
| if err != nil { | |
| return fmt.Errorf("failed to get 'app-version' flag: %w", err) | |
| } |
Adds a tool for quickly generating an entire chain (based on a single validator)