Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reintroduce timestamp checks #1724

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions consensus/src/commons.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub struct RoundUpdate {
seed: Seed,
hash: [u8; 32],
cert: Certificate,
timestamp: u64,

pub base_timeouts: TimeoutSet,
}
Expand All @@ -53,6 +54,7 @@ impl RoundUpdate {
cert: mrb_header.cert,
hash: mrb_header.hash,
seed: mrb_header.seed,
timestamp: mrb_header.timestamp,
base_timeouts,
}
}
Expand All @@ -68,6 +70,10 @@ impl RoundUpdate {
pub fn cert(&self) -> &Certificate {
&self.cert
}

pub fn timestamp(&self) -> u64 {
self.timestamp
}
}

#[derive(Debug, Clone, Copy, Error)]
Expand Down Expand Up @@ -100,6 +106,7 @@ pub enum ConsensusError {
InvalidQuorumType,
InvalidVote(Vote),
InvalidMsgIteration(u8),
InvalidTimestamp,
FutureEvent,
PastEvent,
NotCommitteeMember,
Expand Down
18 changes: 16 additions & 2 deletions consensus/src/proposal/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
//
// Copyright (c) DUSK NETWORK. All rights reserved.

use crate::commons::{ConsensusError, Database, RoundUpdate};
use crate::commons::{
get_current_timestamp, ConsensusError, Database, RoundUpdate,
};
use crate::merkle::merkle_root;
use crate::msg_handler::{HandleMsgOutput, MsgHandler};
use crate::user::committee::Committee;
Expand Down Expand Up @@ -42,11 +44,16 @@ impl<D: Database> MsgHandler for ProposalHandler<D> {
async fn collect(
&mut self,
msg: Message,
_ru: &RoundUpdate,
ru: &RoundUpdate,
_committee: &Committee,
) -> Result<HandleMsgOutput, ConsensusError> {
// store candidate block
let p = Self::unwrap_msg(&msg)?;

if p.candidate.header().timestamp < ru.timestamp() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return Err(ConsensusError::InvalidTimestamp);
}

self.db
.lock()
.await
Expand Down Expand Up @@ -84,10 +91,17 @@ impl<D: Database> ProposalHandler<D> {
// Verify new_block msg signature
p.verify_signature()?;

// We only check the consistency of the declared hash.
// The actual bound with the tip.block_hash is done by
// MsgHandler.is_valid()
if msg.header.prev_block_hash != p.candidate.header().prev_block_hash {
return Err(ConsensusError::InvalidBlockHash);
}

if p.candidate.header().timestamp > get_current_timestamp() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can be more error-tolerant here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

return Err(ConsensusError::InvalidTimestamp);
}

let tx_hashes: Vec<[u8; 32]> =
p.candidate.txs().iter().map(|t| t.hash()).collect();
let tx_root = merkle_root(&tx_hashes[..]);
Expand Down
35 changes: 29 additions & 6 deletions node/src/chain/header_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ use crate::database;
use crate::database::Ledger;
use anyhow::anyhow;
use dusk_bytes::Serializable;
use dusk_consensus::commons::get_current_timestamp;
use dusk_consensus::config::{MAX_STEP_TIMEOUT, RELAX_ITERATION_THRESHOLD};
use dusk_consensus::quorum::verifiers;
use dusk_consensus::quorum::verifiers::QuorumResult;
use dusk_consensus::user::committee::CommitteeSet;
Expand Down Expand Up @@ -78,29 +80,50 @@ impl<'a, DB: database::DB> Validator<'a, DB> {
candidate_block: &'a ledger::Header,
) -> anyhow::Result<()> {
if candidate_block.version > 0 {
return Err(anyhow!("unsupported block version"));
anyhow::bail!("unsupported block version");
}

if candidate_block.hash == [0u8; 32] {
return Err(anyhow!("empty block hash"));
anyhow::bail!("empty block hash");
}

if candidate_block.height != self.prev_header.height + 1 {
return Err(anyhow!(
anyhow::bail!(
"invalid block height block_height: {:?}, curr_height: {:?}",
candidate_block.height,
self.prev_header.height,
));
);
}

if candidate_block.prev_block_hash != self.prev_header.hash {
return Err(anyhow!("invalid previous block hash"));
anyhow::bail!("invalid previous block hash");
}

if candidate_block.timestamp > get_current_timestamp() {
anyhow::bail!("invalid future timestamp");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

if candidate_block.timestamp < self.prev_header.timestamp {
anyhow::bail!("invalid timestamp");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

if candidate_block.iteration < RELAX_ITERATION_THRESHOLD {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

max_delta is depending to the iteration

Indeed is MAX_ITERATION_TIMEOUT*ITERATION

let max_delta = candidate_block.iteration as u64
* MAX_STEP_TIMEOUT.as_secs()
* 3;
let current_delta =
candidate_block.timestamp - self.prev_header.timestamp;
if current_delta > max_delta {
anyhow::bail!(
"invalid timestamp, delta: {current_delta}/{max_delta}"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

);
}
}

// Ensure block is not already in the ledger
self.db.read().await.view(|v| {
if Ledger::get_block_exists(&v, &candidate_block.hash)? {
return Err(anyhow!("block already exists"));
anyhow::bail!("block already exists");
}

Ok(())
Expand Down
Loading