core: flush out trie cache more meaningfully on stop #16143
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
veox
reviewed
Feb 20, 2018
| log.Info("Writing cached state to disk", "block", recent.Number(), "hash", recent.Hash(), "root", recent.Root()) | ||
| if err := triedb.Commit(recent.Root(), true); err != nil { | ||
| log.Error("Failed to commit recent state trie", "err", err) | ||
| for _, offset := range []uint64{0, 1, triesInMemory - 1} { |
There was a problem hiding this comment.
Random question: is triesInMemory guaranteed to never be 0? Or 1?..
There was a problem hiding this comment.
triesInMemory is a constant 128. It's not meant to be changed, since it needs to be in sync with the fasy sync algorithm.
holiman
approved these changes
Feb 23, 2018
|
I added -0 as it was confusing to a lot of people why the chain rolls back at all if nothing went wrong. |
prestonvanloon
pushed a commit
to prestonvanloon/go-ethereum
that referenced
this pull request
Apr 2, 2018
* core: flush out trie cache more meaningfully on stop * core: upgrade legacy tests to chain maker
|
I would also suggest to provide the possibility to periodically write out the HEAD-x trie, especially when running in PoA mode. In such a case it could happen that there is only one PoA geth instance for testing purposes. If this instance crashes the whole blockchain will be "lost", if the node was never gracefully shut down before (see #16131 and #16142): |
mariameda
pushed a commit
to NiluPlatform/go-nilu
that referenced
this pull request
Aug 23, 2018
* core: flush out trie cache more meaningfully on stop * core: upgrade legacy tests to chain maker
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current code writes out the HEAD-127 trie on graceful shutdown. The idea was:
Unfortunately this has the annoying drawback that starting the node always re-executes 128 blocks. Since people in general won't constantly start/stop/start/stop/etc, it's annoying to reprocess that many blocks. If your node was offline for more than 30 mins, you'll either way load enough tries into memory to help remote nodes sync.
This PR fixes the issue by writing out 3 tries instead of only one on stop:
HEAD: So we don't need to reprocess any blocks in the general case.HEAD-1: So we don't do large reorgs if our HEAD becomes an uncle.HEAD-127: So we have a hard limit on the number of blocks reexecuted during deep reorgs.The second commit in this PR upgrades all the legacy blockchain tests to use test chains generated by the chain maker. The old code used "invalid" chains with only some specific header fields set (others left unset) in combination with disabled block validation. This however was both murky even until now, as well as caused the trie cache to blow up due to invalid state roots. Instead of continuing to patch the "invalid" chains to somehow pass the tests, I just completely dumped them and replaces with valid ones generated via chain makers.