search

parallelchain-io / hotstuff_rs

Rust implementation of the HotStuff consensus algorithm.
39 stars 5 forks source link

Three arms in `<QuorumCertificate as Certificate>::is_correct` should be unreachable #46

Open lyulka opened 3 months ago

lyulka commented 3 months ago

Background

Arm 1 Arm 2 Arm 3

Proposed solution

Investigate whether these case should really be reachable. Is there really a case where a block whose height is set as update_height could be marked in a replica's own block tree as having no validator state updates?

If these cases are truly unreachable, then these arms should execute unreachable!()

lyulka commented 3 months ago

Is there really a case...

Actually, maybe there is such a case.

The key point here is that just because a block's height is equal to update_height, doesn't mean that this block is exactly the block that triggered the update. Because QuorumCertificate::is_correct is called before safe_qc or safe_block is evaluated, self in is_correct could be a QC created by a Byzantine adversary that justifies a block which doesn't exist, and therefore is found to have ValidatorSetUpdatesStatus::None (because the block tree does not distinguish the case where a ValidatorSetUpdatesStatus does not exist in the KV store and the case where it does exist and its variant is ValidatorSetUpdatesStatus::None).

lyulka commented 3 months ago

that justifies a block which doesn't exist

But if the block does not exist, then we will enter line 53. If we do not enter this arm, then actually the block does exist in the block tree, and is marked ValidatorSetUpdatesStatus::None.

In summary

I think we should defer dealing with this potential bug until we have merged #44 and overhauled the state module, since this overhaul may entail changes to how we return ValidatorSetUpdatesStatus (maybe in the future, we will distinguish between None and Some(ValidatorSetUpdatesStatus::None)).