-
Notifications
You must be signed in to change notification settings - Fork 17
Add JSON Test Vectors #90
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
base: master
Are you sure you want to change the base?
Conversation
- Add GE.from_bytes_with_infinity method - In encpedpop, unpack simplpedpop's error message from tuple - Temporarily use zeroed 32-byte array for aux_rand arg of schnorr_sign See: BlockstreamResearch#86
outputs one JSON file per API in `python/vectors` folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you! This looks like "design 3" as I had it in mind.
I was surprised, shouldn't this be based on #88? I think the test vectors should contain the messages in their serialized form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting a status update, still reviewing...
"cmsg1": cmsg1_asdict(cmsg1), | ||
"pmsgs2": [pmsg2_asdict(m) for m in invalid_pmsgs2], | ||
"error": error, | ||
"comment": "invalid certificate length", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong comment
"pmsg1": pmsg1_asdict(pmsgs1[0]), | ||
"cmsg1": cmsg1_asdict(invalid_cmsg1), | ||
"error": error, | ||
"comment": "coms_to_secret list in cmsg1 has an invalid value at index 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong comment
"pmsg1": pmsg1_asdict(pmsgs1[0]), | ||
"cmsg1": cmsg1_asdict(invalid_cmsg1), | ||
"error": error, | ||
"comment": "coms_to_secret list in cmsg1 has an invalid value at index 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong comment
def generate_hostpubkey_vectors(): | ||
vectors = {"valid_test_cases": [], "error_test_cases": []} | ||
|
||
# --- Valid test case 1 --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we start numbering with 0? The test cases are stored in a list, where the conventional first index is 0.
rand_encshare = bytes.fromhex( | ||
"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rand_encshare doesn't look rand(om). Should be invalid_encshare?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, thanks.
"hostseckey": None, | ||
"recovery_data": bytes_to_hex(invalid_crec), | ||
"error": error, | ||
"comment": "invalid threshold", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong comment
# --- Error test case 4: invalid threshold --- | ||
invalid_crec = b"\x00" * 4 + crec[4:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# --- Error test case 4: invalid threshold --- | |
invalid_crec = b"\x00" * 4 + crec[4:] | |
# --- Error test case 4: invalid threshold --- | |
invalid_crec = b"\x00" * 4 + crec[4 + 33 * t:] |
Otherwise, recover will fail due to a wrong length of the recovery data, not because the threshold is wrong in params_validate
(which presumably was the intention).
And we need to define t
t = 2
params = chilldkg.SessionParams(hostpubkeys, t)
at the top of the function.
"hostseckey": None, | ||
"recovery_data": bytes_to_hex(invalid_crec), | ||
"error": error, | ||
"comment": "last pubnonce in the pubnonces list is invalid", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is wrong or at least misleading. The pubnonce is not invalid by itself, but recover
fails because this pubnonce hasn't been signed. Which one did you want to test?
"comment": "last pubnonce in the pubnonces list is invalid", | ||
} | ||
) | ||
# --- Error test case 6: last signature in the certificate is invalid --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate test case number
"comment": "coms_to_secret list in cmsg1 has an invalid value at index 0", | ||
} | ||
) | ||
# --- Error Test Case 4: pop list in cmsg1 has an invalid value at index 1 --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate test case number
Thanks for the review. I'll address your comments soon.
I assumed we were favoring design 2 over design 1. That is why the JSON representation of messages shows internal fields (for better readability) rather than a single byte array. The definitions of design 1 and design 2 are in this comment as:
I can switch the message JSON to a single hex-encoded byte array if you prefer. |
I had thought design 1 vs. design 2 was mostly about representing the state objects. As for the messages that are passed between the participants, I believe it's better to include them in the test vectors in their serialized form. This way, the byte encoding is tested as well, which is an important part of the spec. |
"comment": "participant 1 sent an invalid secshare for participant 0", | ||
} | ||
) | ||
# --- Error Test Case 2: Coordinator tampers with participant 0's encrypted secshare --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# --- Error Test Case 2: Coordinator tampers with participant 0's encrypted secshare --- | |
# --- Error Test Case 2: Coordinator tampered with participant 0's encrypted secshare --- |
we use past tense everywhere else
"cmsg1": cmsg1_asdict(invalid_cmsg1), | ||
"cinv_msg": cinv_msg_asdict(cinv_msgs[0]), | ||
"error": error, | ||
"comment": "coordinator tampers with participant 0's encrypted secshare", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"comment": "coordinator tampers with participant 0's encrypted secshare", | |
"comment": "coordinator tampered with participant 0's encrypted secshare", |
} | ||
) | ||
|
||
# --- Error test case 1: Wrong hostseckey length --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about wrong randomness length?
} | ||
) | ||
|
||
# --- Error Test Case 1: Wrong host secret key length --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this commented out?
# }) | ||
# --- Error Test Case 2: pubnonces list in cmsg1 has an invalid value at index 0 --- | ||
invalid_cmsg1 = copy.deepcopy(cmsg1) | ||
invalid_cmsg1.enc_cmsg.pubnonces[0] = b"\xeb" * 32 # random pubnonce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a valid pubnonce? I think if possible for every GE we should have a test where the GE is wrong and a test where the GE is infinity. For every serialized GE, we should also have a test where the byte array does not correspond to a GE. Similarly for scalars. This also applies to other test vectors in other files.
# --- Error Test Case 4: pop list in cmsg1 has an invalid value at index 1 --- | ||
invalid_cmsg1 = copy.deepcopy(cmsg1) | ||
invalid_cmsg1.enc_cmsg.simpl_cmsg.pops[1] = bytes.fromhex( | ||
"09C289578B96E6283AB13E4741FB489FC147FB1A5F446A314BA73C052131EFB04B83247A0BCEDF5205202AD64188B24B0BC5B51A17AEB218BD98DBE000C843B9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a valid signature but for the wrong public key? Please document.
"comment": "host secret key doesn't match any hostpubkey", | ||
} | ||
) | ||
# --- Error test case 3: Invalid threshold --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we order the test cases such that they roughly correspond to the location in the code where the assertions are raised (also in other files)? This would help with review. For example, the threshold is checked in params_validate
before the check for matching hostpubkeys (currently error test case 2).
import chilldkg_ref.chilldkg as chilldkg | ||
|
||
|
||
def generate_coordinator_step1_vectors(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need to test more here (which is also related to message serialization). For example, what if the VSS commitment contains invalid values or has the wrong length.
|
||
def test_hostpubkey_gen_vectors(): | ||
input_file = Path("vectors/hostpubkey_gen_vectors.json") | ||
with open(input_file) as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When run from the parent directory via python python/tests.py
this gives me an error:
FileNotFoundError: [Errno 2] No such file or directory: 'vectors/hostpubkey_gen_vectors.json'
python python/tests.py
works fine on master
invalid_pmsgs1[1].enc_pmsg.enc_shares.pop() | ||
error = expect_exception( | ||
lambda: coordinator_step1(invalid_pmsgs1, params), | ||
chilldkg.FaultyParticipantOrCoordinatorError, # REVIEW: why is this working? shouldn't it raise an error saying encpedpop.FaultyParticipantOrCoordinatorError was raised instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it raise "encpedpop.FaultyParticipantOrCoordinatorError
"? FaultyParticipantOrCoordinatorError
is defined in util.py and it's part of the chilldkg module.
Addresses issue #35.
This PR introduces functionality for generating and validating JSON test vectors for ChillDKG’s public APIs.
Summary:
python/vectors_generator/
generate one JSON vector file per API, saving them in thepython/vectors/
.chilldkg_ref
:aux_rand
value forschnorr_sign
GE.from_bytes_with_infinity
method.tests.py
to load the generated JSON vectors and verify API outputs for correctness.