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

Revocation registry cleanups #233

Merged
merged 3 commits into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ logger = ["env_logger"]
vendored = ["anoncreds-clsignatures/openssl_vendored"]

[dependencies]
anoncreds-clsignatures = "0.2"
anoncreds-clsignatures = "0.2.1"
bs58 = "0.4.0"
env_logger = { version = "0.9.3", optional = true }
ffi-support = { version = "0.4.0", optional = true }
Expand Down
87 changes: 2 additions & 85 deletions src/data_types/rev_reg.rs
Original file line number Diff line number Diff line change
@@ -1,92 +1,9 @@
use serde::de::{self, Deserialize, Deserializer, MapAccess, Visitor};
use serde::Serialize;

use crate::cl::{Accumulator, RevocationRegistry as CryptoRevocationRegistry};
use crate::{impl_anoncreds_object_identifier, Error};
use crate::cl::RevocationRegistry as CryptoRevocationRegistry;
use crate::impl_anoncreds_object_identifier;

impl_anoncreds_object_identifier!(RevocationRegistryId);

#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct RevocationRegistry {
pub value: CryptoRevocationRegistry,
}

#[derive(Clone, Copy, Debug, Serialize)]
pub struct CLSignaturesRevocationRegistry(Accumulator);

impl CLSignaturesRevocationRegistry {
pub fn empty() -> Result<Self, Error> {
let accum = Accumulator::new_inf()?;
Ok(Self(accum))
}
}

impl TryFrom<&str> for CLSignaturesRevocationRegistry {
type Error = Error;

fn try_from(value: &str) -> Result<Self, Self::Error> {
let accum = Accumulator::from_string(value)?;
Ok(Self(accum))
}
}

impl From<CryptoRevocationRegistry> for CLSignaturesRevocationRegistry {
fn from(value: CryptoRevocationRegistry) -> Self {
Self(value.accum)
}
}

impl From<CLSignaturesRevocationRegistry> for CryptoRevocationRegistry {
fn from(value: CLSignaturesRevocationRegistry) -> Self {
Self { accum: value.0 }
}
}

impl<'de> Deserialize<'de> for CLSignaturesRevocationRegistry {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
struct CLSignaturesRevocationRegistryVisitor;

impl<'de> Visitor<'de> for CLSignaturesRevocationRegistryVisitor {
type Value = CLSignaturesRevocationRegistry;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(formatter, "string or map")
}

fn visit_str<E: serde::de::Error>(
self,
value: &str,
) -> Result<CLSignaturesRevocationRegistry, E> {
let accum = Accumulator::from_string(value).map_err(de::Error::custom)?;
Ok(CLSignaturesRevocationRegistry(accum))
}

fn visit_map<V>(self, mut map: V) -> Result<CLSignaturesRevocationRegistry, V::Error>
where
V: MapAccess<'de>,
{
let mut accum = None;
while let Some(key) = map.next_key()? {
match key {
"currentAccumulator " | "accum" => {
if accum.is_some() {
return Err(de::Error::duplicate_field(
"(accum|currentAccumulator)",
));
}
accum = Some(map.next_value()?);
}
_ => (),
}
}
let accum: Accumulator =
accum.ok_or_else(|| de::Error::missing_field("(accum|currentAccumulator)"))?;
Ok(CLSignaturesRevocationRegistry(accum))
}
}
deserializer.deserialize_any(CLSignaturesRevocationRegistryVisitor)
}
}
115 changes: 84 additions & 31 deletions src/data_types/rev_status_list.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use super::issuer_id::IssuerId;
use super::rev_reg::{CLSignaturesRevocationRegistry, RevocationRegistry};
use super::rev_reg::RevocationRegistry;
use super::rev_reg_def::RevocationRegistryDefinitionId;

use crate::cl::RevocationRegistry as CryptoRevocationRegistry;
use crate::cl::{Accumulator, RevocationRegistry as CryptoRevocationRegistry};
use crate::{Error, Result};

use std::collections::BTreeSet;
Expand All @@ -17,35 +17,27 @@ pub struct RevocationStatusList {
issuer_id: IssuerId,
#[serde(with = "serde_revocation_list")]
revocation_list: bitvec::vec::BitVec,
#[serde(rename = "currentAccumulator", skip_serializing_if = "Option::is_none")]
registry: Option<CLSignaturesRevocationRegistry>,
#[serde(
rename = "currentAccumulator",
skip_serializing_if = "Option::is_none",
with = "serde_opt_accumulator"
Copy link
Contributor

@bobozaur bobozaur Aug 17, 2023

Choose a reason for hiding this comment

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

This seems like an overkill. Serialization doesn't seem to require anything custom, so the attribute could instead be deserialize_with. But then the custom deserialization seems to only want to achieve having multiple names for the field, which can also be done with #[serde(alias = "currentAccumulator"].

Depending on which should be the serialization name for the field, you might want to use #[serde(rename = "currentAccumulator", alias = "accum"] instead.

Copy link
Contributor

@berendsliedrecht berendsliedrecht Aug 17, 2023

Choose a reason for hiding this comment

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

The test json_rev_list_can_be_deserialized might also need to be expanded to make sure that the test vector, which includes currentAccumulator checks that des.accum.is_some(). as skip_serializing_if="Option::is_none" is set, it will silently fail if it can not interpret currentAccumulator correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't trying to change the current deserialization, but it's pretty strange. It accepts values like:

{ ...
  "currentAccumulator": {
    "currentAccumulator": "1 000..."
  }
  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because the visitor implements the visit_map method.

Copy link
Member Author

Choose a reason for hiding this comment

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

So do we think that's a mistake and it's just meant to support both currentAccumulator and accum as the field name? In that case it wouldn't need any custom deserialization.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fairly sure that it is a mistake. It was a custom implementation because we could not change it as it was a private field within Ursa (it has been a while so there might be more to it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well... The changes are introduced in this PR, so I was honestly expecting you to know the answer to that 😅 . To me it just seemed weird so I pointed it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved the deserialization code around and changed it to an Accumulator value, the old implementation had the visit_map method.

Copy link
Member Author

@andrewwhitehead andrewwhitehead Aug 17, 2023

Choose a reason for hiding this comment

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

Updated to just deserialize from a string, which is the format the tests use. I'm a little surprised the bitvec serialization is a list instead of hex bytes or something.

)]
accum: Option<Accumulator>,
#[serde(skip_serializing_if = "Option::is_none")]
timestamp: Option<u64>,
}

impl TryFrom<&RevocationStatusList> for Option<CryptoRevocationRegistry> {
type Error = Error;

fn try_from(value: &RevocationStatusList) -> std::result::Result<Self, Self::Error> {
Ok(value.registry.map(From::from))
impl From<&RevocationStatusList> for Option<CryptoRevocationRegistry> {
fn from(value: &RevocationStatusList) -> Self {
value.accum.map(From::from)
}
}

impl From<&RevocationStatusList> for Option<CLSignaturesRevocationRegistry> {
fn from(rev_status_list: &RevocationStatusList) -> Self {
rev_status_list.registry.map(Into::into)
}
}

impl TryFrom<&RevocationStatusList> for Option<RevocationRegistry> {
type Error = Error;

fn try_from(value: &RevocationStatusList) -> std::result::Result<Self, Self::Error> {
let value = value.registry.map(|registry| RevocationRegistry {
impl From<&RevocationStatusList> for Option<RevocationRegistry> {
fn from(value: &RevocationStatusList) -> Self {
value.accum.map(|registry| RevocationRegistry {
value: registry.into(),
});

Ok(value)
})
}
}

Expand All @@ -67,7 +59,7 @@ impl RevocationStatusList {
}

pub fn set_registry(&mut self, registry: CryptoRevocationRegistry) -> Result<()> {
self.registry = Some(registry.into());
self.accum = Some(registry.accum);
Ok(())
}

Expand All @@ -84,7 +76,7 @@ impl RevocationStatusList {
) -> Result<()> {
// only update if input is Some
if let Some(reg) = registry {
self.registry = Some(reg.into());
self.accum = Some(reg.accum);
}
let slots_count = self.revocation_list.len();
if let Some(issued) = issued {
Expand Down Expand Up @@ -124,11 +116,11 @@ impl RevocationStatusList {
Ok(())
}

pub fn new(
pub(crate) fn new(
rev_reg_def_id: Option<&str>,
issuer_id: IssuerId,
revocation_list: bitvec::vec::BitVec,
registry: Option<CLSignaturesRevocationRegistry>,
registry: Option<CryptoRevocationRegistry>,
timestamp: Option<u64>,
) -> Result<Self> {
Ok(Self {
Expand All @@ -137,18 +129,16 @@ impl RevocationStatusList {
.transpose()?,
issuer_id,
revocation_list,
registry,
accum: registry.map(|r| r.accum),
timestamp,
})
}
}

pub mod serde_revocation_list {
use bitvec::vec::BitVec;
use serde::de::Error as DeError;
use serde::de::SeqAccess;
use serde::{
de::{Deserializer, Visitor},
de::{Deserializer, Error as DeError, SeqAccess, Visitor},
ser::{SerializeSeq, Serializer},
};

Expand Down Expand Up @@ -202,6 +192,69 @@ pub mod serde_revocation_list {
}
}

pub mod serde_opt_accumulator {
use crate::cl::Accumulator;
use serde::{
de::{Deserializer, Error, MapAccess, Visitor},
ser::Serializer,
Serialize,
};

pub fn serialize<S>(value: &Option<Accumulator>, s: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
if let Some(acc) = value {
acc.serialize(s)
} else {
s.serialize_none()
}
}

pub fn deserialize<'de, D>(
deserializer: D,
) -> std::result::Result<Option<Accumulator>, D::Error>
where
D: Deserializer<'de>,
{
struct AccumulatorVisitor;

impl<'de> Visitor<'de> for AccumulatorVisitor {
type Value = Option<Accumulator>;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(formatter, "accumulator value as a string or map")
}

fn visit_str<E: serde::de::Error>(self, value: &str) -> Result<Option<Accumulator>, E> {
let accum = Accumulator::from_string(value).map_err(Error::custom)?;
Ok(Some(accum))
}

fn visit_map<V>(self, mut map: V) -> Result<Option<Accumulator>, V::Error>
where
V: MapAccess<'de>,
{
let mut accum = None;
while let Some(key) = map.next_key()? {
match key {
"currentAccumulator " | "accum" => {
if accum.is_some() {
return Err(Error::duplicate_field("(accum|currentAccumulator)"));
}
accum = map.next_value()?;
}
_ => (),
}
}
Ok(accum)
}
}

deserializer.deserialize_any(AccumulatorVisitor)
}
}

#[cfg(test)]
mod rev_reg_tests {
use super::*;
Expand Down
40 changes: 14 additions & 26 deletions src/services/issuer.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::cl::{Issuer, RevocationRegistry};
use crate::cl::{Issuer, RevocationRegistry as CryptoRevocationRegistry};
use crate::data_types::cred_def::CredentialDefinitionId;
use crate::data_types::issuer_id::IssuerId;
use crate::data_types::rev_reg::{CLSignaturesRevocationRegistry, RevocationRegistryId};
use crate::data_types::rev_reg::RevocationRegistryId;
use crate::data_types::rev_reg_def::RevocationRegistryDefinitionId;
use crate::data_types::schema::SchemaId;
use crate::data_types::{
Expand Down Expand Up @@ -353,30 +353,20 @@ pub fn create_revocation_status_list(
timestamp: Option<u64>,
) -> Result<RevocationStatusList> {
let max_cred_num = rev_reg_def.value.max_cred_num;
let mut rev_reg = RevocationRegistry::from(CLSignaturesRevocationRegistry::empty()?);

let list = if issuance_by_default {
let cred_pub_key = cred_def.get_public_key()?;
let issued = (1..=max_cred_num).collect::<BTreeSet<_>>();

Issuer::update_revocation_registry(
&mut rev_reg,
max_cred_num,
issued,
BTreeSet::new(),
&cred_pub_key,
&rev_reg_priv.value,
)?;
bitvec![0; max_cred_num as usize ]
} else {
bitvec![1; max_cred_num as usize ]
};
let cred_pub_key = cred_def.get_public_key()?;
let rev_reg = CryptoRevocationRegistry::initial_state(
&cred_pub_key,
&rev_reg_priv.value,
max_cred_num,
issuance_by_default,
)?;
let list = bitvec![if issuance_by_default { 0 } else { 1 }; max_cred_num as usize ];

RevocationStatusList::new(
Some(rev_reg_def_id.to_string().as_str()),
rev_reg_def.issuer_id.clone(),
list,
Some(rev_reg.into()),
Some(rev_reg),
timestamp,
)
}
Expand Down Expand Up @@ -552,7 +542,7 @@ pub fn update_revocation_status_list(
.collect::<BTreeSet<_>>()
});

let rev_reg_opt: Option<RevocationRegistry> = current_list.try_into()?;
let rev_reg_opt: Option<CryptoRevocationRegistry> = current_list.into();
let mut rev_reg = rev_reg_opt.ok_or_else(|| {
Error::from_msg(
ErrorKind::Unexpected,
Expand Down Expand Up @@ -739,16 +729,14 @@ pub fn create_credential(
(revocation_config, rev_status_list)
{
let rev_reg_def = &revocation_config.reg_def.value;
let rev_reg: Option<CLSignaturesRevocationRegistry> = rev_status_list.into();
let rev_reg = rev_reg.ok_or_else(|| {
let rev_reg: Option<CryptoRevocationRegistry> = rev_status_list.into();
let mut rev_reg = rev_reg.ok_or_else(|| {
err_msg!(
Unexpected,
"RevocationStatusList should have accumulator value"
)
})?;

let mut rev_reg: RevocationRegistry = rev_reg.into();

let status = rev_status_list
.get(revocation_config.registry_idx as usize)
.ok_or_else(|| {
Expand Down
10 changes: 4 additions & 6 deletions src/services/prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -555,10 +555,8 @@ pub fn create_revocation_state_with_witness(
revocation_status_list: &RevocationStatusList,
timestamp: u64,
) -> Result<CredentialRevocationState> {
let rev_reg = <&RevocationStatusList as TryInto<Option<RevocationRegistry>>>::try_into(
revocation_status_list,
)?
.ok_or_else(|| err_msg!(Unexpected, "Revocation Status List must have accum value"))?;
let rev_reg = Option::<RevocationRegistry>::from(revocation_status_list)
.ok_or_else(|| err_msg!(Unexpected, "Revocation Status List must have accum value"))?;

Ok(CredentialRevocationState {
witness,
Expand Down Expand Up @@ -656,7 +654,7 @@ pub fn create_or_update_revocation_state(
old_rev_status_list,
);

let rev_reg: Option<RevocationRegistry> = rev_status_list.try_into()?;
let rev_reg: Option<RevocationRegistry> = rev_status_list.into();
let rev_reg = rev_reg.ok_or_else(|| {
err_msg!("revocation registry is required to create or update the revocation state")
})?;
Expand All @@ -680,7 +678,7 @@ pub fn create_or_update_revocation_state(
&mut revoked,
);

let source_rev_reg: Option<RevocationRegistry> = source_rev_list.try_into()?;
let source_rev_reg: Option<RevocationRegistry> = source_rev_list.into();

let rev_reg_delta = RevocationRegistryDelta::from_parts(
source_rev_reg.as_ref(),
Expand Down
Loading
Loading