From 1a7a840be4701584811de4be131405bbc97f6c58 Mon Sep 17 00:00:00 2001 From: Jeb Bearer Date: Thu, 31 Oct 2024 08:48:53 -0700 Subject: [PATCH] Make `update` fallible again A recent commit made `update` infallible, reasoning that if we fail to store a new leaf, it is better to at least try storing the next one, rather than failing completely. However, this is not accurate: the client uses the success/failure of `udpate` to determine when it is safe to garbage collect data from temporary storage. Thus, returning success when we didn't actually store all the leaves can lead to data loss. This change fixes the situation by again returning an error whenever we fail to insert a leaf. It also tries to make the return value more useful, by returning on error the height of the first leaf we failed to insert (which is useful for garbage collection) instead of just an opaque error message which the caller can't do much with. --- src/data_source/extension.rs | 2 +- src/data_source/fs.rs | 6 ++++-- src/data_source/sql.rs | 6 ++++-- src/data_source/storage/no_storage.rs | 2 +- src/data_source/update.rs | 16 ++++++++++++---- src/lib.rs | 8 +++++--- 6 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/data_source/extension.rs b/src/data_source/extension.rs index bf8f6398d..efc6b13a9 100644 --- a/src/data_source/extension.rs +++ b/src/data_source/extension.rs @@ -433,7 +433,7 @@ mod impl_testable_data_source { } async fn handle_event(&self, event: &Event) { - self.update(event).await; + self.update(event).await.unwrap(); } } } diff --git a/src/data_source/fs.rs b/src/data_source/fs.rs index 9d16aeb75..f46d7c177 100644 --- a/src/data_source/fs.rs +++ b/src/data_source/fs.rs @@ -138,7 +138,9 @@ pub use super::storage::fs::Transaction; /// let mut events = hotshot.event_stream(); /// while let Some(event) = events.next().await { /// let mut state = state.write().await; -/// state.hotshot_qs.update(&event).await; +/// if state.hotshot_qs.update(&event).await.is_err() { +/// continue; +/// } /// /// // Update other modules' states based on `event`. /// let mut tx = state.hotshot_qs.write().await.unwrap(); @@ -269,7 +271,7 @@ mod impl_testable_data_source { } async fn handle_event(&self, event: &Event) { - self.update(event).await; + self.update(event).await.unwrap(); } } } diff --git a/src/data_source/sql.rs b/src/data_source/sql.rs index 44ef0739b..d552ca923 100644 --- a/src/data_source/sql.rs +++ b/src/data_source/sql.rs @@ -267,7 +267,9 @@ impl Config { /// spawn(async move { /// let mut events = hotshot.event_stream(); /// while let Some(event) = events.next().await { -/// state.hotshot_qs.update(&event).await; +/// if state.hotshot_qs.update(&event).await.is_err() { +/// continue; +/// } /// /// let mut tx = state.hotshot_qs.write().await.unwrap(); /// // Update other modules' states based on `event`, using `tx` to access the database. @@ -334,7 +336,7 @@ pub mod testing { } async fn handle_event(&self, event: &Event) { - self.update(event).await; + self.update(event).await.unwrap(); } } } diff --git a/src/data_source/storage/no_storage.rs b/src/data_source/storage/no_storage.rs index 9a01df4b0..2969ca49f 100644 --- a/src/data_source/storage/no_storage.rs +++ b/src/data_source/storage/no_storage.rs @@ -383,7 +383,7 @@ pub mod testing { } async fn handle_event(&self, event: &Event) { - self.update(event).await; + self.update(event).await.unwrap(); } } diff --git a/src/data_source/update.rs b/src/data_source/update.rs index 7f6af335a..fe38494f7 100644 --- a/src/data_source/update.rs +++ b/src/data_source/update.rs @@ -56,7 +56,13 @@ pub trait UpdateDataSource: UpdateAvailabilityData { /// /// If you want to update the data source with an untrusted event, for example one received from /// a peer over the network, you must authenticate it first. - async fn update(&self, event: &Event); + /// + /// # Returns + /// + /// If all provided data is successfully inserted into the database, returns `Ok(())`. If any + /// error occurred, the error is logged, and the return value is the height of the first leaf + /// which failed to be inserted. + async fn update(&self, event: &Event) -> Result<(), u64>; } #[async_trait] @@ -66,7 +72,7 @@ where Payload: QueryablePayload, ::InstanceState: Default, { - async fn update(&self, event: &Event) { + async fn update(&self, event: &Event) -> Result<(), u64> { if let EventType::Decide { leaf_chain, qc, .. } = &event.event { // `qc` justifies the first (most recent) leaf... let qcs = once((**qc).clone()) @@ -95,7 +101,7 @@ where ?qc, "inconsistent leaf; cannot append leaf information: {err:#}" ); - continue; + return Err(leaf.block_header().block_number()); } }; @@ -137,10 +143,12 @@ where .append(BlockInfo::new(leaf_data, block_data, vid_common, vid_share)) .await { - tracing::warn!(height, "failed to append leaf information: {err:#}"); + tracing::error!(height, "failed to append leaf information: {err:#}"); + return Err(leaf.block_header().block_number()); } } } + Ok(()) } } diff --git a/src/lib.rs b/src/lib.rs index bc2e235e6..f9fd3cce7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,7 +77,7 @@ //! let mut events = hotshot.event_stream(); //! while let Some(event) = events.next().await { //! // Update the query data based on this event. -//! data_source.update(&event).await; +//! data_source.update(&event).await.ok(); //! } //! # Ok(()) //! # } @@ -553,8 +553,10 @@ where // Update query data using HotShot events. while let Some(event) = events.next().await { - // Update the query data based on this event. - data_source.update(&event).await; + // Update the query data based on this event. It is safe to ignore errors here; the error + // just returns the failed block height for use in garbage collection, but this simple + // implementation isn't doing any kind of garbage collection. + data_source.update(&event).await.ok(); } Ok(())