cmd,eth: 16400 Add an option to stop geth once in sync. WIP for light mode

[lhendre]

No description provided.

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 71bc75589dc8b08eeeb9ec0a6881519af6fa5064
• Commit subjects should be kept under 100 characters

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 139e226013d24bd7722068a5b5a8716defbf14bd
• Commit subjects should be kept under 100 characters

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[GitCop]

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

• Commit: 208d8654fa4dd49c1b7121ca63d5159014c947b7
• Commit subjects should be kept under 100 characters

Please check the contribution guidelines for more details.

---

This message was auto-generated by https://gitcop.com

[lhendre]

Hi, I am working on the light mode implementation right now. Do you have any feedback on this or why it is failing the two integration tests, I am having trouble running the integration tests and am wondering if this could be a timing out error

[lhendre]

This does appear to work for me for the full node. I am now working on the light node implementation

[lhendre]

Hi @karalabe i have added in support for light mode how can I receive feedback on this?

[lhendre]

Hi @zsfelfoldi light node is working on my end and I resolved the merge conflict, any feedback on why it's not passing the tests?

[holiman]

So regarding the tests:

cmd/geth/main.go:1::warning: file is not gofmted with -s (gofmt)
eth/downloader/downloader.go:1::warning: file is not gofmted with -s (gofmt)
les/fetcher.go:1::warning: file is not gofmted with -s (gofmt)
cmd/utils/flags.go:658:4:warning: unused variable or constant expected ';', found ']' (varcheck)
cmd/geth/main.go:1::warning: file is not goimported (goimports)
eth/downloader/downloader.go:1::warning: file is not goimported (goimports)
les/fetcher.go:1::warning: file is not goimported (goimports)
util.go:45: exit status 1
exit status 1

That means you have not used go fmt or goimports properly. goimports structures the imports alphabetically and places an empty line between golang imports and project-imports.

[holiman]

I'm not quite sure about what happens in this PR, if the sync method is neither fast nor light (i.e, archive). Also, why did you need to define a new flag for light-sync?

[lhendre]

@holiman I think some of the issues were caused by merging with several recent commits. I am working on fixing those. If the node is not fast/light it will stop geth regardless when it is caught up. Should I make it so that it only does that for full?

[lhendre]

@holiman I fixed the issues and I can confirm that for other modes, including archive mode, it also shuts down when synced. Let me know if I should change that, I can set it up so it does that for only full mode and does nothing else for others.

[lhendre]

@holiman @karalabe I have posted the question to issue 16400 and have not gotten clarification for about a week, as such I am taking the issue/feature to ask for a full stop regardless of mode should the flag be raised, which makes sense for the flag and it currently accomplished. Additionally, its cleaned and fits the standards/gofmt and is mergeable. Please let me know on any feedback there is for this

[lhendre]

Hi, Touching base again @holiman @karalabe @zsfelfoldi to see if there are any suggestions or updates

[lhendre]

Hi @karalabe and @rjl493456442 , I have pushed code for the direction I am going based on the last requests. I am not sure if this is the appropriate way to do this and am still waiting for feedback on if we should switch from the downloader event to a timestamp in the header, if we do the switch what the threshold should be, and feedback on what should be done regarding the time delay. Currently, I am looking at grabbing the time stamp in the downloader processFastSyncContent with a new event that triggers the shutdown. I am not 100% familiar with this portion of the downloader and this appears to work for full/fast mode and I am investigating light mode. Additionally, this will require a check for roughly every header. Please let me know your thoughts, I have pushed this early to get feedback.

[lhendre]

Any comments @karalabe or @rjl493456442? Also @adamschmideg the travis build still appears to be failing due to timeout errors unrelated to this code

[lhendre]

Not finished yet, trying a new method based on conversations with @rjl493456442 trying to take in @karalabe request

[lhendre]

I have added changes and am testing it based on earlier feedback

[lhendre]

Based on @rjl493456442 I have passed the latest header through the done event so that I can check and validate, based on time, if this downloader done event is the correct event to resolve @karalabe conversations/suggestions. I have also turned the flag into a boolean as @karalabe suggested. I am wrapping up testing now

[lhendre]

Any thoughts?

[holiman]

I think the current implementation is pretty clean. My approval stands. @karalabe or @rjl493456442 or @fjl please review after @lhendre fixes the travis errors:

cmd/geth/main.go:1::warning: file is not goimported (goimports)
eth/downloader/events.go:1::warning: file is not goimported (goimports)
cmd/geth/main.go:1::warning: file is not gofmted with -s (gofmt)
eth/downloader/events.go:1::warning: file is not gofmted with -s (gofmt)
cmd/utils/flags.go:167:3:warning: unused variable or constant unknown field Value in struct literal (varcheck)
cmd/geth/main.go:338:92:warning: unused variable or constant cannot convert true (untyped bool constant) to int64 (varcheck)

and from https://travis-ci.org/ethereum/go-ethereum/jobs/479247288 :

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x44886fc]
goroutine 10416 [running]:
github.com/ethereum/go-ethereum/eth/downloader.(*Downloader).syncWithPeer.func1(0xc08addfd30, 0xc00009c820)
	/Users/travis/gopath/src/github.com/ethereum/go-ethereum/eth/downloader/downloader.go:418 +0xac
github.com/ethereum/go-ethereum/eth/downloader.(*Downloader).syncWithPeer(0xc00009c820, 0xc0809bd590, 0x1747b5c96b0d8f5b, 0x617aa7829f85be74, 0x370c3e58c87d726b, 0xb7c4a10ddd30d343, 0xc08ad089c0, 0x0, 0x0)
	/Users/travis/gopath/src/github.com/ethereum/go-ethereum/eth/downloader/downloader.go:482 +0x64e

I'm not sure how that happens, it seems d.mux is nil there for some reason.

[rjl493456442]

A few feedbacks, check out here.
And also could please test the functionality by syncing a testnet with exitwhensynced enabled to ensure the code works?

[lhendre]

fixed the sug gested changes, testing on Rinkeby and looking into travis report

[lhendre]

Still testing on Rinkeby, but the travis report looks more promising after implementing the feedback

[rjl493456442]

@lhendre The travis errors are not relative with this PR, but please take a look at lint suggestion and fix it.
I don't know what happen since I didn't click the approve button(But it seems I approved) = =. But this PR now looks good to me. Please ensure the functionality works and fix the lint.

And a few nitpicks:
Please update the exitwhensynced flag description :)

[lhendre]

Linting issues fixed, wrapping up rinkeby testing

[lhendre]

@holiman @rjl493456442 @karalabe The issues where fixed. I have successfully tested this on Rinkeby and it is working properly

[holiman]

Thanks for all the work you've put into this, @lhendre , it's sometimes very difficult to get PRs merged, even for us

[lhendre]

No Problem, Glad to help and thanks for the support!