state: add an 'IsTimely' method to implement the 'timely' check for proposer-based timestamps by williambanfield · Pull Request #7170 · tendermint/tendermint

williambanfield · 2021-10-27T14:26:57Z

This pull requests implements the 'timely' as outlined in the proposer-based timestamps specification.

For a proposed block timestamp to be considered timely, it must satisfy the following inequality:

proposedBlockTime < validatorLocalTime + Precision + MsgDelay && proposedBlockTime > validatorLocaltime - Precision.

Where Precision and MsgDelay are system parameters where Precision bounds the acceptable clock drift between any two correct process and MsgDelay bounds the acceptable delay for full transmitting a message between any two processes.

part of #6942

…proposer-based timestamps

tychoish · 2021-10-27T14:48:28Z

state/state.go

+//
+// For more information on the meaning of 'timely', see the proposer-based timestamp specification:
+// https://github.com/tendermint/spec/tree/master/spec/consensus/proposer-based-timestamp
+func IsTimely(bt time.Time, lt time.Time, precision time.Duration, msgDelay time.Duration) bool {


is there a way that this can be attached as a method to an existing type?

Alternately, seems like there should be a POD type to wrap up these arguments, as "function which takes two time.Time and two time.Duration" would be very easy to flip one or both of the pairs of arguments in a way that would be difficult to notice at the call site.

Yeah good call. I believe this could be a method attached to the proposals themselves (if we first check IsTimely and then that the proposal time matches the received block time). To avoid confusion between precision and msgDelay it may be better to take the new TimeParams (or whatever it's called) from ConsensusParams as an argument.

I've moved this to be a method on the block (since that will be what timely is checked against) and moved the method to take in the timestamp params instead. I also added a mechansim for defining a local time source, that this method will take as an argument, but perhaps that is overkill and a time.Time would suffice instead.

I see the new method, but this code is still there

cmwaters · 2021-11-01T12:02:47Z

types/block.go

+//
+// For more information on the meaning of 'timely', see the proposer-based timestamp specification:
+// https://github.com/tendermint/spec/tree/master/spec/consensus/proposer-based-timestamp
+func (b *Block) IsTimely(clock tmtime.Source, p TimestampParams) bool {


Don't we measure timeliness when we receive the proposal or when we receive the block. I thought it was based on the proposal

After a discussion with @josef-widder and @cason, I determined that it's acceptable to use the propose message. This has the benefit of using a message with a constant size, meaning block size will not affect MSGDELAY.

In terms of the implementation, this also seems easier. If a Proposal is not timely, when it is received and processed at first by the consensus implementation, there is no point in "asking for" and receiving the corresponding BlockParts. In addition, if the timely call is made for the block, when in the implementation will it be called? This is relevant, as different calls to timely may return true and the false, depending on when they are invoked.

types/params.go

cmwaters · 2021-11-01T12:03:53Z

types/params.go

 }

+// TimestampParams influence the validity of block timestamps.
+type TimestampParams struct {


Do we want to add a default?

Set some basics and will follow up in #7202

These would be Synchronous or Timing parameters, wouldn't them?

cason

For me is not clear whether the corner cases are defined or tested properly. I also think that the main isTimely method could be easier to read, by using Before() and After() and Add() and Sub() time methods.

cason · 2021-11-11T13:26:41Z

types/params.go

+	// https://github.com/tendermint/tendermint/issues/7202
+	return TimestampParams{
+		Precision: 2 * time.Second,
+		Accuracy:  500 * time.Millisecond,


Observe that if Accuracy = 0.5ms, two clocks might differ by at most Precision = 1ms.

cason · 2021-11-11T13:33:21Z

types/proposal.go

+// configured Precision and MsgDelay parameters.
+// Specifically, a proposed block timestamp is considered timely if it is satisfies the following inequalities:
+//
+// proposedBlockTime > validatorLocaltime - Precision && proposedBlockTime < validatorLocalTime + Precision + MsgDelay.


proposedBlockTime > validatorLocaltime - Precision

Are you sure that this is correct?

I believe so, according to this description in the spec https://github.com/tendermint/spec/blob/master/spec/consensus/proposer-based-timestamp/pbts-algorithm_001_draft.md#pbts-reception-step0

cason · 2021-11-11T13:36:42Z

types/proposal.go

+	lt := clock.Now()
+	lhs := lt.Add(-tp.Precision)
+	rhs := lt.Add(tp.Precision).Add(tp.MsgDelay)
+	if lhs.Before(p.Timestamp) && p.Timestamp.Before(rhs) {


Maybe just a detail, but I would prefer having something like "a.After(b) and a.Before(c)" for more clarity.

cason · 2021-11-11T13:42:06Z

types/proposal_test.go

+		{
+			// Checking that the following inequality evaluates to true:
+			// 1 - 2 < 0 < 1 + 2 + 1
+			name:         "basic timely",


For me, the "basic" case, meaning the most common, is:

The proposal timestamp is "for sure" in the past, so: proposalTime + Precision < localTime - Precision

The receiving time of the proposal is "fore sure" within the acceptable window, so: localTime + Precision < proposalTime + msgDelay - Precision

cason · 2021-11-11T13:44:18Z

types/proposal_test.go

+		{
+			// Checking that the following inequality evaluates to false:
+			// 3 - 2 < 0 < 3 + 2 + 1
+			name:         "local time too large",


Well, is not the local time that is large, but the Proposal took too long to be received.

cason · 2021-11-11T13:45:02Z

types/proposal_test.go

+		{
+			// Checking that the following inequality evaluates to false:
+			// 0 - 2 < 2 < 2 + 1
+			name:         "proposal time too large",


Maybe a more clear name would be: "proposal timestamp in the future"

cason · 2021-11-11T13:45:56Z

types/proposal_test.go

+			proposalTime: genesisTime,
+			localTime:    genesisTime.Add(1 * time.Nanosecond),
+			precision:    time.Nanosecond * 2,
+			msgDelay:     time.Nanosecond,


I would expect msgDelay >> Precision.

williambanfield · 2021-11-12T14:47:37Z

For me is not clear whether the corner cases are defined or tested properly. I also think that the main isTimely method could be easier to read, by using Before() and After() and Add() and Sub() time methods.

.Sub produces a time.Duration, whereas what we need is a time.Time. Add produces a time.Time, another sort of annoying difference in similarly named functions.

…roposer-based timestamps (#7170) * state: add an IsTimely function to implement the check for timely in proposer-based timestamps * move time checks into block.go and add time source mechanism * timestamp params comment * add todo related to pbts spec and timestamp params * remove old istimely * switch to using built in before function * lint++ * wip * move into proposal and create a default set of params * defer using default cons params for now

state: add an IsTimely function to implement the check for timely in …

c76c078

…proposer-based timestamps

williambanfield requested review from cmwaters, creachadair, ebuchman and tychoish as code owners October 27, 2021 14:26

williambanfield changed the base branch from master to wb/proposer-based-timestamps October 27, 2021 14:27

williambanfield requested review from alexanderbez and tessr as code owners October 27, 2021 14:27

tychoish reviewed Oct 27, 2021

View reviewed changes

williambanfield added 4 commits October 28, 2021 11:29

move time checks into block.go and add time source mechanism

f03da27

timestamp params comment

edc7f97

add todo related to pbts spec and timestamp params

81cf40a

remove old istimely

7180750

tychoish approved these changes Nov 1, 2021

View reviewed changes

williambanfield added 2 commits November 1, 2021 12:04

switch to using built in before function

ee3a25b

lint++

7ca915a

cmwaters reviewed Nov 1, 2021

View reviewed changes

williambanfield added the S:automerge Automatically merge PR when requirements pass label Nov 1, 2021

cmwaters reviewed Nov 1, 2021

View reviewed changes

types/params.go Outdated Show resolved Hide resolved

cmwaters reviewed Nov 1, 2021

View reviewed changes

williambanfield added 3 commits November 2, 2021 16:16

wip

0cfe951

move into proposal and create a default set of params

f4040af

defer using default cons params for now

75e29de

williambanfield merged commit 0d2a5de into wb/proposer-based-timestamps Nov 9, 2021

williambanfield deleted the wb/is-timely branch November 9, 2021 20:23

cason reviewed Nov 11, 2021

View reviewed changes

williambanfield restored the wb/is-timely branch September 9, 2022 16:12

sergio-mena mentioned this pull request Dec 7, 2023

consensus: proposer-based timestamps (PBTS) cometbft/cometbft#1731

Closed

30 tasks

sergio-mena mentioned this pull request Jan 12, 2024

Forward port work present in branch wb/proposer-based-timestamps cometbft/cometbft#2003

Closed

1 task

Conversation

williambanfield commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williambanfield commented Nov 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

williambanfield commented Oct 27, 2021 •

edited

Loading