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

Add custom Debug impl for RecoverableSignature #393

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

tcharding
Copy link
Member

Currently when debug printing the RecoverableSignature we do so byte by byte, this means that the output differs depending on the endianess of the machine. If instead we serialize the signature in compact form then the output is the same irrespective of the endianess.

With this applied the following two commands now pass:

cargo test test_debug_output --features=recovery
cross test --target powerpc-unknown-linux-gnu test_debug_output --features=recovery

Fixes: #375

Currently when debug printing the `RecoverableSignature` we do so byte
by byte, this means that the output differs depending on the endianess
of the machine. If instead we serialize the signature in compact form
then the output is the same irrespective of the endianess.

With this applied the following two commands now pass:

```
cargo test test_debug_output --features=recovery

```
cross test --target powerpc-unknown-linux-gnu test_debug_output --features=recovery
```

Fixes: rust-bitcoin#375
@tcharding
Copy link
Member Author

We have various other users of impl_raw_debug that do not have serialization/debug output roundtrip tests e.g., Signature. I can't work out if this is a problem right now so flagging it.

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 3c9dd2f

Awesome, thanks! We've really gotta get cross-testing into CI..

@apoelstra
Copy link
Member

And yes, I believe this is also a problem with Signature at least.

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 4c43d5e

(The previous commit was for 392, not 393. You'd really think Github would flag this..)

@apoelstra apoelstra merged commit bc278fa into rust-bitcoin:master Feb 9, 2022
@tcharding
Copy link
Member Author

Good job we don't merge with bot when acks come in then :)

@tcharding tcharding deleted the debug-recoverable-sig branch February 11, 2022 08:37
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.

RecoverableSignature relies on libsecp internal byte representation and breaks in BE
2 participants