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 revocation registry definition #56

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Jan 11, 2023

closes #22

Where it was required, e.g. we pass it to Ursa, I used ISSUANCE_BY_DEFAULT which is a 0 or true.

@whalelephant could you also give this a review with regards to any revocation changes, I am not too familiar with the exact internals so just to be safe.

  • removed the revocation registry config type
  • removed V1 wrapper around revocation objects
  • removed issuance by default reference

@berendsliedrecht
Copy link
Contributor Author

@whalelephant this confused me a bit (probably 100% correct):

impl From<IssuanceType> for usize {
     fn from(value: IssuanceType) -> usize {
         match value {
             // Credentials are by default revoked
             IssuanceType::ISSUANCE_ON_DEMAND => 1,
             // Credentials are by default not revoked
             IssuanceType::ISSUANCE_BY_DEFAULT => 0,
         }
     }
 }

 impl IssuanceType {
     pub fn to_bool(&self) -> bool {
         *self == IssuanceType::ISSUANCE_BY_DEFAULT
     }
 }

Here ISSUANCE_BY_DEFAULT is a 0 and true. Normally its 1 == true, right?

Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

LGTM!

Will we be able to drop the revocation registry data type, or do we need to keep it internally? Maybe more a question for @whalelephant

@whalelephant
Copy link
Contributor

@blu3beri

If the status is 1, it is revoked according to the specs
i.e. by default means nothing is revoked, i.e. 0

@berendsliedrecht berendsliedrecht force-pushed the update-revocation-registry-definition branch 2 times, most recently from a26e197 to b7e7ada Compare January 11, 2023 10:55
@berendsliedrecht
Copy link
Contributor Author

berendsliedrecht commented Jan 11, 2023

LGTM!

Will we be able to drop the revocation registry data type, or do we need to keep it internally? Maybe more a question for @whalelephant

Do you mean the revoc_def_type? I can't find something else.

NVM I just found it.

Copy link
Contributor

@whalelephant whalelephant left a comment

Choose a reason for hiding this comment

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

We need to pass in the issuance_by_* bool here.

The issuer needs to know if the registry was created with on_demand or by_default

Copy link
Contributor

@whalelephant whalelephant left a comment

Choose a reason for hiding this comment

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

We need to pass in the issuance_by_* bool here.
The issuer needs to know if the registry was created with on_demand or by_default

See below for comments in code

@@ -118,9 +115,9 @@ where
pub fn create_revocation_registry<TW>(
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to pass in the issuance_by_* bool here.

The issuer needs to decide what type of registry it is creating.

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 believe @TimoGlastra mentioned in the Slack it should always be ISSUANCE_BY_DEFAULT. Do we have to revert this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think internally we always to use ISSUANCE_BY_DEFAULT. The property doesn't exist anymore in the spec, so you shouldn't have to provide it when creating a revocation registry.

The Indy Registry could still use issuance type, but they could just generate the deltas based on it separately: using the full state and you can get the accumulator. Whether you're using issuance on demand or issuance by default the accumulator value will be the same

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct that we are no longer supporting issuer using anoncreds to create ISSUANCE_ON_DEMAND revocation registry?

Whether you're using issuance on demand or issuance by default the accumulator value will be the same

I am not clear about what you mean in the above.

I understand using full state for supporting indy for subsequent updates after the registry is created.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm do you mean the initial revocation list that will be returned by the anoncreds_create_revocation_registry method?

So if I use ISSUANCE_BY_DEFAULT=true it will return a list with all 0s and if I pass ISSUANCE_BY_DEFAULT=false it will return all 1s? I wasn't aware the create_revocation_registry method also returned the first revocation registry entry (which will become the full state right?). In that case the boolean makes sense.

But it won't be present in the revocation registry definition right, it's just to know how to populate the first revocation registry definition?

Copy link
Contributor

Choose a reason for hiding this comment

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

It creates a few things

  • it creates the private key + RevocationRegistryDefinition which will not need to contain the issuance type (all good here)
  • it creates the accumulator value in the RevocationRegistry, which is calculated differently for difference issuance type, regardless if we return the RevocationStatusList
  • it creates the delta (right now) but we need to change it to the RevocationStatusList object to return to the issuer to then publish (containing the RevocationRegistry

i.e. we are going from:

pub fn create_revocation_registry<TW>(
   ...
) -> Result<(
    RevocationRegistryDefinition,
    RevocationRegistryDefinitionPrivate,
    RevocationRegistry,
    RevocationRegistryDelta,
)>

to

pub fn create_revocation_registry<TW>(
   ...
) -> Result<(
    RevocationRegistryDefinition,
    RevocationRegistryDefinitionPrivate,
    RevocationStatusList
)>

But it won't be present in the revocation registry definition right, it's just to know how to populate the first revocation registry definition?

yes exactly this, and it is the same for create_credential, the issuer needs to know what issuance_type the registry was first calculated with.

Copy link
Member

Choose a reason for hiding this comment

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

But the issuer would only need to know it when first creating the revocationList right? Because from then on you can always use the previous revocation list to calculate the new revocation list and accumulator?

In that case it's not exactly this:

the issuer needs to know what issuance_type the registry was first calculated with.

But rather:

The issuer uses the issuance_type to populate the initial revocationList.revocationList (the actual array iniside the model with 0/1s). From this revocationList bitarray it can generate the initial accumulator. So if we were to pass in a bit array instead of the maxCredNum and issuanceType, we would be able to generate the same values. Because the maxCredNum can be calculated from the bitarray length, and we don't actually need the issuanceType, because we have the full state.

Sorry if I'm being pedantic here, but I'm really trying to understand what exactly we need to issuanceType for 😊

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 it is simply easier for the issuer to pass in the issuance_type here rather than passing in the first ever revocationList and then us working out if there are more 1s or more 0s and assume that the majority will mean that it is the initial issuance type value.

(true, &empty, revocation.registry_used)
}
};
let (by_default, issued, revoked) = (true, &empty, revocation.registry_used);
Copy link
Contributor

Choose a reason for hiding this comment

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

The caller of create_credential - the issuer needs to pass in the issuance_by* here to update the rev_reg.

Perhaps add this field in CredentialRevocationConfig

Copy link
Member

Choose a reason for hiding this comment

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

The issuance type is deprecated. We should probably pass in the whole state and we can derive the next accumulator value based on that

As we always work with the full state (not deltas) in the public api we never need the issuance type

Copy link
Member

Choose a reason for hiding this comment

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

I thought we were going to split up the revocation part from the create_credential method and we pass in the whole revocation list to update the state. In that case, we don't need the issuance type right?

@berendsliedrecht
Copy link
Contributor Author

@TimoGlastra @whalelephant I think I have resolved the issue how it was described. The test is now failing because I am not too sure how I can create the revocation status list, with the correct values, before issuing that credential. But the code should be there.

@TimoGlastra
Copy link
Member

@whalelephant any idea how to quickly generate a valid revocation list?

@berendsliedrecht berendsliedrecht force-pushed the update-revocation-registry-definition branch from 424e474 to c25245b Compare January 12, 2023 13:08
@berendsliedrecht berendsliedrecht merged commit 293f1ef into hyperledger:main Jan 16, 2023
@berendsliedrecht berendsliedrecht deleted the update-revocation-registry-definition branch January 16, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the Revocation Registry Definition data type according to the AnonCreds specification
3 participants