Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Considering removing validation of timeout commit and timeout propose from ValidateBasic() after implementing versioned timeouts #1517

Open
staheri14 opened this issue Oct 11, 2024 · 1 comment

Comments

@staheri14
Copy link
Collaborator

staheri14 commented Oct 11, 2024

After introducing versioned timeouts, it may be decided to remove the TimeoutCommit and TimeoutPropose fields from the configs. If this happens, then there is no need to validate them in the ConsensusConfig.ValidateBasic(). This issue serves as a reminder to proceed with this change, if that decision has been made.

@staheri14 staheri14 changed the title Considering removing timeout commit and timeout propose from configs Considering removing timeout commit and timeout propose from configs after implementing versioned timeouts Oct 11, 2024
@staheri14 staheri14 changed the title Considering removing timeout commit and timeout propose from configs after implementing versioned timeouts Considering removing validation of timeout commit and timeout propose ValidateBasic() after implementing versioned timeouts Oct 11, 2024
@staheri14 staheri14 changed the title Considering removing validation of timeout commit and timeout propose ValidateBasic() after implementing versioned timeouts Considering removing validation of timeout commit and timeout propose from ValidateBasic() after implementing versioned timeouts Oct 11, 2024
@rootulp
Copy link
Collaborator

rootulp commented Oct 14, 2024

it may be decided to remove the TimeoutCommit and TimeoutPropose fields from the configs

I think we need to keep these in the celestia-app v3 binary because the config values should be used until the app version upgrades from 2 -> 3. So this issue can only be done in celestia-app v4.

Another idea: removing those two fields from the config may be considered breaking so proposal to keep them in config but document that they have no impact in >= v4 binary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants