Improve readability of test_vectors.csv

[siv2r]

The CSV file's test type and result columns are ambiguous, especially when we test the adapt and extract_secadaptor APIs. For instance,

• What does TRUE or FALSE represent for test type Adapting & Extract Adaptor?
  • adapt, and extract_secadapt APIs don’t output boolean values
• The reader has to dig through the code to find their meaning
  • For Adapting, sig64 == sig_actual
    • Shouldn't we instead use schnorr_verify to verify the output of the adapt API?
  • For Extract Adaptor, t == adaptor

Side Note 1: All the BIP340 test vectors were verified with only the schnorr_verify function. Therefore, the result column was the output of this function and didn’t need a test-type column. In our case, we require three different verification functions:

• presig_verify → tests validity of extract_adaptor result
• schnorr_verify → tests validity of adapt result
• secadaptor_verify → tests validity of extract_secadaptor result

Side Note 2: The API return types are inconsistent. extract_adaptor API (return type Optional[Point]) can return [1] Point, [2] None, or [3] False. However, extract_secadaptor & adapt will never return False. This inconsistency affects the value of the result column in the CSV table.

[siv2r]

I can think of the following solutions:

We can rename the “test type” column to “target api” which will contain the APIs that the test vectors are used for

• soln 1:
  • make adapt and extract_secadaptor return False instead of raiseValueError
  • keep the result column unchanged
  • Pro: easy to implement
  • Cons: Confusing API return values
• soln 2:
  • make extract_adaptor return [isValid, Point] type instead
    • or we can use raiseValueError instead of returning False
    • or leave the return type as it is
  • the values of the result column would represent output values (VALID/INVALID) of the APIs present in the “target api” column
    • maybe add a side note (in the CSV file) about three different types of verification to better explain VALID/INVALID
  • Pros: test vectors linked to APIs, the user doesn’t need to look at Python code
  • Cons:
• soln 3:
  • implement auxiliary APIs presig_verify, schnorr_verify, and secadaptor_verify in reference.py
    • These API names will be values of the “target api” column
    • Their output will be the values of the “result” column
  • Pros: Detailed
  • Cons:
    • user needs to read the auxiliary API code to understand the CSV file
    • CSV file contains auxiliary API name (in "target api" column) rather than main APIs (presign, adapt, etc.)
• soln 4:
  • don't change anything. Just add note in the CSV file for clarity.

I am leaning toward soln 2.

@jonasnick @ZhePang What do you guys suggest?

[jonasnick]

Side Note 2: The API return types are inconsistent.

Yeah, there are multiple places where types are wrong:

❯ mypy test-vector.py
reference.py:143: error: Incompatible types in assignment (expression has type "tuple[int, int]", variable has type "bytes")  [assignment]
reference.py:144: error: Argument 2 to "point_add" has incompatible type "bytes"; expected "tuple[int, int] | None"  [arg-type]
reference.py:145: error: Argument 1 to "has_even_y" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
reference.py:148: error: Argument 1 to "bytes_from_point" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
reference.py:149: error: Argument 1 to "parity_from_point" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
reference.py:149: error: Argument 1 to "bytes_from_point" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
reference.py:151: error: Argument 2 to "schnorr_pre_verify" has incompatible type "bytes"; expected "tuple[int, int]"  [arg-type]
reference.py:175: error: Incompatible return value type (got "bool", expected "tuple[int, int] | None")  [return-value]
reference.py:179: error: Incompatible return value type (got "bool", expected "tuple[int, int] | None")  [return-value]
reference.py:184: error: Incompatible return value type (got "bool", expected "tuple[int, int] | None")  [return-value]
reference.py:194: error: Incompatible return value type (got "bool", expected "tuple[int, int] | None")  [return-value]
reference.py:295: error: Incompatible types in assignment (expression has type "tuple[int, int]", variable has type "bytes")  [assignment]
reference.py:296: error: Argument 2 to "schnorr_pre_verify" has incompatible type "bytes"; expected "tuple[int, int]"  [arg-type]
reference.py:368: error: Argument 1 to "compress_point" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
reference.py:403: error: Missing return statement  [return]
reference.py:413: error: Argument 1 to "compress_point" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
reference.py:427: error: Argument 1 to "compress_point" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
test-vector.py:154: error: Argument 1 to "compress_point" has incompatible type "tuple[int, int] | None"; expected "tuple[int, int]"  [arg-type]
Found 18 errors in 2 files (checked 1 source file)

[jonasnick]

I agree that extract_t, adapt and extract_adaptor should not raise Errors except for the length of the inputs.

Side note: We should rename the functions and variables to be more understandable, similar what we've done in the C implementation.

We could also have separate CSV files for testing different API functions with sensible columns for the specific API tested.

[siv2r]

We could also have separate CSV files for testing different API functions with sensible columns for the specific API tested.

I like this idea. This would simplify things.

How does four CSV files (one for each API) sound?

Presign

seckey	pubkey	message	aux_rand	adaptor	pre-signature	result	comment

The result column is AND of the following boolean values:

• _pre-signature == adaptor_presign(sk, msg, adaptor, auxrand)
• _adaptor_presigverify(pre-signature, adaptor, msg, pubkey) == TRUE

---

Extract Adaptor

pubkey	message	pre-signature	adaptor	result	comment

The result column is: _adaptor == extractadaptor(pre-signature, msg, pubkey)

---

Adapt

pre-signature	secadaptor	bip340 signature	result	comment

The result column is: _schnorrverify(adapt(pre-signature, secadaptor))

---

Extract Secadaptor

pre-signature	bip340 signature	secadaptor	result	comment

The result column is: _secadaptor == extractsecadaptor(pre-signature, bip340 signature)

[siv2r]

How does four CSV files (one for each API) sound?

We can also combine them into two CSV files instead: [1] Presign & Extract Adaptor [2] Adapt & Extract Secadaptor, as their columns are similar.

[siv2r]

Did this issue close automatically after #8 merged? If so, we need to reopen it.

Initially, I had planned to fix this issue in the #8 pull request, but I decided against it to keep the PR brief. I will create a separate pull request for this.

[ZhePang]

Yeah I think it automatically closed it. I'll reopen it.