More conservative purge_zero_lamport_accounts purge logic #7157

sakridge · 2019-11-27T00:26:06Z

Problem

Purging zero lamport accounts where the parent account's appendvec is not removed is not compatible with snapshots. Since the index is not serialized in the snapshot, the old view of the account could be revived.

Summary of Changes

Don't purge zero lamport accounts unless all AppendVecs containing parent accounts can be also removed.

Fixes #7117

codecov · 2019-11-27T04:42:18Z

Codecov Report

Merging #7157 into master will decrease coverage by 6.4%.
The diff coverage is 84.6%.

@@           Coverage Diff            @@
##           master   #7157     +/-   ##
========================================
- Coverage    79.3%   72.8%   -6.5%     
========================================
  Files         230     230             
  Lines       45117   49329   +4212     
========================================
+ Hits        35781   35919    +138     
- Misses       9336   13410   +4074

runtime/src/accounts_index.rs

runtime/src/accounts_db.rs

ryoqun · 2019-11-27T06:08:27Z

runtime/src/accounts_db.rs

+            if let Some(slot_storage) = storage.0.get(&slot_id) {
+                if let Some(store) = slot_storage.get(&account_info.id) {
+                    if let Some(store_count) = store_counts.get_mut(&account_info.id) {
+                        *store_count -= 1;


nits: it might be pedantic to avoid overflows? Or it never happens? Hmm, hard situation, but I might be able to write a test case for it.

I don't think it should be able to happen, but if you can think of any more tests that would be great.

runtime/src/accounts_db.rs

ryoqun · 2019-11-27T06:49:53Z

runtime/src/bank.rs

@@ -1446,20 +1446,6 @@ impl Bank {
        self.rc
            .accounts
            .verify_hash_internal_state(self.slot(), &self.ancestors)
-            && !self.has_accounts_with_zero_lamports()


(note to myself: lifting this restriction is needed because we're purging 0-lamport accounts conservatively, however is this really OK so as not to cause shielding attacks? Maybe I'm still not sure about it completely or missing something.)

Yea, the requirement is not necessarily that there are no 0-lamport accounts, just that they do not alter the accounts view such that it would give incorrect results when the accounts db has operations performed on it. Removing all 0-lamport accounts was just an easy way to verify that there are no incorrect shielding accounts, but it wasn't correct.

We do some checks when we do verify_hash_internal_state that looks at all account paths from the root, I think we have to convince ourselves that this is enough or that we maybe need to make extra checks.

I have an idea to add another path which is like an aggressive purge_zero_lamport_accounts path which removes them from the index and then rewrites the AppendVec stores to not contain those accounts at all so that the accounts store is consistent, but I think that can be in a follow-up PR if required.

Oh I see! Then, purging zero lamports account isn't set in stone as a hard requirement. That makes sense!

runtime/src/accounts_db.rs

ryoqun

Great work so far! So we've taken the conservative (or might be leaky) approach! I hope this generally works this time really. Finally my original concern over #7013 is vanishing.

This impl looks promising. However, still needs more rigorous thought for me to approve this. At the moment, first round of reviews are done! I need to cool down my head a bit...

Pull request has been modified.

ryoqun · 2019-12-02T03:05:49Z

runtime/src/accounts_db.rs

    pub fn purge_zero_lamport_accounts(&self, ancestors: &HashMap<u64, usize>) {
        self.report_store_stats();
+        let mut purges = HashMap::new();


Yeah, this is exactly what I thought would have liked to see! But not bothered to write a comment. :p This way, we can reduce redundant call of would_purge around here. :)

ryoqun · 2019-12-02T03:07:29Z

@sakridge Thanks for addressing all review comments! This looks almost complete! Only remaining is at my part: Come up with a creative testcase (#7157 (comment)) and run this locally.

Also, I think the quality of this PR is well beyond being mere a draft PR. :)

ryoqun

I've tested this locally and it seems this works as intended!

LGTM!

(cherry picked from commit 887bff5)

) automerge

sakridge force-pushed the conservative-purge branch from bd56b48 to 40efd00 Compare November 27, 2019 00:33

sakridge requested a review from ryoqun November 27, 2019 00:46

sakridge force-pushed the conservative-purge branch 3 times, most recently from 72b0b31 to d9bfa39 Compare November 27, 2019 04:10