solana-labs / solana

Web-Scale Blockchain for fast, secure, scalable, decentralized apps and marketplaces.
https://solanalabs.com
Apache License 2.0
13.34k stars 4.35k forks source link

Validator could skip initial clean/shrink if starting from a locally-produced snapshot #23452

Closed sakridge closed 2 years ago

sakridge commented 2 years ago

Problem

Validator startup could be faster

Timings from @HaoranYi :

stage    master      jwash
untar    192.5       189.5       30%
rebuild  83.2        78.6        13%
clean    32.879021   33.763238   5%
shrink   290.741452  296.98629   45%
hash     16.629812   14.531542   3%
ledger   23.910978   21.945671   4%
total    639.861263  635.326741

Proposed Solution

Maybe we can skip the first accounts clean/shrink on startup if the snapshot is locally created since we clean before creating it so it should be relatively compact already and subsequent clean/shrink in the ledger replay + accounts background should compact it further. If we use an external snapshot, then still clean/shrink.

One method could be create an empty file in the ledger directory after we create the snapshot that indicates it's a local one.

sakridge commented 2 years ago

@ryoqun @carllin @brooksprumo Does it sound right?

brooksprumo commented 2 years ago

I think this makes sense at least to try, right? Two questions:

sakridge commented 2 years ago
  • What difference does creating locally or not make w.r.t. shrinking during startup? Shouldn't two nodes produce the same snapshots for a given slot (minimally at least the same reconstructed bank/accounts db/index)?

  • Why is clean/shrink in the startup path currently? Was this before accounts background service was added? Or was it that this startup path didn't used to take so much time when snapshots were significantly smaller?

Locally or not is security related so that someone doesn't hand you a snapshot with a ton of extra accounts in it and ends up starting the validator with a lot more disk usage than necessary. You can't trust an externally downloaded snapshot as you can one you created yourself.

carllin commented 2 years ago

Assuming this is the clean + shrink we're talking about in verify_snapshot_bank()? https://github.com/solana-labs/solana/blob/97d40ba3da7f70fb6f0f6e905f718c13820c09a4/runtime/src/bank.rs#L5961-L5973

Looks like this would be a simple change since there's already a accounts_db_skip_shrink option passed in.

This should be ok I think, but just to be precise, skipping this shrink might inflate the immediate account state by about ~25%, because in AccountsBackgroundService when the snapshot was created, we only shrink when appendvec's have more than DEFAULT_ACCOUNTS_SHRINK_RATIO = .80 dead entries, whereas the shrink on startup does a force shrink on every storage entry.

I suspect though the difference though should be made up quickly once the validator starts running and hits steady state, it'll accumulate this bloat anyways.

HaoranYi commented 2 years ago

@carllin Exactly, that's where we will skip cleaning and shrinking. The above pull request implements this proposal.

HaoranYi commented 2 years ago

With this change, ledger-tool will no longer do shrinking and cleaning unless manually deleting the .local_snapshot file. I am thinking to add another command line argument to ledger-tool, --force-cleaning along with --skip-cleaning (renaming --skip-shrinking to skip-cleaning to be more general. Let me know what do you think.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any activity in past 7 days after it was closed. Please open a new issue for related bugs.