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

[CX-CLEANUP] - DA Integrated Storage #2799

Merged
merged 6 commits into from
Mar 19, 2024
Merged

[CX-CLEANUP] - DA Integrated Storage #2799

merged 6 commits into from
Mar 19, 2024

Conversation

jparr721
Copy link
Contributor

@jparr721 jparr721 commented Mar 18, 2024

Closes #2621

We want to integrate a persistent storage interface within HotShot to enable the application to manage data storage effectively. This feature aims to address potential data storage issues (e.g., full disk space) that can prevent nodes from storing DA blobs or VID shares.

This PR:

  1. Creates Storage Trait
  2. Initializes Storage in SystemTask's init
  3. Add Storage Operations on DA Proposals
    • If this fails, do not vote on the proposal and exit the event handler.
  4. Add Storage Operations on DA Proposals VID Shares
    • If this fails, do not vote on the VID disperse, otherwise, use the old logic to vote.
  5. Refactor In-memory Payload/VID Stores: (Tracking [CX_CLEANUP] - Remove BlockPayload from Leaf and associated types. #2808)

This code also implements a number of changes to introduce a TestBlockStorage type for use in the various testing components.

This PR does not:

Key places to review:

All of the tests and test interface changes. I had to update the view_generator to support the various DA related things that we wanted to add.

@jparr721 jparr721 changed the title [Draft][CX-CLEANUP] - DA Integrated Storage [CX-CLEANUP] - DA Integrated Storage Mar 18, 2024
bfish713
bfish713 previously approved these changes Mar 19, 2024
Copy link
Collaborator

@bfish713 bfish713 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, but lets give @jbearer some time to answer your question

inner: Arc<RwLock<TestStorageState<TYPES>>>,

/// `should_return_err` is a testing utility to validate negative cases.
pub should_return_err: bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to thoughts here. See the tests below for examples. I just need a cheesy "fail immediately" system that does not change the interface. The idea here is that I just want to make sure that any logical changes can catch a regression in the failure case for an append.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is fine, it's just a dummy type for our tests anyway

// Run view 1 (the genesis stage).
let view_1 = TestScriptStage {
inputs: vec![
ViewChange(ViewNumber::new(1)),
Copy link
Contributor Author

@jparr721 jparr721 Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copied direct from the test as it was just to maintain parity. I've been trying to modernize tests as I touch them.

@jparr721 jparr721 requested a review from bfish713 March 19, 2024 14:50
@jparr721 jparr721 merged commit d5aa12b into main Mar 19, 2024
13 checks passed
@jparr721 jparr721 deleted the jp/issue-2621 branch March 19, 2024 15:33
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.

[CX-CLEANUP] - DA Integrated Storage
3 participants