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.

Sign up for GitHub

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

Jump to bottom

feat: Return root result without blocking due to sparse trie Drop #14333

Merged
merged 5 commits into from
Feb 12, 2025
Merged

feat: Return root result without blocking due to sparse trie Drop #14333

merged 5 commits into from
Feb 12, 2025

Conversation

varun-doshi
Copy link
Contributor

@varun-doshi varun-doshi commented Feb 8, 2025 •
edited by Rjected
Loading

Fixes #14316

@emhane
Copy link
Member

emhane commented Feb 8, 2025

duplicate of #14331

@emhane emhane added C-perf A change motivated by improving speed, memory usage or disk footprint A-trie Related to Merkle Patricia Trie implementation labels Feb 8, 2025
emhane
emhane requested changes Feb 8, 2025
View reviewed changes
crates/engine/tree/src/tree/root.rs Outdated Show resolved Hide resolved
@varun-doshi varun-doshi requested a review from emhane February 9, 2025 06:04
emhane
emhane requested changes Feb 9, 2025
View reviewed changes
crates/trie/parallel/src/root.rs Outdated Show resolved Hide resolved
@varun-doshi varun-doshi requested a review from emhane February 9, 2025 14:56
emhane
emhane approved these changes Feb 9, 2025
View reviewed changes
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

approving stylistic changes. not sure if it was out of scope to change the panics to errors, pending @Rjected

crates/trie/parallel/src/root.rs Outdated
#[error("retention must be enabled")]
RetentionNotEnabled,
/// Sparse Tree not revealed and Retention not enabled .
#[error("Both root and trie_update should be visible")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[error("Both root and trie_update should be visible")]
#[error("both root and trie update should be visible")]

nitpick, discouraged to use variable names in error messages

Rjected
Rjected reviewed Feb 11, 2025
View reviewed changes
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

I'd like to wait until #14393 is in, because that will let us get rid of two of these error variants and greatly simplify the errors here

@Rjected Rjected mentioned this pull request Feb 11, 2025
Closed
@Rjected
Copy link
Member

Rjected commented Feb 12, 2025

reverted the error changes because I think we should instead either create a new sparse-trie specific variant of the ParallelStateRoot error, or create a new error type for sparse trie, and convert the expects + custom errors all at once

Rjected
Rjected approved these changes Feb 12, 2025
View reviewed changes
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

ty @varun-doshi!

@emhane emhane added this pull request to the merge queue Feb 12, 2025
Merged via the queue into paradigmxyz:main with commit faa6b9c Feb 12, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rjected Rjected Rjected approved these changes

@emhane emhane emhane approved these changes

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin

Assignees
No one assigned
Labels
A-trie Related to Merkle Patricia Trie implementation C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return root result without blocking due to sparse trie Drop
3 participants
@varun-doshi @emhane @Rjected