Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

Problem (Fix #1307): many unused features and code in merkle trie code dependency #1424

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Apr 14, 2020

Solution:

  • Cleanup jellyfish library
  • Add the integration ground work for jellyfish, step1 of adr-003

Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

looks ok -- changelog can be updated?

}

fn get_rightmost_leaf(&self) -> Result<Option<(NodeKey, LeafNode)>> {
unimplemented!("this feature is only used in merkle tree restore which we don't need yet");
Copy link
Contributor

Choose a reason for hiding this comment

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

the normal "restore" (just brining up the last version number) is enough? when is this needed?

Copy link
Collaborator Author

@yihuang yihuang Apr 14, 2020

Choose a reason for hiding this comment

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

I don't fully understand the purpose of the restore yet, but it seems like a way to set up a partial tree first(with root hash fixed), then adding accounts to it gradually.
I think you can simply insert the accounts into an empty tree, then verify the root hash.

Comment on lines +35 to +38
/// Column for merkle trie storage
pub const COL_TRIE_NODE: u32 = 9;
/// Column for staled node key in merkle trie
pub const COL_TRIE_STALED: u32 = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

this will already "break" backwards-compatibility in RocksDB, as older code would expect a different number of columns, I think -- would be good to add a changelog entry

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@codecov
Copy link

codecov bot commented Apr 14, 2020

Codecov Report

Merging #1424 into master will increase coverage by 0.25%.
The diff coverage is 90.14%.

@@            Coverage Diff             @@
##           master    #1424      +/-   ##
==========================================
+ Coverage   64.24%   64.50%   +0.25%     
==========================================
  Files         160      161       +1     
  Lines       21472    21685     +213     
==========================================
+ Hits        13794    13987     +193     
- Misses       7678     7698      +20     
Impacted Files Coverage Δ
chain-storage/src/lib.rs 53.40% <ø> (ø)
chain-storage/src/jellyfish.rs 90.14% <90.14%> (ø)
chain-core/src/common/merkle_tree.rs 98.01% <0.00%> (+0.22%) ⬆️

@tomtau
Copy link
Contributor

tomtau commented Apr 14, 2020

bors r+

bors bot added a commit that referenced this pull request Apr 14, 2020
1424: Problem (Fix #1307): many unused features and code in merkle trie code dependency r=tomtau a=yihuang

Solution:
- Cleanup jellyfish library
- Add the integration ground work for jellyfish, step1 of adr-003

Co-authored-by: yihuang <yi.codeplayer@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 14, 2020

Canceled.

…e trie code dependency

Solution:
- Cleanup jellyfish library
- Add the integration ground work for jellyfish, step1 of adr-003
@tomtau
Copy link
Contributor

tomtau commented Apr 14, 2020

bors r+

Copy link
Collaborator

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

@bors
Copy link
Contributor

bors bot commented Apr 14, 2020

Build succeeded:

@bors bors bot merged commit 55d1f1b into crypto-com:master Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants