From 5d43a73d4852b40ffa8e3e0b38baaf65bcef79ec Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Tue, 26 Mar 2024 18:25:53 -0400 Subject: [PATCH 1/9] Prevent proposing batches too quickly --- node/bft/src/lib.rs | 2 ++ node/bft/src/primary.rs | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/node/bft/src/lib.rs b/node/bft/src/lib.rs index d6fcbae66f..550fb20232 100644 --- a/node/bft/src/lib.rs +++ b/node/bft/src/lib.rs @@ -49,6 +49,8 @@ pub const MEMORY_POOL_PORT: u16 = 5000; // port /// The maximum number of milliseconds to wait before proposing a batch. pub const MAX_BATCH_DELAY_IN_MS: u64 = 2500; // ms +/// The maximum number of seconds to wait before proposing a batch (rounded down to the nearest second). +pub const MAX_BATCH_DELAY_IN_SECS: u64 = MAX_BATCH_DELAY_IN_MS / 1000; // seconds /// The maximum number of milliseconds to wait before timing out on a fetch. pub const MAX_FETCH_TIMEOUT_IN_MS: u64 = 3 * MAX_BATCH_DELAY_IN_MS; // ms /// The maximum number of seconds allowed for the leader to send their certificate. diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index a2a070183e..5b566a9def 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -33,6 +33,7 @@ use crate::{ Transport, Worker, MAX_BATCH_DELAY_IN_MS, + MAX_BATCH_DELAY_IN_SECS, MAX_WORKERS, PRIMARY_PING_IN_MS, WORKER_PING_IN_MS, @@ -321,6 +322,21 @@ impl Primary { #[cfg(feature = "metrics")] metrics::gauge(metrics::bft::PROPOSAL_ROUND, round as f64); + // Ensure that the primary does not create a new proposal too quickly. + if let Some(previous_certificate) = self + .storage + .get_certificate_for_round_with_author(round.saturating_sub(1), self.gateway.account().address()) + { + // If the primary proposed a previous certificate too recently, return early. + if now().saturating_sub(previous_certificate.timestamp()) < MAX_BATCH_DELAY_IN_SECS as i64 { + debug!( + "Primary is safely skipping a batch proposal {}", + format!("(round {round} was proposed too quickly)").dimmed() + ); + return Ok(()); + } + } + // Ensure the primary has not proposed a batch for this round before. if self.storage.contains_certificate_in_round_from(round, self.gateway.account().address()) { // If a BFT sender was provided, attempt to advance the current round. From 3758d2603db86d745b655495049a17f7e57ebd5e Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Tue, 26 Mar 2024 19:21:13 -0400 Subject: [PATCH 2/9] Prevent signing proposals that were created too quickly --- node/bft/src/primary.rs | 47 +++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index 5b566a9def..e38436eff2 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -318,23 +318,16 @@ impl Primary { // Retrieve the current round. let round = self.current_round(); + // Compute the previous round. + let previous_round = round.saturating_sub(1); #[cfg(feature = "metrics")] metrics::gauge(metrics::bft::PROPOSAL_ROUND, round as f64); // Ensure that the primary does not create a new proposal too quickly. - if let Some(previous_certificate) = self - .storage - .get_certificate_for_round_with_author(round.saturating_sub(1), self.gateway.account().address()) - { - // If the primary proposed a previous certificate too recently, return early. - if now().saturating_sub(previous_certificate.timestamp()) < MAX_BATCH_DELAY_IN_SECS as i64 { - debug!( - "Primary is safely skipping a batch proposal {}", - format!("(round {round} was proposed too quickly)").dimmed() - ); - return Ok(()); - } + if let Err(e) = self.ensure_proposal_frequency(previous_round, self.gateway.account().address(), now()) { + debug!("Primary is safely skipping a batch proposal - {}", format!("{e}").dimmed()); + return Ok(()); } // Ensure the primary has not proposed a batch for this round before. @@ -375,8 +368,6 @@ impl Primary { } } - // Compute the previous round. - let previous_round = round.saturating_sub(1); // Retrieve the previous certificates. let previous_certificates = self.storage.get_certificates_for_round(previous_round); @@ -611,6 +602,15 @@ impl Primary { } } + // Compute the previous round. + let previous_round = batch_round.saturating_sub(1); + // Ensure that the peer did not propose a batch too quickly. + if let Err(e) = self.ensure_proposal_frequency(previous_round, batch_author, batch_header.timestamp()) { + // Proceed to disconnect the validator. + self.gateway.disconnect(peer_ip); + bail!("Malicious peer - {e} from '{peer_ip}'"); + } + // If the peer is ahead, use the batch header to sync up to the peer. let mut transmissions = self.sync_with_batch_header_from_peer(peer_ip, &batch_header).await?; @@ -1205,6 +1205,25 @@ impl Primary { Ok(()) } + /// Ensure the primary is not creating batch proposals too frequently. + /// This checks that the certificate timestamp for the previous round is within the expected range. + fn ensure_proposal_frequency(&self, previous_round: u64, author: Address, timestamp: i64) -> Result<()> { + // Ensure that the primary does not create a new proposal too quickly. + match self.storage.get_certificate_for_round_with_author(previous_round, author) { + // Ensure that the previous certificate was created at least `MAX_BATCH_DELAY_IN_SECS` seconds ago. + Some(certificate) => { + match timestamp.saturating_sub(certificate.timestamp()) < MAX_BATCH_DELAY_IN_SECS as i64 { + true => { + bail!("Proposed batch was created too quickly after the certificate at round {previous_round}") + } + false => Ok(()), + } + } + // If we do not see a previous certificate for the author, then proceed optimistically. + None => Ok(()), + } + } + /// Stores the certified batch and broadcasts it to all validators, returning the certificate. async fn store_and_broadcast_certificate(&self, proposal: &Proposal, committee: &Committee) -> Result<()> { // Create the batch certificate and transmissions. From 95fe67aad4f79674961427a14e11d41fb25d9053 Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Tue, 26 Mar 2024 19:35:06 -0400 Subject: [PATCH 3/9] Fix tests and update naming --- node/bft/src/primary.rs | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index e38436eff2..d5687917b2 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -325,7 +325,7 @@ impl Primary { metrics::gauge(metrics::bft::PROPOSAL_ROUND, round as f64); // Ensure that the primary does not create a new proposal too quickly. - if let Err(e) = self.ensure_proposal_frequency(previous_round, self.gateway.account().address(), now()) { + if let Err(e) = self.check_proposal_timestamp(previous_round, self.gateway.account().address(), now()) { debug!("Primary is safely skipping a batch proposal - {}", format!("{e}").dimmed()); return Ok(()); } @@ -605,7 +605,7 @@ impl Primary { // Compute the previous round. let previous_round = batch_round.saturating_sub(1); // Ensure that the peer did not propose a batch too quickly. - if let Err(e) = self.ensure_proposal_frequency(previous_round, batch_author, batch_header.timestamp()) { + if let Err(e) = self.check_proposal_timestamp(previous_round, batch_author, batch_header.timestamp()) { // Proceed to disconnect the validator. self.gateway.disconnect(peer_ip); bail!("Malicious peer - {e} from '{peer_ip}'"); @@ -1207,12 +1207,17 @@ impl Primary { /// Ensure the primary is not creating batch proposals too frequently. /// This checks that the certificate timestamp for the previous round is within the expected range. - fn ensure_proposal_frequency(&self, previous_round: u64, author: Address, timestamp: i64) -> Result<()> { + fn check_proposal_timestamp(&self, previous_round: u64, author: Address, timestamp: i64) -> Result<()> { // Ensure that the primary does not create a new proposal too quickly. match self.storage.get_certificate_for_round_with_author(previous_round, author) { // Ensure that the previous certificate was created at least `MAX_BATCH_DELAY_IN_SECS` seconds ago. Some(certificate) => { - match timestamp.saturating_sub(certificate.timestamp()) < MAX_BATCH_DELAY_IN_SECS as i64 { + // Determine the elapsed time since the previous certificate. + let elapsed = timestamp.checked_sub(certificate.timestamp()).ok_or_else(|| { + anyhow!("Proposed batch has a timestamp earlier than the certificate at round {previous_round}") + })?; + // Ensure the elapsed time is within the expected range. + match elapsed < MAX_BATCH_DELAY_IN_SECS as i64 { true => { bail!("Proposed batch was created too quickly after the certificate at round {previous_round}") } @@ -1894,7 +1899,7 @@ mod tests { let round = 1; let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now(); + let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -1930,7 +1935,7 @@ mod tests { // Create a valid proposal with an author that isn't the primary. let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now(); + let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -1961,7 +1966,7 @@ mod tests { let round = 1; let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now(); + let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -2003,7 +2008,7 @@ mod tests { // Create a valid proposal with an author that isn't the primary. let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now(); + let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -2041,7 +2046,7 @@ mod tests { // Create a valid proposal. let round = 1; - let timestamp = now(); + let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( primary.gateway.account(), primary.ledger.current_committee().unwrap(), @@ -2114,7 +2119,7 @@ mod tests { // Create a valid proposal. let round = 1; - let timestamp = now(); + let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( primary.gateway.account(), primary.ledger.current_committee().unwrap(), @@ -2151,7 +2156,7 @@ mod tests { let previous_certificates = store_certificate_chain(&primary, &accounts, round, &mut rng); // Create a valid proposal. - let timestamp = now(); + let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( primary.gateway.account(), primary.ledger.current_committee().unwrap(), From 58097554296d80417098710f2bb85e609f38794e Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Tue, 26 Mar 2024 19:42:48 -0400 Subject: [PATCH 4/9] Add tests for invalid proposal timestamps --- node/bft/src/primary.rs | 72 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index d5687917b2..a23291572d 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -2038,6 +2038,78 @@ mod tests { ); } + #[tokio::test] + async fn test_batch_propose_from_peer_with_invalid_timestamp() { + let round = 2; + let mut rng = TestRng::default(); + let (primary, accounts) = primary_without_handlers(&mut rng).await; + + // Generate certificates. + let previous_certificates = store_certificate_chain(&primary, &accounts, round, &mut rng); + + // Create a valid proposal with an author that isn't the primary. + let peer_account = &accounts[1]; + let peer_ip = peer_account.0; + let invalid_timestamp = now(); // Use a timestamp that is too early. + let proposal = create_test_proposal( + &peer_account.1, + primary.ledger.current_committee().unwrap(), + round, + previous_certificates, + invalid_timestamp, + &mut rng, + ); + + // Make sure the primary is aware of the transmissions in the proposal. + for (transmission_id, transmission) in proposal.transmissions() { + primary.workers[0].process_transmission_from_peer(peer_ip, *transmission_id, transmission.clone()) + } + + // The author must be known to resolver to pass propose checks. + primary.gateway.resolver().insert_peer(peer_ip, peer_ip, peer_account.1.address()); + + // Try to process the batch proposal from the peer, should error. + assert!( + primary.process_batch_propose_from_peer(peer_ip, (*proposal.batch_header()).clone().into()).await.is_err() + ); + } + + #[tokio::test] + async fn test_batch_propose_from_peer_with_past_timestamp() { + let round = 2; + let mut rng = TestRng::default(); + let (primary, accounts) = primary_without_handlers(&mut rng).await; + + // Generate certificates. + let previous_certificates = store_certificate_chain(&primary, &accounts, round, &mut rng); + + // Create a valid proposal with an author that isn't the primary. + let peer_account = &accounts[1]; + let peer_ip = peer_account.0; + let past_timestamp = now() - 5; // Use a timestamp that is in the past. + let proposal = create_test_proposal( + &peer_account.1, + primary.ledger.current_committee().unwrap(), + round, + previous_certificates, + past_timestamp, + &mut rng, + ); + + // Make sure the primary is aware of the transmissions in the proposal. + for (transmission_id, transmission) in proposal.transmissions() { + primary.workers[0].process_transmission_from_peer(peer_ip, *transmission_id, transmission.clone()) + } + + // The author must be known to resolver to pass propose checks. + primary.gateway.resolver().insert_peer(peer_ip, peer_ip, peer_account.1.address()); + + // Try to process the batch proposal from the peer, should error. + assert!( + primary.process_batch_propose_from_peer(peer_ip, (*proposal.batch_header()).clone().into()).await.is_err() + ); + } + #[tokio::test(flavor = "multi_thread")] async fn test_batch_signature_from_peer() { let mut rng = TestRng::default(); From 742c67faa5a085aca27463d0fd35811c38ddba56 Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Tue, 26 Mar 2024 20:04:56 -0400 Subject: [PATCH 5/9] Add waits in tests --- node/bft/src/primary.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index a23291572d..2d2ad04585 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -1806,6 +1806,9 @@ mod tests { // Fill primary storage. store_certificate_chain(&primary, &accounts, round, &mut rng); + // Sleep for a while to ensure the primary is ready to propose the next round. + tokio::time::sleep(Duration::from_secs(MAX_BATCH_DELAY_IN_SECS + 1)).await; + // Try to propose a batch. There are no transmissions in the workers so the method should // just return without proposing a batch. assert!(primary.propose_batch().await.is_ok()); @@ -1874,6 +1877,9 @@ mod tests { primary.storage.insert_certificate(certificate, transmissions).unwrap(); } + // Sleep for a while to ensure the primary is ready to propose the next round. + tokio::time::sleep(Duration::from_secs(MAX_BATCH_DELAY_IN_SECS + 1)).await; + // Advance to the next round. assert!(primary.storage.increment_to_next_round(round).is_ok()); From 37bb04b28d8133f96675bb7f720e856d29180e07 Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Wed, 27 Mar 2024 15:26:53 -0400 Subject: [PATCH 6/9] Update log messages --- node/bft/src/primary.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index 2d2ad04585..b267f4fc36 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -1214,13 +1214,11 @@ impl Primary { Some(certificate) => { // Determine the elapsed time since the previous certificate. let elapsed = timestamp.checked_sub(certificate.timestamp()).ok_or_else(|| { - anyhow!("Proposed batch has a timestamp earlier than the certificate at round {previous_round}") + anyhow!("Timestamp cannot be before the previous certificate at round {previous_round}") })?; // Ensure the elapsed time is within the expected range. match elapsed < MAX_BATCH_DELAY_IN_SECS as i64 { - true => { - bail!("Proposed batch was created too quickly after the certificate at round {previous_round}") - } + true => bail!("Timestamp is too soon after the previous certificate at round {previous_round}"), false => Ok(()), } } From bc6086340a3967ce217c8b09009600ad7c0368cc Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Wed, 27 Mar 2024 15:27:12 -0400 Subject: [PATCH 7/9] Use MIN_BATCH_DELAY_IN_SECS --- node/bft/src/lib.rs | 4 ++-- node/bft/src/primary.rs | 24 ++++++++++++------------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/node/bft/src/lib.rs b/node/bft/src/lib.rs index 550fb20232..ff2fe665a5 100644 --- a/node/bft/src/lib.rs +++ b/node/bft/src/lib.rs @@ -49,8 +49,8 @@ pub const MEMORY_POOL_PORT: u16 = 5000; // port /// The maximum number of milliseconds to wait before proposing a batch. pub const MAX_BATCH_DELAY_IN_MS: u64 = 2500; // ms -/// The maximum number of seconds to wait before proposing a batch (rounded down to the nearest second). -pub const MAX_BATCH_DELAY_IN_SECS: u64 = MAX_BATCH_DELAY_IN_MS / 1000; // seconds +/// The minimum number of seconds to wait before proposing a batch. +pub const MIN_BATCH_DELAY_IN_SECS: u64 = 1; // seconds /// The maximum number of milliseconds to wait before timing out on a fetch. pub const MAX_FETCH_TIMEOUT_IN_MS: u64 = 3 * MAX_BATCH_DELAY_IN_MS; // ms /// The maximum number of seconds allowed for the leader to send their certificate. diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index b267f4fc36..8b033fe4a9 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -33,8 +33,8 @@ use crate::{ Transport, Worker, MAX_BATCH_DELAY_IN_MS, - MAX_BATCH_DELAY_IN_SECS, MAX_WORKERS, + MIN_BATCH_DELAY_IN_SECS, PRIMARY_PING_IN_MS, WORKER_PING_IN_MS, }; @@ -1210,14 +1210,14 @@ impl Primary { fn check_proposal_timestamp(&self, previous_round: u64, author: Address, timestamp: i64) -> Result<()> { // Ensure that the primary does not create a new proposal too quickly. match self.storage.get_certificate_for_round_with_author(previous_round, author) { - // Ensure that the previous certificate was created at least `MAX_BATCH_DELAY_IN_SECS` seconds ago. + // Ensure that the previous certificate was created at least `MIN_BATCH_DELAY_IN_MS` seconds ago. Some(certificate) => { // Determine the elapsed time since the previous certificate. let elapsed = timestamp.checked_sub(certificate.timestamp()).ok_or_else(|| { anyhow!("Timestamp cannot be before the previous certificate at round {previous_round}") })?; // Ensure the elapsed time is within the expected range. - match elapsed < MAX_BATCH_DELAY_IN_SECS as i64 { + match elapsed < MIN_BATCH_DELAY_IN_SECS as i64 { true => bail!("Timestamp is too soon after the previous certificate at round {previous_round}"), false => Ok(()), } @@ -1805,7 +1805,7 @@ mod tests { store_certificate_chain(&primary, &accounts, round, &mut rng); // Sleep for a while to ensure the primary is ready to propose the next round. - tokio::time::sleep(Duration::from_secs(MAX_BATCH_DELAY_IN_SECS + 1)).await; + tokio::time::sleep(Duration::from_secs(MIN_BATCH_DELAY_IN_SECS)).await; // Try to propose a batch. There are no transmissions in the workers so the method should // just return without proposing a batch. @@ -1876,7 +1876,7 @@ mod tests { } // Sleep for a while to ensure the primary is ready to propose the next round. - tokio::time::sleep(Duration::from_secs(MAX_BATCH_DELAY_IN_SECS + 1)).await; + tokio::time::sleep(Duration::from_secs(MIN_BATCH_DELAY_IN_SECS)).await; // Advance to the next round. assert!(primary.storage.increment_to_next_round(round).is_ok()); @@ -1903,7 +1903,7 @@ mod tests { let round = 1; let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; + let timestamp = now() + MIN_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -1939,7 +1939,7 @@ mod tests { // Create a valid proposal with an author that isn't the primary. let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; + let timestamp = now() + MIN_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -1970,7 +1970,7 @@ mod tests { let round = 1; let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; + let timestamp = now() + MIN_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -2012,7 +2012,7 @@ mod tests { // Create a valid proposal with an author that isn't the primary. let peer_account = &accounts[1]; let peer_ip = peer_account.0; - let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; + let timestamp = now() + MIN_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( &peer_account.1, primary.ledger.current_committee().unwrap(), @@ -2122,7 +2122,7 @@ mod tests { // Create a valid proposal. let round = 1; - let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; + let timestamp = now() + MIN_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( primary.gateway.account(), primary.ledger.current_committee().unwrap(), @@ -2195,7 +2195,7 @@ mod tests { // Create a valid proposal. let round = 1; - let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; + let timestamp = now() + MIN_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( primary.gateway.account(), primary.ledger.current_committee().unwrap(), @@ -2232,7 +2232,7 @@ mod tests { let previous_certificates = store_certificate_chain(&primary, &accounts, round, &mut rng); // Create a valid proposal. - let timestamp = now() + MAX_BATCH_DELAY_IN_SECS as i64; + let timestamp = now() + MIN_BATCH_DELAY_IN_SECS as i64; let proposal = create_test_proposal( primary.gateway.account(), primary.ledger.current_committee().unwrap(), From 9eb781eba7c26b8f72a8ea9dba1736e8dded3abd Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Wed, 27 Mar 2024 19:30:30 -0400 Subject: [PATCH 8/9] Add to check_proposal_timestamp condition --- node/bft/src/primary.rs | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index 8b033fe4a9..dbfa65d0dc 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -1208,22 +1208,32 @@ impl Primary { /// Ensure the primary is not creating batch proposals too frequently. /// This checks that the certificate timestamp for the previous round is within the expected range. fn check_proposal_timestamp(&self, previous_round: u64, author: Address, timestamp: i64) -> Result<()> { - // Ensure that the primary does not create a new proposal too quickly. - match self.storage.get_certificate_for_round_with_author(previous_round, author) { + // Retrieve the timestamp of the previous timestamp to check against. + let previous_timestamp = match self.storage.get_certificate_for_round_with_author(previous_round, author) { // Ensure that the previous certificate was created at least `MIN_BATCH_DELAY_IN_MS` seconds ago. - Some(certificate) => { - // Determine the elapsed time since the previous certificate. - let elapsed = timestamp.checked_sub(certificate.timestamp()).ok_or_else(|| { - anyhow!("Timestamp cannot be before the previous certificate at round {previous_round}") - })?; - // Ensure the elapsed time is within the expected range. - match elapsed < MIN_BATCH_DELAY_IN_SECS as i64 { - true => bail!("Timestamp is too soon after the previous certificate at round {previous_round}"), - false => Ok(()), - } - } - // If we do not see a previous certificate for the author, then proceed optimistically. - None => Ok(()), + Some(certificate) => certificate.timestamp(), + // If we do not see a previous certificate for the author, then proceed check against the earliest timestamp in the previous round. + None => match self + .storage + .get_certificates_for_round(previous_round) + .iter() + .map(BatchCertificate::timestamp) + .min() + { + Some(latest_timestamp) => latest_timestamp, + // If there are no certificates in the previous round, then return early. + None => return Ok(()), + }, + }; + + // Determine the elapsed time since the previous timestamp. + let elapsed = timestamp + .checked_sub(previous_timestamp) + .ok_or_else(|| anyhow!("Timestamp cannot be before the previous certificate at round {previous_round}"))?; + // Ensure that the previous certificate was created at least `MIN_BATCH_DELAY_IN_MS` seconds ago. + match elapsed < MIN_BATCH_DELAY_IN_SECS as i64 { + true => bail!("Timestamp is too soon after the previous certificate at round {previous_round}"), + false => Ok(()), } } From 03cc214d349edd6e016147817e6a1a91fe07fbc4 Mon Sep 17 00:00:00 2001 From: raychu86 <14917648+raychu86@users.noreply.github.com> Date: Fri, 29 Mar 2024 13:09:24 -0400 Subject: [PATCH 9/9] Track the latest batch proposal timestamp --- node/bft/src/primary.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/node/bft/src/primary.rs b/node/bft/src/primary.rs index dbfa65d0dc..5ecccd2132 100644 --- a/node/bft/src/primary.rs +++ b/node/bft/src/primary.rs @@ -91,6 +91,8 @@ pub struct Primary { bft_sender: Arc>>, /// The batch proposal, if the primary is currently proposing a batch. proposed_batch: Arc>, + /// The timestamp of the most recent proposed batch. + latest_proposed_batch_timestamp: Arc>, /// The recently-signed batch proposals (a map from the address to the round, batch ID, and signature). signed_proposals: Arc, (u64, Field, Signature)>>>, /// The spawned handles. @@ -125,6 +127,7 @@ impl Primary { workers: Arc::from(vec![]), bft_sender: Default::default(), proposed_batch: Default::default(), + latest_proposed_batch_timestamp: Default::default(), signed_proposals: Default::default(), handles: Default::default(), propose_lock: Default::default(), @@ -505,6 +508,8 @@ impl Primary { let proposal = Proposal::new(committee_lookback, batch_header.clone(), transmissions)?; // Broadcast the batch to all validators for signing. self.gateway.broadcast(Event::BatchPropose(batch_header.into())); + // Set the timestamp of the latest proposed batch. + *self.latest_proposed_batch_timestamp.write() = proposal.timestamp(); // Set the proposed batch. *self.proposed_batch.write() = Some(proposal); Ok(()) @@ -1212,17 +1217,11 @@ impl Primary { let previous_timestamp = match self.storage.get_certificate_for_round_with_author(previous_round, author) { // Ensure that the previous certificate was created at least `MIN_BATCH_DELAY_IN_MS` seconds ago. Some(certificate) => certificate.timestamp(), - // If we do not see a previous certificate for the author, then proceed check against the earliest timestamp in the previous round. - None => match self - .storage - .get_certificates_for_round(previous_round) - .iter() - .map(BatchCertificate::timestamp) - .min() - { - Some(latest_timestamp) => latest_timestamp, - // If there are no certificates in the previous round, then return early. - None => return Ok(()), + None => match self.gateway.account().address() == author { + // If we are the author, then ensure the previous proposal was created at least `MIN_BATCH_DELAY_IN_MS` seconds ago. + true => *self.latest_proposed_batch_timestamp.read(), + // If we do not see a previous certificate for the author, then proceed optimistically. + false => return Ok(()), }, };