Skip to content

Commit

Permalink
removed header from FinalBlock
Browse files Browse the repository at this point in the history
  • Loading branch information
pompon0 committed Dec 19, 2023
1 parent 50342c1 commit 5963f69
Show file tree
Hide file tree
Showing 15 changed files with 60 additions and 93 deletions.
5 changes: 2 additions & 3 deletions node/actors/bft/src/replica/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,13 @@ impl StateMachine {
return Ok(());
};
let block = validator::FinalBlock {
header: commit_qc.message.proposal,
payload: payload.clone(),
justification: commit_qc.clone(),
};

info!(
"Finalized a block!\nFinal block: {:#?}",
block.header.hash()
block.header().hash()
);
self.storage
.put_block(ctx, &block)
Expand All @@ -43,7 +42,7 @@ impl StateMachine {

let number_metric = &crate::metrics::METRICS.finalized_block_number;
let current_number = number_metric.get();
number_metric.set(current_number.max(block.header.number.0));
number_metric.set(current_number.max(block.header().number.0));
Ok(())
}
}
1 change: 0 additions & 1 deletion node/actors/bft/src/replica/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ async fn leader_prepare_invalid_payload() {
// Default implementation of verify_payload() fails if
// head block number >= proposal block number.
let block = validator::FinalBlock {
header: leader_prepare.msg.proposal,
payload: leader_prepare.msg.proposal_payload.clone().unwrap(),
justification: CommitQC::from(
&[util.keys[0].sign_msg(ReplicaCommit {
Expand Down
1 change: 0 additions & 1 deletion node/actors/bft/src/testonly/make.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ pub fn make_genesis(
})
.collect();
let final_block = validator::FinalBlock {
header,
payload,
justification: validator::CommitQC::from(&signed_messages, &validator_set).unwrap(),
};
Expand Down
6 changes: 3 additions & 3 deletions node/actors/executor/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ impl ValidatorNode {
iter::successors(Some(genesis_block), |parent| {
let payload: Payload = rng.gen();
let header = validator::BlockHeader {
parent: parent.header.hash(),
number: parent.header.number.next(),
parent: parent.header().hash(),
number: parent.header().number.next(),
payload: payload.hash(),
};
let commit = self.validator.key.sign_msg(validator::ReplicaCommit {
Expand All @@ -41,7 +41,7 @@ impl ValidatorNode {
proposal: header,
});
let justification = validator::CommitQC::from(&[commit], validators).unwrap();
Some(FinalBlock::new(header, payload, justification))
Some(FinalBlock::new(payload, justification))
})
}

Expand Down
6 changes: 3 additions & 3 deletions node/actors/sync_blocks/src/peers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl PeerStates {

scope::run!(ctx, |ctx, s| async {
let start_number = storage.last_contiguous_block_number(ctx).await?;
let mut last_block_number = storage.head_block(ctx).await?.header.number;
let mut last_block_number = storage.head_block(ctx).await?.header().number;
let missing_blocks = storage
.missing_block_numbers(ctx, start_number..last_block_number)
.await?;
Expand Down Expand Up @@ -443,10 +443,10 @@ impl PeerStates {
block_number: BlockNumber,
block: &FinalBlock,
) -> Result<(), BlockValidationError> {
if block.header.number != block_number {
if block.header().number != block_number {
let err = anyhow::anyhow!(
"block does not have requested number (requested: {block_number}, got: {})",
block.header.number
block.header().number
);
return Err(BlockValidationError::Other(err));
}
Expand Down
9 changes: 1 addition & 8 deletions node/actors/sync_blocks/src/peers/tests/fakes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,6 @@ async fn processing_invalid_blocks() {
.unwrap_err();
assert_matches!(err, BlockValidationError::Other(_));

let mut invalid_block = test_validators.final_blocks[1].clone();
invalid_block.justification = test_validators.final_blocks[0].justification.clone();
let err = peer_states
.validate_block(BlockNumber(1), &invalid_block)
.unwrap_err();
assert_matches!(err, BlockValidationError::ProposalMismatch { .. });

let mut invalid_block = test_validators.final_blocks[1].clone();
invalid_block.payload = validator::Payload(b"invalid".to_vec());
let err = peer_states
Expand Down Expand Up @@ -146,7 +139,7 @@ impl Test for PeerWithFakeBlock {
assert_eq!(number, BlockNumber(1));

let mut fake_block = test_validators.final_blocks[2].clone();
fake_block.header.number = BlockNumber(1);
fake_block.justification.message.proposal.number = BlockNumber(1);
response.send(Ok(fake_block)).unwrap();

let peer_event = events_receiver.recv(ctx).await?;
Expand Down
9 changes: 4 additions & 5 deletions node/actors/sync_blocks/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ impl TestValidators {
let mut latest_block = BlockHeader::genesis(payload.hash(), BlockNumber(0));
let final_blocks = (0..block_count).map(|_| {
let final_block = FinalBlock {
header: latest_block,
payload: payload.clone(),
justification: this.certify_block(&latest_block),
};
Expand Down Expand Up @@ -122,9 +121,9 @@ async fn subscribing_to_state_updates() {
let rng = &mut ctx.rng();
let protocol_version = validator::ProtocolVersion::EARLIEST;
let genesis_block = make_genesis_block(rng, protocol_version);
let block_1 = make_block(rng, &genesis_block.header, protocol_version);
let block_2 = make_block(rng, &block_1.header, protocol_version);
let block_3 = make_block(rng, &block_2.header, protocol_version);
let block_1 = make_block(rng, genesis_block.header(), protocol_version);
let block_2 = make_block(rng, block_1.header(), protocol_version);
let block_3 = make_block(rng, block_2.header(), protocol_version);

let storage = InMemoryStorage::new(genesis_block.clone());
let storage = &Arc::new(storage);
Expand Down Expand Up @@ -202,7 +201,7 @@ async fn getting_blocks() {
let storage = InMemoryStorage::new(genesis_block.clone());
let storage = Arc::new(storage);
let blocks = iter::successors(Some(genesis_block), |parent| {
Some(make_block(rng, &parent.header, protocol_version))
Some(make_block(rng, parent.header(), protocol_version))
});
let blocks: Vec<_> = blocks.take(5).collect();
for block in &blocks {
Expand Down
5 changes: 2 additions & 3 deletions node/libs/roles/src/proto/validator.proto
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ message BlockHeader {
}

message FinalBlock {
optional BlockHeader header = 1; // required
optional bytes payload = 2; // required
optional CommitQC justification = 3; // required
optional bytes payload = 1; // required
optional CommitQC justification = 2; // required
}

message ConsensusMsg {
Expand Down
2 changes: 0 additions & 2 deletions node/libs/roles/src/validator/conv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,13 @@ impl ProtoFmt for FinalBlock {
type Proto = proto::FinalBlock;
fn read(r: &Self::Proto) -> anyhow::Result<Self> {
Ok(Self {
header: read_required(&r.header).context("header")?,
payload: Payload(required(&r.payload).context("payload")?.clone()),
justification: read_required(&r.justification).context("justification")?,
})
}

fn build(&self) -> Self::Proto {
Self::Proto {
header: Some(self.header.build()),
payload: Some(self.payload.0.clone()),
justification: Some(self.justification.build()),
}
Expand Down
39 changes: 11 additions & 28 deletions node/libs/roles/src/validator/messages/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ impl Payload {
}

/// Sequential number of the block.
/// Genesis block has number 0.
/// For other blocks: block.number = block.parent.number + 1.
/// Genesis block can have an arbitrary block number.
/// For blocks other than genesis: block.number = block.parent.number + 1.
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct BlockNumber(pub u64);

Expand Down Expand Up @@ -152,8 +152,6 @@ impl BlockHeader {
/// A block that has been finalized by the consensus protocol.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct FinalBlock {
/// Header of the block.
pub header: BlockHeader,
/// Payload of the block. Should match `header.payload` hash.
pub payload: Payload,
/// Justification for the block. What guarantees that the block is final.
Expand All @@ -162,36 +160,32 @@ pub struct FinalBlock {

impl FinalBlock {
/// Creates a new finalized block.
pub fn new(header: BlockHeader, payload: Payload, justification: CommitQC) -> Self {
assert_eq!(header.payload, payload.hash());
assert_eq!(header, justification.message.proposal);
pub fn new(payload: Payload, justification: CommitQC) -> Self {
assert_eq!(justification.message.proposal.payload, payload.hash());
Self {
header,
payload,
justification,
}
}

/// Header fo the block.
pub fn header(&self) -> &BlockHeader {
&self.justification.message.proposal
}

/// Validates internal consistency of this block.
pub fn validate(
&self,
validators: &super::ValidatorSet,
consensus_threshold: usize,
) -> Result<(), BlockValidationError> {
let payload_hash = self.payload.hash();
if payload_hash != self.header.payload {
if payload_hash != self.header().payload {
return Err(BlockValidationError::HashMismatch {
header_hash: self.header.payload,
header_hash: self.header().payload,
payload_hash,
});
}
if self.header != self.justification.message.proposal {
return Err(BlockValidationError::ProposalMismatch {
block_header: Box::new(self.header),
qc_header: Box::new(self.justification.message.proposal),
});
}

self.justification
.verify(validators, consensus_threshold)
.map_err(BlockValidationError::Justification)
Expand Down Expand Up @@ -233,17 +227,6 @@ pub enum BlockValidationError {
/// Hash of the payload.
payload_hash: PayloadHash,
},
/// Quorum certificate proposal doesn't match the block header.
#[error(
"quorum certificate proposal doesn't match the block header (block header: {block_header:?}, \
header in QC: {qc_header:?})"
)]
ProposalMismatch {
/// Block header field.
block_header: Box<BlockHeader>,
/// Block header from the quorum certificate.
qc_header: Box<BlockHeader>,
},
/// Failed verifying quorum certificate.
#[error("failed verifying quorum certificate: {0:#?}")]
Justification(#[source] anyhow::Error),
Expand Down
3 changes: 0 additions & 3 deletions node/libs/roles/src/validator/testonly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ pub fn make_genesis_block<R: Rng>(rng: &mut R, protocol_version: ProtocolVersion
let header = BlockHeader::genesis(payload.hash(), BlockNumber(0));
let justification = make_justification(rng, &header, protocol_version);
FinalBlock {
header,
payload,
justification,
}
Expand All @@ -56,7 +55,6 @@ pub fn make_block<R: Rng>(
let header = BlockHeader::new(parent, payload.hash());
let justification = make_justification(rng, &header, protocol_version);
FinalBlock {
header,
payload,
justification,
}
Expand Down Expand Up @@ -130,7 +128,6 @@ impl Distribution<Payload> for Standard {
impl Distribution<FinalBlock> for Standard {
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> FinalBlock {
FinalBlock {
header: rng.gen(),
payload: rng.gen(),
justification: rng.gen(),
}
Expand Down
8 changes: 4 additions & 4 deletions node/libs/storage/src/in_memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl BlocksInMemoryStore {
}

fn put_block(&mut self, block: validator::FinalBlock) {
let block_number = block.header.number;
let block_number = block.header().number;
tracing::debug!("Inserting block #{block_number} into database");
if let Some(prev_block) = self.blocks.insert(block_number, block) {
tracing::debug!(?prev_block, "Block #{block_number} is overwritten");
Expand Down Expand Up @@ -78,7 +78,7 @@ pub struct InMemoryStorage {
impl InMemoryStorage {
/// Creates a new store containing only the specified `genesis_block`.
pub fn new(genesis_block: validator::FinalBlock) -> Self {
let genesis_block_number = genesis_block.header.number;
let genesis_block_number = genesis_block.header().number;
Self {
blocks: Mutex::new(BlocksInMemoryStore {
blocks: BTreeMap::from([(genesis_block_number, genesis_block)]),
Expand Down Expand Up @@ -137,7 +137,7 @@ impl WriteBlockStore for InMemoryStorage {
block_number: validator::BlockNumber,
_payload: &validator::Payload,
) -> ctx::Result<()> {
let head_number = self.head_block(ctx).await?.header.number;
let head_number = self.head_block(ctx).await?.header().number;
if head_number >= block_number {
return Err(anyhow::anyhow!(
"received proposal for block {block_number:?}, while head is at {head_number:?}"
Expand All @@ -149,7 +149,7 @@ impl WriteBlockStore for InMemoryStorage {

async fn put_block(&self, _ctx: &ctx::Ctx, block: &validator::FinalBlock) -> ctx::Result<()> {
self.blocks.lock().await.put_block(block.clone());
self.blocks_sender.send_replace(block.header.number);
self.blocks_sender.send_replace(block.header().number);
Ok(())
}
}
Expand Down
12 changes: 6 additions & 6 deletions node/libs/storage/src/rocksdb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,11 @@ impl RocksdbStorage {

let this = Self {
inner: RwLock::new(db),
cached_last_contiguous_block_number: AtomicU64::new(genesis_block.header.number.0),
block_writes_sender: watch::channel(genesis_block.header.number).0,
cached_last_contiguous_block_number: AtomicU64::new(genesis_block.header().number.0),
block_writes_sender: watch::channel(genesis_block.header().number).0,
};
if let Some(stored_genesis_block) = this.block(ctx, genesis_block.header.number).await? {
if stored_genesis_block.header != genesis_block.header {
if let Some(stored_genesis_block) = this.block(ctx, genesis_block.header().number).await? {
if stored_genesis_block.header() != genesis_block.header() {
let err = anyhow::anyhow!("Mismatch between stored and expected genesis block");
return Err(err.into());
}
Expand Down Expand Up @@ -251,7 +251,7 @@ impl RocksdbStorage {
/// Insert a new block into the database.
fn put_block_blocking(&self, finalized_block: &validator::FinalBlock) -> anyhow::Result<()> {
let db = self.write();
let block_number = finalized_block.header.number;
let block_number = finalized_block.header().number;
tracing::debug!("Inserting new block #{block_number} into the database.");

let mut write_batch = rocksdb::WriteBatch::default();
Expand Down Expand Up @@ -344,7 +344,7 @@ impl WriteBlockStore for RocksdbStorage {
block_number: validator::BlockNumber,
_payload: &validator::Payload,
) -> ctx::Result<()> {
let head_number = self.head_block(ctx).await?.header.number;
let head_number = self.head_block(ctx).await?.header().number;
if head_number >= block_number {
return Err(anyhow::anyhow!(
"received proposal for block {block_number:?}, while head is at {head_number:?}"
Expand Down
Loading

0 comments on commit 5963f69

Please sign in to comment.