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

Reset cache if version becomes active #4791

Open
wants to merge 7 commits into
base: mainnet_2_3_versioning_mip
Choose a base branch
from

Conversation

Leo-Besancon
Copy link
Collaborator

  • document all added functions
  • try in sandbox /simulation/labnet
    • if part of node-launch, checked using the resync_check flag
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@Leo-Besancon Leo-Besancon requested a review from modship December 17, 2024 08:58
massa-execution-worker/src/execution.rs Outdated Show resolved Hide resolved
@@ -36,7 +36,7 @@ macro_rules! metadata_key {

pub(crate) struct HDCache {
/// RocksDB database
db: DB,
db: Option<DB>,
Copy link
Member

Choose a reason for hiding this comment

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

why it is now Option ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In order to destroy the DB to reset it, I needed a way to drop the current DB, which is not possible without the Option as I can't fully drop the HDCache.
The only alternative I thought was to iterate through the DB to remove elements, but this may be a heavy operation.

Copy link
Member

@damip damip Dec 19, 2024

Choose a reason for hiding this comment

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

the only other solution I see (without using Option) is

        unsafe {
            std::ptr::drop_in_place(&mut self.db);
            std::ptr::write(&mut self.db, create_new_db());
        }

but not great because unsafe

Copy link
Contributor

Choose a reason for hiding this comment

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

Maye we can use the unsafe snippet here but check with Miri first if the code looks ok and sound?
If you want I can have a look at it?

massa-module-cache/src/hd_cache.rs Show resolved Hide resolved
@Leo-Besancon Leo-Besancon marked this pull request as ready for review December 19, 2024 12:29
@Leo-Besancon Leo-Besancon requested review from sydhds and damip December 19, 2024 12:29
massa-execution-worker/src/execution.rs Outdated Show resolved Hide resolved
massa-execution-worker/src/execution.rs Show resolved Hide resolved
@@ -36,7 +36,7 @@ macro_rules! metadata_key {

pub(crate) struct HDCache {
/// RocksDB database
db: DB,
db: Option<DB>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maye we can use the unsafe snippet here but check with Miri first if the code looks ok and sound?
If you want I can have a look at it?

@Leo-Besancon Leo-Besancon force-pushed the reset_module_cache_on_version_active branch from 20bc306 to 1b5868b Compare December 19, 2024 15:21
@Leo-Besancon Leo-Besancon force-pushed the reset_module_cache_on_version_active branch from 1b5868b to b1c8da3 Compare December 19, 2024 16:26
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.

4 participants