Fixup accounts count when importing snapshot append vecs by sakridge · Pull Request #7010 · solana-labs/solana

sakridge · 2019-11-18T02:48:35Z

Problem

count_and_status is not accurate from the serialized snapshot, because the snapshot creator could have removed some accounts from it's append vec view and thus decremented the count.

Summary of Changes

Update count_and_status by reading in the append_vec and correcting the accounts count.

Fixes #

codecov · 2019-11-18T03:13:51Z

Codecov Report

Merging #7010 into master will decrease coverage by 4.9%.
The diff coverage is 60.6%.

@@           Coverage Diff            @@
##           master   #7010     +/-   ##
========================================
- Coverage    79.1%   74.2%     -5%     
========================================
  Files         230     229      -1     
  Lines       44285   47197   +2912     
========================================
- Hits        35051   35029     -22     
- Misses       9234   12168   +2934

ryoqun · 2019-11-18T03:20:21Z

@sakridge Thanks for this PR! FYI, I'm currently working on this as part of #6890. Am I correct to regard this as a temporary fix until my upcoming fix to the root cause? Or have you started to work on this? I'm just asking this to avoid any overlapping work. :)

sakridge · 2019-11-18T03:31:03Z

@ryoqun I didn't really start to look into the root cause. As you said, this is more of a workaround. I think we would still like to root cause the actual issue.

ryoqun · 2019-11-18T03:36:34Z

@sakridge Ok! Thanks for explaining! Also, I just noticed the mention of this PR in the #network-protocols channel of our Discord server.

sakridge · 2019-11-18T20:01:42Z

This may not help actually.

ryoqun · 2019-11-22T23:34:28Z

            .verify_hash_internal_state(self.slot(), &self.ancestors)
-            && !self.has_accounts_with_zero_lamports()
+        {
+            info!(


+1 for more detailed message!

Eventually verify_snapshot_bank causes termination, so can warn! be more appropriate?

Well, I'm thinking if you are running a test which is designed to hit this, then it is not a problem and in the case that we panic, the error is already obvious from that, this just gives some extra detail.

ryoqun

LGTM with nits. (commented concerns are clarified already; thanks for quick replies)

Pull request has been modified.

Snapshots do not load the original index, so they must purge zero lamport accounts again.

sakridge · 2019-11-23T00:54:59Z

        }
+
+        let mut counts = HashMap::new();
+        for slot_list in accounts_index.account_maps.values() {


@ryoqun I think this should be more accurate to capture the updated count, can you take a look?

Yeah, this looks good! This's kind of reverse of my implementation. Simper, nicer :)

ryoqun · 2019-11-23T01:08:54Z

        let accounts = AccountsDB::new_single();
        let mut pubkeys: Vec<Pubkey> = vec![];
+
+        // Create 100 accounts in slot 0


+1 +1 +1 Got really more readable! :)

ryoqun · 2019-11-23T01:42:56Z

The CI passed!

sakridge · 2019-11-23T02:26:20Z

The CI passed!

Yep. I merged. If you can look at your tests to see if there are any cases which are still not covered now, can you add them?

ryoqun · 2019-11-24T23:58:34Z

The CI passed!

Yep. I merged. If you can look at your tests to see if there are any cases which are still not covered now, can you add them?

I've tested this PR locally with tests from #7013 and found there is no such uncovered cases! It seems that this works really well! Thanks for the work!

ryoqun · 2019-12-11T07:47:38Z

+                        count,
+                        store.count_and_status.read().unwrap().0
+                    );
+                    store.count_and_status.write().unwrap().0 = *count;


This discards the original value (=deserialized from snapshot) of count of count_and_status by unconditionally overwriting it.

sakridge force-pushed the snapshot-zero-account-purge branch from 8f86207 to 56944fb Compare November 18, 2019 02:51

mvines added the v0.20 label Nov 18, 2019

sakridge added the work in progress This isn't quite right yet label Nov 18, 2019

mvines removed the v0.20 label Nov 20, 2019

ryoqun mentioned this pull request Nov 21, 2019

Use epoch as the gossip purge timeout for staked nodes. #7005

Merged

sakridge force-pushed the snapshot-zero-account-purge branch 2 times, most recently from c9782f7 to 14e6357 Compare November 22, 2019 21:46

sakridge removed the work in progress This isn't quite right yet label Nov 22, 2019

sakridge changed the title ~~Purge snapshot of 0 lamport accounts before snapshot verify~~ Fixup accounts count when importing snapshot append vecs Nov 22, 2019

sakridge mentioned this pull request Nov 22, 2019

[wip] Correctly purge zero lamport accounts #7013

Closed

ryoqun reviewed Nov 22, 2019

View reviewed changes

ryoqun reviewed Nov 23, 2019

View reviewed changes

Comment thread runtime/src/accounts_db.rs Outdated

ryoqun reviewed Nov 23, 2019

View reviewed changes

Comment thread runtime/src/accounts_db.rs Outdated

ryoqun reviewed Nov 23, 2019

View reviewed changes

Comment thread runtime/src/accounts_db.rs Outdated

ryoqun reviewed Nov 23, 2019

View reviewed changes

Comment thread runtime/src/bank.rs

ryoqun previously approved these changes Nov 23, 2019

View reviewed changes

sakridge force-pushed the snapshot-zero-account-purge branch from 14e6357 to e41ec15 Compare November 23, 2019 00:40

sakridge force-pushed the snapshot-zero-account-purge branch 2 times, most recently from 78fd99b to 22c7e61 Compare November 23, 2019 00:47

Purge zero lamport accounts on snapshot ingestion

942c81e

Snapshots do not load the original index, so they must purge zero lamport accounts again.

sakridge force-pushed the snapshot-zero-account-purge branch from 22c7e61 to 942c81e Compare November 23, 2019 00:53

sakridge commented Nov 23, 2019

View reviewed changes

ryoqun reviewed Nov 23, 2019

View reviewed changes

sakridge merged commit 6c89226 into solana-labs:master Nov 23, 2019

sakridge deleted the snapshot-zero-account-purge branch November 23, 2019 02:22

ryoqun mentioned this pull request Nov 24, 2019

Validator creates bad snapshot when there is unfunded vote account #6890

Closed

ryoqun reviewed Dec 11, 2019

View reviewed changes

Conversation

sakridge commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov Bot commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ryoqun commented Nov 18, 2019

Uh oh!

sakridge commented Nov 18, 2019

Uh oh!

ryoqun commented Nov 18, 2019

Uh oh!

sakridge commented Nov 18, 2019

Uh oh!

ryoqun Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

sakridge Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun left a comment

Choose a reason for hiding this comment

Uh oh!

sakridge Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Nov 23, 2019

Uh oh!

sakridge commented Nov 23, 2019

Uh oh!

ryoqun commented Nov 24, 2019

Uh oh!

ryoqun Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sakridge commented Nov 18, 2019 •

edited

Loading

codecov Bot commented Nov 18, 2019 •

edited

Loading