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

Decouple AuthorityTemporaryStore and AuthorityStore #801

Merged
merged 1 commit into from
Mar 13, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
15 changes: 8 additions & 7 deletions sui_core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use sui_types::{
fp_bail, fp_ensure, gas,
messages::*,
object::{Data, Object, Owner},
storage::{DeleteKind, Storage},
storage::{BackingPackageStore, DeleteKind, Storage},
MOVE_STDLIB_ADDRESS, SUI_FRAMEWORK_ADDRESS,
};
use tracing::*;
Expand Down Expand Up @@ -484,8 +484,9 @@ impl AuthorityState {
transaction: Transaction,
mut inputs: Vec<Object>,
tx_ctx: &mut TxContext,
) -> SuiResult<(AuthorityTemporaryStore, ExecutionStatus)> {
let mut temporary_store = AuthorityTemporaryStore::new(self, &inputs, tx_ctx.digest());
) -> SuiResult<(AuthorityTemporaryStore<AuthorityStore>, ExecutionStatus)> {
let mut temporary_store =
AuthorityTemporaryStore::new(self._database.clone(), &inputs, tx_ctx.digest());
// unwraps here are safe because we built `inputs`
let mut gas_object = inputs.pop().unwrap();

Expand Down Expand Up @@ -573,7 +574,7 @@ impl AuthorityState {
}

fn transfer(
temporary_store: &mut AuthorityTemporaryStore,
temporary_store: &mut AuthorityTemporaryStore<AuthorityStore>,
mut inputs: Vec<Object>,
recipient: SuiAddress,
mut gas_object: Object,
Expand Down Expand Up @@ -824,7 +825,8 @@ impl AuthorityState {
.flatten()
.collect::<Vec<_>>();

let mut temporary_store = AuthorityTemporaryStore::new(self, &input_objects, ctx.digest());
let mut temporary_store =
AuthorityTemporaryStore::new(self._database.clone(), &input_objects, ctx.digest());
let package_id = ObjectID::from(*modules[0].self_id().address());
let natives = self._native_functions.clone();
let vm = adapter::verify_and_link(&temporary_store, &modules, package_id, natives)?;
Expand Down Expand Up @@ -879,8 +881,7 @@ impl AuthorityState {

async fn update_state(
&self,
temporary_store: AuthorityTemporaryStore,

temporary_store: AuthorityTemporaryStore<AuthorityStore>,
certificate: CertifiedTransaction,
signed_effects: SignedTransactionEffects,
) -> Result<(u64, TransactionInfoResponse), SuiError> {
Expand Down
58 changes: 38 additions & 20 deletions sui_core/src/authority/authority_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use rocksdb::Options;
use std::collections::BTreeSet;
use std::convert::TryInto;
use std::path::Path;

use std::sync::atomic::AtomicU64;

use sui_types::base_types::SequenceNumber;
use sui_types::batch::{SignedBatch, TxSequenceNumber};
use tracing::warn;
Expand Down Expand Up @@ -386,7 +386,7 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
// This is the critical region: testing the locks and writing the
// new locks must be atomic, and not writes should happen in between.
{
// Aquire the lock to ensure no one else writes when we are in here.
// Acquire the lock to ensure no one else writes when we are in here.
// MutexGuards are unlocked on drop (ie end of this block)
let _mutexes = self.acquire_locks(mutable_input_objects);

Expand Down Expand Up @@ -424,9 +424,9 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
///
/// Internally it checks that all locks for active inputs are at the correct
/// version, and then writes locks, objects, certificates, parents atomically.
pub fn update_state(
pub fn update_state<S>(
&self,
temporary_store: AuthorityTemporaryStore,
temporary_store: AuthorityTemporaryStore<S>,
certificate: CertifiedTransaction,
signed_effects: SignedTransactionEffects,
) -> Result<(TxSequenceNumber, TransactionInfoResponse), SuiError> {
Expand Down Expand Up @@ -470,9 +470,9 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
}

/// Persist temporary storage to DB for genesis modules
pub fn update_objects_state_for_genesis(
pub fn update_objects_state_for_genesis<S>(
&self,
temporary_store: AuthorityTemporaryStore,
temporary_store: AuthorityTemporaryStore<S>,
transaction_digest: TransactionDigest,
) -> Result<(), SuiError> {
debug_assert_eq!(transaction_digest, TransactionDigest::genesis());
Expand All @@ -482,10 +482,10 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
}

/// Helper function for updating the objects in the state
fn batch_update_objects(
fn batch_update_objects<S>(
&self,
mut write_batch: DBBatch,
temporary_store: AuthorityTemporaryStore,
temporary_store: AuthorityTemporaryStore<S>,
transaction_digest: TransactionDigest,
should_sequence: bool,
) -> Result<Option<TxSequenceNumber>, SuiError> {
Expand Down Expand Up @@ -706,7 +706,7 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
/// Retrieves batches including transactions within a range.
///
/// This function returns all signed batches that enclose the requested transaction
/// including the batch preceeding the first requested transaction, the batch including
/// including the batch preceding the first requested transaction, the batch including
/// the last requested transaction (if there is one) and all batches in between.
///
/// Transactions returned include all transactions within the batch that include the
Expand Down Expand Up @@ -806,22 +806,40 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
}
}

impl ModuleResolver for AuthorityStore {
impl<const ALL_OBJ_VER: bool> BackingPackageStore for SuiDataStore<ALL_OBJ_VER> {
fn get_package(&self, package_id: &ObjectID) -> SuiResult<Option<Object>> {
let package = self.get_object(package_id)?;
if let Some(obj) = &package {
fp_ensure!(
obj.is_package(),
SuiError::BadObjectType {
error: format!("Package expected, Move object found: {}", package_id),
}
);
}
Ok(package)
}
}

impl<const ALL_OBJ_VER: bool> ModuleResolver for SuiDataStore<ALL_OBJ_VER> {
type Error = SuiError;

fn get_module(&self, module_id: &ModuleId) -> Result<Option<Vec<u8>>, Self::Error> {
match self.get_object(&ObjectID::from(*module_id.address()))? {
Some(o) => match &o.data {
Data::Package(c) => Ok(c
// TODO: We should cache the deserialized modules to avoid
// fetching from the store / re-deserializing them everytime.
// https://github.com/MystenLabs/sui/issues/809
Ok(self
.get_package(&ObjectID::from(*module_id.address()))?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think under the hood this de-serializers to get the package ID and then de-serializes again in the "and_then"? Lets add a TODO here to cache since these things are immutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it doesn't. module_id is just a pair of id and name.
Though we should cache the package object to avoid fetching from the store every time. At the same time, I feel the VM should do this inside it (to also avoid de-serialize it).

.and_then(|package| {
// unwrap safe since get_package() ensures it's a package object.
package
.data
.try_as_package()
.unwrap()
.serialized_module_map()
.get(module_id.name().as_str())
.cloned()
.map(|m| m.into_vec())),
_ => Err(SuiError::BadObjectType {
error: "Expected module object".to_string(),
}),
},
None => Ok(None),
}
.map(|m| m.into_vec())
}))
}
}
60 changes: 32 additions & 28 deletions sui_core/src/authority/temporary_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ pub type InnerTemporaryStore = (
Vec<Event>,
);

pub struct AuthorityTemporaryStore {
object_store: Arc<AuthorityStore>,
pub struct AuthorityTemporaryStore<S> {
// The backing store for retrieving Move packages onchain.
// When executing a Move call, the dependent packages are not going to be
// in the input objects. They will be feteched from the backing store.
package_store: Arc<S>,
tx_digest: TransactionDigest,
objects: BTreeMap<ObjectID, Object>,
active_inputs: Vec<ObjectRef>, // Inputs that are not read only
Expand All @@ -31,16 +34,16 @@ pub struct AuthorityTemporaryStore {
created_object_ids: HashSet<ObjectID>,
}

impl AuthorityTemporaryStore {
impl<S> AuthorityTemporaryStore<S> {
/// Creates a new store associated with an authority store, and populates it with
/// initial objects.
pub fn new(
authority_state: &AuthorityState,
package_store: Arc<S>,
_input_objects: &'_ [Object],
tx_digest: TransactionDigest,
) -> AuthorityTemporaryStore {
AuthorityTemporaryStore {
object_store: authority_state._database.clone(),
) -> Self {
Self {
package_store,
tx_digest,
objects: _input_objects.iter().map(|v| (v.id(), v.clone())).collect(),
active_inputs: _input_objects
Expand Down Expand Up @@ -200,7 +203,7 @@ impl AuthorityTemporaryStore {
}
}

impl Storage for AuthorityTemporaryStore {
impl<S> Storage for AuthorityTemporaryStore<S> {
/// Resets any mutations and deletions recorded in the store.
fn reset(&mut self) {
self.written.clear();
Expand All @@ -214,13 +217,7 @@ impl Storage for AuthorityTemporaryStore {
debug_assert!(self.deleted.get(id) == None);
match self.written.get(id) {
Some(x) => Some(x.clone()),
None => match self.objects.get(id) {
Some(x) => Some(x.clone()),
None => match self.object_store.get_object(id) {
Ok(o) => o,
Err(e) => panic!("Could not read object {}", e),
},
},
None => self.objects.get(id).cloned(),
}
}

Expand Down Expand Up @@ -276,26 +273,33 @@ impl Storage for AuthorityTemporaryStore {
}
}

impl ModuleResolver for AuthorityTemporaryStore {
impl<S: BackingPackageStore> ModuleResolver for AuthorityTemporaryStore<S> {
type Error = SuiError;
fn get_module(&self, module_id: &ModuleId) -> Result<Option<Vec<u8>>, Self::Error> {
match self.read_object(&ObjectID::from(*module_id.address())) {
Some(o) => match &o.data {
Data::Package(c) => Ok(c
.serialized_module_map()
.get(module_id.name().as_str())
.cloned()
.map(|m| m.into_vec())),
_ => Err(SuiError::BadObjectType {
error: "Expected module object".to_string(),
}),
let package_id = &ObjectID::from(*module_id.address());
let package = match self.read_object(package_id) {
Some(object) => object,
None => match self.package_store.get_package(package_id)? {
Some(object) => object,
None => {
return Ok(None);
}
},
None => Ok(None),
};
match &package.data {
Data::Package(c) => Ok(c
.serialized_module_map()
.get(module_id.name().as_str())
.cloned()
.map(|m| m.into_vec())),
_ => Err(SuiError::BadObjectType {
error: "Expected module object".to_string(),
}),
}
}
}

impl ResourceResolver for AuthorityTemporaryStore {
impl<S> ResourceResolver for AuthorityTemporaryStore<S> {
type Error = SuiError;

fn get_resource(
Expand Down
8 changes: 7 additions & 1 deletion sui_programmability/adapter/src/unit_tests/adapter_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use sui_types::{
error::SuiResult,
gas_coin::GAS,
object::{Data, Owner},
storage::Storage,
storage::{BackingPackageStore, Storage},
MOVE_STDLIB_ADDRESS, SUI_FRAMEWORK_ADDRESS,
};

Expand All @@ -41,6 +41,12 @@ struct InMemoryStorage {
temporary: ScratchPad,
}

impl BackingPackageStore for InMemoryStorage {
fn get_package(&self, package_id: &ObjectID) -> SuiResult<Option<Object>> {
Ok(self.persistent.get(package_id).cloned())
}
}

impl InMemoryStorage {
pub fn new(objects: Vec<Object>) -> Self {
let mut persistent = BTreeMap::new();
Expand Down
5 changes: 5 additions & 0 deletions sui_types/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::collections::HashSet;

use crate::{
base_types::{ObjectID, SequenceNumber},
error::SuiResult,
event::Event,
object::Object,
};
Expand Down Expand Up @@ -37,3 +38,7 @@ pub trait Storage {

fn delete_object(&mut self, id: &ObjectID, version: SequenceNumber, kind: DeleteKind);
}

pub trait BackingPackageStore {
fn get_package(&self, package_id: &ObjectID) -> SuiResult<Option<Object>>;
}