Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove address_to_object_id_hack part 2 #29

Merged
merged 20 commits into from
Dec 8, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions fastpay/src/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,22 +124,22 @@ impl ClientServerBenchmark {
let mut account_keys = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change, account_keys probably should be renamed to something like acount_objects as it contains both account and object id.

for _ in 0..self.num_accounts {
let keypair = get_key_pair();
let object_id: ObjectID = address_to_object_id_hack(keypair.0);
let object_id: ObjectID = ObjectID::random();
let i = AuthorityState::get_shard(self.num_shards, &object_id) as usize;
assert!(states[i].in_shard(&object_id));
let mut client = Object::with_id_for_testing(object_id);
client.transfer(keypair.0);
states[i].insert_object(client);
account_keys.push(keypair);
account_keys.push((keypair.0, object_id, keypair.1));
}

info!("Preparing transactions.");
// Make one transaction per account (transfer order + confirmation).
let mut orders: Vec<(u32, Bytes)> = Vec::new();
let mut next_recipient = get_key_pair().0;
for (pubx, secx) in account_keys.iter() {
for (pubx, object_id, secx) in account_keys.iter() {
let transfer = Transfer {
object_id: address_to_object_id_hack(*pubx),
object_id: *object_id,
sender: *pubx,
recipient: Address::FastPay(next_recipient),
sequence_number: SequenceNumber::from(0),
Expand Down
67 changes: 42 additions & 25 deletions fastpay/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,22 +84,23 @@ fn make_client_state(
account.next_sequence_number,
account.sent_certificates.clone(),
account.received_certificates.clone(),
account.balance,
account.object_ids.clone(),
)
}

/// Make one transfer order per account, up to `max_orders` transfers.
fn make_benchmark_transfer_orders(
accounts_config: &mut AccountsConfig,
max_orders: usize,
) -> (Vec<Order>, Vec<(FastPayAddress, Bytes)>) {
) -> (Vec<Order>, Vec<(FastPayAddress, ObjectID, Bytes)>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need both FastPayAddress and ObjectID. FastPayAddress should be no longer needed as we just need ObjectID for sharding. Similarly for other make_benchmark_ methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the address is used later on as the recipient's address, we use ObjectID for sharing but we still need Address for the sender and recipient during transfer.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm I don't see it though. Can you point me to the code that uses the address from the serialized_orders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh you are right, address is not needed here, I was confused with the address_keys in bench.rs, thanks!

let mut orders = Vec::new();
let mut serialized_orders = Vec::new();
// TODO: deterministic sequence of orders to recover from interrupted benchmarks.
let mut next_recipient = get_key_pair().0;
for account in accounts_config.accounts_mut() {
let object_id = *account.object_ids.first().unwrap();
let transfer = Transfer {
object_id: address_to_object_id_hack(account.address),
object_id,
sender: account.address,
recipient: Address::FastPay(next_recipient),
sequence_number: account.next_sequence_number,
Expand All @@ -111,7 +112,7 @@ fn make_benchmark_transfer_orders(
let order = Order::new_transfer(transfer.clone(), &account.key);
orders.push(order.clone());
let serialized_order = serialize_order(&order);
serialized_orders.push((account.address, serialized_order.into()));
serialized_orders.push((account.address, object_id, serialized_order.into()));
if serialized_orders.len() >= max_orders {
break;
}
Expand All @@ -123,7 +124,7 @@ fn make_benchmark_transfer_orders(
fn make_benchmark_certificates_from_orders_and_server_configs(
orders: Vec<Order>,
server_config: Vec<&str>,
) -> Vec<(FastPayAddress, Bytes)> {
) -> Vec<(FastPayAddress, ObjectID, Bytes)> {
let mut keys = Vec::new();
for file in server_config {
let server_config = AuthorityServerConfig::read(file).expect("Fail to read server config");
Expand All @@ -149,7 +150,11 @@ fn make_benchmark_certificates_from_orders_and_server_configs(
certificate.signatures.push((*pubx, sig));
}
let serialized_certificate = serialize_cert(&certificate);
serialized_certificates.push((*order.sender(), serialized_certificate.into()));
serialized_certificates.push((
*order.sender(),
*order.object_id(),
serialized_certificate.into(),
));
}
serialized_certificates
}
Expand All @@ -158,14 +163,15 @@ fn make_benchmark_certificates_from_orders_and_server_configs(
fn make_benchmark_certificates_from_votes(
committee_config: &CommitteeConfig,
votes: Vec<SignedOrder>,
) -> Vec<(FastPayAddress, Bytes)> {
) -> Vec<(FastPayAddress, ObjectID, Bytes)> {
let committee = Committee::new(committee_config.voting_rights());
let mut aggregators = HashMap::new();
let mut certificates = Vec::new();
let mut done_senders = HashSet::new();
for vote in votes {
// We aggregate votes indexed by sender.
let address = *vote.order.sender();
let object_id = *vote.order.object_id();
if done_senders.contains(&address) {
continue;
}
Expand All @@ -182,7 +188,7 @@ fn make_benchmark_certificates_from_votes(
Ok(Some(certificate)) => {
debug!("Found certificate: {:?}", certificate);
let buf = serialize_cert(&certificate);
certificates.push((address, buf.into()));
certificates.push((address, object_id, buf.into()));
done_senders.insert(address);
}
Ok(None) => {
Expand All @@ -204,7 +210,7 @@ async fn mass_broadcast_orders(
send_timeout: std::time::Duration,
recv_timeout: std::time::Duration,
max_in_flight: u64,
orders: Vec<(FastPayAddress, Bytes)>,
orders: Vec<(FastPayAddress, ObjectID, Bytes)>,
) -> Vec<Bytes> {
let time_start = Instant::now();
info!("Broadcasting {} {} orders", orders.len(), phase);
Expand All @@ -219,10 +225,8 @@ async fn mass_broadcast_orders(
for (num_shards, client) in authority_clients {
// Re-index orders by shard for this particular authority client.
let mut sharded_requests = HashMap::new();
for (address, buf) in &orders {
// TODO: fix this
let id: ObjectID = address_to_object_id_hack(*address);
let shard = AuthorityState::get_shard(num_shards, &id);
for (_, object_id, buf) in &orders {
let shard = AuthorityState::get_shard(num_shards, object_id);
sharded_requests
.entry(shard)
.or_insert_with(Vec::new)
Expand All @@ -247,9 +251,9 @@ async fn mass_broadcast_orders(

fn mass_update_recipients(
accounts_config: &mut AccountsConfig,
certificates: Vec<(FastPayAddress, Bytes)>,
certificates: Vec<(FastPayAddress, ObjectID, Bytes)>,
) {
for (_sender, buf) in certificates {
for (_sender, _object_id, buf) in certificates {
if let Ok(SerializedMessage::Cert(certificate)) = deserialize_message(&buf[..]) {
accounts_config.update_for_received_transfer(*certificate);
}
Expand Down Expand Up @@ -321,8 +325,8 @@ enum ClientCommands {
#[structopt(long)]
to: String,

/// Amount to transfer
amount: u64,
/// Object to transfer, in 20 bytes Hex string
object_id: String,
},

/// Obtain the spendable balance
Expand Down Expand Up @@ -352,8 +356,8 @@ enum ClientCommands {
#[structopt(name = "create_accounts")]
CreateAccounts {
/// known initial balance of the account
#[structopt(long, default_value = "0")]
initial_funding: Balance,
#[structopt(long)]
initial_objects: Option<Vec<String>>,

/// Number of additional accounts to create
num: u32,
Expand All @@ -376,10 +380,14 @@ fn main() {
CommitteeConfig::read(committee_config_path).expect("Unable to read committee config file");

match options.cmd {
ClientCommands::Transfer { from, to, amount } => {
ClientCommands::Transfer {
from,
to,
object_id,
} => {
let sender = decode_address(&from).expect("Failed to decode sender's address");
let recipient = decode_address(&to).expect("Failed to decode recipient's address");
let amount = Amount::from(amount);
let object_id = ObjectID::from_hex_literal(&object_id).unwrap();

let mut rt = Runtime::new().unwrap();
rt.block_on(async move {
Expand All @@ -394,7 +402,7 @@ fn main() {
info!("Starting transfer");
let time_start = Instant::now();
let cert = client_state
.transfer_to_fastpay(amount, recipient, UserData::default())
.transfer_to_fastpay(object_id, recipient, UserData::default())
.await
.unwrap();
let time_total = time_start.elapsed().as_micros();
Expand Down Expand Up @@ -527,13 +535,22 @@ fn main() {
}

ClientCommands::CreateAccounts {
initial_funding,
initial_objects,
num,
} => {
let num_accounts: u32 = num;
let object_ids = match initial_objects {
Some(object_ids) => object_ids
.into_iter()
.map(|string| ObjectID::from_hex_literal(&string).unwrap())
.collect(),

None => Vec::new(),
};

for _ in 0..num_accounts {
let account = UserAccount::new(initial_funding);
println!("{}:{}", encode_address(&account.address), initial_funding);
let account = UserAccount::new(object_ids.clone());
println!("{}:{:?}", encode_address(&account.address), object_ids);
accounts_config.insert(account);
}
accounts_config
Expand Down
16 changes: 8 additions & 8 deletions fastpay/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,19 @@ pub struct UserAccount {
pub address: FastPayAddress,
pub key: KeyPair,
pub next_sequence_number: SequenceNumber,
pub balance: Balance,
pub object_ids: Vec<ObjectID>,
pub sent_certificates: Vec<CertifiedOrder>,
pub received_certificates: Vec<CertifiedOrder>,
}

impl UserAccount {
pub fn new(balance: Balance) -> Self {
pub fn new(object_ids: Vec<ObjectID>) -> Self {
let (address, key) = get_key_pair();
Self {
address,
key,
next_sequence_number: SequenceNumber::new(),
balance,
object_ids,
sent_certificates: Vec::new(),
received_certificates: Vec::new(),
}
Expand Down Expand Up @@ -145,7 +145,7 @@ impl AccountsConfig {
.get_mut(&state.address())
.expect("Updated account should already exist");
account.next_sequence_number = state.next_sequence_number();
account.balance = state.balance();
account.object_ids = state.object_ids().clone();
account.sent_certificates = state.sent_certificates().clone();
account.received_certificates = state.received_certificates().cloned().collect();
}
Expand Down Expand Up @@ -198,7 +198,7 @@ impl AccountsConfig {
}

pub struct InitialStateConfig {
pub accounts: Vec<(FastPayAddress, Balance)>,
pub accounts: Vec<(FastPayAddress, ObjectID)>,
}

impl InitialStateConfig {
Expand All @@ -213,7 +213,7 @@ impl InitialStateConfig {
failure::bail!("expecting two columns separated with ':'")
}
let address = decode_address(elements[0])?;
let balance = elements[1].parse()?;
let balance = ObjectID::from_hex_literal(elements[1])?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be called balance

accounts.push((address, balance));
}
Ok(Self { accounts })
Expand All @@ -222,8 +222,8 @@ impl InitialStateConfig {
pub fn write(&self, path: &str) -> Result<(), std::io::Error> {
let file = OpenOptions::new().create(true).write(true).open(path)?;
let mut writer = BufWriter::new(file);
for (address, balance) in &self.accounts {
writeln!(writer, "{}:{}", encode_address(address), balance)?;
for (address, object_id) in &self.accounts {
writeln!(writer, "{}:{}", encode_address(address), object_id,)?;
}
Ok(())
}
Expand Down
9 changes: 3 additions & 6 deletions fastpay/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,12 @@ fn make_shard_server(
);

// Load initial states
for (address, _balance) in &initial_accounts_config.accounts {
// TODO: fix this total hack
let id: ObjectID = address_to_object_id_hack(*address);

if AuthorityState::get_shard(num_shards, &id) != shard {
for (address, object_id) in &initial_accounts_config.accounts {
if AuthorityState::get_shard(num_shards, object_id) != shard {
continue;
}

let mut client = Object::with_id_for_testing(id);
let mut client = Object::with_id_for_testing(*object_id);
client.transfer(*address);
state.insert_object(client);
}
Expand Down
17 changes: 1 addition & 16 deletions fastpay_core/src/base_types.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
// Copyright (c) Facebook, Inc. and its affiliates.
// SPDX-License-Identifier: Apache-2.0

use crate::error::FastPayError;
use std::convert::{TryFrom, TryInto};

use ed25519_dalek as dalek;
use ed25519_dalek::{Signer, Verifier};
use move_core_types::account_address::AccountAddress;
use rand::rngs::OsRng;
#[cfg(test)]
use rand::Rng;
use serde::{Deserialize, Serialize};

use crate::error::FastPayError;

#[cfg(test)]
#[path = "unit_tests/base_types_tests.rs"]
mod base_types_tests;
Expand Down Expand Up @@ -53,17 +49,6 @@ pub type AuthorityName = PublicKeyBytes;
pub type ObjectID = AccountAddress;
pub type ObjectRef = (ObjectID, SequenceNumber);

pub fn address_to_object_id_hack(address: FastPayAddress) -> ObjectID {
address.0[0..ObjectID::LENGTH]
.try_into()
.expect("An address is always long enough to extract 16 bytes")
}

#[cfg(test)]
pub fn get_object_id() -> ObjectID {
ObjectID::new(rand::thread_rng().gen::<[u8; ObjectID::LENGTH]>())
}

pub fn get_key_pair() -> (FastPayAddress, KeyPair) {
let mut csprng = OsRng;
let keypair = dalek::Keypair::generate(&mut csprng);
Expand Down
Loading