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

Light client stage 1 #1684

Closed
wants to merge 8 commits into
base: develop
from

Conversation

Projects
None yet
6 participants
@zsfelfoldi
Copy link
Contributor

zsfelfoldi commented Aug 19, 2015

Basic concept of adding a "light" mode to the geth client is the following:

  • package "les" implements a subprotocol which duplicates some code from "eth". It has its own protocol manager which has its own eth/downloader instance (downloader package is not duplicated) that handles header sync in light mode. In this case, the downloader of the eth pm is disabled.
  • "les" protocol manager works in full mode too, in this case only serving retrieval requests of light peers
  • in light mode, BlockChain.currentHeader represents the head of the canonical chain. Functions CurrentBlock(), CurrentBlockHeader(), LastBlockNumberU64(), LastBlockHash() (and other functions referring to the head canonical hash) all use currentHeader in light mode and currentBlock in full mode.
  • package "les/access" implements a ChainAccess object which replaces ethdb.Database wherever ODR (on-demand retrieval) is needed.
  • requests are encoded in different implementations of the ObjectAccess interface (BlockAccess, ReceiptsAccess, NodeDataAccess, TrieEntryAccess). The original database fetch/store code is moved to the DbGet/DbPut functions of these implementations. Request function initiates ODR from a LES peer, Valid function checks if a message is a valid answer to the particular request and if it is, also stores the answer in memory.
  • requests are passed to ChainAccess.Retrieve, which calls the above mentioned functions of the request
  • an OdrContext should also be passed to Retrieve, which can timeout or manually abort the blocking request and also remember if it had actually been cancelled. Contexts are created at RPC level (currently only default timeouts are used, manual cancel will be implemented in the new RPC).
  • contexts are passed to block and receipts retrieval functions and state.New/Copy. Passing NoOdr or NullCtx means ODR is not used.
  • individual receipt retrieval (by tx hash) is not yet implemented, it will be added together with the transaction relaying extension of LES.
@robotally

This comment has been minimized.

Copy link

robotally commented Aug 19, 2015

Vote Count Reviewers
👍 0
👎 0

Updated: Thu Nov 5 07:59:50 UTC 2015

@zsfelfoldi zsfelfoldi changed the title ChainAccess layer skeleton Light client Aug 19, 2015

@zsfelfoldi zsfelfoldi changed the title Light client Light client stage 1 Aug 19, 2015

@obscuren

View changes

core/access/chain_access.go Outdated
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

// Package core implements the Ethereum consensus protocol.

This comment has been minimized.

@obscuren

obscuren Aug 19, 2015

Member

Package access ... :-)

@obscuren obscuren modified the milestone: Light client stage 1 Aug 21, 2015

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

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch 4 times, most recently Sep 8, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch Sep 23, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch 3 times, most recently Sep 30, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch 2 times, most recently Oct 11, 2015

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Oct 11, 2015

see to do list here:
#1890

@zsfelfoldi zsfelfoldi referenced this pull request Oct 11, 2015

Closed

Light client stage 1 "to do" list #1890

9 of 9 tasks complete

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch Oct 14, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch 2 times, most recently Oct 22, 2015

@zelig

View changes

les/handler.go Outdated
defer msg.Discard()

// Handle the message depending on its contents
switch {

This comment has been minimized.

@zelig

zelig Oct 23, 2015

Contributor

you can use

switch msg.Code {
    case StatusMsg:
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 24, 2015

Current coverage is 46.41%

Merging #1684 into develop will decrease coverage by -0.04% as of b7a8344

Powered by Codecov. Updated on successful CI builds.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch 3 times, most recently Oct 29, 2015

// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

// Package les implements the Light Ethereum Subprotocol.
package les

This comment has been minimized.

@karalabe

karalabe Nov 4, 2015

Member

Don't document the les package in every file. Pick whatever one you consider to be the main file and add the comment only there.


// Handshake executes the les protocol handshake, negotiating version number,
// network IDs, difficulties, head and genesis blocks.
func (p *peer) Handshake(td *big.Int, head common.Hash, genesis common.Hash) error {

This comment has been minimized.

@karalabe

karalabe Nov 4, 2015

Member

The handshake code was fixed in eth to work around some faulty peer disconnection issue. Please pull in a new version from there https://github.com/ethereum/go-ethereum/blob/develop/eth/peer.go#L284.

// String implements fmt.Stringer.
func (p *peer) String() string {
return fmt.Sprintf("Peer %s [%s]", p.id,
fmt.Sprintf("les/%d", p.version),

This comment has been minimized.

@karalabe

karalabe Nov 4, 2015

Member

fmt.Sprintf("Peer %s [les/%d]", p.id, p.version)

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light branch to 2c3dba3 Nov 4, 2015

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

A thing newly introduced into develop is detailed information reporting capability from subprotocol level (i.e. any suprotocol can add pieces of information to admin.nodeInfo and admin.peers). Please look through this PR for the details and update LES too to include these additions: #1934.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

The code contains a ton of log entries in the following form:

glog.V(access.LogLevel).Infof("LES: received ReceiptsMsg from peer %v", p.id)

Two issues:

  • What is access.LogLevel good for? The point of the log level here is that you specify the level above which you want this log entry displayed. It's not something that should dynamically change.
  • Please drop from the LES: prefix from all the logs. Unless we're willing to rework all the logs in the entire system to prefix subprotocols, it should stay consistent with the others.
Name: "mode",
Value: "archive",
Usage: "Client mode of operation (archive, full, light)",
}

This comment has been minimized.

@karalabe

karalabe Nov 4, 2015

Member

This flag was dropped because it's too murky what it represents. Please rework the code so that it is based on a single --light flag. If it is passed, the cmd/geth/ package should initialize eth and les properly.

clientMode = eth.LightMode
default:
glog.Fatalf("Unknown node type requested: %s", ctx.GlobalString(EthModeFlag.Name))
}

This comment has been minimized.

@karalabe

karalabe Nov 4, 2015

Member

Please drop these. They do not make sense in the context of the ETH protocol any more. The fast sync was already integrated separately as a bool flag. Why do you need to specify this mode to the eth package? If you are a light client, you should simply never even construct an eth object, so I don't see any rational behind modifying the eth.Config and the related things.

This comment has been minimized.

@zsfelfoldi

zsfelfoldi Nov 4, 2015

Contributor

The whole backend is in eth, I definitely need that, I think duplicating the backend would be wrong. I just won't construct an eth ProtocolManager (I do agree about dropping NoSync, it was a bad idea). I can drop the mode flag and add a bool flag for light mode of course.

}

var NullCtx = (*OdrContext)(nil) // used when creating states
var NoOdr = (*OdrContext)(nil) // used for individual requests

This comment has been minimized.

@karalabe

karalabe Nov 4, 2015

Member

I don't like wrapping nils in various names. It makes the code much harder to read since you need to keep thinking (and look it up for the first time) what these mean. If it's a nil pointer, then simply use nil throughout the code and don't give it a fancy name.

This comment has been minimized.

@zsfelfoldi

zsfelfoldi Nov 4, 2015

Contributor

In the tutorials of the built-in go Context type it is explicitly advised that you should not use nil, exactly because of readability issues. And I agree with this idea, during development it was extremely useful to be able to search for places where I specified null contexts. Specifying "don't use ODR, fail instead when it's not in the DB" is a very explicit statement that should not be considered default and made invisible.
Btw OdrContext is not a descendant of Context because it was easier to implement my own context for my own purpose but the same logic applies.

This comment has been minimized.

@karalabe

karalabe Nov 5, 2015

Member

Please add a reference to back that up. Also as the context.Context was specifically designed for these cases, could you detail why it's better to roll your own against reusing a design from the standard libs?

https://blog.golang.org/context

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

Please rework all doc comments to the standard form:

// MyType bla bla bla
type MyType struct {}

// MyFunc bla bla bla
func MyFunc() {}

Emphasis on the fact that the doc string starts with the method/type name.

Also most of the code is missing documentation. Merging like this would introduce a gigantic technical debt that we won't be able to get rid of for a very long time (if not for other reason, simply because we won't find them). Please ensure all your methods, types and constants are properly documented.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

I very strongly oppose the NoSync modifications made to the downloader. If you don't want the downloader to run, then don't make one. Why would you 1) create it 2) start it 3) call it and after all these, have it do noops for every function, also bloating the entire code base along the way?

@@ -23,4 +23,5 @@ const (
FullSync SyncMode = iota // Synchronise the entire blockchain history from full blocks
FastSync // Quickly download the headers, full sync only at the chain head
LightSync // Download only the headers and terminate afterwards
NoSync // Dummy downloader, only one of the eth/les downloaders is actually working

This comment has been minimized.

@karalabe

karalabe Nov 4, 2015

Member

Please remove all notions of this. It's very very bad design. If you don't want the downloader to run, don't start it. This is just crippling the entire code base.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

The eth protocol cannot depend on the les protocol. Please move the les/access layer (everything related to ODR) into core/access. No file in eth whatsoever should import anything under the les path.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

I haven't read through the code in detail yet, just skimmed it, but is the OdrContext some short lived thing, or is it a long lifetime object. The reason I'm asking is because it's littering the APIs quite heavily and I'm wondering if it would be possible to somehow set a member field on the required objects (e.g. blockchain) and use that instead of passing it all over the place the whole time. Could you describe in short how the context works?

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

I disapprove of the API changes made in blockchain whereby LastBlockHash, LastBlockNumberU64, CurrentBlockHeader etc operate on headers instead of blocks in light mode. If you are in light mode and require to operate on headers then call the appropriate header accessors or create ones if they re missing, but do not modify the API semantics so that under different circumstances they do wildly different things. It just makes code close to impossible to understand since the same code all over the codebase will do completely different things based on some random CLI flag. That will be a nightmare to understand and debug should any issues arise. Please create and use strict APIs and honor their semantics.

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Nov 4, 2015

The lifespan of and OdrContext is one request. It handles timeout/cancel. So it needs to be passed down from the initiator that creates the context (usually the RPC) to the actual request. When accessing blocks, it is necessary to explicitly state if we want ODR or not in each case so I think hiding it would actually be a bad thing.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 4, 2015

Fair enough, if I have a better idea I'll shout out.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Nov 5, 2015

I've skimmed through the light node PR yesterday, added quite a few comments in there as I was going along, but thought that I'd like to summarize those comments about the general architecture, design, mechanism etc and provide a bit more thorough opinion about the matter and how I think we should proceed.

In one sentence, the PR is gigantic - not necessarily in size but rather in depth to as to what it touches and the impact it has on the entire code base - and I don't think we can properly review this as is and fully understand all the implications it would have on the entire system as a whole. I have noted quite a few design issues (none of which are blockers or cannot be solved with relative ease), but tracking these across all the files and modifications made by the PR is simply (IMHO) unwieldy.

My suggestion is that similar to how the fast sync mechanism was merged, we employ a similar merge tactic for the light client too. Namely, I'd like this PR broken up into preferably three individual PRs, each focusing on one particular aspect of the system:

  • The first PR should focus only on the new database access mechanism. It should contain all the modification that are required to core and to the data access layer, so that we can realistically assess them and sort out the side effects it might have on other parts of the code. The proposed PR introduces some quite significant API semantic changes in blockchain among others, that get lost in the sheer number of modifications made. I would really really like to look at these and debate these individually in the scope of the data access layer without needing to think or even see any of the light networking things. An additional design note here is that the proposed PR introduces a les/access package, that is used by core. Imho, that's a very much forbidden design and I would like all access layer stuff to be moved under core/access. That is it's correct conceptual place. Please open a PR that does only this much against develop: introduces the required access mechanism to support the light protocol, and simply modifies the current code base to use that. This way we can look through all the implications the added extra layer has on the existing code without needing to understand a lot of extra things.
  • The second PR should focus exclusively on the networking primitives required by LES. The subprotocol definition, packages, etc, so that we might see and test that the protocol spec itself is implemented correctly, without needing to constantly skip modifications made to core.
  • Finally, a third - probably one of the largest - PR that actually connects these concepts together, so that we may look at the algorithms and mechanisms again individually, without protocol and data access layer stuff cluttering the PR.

IMHO the above mentioned 3 PRs are conceptually very different, and require a completely different review approach, things to consider, things to watch out for, etc. Reviewing them in one big lump is prone to introduce bugs that we'll only find out very late on. Also limiting the scope would allow PRs to be evaluated and merged much faster, iteratively into the code base, avoiding the need to constantly rebase against the latest develop as other changes are merged in.

The last design issue I would like sorted out in a completely different way is the very messy cross dependency between the entire system and the proposed les and les/access packages. The PR proposal places these packages below core and eth, both referencing things from within les. IMHO, les is an extension and none of the base packages should even know it exists, let alone heavily depend on it.

My suggestion hence is twofold: the access layer should be moved into core as speced above, and I propose modifying the eth package so that all functionality generally used by both the main concensus mechanism and the light one are kept (package renamed to ethereum), and the eth / les specific stuff is to be moved into each their own subpackages that should know nothing about each other (or les at best be aware of eth). If you agree with this design, I'd gladly help out and make the changes of splitting the eth package while @zsfelfoldi is opening up the core PR (and we review/merge it).

@zsfelfoldi zsfelfoldi closed this Dec 1, 2015

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

@obscuren obscuren modified the milestone: 1.4.0 Apr 13, 2016

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