Reintroduce timestamp checks #1724

herr-seppia · 2024-05-10T14:09:05Z

Added the following checks:

Timestamp of block with height=H must be greater or equal to timestamp of block with height=H-1
Timestamp of any block must be lower or equal than current OS timestamp
Timestamp of block with height=H generated at iteration=I must be lower or equal to the timestamp of block with height=H-1 plus I*MAX_ITER_TIMEOUT ¹

See also #1649
See also dusk-network/dips#13

EDIT: This PR has to be repurposed to address #1494

This check is enforced only if the iteration I is not relaxed -> I < RELAX_ITERATION_THRESHOLD ↩

See also #1649 See also dusk-network/dips#13

fed-franz · 2024-06-03T14:16:55Z

As far as I remember, we decided to reject the DIP proposal. And the #1649 is to avoid meaningless timestamps (e.g., as stated in the issue: avoid blocks from one day in the future) not to introduce strict timestamp conditions on blocks.

fed-franz

These checks are very strict. Didn't we agree on leaving some margin of error (i.e. the slippage)?

Also, we should probably make sure all nodes use NTP, or are synced in some way, before enforcing these checks on mainnet (see dusk-network/dips#12).

fed-franz · 2024-06-03T13:54:36Z

node/src/chain/header_validation.rs

+        }
+
+        if candidate_block.timestamp < self.prev_header.timestamp {
+            anyhow::bail!("invalid timestamp");


Suggested change

anyhow::bail!("invalid timestamp");

anyhow::bail!("invalid timestamp: lower than previous block");

fed-franz · 2024-06-03T13:57:40Z

node/src/chain/header_validation.rs

+        }
+
+        if candidate_block.timestamp > get_current_timestamp() {
+            anyhow::bail!("invalid future timestamp");


Suggested change

anyhow::bail!("invalid future timestamp");

anyhow::bail!("invalid timestamp: candidate from the future");

fed-franz · 2024-06-03T14:02:59Z

node/src/chain/header_validation.rs

+                candidate_block.timestamp - self.prev_header.timestamp;
+            if current_delta > max_delta {
+                anyhow::bail!(
+                    "invalid timestamp, delta: {current_delta}/{max_delta}"


Suggested change

"invalid timestamp, delta: {current_delta}/{max_delta}"

"invalid timestamp: candidate is too late (delta: {current_delta}/{max_delta})"

Shouldn't we check against the current timestamp (or better, the time at which the candidate was received), instead of the timestamp in the candidate?

Why? If I check the current timestamp the candidate timestamp can be invalid (checks for current timestamp/iteration are natives performed by the iteration timeout mechanism).
Timestamp of candidate should always be lower than the receiving timestamp.

Furthermore this check is performed for any block, not only for live votes

My reasoning was that a candidate that is maliciously sent at a later time could still pass this check by setting an appropriate timestamp. But I guess in non-relaxed iterations, the candidate would be equally rejected due to step timeout.

Still, I'm not sure about the benefit of this check on Candidate messages...

Having this check while accepting the candidate, will reject the candidate itself and let the provisioner to not vote (even if the provisioner has some iteration delay)

goshawk-3

For my benefit, can someone explain how this PR is addressing consensus forks issues? We used to have consensus forks before removing the block timestamp too.

goshawk-3 · 2024-06-04T07:09:22Z

consensus/src/proposal/handler.rs

        if msg.header.prev_block_hash != p.candidate.header().prev_block_hash {
            return Err(ConsensusError::InvalidBlockHash);
        }

+        if p.candidate.header().timestamp > get_current_timestamp() {


Maybe we can be more error-tolerant here?

What do you suggest as a fair more tolerant check?

Having a more tolerant check here will introduce some delay effect in the chain.
Eg: if we accept block 5 mins from the future, that mean that the next block cannot be produced for the next 5 mins due to sequential check

goshawk-3 · 2024-06-04T07:20:26Z

consensus/src/proposal/handler.rs

        _committee: &Committee,
    ) -> Result<HandleMsgOutput, ConsensusError> {
        // store candidate block
        let p = Self::unwrap_msg(&msg)?;
+
+        if p.candidate.header().timestamp < ru.timestamp() {


Do you have any intention by putting this check here instead of ProposalHandler::verify(? If so, let's have it as a comment.

It should be the same, since both will discard the block. I'm not strongly opinionated on this

If ProposalHandler::verify( considers it valid (returns true) the message is rebroadcast. (if that is the intention then it's fine)

goshawk-3 · 2024-06-04T07:27:14Z

node/src/chain/header_validation.rs

+            anyhow::bail!("invalid timestamp");
+        }
+
+        if candidate_block.iteration < RELAX_ITERATION_THRESHOLD {


If we have a candidate above RELAX_ITERATION_THRESHOLD, shouldn't we still have some max_delta?

Relaxed iterations have no time limits, they move on to the next step only when the current step reaches a result.
In the case of Proposal, the step waits indefinitely for a Candidate block.
So there shouldn't be any timestamp check beyond the condition of being subsequent to the previous block

max_delta is depending to the iteration

Indeed is MAX_ITERATION_TIMEOUT*ITERATION

goshawk-3 · 2024-06-04T07:39:19Z

These checks are very strict. Didn't we agree on leaving some margin of error (i.e. the slippage)?

Also, we should probably make sure all nodes use NTP, or are synced in some way, before enforcing these checks on mainnet (see dusk-network/dips#12).

I agree with that.
If we intend to merge this PR into master before enabling NTP, I'd like to request a long-run devnet test where we ensure consensus is stable without NTP enabled.

herr-seppia · 2024-06-04T09:01:39Z

Also, we should probably make sure all nodes use NTP

I agree with that. If we intend to merge this PR into master before enabling NTP

How can rusk binary be sure that NTPd is enabled on the host OS?

fed-franz · 2024-06-04T09:08:43Z

For my benefit, can someone explain how this PR is addressing consensus forks issues? We used to have consensus forks before removing the block timestamp too.

The original intent of (re)introducing some timestamp check on consensus messages was to deal with the "Nothing at stake" problem, where we needed to determine some temporal order of actions (of the same provisioner) on parallel branches.
While this goal has been dismissed, we decided to add some constraints to candidate timestamps as a way of mitigate cases where the candidate is published very late compared to when the iteration occurs.

However, as I commented, I'm not sure about the actual benefit of these checks.
If @herr-seppia has it clear maybe leave a comment about that. Otherwise we might want to have a second round of discussion on this

herr-seppia · 2024-06-04T09:16:49Z

For my benefit, can someone explain how this PR is addressing consensus forks issues?

This PR is not completely addressing that, but it should (IMO) mitigate it.
Right now "forks" happen when delayed provisioner(s) vote for block after a while (2-3 minutes)

It will mitigate it because delayed provisioners will not vote for candidates anymore if the MAX_DELAY has been reached.

We used to have consensus forks before removing the block timestamp too.

Checks we used to have in the past were not addressing this, they were just checking if a block timestamp was too much from the future and then edit it to be no more than 5 mins

fed-franz · 2024-06-04T10:08:32Z

This PR is not completely addressing that, but it should (IMO) mitigate it. Right now "forks" happen when delayed provisioner(s) vote for block after a while (2-3 minutes)

It will mitigate it because delayed provisioners will not vote for candidates anymore if the MAX_DELAY has been reached.

The problem is that the check is done on the timestamp included in the candidate. So, if the candidate was produced in time, late voters will still vote for it. That's why I was suggesting to compare against the current timestamp instead of the candidate's one.
Alternatively, like stated in the original proposal, we should check the vote's timestamp against the candidate's one.

autholykos · 2024-06-11T08:06:43Z

Solves #1494

herr-seppia · 2024-07-11T07:01:14Z

Closing as obsolete

fed-franz · 2024-07-11T07:17:05Z

Can we delete the branch?

herr-seppia added 4 commits June 3, 2024 15:28

consensus: add block timestamp sequence checks

64e9fa0

See also #1649 See also dusk-network/dips#13

node: ensure candidate timestamp are from the past

07761bf

See also #1649 See also dusk-network/dips#13

node: add candidate iteration timestamp checks

30f5959

node: refactor to use anyhow::bail

55ef85a

herr-seppia force-pushed the timestamp_checks branch from 769854f to 55ef85a Compare June 3, 2024 13:28

herr-seppia marked this pull request as ready for review June 3, 2024 13:39

herr-seppia requested review from autholykos, fed-franz and goshawk-3 June 3, 2024 13:39

fed-franz requested changes Jun 3, 2024

View reviewed changes

goshawk-3 suggested changes Jun 4, 2024

View reviewed changes

fed-franz linked an issue Jun 11, 2024 that may be closed by this pull request

consensus: Increase block time #1494

Closed

herr-seppia closed this Jul 11, 2024

herr-seppia deleted the timestamp_checks branch July 11, 2024 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce timestamp checks #1724

Reintroduce timestamp checks #1724

herr-seppia commented May 10, 2024 •

edited by fed-franz

Loading

fed-franz commented Jun 3, 2024

fed-franz left a comment

fed-franz Jun 3, 2024

fed-franz Jun 3, 2024

fed-franz Jun 3, 2024

fed-franz Jun 3, 2024

herr-seppia Jun 3, 2024

fed-franz Jun 3, 2024

fed-franz Jun 3, 2024

herr-seppia Jun 4, 2024

goshawk-3 left a comment

goshawk-3 Jun 4, 2024

herr-seppia Jun 4, 2024

goshawk-3 Jun 4, 2024

herr-seppia Jun 4, 2024

goshawk-3 Jun 4, 2024

goshawk-3 Jun 4, 2024

fed-franz Jun 4, 2024

herr-seppia Jun 4, 2024

goshawk-3 commented Jun 4, 2024

herr-seppia commented Jun 4, 2024

fed-franz commented Jun 4, 2024

herr-seppia commented Jun 4, 2024 •

edited

Loading

fed-franz commented Jun 4, 2024

autholykos commented Jun 11, 2024

herr-seppia commented Jul 11, 2024

fed-franz commented Jul 11, 2024

	anyhow::bail!("invalid timestamp");
	anyhow::bail!("invalid timestamp: lower than previous block");

	anyhow::bail!("invalid future timestamp");
	anyhow::bail!("invalid timestamp: candidate from the future");

	"invalid timestamp, delta: {current_delta}/{max_delta}"
	"invalid timestamp: candidate is too late (delta: {current_delta}/{max_delta})"

Reintroduce timestamp checks #1724

Reintroduce timestamp checks #1724

Conversation

herr-seppia commented May 10, 2024 • edited by fed-franz Loading

Footnotes

fed-franz commented Jun 3, 2024

fed-franz 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

goshawk-3 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

goshawk-3 commented Jun 4, 2024

herr-seppia commented Jun 4, 2024

fed-franz commented Jun 4, 2024

herr-seppia commented Jun 4, 2024 • edited Loading

fed-franz commented Jun 4, 2024

autholykos commented Jun 11, 2024

herr-seppia commented Jul 11, 2024

fed-franz commented Jul 11, 2024

herr-seppia commented May 10, 2024 •

edited by fed-franz

Loading

herr-seppia commented Jun 4, 2024 •

edited

Loading