search

celestiaorg / celestia-app

Celestia consensus node
https://celestiaorg.github.io/celestia-app/
Apache License 2.0
346 stars 293 forks source link

feat!: versioned timeouts #3882

Closed staheri14 closed 1 month ago

staheri14 commented 2 months ago

Closes https://github.com/celestiaorg/celestia-app/issues/3859

manually tested in https://github.com/celestiaorg/celestia-app/pull/3882#issuecomment-2415280773

also tested in knuu

jcstein commented 1 month ago

Should GoalBlockTime = time.Second * 6 on pkg/appconsts/consensus_consts.go?

Linking discussion from my PR which I plan to close: https://github.com/celestiaorg/celestia-app/pull/3953/files#r1794435786

coderabbitai[bot] commented 1 month ago
📝 Walkthrough
📝 Walkthrough ## Walkthrough The pull request introduces extensive modifications to the application's codebase, emphasizing timeout settings and application version management. Key updates include the dynamic adjustment of timeout parameters based on the application version, enhancements to upgrade logic, and the introduction of new constants across various modules. Additionally, several tests have been created or updated to validate the functionality of the application under these new configurations, ensuring that the changes align with the intended version upgrade process. ## Changes | File Path | Change Summary | |---------------------------------------------|------------------------------------------------------------------------------------------------| | app/app.go | Comment corrections, dynamic timeout settings, refined upgrade logic, and new app version initialization method. | | app/default_overrides.go | Updated consensus parameters and customized genesis states for multiple modules. | | app/test/upgrade_test.go | Enhanced tests for application upgrades and timeout parameters. | | go.mod | Updated Go module version and dependencies. | | pkg/appconsts/global_consts.go | Removed `DefaultUpgradeHeightDelay` constant and associated function. | | pkg/appconsts/v1/app_consts.go | Introduced `TimeoutPropose` and `TimeoutCommit` constants. | | pkg/appconsts/v2/app_consts.go | Introduced `TimeoutPropose` and `TimeoutCommit` constants. | | pkg/appconsts/v3/app_consts.go | Introduced `TimeoutPropose` and `TimeoutCommit` constants. | | pkg/appconsts/versioned_consts.go | Added functions to retrieve timeout values based on version. | | x/signal/keeper.go | Removed `upgradeHeightDelayBlocks` field and updated `NewKeeper` function. | | x/signal/keeper_test.go | Enhanced test suite with new test cases and refined assertions. | ## Assessment against linked issues | Objective | Addressed | Explanation | |----------------------------------------------------------------|-----------|---------------------------------------------------| | Override the `timeout_commit` and `timeout_propose` when switching to v3 (#3859) | ✅ | | ## Possibly related PRs - **#3450**: Changes in `app/app.go` regarding the initialization of the app version and timeout settings are related to the modifications in the same file in this PR, which also addresses version handling and initialization logic. - **#3560**: This PR modifies the `EndBlocker` function in `app/app.go`, which is directly related to the changes made in the main PR regarding the upgrade logic and timeout settings. - **#3846**: The changes in `InitChain` and the introduction of the `setDefaultAppVersion` function in this PR are relevant to the modifications in the main PR that also focus on application version management. - **#3980**: This PR discusses changes to the `UpgradeHeightDelay`, which is directly related to the modifications in the main PR that involve timeout settings and upgrade logic in `app/app.go`. ## Suggested labels `WS: V3 3️⃣`, `required`, `WS: Maintenance 🔧` ## Suggested reviewers - cmwaters - ninabarbakadze - rootulp - rach-id

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 - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
🪧 Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): > ‼️ **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](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
rootulp commented 1 month ago

Closes https://github.com/celestiaorg/celestia-app/issues/3859 though is in progress!

Guessing no longer in progress

evan-forbes commented 1 month ago

the knuu cluster is on the fritz, and @rach-id 's local devnet tool comes to the rescue again! here are the timestamps from the logs

15D71B38D250F6D2CB0ADD7E711A8658FD63DA993EE4939E77","total":1}},"height":49,"pol_round":-1,"round":0,"signature":"AVp2K3YL3zxlwbROldfOCbLalitHc5DNU9Q7kZzfFxMZMoy30IsCq3tXaXpCs42GakLe2yWhGtsmFQEGpbzhCg==","timestamp":"2024-10-15T22:21:01.230269104Z"} proposer=12C509ED04EA103C84944017B9E7ADE494D486A3
10:21PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"5A9EDAE7C7F68A36D4D4604693F997C00DFC399D007AC8DDEA48C6E0BC649756","parts":{"hash":"DD427A879508A4D6D3EB497071F437AC0938EB0E5DFB78D1569A5D2CFFA149A4","total":1}},"height":50,"pol_round":-1,"round":0,"signature":"3BzjkhFr/sVGlzWc1yfQyFH1fC3McGKBFdJV00pYG+cyviBjLvCoS6CAqCA7SmVJKlcSshzL+7hPCd98I24FBA==","timestamp":"2024-10-15T22:21:12.783355131Z"} proposer=223467FF1BD29B323D8CF4AFCFECD5ED1C0D4383
10:21PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"ED5731ADB3F97BD54515C3C0B5CBE0E1F95AE79312DDEFC3DA98AC10B32581A1","parts":{"hash":"CDD6126E7D55BA813BAA811637355CC57037E74B278AA453FAAF681EC998E2AC","total":1}},"height":51,"pol_round":-1,"round":0,"signature":"tIyE8v9HPbuA/ZRVJMobBiC1WwCU62aSkKlgmbT2NfhJ26AZxu6RXx8XctuhwU0ompYVOdRoQWGHh2KuMrAJBw==","timestamp":"2024-10-15T22:21:24.202601875Z"} proposer=BC381AF58CB970ACAF795A003461EE02057C5F92
10:21PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"5A95DDAEE62A7DAAA2997E604D513A6B7063C332E8138C2908B6D705DF0B8E11","parts":{"hash":"1580CCD32169396E8DFBB2AD55C3D50A111ADBE8F84E586990A40D723A64A9D0","total":1}},"height":52,"pol_round":-1,"round":0,"signature":"VC1F7OHMIaLxAcwsgjLRUgUNYdNWFPQBjHwDdnJFQvW0V9Njd3YI3XN1KoriwXA42aqbt3xmOCKLAqNqBkyLDQ==","timestamp":"2024-10-15T22:21:35.613200507Z"} proposer=E0B63563D528BDA8AA3D26A703FC8EAAD1556016
10:21PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"E6325454000B16A6CB1963B76F0AF12ED03FE89AC47A8B677C140AD432416862","parts":{"hash":"A72AF13D9118850C4B5904CDABAD78022FAE2B8554E503915493C8F51C84B9C5","total":1}},"height":53,"pol_round":-1,"round":0,"signature":"+sXsU+NF1iNEvaJElb8ax9nXPHh951aVeLNr4hzvCt2TUZKg60HZtvLEBXoFAiKnlM/n3KO+bu/xrmc75A9lAQ==","timestamp":"2024-10-15T22:21:47.032329431Z"} proposer=12C509ED04EA103C84944017B9E7ADE494D486A3
10:21PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"960877E0053D87126B3D3E2991066F1D9D9BA1110CFC811D2C5E9A0864A71F91","parts":{"hash":"7F7D31404FF5AF5C3F52BDE7E769CDE6B00C498EDA7A02FDA0C8DAB9D9AB307B","total":1}},"height":54,"pol_round":-1,"round":0,"signature":"x10mNLY9SZMYTTEpAait+9PZrxW08/k/t2begQiPX9jrHkRlNnRjBVDwYeDQEx655iMTT6xDB3IgDhKhnEadBQ==","timestamp":"2024-10-15T22:21:51.747512718Z"} proposer=223467FF1BD29B323D8CF4AFCFECD5ED1C0D4383
10:21PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"6BF50B6FA84A6CEB80F48E6EBE2344EDAE394B14802D1A5F3F9F3DD13D364540","parts":{"hash":"AF1A821BF5CEA6DC96BCECF65678E07444196050F8034D7336DB59C36179E9FE","total":1}},"height":55,"pol_round":-1,"round":0,"signature":"+v33QJhJ4rJYn/JKzmarV3PZU1MHpKKDWUTO6K5YMNC6Ne5XJJbaMX89lQK8tK39fffP7JnYocSVn28tDSWhCg==","timestamp":"2024-10-15T22:21:56.356056546Z"} proposer=BC381AF58CB970ACAF795A003461EE02057C5F92
10:22PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"AB963F8637A4183BAB94A1AA5016FFC6DE7550D7118928C91C34FE919F412578","parts":{"hash":"964C27E0F75BD14570B1EE2433712B6048667E92A5BECDCB5F407A156814E398","total":1}},"height":56,"pol_round":-1,"round":0,"signature":"T850ryJPbgrtGeDmrHkFtUVopAb+OBSEFiWZO5uueXnbzi7R/U287447BBZH/9wEp+5+UGHPvWKt6t8vROIpBQ==","timestamp":"2024-10-15T22:22:00.964394091Z"} proposer=E0B63563D528BDA8AA3D26A703FC8EAAD1556016
10:22PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"2760E8C5A57C9D9C5BC87ED2D4BCB9F81C2065B3560849FBE6DD7F3EF7C7A570","parts":{"hash":"4C0EF005E86D3274B75353C0B37099B9DCE6E0013BE412ADCDEABC7E4A4E23C5","total":1}},"height":57,"pol_round":-1,"round":0,"signature":"opOXXP6M2PMhZ1pNXniOaJUKinCMJ/dnmhoDIHJpoPxrURvH4TToEWYg3bYtpyqdaGTmPXeu1kDvEwVUqjjTCw==","timestamp":"2024-10-15T22:22:05.484655108Z"} proposer=12C509ED04EA103C84944017B9E7ADE494D486A3
10:22PM INF received proposal module=consensus proposal={"Type":32,"block_id":{"hash":"27861014C6F14575F1C75DCE134BBE7A1B4D3B56885207A78992EA51C2454026","parts":{"hash":"82C556A57098F168F0E71E3A7D9361B654BE2182F0D33610D3592B2D2E2E666C","total":1}},"height":58,"pol_round":-1,"round":0,"signature":"o3Bcw/bfS79fTT3AiWgTBBDuBdhTOAdB42Iut2UKgXwHvIprn7EDiy75l1JqWf7WwxMBCf76VBmYGRHic8VhBA==","timestamp":"2024-10-15T22:22:10.202010713Z"} proposer=223467FF1BD29B323D8CF4AFCFECD5ED1C0D4383

the upgrade works as expected and the timeouts are overwritten. therefore, we can create a release in core and merge.

evan-forbes commented 1 month ago

Can we just create a follow up issue to fix the v3 upgrade test such that we don't need a fixed commit tag

ahh good catch https://github.com/celestiaorg/celestia-app/issues/3981

cmwaters commented 1 month ago

[question] now that TimeoutPropose and TimeoutCommit are versioned constants, should they be removed from consensus constants here?

Yeah they probably can be

evan-forbes commented 1 month ago

In the future, would be nice if PR description contained a testing section so reviewers know how this change was manually tested

@rootulp good idea, added a link to the comment and we can also test in knuu now. it appears to pass if a I manually log it and check, but there's an rpc that keeps failing that seems unrelated