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

Backport writing new fields to snapshot #26394

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Jul 4, 2022

Problem

#23844 and #25364 added new fields to be serialized in snapshots. Partial backports to v1.10 were added to allow a v1.10 client to consume snapshots produced by newer clients that contain these fields.

However, we also need the lamports_per_signature value to be stored in snapshots. If this value isn't stored, we could run into issues when restarting from a snapshot that was taken at a slot where the lamports_per_signature value was non-default. If the value isn't stored, we're forced to use the default on restart. If we use the default instead of what the proper value should have been, we deviate from consensus immediately. See #26069 for more info about how we debugged this.

Summary of Changes

Make v1.10 serialize new fields (default for accountsdb / proper value for lamports_per_signature) into snapshots.

Fixes #26069

Testing

To test the change, I did the following with the slots that were failing in GH 26069:

  • Create new full snapshot at a slot with non-default lamports_per_signature
  • Run solana-ledger-tool verify from the new snapshot
  • Observed that correct bank hash was calculated at slot that was deviating from consensus without this change

@steviez steviez requested a review from AshwinSekar July 4, 2022 23:34
@steviez
Copy link
Contributor Author

steviez commented Jul 4, 2022

@AshwinSekar - Is there any reason we can't include the field in newly serialized snapshots in v1.10? I saw your backport excluded it (#25805), so wondering if there is a scenario I'm not fully considering here. I'll fix CI if I get confirmation that there is no reason not to backport this

@steviez steviez marked this pull request as ready for review July 4, 2022 23:36
@AshwinSekar
Copy link
Contributor

The reason this wasn't backported was because of this previous work #24074 that @jeffwashington backported. If we don't backport the serialization for that field then we can't serialize this one because it will get deserialized as part of the accounts db field.

@steviez steviez changed the title Backport write portion of serialized lamports_per_signature Backport writing new fields to snapshot Jul 6, 2022
@steviez steviez force-pushed the serialize_lamports branch from 60929b6 to 6099698 Compare July 6, 2022 19:32
Serializing and storing lamports_per_signature field is critical to
avoid issues when a snapshot is taken at a slot with a non-default
lamports_per_signature value. In order to keep compatibility between
v1.10 and newer snapshots, we also need to serialize default values for
several AccountsDB fields.
@steviez steviez force-pushed the serialize_lamports branch from 6099698 to 043c108 Compare July 6, 2022 20:06
@steviez steviez requested a review from jeffwashington July 7, 2022 12:20
Copy link
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@steviez steviez merged commit 2b1e4ff into solana-labs:v1.10 Jul 7, 2022
@steviez steviez deleted the serialize_lamports branch July 7, 2022 19:04
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.

v1.10: 'solana-replay-stage' panicked at 'slot=XXX must exist in ProgressMap', core/src/progress_map.rs:576:32
3 participants