Revocation List Model #37

whalelephant · 2022-12-27T15:12:58Z

This is the first PR introducing RevocationList model outlined in hyperledger/anoncreds-spec#108 and #24.

introduce RevocationList struct with serde to match outlined json, with tests
update create_or_update_revocation_state function for prover and FFI. This now takes in either 1 RevocationList to create or optionally an old one, does a comparison and update the revocation state.
initial end to end test in anoncreds_demo

Additional documentation is added to highlight the unchecked interval, which I am looking into for #36 and #41

whalelephant · 2023-01-05T05:31:33Z

Hey @TimoGlastra actually, I have just checked the specs on your PR
https://github.com/hyperledger/anoncreds-spec/pull/125/files#diff-16b69490a805effe319857540a2709114190895ffc75700d9776487de9ca5e2bR602

Are these the field names that should be used instead? If so I will need to update this PR.

TimoGlastra

I think overall this looks good to me, but I would like @blu3beri to also take a close look at this PR

TimoGlastra · 2023-01-05T05:40:57Z

Are these the field names that should be used instead? If so I will need to update this PR.

I think the only difference is the currentAccumulator field right?

whalelephant · 2023-01-05T08:00:18Z

I think the only difference is the currentAccumulator field right?

yep, but that struct serialisation is done in Ursa.

berendsliedrecht

Just some small cleanup thingies, nothing major. Also not too familiar with the revocation changes, but I think combined with @TimoGlastra we have a full code and spec review :).

berendsliedrecht · 2023-01-05T08:40:57Z

anoncreds/src/data_types/anoncreds/rev_reg.rs

+        self.revocation_list.clone()
+    }
+
+    pub fn new(


nit: I prefer to use try_new when it returns a Result<T>. A bit more explicit for the caller.

Following the pattern in impl_anoncreds_object_identifier for new() here, I don't mind either way but we might want to align similar methods?
@blu3beri

Yeah we can just keep it at new. Consistency is more important IMO.

berendsliedrecht · 2023-01-05T08:42:05Z

anoncreds/src/data_types/anoncreds/rev_reg.rs

+            let e = match *element.as_ref() {
+                true => 1,
+                false => 0,
+            };


Suggested change

let e = match *element.as_ref() {

true => 1,

false => 0,

};

let e = element as i32;

Not 100% sure what e is supposed to be. But casting to a number will convert it to 0/1.

berendsliedrecht · 2023-01-05T08:43:50Z

anoncreds/src/data_types/anoncreds/rev_reg.rs

+            fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+                formatter.write_str("a seq containing revoation state, i.e. [1, 0, 1]")
+            }


Suggested change

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {

formatter.write_str("a seq containing revoation state, i.e. [1, 0, 1]")

}

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {

write!(f, "a seq containing revoation state, i.e. [1, 0, 1]")

}

Do you know if there is a difference or will this only add string interpolation? Never used formatter.write_str like this.

They are the same, see https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.write_str

anoncreds/src/data_types/anoncreds/rev_reg.rs

berendsliedrecht · 2023-01-05T09:00:49Z

anoncreds/src/services/prover.rs

+                IssuanceType::ISSUANCE_ON_DEMAND => {
+                    bitvec![1; list_size]
+                }
+                IssuanceType::ISSUANCE_BY_DEFAULT => {
+                    bitvec![0; list_size]
+                }


Might be good to implement Into<u8> for IssuanceType to do the conversion to 1 or 0 there.

berendsliedrecht · 2023-01-05T09:02:50Z

anoncreds/src/services/prover.rs


    Ok(CredentialRevocationState {
        witness,
-        rev_reg: CryptoRevocationRegistry::from(rev_reg_delta.value.clone()),
-        timestamp,
+        rev_reg: rev_reg_list.into(),


Could we not change the type of the state to contain just the rev_reg_list? Now we have the timestamp in there twice.

The rev_reg_list is of type RevocationList which fields such as the revocation_list which is not needed.
CredentialRevocationState does not contain RevocationList so the timestamp is only there once.

TimoGlastra · 2023-01-05T09:44:33Z

yep, but that struct serialisation is done in Ursa.

Ah okay, would it be easy to just assign it to a different key? (that's what I would do in JS, but rust is different lol)

whalelephant · 2023-01-05T10:22:03Z

@TimoGlastra ,
we do not have direct access to the underlying accum value because it is a private field in the RevocationRegistry struct managed by Ursa, so the serde is also handled there as well.
@blu3beri if you have any ideas?

berendsliedrecht · 2023-01-05T10:27:26Z

@TimoGlastra ,
we do not have direct access to the underlying accum value because it is a private field in the RevocationRegistry struct managed by Ursa, so the serde is also handled there as well.
@blu3beri if you have any ideas?

We can create PRs for Ursa with the fixes, but creating a new release would be rather difficult as the pipeline is kind of dead. We can use a git within cargo, but that is a bit nasty.

TimoGlastra · 2023-01-05T12:23:31Z

But as the accum is just a property, isn't there a way to just pick the accum key from object a and place it as key currentAccumulator in object b? It sounds like that should be possible?

whalelephant · 2023-01-06T04:42:53Z

Because the Struct RevocationRegisty did not specify the field accum is public, we cannot access it.

What we could do is to serialise the struct into string, deserialise into another struct that we have in this repo from string and make that field public. thoughts?

TimoGlastra · 2023-01-06T05:29:17Z

What we could do is to serialise the struct into string, deserialise into another struct that we have in this repo from string and make that field public. thoughts?

That sounds fine to me 👍

Signed-off-by: bwty <whalelephant@users.noreply.github.com>

whalelephant · 2023-01-06T13:47:11Z

What we could do is to serialise the struct into string, deserialise into another struct that we have in this repo from string and make that field public. thoughts?

@TimoGlastra actually we cannot construct RevocationRegisty in anoncreds-rs which is needed for Ursa because it does not provide a public constructor either. So I think if we want to rename the field we might be back to forking the repo as @blu3beri mentioned.

TimoGlastra · 2023-01-09T12:34:28Z

Hmm ok. Let's keep it at accum for now and get this PR merged. Can you create a new issue to track this. We might just use accum, or we might find a way to use currentAccumulator.

@whalelephant any other outstanding things, or can the PR be merged?

whalelephant · 2023-01-09T13:01:55Z

Can merge I think, issue at #51

whalelephant force-pushed the feat/revocation-list branch 8 times, most recently from 629ba44 to b16c197 Compare January 3, 2023 09:41

whalelephant changed the title ~~wip: draft create_or_update_rev_state~~ Revocation List Model Jan 3, 2023

whalelephant force-pushed the feat/revocation-list branch from 2a5ee83 to 39b9138 Compare January 3, 2023 14:18

whalelephant marked this pull request as ready for review January 3, 2023 14:18

TimoGlastra reviewed Jan 5, 2023

View reviewed changes

berendsliedrecht approved these changes Jan 5, 2023

View reviewed changes

whalelephant requested review from berendsliedrecht and TimoGlastra and removed request for berendsliedrecht and TimoGlastra January 5, 2023 10:22

whalelephant added 3 commits January 6, 2023 20:40

feat: revocation_list model with demo test

76012b5

Signed-off-by: bwty <whalelephant@users.noreply.github.com>

feat: remove custom impl for ursa struct

45fee32

Signed-off-by: bwty <whalelephant@users.noreply.github.com>

chore: clippy and fmt

fe8a27d

Signed-off-by: bwty <whalelephant@users.noreply.github.com>

whalelephant added 2 commits January 6, 2023 20:40

fix: rebase conflict for demo tests

0263802

Signed-off-by: bwty <whalelephant@users.noreply.github.com>

feat: rev list serde and issuance type enhancements

1577d79

Signed-off-by: bwty <whalelephant@users.noreply.github.com>

whalelephant force-pushed the feat/revocation-list branch from 3201c90 to 3c650af Compare January 6, 2023 13:41

chore: rename to RevocationStatusList

1c380ee

Signed-off-by: bwty <whalelephant@users.noreply.github.com>

whalelephant force-pushed the feat/revocation-list branch from 3c650af to 1c380ee Compare January 6, 2023 13:43

berendsliedrecht mentioned this pull request Jan 9, 2023

Added issuerId to the schema and cred_def anoncreds objects #50

Merged

TimoGlastra merged commit b99a7ee into hyperledger:main Jan 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revocation List Model #37

Revocation List Model #37

whalelephant commented Dec 27, 2022 •

edited

Loading

whalelephant commented Jan 5, 2023 •

edited

Loading

TimoGlastra left a comment

TimoGlastra commented Jan 5, 2023

whalelephant commented Jan 5, 2023

berendsliedrecht left a comment

berendsliedrecht Jan 5, 2023

whalelephant Jan 5, 2023

berendsliedrecht Jan 5, 2023

berendsliedrecht Jan 5, 2023

berendsliedrecht Jan 5, 2023

whalelephant Jan 5, 2023

berendsliedrecht Jan 5, 2023

berendsliedrecht Jan 5, 2023

whalelephant Jan 5, 2023

TimoGlastra commented Jan 5, 2023

whalelephant commented Jan 5, 2023

berendsliedrecht commented Jan 5, 2023

TimoGlastra commented Jan 5, 2023

whalelephant commented Jan 6, 2023 •

edited

Loading

TimoGlastra commented Jan 6, 2023

whalelephant commented Jan 6, 2023

TimoGlastra commented Jan 9, 2023

whalelephant commented Jan 9, 2023 •

edited

Loading

Revocation List Model #37

Revocation List Model #37

Conversation

whalelephant commented Dec 27, 2022 • edited Loading

whalelephant commented Jan 5, 2023 • edited Loading

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra commented Jan 5, 2023

whalelephant commented Jan 5, 2023

berendsliedrecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra commented Jan 5, 2023

whalelephant commented Jan 5, 2023

berendsliedrecht commented Jan 5, 2023

TimoGlastra commented Jan 5, 2023

whalelephant commented Jan 6, 2023 • edited Loading

TimoGlastra commented Jan 6, 2023

whalelephant commented Jan 6, 2023

TimoGlastra commented Jan 9, 2023

whalelephant commented Jan 9, 2023 • edited Loading

whalelephant commented Dec 27, 2022 •

edited

Loading

whalelephant commented Jan 5, 2023 •

edited

Loading

whalelephant commented Jan 6, 2023 •

edited

Loading

whalelephant commented Jan 9, 2023 •

edited

Loading