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

Trie error handling #2005

Merged
merged 1 commit into from Dec 1, 2015

Conversation

Projects
None yet
7 participants
@zsfelfoldi
Copy link
Contributor

zsfelfoldi commented Nov 25, 2015

No description provided.

@robotally

This comment has been minimized.

Copy link

robotally commented Nov 25, 2015

Vote Count Reviewers
👍 2 @fjl @obscuren
👎 0

Updated: Tue Dec 1 11:28:25 UTC 2015

@zsfelfoldi zsfelfoldi self-assigned this Nov 25, 2015

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 25, 2015

Current coverage is 47.40%

Merging #2005 into develop will increase coverage by +0.15% as of 1154bae

Powered by Codecov. Updated on successful CI builds.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-trie branch Nov 26, 2015

@zsfelfoldi zsfelfoldi added pr:review and removed in progress labels Nov 26, 2015

@zelig

View changes

trie/trie.go Outdated
@@ -79,13 +82,16 @@ type Trie struct {
// New will panics if db is nil or root does not exist in the

This comment has been minimized.

@zelig

zelig Nov 26, 2015

Contributor

comment does not reflect what it actally does

@zelig

View changes

trie/secure_trie.go Outdated
}
}

// Delete removes any existing value for key from the trie.

This comment has been minimized.

@zelig

zelig Nov 26, 2015

Contributor

extend comment to differentiate from Delete

@karalabe

View changes

trie/trie.go Outdated

func unhandledError(err error) {
if glog.V(logger.Error) {
glog.Errorf("Unhandled trie error: %v", err)

This comment has been minimized.

@karalabe

karalabe Nov 26, 2015

Member

I don't think it's a good idea to put a log statement in a helper function. The reason is that logs are tagged with line numbers that should provide a hint as to where the issue went wrong. With this approach all error logs will have the same line number, loosing part of its information content.

Further, you can simplify the error reporting to:

glog.V(logger.Error).Errorf("Unhandled trie error: %v", err)

IMHO, simply replace all instances of unhandledError(err) in your current code with the line above. It's much clearer too what's going to happen.

This comment has been minimized.

@fjl

fjl Nov 26, 2015

Contributor

Errorf is not available as a method, unfortunately.
But you can combine the error and logging checks:

if err := operation(); err != nil && glog.V(logger.Error) {
     glog.Errorf("Unhandled trie error: %v", err)
}
return nil
@karalabe

View changes

trie/secure_trie.go Outdated
err := t.TryUpdate(key, value)
if err != nil {
unhandledError(err)
}

This comment has been minimized.

@karalabe

karalabe Nov 26, 2015

Member

You can shorten this to if err := ...; err != nil { ... }

@karalabe

View changes

trie/secure_trie.go Outdated
hk := t.hashKey(key)
t.Trie.Update(hk, value)
err := t.Trie.TryUpdate(hk, value)

This comment has been minimized.

@karalabe

karalabe Nov 26, 2015

Member

Same here, please shorten err assignment and check into the same if construct.

@karalabe

View changes

trie/secure_trie.go Outdated
}

// Delete removes any existing value for key from the trie.
func (t *SecureTrie) Delete(key []byte) {
t.Trie.Delete(t.hashKey(key))
err := t.TryDelete(key)

This comment has been minimized.

@karalabe

karalabe Nov 26, 2015

Member

Same, please shorten.

@karalabe

View changes

trie/trie.go Outdated
@@ -131,17 +153,41 @@ func (t *Trie) Get(key []byte) []byte {
// The value bytes must not be modified by the caller while they are
// stored in the trie.
func (t *Trie) Update(key, value []byte) {
err := t.TryUpdate(key, value)

This comment has been minimized.

@karalabe

karalabe Nov 26, 2015

Member

Please shorten.

@karalabe

View changes

trie/trie.go Outdated
@@ -185,28 +250,45 @@ func (t *Trie) insert(n node, key []byte, value node) node {

// Delete removes any existing value for key from the trie.
func (t *Trie) Delete(key []byte) {
err := t.TryDelete(key)

This comment has been minimized.

@karalabe

karalabe Nov 26, 2015

Member

Please shorten.

@karalabe

View changes

trie/trie.go Outdated
type MissingNodeError struct {
RootHash, NodeHash common.Hash
KeyPrefix, KeySuffix []byte
}

This comment has been minimized.

@karalabe

karalabe Nov 26, 2015

Member

Any particular reason this is in the middle of the file? :P

Please either move this to the top of the file, or better yet I would create an errors.go file to contain this. Usually that is the convention throughout our code base.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-trie branch Nov 27, 2015

@fjl

View changes

trie/proof.go Outdated
var err error
tn, err = t.resolveHash(n, nil, nil)
if err != nil && glog.V(logger.Error) {
glog.Errorf("Unhandled trie error: %v", err)

This comment has been minimized.

@fjl

fjl Nov 30, 2015

Contributor

Maybe return nil here? The proof will not be useful if the trie is incomplete.

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Nov 30, 2015

👍, please squash.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-trie branch Nov 30, 2015

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Nov 30, 2015

squashed.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-trie branch Nov 30, 2015

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Nov 30, 2015

fixed, added description for trie.MissingNodeError

@obscuren

This comment has been minimized.

Copy link
Member

obscuren commented Nov 30, 2015

Minor issue, please format commit messages like <changes packages>: commit message, e.g.

trie: changed error handling

Changed error handling such that X now performs in such a way that X can handle Z properly.
trie: added error handling
Created alternate versions of Trie and SecureTrie functions that can return a MissingNodeError (used by ODR services)

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-trie branch to 52904ae Dec 1, 2015

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Dec 1, 2015

@obscuren ok, I changed the commit message

@obscuren

This comment has been minimized.

Copy link
Member

obscuren commented Dec 1, 2015

👍

obscuren added a commit that referenced this pull request Dec 1, 2015

@obscuren obscuren merged commit 96d8674 into ethereum:develop Dec 1, 2015

4 checks passed

buildbot/ARM Go pull requests DEV build done.
Details
buildbot/Linux Go pull requests DEV build done.
Details
buildbot/Windows Go pull requests DEV build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@obscuren obscuren removed the pr:review label Dec 1, 2015

@obscuren obscuren modified the milestone: 1.4.0 Feb 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment