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

Raise error when status list size does not meet minimum threshold. #115

Merged
merged 3 commits into from
Jan 13, 2024

Conversation

msporny
Copy link
Member

@msporny msporny commented Dec 29, 2023

This PR is an attempt to address issue #87 by raising an error when status list size does not meet minimum threshold.


Preview | Diff

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
@@ -744,6 +745,11 @@ <h3>Validate Algorithm</h3>
<a href="#bitstring-expansion-algorithm">Bitstring Expansion Algorithm</a>.
</li>
<li>
If the length of the |revocation bitstring| divided by
<a href="#size">`size`</a> is less than 131,072, raise a
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<a href="#size">`size`</a> is less than 131,072, raise a
<a href="#statusSize">`statusSize`</a> is less than 131,072, raise a

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'm going to hold off on this change until the change happens in #109.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change is now applied in #109.

index.html Outdated Show resolved Hide resolved
If better herd privacy is required, the bitstring can be made to be larger.
<a>verifiable credential</a> statuses to be placed in the same list.
This specification uses a minimum list length of 131,072. This
size ensures an adequate amount of herd privacy in the average case.
Copy link
Contributor

Choose a reason for hiding this comment

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

The suggestion has been to replace "herd" with "group" everywhere as friendlier terminology.

Suggested change
size ensures an adequate amount of herd privacy in the average case.
size ensures an adequate amount of group privacy in the average case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was thinking the same thing while processing these PRs. I was going to raise an entirely separate PR to do the "herd" -> "group" renaming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Change raised in PR #121.

index.html Outdated
@@ -744,6 +745,11 @@ <h3>Validate Algorithm</h3>
<a href="#bitstring-expansion-algorithm">Bitstring Expansion Algorithm</a>.
</li>
<li>
If the length of the |revocation bitstring| divided by
<a href="#size">`size`</a> is less than 131,072, raise a
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little apprehensive about us forcing implementations to raise this error when perhaps there are legitimate use cases for a smaller size. We started with a default size that we thought was good for the average case, but this PR is now forcing that to be a hard minimum. I think we should be more flexible.

Suggested change
<a href="#size">`size`</a> is less than 131,072, raise a
<a href="#size">`size`</a> is less than 131,072, implementations
SHOULD raise a

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 not sure how SHOULD would actually work in practice. Say a general purpose wallet implements the SHOULD as a MUST then it will always raise an error on short lists. But if this wallet implements the SHOULD as a MAY, then it will never raise an error on short lists and will always accept them. I would not advocate asking the user whether a short list should be accepted or not. Therefore from an implementation perspective I think that MUST should be the standard definition and verifiers should always raise an error on short lists. If a particular eco-system wants to implement short lists then its issuers and wallets and verifiers can do this without affecting the global eco-system.

Copy link
Member Author

@msporny msporny Dec 30, 2023

Choose a reason for hiding this comment

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

Yes, I was somewhat concerned as well... but couldn't think of a use case where a smaller size was warranted.

If the population size is smaller, the 16KB array compresses down to a very small number (a few hundred to thousand bytes in size). So, the argument against needing a smaller size probably doesn't hold... and that's the only argument I can see for allowing a smaller list size.

Copy link
Contributor

Choose a reason for hiding this comment

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

@David-Chadwick,

I am not sure how SHOULD would actually work in practice. Say a general purpose wallet implements the SHOULD as a MUST then it will always raise an error on short lists. But if this wallet implements the SHOULD as a MAY, then it will never raise an error on short lists and will always accept them. I would not advocate asking the user whether a short list should be accepted or not.

But I think this is precisely how a SHOULD would work in practice. A very generic wallet (or verifier) would implement it "as a MUST" as you say and it would not accept anything outside of those bounds.

Therefore from an implementation perspective I think that MUST should be the standard definition and verifiers should always raise an error on short lists. If a particular eco-system wants to implement short lists then its issuers and wallets and verifiers can do this without affecting the global eco-system.

This would cause those implementations (in that particular ecosystem) to be in violation of the standard, which is the undesirable outcome that the SHOULD is intended to avoid.

Perhaps one way to resolve this is to make it a MUST with an "unless clause" that says that there's an agreed upon smaller size for particular credentials within a subecosystem or something along those lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think it is an undesirable outcome if an isolated eco-system chooses to ignore one or more parts of the standard and to implement its own features. Every isolated eco-system is free to implement what it wants to. We should be concerned about the global interworking system, which is why MUST is the preferred statement in the standard.

Copy link
Member

Choose a reason for hiding this comment

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

We SHOULD also be somewhat concerned with such an "isolated eco-system" eventually joining "the global interworking system", or even needing to be blended with a second "isolated eco-system". SHOULD is a far better future-proof than MUST, even though it allows two such "isolated eco-system" to imperfectly interoperate because one raises the error and the other does not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 601246f in a way that achieves what I believe @dlongley, @David-Chadwick, and @TallTed wanted to see.

The minimum number of entries is 131,072 unless a different one is specified by a particular specification and provided as input by the application (presumably through validation rules). The algorithm MUST throw an error if the length of the list is below the acceptable threshold established.

Copy link
Contributor

@David-Chadwick David-Chadwick left a comment

Choose a reason for hiding this comment

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

Without changing MUST to SHOULD

index.html Outdated
@@ -744,6 +745,11 @@ <h3>Validate Algorithm</h3>
<a href="#bitstring-expansion-algorithm">Bitstring Expansion Algorithm</a>.
</li>
<li>
If the length of the |revocation bitstring| divided by
<a href="#size">`size`</a> is less than 131,072, raise a
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 not sure how SHOULD would actually work in practice. Say a general purpose wallet implements the SHOULD as a MUST then it will always raise an error on short lists. But if this wallet implements the SHOULD as a MAY, then it will never raise an error on short lists and will always accept them. I would not advocate asking the user whether a short list should be accepted or not. Therefore from an implementation perspective I think that MUST should be the standard definition and verifiers should always raise an error on short lists. If a particular eco-system wants to implement short lists then its issuers and wallets and verifiers can do this without affecting the global eco-system.

Base automatically changed from msporny-http-errors to main January 13, 2024 17:39
@msporny msporny force-pushed the msporny-size-check branch from 3fc6cba to 83b2f04 Compare January 13, 2024 19:31
@msporny
Copy link
Member Author

msporny commented Jan 13, 2024

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 23d8d7f into main Jan 13, 2024
1 of 2 checks passed
@msporny msporny deleted the msporny-size-check branch January 13, 2024 19:45
@msporny msporny restored the msporny-size-check branch January 13, 2024 19:51
@msporny msporny deleted the msporny-size-check branch January 13, 2024 19:51
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.

5 participants