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

Rearrange tests for XOnlyPublicKey::from_slice #785

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

bigspider
Copy link
Contributor

@bigspider bigspider commented Mar 12, 2025

Adds two test cases for parsing an invalid key (despite it being 32-byte long).

Perhaps the test for ZERO is redundant, as 7 also has no square root, but it felt natural to add it anyway.

@apoelstra
Copy link
Member

apoelstra commented Mar 12, 2025

The CI failures are because when you compile with cfg secp256k1_fuzz, these do parse correctly, since with fuzzer mode on we don't actually parse public keys; we parse a key-like object. (Where "key-like" means "parses super fast and behaves enough like a key that it doesn't crash downstream stuff.)

I think the best solution is to add these tests as a new unit test with #[cfg(not(secp256k1_fuzz))] marked.

@bigspider
Copy link
Contributor Author

Actually, I just saw that there is a test_pubkey_from_bad_slice a few tests below, so I think my PR is pointless.
It might be worth moving them next to each other, and perhaps delete a few redundant test cases.

@bigspider bigspider force-pushed the from_slice_missing_test branch from 0754c66 to a4cb345 Compare March 12, 2025 15:02
@bigspider
Copy link
Contributor Author

bigspider commented Mar 12, 2025

I did the slight refactor (without deleting any test) - feel free to merge or close as you please.

@bigspider bigspider changed the title Add invalid key testcases for XOnlyPublicKey::from_slice Rearrange tests for XOnlyPublicKey::from_slice Mar 13, 2025
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK a4cb345; successfully ran local tests; yeah, sure. I think this makes the tests a bit easier to follow

@apoelstra apoelstra merged commit b6fd763 into rust-bitcoin:master Mar 13, 2025
29 of 30 checks passed
@bigspider bigspider deleted the from_slice_missing_test branch March 13, 2025 15:45
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.

2 participants