From b32c645c4210f32754f0752cc73a2bb416379e81 Mon Sep 17 00:00:00 2001 From: George Danezis Date: Tue, 14 Dec 2021 21:06:39 +0000 Subject: [PATCH 01/10] Mutex protect object store, and make temp store in authority --- fastpay/src/server.rs | 2 +- fastpay_core/src/authority.rs | 99 ++++++++++++++----- .../src/unit_tests/authority_tests.rs | 55 ++++++----- fastpay_core/src/unit_tests/client_tests.rs | 2 +- 4 files changed, 106 insertions(+), 52 deletions(-) diff --git a/fastpay/src/server.rs b/fastpay/src/server.rs index 2b8bd005db9cd..8f2d99009510e 100644 --- a/fastpay/src/server.rs +++ b/fastpay/src/server.rs @@ -31,7 +31,7 @@ fn make_shard_server( let committee = Committee::new(committee_config.voting_rights()); let num_shards = server_config.authority.num_shards; - let mut state = AuthorityState::new_shard( + let state = AuthorityState::new_shard( committee, server_config.authority.address, server_config.key.copy(), diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index 5d74ee1a7aa43..4daec7954371f 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -16,7 +16,7 @@ use move_core_types::{ language_storage::{ModuleId, StructTag}, resolver::{ModuleResolver, ResourceResolver}, }; -use std::{collections::BTreeMap, convert::TryInto}; +use std::{collections::BTreeMap, convert::TryInto, sync::Arc, sync::Mutex}; #[cfg(test)] #[path = "unit_tests/authority_tests.rs"] @@ -37,7 +37,7 @@ pub struct AuthorityState { // The variable length dynamic state of the authority shard /// States of fastnft objects - objects: BTreeMap, + objects: Arc>>, /* Order lock states and invariants @@ -60,7 +60,7 @@ pub struct AuthorityState { certificates: BTreeMap, /// An index mapping object IDs to the Transaction Digest that created them. /// This is used by synchronization logic to sync authorities. - pub parent_sync: BTreeMap<(ObjectID, SequenceNumber), TransactionDigest>, + parent_sync: BTreeMap<(ObjectID, SequenceNumber), TransactionDigest>, } /// Interface provided by each (shard of an) authority. @@ -110,9 +110,8 @@ impl Authority for AuthorityState { // Get a ref to the object concerned by the transaction let object = self - .objects - .get(object_id) - .ok_or(FastPayError::ObjectNotFound)?; + .object_state(object_id) + .map_err(|_| FastPayError::ObjectNotFound)?; // Check that the seq number is the same fp_ensure!( @@ -170,11 +169,11 @@ impl Authority for AuthorityState { // If we have a certificate on the confirmation order it means that the input // object exists on other honest authorities, but we do not have it. The only // way this may happen is if we missed some updates. - let input_object = self.objects.get(&input_object_id).ok_or( + let input_object = self.object_state(&input_object_id).map_err(|_| { FastPayError::MissingEalierConfirmations { current_sequence_number: SequenceNumber::from(0), - }, - )?; + } + })?; let input_sequence_number = input_object.next_sequence_number; @@ -205,6 +204,8 @@ impl Authority for AuthorityState { let transaction_digest = self.add_certificate(certificate); let mut tx_ctx = TxContext::new(transaction_digest); + let mut temporary_store = AuthorityTemporaryStore::new(self, &inputs); + // Order-specific logic // // TODO: think very carefully what to do in case we throw an Err here. @@ -225,9 +226,11 @@ impl Authority for AuthorityState { // TODO(https://github.com/MystenLabs/fastnft/issues/45): charge for gas // TODO(https://github.com/MystenLabs/fastnft/issues/30): read value of c.object_arguments + // pass objects directly to the VM instead of passing ObjectRef's - // Note: They are included in the 'inputs' list of objects. + // Note: They are inlcuded in the 'inputs' list of objects. + + adapter::execute( - self, + &mut temporary_store, &c.module, &c.function, sender, @@ -243,7 +246,7 @@ impl Authority for AuthorityState { OrderKind::Publish(m) => { // TODO(https://github.com/MystenLabs/fastnft/issues/45): charge for gas let sender = m.sender.to_address_hack(); - match adapter::publish(self, m.modules, &sender, &mut tx_ctx) { + match adapter::publish(&mut temporary_store, m.modules, &sender, &mut tx_ctx) { Ok(outputs) => { // TODO: AccountInfoResponse should return all object ID outputs. but for now it only returns one, so use this hack object_id = outputs[0].id(); @@ -308,7 +311,7 @@ impl AuthorityState { committee, name, secret, - objects: BTreeMap::new(), + objects: Arc::new(Mutex::new(BTreeMap::new())), order_lock: BTreeMap::new(), shard_id: 0, number_of_shards: 1, @@ -328,7 +331,7 @@ impl AuthorityState { committee, name, secret, - objects: BTreeMap::new(), + objects: Arc::new(Mutex::new(BTreeMap::new())), order_lock: BTreeMap::new(), shard_id, number_of_shards, @@ -351,19 +354,22 @@ impl AuthorityState { Self::get_shard(self.number_of_shards, object_id) } - fn object_state(&self, object_id: &ObjectID) -> Result<&Object, FastPayError> { + fn object_state(&self, object_id: &ObjectID) -> Result { self.objects + .lock() + .unwrap() .get(object_id) + .cloned() .ok_or(FastPayError::UnknownSenderAccount) } - pub fn insert_object(&mut self, object: Object) { - self.objects.insert(object.id(), object); + pub fn insert_object(&self, object: Object) { + self.objects.lock().unwrap().insert(object.id(), object); } #[cfg(test)] - pub fn accounts_mut(&mut self) -> &mut BTreeMap { - &mut self.objects + pub fn accounts_mut(&self) -> &Arc>> { + &self.objects } /// Make an information summary of an object to help clients @@ -472,26 +478,65 @@ impl AuthorityState { } } -impl Storage for AuthorityState { +pub struct AuthorityTemporaryStore<'a> { + pub authority_state: &'a AuthorityState, + pub objects: BTreeMap, + pub written: Vec, + pub deleted: Vec, +} + +impl<'a> AuthorityTemporaryStore<'a> { + pub fn new( + authority_state: &'a AuthorityState, + _input_objects: &'_ [Object], + ) -> AuthorityTemporaryStore<'a> { + AuthorityTemporaryStore { + authority_state, + objects: _input_objects.iter().map(|v| (v.id(), v.clone())).collect(), + written: Vec::new(), + deleted: Vec::new(), + } + } +} + +impl<'a> Storage for AuthorityTemporaryStore<'a> { fn read_object(&self, id: &ObjectID) -> Option { - self.objects.get(id).cloned() + match self.objects.get(id) { + Some(x) => Some(x.clone()), + None => self + .authority_state + .objects + .lock() + .unwrap() + .get(id) + .cloned(), + } } // TODO: buffer changes to storage + flush buffer after commit() fn write_object(&mut self, object: Object) { - self.insert_object(object) + self.written.push(object.to_object_reference()); + self.objects.insert(object.id(), object); } // TODO: buffer changes to storage + flush buffer after commit() fn delete_object(&mut self, id: &ObjectID) { - self.objects.remove(id); + if let Some(removed) = self.objects.remove(id) { + self.deleted.push(removed.to_object_reference()); + } } } -impl ModuleResolver for AuthorityState { +impl<'a> ModuleResolver for AuthorityTemporaryStore<'a> { type Error = FastPayError; fn get_module(&self, module_id: &ModuleId) -> Result>, Self::Error> { - match self.objects.get(module_id.address()) { + match self + .authority_state + .objects + .lock() + .unwrap() + .get(module_id.address()) + { Some(o) => match &o.data { Data::Module(c) => { let mut bytes = Vec::new(); @@ -507,7 +552,7 @@ impl ModuleResolver for AuthorityState { } } -impl ResourceResolver for AuthorityState { +impl<'a> ResourceResolver for AuthorityTemporaryStore<'a> { type Error = FastPayError; fn get_resource( @@ -515,7 +560,7 @@ impl ResourceResolver for AuthorityState { address: &AccountAddress, struct_tag: &StructTag, ) -> Result>, Self::Error> { - match self.objects.get(address) { + match self.authority_state.objects.lock().unwrap().get(address) { Some(o) => match &o.data { Data::Move(m) => { assert!(struct_tag == &m.type_, "Invariant violation: ill-typed object in storage or bad object resquest from caller\ diff --git a/fastpay_core/src/unit_tests/authority_tests.rs b/fastpay_core/src/unit_tests/authority_tests.rs index efbf321e12b9f..83eac8c6abbef 100644 --- a/fastpay_core/src/unit_tests/authority_tests.rs +++ b/fastpay_core/src/unit_tests/authority_tests.rs @@ -24,7 +24,7 @@ fn test_handle_transfer_order_bad_signature() { .handle_order(bad_signature_transfer_order) .is_err()); - let object = authority_state.objects.get(&object_id).unwrap(); + let object = authority_state.object_state(&object_id).unwrap(); assert!(authority_state .get_order_lock(&object.to_object_reference()) .unwrap() @@ -51,7 +51,7 @@ fn test_handle_transfer_order_unknown_sender() { .handle_order(unknown_sender_transfer_order) .is_err()); - let object = authority_state.objects.get(&object_id).unwrap(); + let object = authority_state.object_state(&object_id).unwrap(); assert!(authority_state .get_order_lock(&object.to_object_reference()) .unwrap() @@ -112,7 +112,7 @@ fn test_handle_transfer_order_ok() { .handle_order(transfer_order.clone()) .unwrap(); - let object = authority_state.objects.get(&object_id).unwrap(); + let object = authority_state.object_state(&object_id).unwrap(); let pending_confirmation = authority_state .get_order_lock(&object.to_object_reference()) .unwrap() @@ -260,12 +260,18 @@ fn test_handle_confirmation_order_bad_sequence_number() { let object_id: ObjectID = ObjectID::random(); let recipient = dbg_addr(2); let mut authority_state = init_state_with_object(sender, object_id); - let sender_account = authority_state.objects.get_mut(&object_id).unwrap(); - sender_account.next_sequence_number = sender_account.next_sequence_number.increment().unwrap(); + + { + let mut lock = authority_state.objects.lock().unwrap(); + let sender_account = lock.get_mut(&object_id).unwrap(); + sender_account.next_sequence_number = + sender_account.next_sequence_number.increment().unwrap(); + } let old_seq_num; { - let old_account = authority_state.objects.get_mut(&object_id).unwrap(); + let mut lock = authority_state.objects.lock().unwrap(); + let old_account = lock.get_mut(&object_id).unwrap(); old_seq_num = old_account.next_sequence_number; } @@ -282,7 +288,7 @@ fn test_handle_confirmation_order_bad_sequence_number() { .handle_confirmation_order(ConfirmationOrder::new(certified_transfer_order)) .is_err()); - let new_account = authority_state.objects.get_mut(&object_id).unwrap(); + let new_account = authority_state.object_state(&object_id).unwrap(); assert_eq!(old_seq_num, new_account.next_sequence_number); assert!(authority_state @@ -290,7 +296,12 @@ fn test_handle_confirmation_order_bad_sequence_number() { .get(&(object_id, new_account.next_sequence_number)) .is_none()); - assert!(authority_state.objects.get(&dbg_object_id(2)).is_none()); + assert!(authority_state + .objects + .lock() + .unwrap() + .get(&dbg_object_id(2)) + .is_none()); } #[test] @@ -310,7 +321,7 @@ fn test_handle_confirmation_order_exceed_balance() { assert!(authority_state .handle_confirmation_order(ConfirmationOrder::new(certified_transfer_order)) .is_ok()); - let new_account = authority_state.objects.get(&object_id).unwrap(); + let new_account = authority_state.object_state(&object_id).unwrap(); assert_eq!(SequenceNumber::from(1), new_account.next_sequence_number); assert!(authority_state .parent_sync @@ -336,7 +347,7 @@ fn test_handle_confirmation_order_receiver_balance_overflow() { assert!(authority_state .handle_confirmation_order(ConfirmationOrder::new(certified_transfer_order)) .is_ok()); - let new_sender_account = authority_state.objects.get(&object_id).unwrap(); + let new_sender_account = authority_state.object_state(&object_id).unwrap(); assert_eq!( SequenceNumber::from(1), new_sender_account.next_sequence_number @@ -364,7 +375,7 @@ fn test_handle_confirmation_order_receiver_equal_sender() { assert!(authority_state .handle_confirmation_order(ConfirmationOrder::new(certified_transfer_order)) .is_ok()); - let account = authority_state.objects.get(&object_id).unwrap(); + let account = authority_state.object_state(&object_id).unwrap(); assert_eq!(SequenceNumber::from(1), account.next_sequence_number); assert!(authority_state @@ -387,7 +398,7 @@ fn test_handle_confirmation_order_ok() { &authority_state, ); - let old_account = authority_state.objects.get_mut(&object_id).unwrap(); + let old_account = authority_state.object_state(&object_id).unwrap(); let mut next_sequence_number = old_account.next_sequence_number; next_sequence_number = next_sequence_number.increment().unwrap(); @@ -424,10 +435,7 @@ fn test_account_state_ok() { let object_id = dbg_object_id(1); let authority_state = init_state_with_object(sender, object_id); - assert_eq!( - authority_state.objects.get(&object_id).unwrap(), - authority_state.object_state(&object_id).unwrap() - ); + authority_state.object_state(&object_id).unwrap(); } #[test] @@ -477,12 +485,13 @@ fn init_state_with_ids>( ) -> AuthorityState { let mut state = init_state(); for (address, object_id) in objects { - let account = state - .objects - .entry(object_id) - .or_insert_with(|| Object::with_id_for_testing(object_id)); - account.transfer(address); - + { + let mut unlocked_db = state.objects.lock().unwrap(); + let account = unlocked_db + .entry(object_id) + .or_insert_with(|| Object::with_id_for_testing(object_id)); + account.transfer(address); + } // drop lock state.init_order_lock((object_id, 0.into())); } state @@ -492,7 +501,7 @@ fn init_state_with_objects>(objects: I) -> Author let mut state = init_state(); for o in objects { let obj_ref = o.to_object_reference(); - state.objects.insert(o.id(), o); + state.objects.lock().unwrap().insert(o.id(), o); state.init_order_lock(obj_ref); } state diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index dd5dd3560c290..360170c37bd57 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -120,7 +120,7 @@ fn fund_account>>( let mut object = Object::with_id_for_testing(object_id); object.transfer(address); let mut client_ref = client.0.as_ref().try_lock().unwrap(); - client_ref.accounts_mut().insert(object_id, object); + client_ref.accounts_mut().lock().unwrap().insert(object_id, object); client_ref.init_order_lock((object_id, 0.into())); } } From 0db2310d19bd58f0ba8133e0b1e27d44f61222bd Mon Sep 17 00:00:00 2001 From: George Danezis Date: Wed, 15 Dec 2021 11:18:21 +0000 Subject: [PATCH 02/10] Connect vm output to dist sys logic --- fastpay_core/src/authority.rs | 72 ++++++++++++++++++++++------------- fastx_types/src/object.rs | 4 ++ 2 files changed, 50 insertions(+), 26 deletions(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index 4daec7954371f..45cd24524548b 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -191,11 +191,6 @@ impl Authority for AuthorityState { inputs.push(input_object.clone()); } - // Here we implement the semantics of a transfer transaction, by which - // the owner changes. Down the line here we do general smart contract - // execution. - let input_object_refs = order.input_objects(); - // Note: State is mutated below and should be committed in an atomic way // to memory or persistent storage. Currently this is done in memory // through the calls to store being infallible. @@ -204,12 +199,11 @@ impl Authority for AuthorityState { let transaction_digest = self.add_certificate(certificate); let mut tx_ctx = TxContext::new(transaction_digest); - let mut temporary_store = AuthorityTemporaryStore::new(self, &inputs); - // Order-specific logic // // TODO: think very carefully what to do in case we throw an Err here. - let outputs = match order.kind { + let mut temporary_store = AuthorityTemporaryStore::new(self, &inputs); + match order.kind { OrderKind::Transfer(t) => { let mut output_object = inputs[0].clone(); output_object.next_sequence_number = @@ -219,7 +213,7 @@ impl Authority for AuthorityState { Address::Primary(_) => PublicKeyBytes([0; 32]), Address::FastPay(addr) => addr, }); - vec![output_object] + temporary_store.write_object(output_object); } OrderKind::Call(c) => { let sender = c.sender.to_address_hack(); @@ -227,8 +221,6 @@ impl Authority for AuthorityState { // TODO(https://github.com/MystenLabs/fastnft/issues/30): read value of c.object_arguments + // pass objects directly to the VM instead of passing ObjectRef's // Note: They are inlcuded in the 'inputs' list of objects. - - adapter::execute( &mut temporary_store, &c.module, @@ -240,8 +232,6 @@ impl Authority for AuthorityState { Some(c.gas_budget), ) .map_err(|_| FastPayError::MoveExecutionFailure)?; - // TODO: return output objects here - Vec::new() } OrderKind::Publish(m) => { // TODO(https://github.com/MystenLabs/fastnft/issues/45): charge for gas @@ -250,31 +240,32 @@ impl Authority for AuthorityState { Ok(outputs) => { // TODO: AccountInfoResponse should return all object ID outputs. but for now it only returns one, so use this hack object_id = outputs[0].id(); - - outputs } Err(_e) => { // TODO: return this error to the client - Vec::new() } } } }; - for input_ref in input_object_refs { + // Extract the new state from the execution + let (objects, active_inputs, written, _deleted) = temporary_store.extract(); + + // Note: State is mutated below and should be committed in an atomic way + // to memory or persistent storage. Currently this is done in memory + // through the calls to store being infallible. + + for input_ref in active_inputs { self.archive_order_lock(&input_ref); } // Insert each output object into the stores, index and make locks for it. - for output_object in outputs { - let output_ref = output_object.to_object_reference(); - + for output_ref in written { // Index the certificate by the objects created self.add_parent_cert(output_ref, transaction_digest); // Add new object, init locks and remove old ones - self.insert_object(output_object); - // TODO: no need to init order locks for immutable outputs + self.insert_object(objects[&output_ref.0].clone()); self.init_order_lock(output_ref); } @@ -481,8 +472,9 @@ impl AuthorityState { pub struct AuthorityTemporaryStore<'a> { pub authority_state: &'a AuthorityState, pub objects: BTreeMap, - pub written: Vec, - pub deleted: Vec, + pub active_inputs: Vec, // Inputs that are not read only + pub written: Vec, // Objects written + pub deleted: Vec, // Objects activelly deleted } impl<'a> AuthorityTemporaryStore<'a> { @@ -493,10 +485,26 @@ impl<'a> AuthorityTemporaryStore<'a> { AuthorityTemporaryStore { authority_state, objects: _input_objects.iter().map(|v| (v.id(), v.clone())).collect(), + active_inputs: _input_objects + .iter() + .filter(|v| !v.is_read_only()) + .map(|v| v.to_object_reference()) + .collect(), written: Vec::new(), deleted: Vec::new(), } } + + pub fn extract( + self, + ) -> ( + BTreeMap, + Vec, + Vec, + Vec, + ) { + (self.objects, self.active_inputs, self.written, self.deleted) + } } impl<'a> Storage for AuthorityTemporaryStore<'a> { @@ -513,14 +521,26 @@ impl<'a> Storage for AuthorityTemporaryStore<'a> { } } - // TODO: buffer changes to storage + flush buffer after commit() fn write_object(&mut self, object: Object) { + // Check it is not read-only + if object.is_read_only() { + // TODO: should we throw an error here? + return; + } self.written.push(object.to_object_reference()); self.objects.insert(object.id(), object); } - // TODO: buffer changes to storage + flush buffer after commit() fn delete_object(&mut self, id: &ObjectID) { + // Check it is not read-only + if let Some(object) = self.read_object(id) { + if object.is_read_only() { + // TODO: should we throw an error here? + return; + } + } + + // If it exists remove it if let Some(removed) = self.objects.remove(id) { self.deleted.push(removed.to_object_reference()); } diff --git a/fastx_types/src/object.rs b/fastx_types/src/object.rs index e05a63f9aa7fe..4fdc61eb4218b 100644 --- a/fastx_types/src/object.rs +++ b/fastx_types/src/object.rs @@ -70,6 +70,10 @@ impl Object { } } + pub fn is_read_only(&self) -> bool { + self.data.is_read_only() + } + pub fn to_object_reference(&self) -> ObjectRef { (self.id(), self.next_sequence_number) } From 58f125806252b6aa89b2f3739b35aaff0d47e4b2 Mon Sep 17 00:00:00 2001 From: George Danezis Date: Wed, 15 Dec 2021 15:12:26 +0000 Subject: [PATCH 03/10] Updated adpater to use correct sequence numbers (issue #30) --- fastpay_core/src/authority.rs | 6 +++++ fastx_programmability/adapter/src/adapter.rs | 25 +++++++++++++------- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index 45cd24524548b..6bab0bb4fccf9 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -521,6 +521,12 @@ impl<'a> Storage for AuthorityTemporaryStore<'a> { } } + /* + Invariant: A key assumption of the write-delete logic + is that an entry is not both added and deleted by the + caller. + */ + fn write_object(&mut self, object: Object) { // Check it is not read-only if object.is_read_only() { diff --git a/fastx_programmability/adapter/src/adapter.rs b/fastx_programmability/adapter/src/adapter.rs index f30c66653cef7..07f8162745cdc 100644 --- a/fastx_programmability/adapter/src/adapter.rs +++ b/fastx_programmability/adapter/src/adapter.rs @@ -70,10 +70,13 @@ pub fn execute + ModuleResolver { // object mutated during execution - // TODO (https://github.com/MystenLabs/fastnft/issues/30): - // eventually, a mutation will only happen to an objects passed as a &mut input to the `main`, so we'll know - // its old sequence number. for now, we fake it. - let sequence_number = SequenceNumber::new(); + let sequence_number = state_view + .read_object(&addr) + .ok_or(FastPayError::ObjectNotFound)? + .next_sequence_number + .increment() + .map_err(|_| FastPayError::InvalidSequenceNumber)?; + let owner = FastPayAddress::from_move_address_hack(&sender); let object = Object::new_move(struct_tag, bytes, owner, sequence_number); @@ -92,14 +95,18 @@ pub fn execute + ModuleResolver unreachable!("Only structs can be transferred"), From 6e3d80180e548c0bfff84f6435b890024d6092d8 Mon Sep 17 00:00:00 2001 From: George Danezis Date: Wed, 15 Dec 2021 15:41:02 +0000 Subject: [PATCH 04/10] Revised based on feedback (Thanks @sam) --- fastpay_core/src/authority.rs | 48 ++++++++++++++++++++++------------- fastx_types/src/error.rs | 2 ++ 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index 6bab0bb4fccf9..07f5027021aa1 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -249,7 +249,7 @@ impl Authority for AuthorityState { }; // Extract the new state from the execution - let (objects, active_inputs, written, _deleted) = temporary_store.extract(); + let (objects, active_inputs, written, _deleted) = temporary_store.into_inner(); // Note: State is mutated below and should be committed in an atomic way // to memory or persistent storage. Currently this is done in memory @@ -474,7 +474,7 @@ pub struct AuthorityTemporaryStore<'a> { pub objects: BTreeMap, pub active_inputs: Vec, // Inputs that are not read only pub written: Vec, // Objects written - pub deleted: Vec, // Objects activelly deleted + pub deleted: Vec, // Objects actively deleted } impl<'a> AuthorityTemporaryStore<'a> { @@ -495,7 +495,7 @@ impl<'a> AuthorityTemporaryStore<'a> { } } - pub fn extract( + pub fn into_inner( self, ) -> ( BTreeMap, @@ -530,8 +530,9 @@ impl<'a> Storage for AuthorityTemporaryStore<'a> { fn write_object(&mut self, object: Object) { // Check it is not read-only if object.is_read_only() { - // TODO: should we throw an error here? - return; + // This is an internal invariant violation. Move only allows us to + // mutate objects if they are &mut so they cannot be read-only. + panic!("Internal invariant violation: Mutating a read-only object.") } self.written.push(object.to_object_reference()); self.objects.insert(object.id(), object); @@ -541,8 +542,9 @@ impl<'a> Storage for AuthorityTemporaryStore<'a> { // Check it is not read-only if let Some(object) = self.read_object(id) { if object.is_read_only() { - // TODO: should we throw an error here? - return; + // This is an internal invariant violation. Move only allows us to + // mutate objects if they are &mut so they cannot be read-only. + panic!("Internal invariant violation: Deleting a read-only object.") } } @@ -586,19 +588,29 @@ impl<'a> ResourceResolver for AuthorityTemporaryStore<'a> { address: &AccountAddress, struct_tag: &StructTag, ) -> Result>, Self::Error> { - match self.authority_state.objects.lock().unwrap().get(address) { - Some(o) => match &o.data { - Data::Move(m) => { - assert!(struct_tag == &m.type_, "Invariant violation: ill-typed object in storage or bad object resquest from caller\ -"); - Ok(Some(m.contents.clone())) + let object = match self.read_object(address) { + Some(x) => x, + None => match self.authority_state.objects.lock().unwrap().get(address) { + None => return Ok(None), + Some(x) => { + if !x.is_read_only() { + fp_bail!(FastPayError::ExecutionInvariantViolation); + } + x.clone() } - other => unimplemented!( - "Bad object lookup: expected Move object, but got {:?}", - other - ), }, - _ => Ok(None), + }; + + match &object.data { + Data::Move(m) => { + assert!(struct_tag == &m.type_, "Invariant violation: ill-typed object in storage or bad object resquest from caller\ +"); + Ok(Some(m.contents.clone())) + } + other => unimplemented!( + "Bad object lookup: expected Move object, but got {:?}", + other + ), } } } diff --git a/fastx_types/src/error.rs b/fastx_types/src/error.rs index 97e2b77e758a3..5a557ae4abac5 100644 --- a/fastx_types/src/error.rs +++ b/fastx_types/src/error.rs @@ -123,6 +123,8 @@ pub enum FastPayError { MoveExecutionFailure, #[error("Insufficent input objects")] InsufficientObjectNumber, + #[error("Execution invariant violated")] + ExecutionInvariantViolation, } pub type FastPayResult = Result; From 214db20c082fe621af640d708382e605717ab2d7 Mon Sep 17 00:00:00 2001 From: George Danezis Date: Wed, 15 Dec 2021 16:54:17 +0000 Subject: [PATCH 05/10] Handle read_only objects correctly in handle_order --- fastpay_core/src/authority.rs | 53 ++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index 07f5027021aa1..497dc667fa750 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -93,6 +93,7 @@ impl Authority for AuthorityState { // We first do all the checks that can be done in parallel with read only access to // the object database and lock database. let input_objects = order.input_objects(); + let mut mutable_objects = Vec::with_capacity(input_objects.len()); // Ensure at least one object is input to be mutated. fp_ensure!( @@ -100,25 +101,33 @@ impl Authority for AuthorityState { FastPayError::InsufficientObjectNumber ); - for object_ref in &input_objects { + for object_ref in input_objects { let (object_id, sequence_number) = object_ref; fp_ensure!( - *sequence_number <= SequenceNumber::max(), + sequence_number <= SequenceNumber::max(), FastPayError::InvalidSequenceNumber ); // Get a ref to the object concerned by the transaction let object = self - .object_state(object_id) + .object_state(&object_id) .map_err(|_| FastPayError::ObjectNotFound)?; // Check that the seq number is the same fp_ensure!( - object.next_sequence_number == *sequence_number, + object.next_sequence_number == sequence_number, FastPayError::UnexpectedSequenceNumber ); + // If this is an immutable object, we do no more checks + // and check no locks. + if object.is_read_only() { + continue; + } + + // Additional checks for mutable objects + // Check the transaction sender is also the object owner fp_ensure!( order.sender() == &object.owner, @@ -126,7 +135,7 @@ impl Authority for AuthorityState { ); // Check that this is the first, or same as the first order we sign. - if let Some(pending_confirmation) = self.get_order_lock(object_ref)? { + if let Some(pending_confirmation) = self.get_order_lock(&object_ref)? { fp_ensure!( pending_confirmation.order.kind == order.kind, FastPayError::PreviousTransferMustBeConfirmedFirst { @@ -134,8 +143,10 @@ impl Authority for AuthorityState { } ); // This exact transfer order was already signed. Return the previous value. - return self.make_object_info(*object_id); + return self.make_object_info(object_id); } + + mutable_objects.push((object_id, sequence_number)); } // TODO(https://github.com/MystenLabs/fastnft/issues/45): check that c.gas_payment exists + that its value is > gas_budget @@ -146,7 +157,7 @@ impl Authority for AuthorityState { let signed_order = SignedOrder::new(order, self.name, &self.secret); // This is the critical section that requires a write lock on the lock DB. - self.set_order_lock(signed_order)?; + self.set_order_lock(&mutable_objects, signed_order)?; let info = self.make_object_info(object_id)?; Ok(info) @@ -218,9 +229,6 @@ impl Authority for AuthorityState { OrderKind::Call(c) => { let sender = c.sender.to_address_hack(); // TODO(https://github.com/MystenLabs/fastnft/issues/45): charge for gas - // TODO(https://github.com/MystenLabs/fastnft/issues/30): read value of c.object_arguments + - // pass objects directly to the VM instead of passing ObjectRef's - // Note: They are inlcuded in the 'inputs' list of objects. adapter::execute( &mut temporary_store, &c.module, @@ -249,7 +257,7 @@ impl Authority for AuthorityState { }; // Extract the new state from the execution - let (objects, active_inputs, written, _deleted) = temporary_store.into_inner(); + let (mut objects, active_inputs, written, _deleted) = temporary_store.into_inner(); // Note: State is mutated below and should be committed in an atomic way // to memory or persistent storage. Currently this is done in memory @@ -265,8 +273,16 @@ impl Authority for AuthorityState { self.add_parent_cert(output_ref, transaction_digest); // Add new object, init locks and remove old ones - self.insert_object(objects[&output_ref.0].clone()); - self.init_order_lock(output_ref); + let object = objects + .remove(&output_ref.0) + .expect("By temporary_authority_store invarient object exists."); + + if !object.is_read_only() { + // Only objects that can be mutated have locks. + self.init_order_lock(output_ref); + } + + self.insert_object(object); } let info = self.make_object_info(object_id)?; @@ -388,7 +404,11 @@ impl AuthorityState { } /// Set the order lock to a specific transaction - pub fn set_order_lock(&mut self, signed_order: SignedOrder) -> Result<(), FastPayError> { + pub fn set_order_lock( + &mut self, + mutable_input_objects: &[ObjectRef], + signed_order: SignedOrder, + ) -> Result<(), FastPayError> { // This is the only function that writes as part of the handle_order flow // and the only that therefore needs an exclusive write lock on the lock // database. Inconsistent / delayed reads of the lock database do not result in safety @@ -400,12 +420,11 @@ impl AuthorityState { // This is a liveness issue for equivocating clients and therefore not an issue // we are trying to resolve. - let input_objects = signed_order.order.input_objects(); - for (object_id, seq) in input_objects { + for obj_ref in mutable_input_objects { // The object / version must exist, and therefore lock initialized. let lock = self .order_lock - .get_mut(&(object_id, seq)) + .get_mut(obj_ref) .ok_or(FastPayError::OrderLockDoesNotExist)?; if let Some(existing_signed_order) = lock { if existing_signed_order.order == signed_order.order { From 61a54b4db0e0edeb67a5e2de6f9f771c9b06e24b Mon Sep 17 00:00:00 2001 From: George Danezis Date: Thu, 16 Dec 2021 12:02:41 +0000 Subject: [PATCH 06/10] Intergrated review comments (thanks @sam & @francois) --- fastpay_core/src/authority.rs | 58 ++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index 497dc667fa750..e680a4056b8d5 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -37,6 +37,13 @@ pub struct AuthorityState { // The variable length dynamic state of the authority shard /// States of fastnft objects + /// + /// This is the placeholder data representation for the actual database + /// of objects that we will eventually have in a persistent store. Since + /// this database will have to be used by many refs of the authority, and + /// others it should be useable as a &ref for both reads and writes/deletes. + /// Right now we do this through placing it in an Arc/Mutex, but eventually + /// we will architect this to ensure perf. objects: Arc>>, /* Order lock states and invariants @@ -275,7 +282,7 @@ impl Authority for AuthorityState { // Add new object, init locks and remove old ones let object = objects .remove(&output_ref.0) - .expect("By temporary_authority_store invarient object exists."); + .expect("By temporary_authority_store invariant object exists."); if !object.is_read_only() { // Only objects that can be mutated have locks. @@ -489,11 +496,11 @@ impl AuthorityState { } pub struct AuthorityTemporaryStore<'a> { - pub authority_state: &'a AuthorityState, - pub objects: BTreeMap, - pub active_inputs: Vec, // Inputs that are not read only - pub written: Vec, // Objects written - pub deleted: Vec, // Objects actively deleted + authority_state: &'a AuthorityState, + objects: BTreeMap, + active_inputs: Vec, // Inputs that are not read only + written: Vec, // Objects written + deleted: Vec, // Objects actively deleted } impl<'a> AuthorityTemporaryStore<'a> { @@ -522,6 +529,38 @@ impl<'a> AuthorityTemporaryStore<'a> { Vec, Vec, ) { + #[cfg(test)] + { + // Check a number of invariants. A correct vm calling the struct should ensure these + // but to help with testing and refactoring we check again here. + for (i, write_ref) in self.written.iter().enumerate() { + for (j, write_ref2) in self.written.iter().enumerate() { + if i != j && write_ref == write_ref2 { + panic!("Invariant violation: duplicate writing."); + } + } + + for del_ref in &self.deleted { + if write_ref == del_ref { + panic!("Invariant violation: both writing and deleting same object."); + } + } + } + + for (k, del_ref) in self.deleted.iter().enumerate() { + for (l, del_ref2) in self.deleted.iter().enumerate() { + if k != l && del_ref == del_ref2 { + panic!("Invariant violation: duplicate deletion."); + } + } + } + + assert!( + self.active_inputs.len() == self.written.len() + self.deleted.len(), + "All mutable objects must be written or deleted." + ) + } + (self.objects, self.active_inputs, self.written, self.deleted) } } @@ -548,17 +587,20 @@ impl<'a> Storage for AuthorityTemporaryStore<'a> { fn write_object(&mut self, object: Object) { // Check it is not read-only + #[cfg(test)] // Movevm should ensure this if object.is_read_only() { // This is an internal invariant violation. Move only allows us to // mutate objects if they are &mut so they cannot be read-only. panic!("Internal invariant violation: Mutating a read-only object.") } + self.written.push(object.to_object_reference()); self.objects.insert(object.id(), object); } fn delete_object(&mut self, id: &ObjectID) { // Check it is not read-only + #[cfg(test)] // Movevm should ensure this if let Some(object) = self.read_object(id) { if object.is_read_only() { // This is an internal invariant violation. Move only allows us to @@ -570,6 +612,8 @@ impl<'a> Storage for AuthorityTemporaryStore<'a> { // If it exists remove it if let Some(removed) = self.objects.remove(id) { self.deleted.push(removed.to_object_reference()); + } else { + panic!("Internal invariant: object must exist to be deleted.") } } } @@ -622,7 +666,7 @@ impl<'a> ResourceResolver for AuthorityTemporaryStore<'a> { match &object.data { Data::Move(m) => { - assert!(struct_tag == &m.type_, "Invariant violation: ill-typed object in storage or bad object resquest from caller\ + assert!(struct_tag == &m.type_, "Invariant violation: ill-typed object in storage or bad object request from caller\ "); Ok(Some(m.contents.clone())) } From c0e9850317810e4de266f487800011345aa1fb7c Mon Sep 17 00:00:00 2001 From: George Danezis Date: Thu, 16 Dec 2021 13:13:38 +0000 Subject: [PATCH 07/10] Rebased, and fixed tests --- fastpay_core/src/authority.rs | 19 ++++++---- .../src/unit_tests/authority_tests.rs | 38 +++++++++++-------- fastx_programmability/adapter/src/adapter.rs | 24 ++++++------ 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index e680a4056b8d5..cfc8b40a64879 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -254,7 +254,7 @@ impl Authority for AuthorityState { match adapter::publish(&mut temporary_store, m.modules, &sender, &mut tx_ctx) { Ok(outputs) => { // TODO: AccountInfoResponse should return all object ID outputs. but for now it only returns one, so use this hack - object_id = outputs[0].id(); + object_id = outputs[0].0; } Err(_e) => { // TODO: return this error to the client @@ -390,7 +390,9 @@ impl AuthorityState { fn make_object_info(&self, object_id: ObjectID) -> Result { let object = self.object_state(&object_id)?; - let lock = self.get_order_lock(&object.to_object_reference())?; + let lock = self + .get_order_lock(&object.to_object_reference()) + .or(Ok(&None))?; Ok(AccountInfoResponse { object_id: object.id(), @@ -433,6 +435,7 @@ impl AuthorityState { .order_lock .get_mut(obj_ref) .ok_or(FastPayError::OrderLockDoesNotExist)?; + if let Some(existing_signed_order) = lock { if existing_signed_order.order == signed_order.order { // For some reason we are re-inserting the same order. Not optimal but correct. @@ -556,7 +559,7 @@ impl<'a> AuthorityTemporaryStore<'a> { } assert!( - self.active_inputs.len() == self.written.len() + self.deleted.len(), + self.active_inputs.len() <= self.written.len() + self.deleted.len(), "All mutable objects must be written or deleted." ) } @@ -588,10 +591,12 @@ impl<'a> Storage for AuthorityTemporaryStore<'a> { fn write_object(&mut self, object: Object) { // Check it is not read-only #[cfg(test)] // Movevm should ensure this - if object.is_read_only() { - // This is an internal invariant violation. Move only allows us to - // mutate objects if they are &mut so they cannot be read-only. - panic!("Internal invariant violation: Mutating a read-only object.") + if let Some(existing_object) = self.read_object(&object.id()) { + if existing_object.is_read_only() { + // This is an internal invariant violation. Move only allows us to + // mutate objects if they are &mut so they cannot be read-only. + panic!("Internal invariant violation: Mutating a read-only object.") + } } self.written.push(object.to_object_reference()); diff --git a/fastpay_core/src/unit_tests/authority_tests.rs b/fastpay_core/src/unit_tests/authority_tests.rs index 83eac8c6abbef..8ee5678a44b6a 100644 --- a/fastpay_core/src/unit_tests/authority_tests.rs +++ b/fastpay_core/src/unit_tests/authority_tests.rs @@ -256,18 +256,17 @@ fn test_handle_confirmation_order_unknown_sender() { #[test] fn test_handle_confirmation_order_bad_sequence_number() { + // TODO: refactor this test to be less magic: + // * Create an explicit state within an authority, by passing objects. + // * Create an explicit transfer, and execute it. + // * Then try to execute it again. + let (sender, sender_key) = get_key_pair(); let object_id: ObjectID = ObjectID::random(); let recipient = dbg_addr(2); let mut authority_state = init_state_with_object(sender, object_id); - { - let mut lock = authority_state.objects.lock().unwrap(); - let sender_account = lock.get_mut(&object_id).unwrap(); - sender_account.next_sequence_number = - sender_account.next_sequence_number.increment().unwrap(); - } - + // Record the old sequence number let old_seq_num; { let mut lock = authority_state.objects.lock().unwrap(); @@ -282,20 +281,29 @@ fn test_handle_confirmation_order_bad_sequence_number() { object_id, &authority_state, ); - // Replays are ignored. + // Increment the sequence number + { + let mut lock = authority_state.objects.lock().unwrap(); + let sender_object = lock.get_mut(&object_id).unwrap(); + sender_object.next_sequence_number = + sender_object.next_sequence_number.increment().unwrap(); + } + + // Explanation: providing an old cert that has already need applied + // returns a Ok(_) with info about the new object states. assert!(authority_state .handle_confirmation_order(ConfirmationOrder::new(certified_transfer_order)) - .is_err()); + .is_ok()); + // Check that the new object is the one recorded. let new_account = authority_state.object_state(&object_id).unwrap(); - assert_eq!(old_seq_num, new_account.next_sequence_number); - - assert!(authority_state - .parent_sync - .get(&(object_id, new_account.next_sequence_number)) - .is_none()); + assert_eq!( + old_seq_num.increment().unwrap(), + new_account.next_sequence_number + ); + // No recipient object was created. assert!(authority_state .objects .lock() diff --git a/fastx_programmability/adapter/src/adapter.rs b/fastx_programmability/adapter/src/adapter.rs index 07f8162745cdc..812bd61dc68ed 100644 --- a/fastx_programmability/adapter/src/adapter.rs +++ b/fastx_programmability/adapter/src/adapter.rs @@ -129,7 +129,7 @@ pub fn publish + ModuleResolver>, sender: &AccountAddress, ctx: &mut TxContext, -) -> Result, FastPayError> { +) -> Result, FastPayError> { if module_bytes.is_empty() { return Err(FastPayError::ModulePublishFailure { error: "Publishing empty list of modules".to_string(), @@ -179,16 +179,18 @@ pub fn publish + ModuleResolver Date: Fri, 17 Dec 2021 11:12:04 +0000 Subject: [PATCH 08/10] Improved checks and tests; fixed gas update bug --- fastpay_core/src/authority.rs | 86 +++++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 30 deletions(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index cfc8b40a64879..12fdceb1828a5 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -253,7 +253,16 @@ impl Authority for AuthorityState { let sender = m.sender.to_address_hack(); match adapter::publish(&mut temporary_store, m.modules, &sender, &mut tx_ctx) { Ok(outputs) => { - // TODO: AccountInfoResponse should return all object ID outputs. but for now it only returns one, so use this hack + // Fake the gas payment + let mut gas_object = temporary_store + .read_object(&object_id) + .expect("Checked existance at start of function."); + gas_object.next_sequence_number = + gas_object.next_sequence_number.increment()?; + temporary_store.write_object(gas_object); + + // TODO: AccountInfoResponse should return all object ID outputs. + // but for now it only returns one, so use this hack object_id = outputs[0].0; } Err(_e) => { @@ -524,6 +533,7 @@ impl<'a> AuthorityTemporaryStore<'a> { } } + /// Break up the structure and return its internal stores (objects, active_inputs, written, deleted) pub fn into_inner( self, ) -> ( @@ -532,39 +542,55 @@ impl<'a> AuthorityTemporaryStore<'a> { Vec, Vec, ) { - #[cfg(test)] - { - // Check a number of invariants. A correct vm calling the struct should ensure these - // but to help with testing and refactoring we check again here. - for (i, write_ref) in self.written.iter().enumerate() { - for (j, write_ref2) in self.written.iter().enumerate() { - if i != j && write_ref == write_ref2 { - panic!("Invariant violation: duplicate writing."); - } - } + self.check_invariants(); + (self.objects, self.active_inputs, self.written, self.deleted) + } - for del_ref in &self.deleted { - if write_ref == del_ref { - panic!("Invariant violation: both writing and deleting same object."); - } - } - } + /// An internal check of the invariants (will only fire in debug) + fn check_invariants(&self) { + // Check uniqueness in the 'written' set + debug_assert!( + { + use std::collections::HashSet; + let mut used = HashSet::new(); + self.written.iter().all(move |elt| used.insert(elt.0)) + }, + "Duplicate object reference in self.written." + ); - for (k, del_ref) in self.deleted.iter().enumerate() { - for (l, del_ref2) in self.deleted.iter().enumerate() { - if k != l && del_ref == del_ref2 { - panic!("Invariant violation: duplicate deletion."); - } - } - } + // Check uniqueness in the 'deleted' set + debug_assert!( + { + use std::collections::HashSet; + let mut used = HashSet::new(); + self.deleted.iter().all(move |elt| used.insert(elt.0)) + }, + "Duplicate object reference in self.deleted." + ); - assert!( - self.active_inputs.len() <= self.written.len() + self.deleted.len(), - "All mutable objects must be written or deleted." - ) - } + // Check not both deleted and written + debug_assert!( + { + use std::collections::HashSet; + let mut used = HashSet::new(); + self.written.iter().all(|elt| used.insert(elt.0)); + self.deleted.iter().all(move |elt| used.insert(elt.0)) + }, + "Object both written and deleted." + ); - (self.objects, self.active_inputs, self.written, self.deleted) + // Check all mutable inputs are either written or deleted + debug_assert!( + { + use std::collections::HashSet; + let mut used = HashSet::new(); + self.written.iter().all(|elt| used.insert(elt.0)); + self.deleted.iter().all(|elt| used.insert(elt.0)); + + self.active_inputs.iter().all(|elt| !used.insert(elt.0)) + }, + "Mutable input neither written nor deleted." + ); } } From 5fc8bfdb4a6050756820edc78c99dc3152b15577 Mon Sep 17 00:00:00 2001 From: George Danezis Date: Fri, 17 Dec 2021 11:23:03 +0000 Subject: [PATCH 09/10] Make fmt happy --- fastpay_core/src/unit_tests/client_tests.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index 360170c37bd57..a02e77eb504bd 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -120,7 +120,11 @@ fn fund_account>>( let mut object = Object::with_id_for_testing(object_id); object.transfer(address); let mut client_ref = client.0.as_ref().try_lock().unwrap(); - client_ref.accounts_mut().lock().unwrap().insert(object_id, object); + client_ref + .accounts_mut() + .lock() + .unwrap() + .insert(object_id, object); client_ref.init_order_lock((object_id, 0.into())); } } From f29e559102ea886a3c44d39f716b000e7d9c87fd Mon Sep 17 00:00:00 2001 From: George Danezis Date: Fri, 17 Dec 2021 15:46:37 +0000 Subject: [PATCH 10/10] Eliminate function call if not debug --- fastpay_core/src/authority.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fastpay_core/src/authority.rs b/fastpay_core/src/authority.rs index 12fdceb1828a5..5908196247db7 100644 --- a/fastpay_core/src/authority.rs +++ b/fastpay_core/src/authority.rs @@ -542,7 +542,10 @@ impl<'a> AuthorityTemporaryStore<'a> { Vec, Vec, ) { - self.check_invariants(); + #[cfg(debug_assertions)] + { + self.check_invariants(); + } (self.objects, self.active_inputs, self.written, self.deleted) }