Skip to content

Conversation

@AadityaKhurana
Copy link
Contributor

  • Added support for creating a PublicKey object from a message and its signature.
  • Supports only Compact signatures, as DER signatures lack the recovery ID, which is essential for public key recovery.

Copy link
Contributor

@JAGADISHSUNILPEDNEKAR JAGADISHSUNILPEDNEKAR left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this feature! I've reviewed the changes to add public key recovery from message and signature. Overall, the implementation looks solid with proper handling of the recovery ID and message magic prefix.

Some suggestions for improvement:

  1. Documentation: Consider adding more detailed comments explaining the Bitcoin message signing process, especially around the recovery ID extraction (signature[0] - 31) and message magic prefix.

  2. Error Handling: The error message "Parameters missing" could be more specific about what parameters are required when neither hex_str nor message+signature are provided.

  3. Tests: Please add unit tests that verify this functionality works correctly with various test vectors. Testing should include:

    • Recovery from known message/signature pairs
    • Error handling for invalid signatures
    • Edge cases like empty messages
  4. Code Formatting: There are some indentation inconsistencies in the diff that should be cleaned up before merging.

  5. Documentation Update: The docstring should explain the format of the expected signature (65 bytes with recovery ID).

I believe this is a valuable addition to the library. Once these items are addressed, It would be easy for @karask to implement this I guess!

@AadityaKhurana
Copy link
Contributor Author

Thank you for the thorough review and valuable feedback!
I've addressed the suggested improvements

Copy link
Contributor

@JAGADISHSUNILPEDNEKAR JAGADISHSUNILPEDNEKAR left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all the feedback so quickly and thoroughly! The changes look great:

  • The improved documentation clearly explains the signature format and recovery process
  • The error handling is now more robust with specific error messages
  • The comprehensive test suite covers all the key scenarios
  • The code is well-formatted and commented

I'm approving this PR from my end so that it's easy for @karask to implement this as it now provides a well-tested, properly documented implementation of public key recovery from message signatures.

@karask karask merged commit 9da3c3d into karask:master Mar 17, 2025
@karask
Copy link
Owner

karask commented Mar 17, 2025

Thank you @AadityaKhurana and @JAGADISHSUNILPEDNEKAR for reviewing.

I noticed that you used tabs. Please use spaces instead in the future. There are settings in the editor that you are using to use, e.g. 4 spaces, every time you press tab.

Thanks

JAGADISHSUNILPEDNEKAR added a commit to JAGADISHSUNILPEDNEKAR/python-bitcoin-utils that referenced this pull request Mar 18, 2025
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.

3 participants