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

Recording partial trie for storage proofs #1177

Merged
merged 2 commits into from
Aug 16, 2019
Merged

Recording partial trie for storage proofs #1177

merged 2 commits into from
Aug 16, 2019

Conversation

mikhailOK
Copy link
Contributor

Record every read of a trie node from storage to use
the recorded values for a proof of execution.

This includes all key paths for keys requested for reads, but also
in case of updates can include additional nodes which are
necessary to provide a canonical resulting trie (i.e. no branch nodes
with <= 1 branch and extension is always followed by a branch).

@MaksymZavershynskyi
Copy link
Contributor

Should merge into staging instead of master.

core/store/src/trie/mod.rs Show resolved Hide resolved
let result = if let Ok(Some(bytes)) = self.store.get(COL_STATE, hash.as_ref()) {
Some(bytes)
let result = {
let mut guard = self.cache.lock().expect(POISONED_LOCK_ERR);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for poisoned locks it makes sense to just unwrap. If lock is poisoned the program will fail with verbose message anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have this pattern everywhere, it's for readability I think

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked to introduce this pattern before. However, now I realized the following:

  • If the Error that we are unwrapping implements Debug the result of calling unwrap on it will be panic with that error message printed.
  • If we say expect("some custom error message") then the result is panic with the custom error message printed.

So unless there is any additional information to say, besides the fact that "lock is poisoned" there is no real reason to use expect since unwrap will have the same effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a code readability pattern - expect() with an explanation that it can never fail or that it's impossible to recover from the error. More explicit than unwrap()

Copy link
Contributor

Choose a reason for hiding this comment

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

unwrap() implies exactly the same semantics. Anywhere in the code if you see unwrap it implies that this should never fail. Are you saying the word unwrap is less descriptive than the word expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let mut guard = self.cache.lock().expect(POISONED_LOCK_ERR);
or
let mut guard = self.cache.lock().unwrap(); // the only possible failure is poisoned lock

chain/chain/src/test_utils.rs Outdated Show resolved Hide resolved
core/store/src/trie/mod.rs Outdated Show resolved Hide resolved
core/store/src/trie/mod.rs Outdated Show resolved Hide resolved
@mikhailOK mikhailOK changed the base branch from master to staging August 16, 2019 17:30
Record every read of a trie node from storage to use
the recorded values for a proof of execution.

This includes all key paths for keys requested for reads, but also
in case of updates can include additional nodes which are
necessary to provide a canonical resulting trie (i.e. no branch nodes
with <= 1 branch and extension is always followed by a branch).
@MaksymZavershynskyi
Copy link
Contributor

Let us know when to review again.

@mikhailOK mikhailOK merged commit e044878 into staging Aug 16, 2019
@bowenwang1996 bowenwang1996 deleted the partial_trie branch August 30, 2019 02:33
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