-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Include rent_epoch and executable into account hash #7415
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 |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ use solana_measure::measure::Measure; | |
| use solana_rayon_threadlimit::get_thread_count; | ||
| use solana_sdk::account::Account; | ||
| use solana_sdk::bank_hash::BankHash; | ||
| use solana_sdk::clock::Slot; | ||
| use solana_sdk::clock::{Epoch, Slot}; | ||
| use solana_sdk::hash::{Hash, Hasher}; | ||
| use solana_sdk::pubkey::Pubkey; | ||
| use solana_sdk::sysvar; | ||
|
|
@@ -808,16 +808,32 @@ impl AccountsDB { | |
| Self::hash_account_data( | ||
| slot, | ||
| account.account_meta.lamports, | ||
| account.account_meta.executable, | ||
| account.account_meta.rent_epoch, | ||
| account.data, | ||
| &account.meta.pubkey, | ||
| ) | ||
| } | ||
|
|
||
| pub fn hash_account(slot: Slot, account: &Account, pubkey: &Pubkey) -> Hash { | ||
| Self::hash_account_data(slot, account.lamports, &account.data, pubkey) | ||
| Self::hash_account_data( | ||
| slot, | ||
| account.lamports, | ||
| account.executable, | ||
| account.rent_epoch, | ||
| &account.data, | ||
| pubkey, | ||
| ) | ||
| } | ||
|
|
||
| pub fn hash_account_data(slot: Slot, lamports: u64, data: &[u8], pubkey: &Pubkey) -> Hash { | ||
| pub fn hash_account_data( | ||
|
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. What do you think about passing the entire
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. Thought about the same, too! But there is a reason for the status quo: This function should be accessible from the other code path (no Of course, we can always normalize to
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. Oh ok. I think the current code is fine then, probably not worth getting fancy yet. Maybe when we add the 5th or 6th account field later :) |
||
| slot: Slot, | ||
| lamports: u64, | ||
| executable: bool, | ||
| rent_epoch: Epoch, | ||
| data: &[u8], | ||
| pubkey: &Pubkey, | ||
| ) -> Hash { | ||
| if lamports == 0 { | ||
|
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. |
||
| return Hash::default(); | ||
| } | ||
|
|
@@ -831,8 +847,17 @@ impl AccountsDB { | |
| LittleEndian::write_u64(&mut buf[..], slot); | ||
| hasher.hash(&buf); | ||
|
|
||
| LittleEndian::write_u64(&mut buf[..], rent_epoch); | ||
| hasher.hash(&buf); | ||
|
|
||
| hasher.hash(&data); | ||
|
|
||
| if executable { | ||
|
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. Note to myself: when sanitizing
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. Reply to myself: yeah, I've done this correctly at #7464 |
||
| hasher.hash(&[1u8; 1]); | ||
| } else { | ||
| hasher.hash(&[0u8; 1]); | ||
| } | ||
|
|
||
| hasher.hash(&pubkey.as_ref()); | ||
|
|
||
| hasher.result() | ||
|
|
@@ -1189,10 +1214,12 @@ impl AccountsDB { | |
| pub mod tests { | ||
| // TODO: all the bank tests are bank specific, issue: 2194 | ||
| use super::*; | ||
| use crate::append_vec::AccountMeta; | ||
| use bincode::serialize_into; | ||
| use rand::{thread_rng, Rng}; | ||
| use solana_sdk::account::Account; | ||
| use std::fs; | ||
| use std::str::FromStr; | ||
| use tempfile::TempDir; | ||
|
|
||
| #[test] | ||
|
|
@@ -2103,4 +2130,56 @@ pub mod tests { | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_hash_stored_account() { | ||
|
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. here |
||
| // This test uses some UNSAFE trick to detect most of account's field | ||
| // addition and deletion without changing the hash code | ||
|
|
||
| const ACCOUNT_DATA_LEN: usize = 3; | ||
| // the type of InputTuple elements must not contain references; | ||
| // they should be simple scalars or data blobs | ||
| type InputTuple = ( | ||
| Slot, | ||
| StoredMeta, | ||
| AccountMeta, | ||
| [u8; ACCOUNT_DATA_LEN], | ||
| usize, // for StoredAccount::offset | ||
|
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. I wanted to write this like |
||
| Hash, | ||
| ); | ||
| const INPUT_LEN: usize = std::mem::size_of::<InputTuple>(); | ||
| type InputBlob = [u8; INPUT_LEN]; | ||
| let mut blob: InputBlob = [0u8; INPUT_LEN]; | ||
|
|
||
| // spray memory with decreasing counts so that, data layout can be detected. | ||
| for (i, byte) in blob.iter_mut().enumerate() { | ||
| *byte = (INPUT_LEN - i) as u8; | ||
| } | ||
|
|
||
| //UNSAFE: forcibly cast the special byte pattern to actual account fields. | ||
|
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. My first UNSAFE here. :) Is there any better way to archive similar test result?
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. Neat idea. An
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.
Thx! This will be a precedent for the follow-up PRs, in which we need more of these crafted binaries to protect validator from crashing from external data. :) |
||
| let (slot, meta, account_meta, data, offset, hash): InputTuple = | ||
| unsafe { std::mem::transmute::<InputBlob, InputTuple>(blob) }; | ||
|
|
||
| let stored_account = StoredAccount { | ||
| meta: &meta, | ||
| account_meta: &account_meta, | ||
| data: &data, | ||
| offset, | ||
| hash: &hash, | ||
| }; | ||
| let account = stored_account.clone_account(); | ||
| let expected_account_hash = | ||
| Hash::from_str("GGTsxvxwnMsNfN6yYbBVQaRgvbVLfxeWnGXNyB8iXDyE").unwrap(); | ||
|
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
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. Out of my pure laziness, I've skipped to calculate this test vector by hand. :p
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. meh! #9917 |
||
|
|
||
| assert_eq!( | ||
| AccountsDB::hash_stored_account(slot, &stored_account), | ||
| expected_account_hash, | ||
| "StoredAccount's data layout might be changed; update hashing if needed." | ||
| ); | ||
| assert_eq!( | ||
| AccountsDB::hash_account(slot, &account, &stored_account.meta.pubkey), | ||
| expected_account_hash, | ||
| "Account-based hashing must be consistent with StoredAccount-based one." | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,8 @@ macro_rules! align_up { | |
| } | ||
|
|
||
| /// Meta contains enough context to recover the index from storage itself | ||
| /// This struct will be backed by mmaped and snapshotted data files. | ||
| /// So the data layout must be stable and consistent across the entire cluster! | ||
| #[derive(Clone, PartialEq, Debug)] | ||
| pub struct StoredMeta { | ||
| /// global write version | ||
|
|
@@ -31,6 +33,8 @@ pub struct StoredMeta { | |
| pub data_len: u64, | ||
| } | ||
|
|
||
| /// This struct will be backed by mmaped and snapshotted data files. | ||
| /// So the data layout must be stable and consistent across the entire cluster! | ||
|
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. Rather than comments, I wonder if we should define an attribute to start clearly structs that are part of the Solana ABI, like This is totally out of scope of this PR but if that sounds good, @ryoqun do you want to think about this more and make a design proposal for how we might start to track ABI changes
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. Heh, let's talk about ABI compat! That task looks interesting to me! I'll give it a try! This reminds of my rookie days when I found an ABI-related bug: #6388 Always, these are pesky bugs.. Hopefully, I can find good solution! |
||
| #[derive(Serialize, Deserialize, Clone, Debug, Default, Eq, PartialEq)] | ||
| pub struct AccountMeta { | ||
| /// lamports in the account | ||
|
|
@@ -187,9 +191,7 @@ impl AppendVec { | |
| } | ||
|
|
||
| fn get_slice(&self, offset: usize, size: usize) -> Option<(&[u8], usize)> { | ||
| let len = self.len(); | ||
|
|
||
| if len < offset + size { | ||
| if offset + size > self.len() { | ||
| return None; | ||
| } | ||
| let data = &self.map[offset..offset + size]; | ||
|
|
||
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.
lamportsandrent_epochare bothu64. So interchanging them caused no compile error. Spent some time to figure out why hashes are different. ;) So I added another assertion covering that kind of bug.