Add test for verify_share return type (#3590)

* Add test for verify_share return type

* Remove patch and ignore test

* Fix typo

* commit other mod.rs file

* just fmt

* Remove jellyfish patch from cargo toml

* PR comments

* Clippy fixes

Cargo.toml

@@ -176,4 +176,4 @@ module_name_repetitions = "allow"
 rust_2018_idioms = "warn"
 # TODO change to deny
 missing_docs = "warn"
-warnings = "warn"
+warnings = "warn"

crates/task-impls/Cargo.toml

@@ -10,6 +10,7 @@ example-upgrade = []
 gpu-vid = ["hotshot-types/gpu-vid"]
 dependency-tasks = []
 rewind = []
+test-srs = ["jf-vid/test-srs"]
 
 [dependencies]
 anyhow = { workspace = true }

crates/task-impls/src/consensus/mod.rs

@@ -177,19 +177,16 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> ConsensusTaskSt
         }
 
         // Validate the VID share.
-        if vid_scheme(self.quorum_membership.total_nodes())
-            .verify_share(
+        // NOTE: `verify_share` returns a nested `Result`, so we must check both the inner
+        // and outer results
+        matches!(
+            vid_scheme(self.quorum_membership.total_nodes()).verify_share(
                 &disperse.data.share,
                 &disperse.data.common,
                 &payload_commitment,
-            )
-            .is_err()
-        {
-            debug!("Invalid VID share.");
-            return false;
-        }
-
-        true
+            ),
+            Ok(Ok(()))
+        )
     }
 
     /// Publishes a proposal

crates/task-impls/src/quorum_vote/mod.rs

@@ -605,16 +605,20 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>, V: Versions> QuorumVoteTaskS
                         return;
                     }
                 }
-                if vid_scheme(self.quorum_membership.total_nodes())
-                    .verify_share(
-                        &disperse.data.share,
-                        &disperse.data.common,
-                        &payload_commitment,
-                    )
-                    .is_err()
-                {
-                    debug!("Invalid VID share.");
-                    return;
+                // NOTE: `verify_share` returns a nested `Result`, so we must check both the inner
+                // and outer results
+                #[allow(clippy::no_effect)]
+                match vid_scheme(self.quorum_membership.total_nodes()).verify_share(
+                    &disperse.data.share,
+                    &disperse.data.common,
+                    &payload_commitment,
+                ) {
+                    Ok(Err(())) | Err(_) => {
+                        return;
+                    }
+                    Ok(Ok(())) => {
+                        ();
+                    }
                 }
 
                 self.consensus

crates/testing/Cargo.toml

@@ -12,6 +12,7 @@ slow-tests = []
 gpu-vid = ["hotshot-types/gpu-vid"]
 dependency-tasks = ["hotshot/dependency-tasks"]
 rewind = ["hotshot/rewind"]
+test-srs = ["jf-vid/test-srs"]
 
 [dependencies]
 automod = "1.0.14"

crates/testing/tests/tests_1/consensus_task.rs

@@ -602,3 +602,78 @@ async fn test_vid_disperse_storage_failure() {
 
     run_test![inputs, consensus_script].await;
 }
+
+/// Tests that VID shares that return validation with an Ok(Err) result
+/// are correctly rejected
+#[cfg(test)]
+#[cfg_attr(async_executor_impl = "tokio", tokio::test(flavor = "multi_thread"))]
+#[cfg_attr(async_executor_impl = "async-std", async_std::test)]
+#[cfg(feature = "test-srs")]
+async fn test_invalid_vid_disperse() {
+    use hotshot_testing::{
+        helpers::{build_payload_commitment, build_vid_proposal},
+        test_builder::TestDescription,
+    };
+    use hotshot_types::traits::{
+        consensus_api::ConsensusApi, network::Topic, node_implementation::NodeType,
+    };
+
+    async_compatibility_layer::logging::setup_logging();
+    async_compatibility_layer::logging::setup_backtrace();
+
+    let handle = build_system_handle::<TestTypes, MemoryImpl, TestVersions>(0)
+        .await
+        .0;
+
+    let quorum_membership = handle.hotshot.memberships.quorum_membership.clone();
+    let da_membership = handle.hotshot.memberships.da_membership.clone();
+
+    let mut generator =
+        TestViewGenerator::generate(quorum_membership.clone(), da_membership.clone());
+
+    let mut proposals = Vec::new();
+    let mut leaders = Vec::new();
+    let mut votes = Vec::new();
+    let mut dacs = Vec::new();
+    let mut vids = Vec::new();
+    for view in (&mut generator).take(1).collect::<Vec<_>>().await {
+        proposals.push(view.quorum_proposal.clone());
+        leaders.push(view.leader_public_key);
+        votes.push(view.create_quorum_vote(&handle));
+        dacs.push(view.da_certificate.clone());
+        vids.push(view.vid_proposal.clone());
+    }
+
+    let vid_scheme =
+        vid_scheme_from_view_number::<TestTypes>(&quorum_membership, ViewNumber::new(0));
+
+    let corrupt_share = vid_scheme.corrupt_share_index(vids[0].0[0].data.share.clone());
+
+    // Corrupt one of the shares
+    let mut share = vid_share(&vids[0].0, handle.public_key());
+    share.data.share = corrupt_share;
+
+    let inputs = vec![random![
+        VidShareRecv(share),
+        DaCertificateRecv(dacs[0].clone()),
+        QuorumProposalRecv(proposals[0].clone(), leaders[0]),
+    ]];
+
+    // If verify_share does not correctly handle this case, a `QuorumVote`
+    // will be emitted and cause a test failure
+    let expectations = vec![Expectations::from_outputs(all_predicates![
+        validated_state_updated(),
+        exact(ViewChange(ViewNumber::new(1))),
+        quorum_proposal_validated(),
+    ])];
+
+    let consensus_state =
+        ConsensusTaskState::<TestTypes, MemoryImpl, TestVersions>::create_from(&handle).await;
+    let mut consensus_script = TaskScript {
+        timeout: TIMEOUT,
+        state: consensus_state,
+        expectations,
+    };
+
+    run_test![inputs, consensus_script].await;
+}

crates/types/src/vid.rs

@@ -231,6 +231,12 @@ impl VidScheme for VidSchemeType {
     fn get_multiplicity(common: &Self::Common) -> u32 {
         <Advz as VidScheme>::get_multiplicity(common)
     }
+
+    /// Helper function for testing only
+    #[cfg(feature = "test-srs")]
+    fn corrupt_share_index(&self, share: Self::Share) -> Self::Share {
+        self.0.corrupt_share_index(share)
+    }
 }
 
 impl PayloadProver<LargeRangeProofType> for VidSchemeType {