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

Merklized but not diffed #3293

Merged
merged 8 commits into from
May 24, 2024
Merged

Merklized but not diffed #3293

merged 8 commits into from
May 24, 2024

Conversation

yito88
Copy link
Member

@yito88 yito88 commented May 22, 2024

Describe your changes

  • Update some data in the subspace and the merkle tree, but not update the diffs
    • Add diff_key_filter to check if the data should be stored to diffs even if merkle_tree_key_filter is true
  • Add NoDiff subtree in Merkle tree
    • Need it because we can't rebuild each subtree when some data aren't in diffs

Indicate on which release or other PRs this topic is based on

v0.36.1

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 94.83871% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 53.82%. Comparing base (c7d79f4) to head (fc483da).
Report is 3 commits behind head on main.

Files Patch % Lines
crates/merkle_tree/src/lib.rs 86.84% 5 Missing ⚠️
crates/state/src/wl_state.rs 94.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3293      +/-   ##
==========================================
+ Coverage   53.79%   53.82%   +0.02%     
==========================================
  Files         314      314              
  Lines      105784   105843      +59     
==========================================
+ Hits        56903    56966      +63     
+ Misses      48881    48877       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

batconjurer
batconjurer previously approved these changes May 22, 2024
crates/node/src/shell/mod.rs Outdated Show resolved Hide resolved
);
let make_key = |suffix: u64| {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this doing exactly? Needs some comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a comment cd441a2

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what this whole suffix % 3u64 logic is doing, though?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also what does "probable" mean in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

these data require proofs of the existence or non-existence

let tree =
state.get_merkle_tree(height, Some(StoreType::NoDiff))?;
assert_eq!(tree.root().0, roots.get(&height).unwrap().0);
match write_type {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs comments, this logic is not clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a comment cd441a2

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the semantics of these 0/1/3 write types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

I still want clearer comments 😁

);
let make_key = |suffix: u64| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what this whole suffix % 3u64 logic is doing, though?

let tree =
state.get_merkle_tree(height, Some(StoreType::NoDiff))?;
assert_eq!(tree.root().0, roots.get(&height).unwrap().0);
match write_type {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the semantics of these 0/1/3 write types?

@yito88 yito88 requested a review from cwgoes May 22, 2024 20:35
@brentstone
Copy link
Collaborator

@yito88 @cwgoes do we need to use this for replay protection too or do we only need what is already in #3284?

let make_key = |suffix: u64| {
// For three type keys
match suffix % 3u64 {
// for non-probable data
Copy link
Contributor

Choose a reason for hiding this comment

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

provable?

match suffix % 3u64 {
// for non-probable data
0 => Key::parse(format!("key{suffix}")).unwrap(),
// for probable data
Copy link
Contributor

Choose a reason for hiding this comment

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

provable?

assert!(!tree.has_key(&merkle_key)?);
}
}
1 | 3 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

but what do these numbers 1 or 3 mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're indexes generated by proptest

        let blocks_write_type = blocks_write_type.into_iter().enumerate().map(
            |(index, write_type)| {

@cwgoes
Copy link
Contributor

cwgoes commented May 23, 2024

@yito88 @cwgoes do we need to use this for replay protection too or do we only need what is already in #3284?

We do not need to use this for replay protection, but we do need to merge this to include everything in the app hash.

@batconjurer
Copy link
Member

@yito88 @cwgoes do we need to use this for replay protection too or do we only need what is already in #3284?

That is indeed handled in #3284

tzemanovic added a commit that referenced this pull request May 23, 2024
* origin/yuji/merklized-not-diffed:
  fix typo
  add comments
  remove is_merklized_storage_key
  fix for tests
  add changelog
  fmt
  add NoDiff subtree
  add diff_key_filter
brentstone added a commit that referenced this pull request May 23, 2024
* origin/yuji/merklized-not-diffed:
  fix typo
  add comments
  remove is_merklized_storage_key
  fix for tests
  add changelog
  fmt
  add NoDiff subtree
  add diff_key_filter
@brentstone brentstone merged commit 0e59bbc into main May 24, 2024
18 of 19 checks passed
@brentstone brentstone deleted the yuji/merklized-not-diffed branch May 24, 2024 03:08
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.

5 participants