Add test for verify_share return type #3590
Conversation
| @@ -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")] | |||
There was a problem hiding this comment.
I don't think we ever run tests with test-srs -- it doesn't seem to even be in the justfile. Is this just for downstream users like the sequencer?
There was a problem hiding this comment.
Oops, ignore this one -- I see it was used in the test which was also feature gated
There was a problem hiding this comment.
Yes, that's intentional that we never run with this feature. It is a feature to match the 'test-srs' feature in jellyfish.
| #[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")] |
There was a problem hiding this comment.
Is it possible to write this test without the feature gate so that it runs in CI? We can run it with the feature gate, but it would take more work and we'd have to add it separately
There was a problem hiding this comment.
I feature gated it because it relies on changes in Jellyfish that may not be merged for a while. So I don't think it makes sense to run it in CI until jellyfish is updated.
There was a problem hiding this comment.
Also, as I suggested above, it may make sense just to merge the fix for now, and wait to merge the test until later.
Closes #<ISSUE_NUMBER>
This PR:
This PR fixes a bug where an invalid vid share could be marked as valid. Specifically, the
verify_sharefunction from jellyfish returns a nestedResult. Previously we were only checking the outer result. This means that a call toverify_sharethat returned a result ofOk(Err)would be incorrectly returntrue. This PR checks both the inner and outer results.This PR does not:
Key places to review:
task_impls/src/consensus/mod.rstesting/test-1/consensus_task.rsHow to test this PR:
To test this PR, check out this branch. Next, check out the following branch of jellyfish locally,
309fb5ba80a2b401dc7d5c355ff5cecece8fa412. This jellyfish branch contains a function to create a corrupt VID share in the exact way we were not covering before. This commit in HotShot contains thetest_invalid_vid_dispersetest, but it is currently ignored. To test that the test is doing what it is supposed to, revert the change in theconsensus/mod.rsfile to what it was previously. The test will now fail.To run the test use the following command:
just tokio test test_invalid_vid_disperse --features=test-srsNOTE: This test is ignored in the final commit because the corresponding function in jellyfish will likely not be merged soon, if at all. The test is gated by the
test-srsfeature to ensure it does not compile during normal testing. The point is just to demonstrate the old bug and that it was fixed. It might make sense to only merge the bug fixes and not the tests.Tagging @jparr721 in case we want to look for bugs of a similar class.