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

Serialize lamports per signature in snapshots#25181

Merged
AshwinSekar merged 2 commits intosolana-labs:masterfrom
AshwinSekar:lamps-per-sig
May 14, 2022
Merged

Serialize lamports per signature in snapshots#25181
AshwinSekar merged 2 commits intosolana-labs:masterfrom
AshwinSekar:lamps-per-sig

Conversation

@AshwinSekar
Copy link
Copy Markdown
Contributor

Problem

The FeeRateGovernor in a bank does not serialize its current lamports_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 this lamports_per_signature to be invalid.

Summary of Changes

Serialize and deserialize this field by accessing it from the FeeRateGovernor

Fixes #23545

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2022

Codecov Report

Merging #25181 (857e0ba) into master (69a0ff9) will increase coverage by 0.0%.
The diff coverage is 76.6%.

❗ Current head 857e0ba differs from pull request most recent head b60ac35. Consider uploading reports for the commit b60ac35 to get more accurate results

@@            Coverage Diff            @@
##           master   #25181     +/-   ##
=========================================
  Coverage    82.0%    82.1%             
=========================================
  Files         598      610     +12     
  Lines      165882   168250   +2368     
=========================================
+ Hits       136125   138143   +2018     
- Misses      29757    30107    +350     

@AshwinSekar AshwinSekar requested a review from brooksprumo May 13, 2022 16:42
@AshwinSekar AshwinSekar marked this pull request as ready for review May 13, 2022 16:42
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Looks good. One nit/comment below, and then also posted over in the main issue about the direction for this fix.

Comment thread sdk/program/src/fee_calculator.rs Outdated
Comment on lines +168 to +180
pub fn copy_with_new_lamports_per_signature(
old: FeeRateGovernor,
lamports_per_signature: u64,
) -> FeeRateGovernor {
FeeRateGovernor {
lamports_per_signature,
target_lamports_per_signature: old.target_lamports_per_signature,
target_signatures_per_slot: old.target_signatures_per_slot,
min_lamports_per_signature: old.min_lamports_per_signature,
max_lamports_per_signature: old.max_lamports_per_signature,
burn_percent: old.burn_percent,
}
}
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo May 13, 2022

Choose a reason for hiding this comment

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

Minor nits/suggestions here:

  • How about building off of a self reference instead of consuming the old governor?
  • Should be able to use the .. syntax to construct the new governor
  • Rename the fn to use clone instead of copy (since the type itself doesn't implement Copy)
Suggested change
pub fn copy_with_new_lamports_per_signature(
old: FeeRateGovernor,
lamports_per_signature: u64,
) -> FeeRateGovernor {
FeeRateGovernor {
lamports_per_signature,
target_lamports_per_signature: old.target_lamports_per_signature,
target_signatures_per_slot: old.target_signatures_per_slot,
min_lamports_per_signature: old.min_lamports_per_signature,
max_lamports_per_signature: old.max_lamports_per_signature,
burn_percent: old.burn_percent,
}
}
pub fn clone_with_lamports_per_signature(
&self,
lamports_per_signature: u64,
) -> Self {
Self {
lamports_per_signature,
..self
}
}

(Note: I haven't tried compiling this to ensure it works heh 😅)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Spot on 😄 , just had to dereference that last self

@AshwinSekar AshwinSekar merged commit 35d2a0f into solana-labs:master May 14, 2022
mergify Bot pushed a commit that referenced this pull request May 14, 2022
* Serialize lamports per signature

* pr comments

(cherry picked from commit 35d2a0f)

# Conflicts:
#	runtime/src/serde_snapshot/newer.rs
#	runtime/src/serde_snapshot/tests.rs
jstarry added a commit that referenced this pull request May 15, 2022
mergify Bot pushed a commit that referenced this pull request May 15, 2022
@brooksprumo
Copy link
Copy Markdown
Contributor

@AshwinSekar It'll probably be valuable to add tests to ensure compatibility between snapshots with and without the new lamports_per_signature field.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bank hash mismatch when a validator is offline for more than 512 slots (Slot hashes max entries)

2 participants