Feature/private auto sync

[zilm13]

Possible alternative for #1093
Some thoughts:
we couldn't have block hash + total difficulty in validateAndAddNewBlock as there could be gaps etc, so not much sense to increase code difficulty (not a good thing to compare only block number) for this custom hack

[coveralls]

Coverage decreased (-0.1%) to 55.906% when pulling 2de81aa on feature/private-auto-sync into 6162302 on develop.

[mkalinin]

Do we need this update? During our latest discussion we decided to count that time from sync start up, not from last imported block

[pschweitz]

Hi,

We said, we make sync done when the last known block arrived, instead of waiting for a new block.
When I was reading some other comments, I am afraid it could have some gaps in the chain. That is something we must also prevent before raising the make sync done event.

The timeout applied in the header retriever loop to make sync done if no peer was responding after this time only if the know best block was not updated since Ethereum start (standalone node or in a private network but disconnected from other nodes)

[zilm13]

@pschweitz I think we could remove updating timer on every block, but we couldn't remove it with first peer, because it's pretty common to have private networks with more than 2 peers. And those peers could be on different height. So it could go buggy in that cases.

[mkalinin]

@pschweitz isn't it similar behavior? in your case it's delayed to wait for new peer, here it's delayed for network exploring with no regard on what's happening with peers.

[pschweitz]

@zilm13
I understand it should have more that one second peer in a private network, but cannot the local peer identify according to the internal peer list it maintains (or with "peer.active" list), that everything is synchronized, regardless the potential number of nodes in the network ?
The logic should not be the same whatever the number of peers in the network (2 up to N) ?

Once synchronized, it isn't the job of the header retriever loop to fill potential missing blocks afterwards , up to the real final best block ? (the loop you didn't want me to break it I understand well)

Otherwise if we extrapolate was you explained, we never could ensure having synchronized everything properly, whatever the number of nodes involved in the network, because it could happen that we reach a networking status (Murphy's Law), where the nodes are disconnected between themselves after all (as you have a probabilist approach).

Then shouldn't be one job of the Ethereum protocol to recognize the legitimate blockchain at re-connection ?

@mkalinin
Yes it is OK like that as well (I disabled network discovery in my case for the moment)

Thanks to both of you for your support

[zilm13]

@pschweitz checking node in "active peers" list doesn't mean another node couldn't connect to you, moreover it's the way people sync in private networks too. Plus you could have outdated list with, say, 2 peers live and 3rd - not used today.
About some prove of HEAD reach on protocol level - I think the existence of such thing is not possible due to decentralization etc. I think it's not even possible with Casper, channel payments (with penalization of course, if node provided wrong info) etc.

[mkalinin]

I am up to merge. guys?

[pschweitz]

Yes, you can merge when you feel confident about the content.

I will test again and give you a feedback on Monday.

[pschweitz]

I confirm everything is working like expected.

Thanks a lot for your work.

I would just add one last suggestion.

A big part of this work is also for letting developers working faster onto a standalone node. And around this new timeout, there was a timer scheduled every 10 seconds.
However if we want this triggered asap like 1 sec, we would wait for 10 seconds anyway which reduces its usability, and enforces to wait for such 10 seconds after each compilation...

I would suggest to reduce this scheduling to 1 second at line 189 of the SyncManager.java. It will not have so much impact during the application runtime.

Otherwise I am sure this changes will increase adoption of this Java/blockchain solution by the developers thanks to more simplicity at the beginning !

[zilm13]

@pschweitz you could set in config now or pass in command line, use sync.makeDoneByTimeout, set it to any value you want.

[pschweitz]

Thanks @zilm13
I have understood that part, and it is working as described.

Sorry if I was not clear in my last post, I just was telling that even if you set 1 sec of timeout, you will wait for at least 10 seconds, because of the timer that performs the check that is run every 10 sec only...

line 189 of the SyncManager.java

[zilm13]

@pschweitz ahhhh I got you. I think we could decrease it to 1/2 seconds

[pschweitz]

yes we can

[zilm13]

@pschweitz done a2f5061 , but in develop only, not urgent I think