Merge pull request #714 from EspressoSystems/jb/elide-leaf-proposal

Do not store payloads with leaves

Cargo.toml

@@ -72,9 +72,9 @@ derivative = "2.2"
 derive_more = "0.99"
 either = "1.12"
 futures = "0.3"
-hotshot = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78" }
-hotshot-testing = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78" }
-hotshot-types = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78" }
+hotshot = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78-patch4" }
+hotshot-testing = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78-patch4" }
+hotshot-types = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78-patch4" }
 itertools = "0.12.1"
 jf-merkle-tree = { version = "0.1.0", git = "https://github.com/EspressoSystems/jellyfish", tag = "0.4.5", features = [
 	"std",
@@ -116,7 +116,7 @@ sqlx = { version = "0.8", features = [
 
 # Dependencies enabled by feature "testing".
 espresso-macros = { git = "https://github.com/EspressoSystems/espresso-macros.git", tag = "0.1.0", optional = true }
-hotshot-example-types = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78", optional = true }
+hotshot-example-types = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78-patch4", optional = true }
 portpicker = { version = "0.1", optional = true }
 rand = { version = "0.8", optional = true }
 spin_sleep = { version = "1.2", optional = true }
@@ -137,7 +137,7 @@ backtrace-on-stack-overflow = { version = "0.3", optional = true }
 clap = { version = "4.5", features = ["derive", "env"] }
 espresso-macros = { git = "https://github.com/EspressoSystems/espresso-macros.git", tag = "0.1.0" }
 generic-array = "0.14"
-hotshot-example-types = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78" }
+hotshot-example-types = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "0.5.78-patch4" }
 portpicker = "0.1"
 rand = "0.8"
 reqwest = "0.12.3"

migrations/V100__drop_leaf_payload.sql

@@ -0,0 +1,5 @@
+-- A previous version of the software erroneously stored leaves in the database with the full
+-- payload. This is unnecesssary, since we store payloads in their own separate table, and hurts
+-- performance. The updated software no longer does this for new leaves. This migration removes the
+-- redundant payloads for old leaves.
+UPDATE leaf SET leaf = jsonb_set(leaf, '{block_payload}', 'null');

src/availability/query_data.rs

@@ -210,7 +210,7 @@ impl<Types: NodeType> LeafQueryData<Types> {
     ///
     /// Fails with an [`InconsistentLeafError`] if `qc` does not reference `leaf`.
     pub fn new(
-        leaf: Leaf<Types>,
+        mut leaf: Leaf<Types>,
         qc: QuorumCertificate<Types>,
     ) -> Result<Self, InconsistentLeafError<Types>> {
         // TODO: Replace with the new `commit` function in HotShot. Add an `upgrade_lock` parameter
@@ -224,6 +224,11 @@ impl<Types: NodeType> LeafQueryData<Types> {
                 qc_leaf: qc.data.leaf_commit
             }
         );
+
+        // We only want the leaf for the block header and consensus metadata. The payload will be
+        // stored separately.
+        leaf.unfill_block_payload();
+
         Ok(Self { leaf, qc })
     }
 

src/data_source/fetching.rs

@@ -1348,7 +1348,7 @@ impl<T, E> ResultExt<T, E> for Result<T, E> {
         match self {
             Ok(t) => Some(t),
             Err(err) => {
-                tracing::warn!(
+                tracing::info!(
                     "error loading resource from local storage, will try to fetch: {err:#}"
                 );
                 None

src/data_source/sql.rs

@@ -150,7 +150,7 @@ impl Config {
 ///
 /// Custom migrations can be inserted using [`Config::migrations`]. Each custom migration will be
 /// inserted into the overall sequence of migrations in order of version number. The migrations
-/// provided by this crate only use version numbers which are multiples of 10, so the non-multiples
+/// provided by this crate only use version numbers which are multiples of 100, so the non-multiples
 /// can be used to insert custom migrations between the default migrations. You can also replace a
 /// default migration completely by providing a custom migration with the same version number. This
 /// may be useful when an earlier custom migration has altered the schema in such a way that a later

src/data_source/storage/sql.rs

@@ -74,8 +74,9 @@ use self::{migrate::Migrator, transaction::PoolMetrics};
 ///
 /// ```
 /// # use hotshot_query_service::data_source::sql::{include_migrations, Migration};
-/// let migrations: Vec<Migration> =
+/// let mut migrations: Vec<Migration> =
 ///     include_migrations!("$CARGO_MANIFEST_DIR/migrations").collect();
+/// migrations.sort();
 /// assert_eq!(migrations[0].version(), 10);
 /// assert_eq!(migrations[0].name(), "init_schema");
 /// ```
@@ -120,12 +121,22 @@ pub fn default_migrations() -> Vec<Migration> {
     // Check version uniqueness and sort by version.
     validate_migrations(&mut migrations).expect("default migrations are invalid");
 
-    // Check that all migration versions are multiples of 10, so that custom migrations can be
+    // Check that all migration versions are multiples of 100, so that custom migrations can be
     // inserted in between.
     for m in &migrations {
-        if m.version() == 0 || m.version() % 10 != 0 {
-            panic!(
-                "default migration version {} is not a positive multiple of 10",
+        if m.version() <= 30 {
+            // An older version of this software used intervals of 10 instead of 100. This was
+            // changed to allow more custom migrations between each default migration, but we must
+            // still accept older migrations that followed the older rule.
+            assert!(
+                m.version() > 0 && m.version() % 10 == 0,
+                "legacy default migration version {} is not a positive multiple of 10",
+                m.version()
+            );
+        } else {
+            assert!(
+                m.version() % 100 == 0,
+                "default migration version {} is not a multiple of 100",
                 m.version()
             );
         }
@@ -946,11 +957,11 @@ mod test {
         // The SQL commands used here will fail if not run in order.
         let migrations = vec![
             Migration::unapplied(
-                "V33__create_test_table.sql",
+                "V103__create_test_table.sql",
                 "ALTER TABLE test ADD COLUMN data INTEGER;",
             )
             .unwrap(),
-            Migration::unapplied("V32__create_test_table.sql", "CREATE TABLE test ();").unwrap(),
+            Migration::unapplied("V102__create_test_table.sql", "CREATE TABLE test ();").unwrap(),
         ];
         connect(true, migrations.clone()).await.unwrap();
 

src/fetching/provider/query_service.rs

@@ -100,16 +100,23 @@ where
     async fn fetch(&self, req: LeafRequest) -> Option<LeafQueryData<Types>> {
         match self
             .client
-            .get(&format!("availability/leaf/{}", usize::from(req)))
+            .get::<LeafQueryData<Types>>(&format!("availability/leaf/{}", usize::from(req)))
             .send()
             .await
         {
-            Ok(leaf) => {
+            Ok(mut leaf) => {
                 // TODO we should also download a chain of QCs justifying the inclusion of `leaf` in
                 // the chain at the requested height. However, HotShot currently lacks a good light
                 // client API to verify this chain, so for now we just trust the other server.
                 // https://github.com/EspressoSystems/HotShot/issues/2137
                 // https://github.com/EspressoSystems/hotshot-query-service/issues/354
+
+                // There is a potential DOS attack where the peer sends us a leaf with the full
+                // payload in it, which uses redundant resources in the database, since we fetch and
+                // store payloads separately. We can defend ourselves by simply dropping the payload
+                // if present.
+                leaf.leaf.unfill_block_payload();
+
                 Some(leaf)
             }
             Err(err) => {