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

feat(storage): support pruning Merkle tries #1747

Merged
merged 7 commits into from
Apr 2, 2024

Conversation

kkovaacs
Copy link
Contributor

@kkovaacs kkovaacs commented Feb 6, 2024

This PR adds support for a mode of operation where only the last Merkle tries are kept in storage.

Storage can now be configured to prune old nodes when doing trie updates. Upon creating the storage this setting is validated to be safe. Pruning can be enabled only for databases with empty tries and the fact that pruning was enabled is stored in a new storage_flags table.

The storage.prune-state-tries command line flag is added to explicitly set the pruning mode. When not specified the default is the setting set in the database. For new databases pruning is disabled unless explicitly requested via --storage.prune-state-tries true.

Closes #1488

@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch from 481820e to da6525f Compare February 9, 2024 13:18
@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch from da6525f to 4516de3 Compare March 18, 2024 15:54
@kkovaacs kkovaacs changed the base branch from main to krisztian/reverse-state-update March 18, 2024 15:55
Base automatically changed from krisztian/reverse-state-update to main March 19, 2024 12:06
@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch 5 times, most recently from d72b5af to 7b36566 Compare March 22, 2024 13:30
@kkovaacs kkovaacs changed the base branch from main to krisztian/merkle-tree-track-removed-nodes March 22, 2024 13:31
Base automatically changed from krisztian/merkle-tree-track-removed-nodes to main March 22, 2024 13:48
@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch from 7b36566 to 5b6a50a Compare March 22, 2024 13:49
@kkovaacs kkovaacs marked this pull request as ready for review March 22, 2024 15:19
@kkovaacs kkovaacs requested a review from a team as a code owner March 22, 2024 15:19
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

LGTM; some (very) minor comments which you can ignore.

Am I correct in saying this isn't actually enabled within the sync process yet?

I am in fact just blind. Its internalised/abstracted within the storage code.

Comment on lines 243 to 256
let prune_flag_is_set = connection
.query_row(
"SELECT 1 FROM storage_flags WHERE flag = 'prune_tries'",
[],
|_| Ok(()),
)
.optional()
.map(|x| x.is_some())?;

let tries_are_non_empty: bool = connection.query_row(
"SELECT EXISTS (SELECT 1 FROM trie_storage) OR EXISTS (SELECT 1 FROM trie_contracts) OR EXISTS (SELECT 1 FROM trie_class)",
[],
|row| row.get(0),
)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This works; but I'm thinking about the ergonomics long term here.

Ideally I think we would:

  1. Set the flag the very first time the db is created
  2. Only check the flag matches the provided one

Currently we can't do this because the rusqlite Connection::open by default enables the SQLITE_OPEN_CREATE flag.

What do you think about changing the first migration step to:

let mut connection = {
    // Try open existing db first
    let mut open_flags = rusqlite::OpenFlags::default();
    open_flags.remove(SQLITE_OPEN_CREATE);

    // TODO: actually ensure that the error is file not found..
    if let Ok(connection) = Connection::open_with_flags(open_flags) {
        break connection;
    }
    
    // Database file should be created, and flags should be set.
    let mut connection = Connection::open()?;
    // TODO: set the flag(s).
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed something similar as an update: 9ba6f1d

Does that make more sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its better; I don't like that the verification does both verification but also sometimes writes the flags.

// This also sets the one-time flags when creating the database.
let connection = open_or_create(flags)?;[
...
migratate(connection)?;

// Only verification/ensure db makes sense with options.
verify_config(connection)?;

But now that I consider the issue more closely - its that you need the migrations to be completed before you can write the flags? So one would have to bundle the open/create and migration into one step.. bleg.

Okay 👍 I give up 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this breaks enabling pruning for the in-memory DB created by StorageBuilder::in_memory(). That's because we do have to pre-create the in-memory DB and keep a connection open to that while migrate() is running. That's something we can take care of after merging this PR though?

@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch 2 times, most recently from ff6383b to f9d6047 Compare March 25, 2024 10:04
Copy link
Contributor

@sistemd sistemd left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe flags should be bool columns instead of one text column? Not sure if that makes sense, but might be easier to understand.

@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch 2 times, most recently from 9ba6f1d to 7474e76 Compare March 26, 2024 10:30
Copy link
Member

@CHr15F0x CHr15F0x left a comment

Choose a reason for hiding this comment

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

LGTM

@kkovaacs
Copy link
Contributor Author

I'll make this a draft until PR #1898 is merged. I've already resolved conflicts and would rather keep it that way.

@kkovaacs kkovaacs marked this pull request as draft March 26, 2024 14:47
@kkovaacs kkovaacs changed the base branch from main to sistemd/1843-split-receipts-and-events March 26, 2024 14:48
Base automatically changed from sistemd/1843-split-receipts-and-events to main March 27, 2024 11:48
@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch from 7474e76 to 9982ee3 Compare March 27, 2024 13:16
@sistemd sistemd force-pushed the krisztian/merkle-tree-prune branch from 9982ee3 to b3e00e7 Compare April 1, 2024 10:20
@kkovaacs kkovaacs marked this pull request as ready for review April 2, 2024 09:32
@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch from b3e00e7 to 6719b15 Compare April 2, 2024 09:43
@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch from 6719b15 to 366675f Compare April 2, 2024 09:47
@kkovaacs kkovaacs force-pushed the krisztian/merkle-tree-prune branch from 366675f to ae861be Compare April 2, 2024 09:49
@kkovaacs kkovaacs merged commit 704a3aa into main Apr 2, 2024
7 checks passed
@kkovaacs kkovaacs deleted the krisztian/merkle-tree-prune branch April 2, 2024 09:59
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.

Support pruning in the sync logic
4 participants