-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Fix fee mismatch on snapshot deserialize #12697
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -443,6 +443,7 @@ pub(crate) struct BankFieldsToDeserialize { | |
| // This is separated from BankFieldsToDeserialize to avoid cloning by using refs. | ||
| // So, sync fields with BankFieldsToDeserialize! | ||
| // all members are made public to remain Bank private and to make versioned serializer workable on this | ||
| #[derive(Debug)] | ||
| pub(crate) struct BankFieldsToSerialize<'a> { | ||
| pub(crate) blockhash_queue: &'a RwLock<BlockhashQueue>, | ||
| pub(crate) ancestors: &'a Ancestors, | ||
|
|
@@ -477,6 +478,46 @@ pub(crate) struct BankFieldsToSerialize<'a> { | |
| pub(crate) is_delta: bool, | ||
| } | ||
|
|
||
| // Can't derive PartialEq because RwLock doesn't implement PartialEq | ||
| impl PartialEq for Bank { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| if ptr::eq(self, other) { | ||
| return true; | ||
| } | ||
| *self.blockhash_queue.read().unwrap() == *other.blockhash_queue.read().unwrap() | ||
| && self.ancestors == other.ancestors | ||
| && *self.hash.read().unwrap() == *other.hash.read().unwrap() | ||
| && self.parent_hash == other.parent_hash | ||
| && self.parent_slot == other.parent_slot | ||
| && *self.hard_forks.read().unwrap() == *other.hard_forks.read().unwrap() | ||
| && self.transaction_count.load(Relaxed) == other.transaction_count.load(Relaxed) | ||
| && self.tick_height.load(Relaxed) == other.tick_height.load(Relaxed) | ||
| && self.signature_count.load(Relaxed) == other.signature_count.load(Relaxed) | ||
| && self.capitalization.load(Relaxed) == other.capitalization.load(Relaxed) | ||
| && self.max_tick_height == other.max_tick_height | ||
| && self.hashes_per_tick == other.hashes_per_tick | ||
| && self.ticks_per_slot == other.ticks_per_slot | ||
| && self.ns_per_slot == other.ns_per_slot | ||
| && self.genesis_creation_time == other.genesis_creation_time | ||
| && self.slots_per_year == other.slots_per_year | ||
| && self.unused == other.unused | ||
| && self.slot == other.slot | ||
| && self.epoch == other.epoch | ||
| && self.block_height == other.block_height | ||
| && self.collector_id == other.collector_id | ||
| && self.collector_fees.load(Relaxed) == other.collector_fees.load(Relaxed) | ||
| && self.fee_calculator == other.fee_calculator | ||
| && self.fee_rate_governor == other.fee_rate_governor | ||
| && self.collected_rent.load(Relaxed) == other.collected_rent.load(Relaxed) | ||
| && self.rent_collector == other.rent_collector | ||
| && self.epoch_schedule == other.epoch_schedule | ||
| && *self.inflation.read().unwrap() == *other.inflation.read().unwrap() | ||
| && *self.stakes.read().unwrap() == *other.stakes.read().unwrap() | ||
| && self.epoch_stakes == other.epoch_stakes | ||
| && self.is_delta.load(Relaxed) == other.is_delta.load(Relaxed) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder how we can maintain this for the addition of newer fields... This problem exists from |
||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, PartialEq, Serialize, Deserialize, AbiExample, Default, Clone, Copy)] | ||
| pub struct RewardInfo { | ||
| pub lamports: i64, // Reward amount | ||
|
|
@@ -878,7 +919,11 @@ impl Bank { | |
| ); | ||
| assert_eq!(bank.epoch_schedule, genesis_config.epoch_schedule); | ||
| assert_eq!(bank.epoch, bank.epoch_schedule.get_epoch(bank.slot)); | ||
|
|
||
| bank.fee_rate_governor.lamports_per_signature = bank.fee_calculator.lamports_per_signature; | ||
| assert_eq!( | ||
| bank.fee_rate_governor.create_fee_calculator(), | ||
| bank.fee_calculator | ||
| ); | ||
| bank | ||
| } | ||
|
|
||
|
|
@@ -3590,51 +3635,6 @@ impl Bank { | |
| } | ||
| } | ||
|
|
||
| pub fn compare_bank(&self, dbank: &Bank) { | ||
| if ptr::eq(self, dbank) { | ||
| return; | ||
| } | ||
|
Comment on lines
-3594
to
-3596
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this should be kept to deadlock free? #10466 Yeah, we should have added an comment...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yeah good catch, thanks! |
||
| assert_eq!(self.slot, dbank.slot); | ||
| assert_eq!(self.collector_id, dbank.collector_id); | ||
| assert_eq!(self.epoch_schedule, dbank.epoch_schedule); | ||
| assert_eq!(self.hashes_per_tick, dbank.hashes_per_tick); | ||
| assert_eq!(self.ticks_per_slot, dbank.ticks_per_slot); | ||
| assert_eq!(self.parent_hash, dbank.parent_hash); | ||
| assert_eq!( | ||
| self.tick_height.load(Relaxed), | ||
| dbank.tick_height.load(Relaxed) | ||
| ); | ||
| assert_eq!(self.is_delta.load(Relaxed), dbank.is_delta.load(Relaxed)); | ||
|
|
||
| { | ||
| let bh = self.hash.read().unwrap(); | ||
| let dbh = dbank.hash.read().unwrap(); | ||
| assert_eq!(*bh, *dbh); | ||
| } | ||
|
|
||
| { | ||
| let st = self.stakes.read().unwrap(); | ||
| let dst = dbank.stakes.read().unwrap(); | ||
| assert_eq!(*st, *dst); | ||
| } | ||
|
|
||
| { | ||
| let bhq = self.blockhash_queue.read().unwrap(); | ||
| let dbhq = dbank.blockhash_queue.read().unwrap(); | ||
| assert_eq!(*bhq, *dbhq); | ||
| } | ||
|
|
||
| { | ||
| let sc = self.src.status_cache.read().unwrap(); | ||
| let dsc = dbank.src.status_cache.read().unwrap(); | ||
| assert_eq!(*sc, *dsc); | ||
| } | ||
| assert_eq!( | ||
| self.rc.accounts.bank_hash_at(self.slot), | ||
| dbank.rc.accounts.bank_hash_at(dbank.slot) | ||
| ); | ||
| } | ||
|
|
||
| pub fn clean_accounts(&self, skip_last: bool) { | ||
| let max_clean_slot = if skip_last { | ||
| // Don't clean the slot we're snapshotting because it may have zero-lamport | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,7 +219,7 @@ fn test_bank_serialize_style(serde_style: SerdeStyle) { | |
| assert_eq!(dbank.get_balance(&key1.pubkey()), 0); | ||
| assert_eq!(dbank.get_balance(&key2.pubkey()), 10); | ||
| assert_eq!(dbank.get_balance(&key3.pubkey()), 0); | ||
| bank2.compare_bank(&dbank); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 (I wonder why this was like this not defining |
||
| assert!(bank2 == dbank); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits: how about
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue I ran into with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @carllin FYI, I've took the initiative here after stressed with not being able to freely |
||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,7 +68,7 @@ pub struct FeeRateGovernor { | |
| // The current cost of a signature This amount may increase/decrease over time based on | ||
| // cluster processing load. | ||
| #[serde(skip)] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is where gating would be necessary. Alternatively we just set this value on deserialize to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@carllin yeah, I think this solution is good.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, when did this bug started to occur? Since beginning? ;)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm yeah I would guess since it was first checked in xD |
||
| lamports_per_signature: u64, | ||
| pub lamports_per_signature: u64, | ||
|
|
||
| // The target cost of a signature when the cluster is operating around target_signatures_per_slot | ||
| // signatures | ||
|
|
||
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.
By the way, why can't we just
derivePartialEq? A comment might be desirable to explain the limitation.Uh oh!
There was an error while loading. Please reload this page.
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.
hehe so the issue was RwLock doesn't implement PartialEq, and we can't implement it for RwLock because it doesn't belong to this crate, added comment!