Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add test hook to corrupt share index #664

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

elliedavidson
Copy link
Member

@elliedavidson elliedavidson commented Aug 20, 2024

closes: #XXXX

This PR:

This PR adds a small testing function to corrupt VID share indices. The purpose of this function is to create VID shares that fail verify_share with an Ok(Err) result. This PR doesn't need to be merged - it is just to show the bug that was fixed in HotShot. It probably makes sense to instead write logic that returns any form of a corrupt share, instead of writing to specifically corrupt the index of the share.

This PR does not:

Key places to review:


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

@ggutoski ggutoski changed the title Ed/corrupt share index test: add test hook to corrupt share index Aug 20, 2024
@ggutoski
Copy link
Contributor

Thanks @elliedavidson ! Is there a reason not to merge this PR?

Sucks that you need to touch the VidScheme trait. That could probably be avoided by setting the share index to u32::MAX or whatever so you don't need self.num_storage_nodes. But that might not be worth the trouble.

@elliedavidson
Copy link
Member Author

Thanks @elliedavidson ! Is there a reason not to merge this PR?

Sucks that you need to touch the VidScheme trait. That could probably be avoided by setting the share index to u32::MAX or whatever so you don't need self.num_storage_nodes. But that might not be worth the trouble.

Let me try moving it out of VID Scheme!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants