[execution] change HashMap to BTreeMap in VMChangeSet (aptos-labs#10582)

* fix

* lints + tests

aptos-move/aptos-vm-types/src/change_set.rs

@@ -13,8 +13,8 @@ use aptos_types::{
 use move_binary_format::errors::Location;
 use move_core_types::vm_status::{err_msg, StatusCode, VMStatus};
 use std::collections::{
-    hash_map::Entry::{Occupied, Vacant},
-    HashMap,
+    btree_map::Entry::{Occupied, Vacant},
+    BTreeMap,
 };
 
 /// A change set produced by the VM.
@@ -23,10 +23,10 @@ use std::collections::{
 /// VM. For storage backends, use `ChangeSet`.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct VMChangeSet {
-    resource_write_set: HashMap<StateKey, WriteOp>,
-    module_write_set: HashMap<StateKey, WriteOp>,
-    aggregator_write_set: HashMap<StateKey, WriteOp>,
-    aggregator_delta_set: HashMap<StateKey, DeltaOp>,
+    resource_write_set: BTreeMap<StateKey, WriteOp>,
+    module_write_set: BTreeMap<StateKey, WriteOp>,
+    aggregator_write_set: BTreeMap<StateKey, WriteOp>,
+    aggregator_delta_set: BTreeMap<StateKey, DeltaOp>,
     events: Vec<ContractEvent>,
 }
 
@@ -49,19 +49,19 @@ macro_rules! squash_writes_pair {
 impl VMChangeSet {
     pub fn empty() -> Self {
         Self {
-            resource_write_set: HashMap::new(),
-            module_write_set: HashMap::new(),
-            aggregator_write_set: HashMap::new(),
-            aggregator_delta_set: HashMap::new(),
+            resource_write_set: BTreeMap::new(),
+            module_write_set: BTreeMap::new(),
+            aggregator_write_set: BTreeMap::new(),
+            aggregator_delta_set: BTreeMap::new(),
             events: vec![],
         }
     }
 
     pub fn new(
-        resource_write_set: HashMap<StateKey, WriteOp>,
-        module_write_set: HashMap<StateKey, WriteOp>,
-        aggregator_write_set: HashMap<StateKey, WriteOp>,
-        aggregator_delta_set: HashMap<StateKey, DeltaOp>,
+        resource_write_set: BTreeMap<StateKey, WriteOp>,
+        module_write_set: BTreeMap<StateKey, WriteOp>,
+        aggregator_write_set: BTreeMap<StateKey, WriteOp>,
+        aggregator_delta_set: BTreeMap<StateKey, DeltaOp>,
         events: Vec<ContractEvent>,
         checker: &dyn CheckChangeSet,
     ) -> anyhow::Result<Self, VMStatus> {
@@ -92,8 +92,8 @@ impl VMChangeSet {
 
         // There should be no aggregator writes if we have a change set from
         // storage.
-        let mut resource_write_set = HashMap::new();
-        let mut module_write_set = HashMap::new();
+        let mut resource_write_set = BTreeMap::new();
+        let mut module_write_set = BTreeMap::new();
 
         for (state_key, write_op) in write_set {
             if matches!(state_key.inner(), StateKeyInner::AccessPath(ap) if ap.is_code()) {
@@ -109,8 +109,8 @@ impl VMChangeSet {
         let change_set = Self {
             resource_write_set,
             module_write_set,
-            aggregator_write_set: HashMap::new(),
-            aggregator_delta_set: HashMap::new(),
+            aggregator_write_set: BTreeMap::new(),
+            aggregator_delta_set: BTreeMap::new(),
             events,
         };
         checker.check_change_set(&change_set)?;
@@ -172,11 +172,11 @@ impl VMChangeSet {
             .chain(self.aggregator_write_set.iter_mut())
     }
 
-    pub fn resource_write_set(&self) -> &HashMap<StateKey, WriteOp> {
+    pub fn resource_write_set(&self) -> &BTreeMap<StateKey, WriteOp> {
         &self.resource_write_set
     }
 
-    pub fn module_write_set(&self) -> &HashMap<StateKey, WriteOp> {
+    pub fn module_write_set(&self) -> &BTreeMap<StateKey, WriteOp> {
         &self.module_write_set
     }
 
@@ -189,11 +189,11 @@ impl VMChangeSet {
             .extend(additional_aggregator_writes)
     }
 
-    pub fn aggregator_v1_write_set(&self) -> &HashMap<StateKey, WriteOp> {
+    pub fn aggregator_v1_write_set(&self) -> &BTreeMap<StateKey, WriteOp> {
         &self.aggregator_write_set
     }
 
-    pub fn aggregator_v1_delta_set(&self) -> &HashMap<StateKey, DeltaOp> {
+    pub fn aggregator_v1_delta_set(&self) -> &BTreeMap<StateKey, DeltaOp> {
         &self.aggregator_delta_set
     }
 
@@ -222,23 +222,23 @@ impl VMChangeSet {
             aggregator_delta_set
                 .into_iter()
                 .map(into_write)
-                .collect::<anyhow::Result<HashMap<StateKey, WriteOp>, VMStatus>>()?;
+                .collect::<anyhow::Result<BTreeMap<StateKey, WriteOp>, VMStatus>>()?;
         aggregator_write_set.extend(materialized_aggregator_delta_set.into_iter());
 
         Ok(Self {
             resource_write_set,
             module_write_set,
             aggregator_write_set,
-            aggregator_delta_set: HashMap::new(),
+            aggregator_delta_set: BTreeMap::new(),
             events,
         })
     }
 
     fn squash_additional_aggregator_changes(
-        aggregator_write_set: &mut HashMap<StateKey, WriteOp>,
-        aggregator_delta_set: &mut HashMap<StateKey, DeltaOp>,
-        additional_aggregator_write_set: HashMap<StateKey, WriteOp>,
-        additional_aggregator_delta_set: HashMap<StateKey, DeltaOp>,
+        aggregator_write_set: &mut BTreeMap<StateKey, WriteOp>,
+        aggregator_delta_set: &mut BTreeMap<StateKey, DeltaOp>,
+        additional_aggregator_write_set: BTreeMap<StateKey, WriteOp>,
+        additional_aggregator_delta_set: BTreeMap<StateKey, DeltaOp>,
     ) -> anyhow::Result<(), VMStatus> {
         use WriteOp::*;
 
@@ -321,8 +321,8 @@ impl VMChangeSet {
     }
 
     fn squash_additional_writes(
-        write_set: &mut HashMap<StateKey, WriteOp>,
-        additional_write_set: HashMap<StateKey, WriteOp>,
+        write_set: &mut BTreeMap<StateKey, WriteOp>,
+        additional_write_set: BTreeMap<StateKey, WriteOp>,
     ) -> anyhow::Result<(), VMStatus> {
         for (key, additional_write_op) in additional_write_set.into_iter() {
             match write_set.entry(key) {

aptos-move/aptos-vm-types/src/tests/test_change_set.rs

@@ -20,7 +20,7 @@ use move_core_types::{
     language_storage::{ModuleId, StructTag},
     vm_status::{StatusCode, VMStatus},
 };
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 
 /// Testcases:
 /// ```text
@@ -89,7 +89,7 @@ macro_rules! write_set_2 {
 
 macro_rules! expected_write_set {
     ($d:ident) => {
-        HashMap::from([
+        BTreeMap::from([
             mock_create(format!("0{}", $d), 0),
             mock_modify(format!("1{}", $d), 1),
             mock_delete(format!("2{}", $d)),
@@ -164,13 +164,13 @@ fn test_successful_squash() {
         &expected_write_set!(descriptor)
     );
 
-    let expected_aggregator_write_set = HashMap::from([
+    let expected_aggregator_write_set = BTreeMap::from([
         mock_create("18a", 136),
         mock_modify("19a", 138),
         mock_modify("22a", 122),
         mock_delete("23a"),
     ]);
-    let expected_aggregator_delta_set = HashMap::from([
+    let expected_aggregator_delta_set = BTreeMap::from([
         mock_add("15a", 15),
         mock_add("16a", 116),
         mock_add("17a", 134),

aptos-move/aptos-vm-types/src/tests/test_output.rs

@@ -12,7 +12,7 @@ use aptos_types::{
 };
 use claims::{assert_err, assert_matches, assert_ok};
 use move_core_types::vm_status::{AbortLocation, VMStatus};
-use std::collections::{BTreeMap, HashMap};
+use std::collections::BTreeMap;
 
 fn assert_eq_outputs(vm_output: &VMOutput, txn_output: TransactionOutput) {
     let vm_output_writes = &vm_output
@@ -77,7 +77,8 @@ fn test_ok_output_equality_with_deltas() {
         .clone()
         .into_transaction_output_with_materialized_deltas(vec![mock_modify("3", 400)]);
 
-    let expected_aggregator_write_set = HashMap::from([mock_modify("2", 2), mock_modify("3", 400)]);
+    let expected_aggregator_write_set =
+        BTreeMap::from([mock_modify("2", 2), mock_modify("3", 400)]);
     assert_eq!(
         materialized_vm_output.change_set().resource_write_set(),
         vm_output.change_set().resource_write_set()

aptos-move/aptos-vm-types/src/tests/utils.rs

@@ -10,7 +10,7 @@ use aptos_types::{
     write_set::WriteOp,
 };
 use move_core_types::vm_status::VMStatus;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 
 pub(crate) struct MockChangeSetChecker;
 
@@ -57,10 +57,10 @@ pub(crate) fn build_change_set(
     aggregator_delta_set: impl IntoIterator<Item = (StateKey, DeltaOp)>,
 ) -> VMChangeSet {
     VMChangeSet::new(
-        HashMap::from_iter(resource_write_set),
-        HashMap::from_iter(module_write_set),
-        HashMap::from_iter(aggregator_write_set),
-        HashMap::from_iter(aggregator_delta_set),
+        BTreeMap::from_iter(resource_write_set),
+        BTreeMap::from_iter(module_write_set),
+        BTreeMap::from_iter(aggregator_write_set),
+        BTreeMap::from_iter(aggregator_delta_set),
         vec![],
         &MockChangeSetChecker,
     )

aptos-move/aptos-vm/src/block_executor/mod.rs

@@ -38,7 +38,7 @@ use aptos_vm_types::output::VMOutput;
 use move_core_types::vm_status::VMStatus;
 use once_cell::sync::OnceCell;
 use rayon::{prelude::*, ThreadPool};
-use std::{collections::HashMap, sync::Arc};
+use std::{collections::BTreeMap, sync::Arc};
 
 impl BlockExecutorTransaction for PreprocessedTransaction {
     type Event = ContractEvent;
@@ -90,7 +90,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput {
 
     /// Should never be called after incorporate_delta_writes, as it
     /// will consume vm_output to prepare an output with deltas.
-    fn resource_write_set(&self) -> HashMap<StateKey, WriteOp> {
+    fn resource_write_set(&self) -> BTreeMap<StateKey, WriteOp> {
         self.vm_output
             .lock()
             .as_ref()
@@ -102,7 +102,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput {
 
     /// Should never be called after incorporate_delta_writes, as it
     /// will consume vm_output to prepare an output with deltas.
-    fn module_write_set(&self) -> HashMap<StateKey, WriteOp> {
+    fn module_write_set(&self) -> BTreeMap<StateKey, WriteOp> {
         self.vm_output
             .lock()
             .as_ref()
@@ -114,7 +114,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput {
 
     /// Should never be called after incorporate_delta_writes, as it
     /// will consume vm_output to prepare an output with deltas.
-    fn aggregator_v1_write_set(&self) -> HashMap<StateKey, WriteOp> {
+    fn aggregator_v1_write_set(&self) -> BTreeMap<StateKey, WriteOp> {
         self.vm_output
             .lock()
             .as_ref()
@@ -126,7 +126,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput {
 
     /// Should never be called after incorporate_delta_writes, as it
     /// will consume vm_output to prepare an output with deltas.
-    fn aggregator_v1_delta_set(&self) -> HashMap<StateKey, DeltaOp> {
+    fn aggregator_v1_delta_set(&self) -> BTreeMap<StateKey, DeltaOp> {
         self.vm_output
             .lock()
             .as_ref()

aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs

@@ -151,7 +151,7 @@ mod test {
     use aptos_language_e2e_tests::data_store::FakeDataStore;
     use aptos_types::write_set::WriteOp;
     use aptos_vm_types::check_change_set::CheckChangeSet;
-    use std::collections::HashMap;
+    use std::collections::BTreeMap;
 
     /// A mock for testing. Always succeeds on checking a change set.
     struct NoOpChangeSetChecker;
@@ -187,23 +187,23 @@ mod test {
         base_view.set_legacy(key("aggregator_both"), serialize(&60));
         base_view.set_legacy(key("aggregator_delta_set"), serialize(&70));
 
-        let resource_write_set = HashMap::from([
+        let resource_write_set = BTreeMap::from([
             (key("resource_both"), write(80)),
             (key("resource_write_set"), write(90)),
         ]);
 
-        let module_write_set = HashMap::from([
+        let module_write_set = BTreeMap::from([
             (key("module_both"), write(100)),
             (key("module_write_set"), write(110)),
         ]);
 
-        let aggregator_write_set = HashMap::from([
+        let aggregator_write_set = BTreeMap::from([
             (key("aggregator_both"), write(120)),
             (key("aggregator_write_set"), write(130)),
         ]);
 
         let aggregator_delta_set =
-            HashMap::from([(key("aggregator_delta_set"), delta_add(1, 1000))]);
+            BTreeMap::from([(key("aggregator_delta_set"), delta_add(1, 1000))]);
 
         let change_set = VMChangeSet::new(
             resource_write_set,

aptos-move/aptos-vm/src/move_vm_ext/session.rs

@@ -34,7 +34,7 @@ use move_vm_runtime::{move_vm::MoveVM, session::Session};
 use serde::{Deserialize, Serialize};
 use std::{
     borrow::BorrowMut,
-    collections::{BTreeMap, HashMap},
+    collections::BTreeMap,
     ops::{Deref, DerefMut},
     sync::Arc,
 };
@@ -318,10 +318,10 @@ impl<'r, 'l> SessionExt<'r, 'l> {
             new_slot_metadata,
         };
 
-        let mut resource_write_set = HashMap::new();
-        let mut module_write_set = HashMap::new();
-        let mut aggregator_write_set = HashMap::new();
-        let mut aggregator_delta_set = HashMap::new();
+        let mut resource_write_set = BTreeMap::new();
+        let mut module_write_set = BTreeMap::new();
+        let mut aggregator_write_set = BTreeMap::new();
+        let mut aggregator_delta_set = BTreeMap::new();
 
         for (addr, account_changeset) in change_set.into_inner() {
             let (modules, resources) = account_changeset.into_inner();

aptos-move/block-executor/src/proptest_types/types.rs

@@ -25,7 +25,7 @@ use once_cell::sync::OnceCell;
 use proptest::{arbitrary::Arbitrary, collection::vec, prelude::*, proptest, sample::Index};
 use proptest_derive::Arbitrary;
 use std::{
-    collections::{hash_map::DefaultHasher, BTreeSet, HashMap},
+    collections::{hash_map::DefaultHasher, BTreeMap, BTreeSet},
     convert::TryInto,
     fmt::Debug,
     hash::{Hash, Hasher},
@@ -571,15 +571,15 @@ where
 {
     type Txn = MockTransaction<K, V, E>;
 
-    fn resource_write_set(&self) -> HashMap<K, V> {
+    fn resource_write_set(&self) -> BTreeMap<K, V> {
         self.writes
             .iter()
             .filter(|(k, _)| k.module_path().is_none())
             .cloned()
             .collect()
     }
 
-    fn module_write_set(&self) -> HashMap<K, V> {
+    fn module_write_set(&self) -> BTreeMap<K, V> {
         self.writes
             .iter()
             .filter(|(k, _)| k.module_path().is_some())
@@ -589,11 +589,11 @@ where
 
     // Aggregator v1 writes are included in resource_write_set for tests (writes are produced
     // for all keys including ones for v1_aggregators without distinguishing).
-    fn aggregator_v1_write_set(&self) -> HashMap<K, V> {
-        HashMap::new()
+    fn aggregator_v1_write_set(&self) -> BTreeMap<K, V> {
+        BTreeMap::new()
     }
 
-    fn aggregator_v1_delta_set(&self) -> HashMap<K, DeltaOp> {
+    fn aggregator_v1_delta_set(&self) -> BTreeMap<K, DeltaOp> {
         self.deltas.iter().cloned().collect()
     }