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

ScriptSignature from P2WSH Witness #1605

Merged
merged 1 commit into from Jun 24, 2020

Conversation

nkohen
Copy link
Collaborator

@nkohen nkohen commented Jun 24, 2020

Added method to pull script signature out of P2WSH witness

@nkohen nkohen added the core work for the core project label Jun 24, 2020
Copy link
Collaborator

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

ACK on CI passing

@benthecarman benthecarman merged commit 24f60c0 into bitcoin-s:master Jun 24, 2020
@@ -106,6 +110,21 @@ sealed abstract class P2WSHWitnessV0 extends ScriptWitnessV0 {
relevantStack.map(ECDigitalSignature.fromBytes)
}

// Note that this is not guaranteed to work for non-standard script signatures
lazy val scriptSignature: ScriptSignature = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an Option[ScriptSignature]?

This throws on the case where the stack is empty (.tail)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upon further evaluation, I believe that this actually does work in all cases, and the thing that doesn't work is the apply method below which takes in a ScriptSignature (which will fail to act as expected if there's anything other than push ops). So no Option, but you're right that we should require that there be a redeem script (i.e. stack.nonEmpty) at the top of this trait

lazy val scriptSignature: ScriptSignature = {
val asm = stack.toVector.tail.reverse.flatMap { bytes =>
if (bytes.isEmpty) {
Vector(OP_0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why OP_0 if there is nothing on the stack?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not nothing on the stack, an empty element on the stack (a Script zero)

val asm = stack.toVector.tail.reverse.flatMap { bytes =>
if (bytes.isEmpty) {
Vector(OP_0)
} else if (bytes.length == 1 && bytes.head <= 16 && bytes.head >= -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just

val numOpt = bytes.headOption.flatMap(byte => ScriptNumberOperation.fromNumber(byte))

...

else if (numOpt.isDefined)) { 
  Vector(numOpt.get)
} 
...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I personally like how it is because it clearly enumerates all cases

Christewart pushed a commit that referenced this pull request May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core work for the core project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants