Skip to content

Light state#2019

Merged
obscuren merged 1 commit intoethereum:developfrom
zsfelfoldi:light-state
Dec 18, 2015
Merged

Light state#2019
obscuren merged 1 commit intoethereum:developfrom
zsfelfoldi:light-state

Conversation

@zsfelfoldi
Copy link
Copy Markdown
Contributor

This PR implements ODR capable state & trie structures for the light client.

@robotally
Copy link
Copy Markdown

Vote Count Reviewers
👍 2 @fjl @zelig
👎 1 @obscuren

Updated: Thu Dec 17 15:10:10 UTC 2015

@codecov-io
Copy link
Copy Markdown

Current coverage is 44.13%

Merging #2019 into develop will increase coverage by +0.25% as of fa89320

Powered by Codecov. Updated on successful CI builds.

@zsfelfoldi zsfelfoldi force-pushed the light-state branch 4 times, most recently from eaec7f2 to 5ab52d8 Compare December 1, 2015 13:03
@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

@obscuren fixed the issues you mentioned and rebased on develop

@zsfelfoldi zsfelfoldi force-pushed the light-state branch 3 times, most recently from ef5968d to 053cd5f Compare December 4, 2015 18:32
Comment thread light/state_object.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to find a way to share some of the logic with package state.
Most of the code (decoding, accessors, etc.) is simply copied from package state.

Maybe one way to do it would be to split state.StateObject into two parts, one of which doesn't
contain a DB. In that case, you could simply embed it here and get all the boring methods for free.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On second thought, we could also address the duplication later when the protocol is in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The highest priority is to not mess up existing stuff so I'd definitely wait with that until we have a functional light client. After everything is built up (including the VM calls) we can make better decisions about what code can be shared and how that should be done.

@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

@fjl the issues you mentioned are fixed now

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Dec 13, 2015

👍

Comment thread light/odr.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you wrap the request in a struct, you can DRY up all the GetCtx redundancies.

type OdrBackend interface {
   Database() ethdb.Database
  Retrieve(req *OdrRequestWithCtx)
}
type OdrRequestWithCtx struct {
 ctx   context.Context
 OdrRequest
}

type OdrRequest interface {
  StoreResult(db ethdb.Database)
}
// GetCtx not needs to be defined exactly once not 5 times
func (req *OdrRequestWithCtx) GetCtx() context.Context { return req.ctx }


// this now does not need the ctx field and the GetCtx method to be defined
type TrieRequest struct {
    root  common.Hash
    key   []byte
    proof []rlp.RawValue
}

But the question is, do you really need the ctx embedded in the request? cant you always just pass it to Retrieve if it was OdrBackend#Retrieve(req OdrRequest, context.Context)
and eliminate the extra wrapping, GetCtx and ctx fields altogether.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, that was a remnant of an old design. I don't want it wrapped in a struct but contexts shouldn't be wrapped anyways so they are now directly passed to Retrieve.

@zelig
Copy link
Copy Markdown
Contributor

zelig commented Dec 16, 2015

👍 great stuff

@obscuren
Copy link
Copy Markdown
Contributor

@fjl renamed a few methods in #2064 such as NewStateObjectFromBytes to DecodeObject. Please change this PR so it matching the other.

Additionally you need to document the exported methods :-)

@obscuren
Copy link
Copy Markdown
Contributor

👎 (just so it shown up for @robotally and doesn't get accidentally merged)

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Dec 16, 2015

@zsfelfoldi I'll update #2064 tonight so it is mergeable

@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

@obscuren I updated the entire stateobject.go with all the rlp changes and added missing comments to trie.go. I did not yet comment all the state and stateobject functions because there's a shitload of them and they're not commented in the original version either :) I'll do those too of course but maybe I should do that in the two versions together in a separate PR after both this PR and #2064 are merged.

@obscuren
Copy link
Copy Markdown
Contributor

I don't see why we should neglect on commenting because someone else has a year ago (me). Since this PR adds the methods we should add the documentation in this PR.

@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

@obscuren ok, I'll do it in this PR today.

@zsfelfoldi
Copy link
Copy Markdown
Contributor Author

@obscuren done

obscuren added a commit that referenced this pull request Dec 18, 2015
@obscuren obscuren merged commit fd69d2b into ethereum:develop Dec 18, 2015
@obscuren obscuren removed the review label Dec 18, 2015
@obscuren obscuren modified the milestone: 1.4.0 Feb 6, 2016
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Dec 19, 2023
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.

6 participants