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

Secure sysvars under hash by freezing all strictly #7892

Merged
merged 4 commits into from
Jan 24, 2020
Merged

Secure sysvars under hash by freezing all strictly #7892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2d2413
Secure sysvars under hash by freezing all strictly
ryoqun Jan 20, 2020
11c28cf
Fix hash's non-idempotnet and add new test
ryoqun Jan 23, 2020
6f9ef48
Clean up
ryoqun Jan 23, 2020
4f1fe0a
More cleanups
ryoqun Jan 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions runtime/src/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,6 @@ mod tests {
use solana_sdk::rent::Rent;
use solana_sdk::signature::{Keypair, KeypairUtil};
use solana_sdk::system_program;
use solana_sdk::sysvar;
use solana_sdk::transaction::Transaction;
use std::io::Cursor;
use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
Expand Down Expand Up @@ -1295,14 +1294,6 @@ mod tests {
accounts.bank_hash_at(0);
}

#[test]
#[should_panic]
fn test_accounts_empty_account_bank_hash() {
let accounts = Accounts::new(Vec::new());
accounts.store_slow(0, &Pubkey::default(), &Account::new(1, 0, &sysvar::id()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sysvars are now the first citizen in the AccountsDB land. :)

accounts.bank_hash_at(0);
}

fn check_accounts(accounts: &Accounts, pubkeys: &Vec<Pubkey>, num: usize) {
for _ in 1..num {
let idx = thread_rng().gen_range(0, num - 1);
Expand Down
61 changes: 31 additions & 30 deletions runtime/src/accounts_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ use solana_sdk::bank_hash::BankHash;
use solana_sdk::clock::{Epoch, Slot};
use solana_sdk::hash::{Hash, Hasher};
use solana_sdk::pubkey::Pubkey;
use solana_sdk::sysvar;
Copy link
Contributor Author

@ryoqun ryoqun Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Bye bye, leaky abstraction; Feels so good. :)

use std::collections::{HashMap, HashSet};
use std::fmt;
use std::io::{BufReader, Cursor, Error as IOError, ErrorKind, Read, Result as IOResult};
Expand Down Expand Up @@ -759,6 +758,14 @@ impl AccountsDB {
let hash_info = bank_hashes
.get(&parent_slot)
.expect("accounts_db::set_hash::no parent slot");
if bank_hashes.get(&slot).is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this additional line in scope for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rob-solana Yes!

First, this PR added additional tests for the lazy initialization of slot hashes sysvar around here. That exact line internally hits here and bails out with this if condition. So, this if is needed to pass the test correctly. Otherwise, this following assertion for idempotent bank hash would fail because set_file without this if would incorrectly reset the bank_hash for the tested slot, which is shared by bank2 and bank3.

I know such sharing of a slot between child banks doesn't occur in the real validator behavior. Thus, I alternatively could replace this error! with panic! and adjust all tests. But it takes extra effort and introduces an artifical limitation to AccountsDB: Currently, all of other AccountsDB code doesn't forbid such slot sharing, it just works. Only this part didn't work; So I wanted to preserve that flexible property and managed to do that with this tiny additional if clause.

In other words, AccountsDB isn't aware of anything called forks and free of leaky abstraction now and forever. I think that's nice and beautiful design and separation of concern. :)

error!(
"set_hash: already exists; multiple forks with shared slot {} as child (parent: {})!?",
slot, parent_slot,
);
return;
}

let hash = hash_info.hash;
let new_hash_info = BankHashInfo {
hash,
Expand Down Expand Up @@ -1028,18 +1035,16 @@ impl AccountsDB {
|(collector, mismatch_found): &mut (Vec<BankHash>, bool),
option: Option<(&Pubkey, Account, Slot)>| {
if let Some((pubkey, account, slot)) = option {
if !sysvar::check_id(&account.owner) {
let hash = Self::hash_account(slot, &account, pubkey);
if hash != account.hash {
*mismatch_found = true;
}
if *mismatch_found {
return;
}
let hash = BankHash::from_hash(&hash);
debug!("xoring..{} key: {}", hash, pubkey);
collector.push(hash);
let hash = Self::hash_account(slot, &account, pubkey);
if hash != account.hash {
*mismatch_found = true;
}
if *mismatch_found {
return;
}
let hash = BankHash::from_hash(&hash);
debug!("xoring..{} key: {}", hash, pubkey);
collector.push(hash);
}
},
);
Expand Down Expand Up @@ -1162,26 +1167,22 @@ impl AccountsDB {
let hashes: Vec<_> = accounts
.iter()
.map(|(pubkey, account)| {
if !sysvar::check_id(&account.owner) {
let hash = BankHash::from_hash(&account.hash);
stats.update(account);
let new_hash = Self::hash_account(slot_id, account, pubkey);
let new_bank_hash = BankHash::from_hash(&new_hash);
debug!(
"hash_accounts: key: {} xor {} current: {}",
pubkey, hash, hash_state
);
if !had_account {
hash_state = hash;
had_account = true;
} else {
hash_state.xor(hash);
}
hash_state.xor(new_bank_hash);
new_hash
let hash = BankHash::from_hash(&account.hash);
stats.update(account);
let new_hash = Self::hash_account(slot_id, account, pubkey);
let new_bank_hash = BankHash::from_hash(&new_hash);
debug!(
"hash_accounts: key: {} xor {} current: {}",
pubkey, hash, hash_state
);
if !had_account {
hash_state = hash;
had_account = true;
} else {
Hash::default()
hash_state.xor(hash);
}
hash_state.xor(new_bank_hash);
new_hash
})
.collect();

Expand Down
Loading