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

Move chainstate storage init inside constructor #1118

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

azarovh
Copy link
Member

@azarovh azarovh commented Aug 1, 2023

This is a fix for previous review #1098:

  • Create dedicated type for storage version
  • Avoid using mut storage in check functions

Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

Nice, definitely an improvement. Some suggestions below:

chainstate/storage/src/internal/mod.rs Outdated Show resolved Hide resolved
chainstate/storage/src/internal/version.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

This looks fine. Though just a question out of carefulness: I remember we deleting only the database directory. Please confirm that's the case, and that we're not deleting the whole data directory. This is because a user setting a wrong data directory could cause a disaster if a wildcard is used.

@azarovh
Copy link
Member Author

azarovh commented Aug 1, 2023

Though just a question out of carefulness: I remember we deleting only the database directory.

yes we are removing only that. So in case chainstate is configured with LMDB ~/Library/Application Support/Mintlayer/testnet/chainstate-lmdb will be removed.

@azarovh azarovh force-pushed the fix/storage_version_check_mut branch from 0e67fa8 to 552e5da Compare August 1, 2023 18:58
Copy link
Contributor

@muursh muursh left a comment

Choose a reason for hiding this comment

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

Looks good to me

chainstate/src/detail/error.rs Show resolved Hide resolved
chainstate/storage/src/internal/version.rs Outdated Show resolved Hide resolved
use serialization::{Decode, Encode};

#[derive(Debug, Encode, Decode, Clone, Copy, Eq, PartialEq)]
pub struct ChainstateStorageVersion(u32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've seen many discussions here about what to do about this... and I just want to point out that if we want to make it perfect, we can put all the information (version, magic bytes and net type) in one struct. But I don't really care that much. I'm OK with the way things are now as long as it doesn't lead to a crash and the worst thing that can happen is a wipe and resync.

@azarovh azarovh merged commit b798498 into master Aug 2, 2023
23 checks passed
@azarovh azarovh deleted the fix/storage_version_check_mut branch August 2, 2023 12:41
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.

None yet

5 participants