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

fix/secp256k1 #99

Merged
merged 10 commits into from
Oct 26, 2023
Merged

fix/secp256k1 #99

merged 10 commits into from
Oct 26, 2023

Conversation

mistermoe
Copy link
Contributor

  • Account for signatures less than 64 bytes
  • add test
  • bump version

mistermoe and others added 4 commits October 26, 2023 00:53
Co-Authored-By: Tom Daffurn <[email protected]>
Co-Authored-By: Tom Daffurn <[email protected]>
Co-Authored-By: Tom Daffurn <[email protected]>
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #99 (e2d226f) into main (6ceafc5) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
+ Coverage   69.92%   70.10%   +0.18%     
==========================================
  Files          21       21              
  Lines        1197     1201       +4     
  Branches      135      137       +2     
==========================================
+ Hits          837      842       +5     
  Misses        292      292              
+ Partials       68       67       -1     
Components Coverage Δ
credentials 67.12% <ø> (ø)
crypto 34.67% <100.00%> (+1.33%) ⬆️
dids 92.81% <ø> (ø)
common 60.83% <ø> (ø)

* @param size The expected fixed length of the resulting byte array.
* @return A [ByteArray] of size [size], representing the [BigInteger] value.
*/
private fun BigInteger.toFixedByteArray(size: Int): ByteArray {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL you can private a function outside of a class in Kotlin

}

@Test
fun fuzz() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fun fuzz() {
fun `can verify 10k random payloads`() {

or whatevs, not sure what the intent here is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

point here is to generate a sufficient number of signatures (aka 2 big ints) across a large enough surface area to suss out any edge cases there may be with signature verification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool with "pressure test signature verification" as the test name?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done - 8049d78

Copy link
Contributor

@andresuribe87 andresuribe87 left a comment

Choose a reason for hiding this comment

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

lgtm, smol suggestion

Would have been awesome to identify a test that fails before, but passes now.

crypto/src/test/kotlin/web5/sdk/crypto/Secp256k1Test.kt Outdated Show resolved Hide resolved
@mistermoe
Copy link
Contributor Author

lgtm, smol suggestion

Would have been awesome to identify a test that fails before, but passes now.

good point @andresuribe87 . will keep this in mind for future PRs! less valuable now but for context:

we were running into intermittent unit test failures on the tbdex side w.r.t. signature verification e.g.

OfferingTest > can parse offering from a json string() FAILED
    java.security.SignatureException: Invalid Signature
        at web5.sdk.crypto.Secp256k1.verify(Secp256k1.kt:280)
        at web5.sdk.crypto.Signer$DefaultImpls.verify$default(Signer.kt:115)
        at web5.sdk.crypto.Crypto.verify(Crypto.kt:144)

@tomdaffurn and i narrowed it down to secp256k1 and wrote the test that we added to this PR to consistently reproduce the issue. we used the failing test to track down and fix the bug.

is this the type of context you were suggesting might be helpful?

Co-authored-by: Andres Uribe <[email protected]>
@andresuribe87
Copy link
Contributor

@mistermoe I meant more to include a test in this PR that always failed before your fix, but passes after the fix. Sounds like you kinda had it? No big deal.

Context was super useful, thx!

@mistermoe mistermoe merged commit 934eb70 into main Oct 26, 2023
4 checks passed
@mistermoe mistermoe deleted the fix/secp256k1 branch October 26, 2023 19:13
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.

4 participants