Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

cmd, core, eth, graphql, trie: no persisted clean trie cache file #27525

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

rjl493456442
Copy link
Member

The clean trie cache is persisted periodically, therefore Geth can
quickly warmup the cache in next restart.

However it will reduce the robustness of system. The assumption is
held in Geth that if the parent trie node is present, then the entire
sub-trie associated with the parent are all present.

Imagine the scenario that Geth rewinds itself to a past block and
restart, but Geth finds the root node of "future state" in clean
cache then regard this state is present in disk, while is not in fact.

Another example is offline pruning tool. Whenever an offline pruning
is performed, the clean cache file has to be removed to avoid hitting
the root node of "deleted states" in clean cache.

All in all, compare with the minor performance gain, system robustness
is something we care more.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

I think the code-change here is fine, but we should discuss whether we want to remove this feature or not.
I'm not sure what the performance-hit is (?)

@rjl493456442
Copy link
Member Author

Performance wise, we will have a cache warm time if we get rid of the clean cache.

The clean cache allowance is 614 mega bytes by default(mainnet). I think the cache
warm time is acceptable.

Compare with the performance consideration, correctness is something we care more.

E.g. https://github.com/ethereum/go-ethereum/blob/master/core/blockchain.go#L649
we may stop at the incomplete state in repair procedure if the root node is found in
clean cache.

@karalabe
Copy link
Member

I kind of agree that this is a wonky mechanism and would be nice to get rid of it, but it's a non-trivial thing we added so it seems there probably was a good reason to do it in the first place.

Could we benchmark the hit somehow? E.g. take 2 synced bench pairs, restart the machines (without starting Geth, to flush out any warm caches) and tehn start master vs. this PR and see what happens?

The clean trie cache is persisted periodically, therefore Geth can
quickly warmup the cache in next restart.

However it will reduce the robustness of system. The assumption is
held in Geth that if the parent trie node is present, then the entire
sub-trie associated with the parent are all prensent.

Imagine the scenario that Geth rewinds itself to a past block and
restart, but Geth finds the root node of "future state" in clean
cache then regard this state is present in disk, while is not in fact.

Another example is offline pruning tool. Whenever an offline pruning
is performed, the clean cache file has to be removed to aviod hitting
the root node of "deleted states" in clean cache.

All in all, compare with the minor performance gain, system robustness
is something we care more.
@rjl493456442
Copy link
Member Author

image

07 is this PR, without loading clean cache from file. The overall performance is not too different.

And besides, the hit rate in clean hit is not very high, so I don't think this change will cause a big performance impact, should be good to go.

image
image

@karalabe karalabe added this to the 1.12.1 milestone Jul 4, 2023
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

SGTM

@karalabe karalabe merged commit 59f7b28 into ethereum:master Jul 4, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…hereum#27525)

The clean trie cache is persisted periodically, therefore Geth can
quickly warmup the cache in next restart.

However it will reduce the robustness of system. The assumption is
held in Geth that if the parent trie node is present, then the entire
sub-trie associated with the parent are all prensent.

Imagine the scenario that Geth rewinds itself to a past block and
restart, but Geth finds the root node of "future state" in clean
cache then regard this state is present in disk, while is not in fact.

Another example is offline pruning tool. Whenever an offline pruning
is performed, the clean cache file has to be removed to aviod hitting
the root node of "deleted states" in clean cache.

All in all, compare with the minor performance gain, system robustness
is something we care more.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants