Skip to content

feat(types): increase default SynchronyParams consensus parameters values#4249

Merged
cason merged 5 commits intomainfrom
cason/4246-sync-params
Oct 7, 2024
Merged

feat(types): increase default SynchronyParams consensus parameters values#4249
cason merged 5 commits intomainfrom
cason/4246-sync-params

Conversation

@cason
Copy link

@cason cason commented Oct 4, 2024

Closes #4246


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@cason cason requested a review from a team as a code owner October 4, 2024 10:40
@cason cason requested a review from a team October 4, 2024 10:40
@cason cason self-assigned this Oct 4, 2024
@cason cason changed the title types: increase default consensus parameters SynchronyParams values feat(types): increase default consensus parameters SynchronyParams values Oct 4, 2024
@cason cason changed the title feat(types): increase default consensus parameters SynchronyParams values feat(types): increase default SynchronyParams consensus parameters values Oct 4, 2024
@cason cason added the pbts label Oct 4, 2024
Copy link
Collaborator

@hvanz hvanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return SynchronyParams{
Precision: 500 * time.Millisecond,
MessageDelay: 2 * time.Second,
Precision: 505 * time.Millisecond,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why 505 ms? (I mean, why the extra 5 ms)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea. It is more than 500, we could use 501, ehehe.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the number that Willian has adopted in the original implementation.

Copy link
Collaborator

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this 🙏

@cason cason added this pull request to the merge queue Oct 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 7, 2024
@cason cason added this pull request to the merge queue Oct 7, 2024
Merged via the queue into main with commit bbb86ce Oct 7, 2024
@cason cason deleted the cason/4246-sync-params branch October 7, 2024 08:50
mergify bot pushed a commit that referenced this pull request Oct 7, 2024
…values (#4249)

Closes #4246

- `Precision` was set to `505ms`, because it should be greater than
0.5s, as in tendermint/tendermint#7724
- `MessageDelay` was increased to `15s`, following the measurements
reported in tendermint/tendermint#7788, and
considering some outputs of `e2e` tests

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments

(cherry picked from commit bbb86ce)
cason pushed a commit that referenced this pull request Oct 7, 2024
…values (backport #4249) (#4278)

Closes #4246

- `Precision` was set to `505ms`, because it should be greater than
0.5s, as in tendermint/tendermint#7724
- `MessageDelay` was increased to `15s`, following the measurements
reported in tendermint/tendermint#7788, and
considering some outputs of `e2e` tests

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
<hr>This is an automatic backport of pull request #4249 done by
[Mergify](https://mergify.com).

Co-authored-by: Daniel <daniel.cason@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pbts: increase the default synchrony values to higher ones

3 participants