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

Sighash taproot keyspend bug fix #1941

Merged
merged 3 commits into from
Jan 25, 2023

Conversation

Roasbeef
Copy link
Member

In this PR, we fix a bug in RawTxInTaprootSignature that would cause
the function to not properly apply the sighash flag for non-default
sighash signatures. The logic would end up applying 0x00 as a mask,
which will always be 0x00 on the other end.

The RawTxInTapscriptSignature function was correct, though it had the
ordering switched as it applies the sighash if the type doesn't equal
default.

The second commit adds tests that fail w/o the first commit. Without
the first commit, the test fails for the keyspend sigs as they're always
64 bytes, since the sighash wasn't added for non-default types.

@coveralls
Copy link

coveralls commented Jan 24, 2023

Pull Request Test Coverage Report for Build 4002408802

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 55.248%

Totals Coverage Status
Change from base Build 3968292573: 0.08%
Covered Lines: 26610
Relevant Lines: 48165

💛 - Coveralls

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice find!

Looks good, just one comment about adding PayToTaprootScript to txscript, as there are several occurrences of it in other code (lnd, taro) and even two different approaches in the unit tests here.

txscript/sign_test.go Outdated Show resolved Hide resolved
txscript/sign_test.go Outdated Show resolved Hide resolved
txscript/sign_test.go Show resolved Hide resolved
In this commit, we fix a bug in RawTxInTaprootSignature that would cause
the function to not properly apply the sighash flag for non-default
sighash signatures. The logic would end up applying `0x00` as a mask,
which will always be `0x00` on the other end.

The RawTxInTapscriptSignature function was correct, though it had the
ordering switched as it applies the sighash if the type doesn't equal
default.
…ignature

In this commit, we add tests for the public functions used to generate
keyspend and tapscript signatures. Without the prior commit, these tests
will fail as the keyspend function won't properly add the sighash bytes
for things that aren't sighash default.
@Roasbeef Roasbeef force-pushed the sighash-taproot-keyspend-bug-fix branch from ec7b2af to d6efaa7 Compare January 25, 2023 02:47
@Roasbeef Roasbeef merged commit be056b0 into btcsuite:master Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants