Skip to content

Fix is_invalid_use_of_sighash_single() incompatibility with Bitcoin Core #4113

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 25, 2025
Merged

Fix is_invalid_use_of_sighash_single() incompatibility with Bitcoin Core #4113

merged 2 commits into from
Feb 25, 2025

Conversation

liuchengxu
Copy link
Contributor

Close #4112

@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Feb 24, 2025
@coveralls
Copy link

coveralls commented Feb 24, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 13512629499

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 82.725%
Totals Coverage Status
Change from base Build 13508562958: 0.005%
Covered Lines: 21195
Relevant Lines: 25621
💛 - Coveralls

@liuchengxu liuchengxu force-pushed the fix-sighash-single-bug branch from 510d0ac to bf5d00e Compare February 24, 2025 16:03
Kixunil
Kixunil previously approved these changes Feb 24, 2025
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK bf5d00e

tcharding
tcharding previously approved these changes Feb 24, 2025
View reviewed changes
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.

ACK bf5d00e

@liuchengxu liuchengxu dismissed stale reviews from tcharding and Kixunil via ce29c18 February 24, 2025 23:41
@liuchengxu liuchengxu force-pushed the fix-sighash-single-bug branch from bf5d00e to ce29c18 Compare February 24, 2025 23:41
@liuchengxu
Copy link
Contributor Author

The test is now added in a separate commit, otherwise, nothing has been changed since the last review.

@tcharding
Copy link
Member

tcharding commented Feb 25, 2025 •
edited
Loading

Thank for making the effort to split them up.

The test patch and fix are the wrong way around. Every patch should build and test cleanly. The idea is we put the test second but then during review a reviewer can rearrange the patch if they want and verify that it fails without the fix.

(The 'every-patch-builds-and-tests' is not enforced by GitHub CI but @apoelstra runs additional CI checks that verify it.)

@liuchengxu liuchengxu force-pushed the fix-sighash-single-bug branch from ce29c18 to b07c301 Compare February 25, 2025 02:54
@liuchengxu
Copy link
Contributor Author

@tcharding Thanks for the clarification. I misread the comment from @Kixunil. Rebased to put the test after the fix.

@liuchengxu liuchengxu force-pushed the fix-sighash-single-bug branch from b07c301 to 7ab2f5b Compare February 25, 2025 02:56
tcharding
tcharding approved these changes Feb 25, 2025
View reviewed changes
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.

ACK 7ab2f5b

@tcharding
Copy link
Member

Verified that the test fails if put up front. Thanks @liuchengxu!

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 25, 2025

BTW I was thinking we could have a special marker in the commit message for tests that test fixes in previous commits. Then CI could do the rearranging and testing.

Kixunil
Kixunil approved these changes Feb 25, 2025
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 7ab2f5b

apoelstra
apoelstra approved these changes Feb 25, 2025
View reviewed changes
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 7ab2f5b; successfully ran local tests

@apoelstra apoelstra merged commit e80ce4a into rust-bitcoin:master Feb 25, 2025
24 checks passed
@Kixunil
Copy link
Collaborator

Kixunil commented Feb 25, 2025

@liuchengxu do you want to open the backport PR as well?

@tcharding tcharding added the port-0.32.x Needs porting to 0.32.x branch label Feb 25, 2025
@tcharding tcharding mentioned this pull request Feb 25, 2025
Merged
@tcharding
Copy link
Member

Backported in #4122

@tcharding
Copy link
Member

tcharding commented Feb 25, 2025 •
edited
Loading

@liuchengxu do you want to open the backport PR as well?

I'm happy to do backports as needed. Its boring maintainer work, I don't think we should ask external contributors to do it.

apoelstra added a commit that referenced this pull request Feb 28, 2025
@apoelstra
Merge #4122: backport: Fix is_invalid_use_of_sighash_single() incom…
2c97c79 
…patibility with Bitcoin Core

18c2cad Add test for sighash_single_bug incompatility fix (Liu-Cheng Xu)
068c3f2 Fix `is_invalid_use_of_sighash_single()` incompatibility with Bitcoin Core (Liu-Cheng Xu)

Pull request description:

  Backport #4113

  Cherry-picked the 2 commits and manually fixed merge conflicts.

ACKs for top commit:
  apoelstra:
    ACK 18c2cad; successfully ran local tests

Tree-SHA512: 9ddc3950e6f35c1fb6d44156a26a8111fe73e6189b1e6c57464173671ce1fab95558485844f72abb33f632b5015089733f3a0e4acdb11005c06c58d466a63706
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
C-bitcoin PRs modifying the bitcoin crate port-0.32.x Needs porting to 0.32.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

is_invalid_use_of_sighash_single() incompatibility with Bitcoin Core
5 participants
@liuchengxu @coveralls @tcharding @Kixunil @apoelstra