From c0c0c08ad2ad3edb3aa3b390d3f30b21d5636399 Mon Sep 17 00:00:00 2001 From: patrick Date: Thu, 9 Dec 2021 12:29:29 +0000 Subject: [PATCH 1/9] fund account using objects instead of amount --- fastpay_core/src/client.rs | 2 - fastpay_core/src/unit_tests/client_tests.rs | 174 ++++++++++++-------- 2 files changed, 103 insertions(+), 73 deletions(-) diff --git a/fastpay_core/src/client.rs b/fastpay_core/src/client.rs index 9f3af09391de6..7774ac2a0a776 100644 --- a/fastpay_core/src/client.rs +++ b/fastpay_core/src/client.rs @@ -90,7 +90,6 @@ pub trait Client { /// Do not confirm the transaction. fn transfer_to_fastpay_unsafe_unconfirmed( &mut self, - amount: Amount, recipient: FastPayAddress, object_id: ObjectID, user_data: UserData, @@ -686,7 +685,6 @@ where fn transfer_to_fastpay_unsafe_unconfirmed( &mut self, - _amount: Amount, recipient: FastPayAddress, object_id: ObjectID, user_data: UserData, diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index 28df5bb58725d..d85dbe5320513 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -4,7 +4,7 @@ use super::*; use crate::authority::{Authority, AuthorityState}; -use fastx_types::{base_types::Amount, object::Object}; +use fastx_types::object::Object; use futures::lock::Mutex; use std::{ collections::{BTreeMap, HashMap}, @@ -95,7 +95,6 @@ fn init_local_authorities_bad_1( fn make_client( authority_clients: HashMap, committee: Committee, - object_ids: Vec, ) -> ClientState { let (address, secret) = get_key_pair(); ClientState::new( @@ -106,58 +105,65 @@ fn make_client( SequenceNumber::new(), Vec::new(), Vec::new(), - object_ids, + Vec::new(), ) } #[cfg(test)] -fn fund_account>( - clients: &mut HashMap, +fn fund_account>>( + clients: Vec<&LocalAuthorityClient>, address: FastPayAddress, - balances: I, - object_id: ObjectID, + object_ids: I, ) { - let _balances = balances.into_iter().map(Balance::from); - for (_, client) in clients.iter_mut() { + let mut object_ids = object_ids.into_iter(); + for client in clients.clone() { let addr = address; - let object_id = object_id; - let mut object = Object::with_id_for_testing(object_id); - object.transfer(addr); - - client - .0 - .as_ref() - .try_lock() - .unwrap() - .accounts_mut() - .insert(object_id, object); - - client - .0 - .as_ref() - .try_lock() - .unwrap() - .init_order_lock((object_id, 0.into())); + let object_ids: Vec = object_ids.next().unwrap_or(Vec::new()); + + for object_id in object_ids { + let mut object = Object::with_id_for_testing(object_id); + object.transfer(addr); + client + .0 + .as_ref() + .try_lock() + .unwrap() + .accounts_mut() + .insert(object_id, object); + + client + .0 + .as_ref() + .try_lock() + .unwrap() + .init_order_lock((object_id, 0.into())); + } } } #[cfg(test)] -fn init_local_client_state(balances: Vec) -> ClientState { - let (mut authority_clients, committee) = init_local_authorities(balances.len()); - let object_id = ObjectID::random(); - let client = make_client(authority_clients.clone(), committee, vec![object_id]); - fund_account(&mut authority_clients, client.address, balances, object_id); +fn init_local_client_state(object_ids: Vec>) -> ClientState { + let (authority_clients, committee) = init_local_authorities(object_ids.len()); + let client = make_client(authority_clients.clone(), committee); + fund_account( + authority_clients.values().collect(), + client.address, + object_ids, + ); client } #[cfg(test)] fn init_local_client_state_with_bad_authority( - balances: Vec, + object_ids: Vec>, ) -> ClientState { - let (mut authority_clients, committee) = init_local_authorities_bad_1(balances.len()); - let object_id = ObjectID::random(); - let client = make_client(authority_clients.clone(), committee, vec![object_id]); - fund_account(&mut authority_clients, client.address, balances, object_id); + let (authority_clients, committee) = init_local_authorities_bad_1(object_ids.len()); + let client = make_client(authority_clients.clone(), committee); + fund_account( + authority_clients.values().collect(), + client.address, + object_ids, + ); client } @@ -180,11 +186,19 @@ fn test_get_strong_majority_balance() { fn test_initiating_valid_transfer() { let mut rt = Runtime::new().unwrap(); let (recipient, _) = get_key_pair(); - let mut sender = init_local_client_state(vec![2, 4, 4, 4]); - let object_id = *sender.object_ids.first().unwrap(); + let object_id_1 = ObjectID::random(); + let object_id_2 = ObjectID::random(); + let authority_objects = vec![ + vec![object_id_1], + vec![object_id_1, object_id_2], + vec![object_id_1, object_id_2], + vec![object_id_1, object_id_2], + ]; + + let mut sender = init_local_client_state(authority_objects); let certificate = rt .block_on(sender.transfer_to_fastpay( - object_id, + object_id_1, recipient, UserData(Some(*b"hello...........hello...........")), )) @@ -192,12 +206,16 @@ fn test_initiating_valid_transfer() { assert_eq!(sender.next_sequence_number, SequenceNumber::from(1)); assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.get_strong_majority_balance(object_id)), + rt.block_on(sender.get_strong_majority_balance(object_id_1)), Balance::from(0) ); assert_eq!( - rt.block_on(sender.request_certificate(sender.address, object_id, SequenceNumber::from(0))) - .unwrap(), + rt.block_on(sender.request_certificate( + sender.address, + object_id_1, + SequenceNumber::from(0) + )) + .unwrap(), certificate ); } @@ -206,8 +224,14 @@ fn test_initiating_valid_transfer() { fn test_initiating_valid_transfer_despite_bad_authority() { let mut rt = Runtime::new().unwrap(); let (recipient, _) = get_key_pair(); - let mut sender = init_local_client_state_with_bad_authority(vec![4, 4, 4, 4]); - let object_id = *sender.object_ids.first().unwrap(); + let object_id = ObjectID::random(); + let authority_objects = vec![ + vec![object_id], + vec![object_id], + vec![object_id], + vec![object_id], + ]; + let mut sender = init_local_client_state_with_bad_authority(authority_objects); let certificate = rt .block_on(sender.transfer_to_fastpay( object_id, @@ -232,19 +256,23 @@ fn test_initiating_valid_transfer_despite_bad_authority() { fn test_initiating_transfer_low_funds() { let mut rt = Runtime::new().unwrap(); let (recipient, _) = get_key_pair(); - let mut sender = init_local_client_state(vec![2, 2, 4, 4]); + let object_id_1 = ObjectID::random(); + let object_id_2 = ObjectID::random(); + let authority_objects = vec![ + vec![object_id_1], + vec![object_id_1], + vec![object_id_1, object_id_2], + vec![object_id_1, object_id_2], + ]; + let mut sender = init_local_client_state(authority_objects); assert!(rt - .block_on(sender.transfer_to_fastpay( - *sender.object_ids.first().unwrap(), - recipient, - UserData::default() - )) + .block_on(sender.transfer_to_fastpay(object_id_1, recipient, UserData::default(),)) .is_ok()); // Trying to overspend does not block an account. assert_eq!(sender.next_sequence_number, SequenceNumber::from(1)); // assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.get_strong_majority_balance(*sender.object_ids.first().unwrap())), + rt.block_on(sender.get_strong_majority_balance(object_id_1)), Balance::from(0) ); } @@ -252,19 +280,21 @@ fn test_initiating_transfer_low_funds() { #[test] fn test_bidirectional_transfer() { let mut rt = Runtime::new().unwrap(); - let (mut authority_clients, committee) = init_local_authorities(4); + let (authority_clients, committee) = init_local_authorities(4); + let mut client1 = make_client(authority_clients.clone(), committee.clone()); + let mut client2 = make_client(authority_clients.clone(), committee); + let object_id = ObjectID::random(); - let mut client1 = make_client( - authority_clients.clone(), - committee.clone(), + let authority_objects = vec![ vec![object_id], - ); - let mut client2 = make_client(authority_clients.clone(), committee, Vec::new()); + vec![object_id], + vec![object_id], + vec![object_id], + ]; fund_account( - &mut authority_clients, + authority_clients.values().collect(), client1.address, - vec![2, 3, 4, 4], - object_id, + authority_objects, ); // Update client1's local balance accordingly. @@ -332,25 +362,27 @@ fn test_bidirectional_transfer() { #[test] fn test_receiving_unconfirmed_transfer() { let mut rt = Runtime::new().unwrap(); - let (mut authority_clients, committee) = init_local_authorities(4); + let (authority_clients, committee) = init_local_authorities(4); + let mut client1 = make_client(authority_clients.clone(), committee.clone()); + let mut client2 = make_client(authority_clients.clone(), committee); + let object_id = ObjectID::random(); - let mut client1 = make_client( - authority_clients.clone(), - committee.clone(), + let authority_objects = vec![ vec![object_id], - ); - let mut client2 = make_client(authority_clients.clone(), committee, Vec::new()); + vec![object_id], + vec![object_id], + vec![object_id], + ]; + fund_account( - &mut authority_clients, + authority_clients.values().collect(), client1.address, - vec![2, 3, 4, 4], - object_id, + authority_objects, ); // not updating client1.balance let certificate = rt .block_on(client1.transfer_to_fastpay_unsafe_unconfirmed( - Amount::from(2), client2.address, object_id, UserData::default(), From 7c47f290b7bb6e7c58db451dd47de5f7c032bd57 Mon Sep 17 00:00:00 2001 From: patrick Date: Fri, 10 Dec 2021 15:28:09 +0000 Subject: [PATCH 2/9] refactored get_strong_majority_balance to object_ownership_have_quorum to confirm the client have ownership of an object in test --- fastpay_core/src/client.rs | 30 ++++-- fastpay_core/src/unit_tests/client_tests.rs | 113 ++++++++++++++------ 2 files changed, 100 insertions(+), 43 deletions(-) diff --git a/fastpay_core/src/client.rs b/fastpay_core/src/client.rs index 7774ac2a0a776..47aae152a9aa2 100644 --- a/fastpay_core/src/client.rs +++ b/fastpay_core/src/client.rs @@ -6,6 +6,7 @@ use anyhow::{bail, ensure}; use fastx_types::{ base_types::*, committee::Committee, error::FastPayError, fp_ensure, messages::*, }; +use futures::stream::FuturesUnordered; use futures::{future, StreamExt}; use rand::seq::SliceRandom; use std::collections::{btree_map, BTreeMap, BTreeSet, HashMap}; @@ -258,7 +259,7 @@ where request_sequence_number: None, request_received_transfers_excluding_first_nth: None, }; - let numbers: futures::stream::FuturesUnordered<_> = self + let numbers: FuturesUnordered<_> = self .authority_clients .iter_mut() .map(|(name, client)| { @@ -276,31 +277,40 @@ where ) } - /// Find the highest balance that is backed by a quorum of authorities. + /// Return true if the ownership of the object is backed by a quorum of authorities. /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value. #[cfg(test)] - async fn get_strong_majority_balance(&mut self, object_id: ObjectID) -> Balance { + async fn object_ownership_have_quorum(&mut self, object_id: ObjectID) -> bool { let request = AccountInfoRequest { object_id, request_sequence_number: None, request_received_transfers_excluding_first_nth: None, }; - let numbers: futures::stream::FuturesUnordered<_> = self + let address = self.address; + let numbers: FuturesUnordered<_> = self .authority_clients .iter_mut() .map(|(name, client)| { let fut = client.handle_account_info_request(request.clone()); async move { match fut.await { - Ok(_info) => Some((*name, Balance::from(0))), - _ => None, + Ok(info) => { + if info.owner == address { + Some((*name, Some(info.object_id))) + } else { + Some((*name, None)) + } + } + _ => Some((*name, None)), } } }) .collect(); - self.committee.get_strong_majority_lower_bound( - numbers.filter_map(|x| async move { x }).collect().await, - ) + self.committee + .get_strong_majority_lower_bound( + numbers.filter_map(|x| async move { x }).collect().await, + ) + .is_some() } /// Execute a sequence of actions in parallel for a quorum of authorities. @@ -313,7 +323,7 @@ where { let committee = &self.committee; let authority_clients = &mut self.authority_clients; - let mut responses: futures::stream::FuturesUnordered<_> = authority_clients + let mut responses: FuturesUnordered<_> = authority_clients .iter_mut() .map(|(name, client)| { let execute = execute.clone(); diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index d85dbe5320513..4cb6b78eea815 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -167,20 +167,43 @@ fn init_local_client_state_with_bad_authority( client } -/*#[test] -fn test_get_strong_majority_balance() { +#[test] +fn test_object_ownership_have_quorum() { let mut rt = Runtime::new().unwrap(); rt.block_on(async { - let mut client = init_local_client_state(vec![3, 4, 4, 4]); - assert_eq!(client.get_strong_majority_balance().await, Balance::from(0)); - - let mut client = init_local_client_state(vec![0, 3, 4, 4]); - assert_eq!(client.get_strong_majority_balance().await, Balance::from(0)); - - let mut client = init_local_client_state(vec![0, 3, 4]); - assert_eq!(client.get_strong_majority_balance().await, Balance::from(0)); + let object_id_1 = ObjectID::random(); + let object_id_2 = ObjectID::random(); + let authority_objects = vec![ + vec![object_id_1], + vec![object_id_1, object_id_2], + vec![object_id_1, object_id_2], + vec![object_id_1, object_id_2], + ]; + let mut client = init_local_client_state(authority_objects); + assert_eq!(client.object_ownership_have_quorum(object_id_1).await, true); + assert_eq!(client.object_ownership_have_quorum(object_id_2).await, true); + + let object_id_1 = ObjectID::random(); + let object_id_2 = ObjectID::random(); + let object_id_3 = ObjectID::random(); + let authority_objects = vec![ + vec![object_id_1], + vec![object_id_2, object_id_3], + vec![object_id_3, object_id_2], + vec![object_id_3], + ]; + let mut client = init_local_client_state(authority_objects); + assert_eq!( + client.object_ownership_have_quorum(object_id_1).await, + false + ); + assert_eq!( + client.object_ownership_have_quorum(object_id_2).await, + false + ); + assert_eq!(client.object_ownership_have_quorum(object_id_3).await, true); }); -}*/ +} #[test] fn test_initiating_valid_transfer() { @@ -196,6 +219,14 @@ fn test_initiating_valid_transfer() { ]; let mut sender = init_local_client_state(authority_objects); + assert_eq!( + rt.block_on(sender.object_ownership_have_quorum(object_id_1)), + true + ); + assert_eq!( + rt.block_on(sender.object_ownership_have_quorum(object_id_2)), + true + ); let certificate = rt .block_on(sender.transfer_to_fastpay( object_id_1, @@ -206,14 +237,18 @@ fn test_initiating_valid_transfer() { assert_eq!(sender.next_sequence_number, SequenceNumber::from(1)); assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.get_strong_majority_balance(object_id_1)), - Balance::from(0) + rt.block_on(sender.object_ownership_have_quorum(object_id_1)), + false + ); + assert_eq!( + rt.block_on(sender.object_ownership_have_quorum(object_id_2)), + true ); assert_eq!( rt.block_on(sender.request_certificate( sender.address, object_id_1, - SequenceNumber::from(0) + SequenceNumber::from(0), )) .unwrap(), certificate @@ -242,8 +277,8 @@ fn test_initiating_valid_transfer_despite_bad_authority() { assert_eq!(sender.next_sequence_number, SequenceNumber::from(1)); assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.get_strong_majority_balance(object_id)), - Balance::from(0) + rt.block_on(sender.object_ownership_have_quorum(object_id)), + false ); assert_eq!( rt.block_on(sender.request_certificate(sender.address, object_id, SequenceNumber::from(0))) @@ -266,14 +301,18 @@ fn test_initiating_transfer_low_funds() { ]; let mut sender = init_local_client_state(authority_objects); assert!(rt - .block_on(sender.transfer_to_fastpay(object_id_1, recipient, UserData::default(),)) - .is_ok()); + .block_on(sender.transfer_to_fastpay(object_id_2, recipient, UserData::default())) + .is_err()); // Trying to overspend does not block an account. - assert_eq!(sender.next_sequence_number, SequenceNumber::from(1)); + assert_eq!(sender.next_sequence_number, SequenceNumber::from(0)); // assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.get_strong_majority_balance(object_id_1)), - Balance::from(0) + rt.block_on(sender.object_ownership_have_quorum(object_id_1)), + true, + ); + assert_eq!( + rt.block_on(sender.object_ownership_have_quorum(object_id_2)), + false ); } @@ -296,6 +335,14 @@ fn test_bidirectional_transfer() { client1.address, authority_objects, ); + assert_eq!( + rt.block_on(client1.object_ownership_have_quorum(object_id)), + true + ); + assert_eq!( + rt.block_on(client2.object_ownership_have_quorum(object_id)), + false + ); // Update client1's local balance accordingly. let certificate = rt @@ -305,8 +352,12 @@ fn test_bidirectional_transfer() { assert_eq!(client1.next_sequence_number, SequenceNumber::from(1)); assert_eq!(client1.pending_transfer, None); assert_eq!( - rt.block_on(client1.get_strong_majority_balance(object_id)), - Balance::from(0) + rt.block_on(client1.object_ownership_have_quorum(object_id)), + false + ); + assert_eq!( + rt.block_on(client2.object_ownership_have_quorum(object_id)), + true ); assert_eq!( rt.block_on(client1.get_strong_majority_sequence_number(object_id)), @@ -323,16 +374,12 @@ fn test_bidirectional_transfer() { certificate ); // Our sender already confirmed. - assert_eq!( - rt.block_on(client2.get_strong_majority_balance(object_id)), - Balance::from(0) - ); // Try to confirm again. rt.block_on(client2.receive_from_fastpay(certificate)) .unwrap(); assert_eq!( - rt.block_on(client2.get_strong_majority_balance(object_id)), - Balance::from(0) + rt.block_on(client2.object_ownership_have_quorum(object_id)), + true, ); /* TODO: Fix client to track objects rather than accounts and test sending back to object to previous sender. @@ -394,8 +441,8 @@ fn test_receiving_unconfirmed_transfer() { assert_eq!(client1.pending_transfer, None); // ..but not confirmed remotely, hence an unchanged balance and sequence number. assert_eq!( - rt.block_on(client1.get_strong_majority_balance(object_id)), - Balance::from(0) + rt.block_on(client1.object_ownership_have_quorum(object_id)), + true, ); assert_eq!( rt.block_on(client1.get_strong_majority_sequence_number(object_id)), @@ -405,8 +452,8 @@ fn test_receiving_unconfirmed_transfer() { rt.block_on(client2.receive_from_fastpay(certificate)) .unwrap(); assert_eq!( - rt.block_on(client2.get_strong_majority_balance(object_id)), - Balance::from(0) + rt.block_on(client2.object_ownership_have_quorum(object_id)), + true ); } From b85667b060ec9778f8dd4dbc760a72bec06825cd Mon Sep 17 00:00:00 2001 From: patrick Date: Fri, 10 Dec 2021 18:26:15 +0000 Subject: [PATCH 3/9] removed unnecessary changes --- fastpay_core/src/client.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fastpay_core/src/client.rs b/fastpay_core/src/client.rs index 47aae152a9aa2..e1bedd96e394a 100644 --- a/fastpay_core/src/client.rs +++ b/fastpay_core/src/client.rs @@ -6,7 +6,6 @@ use anyhow::{bail, ensure}; use fastx_types::{ base_types::*, committee::Committee, error::FastPayError, fp_ensure, messages::*, }; -use futures::stream::FuturesUnordered; use futures::{future, StreamExt}; use rand::seq::SliceRandom; use std::collections::{btree_map, BTreeMap, BTreeSet, HashMap}; @@ -259,7 +258,7 @@ where request_sequence_number: None, request_received_transfers_excluding_first_nth: None, }; - let numbers: FuturesUnordered<_> = self + let numbers: futures::stream::FuturesUnordered<_> = self .authority_clients .iter_mut() .map(|(name, client)| { @@ -287,7 +286,7 @@ where request_received_transfers_excluding_first_nth: None, }; let address = self.address; - let numbers: FuturesUnordered<_> = self + let numbers: futures::stream::FuturesUnordered<_> = self .authority_clients .iter_mut() .map(|(name, client)| { @@ -323,7 +322,7 @@ where { let committee = &self.committee; let authority_clients = &mut self.authority_clients; - let mut responses: FuturesUnordered<_> = authority_clients + let mut responses: futures::stream::FuturesUnordered<_> = authority_clients .iter_mut() .map(|(name, client)| { let execute = execute.clone(); From 20e28f7c9c59256e2d8f66271d218db06be2c9a8 Mon Sep 17 00:00:00 2001 From: patrick Date: Mon, 13 Dec 2021 13:01:46 +0000 Subject: [PATCH 4/9] address PR comments --- fastpay_core/src/client.rs | 19 ++++---- fastpay_core/src/unit_tests/client_tests.rs | 49 +++++++++++---------- 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/fastpay_core/src/client.rs b/fastpay_core/src/client.rs index e1bedd96e394a..d098eceb495be 100644 --- a/fastpay_core/src/client.rs +++ b/fastpay_core/src/client.rs @@ -279,7 +279,10 @@ where /// Return true if the ownership of the object is backed by a quorum of authorities. /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value. #[cfg(test)] - async fn object_ownership_have_quorum(&mut self, object_id: ObjectID) -> bool { + async fn object_ownership_have_quorum( + &mut self, + object_id: ObjectID, + ) -> Option { let request = AccountInfoRequest { object_id, request_sequence_number: None, @@ -295,21 +298,19 @@ where match fut.await { Ok(info) => { if info.owner == address { - Some((*name, Some(info.object_id))) + Some((*name, Some(info.next_sequence_number))) } else { - Some((*name, None)) + None } } - _ => Some((*name, None)), + _ => None, } } }) .collect(); - self.committee - .get_strong_majority_lower_bound( - numbers.filter_map(|x| async move { x }).collect().await, - ) - .is_some() + self.committee.get_strong_majority_lower_bound( + numbers.filter_map(|x| async move { x }).collect().await, + ) } /// Execute a sequence of actions in parallel for a quorum of authorities. diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index 4cb6b78eea815..8814d7d286123 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -180,8 +180,14 @@ fn test_object_ownership_have_quorum() { vec![object_id_1, object_id_2], ]; let mut client = init_local_client_state(authority_objects); - assert_eq!(client.object_ownership_have_quorum(object_id_1).await, true); - assert_eq!(client.object_ownership_have_quorum(object_id_2).await, true); + assert_eq!( + client.object_ownership_have_quorum(object_id_1).await, + Some(SequenceNumber::from(0)) + ); + assert_eq!( + client.object_ownership_have_quorum(object_id_2).await, + Some(SequenceNumber::from(0)) + ); let object_id_1 = ObjectID::random(); let object_id_2 = ObjectID::random(); @@ -193,15 +199,12 @@ fn test_object_ownership_have_quorum() { vec![object_id_3], ]; let mut client = init_local_client_state(authority_objects); + assert_eq!(client.object_ownership_have_quorum(object_id_1).await, None); + assert_eq!(client.object_ownership_have_quorum(object_id_2).await, None); assert_eq!( - client.object_ownership_have_quorum(object_id_1).await, - false - ); - assert_eq!( - client.object_ownership_have_quorum(object_id_2).await, - false + client.object_ownership_have_quorum(object_id_3).await, + Some(SequenceNumber::from(0)) ); - assert_eq!(client.object_ownership_have_quorum(object_id_3).await, true); }); } @@ -221,11 +224,11 @@ fn test_initiating_valid_transfer() { let mut sender = init_local_client_state(authority_objects); assert_eq!( rt.block_on(sender.object_ownership_have_quorum(object_id_1)), - true + Some(SequenceNumber::from(0)) ); assert_eq!( rt.block_on(sender.object_ownership_have_quorum(object_id_2)), - true + Some(SequenceNumber::from(0)) ); let certificate = rt .block_on(sender.transfer_to_fastpay( @@ -238,11 +241,11 @@ fn test_initiating_valid_transfer() { assert_eq!(sender.pending_transfer, None); assert_eq!( rt.block_on(sender.object_ownership_have_quorum(object_id_1)), - false + None ); assert_eq!( rt.block_on(sender.object_ownership_have_quorum(object_id_2)), - true + Some(SequenceNumber::from(0)) ); assert_eq!( rt.block_on(sender.request_certificate( @@ -278,7 +281,7 @@ fn test_initiating_valid_transfer_despite_bad_authority() { assert_eq!(sender.pending_transfer, None); assert_eq!( rt.block_on(sender.object_ownership_have_quorum(object_id)), - false + None ); assert_eq!( rt.block_on(sender.request_certificate(sender.address, object_id, SequenceNumber::from(0))) @@ -308,11 +311,11 @@ fn test_initiating_transfer_low_funds() { // assert_eq!(sender.pending_transfer, None); assert_eq!( rt.block_on(sender.object_ownership_have_quorum(object_id_1)), - true, + Some(SequenceNumber::from(0)), ); assert_eq!( rt.block_on(sender.object_ownership_have_quorum(object_id_2)), - false + None ); } @@ -337,11 +340,11 @@ fn test_bidirectional_transfer() { ); assert_eq!( rt.block_on(client1.object_ownership_have_quorum(object_id)), - true + Some(SequenceNumber::from(0)) ); assert_eq!( rt.block_on(client2.object_ownership_have_quorum(object_id)), - false + None ); // Update client1's local balance accordingly. @@ -353,11 +356,11 @@ fn test_bidirectional_transfer() { assert_eq!(client1.pending_transfer, None); assert_eq!( rt.block_on(client1.object_ownership_have_quorum(object_id)), - false + None ); assert_eq!( rt.block_on(client2.object_ownership_have_quorum(object_id)), - true + Some(SequenceNumber::from(1)) ); assert_eq!( rt.block_on(client1.get_strong_majority_sequence_number(object_id)), @@ -379,7 +382,7 @@ fn test_bidirectional_transfer() { .unwrap(); assert_eq!( rt.block_on(client2.object_ownership_have_quorum(object_id)), - true, + Some(SequenceNumber::from(1)), ); /* TODO: Fix client to track objects rather than accounts and test sending back to object to previous sender. @@ -442,7 +445,7 @@ fn test_receiving_unconfirmed_transfer() { // ..but not confirmed remotely, hence an unchanged balance and sequence number. assert_eq!( rt.block_on(client1.object_ownership_have_quorum(object_id)), - true, + Some(SequenceNumber::from(0)), ); assert_eq!( rt.block_on(client1.get_strong_majority_sequence_number(object_id)), @@ -453,7 +456,7 @@ fn test_receiving_unconfirmed_transfer() { .unwrap(); assert_eq!( rt.block_on(client2.object_ownership_have_quorum(object_id)), - true + Some(SequenceNumber::from(1)) ); } From 8f272edf9c7c58757e22ae2ec04d15f4ff52a3ea Mon Sep 17 00:00:00 2001 From: patrick Date: Wed, 15 Dec 2021 16:15:44 +0000 Subject: [PATCH 5/9] address PR comments --- fastpay_core/src/client.rs | 25 +++---- fastpay_core/src/unit_tests/client_tests.rs | 78 ++++++++++----------- 2 files changed, 48 insertions(+), 55 deletions(-) diff --git a/fastpay_core/src/client.rs b/fastpay_core/src/client.rs index d098eceb495be..53d20b3643520 100644 --- a/fastpay_core/src/client.rs +++ b/fastpay_core/src/client.rs @@ -252,14 +252,14 @@ where /// Find the highest sequence number that is known to a quorum of authorities. /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value. #[cfg(test)] - async fn get_strong_majority_sequence_number(&mut self, object_id: ObjectID) -> SequenceNumber { + async fn get_strong_majority_sequence_number(&self, object_id: ObjectID) -> SequenceNumber { let request = AccountInfoRequest { object_id, request_sequence_number: None, request_received_transfers_excluding_first_nth: None, }; - let numbers: futures::stream::FuturesUnordered<_> = self - .authority_clients + let mut authority_clients = self.authority_clients.clone(); + let numbers: futures::stream::FuturesUnordered<_> = authority_clients .iter_mut() .map(|(name, client)| { let fut = client.handle_account_info_request(request.clone()); @@ -279,30 +279,23 @@ where /// Return true if the ownership of the object is backed by a quorum of authorities. /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value. #[cfg(test)] - async fn object_ownership_have_quorum( - &mut self, + async fn get_strong_majority_owner( + &self, object_id: ObjectID, - ) -> Option { + ) -> Option<(FastPayAddress, SequenceNumber)> { let request = AccountInfoRequest { object_id, request_sequence_number: None, request_received_transfers_excluding_first_nth: None, }; - let address = self.address; - let numbers: futures::stream::FuturesUnordered<_> = self - .authority_clients + let mut authority_clients = self.authority_clients.clone(); + let numbers: futures::stream::FuturesUnordered<_> = authority_clients .iter_mut() .map(|(name, client)| { let fut = client.handle_account_info_request(request.clone()); async move { match fut.await { - Ok(info) => { - if info.owner == address { - Some((*name, Some(info.next_sequence_number))) - } else { - None - } - } + Ok(info) => Some((*name, Some((info.owner, info.next_sequence_number)))), _ => None, } } diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index 8814d7d286123..04a03162076dc 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -168,7 +168,7 @@ fn init_local_client_state_with_bad_authority( } #[test] -fn test_object_ownership_have_quorum() { +fn test_get_strong_majority_owner() { let mut rt = Runtime::new().unwrap(); rt.block_on(async { let object_id_1 = ObjectID::random(); @@ -179,14 +179,14 @@ fn test_object_ownership_have_quorum() { vec![object_id_1, object_id_2], vec![object_id_1, object_id_2], ]; - let mut client = init_local_client_state(authority_objects); + let client = init_local_client_state(authority_objects); assert_eq!( - client.object_ownership_have_quorum(object_id_1).await, - Some(SequenceNumber::from(0)) + client.get_strong_majority_owner(object_id_1).await, + Some((client.address, SequenceNumber::from(0))) ); assert_eq!( - client.object_ownership_have_quorum(object_id_2).await, - Some(SequenceNumber::from(0)) + client.get_strong_majority_owner(object_id_2).await, + Some((client.address, SequenceNumber::from(0))) ); let object_id_1 = ObjectID::random(); @@ -198,12 +198,12 @@ fn test_object_ownership_have_quorum() { vec![object_id_3, object_id_2], vec![object_id_3], ]; - let mut client = init_local_client_state(authority_objects); - assert_eq!(client.object_ownership_have_quorum(object_id_1).await, None); - assert_eq!(client.object_ownership_have_quorum(object_id_2).await, None); + let client = init_local_client_state(authority_objects); + assert_eq!(client.get_strong_majority_owner(object_id_1).await, None); + assert_eq!(client.get_strong_majority_owner(object_id_2).await, None); assert_eq!( - client.object_ownership_have_quorum(object_id_3).await, - Some(SequenceNumber::from(0)) + client.get_strong_majority_owner(object_id_3).await, + Some((client.address, SequenceNumber::from(0))) ); }); } @@ -223,12 +223,12 @@ fn test_initiating_valid_transfer() { let mut sender = init_local_client_state(authority_objects); assert_eq!( - rt.block_on(sender.object_ownership_have_quorum(object_id_1)), - Some(SequenceNumber::from(0)) + rt.block_on(sender.get_strong_majority_owner(object_id_1)), + Some((sender.address, SequenceNumber::from(0))) ); assert_eq!( - rt.block_on(sender.object_ownership_have_quorum(object_id_2)), - Some(SequenceNumber::from(0)) + rt.block_on(sender.get_strong_majority_owner(object_id_2)), + Some((sender.address, SequenceNumber::from(0))) ); let certificate = rt .block_on(sender.transfer_to_fastpay( @@ -240,12 +240,12 @@ fn test_initiating_valid_transfer() { assert_eq!(sender.next_sequence_number, SequenceNumber::from(1)); assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.object_ownership_have_quorum(object_id_1)), - None + rt.block_on(sender.get_strong_majority_owner(object_id_1)), + Some((recipient, SequenceNumber::from(1))) ); assert_eq!( - rt.block_on(sender.object_ownership_have_quorum(object_id_2)), - Some(SequenceNumber::from(0)) + rt.block_on(sender.get_strong_majority_owner(object_id_2)), + Some((sender.address, SequenceNumber::from(0))) ); assert_eq!( rt.block_on(sender.request_certificate( @@ -280,8 +280,8 @@ fn test_initiating_valid_transfer_despite_bad_authority() { assert_eq!(sender.next_sequence_number, SequenceNumber::from(1)); assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.object_ownership_have_quorum(object_id)), - None + rt.block_on(sender.get_strong_majority_owner(object_id)), + Some((recipient, SequenceNumber::from(1))) ); assert_eq!( rt.block_on(sender.request_certificate(sender.address, object_id, SequenceNumber::from(0))) @@ -310,12 +310,12 @@ fn test_initiating_transfer_low_funds() { assert_eq!(sender.next_sequence_number, SequenceNumber::from(0)); // assert_eq!(sender.pending_transfer, None); assert_eq!( - rt.block_on(sender.object_ownership_have_quorum(object_id_1)), - Some(SequenceNumber::from(0)), + rt.block_on(sender.get_strong_majority_owner(object_id_1)), + Some((sender.address, SequenceNumber::from(0))), ); assert_eq!( - rt.block_on(sender.object_ownership_have_quorum(object_id_2)), - None + rt.block_on(sender.get_strong_majority_owner(object_id_2)), + None, ); } @@ -339,12 +339,12 @@ fn test_bidirectional_transfer() { authority_objects, ); assert_eq!( - rt.block_on(client1.object_ownership_have_quorum(object_id)), - Some(SequenceNumber::from(0)) + rt.block_on(client1.get_strong_majority_owner(object_id)), + Some((client1.address, SequenceNumber::from(0))) ); assert_eq!( - rt.block_on(client2.object_ownership_have_quorum(object_id)), - None + rt.block_on(client2.get_strong_majority_owner(object_id)), + Some((client1.address, SequenceNumber::from(0))) ); // Update client1's local balance accordingly. @@ -355,12 +355,12 @@ fn test_bidirectional_transfer() { assert_eq!(client1.next_sequence_number, SequenceNumber::from(1)); assert_eq!(client1.pending_transfer, None); assert_eq!( - rt.block_on(client1.object_ownership_have_quorum(object_id)), - None + rt.block_on(client1.get_strong_majority_owner(object_id)), + Some((client2.address, SequenceNumber::from(1))) ); assert_eq!( - rt.block_on(client2.object_ownership_have_quorum(object_id)), - Some(SequenceNumber::from(1)) + rt.block_on(client2.get_strong_majority_owner(object_id)), + Some((client2.address, SequenceNumber::from(1))) ); assert_eq!( rt.block_on(client1.get_strong_majority_sequence_number(object_id)), @@ -381,8 +381,8 @@ fn test_bidirectional_transfer() { rt.block_on(client2.receive_from_fastpay(certificate)) .unwrap(); assert_eq!( - rt.block_on(client2.object_ownership_have_quorum(object_id)), - Some(SequenceNumber::from(1)), + rt.block_on(client2.get_strong_majority_owner(object_id)), + Some((client2.address, SequenceNumber::from(1))) ); /* TODO: Fix client to track objects rather than accounts and test sending back to object to previous sender. @@ -444,8 +444,8 @@ fn test_receiving_unconfirmed_transfer() { assert_eq!(client1.pending_transfer, None); // ..but not confirmed remotely, hence an unchanged balance and sequence number. assert_eq!( - rt.block_on(client1.object_ownership_have_quorum(object_id)), - Some(SequenceNumber::from(0)), + rt.block_on(client1.get_strong_majority_owner(object_id)), + Some((client1.address, SequenceNumber::from(0))) ); assert_eq!( rt.block_on(client1.get_strong_majority_sequence_number(object_id)), @@ -455,8 +455,8 @@ fn test_receiving_unconfirmed_transfer() { rt.block_on(client2.receive_from_fastpay(certificate)) .unwrap(); assert_eq!( - rt.block_on(client2.object_ownership_have_quorum(object_id)), - Some(SequenceNumber::from(1)) + rt.block_on(client2.get_strong_majority_owner(object_id)), + Some((client2.address, SequenceNumber::from(1))) ); } From 81bcc6f22c506fd29231517bb1199584a698394d Mon Sep 17 00:00:00 2001 From: patrick Date: Wed, 15 Dec 2021 23:10:10 +0000 Subject: [PATCH 6/9] address PR comment --- fastpay_core/src/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fastpay_core/src/client.rs b/fastpay_core/src/client.rs index 53d20b3643520..b89e1be7754ad 100644 --- a/fastpay_core/src/client.rs +++ b/fastpay_core/src/client.rs @@ -276,7 +276,7 @@ where ) } - /// Return true if the ownership of the object is backed by a quorum of authorities. + /// Return owner address and sequence number of an object backed by a quorum of authorities. /// NOTE: This is only reliable in the synchronous model, with a sufficient timeout value. #[cfg(test)] async fn get_strong_majority_owner( From d01c7ad4dfed8c826bd801d505e1a4c6f4433ebf Mon Sep 17 00:00:00 2001 From: patrick Date: Thu, 16 Dec 2021 09:58:30 +0000 Subject: [PATCH 7/9] address PR comment, removed mut from `send_recv_bytes_internal` and all functions uses it. --- fastpay/src/bench.rs | 2 +- fastpay/src/network.rs | 6 +++--- fastpay_core/src/client.rs | 6 +++--- fastpay_core/src/unit_tests/client_tests.rs | 10 +++------- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/fastpay/src/bench.rs b/fastpay/src/bench.rs index 50f3685c3c234..748eed109a6b2 100644 --- a/fastpay/src/bench.rs +++ b/fastpay/src/bench.rs @@ -214,7 +214,7 @@ impl ClientServerBenchmark { info!("Received {} responses.", responses.len(),); } else { // Use actual client core - let mut client = network::Client::new( + let client = network::Client::new( self.protocol, self.host.clone(), self.port, diff --git a/fastpay/src/network.rs b/fastpay/src/network.rs index 01f464be7b26f..78282ddf7f3b7 100644 --- a/fastpay/src/network.rs +++ b/fastpay/src/network.rs @@ -172,7 +172,7 @@ impl Client { } async fn send_recv_bytes_internal( - &mut self, + &self, shard: ShardId, buf: Vec, ) -> Result, io::Error> { @@ -188,7 +188,7 @@ impl Client { } pub async fn send_recv_bytes( - &mut self, + &self, shard: ShardId, buf: Vec, ) -> Result { @@ -233,7 +233,7 @@ impl AuthorityClient for Client { /// Handle information requests for this account. fn handle_account_info_request( - &mut self, + &self, request: AccountInfoRequest, ) -> AsyncResult<'_, AccountInfoResponse, FastPayError> { Box::pin(async move { diff --git a/fastpay_core/src/client.rs b/fastpay_core/src/client.rs index b89e1be7754ad..cc475089ce3cf 100644 --- a/fastpay_core/src/client.rs +++ b/fastpay_core/src/client.rs @@ -28,7 +28,7 @@ pub trait AuthorityClient { /// Handle information requests for this account. fn handle_account_info_request( - &mut self, + &self, request: AccountInfoRequest, ) -> AsyncResult<'_, AccountInfoResponse, FastPayError>; } @@ -288,9 +288,9 @@ where request_sequence_number: None, request_received_transfers_excluding_first_nth: None, }; - let mut authority_clients = self.authority_clients.clone(); + let authority_clients = self.authority_clients.clone(); let numbers: futures::stream::FuturesUnordered<_> = authority_clients - .iter_mut() + .iter() .map(|(name, client)| { let fut = client.handle_account_info_request(request.clone()); async move { diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index 04a03162076dc..8d20e88947b31 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -30,7 +30,7 @@ impl AuthorityClient for LocalAuthorityClient { } fn handle_account_info_request( - &mut self, + &self, request: AccountInfoRequest, ) -> AsyncResult<'_, AccountInfoResponse, FastPayError> { let state = self.0.clone(); @@ -115,14 +115,10 @@ fn fund_account>>( address: FastPayAddress, object_ids: I, ) { - let mut object_ids = object_ids.into_iter(); - for client in clients.clone() { - let addr = address; - let object_ids: Vec = object_ids.next().unwrap_or(Vec::new()); - + for (client, object_ids) in clients.into_iter().zip(object_ids.into_iter()) { for object_id in object_ids { let mut object = Object::with_id_for_testing(object_id); - object.transfer(addr); + object.transfer(address); client .0 .as_ref() From a26b218c1d4072778f9db559d0b8fb5298e59470 Mon Sep 17 00:00:00 2001 From: Patrick Kuo Date: Fri, 17 Dec 2021 10:23:33 +0000 Subject: [PATCH 8/9] Update fastpay_core/src/unit_tests/client_tests.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com> --- fastpay_core/src/unit_tests/client_tests.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index 8d20e88947b31..38f27e4a500e2 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -119,21 +119,9 @@ fn fund_account>>( for object_id in object_ids { let mut object = Object::with_id_for_testing(object_id); object.transfer(address); - client - .0 - .as_ref() - .try_lock() - .unwrap() - .accounts_mut() - .insert(object_id, object); - - client - .0 - .as_ref() - .try_lock() - .unwrap() - .init_order_lock((object_id, 0.into())); - } + let mut client_ref = client.0.as_ref().try_lock().unwrap(); + client_ref.accounts_mut().insert(object_id, object); + client_ref.init_order_lock((object_id, 0.into())); } } From 82c1855fbb96db0cf90df40ae769a291134395f7 Mon Sep 17 00:00:00 2001 From: patrick Date: Fri, 17 Dec 2021 10:29:12 +0000 Subject: [PATCH 9/9] minor fix --- fastpay_core/src/unit_tests/client_tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/fastpay_core/src/unit_tests/client_tests.rs b/fastpay_core/src/unit_tests/client_tests.rs index 38f27e4a500e2..dd5dd3560c290 100644 --- a/fastpay_core/src/unit_tests/client_tests.rs +++ b/fastpay_core/src/unit_tests/client_tests.rs @@ -122,6 +122,7 @@ fn fund_account>>( let mut client_ref = client.0.as_ref().try_lock().unwrap(); client_ref.accounts_mut().insert(object_id, object); client_ref.init_order_lock((object_id, 0.into())); + } } }