Failure to correctly type P2PKHScriptSignatures

[Christewart]

Problem

Every P2PKHScriptSignature should follow the following format:

<sig> <pubkey>

Currently we have a naive typing algorithm for classifying P2PKHScriptSignature. The algorithm is defined here

  /** Determines if the given asm matches a [[P2PKHScriptSignature]] */
  def isP2PKHScriptSig(asm: Seq[ScriptToken]): Boolean = asm match {
    case List(w : BytesToPushOntoStack, x : ScriptConstant, y : BytesToPushOntoStack,
      z : ScriptConstant) =>
      //TODO: We need to change this to use CPubKey::IsFullyValid() when we integrate libsecp256k1
      //this checks to make sure we do not have a redeem script as the 'z' constant
      //for instance, we can have a p2sh scriptsignature that has a redeem script for a p2pkhScriptSig
      //and it will have the same format as the pattern match before
      !P2SHScriptSignature.isRedeemScript(z)
    case _ => false
}

as you can see, we pattern match on the given asm tokens and see if we have a script signature of the format <pushop for sig> <sig> <pushop for pubkey> <pubkey>

This works in 99% of cases, except when we have a P2SHScriptSignature which is required to only be pushops. We can nest a P2PKScriptPubKey inside of a P2SHScriptSignature to have the exact same format as above. The P2SHScriptSignature looks like:

<pushop for sig> <sig> <pushop for redeemScript> <P2PK redeemScript>

This causes isP2PKHScriptSig to return false, which is the cause for this travis failure:

https://travis-ci.org/bitcoin-s/bitcoin-s-core/builds/201652191

Solution

The most obvious solution is after we add libsecp256k1 we add a check inside of isP2PKHScriptSig to detect if the constant z is a valid public key. If it is a valid public key, we assume that the given asm is not a P2PKHScriptSignature

Bitcoin Core relies on libsecp256k1 functionality to determine if a pubkey is valid or not, I suggest using that same check inside of isValidP2PKHScriptSig once we integrate libsecp256k1 which should solve our problem. Depends on #50

[Christewart]

fixed in #61

[Christewart]

This issue still isn't resolved, just had a build fail, see:

https://travis-ci.org/bitcoin-s/bitcoin-s-core/builds/207431219#L2767