fix(spec/consensus): Small fixes to overview.md file. #873

nenadmilosevic95 · 2025-02-24T13:00:56Z

I made a pass on overview.md file.
IMHO this is a very nice explanation of Tendermint. Great job @cason and @josef-widder and who else contributed !!!
I made some minor fixes, and made some comments. I am available if you want to discuss anything, and feel free to ignore the things you think are not relevant!

PR author checklist

For all contributors

Reference GitHub issue
Ensure PR title follows the conventional commits spec

For external contributors

Maintainers of Malachite are allowed to push changes to the branch

cason

Thank you very much for taking your time to review this.

I tried to fix some of the points you have raised and we need to further discuss others.

The point is that we can't commit your \NM{} comments to main, so either we keep it as a draft, or we work to rephrase some content in order to address the comments.

cason · 2025-02-24T15:29:24Z

specs/consensus/overview.md

-The algorithm presented in the [pseudo-code][pseudo-code] represent the
-operation of an instance of consensus in a process `p`.
+The algorithm presented in the [pseudo-code][pseudo-code] represents the
+operation of an instance of consensus in a process `p`. NM{The pseudo-code from the paper is not one instance/height, it is multi instance? }


There is a confusion between instance of consensus and instance of the protocol, also known as a process.

Thank you for spotting that.

specs/consensus/overview.md

cason · 2025-02-24T15:31:30Z

specs/consensus/overview.md

@@ -39,7 +39,7 @@ A this point, the process increases `h_p` (line 52) and starts the next height
 of consensus, in which the same algorithm is executed again.

 For the sake of the operation of the consensus algorithm, heights are
-completely independent executions.
+completely independent executions. NM{Not sure I fully understand what do we want to say here?}


Messages and events from a height H have no effect on the operation of any height H' != H.

Suggested change

completely independent executions. NM{Not sure I fully understand what do we want to say here?}

communication-closed, that is, a message from some height `H` does not influence a process that is in height `H' != H`.

specs/consensus/overview.md

cason · 2025-02-24T15:36:37Z

specs/consensus/overview.md

@@ -181,6 +181,7 @@ associated to a [round step](#round-steps):
  The last field can be either the unique identifier `id(v)` of a proposed
  value `v` for which the process has received `⟨PREVOTE, h, r, id(v)⟩`
  messages from a super-majority of processes, or the special `nil` value otherwise.
+  NM{I am not sure we have defined super-majority anywhere?} 


We can add a link to the #voting-power section, where we define it properly. Since we indeed did not define it yet.

cason · 2025-02-24T15:54:48Z

specs/consensus/overview.md

 None of the locked values can be decided, for the lack of votes, but liveness
-is under threat, as detailed in this [discussion][equivocation-discussion].
+is under threat, as detailed in this [discussion][equivocation-discussion]. \NM{Potentially they cannot be decided, so maybe we can rephrase a bit this sentence.  }


This paragraph is describing a scenario, where we assume that this happens. So I am not sure if we need to fix the sentence.

specs/consensus/overview.md

cason

I have now completed covering your reviews.

Can you check the suggestions that are good, or provide alternative version of them, so we can remove some of the added "comments"?

cason · 2025-02-24T16:54:04Z

specs/consensus/overview.md

@@ -879,6 +883,8 @@ There are however some scenarios to consider:
 3. `step_p = precommit`: in this case, `p` only updates `validValue_p` to `v`
   and `validRound_p` to `r`.

+\NM{I would maybe just talk about two scenarios here, when it locks and when it just updates valid values}


Yes, at the end there are two. For the first we say that either 2. or 3. apply (or maybe none of them).

cason · 2025-02-24T16:57:03Z

specs/consensus/overview.md

@@ -929,7 +940,8 @@ correct processes, and consider the [Locked Value](#locked-value) handling:
 - every process `p` in `C` has set `lockedValue_p` to `v` and `lockedRound_p`
  to `r` ;
 - every process `p` in `C` will broadcast a `PREVOTE` for `nil` in rounds
-  `r' > r` where a value `v' != v` is proposed.
+  `r' > r` where a value `v' != v` is proposed. \NM{this might not be clear for all and this is 
+  what we explain below, so maybe we can somehow rephrase" }


Where we describe there, I could not follow here. Maybe we can explain why they prevote nil, otherwise I don't know how to change the description.

specs/consensus/overview.md

cason · 2025-02-24T17:07:55Z

specs/consensus/overview.md

@@ -1019,6 +1031,10 @@ Notice, however, that from the [Gossip communication property](#network), `q`
 should eventually receive the POL for `v` in round `vr`, since `p` is a correct
 process.

+\NM{I think we should mention in the text here that we show the intution how condition 2 is achieved later, because


I agree. Added a sentence to the last paragraph, check if it works. :)

specs/consensus/overview.md

Co-authored-by: Daniel <daniel.cason@informal.systems> Signed-off-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>

josef-widder

Thanks for the improvements. I left a few suggestions.

josef-widder · 2025-02-26T09:03:02Z

specs/consensus/overview.md

@@ -39,7 +39,7 @@ A this point, the process increases `h_p` (line 52) and starts the next height
 of consensus, in which the same algorithm is executed again.

 For the sake of the operation of the consensus algorithm, heights are
-completely independent executions.
+completely independent executions. NM{Not sure I fully understand what do we want to say here?}


Suggested change

completely independent executions. NM{Not sure I fully understand what do we want to say here?}

communication-closed, that is, a message from some height `H` does not influence a process that is in height `H' != H`.

josef-widder · 2025-02-26T09:03:59Z

specs/consensus/overview.md

@@ -180,7 +180,8 @@ associated to a [round step](#round-steps):
  [`precommit`](#precommit) step of round `r` of height `h`.
  The last field can be either the unique identifier `id(v)` of a proposed
  value `v` for which the process has received `⟨PREVOTE, h, r, id(v)⟩`
-  messages from a super-majority of processes, or the special `nil` value otherwise.
+  messages from a [super-majority](#voting-power) of processes, or the special `nil` value otherwise.
+  NM{I am not sure we have defined super-majority anywhere?} 


Suggested change

NM{I am not sure we have defined super-majority anywhere?}

I think @cason already addressed this

josef-widder · 2025-02-26T09:13:03Z

specs/consensus/overview.md

 `⟨PREVOTE, h, r, *⟩` or `⟨PRECOMMIT, h, r, *⟩` messages received from a process
 in a round `r` of height `h`.
 Different (equivocating) versions of the same message from the same sender
 should, from a defensive point of view, be disregarded and dropped by the
-consensus logic as they were duplicated messages.
+consensus logic as they were duplicated messages. NM{Ok I guess this is the change you want to implement or have alrady implemented, that is different from the paper?}


Suggested change

consensus logic as they were duplicated messages. NM{Ok I guess this is the change you want to implement or have alrady implemented, that is different from the paper?}

consensus logic as they were duplicated messages (note that doing this maintains safety, but leads to violations of the gossip property from the [Tendermint paper](https://arxiv.org/abs/1807.04938), and thus introduces liveness issues; see below).

josef-widder · 2025-02-26T09:18:41Z

specs/consensus/overview.md

@@ -744,7 +748,7 @@ But `∆` is not always observed by the system, which may operate asynchronously
 for an arbitrary amount of time.
 There is, however, a (possibly unknown) Global Stabilization Time (`GST`), a
 time from which the system becomes synchronous and `∆` is observed for all
-messages sent by correct processes.
+messages sent by correct processes. \NM{Ok this "possibly unknown" in both sentences is confusing me? Did you try to somehow capture both versions of partially synchronous system models? }


I think it is fine, I would just keep it. Even if both are unknown, the algorithm should work, no? That would be the model in the Chandra/Toueg paper on failure detectors.

josef-widder · 2025-02-26T09:21:00Z

specs/consensus/overview.md

+\NM{The explanations here are fine for me, but I am not good to review this as I know in detail how this mechanism 
+works. I think we should ask someone who does not understand how valid value and locked value correlate and see
+if he/she got it from this. I think this explanation introduces the difference but do not explain the mechanism. 
+Update: I saw that latter you do explain, so I am happy with the explanations. }


Suggested change

\NM{The explanations here are fine for me, but I am not good to review this as I know in detail how this mechanism

works. I think we should ask someone who does not understand how valid value and locked value correlate and see

if he/she got it from this. I think this explanation introduces the difference but do not explain the mechanism.

Update: I saw that latter you do explain, so I am happy with the explanations. }

I guess the update says that it is fine as is.

Nenad Milosevic added 4 commits February 20, 2025 17:57

pass until Byzantine Proposers paragraph done

050e3bf

pass until correctness

e211fdb

pass until protocol correctness

c5186d7

full pass done

a2d7521

nenadmilosevic95 requested review from cason and josef-widder as code owners February 24, 2025 13:00

cason requested changes Feb 24, 2025

View reviewed changes

cason reviewed Feb 24, 2025

View reviewed changes

Apply suggestions from code review

c3055a7

Co-authored-by: Daniel <daniel.cason@informal.systems> Signed-off-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>

josef-widder reviewed Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(spec/consensus): Small fixes to overview.md file. #873

fix(spec/consensus): Small fixes to overview.md file. #873

nenadmilosevic95 commented Feb 24, 2025

cason left a comment

cason Feb 24, 2025

cason Feb 24, 2025

josef-widder Feb 26, 2025

cason Feb 24, 2025

cason Feb 24, 2025

cason left a comment

cason Feb 24, 2025

cason Feb 24, 2025

cason Feb 24, 2025

josef-widder left a comment

josef-widder Feb 26, 2025

josef-widder Feb 26, 2025

josef-widder Feb 26, 2025

josef-widder Feb 26, 2025

josef-widder Feb 26, 2025

	completely independent executions. NM{Not sure I fully understand what do we want to say here?}
	communication-closed, that is, a message from some height `H` does not influence a process that is in height `H' != H`.

	consensus logic as they were duplicated messages. NM{Ok I guess this is the change you want to implement or have alrady implemented, that is different from the paper?}
	consensus logic as they were duplicated messages (note that doing this maintains safety, but leads to violations of the gossip property from the [Tendermint paper](https://arxiv.org/abs/1807.04938), and thus introduces liveness issues; see below).

fix(spec/consensus): Small fixes to overview.md file. #873

Are you sure you want to change the base?

fix(spec/consensus): Small fixes to overview.md file. #873

Conversation

nenadmilosevic95 commented Feb 24, 2025

PR author checklist

For all contributors

For external contributors

cason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josef-widder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment