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.

Sign up for GitHub

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

Jump to bottom

Support custom timeout_commit in testing mode #4080

Closed
cmwaters opened this issue Dec 4, 2024 · 3 comments · Fixed by #4103
Closed

Support custom timeout_commit in testing mode #4080

cmwaters opened this issue Dec 4, 2024 · 3 comments · Fixed by #4103
Assignees
@rootulp
Labels
WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc

Comments

@cmwaters
Copy link
Contributor

cmwaters commented Dec 4, 2024 •
edited
Loading

Summary

Since v3, timeout commits are now dictated by the application. This has made it difficult in the case of testing, where we and others want to be able to have short timeouts. Both sovereign and eiger have reported this being problematic for them.

Proposal

Introduce either an environment variable or a flag to force set the timeout commit. We may want to consider this to also set the square size as well. If we're concerned about misuse on a live network we can add chainID checks.
@cmwaters
Copy link
Contributor Author

cmwaters commented Dec 4, 2024

Furthermore, we might want to, if it's not too breaking, to rename the current timeout_commit and timeout_propose in the config.toml (or at least the go struct) to something else so as to avoid confusion. Most teams may still be setting these values and think it will change the times

@rootulp
Copy link
Collaborator

rootulp commented Dec 4, 2024

Introduce either an environment variable or a flag to force set the timeout commit. We may want to consider this to also set the square size as well.

We have the OVERRIDE_MAX_SQUARE_SIZE env variable which can override the max square size at binary build time so we could add another for overriding timeout_commit.

Alternatively, we could override the timeout_commit when the chain_id is test similar to how we override the upgrade height delay to 3 here.

Furthermore, we might want to, if it's not too breaking, to rename the current timeout_commit and timeout_propose in the config.toml (or at least the go struct) to something else so as to avoid confusion. Most teams may still be setting these values and think it will change the times

IMO we should deprecate the config timeout_commit and timeout_propose in v3.x.x and remove them in v4.x.x.

@evan-forbes evan-forbes added WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc and removed needs:triage labels Dec 5, 2024
@rootulp
Copy link
Collaborator

rootulp commented Dec 6, 2024

Related: #4023

@rootulp rootulp mentioned this issue Dec 9, 2024
Closed
@evan-forbes evan-forbes mentioned this issue Dec 9, 2024
Open
@rootulp rootulp mentioned this issue Dec 9, 2024
Merged
rootulp added a commit that referenced this issue Dec 12, 2024
@rootulp
feat: override timeout commit via --timeout-commit (#4103)
d20916c 
Closes #4080

## Testing

1. `./scripts/single-node.sh` now makes blocks faster 🎊 
2. `./scripts/arabica.sh` throws an error if you try to add
`--timeout-commit 1s`

```
Error: the --timeout-commit flag was used on arabica-11 but it is unsupported on public networks: arabica-11, mocha-4, celestia. The --timeout-commit flag should only be used on private testnets.
```
mergify bot pushed a commit that referenced this issue Dec 12, 2024
@rootulp @mergify
feat: override timeout commit via --timeout-commit (#4103)
ac69604 
Closes #4080

## Testing

1. `./scripts/single-node.sh` now makes blocks faster 🎊
2. `./scripts/arabica.sh` throws an error if you try to add
`--timeout-commit 1s`

```
Error: the --timeout-commit flag was used on arabica-11 but it is unsupported on public networks: arabica-11, mocha-4, celestia. The --timeout-commit flag should only be used on private testnets.
```

(cherry picked from commit d20916c)

# Conflicts:
#	cmd/celestia-appd/cmd/start.go
#	scripts/single-node.sh
@mergify mergify bot mentioned this issue Dec 12, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rootulp rootulp

Labels
WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: override timeout commit via --timeout-commit rootulp/celestia-app
3 participants
@rootulp @cmwaters @evan-forbes