Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

PBTS: spec reorganization, summary of changes on README.md #399

Merged
merged 16 commits into from
Feb 4, 2022

Conversation

cason
Copy link

@cason cason commented Feb 2, 2022

No description provided.

Copy link
Contributor

@josef-widder josef-widder left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks. I just left two text suggestions to clarify the context a bit.

spec/consensus/proposer-based-timestamp/README.md Outdated Show resolved Hide resolved
spec/consensus/proposer-based-timestamp/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@williambanfield williambanfield left a comment

Choose a reason for hiding this comment

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

Some nits, but nothing major. A general question: Is there a value to keeping the old versions and drafts in the repo? They are accessible via the git history and, in my view the clutter the repository, making it a bit unclear. Additionally, is there a reason to continue using the _draft suffix for these files? As far as I can tell, this is no longer a draft.

spec/consensus/proposer-based-timestamp/README.md Outdated Show resolved Hide resolved
spec/consensus/proposer-based-timestamp/README.md Outdated Show resolved Hide resolved
spec/consensus/proposer-based-timestamp/README.md Outdated Show resolved Hide resolved
Assuming that the voting power controlled by Byzantine validators is bounded by `f`,
the cumulative voting power of any valid `Commit` set must be at least `2f+1`.
As a result, the timestamp computed by `BFTTime` is not influenced by Byzantine validators,
as the weighted median of `Commit` timestamps comes from the clock of a non-faulty validator.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand why this is true. If f of the voting power is controlled by byzantine validators, couldn't they shift the median by generating faulty timestamps?

Copy link
Author

Choose a reason for hiding this comment

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

The point here is: if Byzantine validators produce timestamps in the past (or the future), when the 2f + 1 timestamps are ordered, the f+1th timestamp will not be in the past (or future).

spec/consensus/proposer-based-timestamp/README.md Outdated Show resolved Hide resolved
spec/consensus/proposer-based-timestamp/README.md Outdated Show resolved Hide resolved
by simplifying the way this situation is handled,
from a recursive reasoning regarding valid blocks that are re-proposed.

The full solution is detailed and formalized in the [Protocol Specification][algorithm] document.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is linked right below, do we need to link it here as well?

Copy link
Author

Choose a reason for hiding this comment

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

No, ehehhe.

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
@cason
Copy link
Author

cason commented Feb 4, 2022

Some nits, but nothing major. A general question: Is there a value to keeping the old versions and drafts in the repo? They are accessible via the git history and, in my view the clutter the repository, making it a bit unclear. Additionally, is there a reason to continue using the _draft suffix for these files? As far as I can tell, this is no longer a draft.

I agree, I would remove the _draft suffix after the next round of reviews.

@josef-widder
Copy link
Contributor

Let's use the suffix _reviewed then.

@cason cason closed this Feb 4, 2022
@cason cason reopened this Feb 4, 2022
@josef-widder josef-widder merged commit 4fb99af into master Feb 4, 2022
@josef-widder josef-widder deleted the cason/pbts branch February 4, 2022 09:44
sergio-mena pushed a commit that referenced this pull request Feb 4, 2022
* PBTS: brief context and proposal added to README

* PBTS: summary of algorithmic solution added to README

* PBTS: Context section of README improved

* PBTS: fixing links and page titles

* PBTS: moved first drafts to v1/, links updated

* PBTS: added issues to README, link to arXiv PDF

* PBTS: brief context and proposal added to README

* PBTS: summary of algorithmic solution added to README

* PBTS: Context section of README improved

* PBTS: fixing links and page titles

* PBTS: moved first drafts to v1/, links updated

* PBTS: added issues to README, link to arXiv PDF

* Apply suggestions from code review

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>

* Fixing linting problems

Co-authored-by: Daniel Cason <cason@gandria>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
@sergio-mena
Copy link
Contributor

Folks, these are my notes while reading the PBTS spec before the meeting. So some of those comments are now obsolete after the discussion and clarifications. Still, I think those have some value in the sense those were the questions I had by just reading the spec and having little context on what PBTS is.

Don't hesitate to come back to me if something is not clear.

  • Overall comments:

    • What happens if MSGDELAY is too big? From the text, I understand it won't be corrected; but then doesn't it become too flexible for the whole lifetime of the system?

      More precisely, when discussing the properties (liveness in particular), references to "good periods" or "bad periods" are not included in the properties' definitions, albeit one reference to GST in a note. It gives the impression that the algorithm assumes a (somewhat) synchronous system (at least w.r.t. message delays). But then we have that timing assumptions affect safety

    • What if "synchronized clocks" assumption is broken in the following way: 2/3+ processes temporarily (a glitch?) produce a block far in the future. Then their clocks become synchronous. How do we recover from this?
      In other words: isn't it dangerous to assume "synchronized clocks" as a safety assumption?

      An idea: add a parameter "offset" to the implementation, so that a process's clock way in the past can still participate. [Note after the meeting: I see now that this may defeat the purpose of the check for MSGDELAY]

    • "Derived POL" concept is not totally clear to me after going through the problem statement. Maybe we can discuss about it.

  • README.md

    • Time-validitty <--> "timely predicate". Probably best to refer to it consistently
  • System model

    • PBTS-MSG-D.0: ts and t are used together in the inequality, aren't we mixing up real and measured time?
    • Timely Proof-of-Locks: (very minor) the initial definition should include the height to be non-ambiguous
    • PBTS-TIMELY-POL.1: the third condition follows from the definition of POL(v,r)
    • I'm confused by the definition of POL(v,r). "There exists", yes but what if all those PREVOTE messages are delayed for a long long time?
    • POLRound is not defined in the text
    • PBTS-DERIVED-POL.1:
      • the second condition follows from the definition of POL(v,r)
      • shouldn't you define r* as you did in PBTS-TIMELY-POL.1 ?
  • Protocol Specification

    • "when it becomes locked by the network": I'm bewildered by this sentence. The fact that the proposer receives 2/3+ prevotes does not make the value locked across the network... only at the proposer
    • PBTS-ALG-UPON-PROP.1: Although adding timely in the Upon's guard is correct, wouldn't it be more efficient (and maybe easier to understand) to add it in the "if"? Since we already received an untimely proposal, it's pointless to keep on waiting in this step as no other proposal is supposed to come.
    • Rules at Lines 28 - 33 remain unchanged: "... at least f + 1 processes judged v as timely". Shouldn't it read "... at least f+1 correct processes..." ?
  • Editorial

    • s/interest is to possibility of/interest is the possibility of/
    • s/when adjusted by the clocks/when adjusted by the clocks'/
    • s/cycle aligns the process local clock/cycle aligns the process' local clock/
    • s/is a process is equipped/is a process equipped/
    • s/be time a process/be the time a process/
    • s/do not simultaneous read/do not simultaneously read/
    • s/behind of the clock/behind the clock
    • s/has not being updated/has not been updated/
    • s/other rules remains unchanged/other rules remain unchanged/

@cason
Copy link
Author

cason commented Feb 7, 2022

A brief point here, before going further on @sergio-mena comments.

Yes, we assume a synchronous system with respect to the propagation of Proposal messages, which carry (in particular) the timestamp of a block. Moreover, the timestamp are generated from clocks that are assumed to be synchronized (all the times, again, a synchronous assumption).

While I do agree those are strong assumptions, they don't affect safety. The non-observance of the synchronous assumptions jeopardize liveness, as blocks will be arbitrarily rejected. Two blocks are not decided under any timing behavior, that is, even if (most) clocks are not synchronized and proposals take more than MSGDELAY to be delivered.

@sergio-mena
Copy link
Contributor

Thanks @cason for clarifying. It makes sense to me. We can keep safety out of the discussion.

evan-forbes pushed a commit to celestiaorg/spec that referenced this pull request Feb 10, 2022
…t#399)

* PBTS: brief context and proposal added to README

* PBTS: summary of algorithmic solution added to README

* PBTS: Context section of README improved

* PBTS: fixing links and page titles

* PBTS: moved first drafts to v1/, links updated

* PBTS: added issues to README, link to arXiv PDF

* PBTS: brief context and proposal added to README

* PBTS: summary of algorithmic solution added to README

* PBTS: Context section of README improved

* PBTS: fixing links and page titles

* PBTS: moved first drafts to v1/, links updated

* PBTS: added issues to README, link to arXiv PDF

* Apply suggestions from code review

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>

* Fixing linting problems

Co-authored-by: Daniel Cason <cason@gandria>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants