-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Serialize lamports per signature #25364
Conversation
cc9ea02
to
76cf6fe
Compare
76cf6fe
to
8e699a9
Compare
3aa6098
to
5baba2c
Compare
Codecov Report
@@ Coverage Diff @@
## master #25364 +/- ##
=========================================
Coverage 82.0% 82.0%
=========================================
Files 655 620 -35
Lines 171822 170582 -1240
Branches 335 0 -335
=========================================
- Hits 140972 140027 -945
+ Misses 30734 30555 -179
+ Partials 116 0 -116 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Sorry about the long review delay...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the previous replies; this PR looks good.
One last question is on the decision to put lamports_per_signature
as a new field in the returned tuple from serialize_bank_and_storage()
:
solana/runtime/src/serde_snapshot/newer.rs
Lines 208 to 211 in 5baba2c
// Additional fields, we manually store the lamps per signature here so that | |
// we can grab it on restart. | |
// TODO: if we do a snapshot version bump, consider moving this out. | |
lamports_per_signature, |
Instead, would it be possible to put lamports_per_signature
at the end of DeserializableVersionedBank
with the serde default_on_eof
?
#[serde(deserialize_with = "default_on_eof")]
lamports_per_signature: u64,
solana/runtime/src/serde_snapshot/newer.rs
Lines 29 to 63 in 5baba2c
struct DeserializableVersionedBank { | |
blockhash_queue: BlockhashQueue, | |
ancestors: AncestorsForSerialization, | |
hash: Hash, | |
parent_hash: Hash, | |
parent_slot: Slot, | |
hard_forks: HardForks, | |
transaction_count: u64, | |
tick_height: u64, | |
signature_count: u64, | |
capitalization: u64, | |
max_tick_height: u64, | |
hashes_per_tick: Option<u64>, | |
ticks_per_slot: u64, | |
ns_per_slot: u128, | |
genesis_creation_time: UnixTimestamp, | |
slots_per_year: f64, | |
accounts_data_len: u64, | |
slot: Slot, | |
epoch: Epoch, | |
block_height: u64, | |
collector_id: Pubkey, | |
collector_fees: u64, | |
fee_calculator: FeeCalculator, | |
fee_rate_governor: FeeRateGovernor, | |
collected_rent: u64, | |
rent_collector: RentCollector, | |
epoch_schedule: EpochSchedule, | |
inflation: Inflation, | |
stakes: Stakes<Delegation>, | |
#[allow(dead_code)] | |
unused_accounts: UnusedAccounts, | |
epoch_stakes: HashMap<Epoch, EpochStakes>, | |
is_delta: bool, | |
} |
That's how historical_roots
is handled for AccountsDbFields
:
solana/runtime/src/serde_snapshot.rs
Lines 67 to 78 in 5baba2c
struct AccountsDbFields<T>( | |
HashMap<Slot, Vec<T>>, | |
StoredMetaWriteVersion, | |
Slot, | |
BankHashInfo, | |
/// all slots that were roots within the last epoch | |
#[serde(deserialize_with = "default_on_eof")] | |
Vec<Slot>, | |
/// slots that were roots within the last epoch for which we care about the hash value | |
#[serde(deserialize_with = "default_on_eof")] | |
Vec<(Slot, Hash)>, | |
); |
That may make the impl simpler. Sorry if you already tried doing this but I missed it!
That was actually my initial approach see here (https://github.com/solana-labs/solana/pull/25181/files). Turns out it doesn't https://github.com/solana-labs/solana/blob/master/runtime/src/serde_snapshot/newer.rs#L199 Because of this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
* Serialize lamports per signature * Add full snapshot archive test, enable features in previous tests (cherry picked from commit 8caced6) # Conflicts: # runtime/src/serde_snapshot.rs # runtime/src/serde_snapshot/newer.rs # runtime/src/serde_snapshot/tests.rs
* Serialize lamports per signature * Add full snapshot archive test, enable features in previous tests (cherry picked from commit 8caced6) # Conflicts: # runtime/src/serde_snapshot.rs # runtime/src/serde_snapshot/newer.rs # runtime/src/serde_snapshot/tests.rs
…25805) This allows us to deserialize v1.11 snapshots Original: Serialize lamports per signature (#25364) * Serialize lamports per signature * Add full snapshot archive test, enable features in previous tests (cherry picked from commit 8caced6) Co-authored-by: Ashwin Sekar <[email protected]>
Problem
The
FeeRateGovernor
in a bank does not serialize its currentlamports_per_signature
on snapshot. This causes issues if the block that we start a snapshot from is lined up with a fee increase, it is possible for thislamports_per_signature
to be invalid.Summary of Changes
Serialize and deserialize this field by accessing it from the
FeeRateGovernor
To avoid the same issue as #25181, we manually serialize it as an extra field at the end of the snapshot. The issue with the previous solution was that we serialized it as part of the bank which caused problems as the accounts db fields were serialized after it.
Fixes #23545