AccountsIndex::get_cloned() *must* add entry to in-mem cache #35322
Conversation
|
@jeffwashington @HaoranYi I'm requesting your review here before CI finishes because I wanted to get your eyes/thoughts on the comments I added. If there are any requested changes to the text, it'd be nice to not have to wait for all of CI to complete first. |
| // We *must* add the index entry to the in-mem cache! | ||
| // If the index entry is only on-disk, returning a clone would allow the entry | ||
| // to be modified, but those modifications would be lost on drop! | ||
| self.get_and_then(pubkey, |entry| (true, entry.cloned())) |
There was a problem hiding this comment.
What if the caller is only reading the entry? I wonder if we should add a bool arg to the fn and let the caller decide whether the entry need to be added to in-mem cache?
There was a problem hiding this comment.
I have a follow-up PR in the works that'll enable this safely. Right now, the caller can use get_and_then() to clone and decide to add to the in-mem cache or not. If they do this wrong, it's still unsafe.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35322 +/- ##
=======================================
Coverage 81.7% 81.7%
=======================================
Files 834 834
Lines 224232 224232
=======================================
+ Hits 183351 183377 +26
+ Misses 40881 40855 -26 |
Problem
AccountsIndex::get_cloned()currently does not add the entry to the in-mem cache, but that could lead to lost modifications.If the entry is only on-disk, returning the cloned entry would allow the caller to modify the entry (i.e. make it 'dirty'). Since the entry is still not in-mem, when the entry gets dropped, those modifications would not get written back to disk, thus losing any modification.
Summary of Changes
Always add the index entry to the in-mem cache when using
get_cloned().