From 63fc178b7be4c32511404314237cc0dd4ac2640f Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Sun, 8 Jul 2018 20:26:43 -0600 Subject: [PATCH 1/3] Remove time_sources from bank I wrote this, but per https://github.com/solana-labs/solana#code-coverage, if it doesn't break a test, it's fair game to delete. --- src/bank.rs | 33 +++------------------------------ 1 file changed, 3 insertions(+), 30 deletions(-) diff --git a/src/bank.rs b/src/bank.rs index c3f0fec8aa164f..bd0c4e3c73e8e8 100644 --- a/src/bank.rs +++ b/src/bank.rs @@ -81,11 +81,6 @@ pub struct Bank { /// reject transactions with signatures its seen before last_ids_sigs: RwLock>>, - /// The set of trusted timekeepers. A Timestamp transaction from a `PublicKey` - /// outside this set will be discarded. Note that if validators do not have the - /// same set as leaders, they may interpret the ledger differently. - time_sources: RwLock>, - /// The most recent timestamp from a trusted timekeeper. This timestamp is applied /// to every smart contract when it enters the system. If it is waiting on a /// timestamp witness before that timestamp, the bank will execute it immediately. @@ -103,7 +98,6 @@ impl Default for Bank { pending: RwLock::new(HashMap::new()), last_ids: RwLock::new(VecDeque::new()), last_ids_sigs: RwLock::new(HashMap::new()), - time_sources: RwLock::new(HashSet::new()), last_time: RwLock::new(Utc.timestamp(0, 0)), transaction_count: AtomicUsize::new(0), } @@ -416,30 +410,9 @@ impl Bank { /// Process a Witness Timestamp. Any payment plans waiting on this timestamp /// will progress one step. - fn apply_timestamp(&self, from: PublicKey, dt: DateTime) -> Result<()> { - // If this is the first timestamp we've seen, it probably came from the genesis block, - // so we'll trust it. - if *self.last_time - .read() - .expect("'last_time' read lock on first timestamp check") - == Utc.timestamp(0, 0) - { - self.time_sources - .write() - .expect("'time_sources' write lock on first timestamp") - .insert(from); - } - - if self.time_sources - .read() - .expect("'time_sources' read lock") - .contains(&from) - { - if dt > *self.last_time.read().expect("'last_time' read lock") { - *self.last_time.write().expect("'last_time' write lock") = dt; - } - } else { - return Ok(()); + fn apply_timestamp(&self, _from: PublicKey, dt: DateTime) -> Result<()> { + if dt > *self.last_time.read().expect("'last_time' read lock") { + *self.last_time.write().expect("'last_time' write lock") = dt; } // Check to see if any timelocked transactions can be completed. From 6d7e4717d68b5495dac7c714514321b1c6442f70 Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Sun, 8 Jul 2018 20:33:12 -0600 Subject: [PATCH 2/3] Remove last_time from bank We had a test for this, but without `Bank::time_sources` (removed in the last commit), there's no last_time that can be trusted. --- src/bank.rs | 36 ++---------------------------------- 1 file changed, 2 insertions(+), 34 deletions(-) diff --git a/src/bank.rs b/src/bank.rs index bd0c4e3c73e8e8..320fbc4fe00652 100644 --- a/src/bank.rs +++ b/src/bank.rs @@ -81,11 +81,6 @@ pub struct Bank { /// reject transactions with signatures its seen before last_ids_sigs: RwLock>>, - /// The most recent timestamp from a trusted timekeeper. This timestamp is applied - /// to every smart contract when it enters the system. If it is waiting on a - /// timestamp witness before that timestamp, the bank will execute it immediately. - last_time: RwLock>, - /// The number of transactions the bank has processed without error since the /// start of the ledger. transaction_count: AtomicUsize, @@ -98,7 +93,6 @@ impl Default for Bank { pending: RwLock::new(HashMap::new()), last_ids: RwLock::new(VecDeque::new()), last_ids_sigs: RwLock::new(HashMap::new()), - last_time: RwLock::new(Utc.timestamp(0, 0)), transaction_count: AtomicUsize::new(0), } } @@ -238,11 +232,7 @@ impl Bank { fn apply_credits(&self, tx: &Transaction, balances: &mut HashMap) { match &tx.instruction { Instruction::NewContract(contract) => { - let mut plan = contract.plan.clone(); - plan.apply_witness(&Witness::Timestamp(*self.last_time - .read() - .expect("timestamp creation in apply_credits"))); - + let plan = contract.plan.clone(); if let Some(payment) = plan.final_payment() { self.apply_payment(&payment, balances); } else { @@ -411,10 +401,6 @@ impl Bank { /// Process a Witness Timestamp. Any payment plans waiting on this timestamp /// will progress one step. fn apply_timestamp(&self, _from: PublicKey, dt: DateTime) -> Result<()> { - if dt > *self.last_time.read().expect("'last_time' read lock") { - *self.last_time.write().expect("'last_time' write lock") = dt; - } - // Check to see if any timelocked transactions can be completed. let mut completed = vec![]; @@ -424,9 +410,7 @@ impl Bank { .write() .expect("'pending' write lock in apply_timestamp"); for (key, plan) in pending.iter_mut() { - plan.apply_witness(&Witness::Timestamp(*self.last_time - .read() - .expect("'last_time' read lock when creating timestamp"))); + plan.apply_witness(&Witness::Timestamp(dt)); if let Some(payment) = plan.final_payment() { self.apply_payment(&payment, &mut self.balances.write().unwrap()); completed.push(key.clone()); @@ -607,22 +591,6 @@ mod tests { assert_ne!(bank.get_balance(&pubkey), 2); } - #[test] - fn test_transfer_after_date() { - let mint = Mint::new(1); - let bank = Bank::new(&mint); - let pubkey = KeyPair::new().pubkey(); - let dt = Utc::now(); - bank.apply_timestamp(mint.pubkey(), dt).unwrap(); - - // It's now past now, so this transfer should be processed immediately. - bank.transfer_on_date(1, &mint.keypair(), pubkey, dt, mint.last_id()) - .unwrap(); - - assert_eq!(bank.get_balance(&mint.pubkey()), 0); - assert_eq!(bank.get_balance(&pubkey), 1); - } - #[test] fn test_cancel_transfer() { let mint = Mint::new(1); From 60cbe726b6c3435c7f40121b1acd4e9ec15aec36 Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Sun, 8 Jul 2018 21:04:55 -0600 Subject: [PATCH 3/3] Vet timestamp source from contract, not leader Per @aeyakovenko, contracts shouldn't trust the network for timestamps. Instead, pass the verified public key to the contract and let it decide if that's a public key it wants to trust the timestamp from. Fixes #405 --- src/bank.rs | 6 ++-- src/budget.rs | 70 ++++++++++++++++++++++++++++++--------------- src/payment_plan.rs | 4 +-- src/transaction.rs | 6 ++-- 4 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/bank.rs b/src/bank.rs index 320fbc4fe00652..d9f3182895d2d3 100644 --- a/src/bank.rs +++ b/src/bank.rs @@ -388,7 +388,7 @@ impl Bank { .expect("write() in apply_signature") .entry(tx_sig) { - e.get_mut().apply_witness(&Witness::Signature(from)); + e.get_mut().apply_witness(&Witness::Signature, &from); if let Some(payment) = e.get().final_payment() { self.apply_payment(&payment, &mut self.balances.write().unwrap()); e.remove_entry(); @@ -400,7 +400,7 @@ impl Bank { /// Process a Witness Timestamp. Any payment plans waiting on this timestamp /// will progress one step. - fn apply_timestamp(&self, _from: PublicKey, dt: DateTime) -> Result<()> { + fn apply_timestamp(&self, from: PublicKey, dt: DateTime) -> Result<()> { // Check to see if any timelocked transactions can be completed. let mut completed = vec![]; @@ -410,7 +410,7 @@ impl Bank { .write() .expect("'pending' write lock in apply_timestamp"); for (key, plan) in pending.iter_mut() { - plan.apply_witness(&Witness::Timestamp(dt)); + plan.apply_witness(&Witness::Timestamp(dt), &from); if let Some(payment) = plan.final_payment() { self.apply_payment(&payment, &mut self.balances.write().unwrap()); completed.push(key.clone()); diff --git a/src/budget.rs b/src/budget.rs index f2e9af55051f45..41f4f1033229bc 100644 --- a/src/budget.rs +++ b/src/budget.rs @@ -12,7 +12,7 @@ use std::mem; #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] pub enum Condition { /// Wait for a `Timestamp` `Witness` at or after the given `DateTime`. - Timestamp(DateTime), + Timestamp(DateTime, PublicKey), /// Wait for a `Signature` `Witness` from `PublicKey`. Signature(PublicKey), @@ -20,10 +20,12 @@ pub enum Condition { impl Condition { /// Return true if the given Witness satisfies this Condition. - pub fn is_satisfied(&self, witness: &Witness) -> bool { + pub fn is_satisfied(&self, witness: &Witness, from: &PublicKey) -> bool { match (self, witness) { - (Condition::Signature(pubkey), Witness::Signature(from)) => pubkey == from, - (Condition::Timestamp(dt), Witness::Timestamp(last_time)) => dt <= last_time, + (Condition::Signature(pubkey), Witness::Signature) => pubkey == from, + (Condition::Timestamp(dt, pubkey), Witness::Timestamp(last_time)) => { + pubkey == from && dt <= last_time + } _ => false, } } @@ -56,8 +58,13 @@ impl Budget { } /// Create a budget that pays `tokens` to `to` after the given DateTime. - pub fn new_future_payment(dt: DateTime, tokens: i64, to: PublicKey) -> Self { - Budget::After(Condition::Timestamp(dt), Payment { tokens, to }) + pub fn new_future_payment( + dt: DateTime, + from: PublicKey, + tokens: i64, + to: PublicKey, + ) -> Self { + Budget::After(Condition::Timestamp(dt, from), Payment { tokens, to }) } /// Create a budget that pays `tokens` to `to` after the given DateTime @@ -69,7 +76,7 @@ impl Budget { to: PublicKey, ) -> Self { Budget::Or( - (Condition::Timestamp(dt), Payment { tokens, to }), + (Condition::Timestamp(dt, from), Payment { tokens, to }), (Condition::Signature(from), Payment { tokens, to: from }), ) } @@ -94,11 +101,11 @@ impl PaymentPlan for Budget { /// Apply a witness to the budget to see if the budget can be reduced. /// If so, modify the budget in-place. - fn apply_witness(&mut self, witness: &Witness) { + fn apply_witness(&mut self, witness: &Witness, from: &PublicKey) { let new_payment = match self { - Budget::After(cond, payment) if cond.is_satisfied(witness) => Some(payment), - Budget::Or((cond, payment), _) if cond.is_satisfied(witness) => Some(payment), - Budget::Or(_, (cond, payment)) if cond.is_satisfied(witness) => Some(payment), + Budget::After(cond, payment) if cond.is_satisfied(witness, from) => Some(payment), + Budget::Or((cond, payment), _) if cond.is_satisfied(witness, from) => Some(payment), + Budget::Or(_, (cond, payment)) if cond.is_satisfied(witness, from) => Some(payment), _ => None, }.cloned(); @@ -111,20 +118,22 @@ impl PaymentPlan for Budget { #[cfg(test)] mod tests { use super::*; + use signature::{KeyPair, KeyPairUtil}; #[test] fn test_signature_satisfied() { - let sig = PublicKey::default(); - assert!(Condition::Signature(sig).is_satisfied(&Witness::Signature(sig))); + let from = PublicKey::default(); + assert!(Condition::Signature(from).is_satisfied(&Witness::Signature, &from)); } #[test] fn test_timestamp_satisfied() { let dt1 = Utc.ymd(2014, 11, 14).and_hms(8, 9, 10); let dt2 = Utc.ymd(2014, 11, 14).and_hms(10, 9, 8); - assert!(Condition::Timestamp(dt1).is_satisfied(&Witness::Timestamp(dt1))); - assert!(Condition::Timestamp(dt1).is_satisfied(&Witness::Timestamp(dt2))); - assert!(!Condition::Timestamp(dt2).is_satisfied(&Witness::Timestamp(dt1))); + let from = PublicKey::default(); + assert!(Condition::Timestamp(dt1, from).is_satisfied(&Witness::Timestamp(dt1), &from)); + assert!(Condition::Timestamp(dt1, from).is_satisfied(&Witness::Timestamp(dt2), &from)); + assert!(!Condition::Timestamp(dt2, from).is_satisfied(&Witness::Timestamp(dt1), &from)); } #[test] @@ -134,7 +143,7 @@ mod tests { let to = PublicKey::default(); assert!(Budget::new_payment(42, to).verify(42)); assert!(Budget::new_authorized_payment(from, 42, to).verify(42)); - assert!(Budget::new_future_payment(dt, 42, to).verify(42)); + assert!(Budget::new_future_payment(dt, from, 42, to).verify(42)); assert!(Budget::new_cancelable_future_payment(dt, from, 42, to).verify(42)); } @@ -144,20 +153,35 @@ mod tests { let to = PublicKey::default(); let mut budget = Budget::new_authorized_payment(from, 42, to); - budget.apply_witness(&Witness::Signature(from)); + budget.apply_witness(&Witness::Signature, &from); assert_eq!(budget, Budget::new_payment(42, to)); } #[test] fn test_future_payment() { let dt = Utc.ymd(2014, 11, 14).and_hms(8, 9, 10); - let to = PublicKey::default(); + let from = KeyPair::new().pubkey(); + let to = KeyPair::new().pubkey(); - let mut budget = Budget::new_future_payment(dt, 42, to); - budget.apply_witness(&Witness::Timestamp(dt)); + let mut budget = Budget::new_future_payment(dt, from, 42, to); + budget.apply_witness(&Witness::Timestamp(dt), &from); assert_eq!(budget, Budget::new_payment(42, to)); } + #[test] + fn test_unauthorized_future_payment() { + // Ensure timestamp will only be acknowledged if it came from the + // whitelisted public key. + let dt = Utc.ymd(2014, 11, 14).and_hms(8, 9, 10); + let from = KeyPair::new().pubkey(); + let to = KeyPair::new().pubkey(); + + let mut budget = Budget::new_future_payment(dt, from, 42, to); + let orig_budget = budget.clone(); + budget.apply_witness(&Witness::Timestamp(dt), &to); // <-- Attack! + assert_eq!(budget, orig_budget); + } + #[test] fn test_cancelable_future_payment() { let dt = Utc.ymd(2014, 11, 14).and_hms(8, 9, 10); @@ -165,11 +189,11 @@ mod tests { let to = PublicKey::default(); let mut budget = Budget::new_cancelable_future_payment(dt, from, 42, to); - budget.apply_witness(&Witness::Timestamp(dt)); + budget.apply_witness(&Witness::Timestamp(dt), &from); assert_eq!(budget, Budget::new_payment(42, to)); let mut budget = Budget::new_cancelable_future_payment(dt, from, 42, to); - budget.apply_witness(&Witness::Signature(from)); + budget.apply_witness(&Witness::Signature, &from); assert_eq!(budget, Budget::new_payment(42, from)); } } diff --git a/src/payment_plan.rs b/src/payment_plan.rs index 059c63df60459d..29b82a0056a93e 100644 --- a/src/payment_plan.rs +++ b/src/payment_plan.rs @@ -13,7 +13,7 @@ pub enum Witness { Timestamp(DateTime), /// A siganture from PublicKey. - Signature(PublicKey), + Signature, } /// Some amount of tokens that should be sent to the `to` `PublicKey`. @@ -36,5 +36,5 @@ pub trait PaymentPlan { /// Apply a witness to the payment plan to see if the plan can be reduced. /// If so, modify the plan in-place. - fn apply_witness(&mut self, witness: &Witness); + fn apply_witness(&mut self, witness: &Witness, from: &PublicKey); } diff --git a/src/transaction.rs b/src/transaction.rs index 2a8288c123c88f..f8df220673c136 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -32,9 +32,9 @@ impl PaymentPlan for Plan { } } - fn apply_witness(&mut self, witness: &Witness) { + fn apply_witness(&mut self, witness: &Witness, from: &PublicKey) { match self { - Plan::Budget(budget) => budget.apply_witness(witness), + Plan::Budget(budget) => budget.apply_witness(witness, from), } } } @@ -145,7 +145,7 @@ impl Transaction { ) -> Self { let from = from_keypair.pubkey(); let budget = Budget::Or( - (Condition::Timestamp(dt), Payment { tokens, to }), + (Condition::Timestamp(dt, from), Payment { tokens, to }), (Condition::Signature(from), Payment { tokens, to: from }), ); let plan = Plan::Budget(budget);