-
Notifications
You must be signed in to change notification settings - Fork 781
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
Trie/Client: Add permanent Trie Node Cache + Client Integration #2667
Conversation
…w CheckpointDBopts type, fix tests
…ffects), test integration
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good over all. Left a couple of comments we should consider addressing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great pending CI passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds a permanent Node cache to the Trie checkpoint DB class which increases client block execution in particular and repetitive trie reads in general.
Cache design is extremely simple since trie node keys are unique, so there is no need for this back-and-forth related to checkpointing, since there is no risk to overwrite and old entry and therefore accidentally introduce a wrong key. I tested this with longer client sync ranges and there were no problems.
It is also "by design" that reverted node entries are kept in the cache, since this might help to mitigate repeated revert-attacks with the same attack setup, always re-reading the same accounts/slots e.g. and then revert.
I in-between planned to activate this LRU cache by default, since in most cases there should be no downsides. I then discovered several ad-hoc Trie instantiations in our own code base (mostly related to proofs), so I restrained since there might be side effects if regarding too frequent instantiations.
(Side question: do these Proof methods need any Trie object state or might it generally be an option (respectively occasion now during breaking period) to make these methods static?)
Another note: I first added an active cache to the Trie checkpointing tests and these were actually failing. I then had a closer look and realized that these were just creating "un-realistic" trie scenarios - by directly operating on the db and then adding a key and updating the same key with a different value. This scenario can - by Merkle Patricia Tree design - just not happen, since keys are always unique (in relation to the content/subtree they refer to). I already thought if these tests should generally be deleted, but refrained since they do test the general DB functionality nevertheless. All other tests (which I ran in between with the Trie-cache-active-by-default work state) are passing with the cache though.
Open for review and merge.