Skip to content

Commit

Permalink
Various fixes assert_eq, unnecessary path prefix, duplicated code, re…
Browse files Browse the repository at this point in the history
…place deprecated functions, lift return of match, apply same order member with trait etc
  • Loading branch information
kchalkias committed Oct 7, 2022
1 parent 04ac466 commit 44436ed
Show file tree
Hide file tree
Showing 81 changed files with 264 additions and 240 deletions.
1 change: 1 addition & 0 deletions crates/sui-adapter/src/in_memory_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ impl ParentSync for InMemoryStorage {
impl ModuleResolver for InMemoryStorage {
type Error = SuiError;

// TODO: duplicated code with ModuleResolver for SuiDataStore in authority_store.rs.
fn get_module(&self, module_id: &ModuleId) -> Result<Option<Vec<u8>>, Self::Error> {
Ok(self
.get_package(&ObjectID::from(*module_id.address()))?
Expand Down
7 changes: 5 additions & 2 deletions crates/sui-adapter/src/temporary_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,11 @@ impl<S> ResourceResolver for TemporaryStore<S> {

match &object.data {
Data::Move(m) => {
assert!(struct_tag == &m.type_, "Invariant violation: ill-typed object in storage or bad object request from caller\
");
assert_eq!(
struct_tag, &m.type_,
"Invariant violation: ill-typed object in storage \
or bad object request from caller"
);
Ok(Some(m.contents().to_vec()))
}
other => unimplemented!(
Expand Down
50 changes: 34 additions & 16 deletions crates/sui-adapter/src/unit_tests/bytecode_rewriter_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,20 +81,26 @@ fn add_address_and_identifier() {
assert!(!m.address_identifiers.contains(addr));

let addr_idx = ModuleHandleRewriter::get_or_create_address(addr, &mut m);
assert!(m.address_identifiers.len() == old_addrs_len + 1);
assert!(m.address_identifier_at(addr_idx) == addr);
assert_eq!(m.address_identifiers.len(), old_addrs_len + 1);
assert_eq!(m.address_identifier_at(addr_idx), addr);
// after addition, should look up existing index instead of adding a new one
assert!(ModuleHandleRewriter::get_or_create_address(addr, &mut m) == addr_idx);
assert_eq!(
ModuleHandleRewriter::get_or_create_address(addr, &mut m),
addr_idx
);

// add identifier
let old_ids_len = m.identifiers.len();
assert!(!m.identifiers.contains(&name.to_owned()));

let id_idx = ModuleHandleRewriter::get_or_create_identifier(name, &mut m);
assert!(m.identifiers.len() == old_ids_len + 1);
assert!(m.identifier_at(id_idx) == name);
assert_eq!(m.identifiers.len(), old_ids_len + 1);
assert_eq!(m.identifier_at(id_idx), name);
// after addition, should look up existing index instead of adding a new one
assert!(ModuleHandleRewriter::get_or_create_identifier(name, &mut m) == id_idx);
assert_eq!(
ModuleHandleRewriter::get_or_create_identifier(name, &mut m),
id_idx
);
}

// Check enforcement of the internal "sub map domain and range are disjoint" invariant
Expand Down Expand Up @@ -150,8 +156,11 @@ fn sub_friend_only() {
};
rewriter.sub_module_ids(&mut m);

assert!(m.address_identifier_at(m.friend_decls[0].address) == id2.address());
assert!(m.identifier_at(m.friend_decls[0].name) == id2.name());
assert_eq!(
m.address_identifier_at(m.friend_decls[0].address),
id2.address()
);
assert_eq!(m.identifier_at(m.friend_decls[0].name), id2.name());
}

// substitution where the new ID does not yet exist in the module table
Expand Down Expand Up @@ -189,16 +198,25 @@ fn sub_non_existing() {
};
rewriter.sub_module_ids(&mut m);
// module handles and friends tables should not change in size
assert!(m.module_handles.len() == old_handles_len);
assert!(m.friend_decls.len() == old_friends_len);
assert_eq!(m.module_handles.len(), old_handles_len);
assert_eq!(m.friend_decls.len(), old_friends_len);
// substituted handles and friends should have new id's
assert!(m.module_id_for_handle(m.module_handle_at(old_idx1)) == new_id);
assert!(m.address_identifier_at(m.friend_decls[1].address) == new_id.address());
assert!(m.identifier_at(m.friend_decls[1].name) == new_id.name());
assert_eq!(m.module_id_for_handle(m.module_handle_at(old_idx1)), new_id);
assert_eq!(
m.address_identifier_at(m.friend_decls[1].address),
new_id.address()
);
assert_eq!(m.identifier_at(m.friend_decls[1].name), new_id.name());
// unrelated handles and friends should not have changed
assert!(m.module_id_for_handle(m.module_handle_at(old_idx2)) == old_id2);
assert!(m.address_identifier_at(m.friend_decls[0].address) == old_id2.address());
assert!(m.identifier_at(m.friend_decls[0].name) == old_id2.name());
assert_eq!(
m.module_id_for_handle(m.module_handle_at(old_idx2)),
old_id2
);
assert_eq!(
m.address_identifier_at(m.friend_decls[0].address),
old_id2.address()
);
assert_eq!(m.identifier_at(m.friend_decls[0].name), old_id2.name());
}

// Substitution between two module ID's that already exist in the module table.
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-benchmark/src/bin/stress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ async fn main() -> Result<()> {
})?;
let config: GatewayConfig = PersistedConfig::read(&config_path)?;
let committee = GatewayState::make_committee(&config)?;
let registry = prometheus::Registry::new();
let registry = Registry::new();
let authority_clients = GatewayState::make_authority_clients(
&config,
NetworkAuthorityClientMetrics::new(&registry),
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-config/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ impl<R> ConfigBuilder<R> {
self
}

pub fn rng<N: ::rand::RngCore + ::rand::CryptoRng>(self, rng: N) -> ConfigBuilder<N> {
pub fn rng<N: rand::RngCore + rand::CryptoRng>(self, rng: N) -> ConfigBuilder<N> {
ConfigBuilder {
rng: Some(rng),
config_directory: self.config_directory,
Expand All @@ -106,7 +106,7 @@ impl<R> ConfigBuilder<R> {
}
}

impl<R: ::rand::RngCore + ::rand::CryptoRng> ConfigBuilder<R> {
impl<R: rand::RngCore + rand::CryptoRng> ConfigBuilder<R> {
//TODO right now we always randomize ports, we may want to have a default port configuration
pub fn build(mut self) -> NetworkConfig {
let committee = self.committee.take().unwrap();
Expand Down
6 changes: 3 additions & 3 deletions crates/sui-config/src/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,11 +393,11 @@ impl Builder {
let path = path.as_ref();
trace!("Writing Genesis Builder to {}", path.display());

std::fs::create_dir_all(path)?;
fs::create_dir_all(path)?;

// Write Objects
let object_dir = path.join(GENESIS_BUILDER_OBJECT_DIR);
std::fs::create_dir_all(&object_dir)?;
fs::create_dir_all(&object_dir)?;

for (_id, object) in self.objects {
let object_bytes = serde_yaml::to_vec(&object)?;
Expand All @@ -407,7 +407,7 @@ impl Builder {

// Write validator infos
let committee_dir = path.join(GENESIS_BUILDER_COMMITTEE_DIR);
std::fs::create_dir_all(&committee_dir)?;
fs::create_dir_all(&committee_dir)?;

for (_pubkey, validator) in self.validators {
let validator_info_bytes = serde_yaml::to_vec(&validator)?;
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-config/src/genesis_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub struct GenesisConfig {
impl Config for GenesisConfig {}

impl GenesisConfig {
pub fn generate_accounts<R: ::rand::RngCore + ::rand::CryptoRng>(
pub fn generate_accounts<R: rand::RngCore + rand::CryptoRng>(
&self,
mut rng: R,
) -> Result<(Vec<AccountKeyPair>, Vec<Object>)> {
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ pub fn sui_config_dir() -> Result<PathBuf, anyhow::Error> {
}
.and_then(|dir| {
if !dir.exists() {
std::fs::create_dir_all(dir.clone())?;
fs::create_dir_all(dir.clone())?;
}
Ok(dir)
})
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-config/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub fn get_available_port() -> u16 {
panic!("Error: could not find an available port");
}

fn get_ephemeral_port() -> ::std::io::Result<u16> {
fn get_ephemeral_port() -> std::io::Result<u16> {
// Request a random available port from the OS
let listener = TcpListener::bind(("localhost", 0))?;
let addr = listener.local_addr()?;
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1401,7 +1401,7 @@ impl AuthorityState {

// Continually pop in-progress txes from the WAL and try to drive them to completion.
pub async fn process_tx_recovery_log(&self, limit: Option<usize>) -> SuiResult {
let mut limit = limit.unwrap_or(usize::max_value());
let mut limit = limit.unwrap_or(usize::MAX);
while limit > 0 {
limit -= 1;
if let Some((cert, tx_guard)) = self.database.wal.read_one_recoverable_tx().await? {
Expand Down
16 changes: 5 additions & 11 deletions crates/sui-core/src/authority/authority_notifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,7 @@ impl TransactionNotifier {
));
} else {
// If the notifier is closed, then exit
if transaction_notifier
.is_closed
.load(std::sync::atomic::Ordering::SeqCst)
{
if transaction_notifier.is_closed.load(Ordering::SeqCst) {
return None;
}
}
Expand All @@ -185,8 +182,7 @@ impl TransactionNotifier {

/// Signal we want to close this channel.
pub fn close(&self) {
self.is_closed
.store(true, std::sync::atomic::Ordering::SeqCst);
self.is_closed.store(true, Ordering::SeqCst);
self.notify.notify_one();
}
}
Expand All @@ -195,9 +191,7 @@ struct IterUniquenessGuard(Arc<TransactionNotifier>);

impl Drop for IterUniquenessGuard {
fn drop(&mut self) {
self.0
.has_stream
.store(false, std::sync::atomic::Ordering::SeqCst);
self.0.has_stream.store(false, Ordering::SeqCst);
}
}

Expand Down Expand Up @@ -225,7 +219,7 @@ impl TransactionNotifierTicket {

self.transaction_notifier
.low_watermark
.store(new_low_watermark, std::sync::atomic::Ordering::SeqCst);
.store(new_low_watermark, Ordering::SeqCst);
self.transaction_notifier.notify.notify_one();
}
}
Expand Down Expand Up @@ -291,7 +285,7 @@ mod tests {

{
let t0 = notifier.ticket().expect("ok");
assert!(t0.seq() == 3);
assert_eq!(t0.seq(), 3);
t0.notify();
}

Expand Down
17 changes: 8 additions & 9 deletions crates/sui-core/src/authority/authority_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
}

/// A function that acquires all locks associated with the objects (in order to avoid deadlocks).
async fn acquire_locks<'a, 'b>(&'a self, input_objects: &'b [ObjectRef]) -> Vec<LockGuard> {
async fn acquire_locks(&self, input_objects: &[ObjectRef]) -> Vec<LockGuard> {
self.mutex_table
.acquire_locks(input_objects.iter().map(|(_, _, digest)| *digest))
.await
Expand Down Expand Up @@ -652,7 +652,7 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
let transaction_digest: &TransactionDigest = certificate.digest();
write_batch = write_batch.insert_batch(
&self.tables.certificates,
std::iter::once((transaction_digest, certificate)),
iter::once((transaction_digest, certificate)),
)?;

self.sequence_tx(
Expand Down Expand Up @@ -719,7 +719,7 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
// Store the certificate indexed by transaction digest
write_batch = write_batch.insert_batch(
&self.tables.certificates,
std::iter::once((transaction_digest, &certificate)),
iter::once((transaction_digest, &certificate)),
)?;
self.sequence_tx(
write_batch,
Expand Down Expand Up @@ -870,10 +870,8 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
// Once a transaction is done processing and effects committed, we no longer
// need it in the transactions table. This also allows us to track pending
// transactions.
write_batch = write_batch.delete_batch(
&self.tables.transactions,
std::iter::once(transaction_digest),
)?;
write_batch =
write_batch.delete_batch(&self.tables.transactions, iter::once(transaction_digest))?;

// Update the indexes of the objects written
write_batch = write_batch.insert_batch(
Expand Down Expand Up @@ -1165,7 +1163,7 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
"locking shared objects");

// Make an iterator to update the last consensus index.
let index_to_write = std::iter::once((LAST_CONSENSUS_INDEX_ADDR, consensus_index));
let index_to_write = iter::once((LAST_CONSENSUS_INDEX_ADDR, consensus_index));

// Holding _tx_lock avoids the following race:
// - we check effects_exist, returns false
Expand Down Expand Up @@ -1212,7 +1210,7 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
write_batch.insert_batch(&self.tables.last_consensus_index, index_to_write)?;
write_batch = write_batch.insert_batch(
&self.tables.consensus_message_processed,
std::iter::once((transaction_digest, true)),
iter::once((transaction_digest, true)),
)?;
write_batch.write().map_err(SuiError::from)
}
Expand Down Expand Up @@ -1431,6 +1429,7 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> ParentSync for SuiDa
impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> ModuleResolver for SuiDataStore<S> {
type Error = SuiError;

// TODO: duplicated code with ModuleResolver for InMemoryStorage in memory_storage.rs.
fn get_module(&self, module_id: &ModuleId) -> Result<Option<Vec<u8>>, Self::Error> {
// TODO: We should cache the deserialized modules to avoid
// fetching from the store / re-deserializing them everytime.
Expand Down
10 changes: 2 additions & 8 deletions crates/sui-core/src/authority_active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ impl AuthorityHealth {
}
}

struct NodeSyncProcessHandle(tokio::task::JoinHandle<()>, oneshot::Sender<()>);
struct NodeSyncProcessHandle(JoinHandle<()>, oneshot::Sender<()>);

pub struct ActiveAuthority<A> {
// The local authority state
Expand Down Expand Up @@ -202,13 +202,7 @@ impl<A> ActiveAuthority<A> {

let health_overview: Vec<_> = lock
.iter()
.map(|(name, h)| {
(
*name,
h.retries,
h.no_contact_before - tokio::time::Instant::now(),
)
})
.map(|(name, h)| (*name, h.retries, h.no_contact_before - Instant::now()))
.collect();
debug!(health_overview = ?health_overview, "Current validator health metrics");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ async fn checkpoint_active_flow_crash_client_with_gossip() {
}

let sender_aggregator = aggregator.clone();
// TODO: duplicated code in the same file `_end_of_sending_join`
let _end_of_sending_join = tokio::task::spawn(async move {
while let Some(t) = transactions.pop() {
// Get a cert
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ async fn pending_exec_storage_notify() {

let authority_state = authorities[0].authority.clone();

// TODO: duplicated with checkpoint_driver/tests.rs
// Start active part of authority.
for inner_state in authorities.clone() {
let inner_agg = aggregator.clone();
Expand Down
1 change: 1 addition & 0 deletions crates/sui-core/src/authority_active/gossip/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub async fn test_gossip_plain() {
let _active_authorities = start_gossip_process(states.clone(), net.clone()).await;
tokio::time::sleep(Duration::from_secs(20)).await;

// TODO: duplicated code in this file `test_gossip_error`
// Expected outcome of gossip: each digest's tx signature and cert is now on every authority.
for client in net.clone_inner_clients().values() {
for digest in &digests {
Expand Down
Loading

0 comments on commit 44436ed

Please sign in to comment.