From 30376d0f30f02aab1ff92cb9bb6033f956adf150 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 23 Jan 2024 11:15:55 -0500 Subject: [PATCH 1/5] index payloads by view --- crates/hotshot/src/lib.rs | 7 +++---- crates/task-impls/src/consensus.rs | 2 +- crates/task-impls/src/da.rs | 2 +- crates/types/src/consensus.rs | 13 +++---------- 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/crates/hotshot/src/lib.rs b/crates/hotshot/src/lib.rs index 69a10e582a..9612df7a78 100644 --- a/crates/hotshot/src/lib.rs +++ b/crates/hotshot/src/lib.rs @@ -39,7 +39,7 @@ use hotshot_task::{ use hotshot_task_impls::{events::HotShotEvent, network::NetworkTaskKind}; use hotshot_types::{ - consensus::{Consensus, ConsensusMetricsValue, PayloadStore, View, ViewInner, ViewQueue}, + consensus::{Consensus, ConsensusMetricsValue, View, ViewInner, ViewQueue}, data::Leaf, error::StorageSnafu, event::EventType, @@ -217,9 +217,8 @@ impl> SystemContext { ); let mut saved_leaves = HashMap::new(); - let mut saved_payloads = PayloadStore::default(); + let mut saved_payloads = BTreeMap::new(); saved_leaves.insert(anchored_leaf.commit(), anchored_leaf.clone()); - let payload_commitment = anchored_leaf.get_payload_commitment(); if let Some(payload) = anchored_leaf.get_block_payload() { let encoded_txns = match payload.encode() { // TODO (Keyao) [VALIDATED_STATE] - Avoid collect/copy on the encoded transaction bytes. @@ -229,7 +228,7 @@ impl> SystemContext { return Err(HotShotError::BlockError { source: e }); } }; - saved_payloads.insert(payload_commitment, encoded_txns); + saved_payloads.insert(anchored_leaf.get_view_number(), encoded_txns); } let start_view = anchored_leaf.get_view_number(); diff --git a/crates/task-impls/src/consensus.rs b/crates/task-impls/src/consensus.rs index e2848584c3..7acc4254cf 100644 --- a/crates/task-impls/src/consensus.rs +++ b/crates/task-impls/src/consensus.rs @@ -702,7 +702,7 @@ impl, A: ConsensusApi + // If the block payload is available for this leaf, include it in // the leaf chain that we send to the client. if let Some(encoded_txns) = - consensus.saved_payloads.get(leaf.get_payload_commitment()) + consensus.saved_payloads.get(&leaf.get_view_number()) { let payload = BlockPayload::from_bytes( encoded_txns.clone().into_iter(), diff --git a/crates/task-impls/src/da.rs b/crates/task-impls/src/da.rs index a420a15946..bab588d1dd 100644 --- a/crates/task-impls/src/da.rs +++ b/crates/task-impls/src/da.rs @@ -192,7 +192,7 @@ impl, A: ConsensusApi + // Record the payload we have promised to make available. consensus .saved_payloads - .insert(payload_commitment, proposal.data.encoded_transactions); + .insert(view, proposal.data.encoded_transactions); } HotShotEvent::DAVoteRecv(ref vote) => { debug!("DA vote recv, Main Task {:?}", vote.get_view_number()); diff --git a/crates/types/src/consensus.rs b/crates/types/src/consensus.rs index e86939804e..d18622ae03 100644 --- a/crates/types/src/consensus.rs +++ b/crates/types/src/consensus.rs @@ -55,7 +55,7 @@ pub struct Consensus { /// /// Contains the block payload commitment and encoded transactions for every leaf in /// `saved_leaves` if that payload is available. - pub saved_payloads: PayloadStore, + pub saved_payloads: BTreeMap>, /// The `locked_qc` view number pub locked_view: TYPES::Time, @@ -316,21 +316,14 @@ impl Consensus { // perform gc self.saved_da_certs .retain(|view_number, _| *view_number >= old_anchor_view); - self.state_map - .range(old_anchor_view..new_anchor_view) - .filter_map(|(_view_number, view)| view.get_payload_commitment()) - .for_each(|payload_commitment| { - self.saved_payloads.remove(payload_commitment); - }); self.state_map .range(old_anchor_view..new_anchor_view) .filter_map(|(_view_number, view)| view.get_leaf_commitment()) .for_each(|leaf| { - if let Some(removed) = self.saved_leaves.remove(&leaf) { - self.saved_payloads.remove(removed.get_payload_commitment()); - } + self.saved_leaves.remove(&leaf); }); self.state_map = self.state_map.split_off(&new_anchor_view); + self.saved_payloads = self.saved_payloads.split_off(&new_anchor_view); } /// Gets the last decided state From 8e61eb3d749b603c96ad3aec6762ea02bcf27167 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 23 Jan 2024 12:06:15 -0500 Subject: [PATCH 2/5] update comment --- crates/types/src/consensus.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/types/src/consensus.rs b/crates/types/src/consensus.rs index d18622ae03..16e4d8b710 100644 --- a/crates/types/src/consensus.rs +++ b/crates/types/src/consensus.rs @@ -53,8 +53,7 @@ pub struct Consensus { /// Saved payloads. /// - /// Contains the block payload commitment and encoded transactions for every leaf in - /// `saved_leaves` if that payload is available. + /// Encoded transactions for every view if we got a payload for that view. pub saved_payloads: BTreeMap>, /// The `locked_qc` view number From c199b8308dc6f19bcbb485bc4c0d1e2faa3a0d58 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 23 Jan 2024 12:12:39 -0500 Subject: [PATCH 3/5] kill 'PayloadStore' --- crates/task-impls/src/consensus.rs | 3 +- crates/types/src/consensus.rs | 60 ++---------------------------- 2 files changed, 4 insertions(+), 59 deletions(-) diff --git a/crates/task-impls/src/consensus.rs b/crates/task-impls/src/consensus.rs index 7acc4254cf..6e7f540816 100644 --- a/crates/task-impls/src/consensus.rs +++ b/crates/task-impls/src/consensus.rs @@ -558,8 +558,7 @@ impl, A: ConsensusApi + let liveness_check = justify_qc.get_view_number() > consensus.locked_view; let high_qc = consensus.high_qc.clone(); - let locked_view = consensus.locked_view; - + let locked_view = consensus.locked_view; drop(consensus); diff --git a/crates/types/src/consensus.rs b/crates/types/src/consensus.rs index 16e4d8b710..80bea89de4 100644 --- a/crates/types/src/consensus.rs +++ b/crates/types/src/consensus.rs @@ -7,7 +7,7 @@ pub use crate::{ use displaydoc::Display; use crate::{ - data::{Leaf, VidCommitment}, + data::Leaf, error::HotShotError, simple_certificate::{DACertificate, QuorumCertificate}, traits::{ @@ -17,9 +17,9 @@ use crate::{ utils::Terminator, }; use commit::Commitment; -use derivative::Derivative; + use std::{ - collections::{hash_map::Entry, BTreeMap, HashMap}, + collections::{BTreeMap, HashMap}, sync::{Arc, Mutex}, }; use tracing::error; @@ -339,57 +339,3 @@ impl Consensus { self.saved_leaves.get(&leaf).unwrap().clone() } } - -/// Mapping from block payload commitments to the encoded transactions. -/// -/// Entries in this mapping are reference-counted, so multiple consensus objects can refer to the -/// same block, and the block will only be deleted after _all_ such objects are garbage collected. -/// For example, multiple leaves may temporarily reference the same block on different branches, -/// before all but one branch are ultimately garbage collected. -#[derive(Clone, Debug, Derivative)] -#[derivative(Default(bound = ""))] -pub struct PayloadStore(HashMap, u64)>); - -impl PayloadStore { - /// Save the encoded transactions for later retrieval. - /// - /// After calling this function, and before the corresponding call to [`remove`](Self::remove), - /// `self.get(payload_commitment)` will return `Some(encoded_transactions)`. - /// - /// This function will increment a reference count on the saved payload commitment, so that - /// multiple calls to [`insert`](Self::insert) for the same payload commitment result in - /// multiple owning references to the payload commitment. [`remove`](Self::remove) must be - /// called once for each reference before the payload commitment will be deallocated. - pub fn insert(&mut self, payload_commitment: VidCommitment, encoded_transactions: Vec) { - self.0 - .entry(payload_commitment) - .and_modify(|(_, refcount)| *refcount += 1) - .or_insert((encoded_transactions, 1)); - } - - /// Get the saved encoded transactions, if available. - /// - /// If the encoded transactions has been saved with [`insert`](Self::insert), this function - /// will retrieve it. It may return [`None`] if a block with the given commitment has not been - /// saved or if the block has been dropped with [`remove`](Self::remove). - #[must_use] - pub fn get(&self, payload_commitment: VidCommitment) -> Option<&Vec> { - self.0.get(&payload_commitment).map(|(encoded, _)| encoded) - } - - /// Drop a reference to the saved encoded transactions. - /// - /// If the set exists and this call drops the last reference to it, the set will be returned, - /// Otherwise, the return value is [`None`]. - pub fn remove(&mut self, payload_commitment: VidCommitment) -> Option> { - if let Entry::Occupied(mut e) = self.0.entry(payload_commitment) { - let (_, refcount) = e.get_mut(); - *refcount -= 1; - if *refcount == 0 { - let (encoded, _) = e.remove(); - return Some(encoded); - } - } - None - } -} From 5cbb87c13b34d4309d08722917ab311f38253b3a Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Wed, 24 Jan 2024 10:32:25 -0500 Subject: [PATCH 4/5] Init view 1 with empty payload as well --- crates/hotshot/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/hotshot/src/lib.rs b/crates/hotshot/src/lib.rs index 9612df7a78..8608902394 100644 --- a/crates/hotshot/src/lib.rs +++ b/crates/hotshot/src/lib.rs @@ -220,7 +220,7 @@ impl> SystemContext { let mut saved_payloads = BTreeMap::new(); saved_leaves.insert(anchored_leaf.commit(), anchored_leaf.clone()); if let Some(payload) = anchored_leaf.get_block_payload() { - let encoded_txns = match payload.encode() { + let encoded_txns: Vec = match payload.encode() { // TODO (Keyao) [VALIDATED_STATE] - Avoid collect/copy on the encoded transaction bytes. // Ok(encoded) => encoded.into_iter().collect(), @@ -228,7 +228,8 @@ impl> SystemContext { return Err(HotShotError::BlockError { source: e }); } }; - saved_payloads.insert(anchored_leaf.get_view_number(), encoded_txns); + saved_payloads.insert(anchored_leaf.get_view_number(), encoded_txns.clone()); + saved_payloads.insert(TYPES::Time::new(1), encoded_txns); } let start_view = anchored_leaf.get_view_number(); From 6289ce1815ef3af6bb7865fdbfa95c456adeed43 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Wed, 24 Jan 2024 11:00:41 -0500 Subject: [PATCH 5/5] lint unrelated files --- crates/testing/tests/unreliable_network.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/testing/tests/unreliable_network.rs b/crates/testing/tests/unreliable_network.rs index c2e4a56249..e39ae11e98 100644 --- a/crates/testing/tests/unreliable_network.rs +++ b/crates/testing/tests/unreliable_network.rs @@ -44,7 +44,7 @@ async fn libp2p_network_sync() { .gen_launcher::(0) .launch() .run_test() - .await + .await; } #[cfg(test)] @@ -122,7 +122,7 @@ async fn libp2p_network_async() { .gen_launcher::(0) .launch() .run_test() - .await + .await; } #[cfg(test)] @@ -260,7 +260,7 @@ async fn libp2p_network_partially_sync() { .gen_launcher::(0) .launch() .run_test() - .await + .await; } #[cfg(test)] @@ -339,5 +339,5 @@ async fn libp2p_network_chaos() { .gen_launcher::(0) .launch() .run_test() - .await + .await; }