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

Purge accounts with lamports=0 on rooted forks #6315

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

sakridge
Copy link
Member

@sakridge sakridge commented Oct 10, 2019

Problem

Accounts with 0 lamports could never be purged from accounts_db, thus causing potential for resource exhaustion attacks.

Summary of Changes

Add a function to purge accounts with 0 lamports on rooted forks.
TODO:

  • call this from an asynchronous cleanup thread or somewhere else?
  • Figure out how this works with hash_internal_state

Fixes #6299

@sakridge
Copy link
Member Author

@rob-solana this is WIP, but I think this is what you meant?

@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #6315 into master will increase coverage by 0.1%.
The diff coverage is 92.3%.

@@           Coverage Diff            @@
##           master   #6315     +/-   ##
========================================
+ Coverage    78.7%   78.8%   +0.1%     
========================================
  Files         217     217             
  Lines       41184   41276     +92     
========================================
+ Hits        32424   32564    +140     
+ Misses       8760    8712     -48

rob-solana
rob-solana previously approved these changes Oct 10, 2019
Copy link
Contributor

@rob-solana rob-solana left a comment

Choose a reason for hiding this comment

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

this is exactly what I meant.

Nice. now we just need to figure out where to call it. maybe for snapshots?

@sakridge
Copy link
Member Author

this is exactly what I meant.

Nice. now we just need to figure out where to call it. maybe for snapshots?

Yea that's not a bad idea, cleaning up makes sense before the snapshot and that's already async. I thought of that but thought they didn't really fit together, but as a cleanup to make the snapshot size smaller makes sense.

@sakridge sakridge force-pushed the cleanup-zero-lamport-accounts branch from 1b09cb7 to 4bd6db3 Compare October 10, 2019 23:13
@mergify mergify bot dismissed rob-solana’s stale review October 10, 2019 23:14

Pull request has been modified.

@sakridge sakridge force-pushed the cleanup-zero-lamport-accounts branch 4 times, most recently from d82b25c to 05cb78e Compare October 22, 2019 21:52
@sakridge sakridge marked this pull request as ready for review October 22, 2019 21:53
@sakridge sakridge requested a review from pgarg66 October 22, 2019 21:53
@sakridge sakridge force-pushed the cleanup-zero-lamport-accounts branch from 05cb78e to fc4c8fb Compare October 22, 2019 22:14
@sakridge sakridge force-pushed the cleanup-zero-lamport-accounts branch from fc4c8fb to 67f08c3 Compare October 22, 2019 22:18
@sakridge sakridge requested a review from rob-solana October 23, 2019 19:11
@@ -746,6 +766,10 @@ impl AccountsDB {
}

pub fn hash_account_data(fork: Fork, lamports: u64, data: &[u8], pubkey: &Pubkey) -> Hash {
if lamports == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

this one's interesting, how come?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because if we purge, we would not maintain the correct bank hash value.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah!

@sakridge sakridge merged commit f117261 into solana-labs:master Oct 23, 2019
@sakridge sakridge deleted the cleanup-zero-lamport-accounts branch October 23, 2019 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Purge rooted lamports==0 accounts in accountsdb
2 participants