feat(server): in-place testnet creator #19280
Conversation
czarcas7ic
changed the title
| if opts.AddFlags != nil { | ||
| opts.AddFlags(cmd) | ||
| } | ||
| addStartNodeFlags(cmd, opts) |
There was a problem hiding this comment.
Created helper function to reduce code duplication across the normal start command and the testnet creator command, which also shares the same flags.
| if isTestnet, ok := svrCtx.Viper.Get(KeyIsTestnet).(bool); ok && isTestnet { | ||
| app, err = testnetify(svrCtx, home, appCreator, db, traceWriter) | ||
| if err != nil { | ||
| return app, traceCleanupFn, err | ||
| } | ||
| } else { | ||
| app = appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the only code that touches "core logic" of the normal startApp flow.
| // Set testnet keys to be used by the application. | ||
| // This is done to prevent changes to existing start API. | ||
| serverCtx.Viper.Set(KeyIsTestnet, true) | ||
| serverCtx.Viper.Set(KeyNewChainID, newChainID) | ||
| serverCtx.Viper.Set(KeyNewOpAddr, newOperatorAddress) |
There was a problem hiding this comment.
We set keys in Viper in order to prevent changing existing APIs. This allows us to share the same flow across start and in-place-testnet commands.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
server/start.go
Dismissed
| Height: state.LastBlockHeight, | ||
| Round: 0, | ||
| BlockID: state.LastBlockID, | ||
| Timestamp: time.Now(), |
Check warning
Code scanning / CodeQL
Calling the system time Warning
There was a problem hiding this comment.
This is fine, this is the only validator in the set, set setting the timestamp to local time is a non issue.
|
Warning Rate Limit Exceeded@czarcas7ic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 22 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis change introduces a significant feature aimed at enhancing the testing capabilities within the Changes
Assessment against linked issues
The code changes seem to address most of the key objectives outlined in the linked issue, with a primary focus on simplifying the testing process by enabling in-place modifications of the mainnet state. However, there is some ambiguity regarding the implementation of state transition functions or tools behind a build flag, and it appears that the approach taken leans more towards in-place modifications rather than developing a separate tool for state transitions. 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 (
|
|
The leaking resource CI check is failing across all PRs. |
alexanderbez
requested review from
alexanderbez,
facundomedica and
julienrbrt
| @@ -631,7 +596,15 @@ func startApp(svrCtx *Context, appCreator types.AppCreator, opts StartCmdOptions | |||
| return app, traceCleanupFn, err | |||
| } | |||
|
|
|||
| app = appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper) | |||
| if isTestnet, ok := svrCtx.Viper.Get(KeyIsTestnet).(bool); ok && isTestnet { | |||
There was a problem hiding this comment.
So what is the relationship between the start command, which checks KeyIsTestnet, and the new testnet command?
There was a problem hiding this comment.
The testnet command and the start command are essentially identical except:
- The testnet command provides 2 inputs necessary for making the testnet
- The testnet command runs the testnetify logic before returning the app from the appCreator
- The pubkey and validator address retrieved from testnetify are stored to viper keys which are used at the application level
I am not sure if this answers your question, please lmk if you wanted clarification on something else.
There was a problem hiding this comment.
im not sure this needs to exist. I may be missing something but this tool should only modify the underlying db.
|
Added a small feat, which allows the upgrade handler to be triggered on the first block. We ran into the case where, if new stores were introduced, the flow required would be:
This flag allows us to skip all this and just use the current daemon on block one. Besides this, have not had any other issues, and has been working great. |
There was a problem hiding this comment.
overall looks really good, i think its in the wrong place, this code will be gone in a few months. I think it makes more sense to have it separate and imported into the apps cmd file and set, similar to how we do other commands. Would you be open to changing it?
I think i understand the design, you aimed at taking a db and just starting it in testnet mode. The mix of testing code and production code is something we want to avoid. It is common in the sdk but this is because most of the designs were not thought through to keep these things separated. I think taking a db, running a command to migrate then starting is a cleaner separation of the two functionalities
|
Had a discussion with @tac0turtle offline, and it seems like this implementation is good for merge / backport. Please lmk if there are any further desired changes I should make. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- server/start.go (5 hunks)
- server/util.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- server/start.go
- server/util.go
|
I just added a skip-confirmation flag to allow our infra/devops to automate testnet creation. |
|
I wont push any more to this. Just wanted to fix a small bug we found after creating many testnets, we saw that stopping a node with the halt height flag causes the application height to match the blockstore height, which needs to be handled slightly differently. |
julienrbrt
added
the
backport/v0.50.x
julienrbrt
changed the title
server/start.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| // Modify genesis chain ID and save to genesis file. |
There was a problem hiding this comment.
We should use an app genesis here, otherwise we may overwrite the app genesis into a comet genesis
There was a problem hiding this comment.
@julienrbrt I think this addresses your concern, if you could look it over before merge that would be much appreciated!
|
Could you have a look at the linter complaining? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- server/start.go (5 hunks)
Additional comments: 5
server/start.go (5)
- 4-27: The addition of new imports is correctly done and aligns with the described functionality in the PR objectives. Ensure that each imported package is used within the file to avoid unnecessary imports.
- 600-608: The modifications in the
startAppfunction to include conditional logic based on theisTestnetflag and the addition of thetestnetifyfunction call are correctly implemented. Ensure that theisTestnetflag is properly set and used throughout the application to maintain consistent behavior.- 618-710: The
InPlaceTestnetCreatorfunction is correctly implemented, providing a command to create and start a testnet from the current local state. Ensure that the user prompts and flag handling are clear and user-friendly. Additionally, validate that the error handling and cleanup processes are robust to prevent state corruption.- 712-929: The
testnetifyfunction correctly modifies both state and blockStore to allow the provided operator address and local validator key to control the network. Ensure that the modifications to the genesis file, state, and blockStore are correctly implemented and that error handling is comprehensive to prevent partial state modifications.- 931-983: The modifications to the
addStartNodeFlagsfunction to include new flags are correctly implemented. Ensure that each flag has a clear and concise description and that the flags are used appropriately within the application to control behavior based on user input.
czarcas7ic
added
the
backport/v0.47.x
|
I added the 47 backport label, although the logic will need to differ sightly due to obv difference between the versions. It will mirror the implementation on the osmosis fork. |
v0.47 is bugfix only, so in theory it shouldn't get this. |
|
@julienrbrt how do you all backport state compatible features to previous versions? That's what I meant by adding the tag. |
|
In theory, we don't backport features or improvements to v0.47. As it is in its bugfix only phase. |
|
Gotcha. I think that there will likely be quite a few chains on v47 for at least the next year, and this feature (if utilized) will improve security across these chains. But totally respect the decision if thats not what you guys want to do! |
(cherry picked from commit 89df28c) # Conflicts: # CHANGELOG.md # baseapp/options.go # server/start.go
(cherry picked from commit 89df28c) # Conflicts: # CHANGELOG.md # server/start.go
Description
Closes: #18706
This PR introduces the sdk side logic required to create testnets from local state. This feature is useful to create testnets that mirror mainnet state. When running on a recent snapshot, this process takes less than 3 seconds on Osmosis. This feature intends to replace the current method of state exported testnets, which currently takes 6 hours + requires 256 GB RAM VMs for Osmosis. Also, state exported testnets requires some esoteric knowledge on how to modify the state properly, whereas this should be much simpler for other chains to implement, with the goal being improved testing environments across all Cosmos chains.
Here is an example of this feature being implemented app side on Osmosis:
Important to note, I tested this throughly on v0.47 and it works as intended. I did, however, need to make a few changes to target the sdk main branch, which I am not able to test against Osmosis with. Here is the PR to the sdk fork for Osmosis, which will also be helpful if we want to back port this (we should):
osmosis-labs#506
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit