Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

les: fix private net issues, enable adding peers manually again #3607

Merged
merged 3 commits into from
Feb 1, 2017

Conversation

zsfelfoldi
Copy link
Contributor

This PR solves #3510

  • enables adding peers manually by adding new incoming connections to the server pool instead of rejecting everything not initiated by it
  • removes the mechanism which delayed starting the LES server side protocol until ETH was fully synced. This mechanism solved a problem that does not exist anymore, clients now check server's reported head before sending requests and choose a server that can actually serve it. At the same time it caused other problems, like not starting LES server at all when the node was mining alone on a private net. Also, not starting the protocol stalled connections to other LES servers through ETH, even though they did not want to do anything with each other through LES.
  • allows disabling topic discovery with --nodiscover option

@mention-bot
Copy link

@zsfelfoldi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @karalabe, @fjl and @obscuren to be potential reviewers.

@@ -663,7 +663,7 @@ func MakeNode(ctx *cli.Context, name, gitCommit string) *node.Node {
Version: vsn,
UserIdent: makeNodeUserIdent(ctx),
NoDiscovery: ctx.GlobalBool(NoDiscoverFlag.Name) || ctx.GlobalBool(LightModeFlag.Name),
DiscoveryV5: ctx.GlobalBool(DiscoveryV5Flag.Name) || ctx.GlobalBool(LightModeFlag.Name) || ctx.GlobalInt(LightServFlag.Name) > 0,
DiscoveryV5: ctx.GlobalBool(DiscoveryV5Flag.Name) || (ctx.GlobalBool(LightModeFlag.Name) || ctx.GlobalInt(LightServFlag.Name) > 0) && !ctx.GlobalBool(NoDiscoverFlag.Name),
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this hard to parse for a human brain. Is it possible to reconstruct this statement so it's easier to brain-validate e.g. what effect the NoDiscoverFlag has on the outcome?

Copy link
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, I shouldn't write expressions like this because it was not exactly what I intended to do :) it's now fixed, reconstructed and clarified.

@@ -173,7 +173,7 @@ func NewProtocolManager(config *params.ChainConfig, fastSync bool, networkId int
return blockchain.CurrentBlock().NumberU64()
}
inserter := func(blocks types.Blocks) (int, error) {
manager.setSynced() // Mark initial sync done on any fetcher import
atomic.StoreUint32(&manager.synced, 1) // Mark initial sync done on any fetcher import
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't hurt to use atomic here, but I'm curious... Previously, you actually did something if synched went from 0 to 1, and thus wanted it to be threadsafe/atomic. Now it seems you could just aswell use non-atomic, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, synced is being read and written in different goroutines, see:
https://github.com/ethereum/go-ethereum/blob/master/eth/handler.go#L663
This is how it worked before I added setSynced (I restored it in its original state).

@zsfelfoldi zsfelfoldi added this to the 1.5.8 milestone Jan 27, 2017
@fjl fjl merged commit 9c45b44 into ethereum:master Feb 1, 2017
@fjl fjl removed the review label Feb 1, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants