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

Test that the encrypted payloads are uniform. #374

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Oct 22, 2024

close #371

This randomized test will generate a false negative with negligible probability
if all encrypted messages share an identical byte at a given position by chance.
It should fail deterministically if any bit position has a fixed value.

re #364 (review) from @nothingmuch

I did check that this test would indeed fail before the ellswift changes by cherry-picking the test on 9c4880c

Copy link
Collaborator

@nothingmuch nothingmuch left a comment

Choose a reason for hiding this comment

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

ACK with some minor corrections, IMO can be merged with or without my last proposed change (bit-wise instead of byte-wise comparison) at your discretion (if you do then make sure to adjust the number of messages I didn't add a suggestion for that).

payjoin/src/hpke.rs Outdated Show resolved Hide resolved
payjoin/src/hpke.rs Outdated Show resolved Hide resolved
}

assert!(
accumulator.iter().all(|&b| b != 0),
Copy link
Collaborator

@nothingmuch nothingmuch Oct 22, 2024

Choose a reason for hiding this comment

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

Another approach would be to check that all bits (as opposed to bytes) contain variation. For this check the number of messages in the set should be 80 or 128 or whatever instead of 16 in order to have negligible chance of false positive.

Suggested change
accumulator.iter().all(|&b| b != 0),
accumulator.iter().any(|&b| b != 0xff),

edit: previous suggestion was incorrect, comparing to 1 instead of 0xff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slight typo in your suggestion:

!accumulator.iter().any(|&b| b != 0xff) == accumulator.iter().all(|&b| b == 0xFF).

I chose to implement the latter for readability.

@DanGould DanGould force-pushed the test-payload-uniformity branch from b063380 to cf43240 Compare October 23, 2024 17:04
@DanGould DanGould requested a review from nothingmuch October 23, 2024 17:05
@DanGould DanGould force-pushed the test-payload-uniformity branch 2 times, most recently from dfb2456 to 5fc4d03 Compare October 23, 2024 17:54
Copy link
Collaborator

@nothingmuch nothingmuch left a comment

Choose a reason for hiding this comment

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

oof,3rd time i'm trying to post this "pending" comment, not sure what went wrong the first two times.... sorry for the delay

payjoin/src/hpke.rs Outdated Show resolved Hide resolved
This randomized test will generate a false negative with negligible probability
if all encrypted messages share an identical byte at a given position by chance.
It should fail deterministically if any bit position has a fixed value.
@DanGould DanGould force-pushed the test-payload-uniformity branch from 5fc4d03 to 306607e Compare October 24, 2024 14:12
@DanGould DanGould requested a review from nothingmuch October 24, 2024 14:27
@DanGould DanGould merged commit f70619b into payjoin:master Oct 24, 2024
4 checks passed
@DanGould DanGould deleted the test-payload-uniformity branch October 24, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Consider Making pjv2 payloads and IDs Uniform so they Might be Indistinguishable from Future Protocols'
2 participants