Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Make AccountsHashVerifier make aware of Incremental Snapshots #19167

Closed
Tracked by #18828
brooksprumo opened this issue Aug 11, 2021 · 1 comment · Fixed by #19401
Closed
Tracked by #18828

Make AccountsHashVerifier make aware of Incremental Snapshots #19167

brooksprumo opened this issue Aug 11, 2021 · 1 comment · Fixed by #19401
Assignees

Comments

@brooksprumo
Copy link
Contributor

brooksprumo commented Aug 11, 2021

AccountsHashVerifier needs to know if it should generate a full snapshot package or an incremental snapshot package when sending over to SnapshotPackagerService.

@brooksprumo brooksprumo changed the title AccountsHashVerifier Make AccountsHashVerifier make aware of Incremental Snapshots Aug 11, 2021
@brooksprumo brooksprumo self-assigned this Aug 11, 2021
brooksprumo added a commit to brooksprumo/solana that referenced this issue Aug 17, 2021
Fixes solana-labs#19167

mvp: AccountsHashVerifier now takes incremental snapshots!

ahv: simplify test

add TMP_BANK_SNAPSHOT_PREFIX

wip: AccountsPackage shouldn't know anything about full vs incremental snapshots, so move than in SnapshotPackage instead (i.e. filtering)

wip: mark TODOs

removed SnapshotPackage::new()

fixup: use statements

removed snapshot_utils::process_accounts_pacakge(), in the middle of removing snapshot_utils::verify_accounts_pacakge_hash()

refactor package_and_archive_snapshot functions

fixup! rename to thread_pool

cherry-pick ish the to_prefix() change

fixup! TMP_SNAPSHOT_ARCHIVE_PREFIX

remove unused fields from AccountsPackage
brooksprumo added a commit to brooksprumo/solana that referenced this issue Aug 17, 2021
Fixes solana-labs#19167

- Move some fields to SnapshotPackage and remove from AccountsPackage
- Move snapshot storages filtering into SnapshotPackage, since the
  AccountsPackage doesn't know
- SnapshotPackage knows abouts full snapshots vs incremental snapshots,
  and AccountsPackage no longer does
- Remove the "processing" of AccountsPackage, as (1) it only conditional
  verifies the hash, and (2), the verification is only done in
  AccountsHashVerifier
- Refactor AccountsHashVerifier
brooksprumo added a commit to brooksprumo/solana that referenced this issue Aug 17, 2021
Fixes solana-labs#19167

- Move some fields to SnapshotPackage and remove from AccountsPackage
- Move snapshot storages filtering into SnapshotPackage, since the
  AccountsPackage doesn't know
- SnapshotPackage knows abouts full snapshots vs incremental snapshots,
  and AccountsPackage no longer does
- Remove the "processing" of AccountsPackage, as (1) it only conditional
  verifies the hash, and (2), the verification is only done in
  AccountsHashVerifier
- Refactor AccountsHashVerifier
brooksprumo added a commit to brooksprumo/solana that referenced this issue Aug 17, 2021
Fixes solana-labs#19167

- Move some fields to SnapshotPackage and remove from AccountsPackage
- Move snapshot storages filtering into SnapshotPackage, since the
  AccountsPackage doesn't know
- SnapshotPackage knows abouts full snapshots vs incremental snapshots,
  and AccountsPackage no longer does
- Remove the "processing" of AccountsPackage, as (1) it only conditional
  verifies the hash, and (2), the verification is only done in
  AccountsHashVerifier
- Refactor AccountsHashVerifier
brooksprumo added a commit to brooksprumo/solana that referenced this issue Aug 18, 2021
Fixes solana-labs#19167

Additionally:

- Move some fields to SnapshotPackage and remove from AccountsPackage
- Move snapshot storages filtering into SnapshotPackage, since the
  AccountsPackage doesn't know
- SnapshotPackage knows about full snapshots vs incremental snapshots,
  and AccountsPackage no longer does
- Remove the "processing" of AccountsPackage, as (1) it only conditional
  verifies the hash, and (2), the verification is only done in
  AccountsHashVerifier
- Refactor AccountsHashVerifier
- Update snapshots background services test to now take/expect
  incremental snapshots
brooksprumo added a commit that referenced this issue Aug 31, 2021
AccountsBackgroundService now knows about incremental snapshots.  It is
now also in charge of deciding if an AccountsPackage is destined to be a
SnapshotPackage or not (or just used by AccountsHashVerifier).

!!! New behavior changes !!!

Taking snapshots (both bank and archive) **MUST** succeed.

This is required because of how the last full snapshot slot is
calculated, which is used by AccountsBackgroundService when calling
`clean_accounts()`.

File system calls are now unwrapped and will result in a crash. As Trent told me:

>Well I think if a snapshot fails due to some IO error, it's very likely that the operator is going to have to intervene before it works.  We should exit error in this case, otherwise the validator might happily spin for several more hours, never successfully writing a complete snapshot, before something else brings it down.  This would leave the validator's last local snapshot many more slots behind than it would be had we exited outright and potentially force the operator to abandon ledger continuity in favor of a quick catchup

Other errors will set the `exit` flag to `true`, and the node will gracefully shutdown.

Fixes #19167 
Fixes #19168
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
1 participant