core/rawdb, eth, les,node: implemented update for unclean shutdown marker

[BitBaseBit]

Fixes #22580. As mentioned there, the unclean shutdown marker was using the initial boot time of the last know crash to report to the user that a crash had occurred. It would be more useful if the user know when the crash happened, instead of only knowing when the node was booted before it crashed. As discussed in #22580, the marker update time is set to 5 minutes. There is also a global in there that I know shouldn't be, my initial though was to put it in handler in the Ethereum struct, but I'm not sure that's appropriate.

[BitBaseBit]

Seems like the CI build fail is unrelated to this PR, I can't reproduce the fail locally.

[karalabe]

The PR is a good idea, but the problematic part is what you also mentioned in the code, that you have this dangling global channel to terminate the goroutine. We've discussed it a bit today that this mechanism might be better in the database layer, since "unclean shutdown" really means "was the db closed properly or not".

We think the best place we could add it would be core/rawdb/database.go (you might need to think it over a bit how best to add so both the "freezer" and "non freezer" dbs are covered). This way you can start the goroutine when the database is opened and terminate it when the database is closed, and instead of a dangling channel, you can keep it internal to the database fields.

[BitBaseBit]

The PR is a good idea, but the problematic part is what you also mentioned in the code, that you have this dangling global channel to terminate the goroutine. We've discussed it a bit today that this mechanism might be better in the database layer, since "unclean shutdown" really means "was the db closed properly or not".

We think the best place we could add it would be core/rawdb/database.go (you might need to think it over a bit how best to add so both the "freezer" and "non freezer" dbs are covered). This way you can start the goroutine when the database is opened and terminate it when the database is closed, and instead of a dangling channel, you can keep it internal to the database fields.

Thanks for the feedback! I will make the suggested changes.

[BitBaseBit]

The PR is a good idea, but the problematic part is what you also mentioned in the code, that you have this dangling global channel to terminate the goroutine. We've discussed it a bit today that this mechanism might be better in the database layer, since "unclean shutdown" really means "was the db closed properly or not".

We think the best place we could add it would be core/rawdb/database.go (you might need to think it over a bit how best to add so both the "freezer" and "non freezer" dbs are covered). This way you can start the goroutine when the database is opened and terminate it when the database is closed, and instead of a dangling channel, you can keep it internal to the database fields.

I have implemented what you suggested.

[holiman]

I pushed a commit to clean it up a bit -- using a switch instead of a sequence of if-clauses, and removing an unnecesary param. It's a little smaller now, easier to evaluate.
It's still not quite production ready though.

[jayboy-mabushi]

@BitBaseBit I am having this issue. How can I resolve it?

[holiman]

Huh, I made this comment a long++ time ago, but it got left in "Pending" -- I never actually submittted the review. Doh!

[holiman]

This is very un-intuitive IMO. Would be better to have the channel be nil if it's not needed, instead of having a separate boolean on-the-side to tell whether it's used or not.

[s1na]

This PR was superseded by #24077. Closing.