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

hash_types: add unit tests for display of all hash types in the library #2528

Merged
merged 1 commit into from Mar 20, 2024

Conversation

apoelstra
Copy link
Member

This can be checked against the 0.29.x branch, and against the commit prior to #1659 (40c2467^) and you will see that it is consistent EXCEPT:

  • In rust-bitcoin 0.29.x we did not have multiple sighash types, only Sighash; we now have LegacySighash, SegwitV0Sighash, and TapSighash.
  • In Reduce usage of FromHex #1565 we deliberately changed the display direction of the sighashes, to match BIP 143.

Fixes #2495.

This can be checked against the 0.29.x branch, and against the commit
prior to rust-bitcoin#1659 (40c2467^) and you will see that it is consistent
EXCEPT:

* In rust-bitcoin 0.29.x we did not have multiple sighash types, only
  `Sighash`; we now have `LegacySighash`, `SegwitV0Sighash`, and
  `TapSighash`.
* In rust-bitcoin#1565 we deliberately changed the display direction of the
  sighashes, to match BIP 143.

Fixes rust-bitcoin#2495.
@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Feb 29, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8101066712

Details

  • 67 of 67 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 83.904%

Totals Coverage Status
Change from base Build 8097822909: 0.05%
Covered Lines: 19527
Relevant Lines: 23273

💛 - Coveralls

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

That's a win. ACK b816c0b

@tcharding tcharding added this to the 0.32.0 milestone Mar 14, 2024
@tcharding
Copy link
Member

This an go in by way of the carve out, its been open and ack'd for more than 2 weeks.

@apoelstra
Copy link
Member Author

Sounds good. And you could argue that adding unit tests is "CI only".

@apoelstra apoelstra merged commit 4d90e0b into rust-bitcoin:master Mar 20, 2024
187 checks passed
@apoelstra apoelstra deleted the 2024-02--hash-order branch March 20, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bitcoin PRs modifying the bitcoin crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We may have hash printing backwards wrong for some of the hashes
3 participants