Skip to content

Commit 06fcbd1

Browse files
committed
fix(consensus-db): measure extend_certificate through the commit
Address review on #142: extend_certificate stopped its timer before tx.commit(), excluding redb's durable-write (fsync) cost -- the dominant term -- so it fed write_time with a narrower scope than insert_decided_block. Record write_time after the commit so both write paths share one scope. Also add a regression test for extend_certificate asserting write_count increments by exactly one, mirroring the decided-block test.
1 parent 9d7cbe3 commit 06fcbd1

1 file changed

Lines changed: 60 additions & 2 deletions

File tree

‎crates/consensus-db/src/store.rs‎

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -557,11 +557,13 @@ impl Db {
557557
CommitCertificateType::Extended,
558558
existing.proposer,
559559
)?;
560-
let write_time = start.elapsed();
561560

562561
tx.commit()?;
563562

564-
self.update_write_metrics(write_bytes, write_time);
563+
// Measure through the commit: for redb the commit is where the durable
564+
// write (fsync) cost lives, and `insert_decided_block` records its write
565+
// the same way, so both paths feed `write_time` with the same scope.
566+
self.update_write_metrics(write_bytes, start.elapsed());
565567

566568
Ok(())
567569
}
@@ -1980,6 +1982,62 @@ mod tests {
19801982
assert_eq!(retrieved.certificate.commit_signatures.len(), 4);
19811983
}
19821984

1985+
#[tokio::test]
1986+
async fn extend_certificate_counts_a_single_write() {
1987+
use malachitebft_core_types::{NilOrVal, SignedMessage};
1988+
1989+
// extend_certificate is the other path whose metric ownership this change
1990+
// touches (it now records the write itself instead of relying on
1991+
// insert_certificate). A certificate extension is one committed
1992+
// transaction, so it must be counted exactly once.
1993+
let dir = tempdir().unwrap();
1994+
let metrics = DbMetrics::default();
1995+
let store = Store::open(
1996+
dir.path().join("db"),
1997+
metrics.clone(),
1998+
DbUpgrade::Skip,
1999+
TEST_CACHE_SIZE,
2000+
)
2001+
.await
2002+
.unwrap();
2003+
2004+
let height = Height::new(1);
2005+
let round = Round::new(0);
2006+
let payload = arbitrary_payload();
2007+
let block_hash = payload.payload_inner.payload_inner.block_hash;
2008+
let value_id = ValueId::new(block_hash);
2009+
2010+
let signature = Signature::from_bytes([0xab; 64]);
2011+
let vote =
2012+
Vote::new_precommit(height, round, NilOrVal::Val(value_id), Address::new([1u8; 20]));
2013+
let cert = CommitCertificate::<ArcContext>::new(
2014+
height,
2015+
round,
2016+
value_id,
2017+
vec![SignedMessage::new(vote, signature)],
2018+
);
2019+
2020+
store
2021+
.store_decided_block(cert, payload, Address::new([0u8; 20]))
2022+
.await
2023+
.unwrap();
2024+
2025+
let mut stored = store.get_certificate(Some(height)).await.unwrap().unwrap();
2026+
stored.certificate.commit_signatures.push(CommitSignature::new(
2027+
Address::new([4u8; 20]),
2028+
Signature::from_bytes([0xcd; 64]),
2029+
));
2030+
2031+
let writes_before = metrics.write_count();
2032+
store.extend_certificate(stored.certificate).await.unwrap();
2033+
2034+
assert_eq!(
2035+
metrics.write_count(),
2036+
writes_before + 1,
2037+
"extending a certificate is one committed transaction and must be counted once"
2038+
);
2039+
}
2040+
19832041
#[tokio::test]
19842042
async fn test_extend_certificate_without_existing_fails() {
19852043
let store = create_store().await;

0 commit comments

Comments
 (0)