[pipeline/minimmit] Fix typos/misprints #1151
Conversation
|
Added #1153 |
| // Select the leader for view `v` | ||
| fn leader(v) -> L; | ||
| // Select the leader replica for view `v` | ||
| fn leader(v) -> r; |
There was a problem hiding this comment.
| fn leader(v) -> r; | |
| fn leader(v) -> R; |
There was a problem hiding this comment.
upper-case R is not mentioned anywhere anymore, any reason this can't be any arbitrary variable? r seems most appropriate
There was a problem hiding this comment.
It was in my PR - https://github.com/commonwarexyz/monorepo/pull/1153/files#diff-a6e0dc9f9fb285ab77cad5cacecb0427485dc55b7372dcb8927b59fa32476fbdR9
There is a set `R` of `n` replicas of which at most `f` are Byzantine, such that `n ≥ 5f + 1`
There was a problem hiding this comment.
Yeah I ended up deleting that part since it was used ONLY here and I didn't feel it clarified for the cost of introducing another "variable"
There was a problem hiding this comment.
fn leader(v) -> r and, for example, fn select_parent(r, v) and many other statements with r as an element should not be used together, since then the meaning of -> and r is then misleading
There was a problem hiding this comment.
I understand the frustration with the inconsistency between using variable r as the return "value" and bool elsewhere as what appears to be the return "type". I think that we may choose to fix that later. I'm sure there are several places in the writeup where we are not using "proper" notation.
Despite that, I personally still feel that this is currently the clearest version that we can get to with reasonably minimal changes. I don't feel that this should confuse people
| if notarization(c', i) ∈ r.proofs[i] { | ||
| return (c', i); // If there are multiple, pick any |
There was a problem hiding this comment.
This whole function remains one of the most awkward... but I think something closer to the original version remains the clearest (and has parallel structure to the below line if nullification(i) ∈ r.proofs[i] {).
Lastly there was kind of a bug before where genesis had an unspecified view, then had a view of 0. Rather than changing the rest of the spec to start at view 1, I simply said above genesis has a view of ⊥ < 0
| return (⊥, ⊥); // No proofs for view i, cannot proceed | ||
| } | ||
| return genesis; | ||
| return (genesis, ⊥); |
There was a problem hiding this comment.
In our code, we start at 1 to avoid this ⊥ notation for genesis (which we consider view 0). Wonder if it makes to do the same here?
There was a problem hiding this comment.
I think this requires more debate and will leave as-is for now (for future consideration).
|
Will fix last round of nits and merge. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #1151 +/- ##
=======================================
Coverage 91.02% 91.02%
=======================================
Files 215 215
Lines 57457 57457
=======================================
Hits 52300 52300
Misses 5157 5157 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Fixes #1148