Skip to content

light client core#1989

Closed
zsfelfoldi wants to merge 4 commits intoethereum:developfrom
zsfelfoldi:light-core
Closed

light client core#1989
zsfelfoldi wants to merge 4 commits intoethereum:developfrom
zsfelfoldi:light-core

Conversation

@zsfelfoldi
Copy link
Copy Markdown
Contributor

No description provided.

@robotally
Copy link
Copy Markdown

Vote Count Reviewers
👍 0
👎 0

Updated: Tue Nov 24 04:01:28 UTC 2015

@codecov-io
Copy link
Copy Markdown

Current coverage is 46.72%

Merging #1989 into develop will increase coverage by +0.05% as of 1101763

Powered by Codecov. Updated on successful CI builds.

Comment thread xeth/xeth.go
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is the "TODO" still relevant?

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.

It is, retrieving individual receipts by tx hash doesn't work yet, only retrieving all receipts from a block by block hash. We have to add some new messages to the LES/1 protocol to cover relaying transactions and checking their status but it's still being figured out.

@zsfelfoldi zsfelfoldi self-assigned this Nov 21, 2015
Comment thread core/access/peer.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.

var list []*Peer
for _, p := range ps.peers {
    list = append(list, p)
}

or

list := make([]*Peer, len(ps.peers))
var i int
for _, p := range ps.peers {
   list[i]=p
   i++
}

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.

This is actually fine since append does not grow the array. Just the slice and there's no additional overhead of realloc every time append is called.

@karalabe
Copy link
Copy Markdown
Member

One general comment. I wouldn't rename the db fields everywhere to ca. CA usually represents Certificate Authority and imho it's kind of misleading/confusing as to what it is. The db naming should be fine even if it's a "remote access thing". It conveys the idea of what it is much better vs. having to think what a ca is.

Comment thread core/access/chain_access.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't really see the value in this method. It just wraps the one below, gives it a weird name and provides confusion as to why the same thing can be done multiple ways. NewChainAccess(db, false) would be ihmo just fine to call from client code.

@karalabe
Copy link
Copy Markdown
Member

The PR very heavily leaked networking/protocol primitives into the database access layer. IMHO, no notion of peers, peer sets, packets, protocols, etc should be allowed in core or core/access. Please remove these as their proper place will be in ethereum/les and simply define an interface (that ethereum most probably will implement) through which you can request the light ethereum object to fetch you some data.

The access layer in core should be simply an adapter to either a database, or a light ethereum object. It must not have a clue about how the data is retrieved by the underlying implementations. My suggestion would be to define maybe two interfaces: a full db and light db that other objects can implement. Then ethdb would implement full db, ethereum/light would implement light db. This way the LES protocol would be completely outside of core, which was the original idea behind the PR split in the first place.

Comment thread core/chain_makers.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't create a new access layer every time you want to generate a chain. Pass it as a parameter instead of the db ethdb.Database. Maybe @fjl has an input on this? But IMHO if we replace the databases with accessor objects, then they should be fully replaced from their point of creation and not just wrapped at the last minute (would be very very wasteful memory wise to have a lot of small wraps deep in the code).

@karalabe
Copy link
Copy Markdown
Member

Please gofmt your code. There are a lot of changes which broke the formatting, some introduced very weird ones actually.

Comment thread core/requests/trie_access.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.

it is thread safe to write the results because Deliver that calls this lock chainAccess including all requests sent. Please comment that caller must hold the lock over req to be used concurrently.
the same for Valid functions of all request types

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants