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

Update Stronghold #691

Merged
merged 14 commits into from
Mar 9, 2022
Merged

Update Stronghold #691

merged 14 commits into from
Mar 9, 2022

Conversation

abdulmth
Copy link
Contributor

@abdulmth abdulmth commented Mar 7, 2022

Description of change

Update the Stronghold dependency to use the commit 969df405661ba4977f2cf30e9909cef7e30cefa2.
This update includes new error handling and procedures API.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

@abdulmth abdulmth self-assigned this Mar 7, 2022
@abdulmth abdulmth added Chore Tedious, typically non-functional change Rust Related to the core Rust code. Becomes part of the Rust changelog. Added A new feature that requires a minor release. Part of "Added" section in changelog labels Mar 7, 2022
@abdulmth abdulmth marked this pull request as ready for review March 7, 2022 09:43
@abdulmth abdulmth changed the title Upgrade Stronghold Update Stronghold Mar 7, 2022
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good, but we can remove a few things and also get rid of some error variants afterwards.

Comment on lines 54 to 65
pub async fn get(&self, location: Location) -> Result<Vec<u8>> {
match self.get_strict(location).await {
pub async fn get(&self, location: Location) -> IotaStrongholdResult<Option<Vec<u8>>> {
match self.get_strict(location.vault_path().to_vec()).await {
Ok(data) => Ok(data),
Err(Error::StrongholdResult(message)) if message == STRONG_404 => Ok(Vec::new()),
Err(StrongholdError::StrongholdResult(message)) if message == STRONG_404 => Ok(Some(Vec::new())),
Err(error) => Err(error),
}
}

/// Gets a record.
pub async fn get_strict(&self, location: Location) -> Result<Vec<u8>> {
pub async fn get_strict(&self, location: Vec<u8>) -> IotaStrongholdResult<Option<Vec<u8>>> {
let scope: _ = Context::scope(self.path, &self.name, &self.flags).await?;
let (data, status): (Vec<u8>, _) = scope.read_from_store(location).await;

status.to_result()?;

Ok(data)
Ok(scope.read_from_store(location).await?)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove get_strict, the STRONG_404 constant and use:

/// Gets a record or returns `None` if it doesn't exist.
pub async fn get(&self, location: Location) -> IotaStrongholdResult<Option<Vec<u8>>> {
  let scope: _ = Context::scope(self.path, &self.name, &self.flags).await?;
  Ok(scope.read_from_store(location.vault_path().to_vec()).await?)
}

The behaviour of read_from_store in stronghold has changed and it no longer returns an error if it doesn't find an entry but None instead, which is what we want.

Comment on lines +36 to +37
#[error("Record Error")]
RecordError,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the Records type is actually unused and you could probably remove all that. Also remove Snapshot::records in that case. Then you could also remove this error variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need records at all, even to implement listing all DIDs in a Stronghold #567 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed Records for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need records at all, even to implement listing all DIDs in a Stronghold #567 ?

I guess we could use it for that, yeah. I actually thought this was wrapping the stronghold record hints API which is going to be removed with the refactor, which is why I didn't look that closely at what Records was doing. I think we can also just reintroduce it in a future PR, if it turns out to be useful then, since it's already removed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the commit that removed the records 😅

Comment on lines 456 to 449
fn event_listeners(&self) -> Result<MutexGuard<'_, Vec<Listener>>> {
fn event_listeners(&self) -> IotaStrongholdResult<MutexGuard<'_, Vec<Listener>>> {
self
.event_listeners
.lock()
.map_err(|_| Error::StrongholdMutexPoisoned("listeners"))
.map_err(|_| StrongholdError::StrongholdMutexPoisoned("listeners"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we could get rid of the error variant by doing:

match self.event_listeners.lock() {
      Ok(guard) => Ok(guard),
      Err(err) => Ok(err.into_inner()),
    }

And similarly for the other functions. Then we should be able to remove StrongholdError::StrongholdMutexPoisoned. Thanks for the hint on this one, @olivereanderson!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to give the guard and allow access if the mutex is poisoned? it sounds to me more of an error case, not sure if we want to ignore it completely.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Mar 7, 2022

Choose a reason for hiding this comment

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

I'm not entirely sure what to do here. A typical solution is to unwrap on the .lock() call, so if some other thread panicked, we panic as well. The current solution returns an error, and that at least gives the caller a chance to react to it. Basically, if some other part of the program already panicked, which we try to avoid in the first place, then it seems okay-ish to call unwrap(), because the error in the other thread was already severe enough to warrant a panic.
Using PoisonError::into_inner grants access to the MutexGuard despite the lock being poisoned, but it's a bit unclear to me whether the lock semantics are still being upheld, e.g. can two threads that haven't panicked access the data concurrently? I strongly assume not, in which case doing this seems okay.

I'm mainly trying to get rid of the error variant, because it doesn't neatly fall into a category of errors, which is what we will have to come up with to implement a more generic/abstract StorageError for the storage interface, but that's probably not that important to consider here.

Does anyone else have an opinion on what's best here, @cycraig or @olivereanderson?

Copy link
Contributor

Choose a reason for hiding this comment

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

@olivereanderson had a neat suggestion, which is to use the parking_lot::Mutex instead, which does not have poisoning in the first place, so we can avoid the error without a potential panic. parking_lot is not a direct dependency of the account yet, but it is used by our transitive dependencies (like tokio) already, so adding it seems totally fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Philipp and Oliver for the suggestion. I replaced it with parking_lot::Mutex.

identity-account/src/stronghold/error.rs Outdated Show resolved Hide resolved
identity-account/src/stronghold/store.rs Outdated Show resolved Hide resolved
Comment on lines 198 to 201
// No state data found
if data.is_empty() {
return Ok(None);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think since Store::get returns None if the key wasn't found, this doesn't need to be handled anymore, the way it was previously. In other words, we want IdentityState::from_json_slice to error if data.is_empty() because that indicates a storage error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the .is_empty() check.

This reverts commit fe8c90a.
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

The only thing left is making sure there is no dependency on identity_account::error::Error within the stronghold directory. Try temporarily commenting the StrongholdError(#[from] StrongholdError), variant in the account error, and check that there are no errors within the stronghold directory. That will make it easier to split the account and storage into separate crates.

identity-account/src/stronghold/context.rs Outdated Show resolved Hide resolved
@@ -37,7 +40,7 @@ pub struct Context {

async fn clear_expired_passwords() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also return an IotaStrongholdResult.

@@ -48,18 +50,23 @@ impl Records<'_> {
}

pub async fn index(&self) -> Result<RecordIndex> {
Copy link
Contributor