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

docs: added a ramification of opt out #1533

Merged
merged 2 commits into from
Dec 22, 2023
Merged

Conversation

insumity
Copy link
Contributor

@insumity insumity commented Dec 20, 2023

Description

Added a ramification of ADR 009, namely that soft opt-out would delay the decision of blocks on consumer chains.

Probably, the most relevant document to read about rounds on the underlying Tendermint consensus algorithm is The latest gossip on BFT consensus paper where, among others, it's written:

The algorithm proceeds in rounds, where each round has a dedicated proposer. The mapping of rounds to proposers is known to all processes and is given as a function proposer(h, round), returning the proposer for the round round in the consensus instance h.

And then in the pseudocode presented in Algorithm 1, we can see that if a proposer is down, other validators would prevote on nil and eventually OnTimeoutPrecommit would be called and increase the round number.

Thanks to @sergio-mena for mentioning this!


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...

  • included the correct docs: prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers 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...

  • Confirmed the correct docs: prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR only changes documentation
  • Reviewed content for consistency
  • Reviewed content for spelling and grammar
  • Tested instructions (if applicable)
  • Checked that the documentation website can be built and deployed successfully (run make build-docs)

@insumity insumity requested a review from a team as a code owner December 20, 2023 12:46
@insumity insumity changed the title docs: added one ramification of opt out. docs: added one ramification of opt out Dec 20, 2023
@insumity insumity changed the title docs: added one ramification of opt out docs: added a ramification of opt out Dec 20, 2023
@github-actions github-actions bot added C:Docs Assigned automatically by the PR labeler C:ADR Assigned automatically by the PR labeler labels Dec 20, 2023
@@ -29,8 +29,9 @@ Refer to the [upgrading guide](https://github.com/cosmos/interchain-security/blo
<!-- Add any highlights of this release -->

## ❤️ Contributors

<!-- markdown-link-check-disable -->
* Informal Systems ([@informalinc](https://twitter.com/informalinc))
Copy link
Contributor Author

@insumity insumity Dec 20, 2023

Choose a reason for hiding this comment

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

Tiny change because PR was failing on markdown-link checking.
We could also use Nitter instead but it's quite likely for Nitter to be down once in a while.

Copy link
Contributor

@p-offtermatt p-offtermatt left a comment

Choose a reason for hiding this comment

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

Is it easy to add a reference to the code that shows that this is happening?
I think it's probably hard, but if possible it would be cool to have something here
that future readers can check to convince themselves this is the behaviour

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Nice catch

@insumity insumity closed this Dec 22, 2023
@insumity insumity reopened this Dec 22, 2023
@insumity
Copy link
Contributor Author

@p-offtermatt

Is it easy to add a reference to the code that shows that this is happening?
I think it's probably hard, but if possible it would be cool to have something here
that future readers can check to convince themselves this is the behaviour

I think it might be a bit confusing to point to CometBFT code snippets as part of this ADR. It could be done though if you think it would add value.
I augmented the description in the PR with a pointer to the Tendermint paper. I feel that would be a more reasonable starting point for someone that would be interested in this. Again, I felt it might be a bit too much to add this information on the actual ADR.

Copy link
Contributor

@p-offtermatt p-offtermatt left a comment

Choose a reason for hiding this comment

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

Yup, that works for me, thanks for going through the trouble!

@insumity insumity added this pull request to the merge queue Dec 22, 2023
Merged via the queue into main with commit 1f26c37 Dec 22, 2023
27 of 28 checks passed
@insumity insumity deleted the insumity/added-negative-ADR-009 branch December 22, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:ADR Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants