FIP-0086 Include message-rebroadcast and skipping rounds #998

ranchalp · 2024-04-26T14:21:47Z

Follow design master issue at filecoin-project/go-f3#177

masih

Indentation in pseudocode looks uneven due to intermittent use if tab vs. space. A quick find/replace should get those sorted out. I recommend replacing them all with spaces.

I am worried that I might misinterpret the pseudocode because of the indentation. I'd be grateful if you can get the indentation fixed first before I leave too many silly comments 😊

FIPS/fip-0086.md

Co-authored-by: Masih H. Derkani <m@derkani.org>

ranchalp · 2024-05-03T13:35:27Z

@masih the indenting has been fixed. Note indenting is not perfect (at least in edit mode) because the \bottom special character occupies more than a regular character (not fully monospace).

anorth

More substantive technical questions in #809 (comment) and #809 (comment)

FIPS/fip-0086.md

anorth · 2024-05-06T23:29:14Z

FIPS/fip-0086.md

+
+45:     reBroadcast <COMMIT, value, instance, round, evidence>; trigger(timeout)
+46:     collect a clean set M of valid COMMIT messages from this round and instance
+           OR (NOT mayHaveStrongQuorum(value, round, step, 0) for all value ≠ 丄)


Can we just say has strong quorum of bottom, instead of does not have for any other value? I believe that's a necessary equivalent condition, and this result is relied on at 55. Looping over all values and checking is inelegant if we have this assumption.

Yes, thanks, good catch. Done.

anorth · 2024-05-06T23:32:50Z

FIPS/fip-0086.md

+47:     if (HasStrongQuorumValue(M) AND StrongQuorumValue(M) ≠ 丄)    \* decide
+48:       evidence ← Aggregate(M)
+49:       reBroadcast <DECIDE, StrongQuorumValue(M), instance, evidence>  \* break loop, wait for other DECIDE messages
+50:     if (∃ m ∈ M: m.value ≠ 丄 s.t. mayHaveStrongQuorum(m.value, r, COMMIT, 1/3)) \* m.value was possibly decided by others


As I noted while implementing, this is simpler to follow if the branches of this if are flipped. Test explicitly for strong quorum of bottom. If that is not found, take the non-buttom value that must exist in some message, with the assertion that it might have been decided by another participant.

Done, thanks.

anorth · 2024-05-06T23:33:26Z

FIPS/fip-0086.md

+
+45:     reBroadcast <COMMIT, value, instance, round, evidence>; trigger(timeout)
+46:     collect a clean set M of valid COMMIT messages from this round and instance
+           OR (NOT mayHaveStrongQuorum(value, round, step, 0) for all value ≠ 丄)


Suggested change

OR (NOT mayHaveStrongQuorum(value, round, step, 0) for all value ≠ 丄)

OR (NOT mayHaveStrongQuorum(value, round, COMMIT, 0) for all value ≠ 丄)

?

Wrote in a commit not to spam with a commit for this. Hence why not committing the suggestion.

FIPS/fip-0086.md

anorth · 2024-05-07T02:00:08Z

FIPS/fip-0086.md

+OR (step = DECIDE                                         | valid DECIDE evidence
+      AND (∃ M: Power(M)>⅔ AND evidence=Aggregate(M)      | is a strong quorum 
+        AND ∀ m’ ∈ M: m’.step = COMMIT                    | of COMMIT messages
+        AND ∀ m’ ∈ M: m’.round = round                    | from the same round 


DECIDE round is always zero, so this never passes. This instead wants to say the COMMIT messages all have the same round as each other.

(I thought we fixed this once already...)

Good catch, thanks. And yes, it sounds just like what we discussed at some point, but it is part of the modifications in this PR 🤔

Traced back the reintroduction of this typo to #985 . Went through the changes there and could not find any other typo being reintroduced, other than using Power(M)>2/3 instead of IsStrongQuorum (and fixed that in this PR too).

Co-authored-by: Alex North <445306+anorth@users.noreply.github.com>

ranchalp · 2024-05-07T08:59:26Z

Thanks for the review @anorth , the changes have been added.

jsoares

Editorial review. Looks good apart from some formatting issues. Did some checking against the Google doc and they seem consistent -- however, due to some changes in notation and structure, I may have missed something.

jsoares · 2024-05-09T14:08:03Z

FIPS/fip-0086.md

+
+12:   while (step != DECIDE)  {
+13:      if (round = 0)
+14:       BEBroadcast <QUALITY, value, instance>; trigger (timeout)


The indentation step remains inconsistent, also on formatted preview. Can we standardise on 2 spaces per level or something alike?

anorth · 2024-05-09T22:44:42Z

I made my comments on the new v3 version in the Google doc which was published at the same time, for faster/easier initial discussion.

ranchalp · 2024-05-10T07:27:07Z

Went through you comments in the doc and applied most of the changes both here and there. @anorth A couple of standing comments were left in the doc with replies.

anorth

Thanks, the major items I raised in discussion are now addressed. There are parts where this spec is much too concrete and doesn't appear to allow implementers much freedom to do it better. I think these would be better if made more abstract and directly describing the properties required, rather than how to achieve them.

There's lots of inconsistent indentation in the pseudocode. Can we stick to 2 spaces?

FIPS/fip-0086.md

…n to 2 spaces

vukolic · 2024-05-20T14:24:57Z

@anorth your last batch of comments is hopefully now fixed + I polished the indentation

FIPS/fip-0086.md

anorth

Thanks, changes LGTM now

jennijuju · 2024-05-21T03:51:28Z

@masih / @anorth, can one of you resolve the conflict so we can merge this?

# Conflicts: # FIPS/fip-0086.md

masih · 2024-05-21T09:25:35Z

Conflicts resolved.

ranchalp added 4 commits April 26, 2024 16:11

Include message-rebroadcast and skipping rounds

6c051aa

Typo

d2c1abf

Bit of text

f1353c8

Fix line numbering

01e1a10

ranchalp mentioned this pull request Apr 29, 2024

Design questions left for FIP filecoin-project/go-f3#177

Closed

ranchalp added 4 commits April 29, 2024 11:19

Fix typo and line numbering again

34e6f49

Fix comment

969f913

Fix comment

9498531

Merge branch 'master' into message-rebroadcast

9204e51

ranchalp marked this pull request as ready for review May 3, 2024 08:23

ranchalp requested review from momack2, arajasek, jennijuju, kaitlin-beegle, anorth, raulk, jsoares and TippyFlitsUK as code owners May 3, 2024 08:23

jsoares requested review from Stebalien, Kubuxu and masih May 3, 2024 10:59

masih reviewed May 3, 2024

View reviewed changes

FIPS/fip-0086.md Outdated Show resolved Hide resolved

FIPS/fip-0086.md Outdated Show resolved Hide resolved

FIPS/fip-0086.md Outdated Show resolved Hide resolved

ranchalp and others added 4 commits May 3, 2024 15:13

Update FIPS/fip-0086.md

2d68ce7

Co-authored-by: Masih H. Derkani <m@derkani.org>

Update FIPS/fip-0086.md

0cab77a

Co-authored-by: Masih H. Derkani <m@derkani.org>

Update FIPS/fip-0086.md

9cfd267

Co-authored-by: Masih H. Derkani <m@derkani.org>

Fix indenting

552d411

anorth reviewed May 6, 2024

View reviewed changes

anorth reviewed May 7, 2024

View reviewed changes

Update FIPS/fip-0086.md

e5d9931

Co-authored-by: Alex North <445306+anorth@users.noreply.github.com>

ranchalp added 4 commits May 7, 2024 10:38

introduce backOffExponent

0f46ffd

Update COMMIT unblocking rule

fb378e6

flip if statements

abb5502

Fix bug in DECIDE message validation

77161fa

ranchalp added 2 commits May 7, 2024 11:05

Update Power(M)>2/3 to IsStrongQuorum

80bbd09

Latest changes discussed in sync

f8a91dd

jsoares approved these changes May 9, 2024

View reviewed changes

Update fip-0086.md

63114eb

anorth reviewed May 17, 2024

View reviewed changes

FIPS/fip-0086.md Outdated Show resolved Hide resolved

FIPS/fip-0086.md Outdated Show resolved Hide resolved

FIPS/fip-0086.md Outdated Show resolved Hide resolved

FIPS/fip-0086.md Outdated Show resolved Hide resolved

FIPS/fip-0086.md Outdated Show resolved Hide resolved

addressed anorth's filecoin-project#998 (review) and fixed indentatio…

2981dfb

…n to 2 spaces

masih reviewed May 20, 2024

View reviewed changes

FIPS/fip-0086.md Outdated Show resolved Hide resolved

addresses filecoin-project#998 (comment)

f1203ae

anorth approved these changes May 20, 2024

View reviewed changes

Merge branch 'refs/heads/master' into fork/message-rebroadcast

f5e6dc3

# Conflicts: # FIPS/fip-0086.md

Fix reference to line number for Valid predicate

d6c336b

Kubuxu approved these changes May 21, 2024

View reviewed changes

jennijuju merged commit 32f4e59 into filecoin-project:master May 22, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIP-0086 Include message-rebroadcast and skipping rounds #998

FIP-0086 Include message-rebroadcast and skipping rounds #998

ranchalp commented Apr 26, 2024 •

edited

Loading

masih left a comment

ranchalp commented May 3, 2024

anorth left a comment

anorth May 6, 2024

ranchalp May 7, 2024

anorth May 6, 2024

ranchalp May 7, 2024

anorth May 6, 2024

ranchalp May 7, 2024

anorth May 7, 2024

ranchalp May 7, 2024

ranchalp May 7, 2024

ranchalp commented May 7, 2024

jsoares left a comment

jsoares May 9, 2024

anorth commented May 9, 2024

ranchalp commented May 10, 2024 •

edited

Loading

anorth left a comment

vukolic commented May 20, 2024

anorth left a comment

jennijuju commented May 21, 2024

masih commented May 21, 2024

	OR (NOT mayHaveStrongQuorum(value, round, step, 0) for all value ≠ 丄)
	OR (NOT mayHaveStrongQuorum(value, round, COMMIT, 0) for all value ≠ 丄)

FIP-0086 Include message-rebroadcast and skipping rounds #998

FIP-0086 Include message-rebroadcast and skipping rounds #998

Conversation

ranchalp commented Apr 26, 2024 • edited Loading

masih left a comment

Choose a reason for hiding this comment

ranchalp commented May 3, 2024

anorth 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

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

ranchalp commented May 7, 2024

jsoares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth commented May 9, 2024

ranchalp commented May 10, 2024 • edited Loading

anorth left a comment

Choose a reason for hiding this comment

vukolic commented May 20, 2024

anorth left a comment

Choose a reason for hiding this comment

jennijuju commented May 21, 2024

masih commented May 21, 2024

ranchalp commented Apr 26, 2024 •

edited

Loading

ranchalp commented May 10, 2024 •

edited

Loading