lnwallet: add new concrete error InvalidHtlcSigError for failed htlc sig validation

In this commit we add a new error: InvalidHtlcSigError. This error will
be returned when we're unable to validate an HTLC signature sent by the
remote party. This will allow other nodes to more easily debug _why_ the
signature was rejected.

lnwallet/channel.go

@@ -1881,8 +1881,9 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64,
 	if err != nil {
 		return nil, err
 	}
-	commitmentSecret, commitmentPoint := btcec.PrivKeyFromBytes(btcec.S256(),
+	commitmentSecret, commitmentPoint := btcec.PrivKeyFromBytes(
-		revocationPreimage[:])
+		btcec.S256(), revocationPreimage[:],
+	)
 
 	// With the commitment point generated, we can now generate the four
 	// keys we'll need to reconstruct the commitment state,
@@ -1893,8 +1894,9 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64,
 	// number so we can have the proper witness script to sign and include
 	// within the final witness.
 	remoteDelay := uint32(chanState.RemoteChanCfg.CsvDelay)
-	remotePkScript, err := commitScriptToSelf(remoteDelay, keyRing.DelayKey,
+	remotePkScript, err := commitScriptToSelf(
-		keyRing.RevocationKey)
+		remoteDelay, keyRing.DelayKey, keyRing.RevocationKey,
+	)
 	if err != nil {
 		return nil, err
 	}
@@ -1999,10 +2001,10 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64,
 				return nil, err
 			}
 
-			// Otherwise, is this was an outgoing HTLC that we sent, then
-			// from the PoV of the remote commitment state, they're the
-			// receiver of this HTLC.
 		} else {
+			// Otherwise, is this was an outgoing HTLC that we
+			// sent, then from the PoV of the remote commitment
+			// state, they're the receiver of this HTLC.
 			htlcScript, err = receiverHTLCScript(
 				htlc.RefundTimeout, keyRing.LocalHtlcKey,
 				keyRing.RemoteHtlcKey, keyRing.RevocationKey,
@@ -2582,9 +2584,11 @@ func genRemoteHtlcSigJobs(keyRing *CommitmentKeyRing,
 			Hash:  txHash,
 			Index: uint32(htlc.remoteOutputIndex),
 		}
-		sigJob.tx, err = createHtlcTimeoutTx(op, outputAmt,
+		sigJob.tx, err = createHtlcTimeoutTx(
-			htlc.Timeout, uint32(remoteChanCfg.CsvDelay),
+			op, outputAmt, htlc.Timeout,
-			keyRing.RevocationKey, keyRing.DelayKey)
+			uint32(remoteChanCfg.CsvDelay),
+			keyRing.RevocationKey, keyRing.DelayKey,
+		)
 		if err != nil {
 			return nil, nil, err
 		}
@@ -2632,9 +2636,10 @@ func genRemoteHtlcSigJobs(keyRing *CommitmentKeyRing,
 			Hash:  txHash,
 			Index: uint32(htlc.remoteOutputIndex),
 		}
-		sigJob.tx, err = createHtlcSuccessTx(op, outputAmt,
+		sigJob.tx, err = createHtlcSuccessTx(
-			uint32(remoteChanCfg.CsvDelay), keyRing.RevocationKey,
+			op, outputAmt, uint32(remoteChanCfg.CsvDelay),
-			keyRing.DelayKey)
+			keyRing.RevocationKey, keyRing.DelayKey,
+		)
 		if err != nil {
 			return nil, nil, err
 		}
@@ -3483,20 +3488,23 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment,
 	i := 0
 	for index := range localCommitmentView.txn.TxOut {
 		var (
-			sigHash func() ([]byte, error)
+			htlcIndex uint64
-			sig     *btcec.Signature
+			sigHash   func() ([]byte, error)
-			err     error
+			sig       *btcec.Signature
+			err       error
 		)
 
 		outputIndex := int32(index)
 		switch {
 
-		// If this output index is found within the incoming HTLC index,
+		// If this output index is found within the incoming HTLC
-		// then this means that we need to generate an HTLC success
+		// index, then this means that we need to generate an HTLC
-		// transaction in order to validate the signature.
+		// success transaction in order to validate the signature.
 		case localCommitmentView.incomingHTLCIndex[outputIndex] != nil:
 			htlc := localCommitmentView.incomingHTLCIndex[outputIndex]
 
+			htlcIndex = htlc.HtlcIndex
+
 			sigHash = func() ([]byte, error) {
 				op := wire.OutPoint{
 					Hash:  txHash,
@@ -3547,6 +3555,8 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment,
 		case localCommitmentView.outgoingHTLCIndex[outputIndex] != nil:
 			htlc := localCommitmentView.outgoingHTLCIndex[outputIndex]
 
+			htlcIndex = htlc.HtlcIndex
+
 			sigHash = func() ([]byte, error) {
 				op := wire.OutPoint{
 					Hash:  txHash,
@@ -3598,9 +3608,10 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment,
 		}
 
 		verifyJobs = append(verifyJobs, verifyJob{
-			pubKey:  keyRing.RemoteHtlcKey,
+			htlcIndex: htlcIndex,
-			sig:     sig,
+			pubKey:    keyRing.RemoteHtlcKey,
-			sigHash: sigHash,
+			sig:       sig,
+			sigHash:   sigHash,
 		})
 
 		i++
@@ -3617,7 +3628,7 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment,
 }
 
 // InvalidCommitSigError is a struct that implements the error interface to
-// report a failure to validation a commitment signature for a remote peer.
+// report a failure to validate a commitment signature for a remote peer.
 // We'll use the items in this struct to generate a rich error message for the
 // remote peer when we receive an invalid signature from it. Doing so can
 // greatly aide in debugging cross implementation issues.
@@ -3635,7 +3646,7 @@ type InvalidCommitSigError struct {
 // caused an invalid commitment signature.
 func (i *InvalidCommitSigError) Error() string {
 	return fmt.Sprintf("rejected commitment: commit_height=%v, "+
-		"invalid_sig=%x, commit_tx=%x, sig_hash=%x", i.commitHeight,
+		"invalid_commit_sig=%x, commit_tx=%x, sig_hash=%x", i.commitHeight,
 		i.commitSig[:], i.commitTx, i.sigHash[:])
 }
 
@@ -3643,6 +3654,35 @@ func (i *InvalidCommitSigError) Error() string {
 // error interface.
 var _ error = (*InvalidCommitSigError)(nil)
 
+// InvalidCommitSigError is a struc that implements the error interface to
+// report a failure to validate an htlc signature from a remote peer. We'll use
+// the items in this struct to generate a rich error message for the remote
+// peer when we receive an invalid signature from it. Doing so can greatly aide
+// in debugging across implementation issues.
+type InvalidHtlcSigError struct {
+	commitHeight uint64
+
+	htlcSig []byte
+
+	htlcIndex uint64
+
+	sigHash []byte
+
+	commitTx []byte
+}
+
+// Error returns a detailed error string including the exact transaction that
+// caused an invalid htlc signature.
+func (i *InvalidHtlcSigError) Error() string {
+	return fmt.Sprintf("rejected commitment: commit_height=%v, "+
+		"invalid_htlc_sig=%x, commit_tx=%x, sig_hash=%x", i.commitHeight,
+		i.htlcSig, i.commitTx, i.sigHash[:])
+}
+
+// A compile time flag to ensure that InvalidCommitSigError implements the
+// error interface.
+var _ error = (*InvalidCommitSigError)(nil)
+
 // ReceiveNewCommitment process a signature for a new commitment state sent by
 // the remote party. This method should be called in response to the
 // remote party initiating a new change, or when the remote party sends a
@@ -3772,10 +3812,30 @@ func (lc *LightningChannel) ReceiveNewCommitment(commitSig lnwire.Sig,
 		// In the case that a single signature is invalid, we'll exit
 		// early and cancel all the outstanding verification jobs.
 		select {
-		case err := <-verifyResps:
+		case htlcErr := <-verifyResps:
-			if err != nil {
+			if htlcErr != nil {
 				close(cancelChan)
-				return fmt.Errorf("invalid htlc signature: %v", err)
+
+				sig, err := lnwire.NewSigFromSignature(
+					htlcErr.sig,
+				)
+				if err != nil {
+					return err
+				}
+				sigHash, err := htlcErr.sigHash()
+				if err != nil {
+					return err
+				}
+
+				var txBytes bytes.Buffer
+				localCommitTx.Serialize(&txBytes)
+				return &InvalidHtlcSigError{
+					commitHeight: nextHeight,
+					htlcSig:      sig.ToSignatureBytes(),
+					htlcIndex:    htlcErr.htlcIndex,
+					sigHash:      sigHash,
+					commitTx:     txBytes.Bytes(),
+				}
 			}
 		case <-lc.quit:
 			return fmt.Errorf("channel shutting down")

lnwallet/sigpool.go

@@ -41,6 +41,10 @@ type verifyJob struct {
 	// passed signature is known to have signed.
 	sigHash func() ([]byte, error)
 
+	// htlcIndex is the index of the HTLC from the PoV of the remote
+	// party's update log.
+	htlcIndex uint64
+
 	// cancel is a channel that should be closed if the caller wishes to
 	// cancel all pending verification jobs part of a single batch. This
 	// channel is to be closed in the case that a single signature in a
@@ -52,7 +56,16 @@ type verifyJob struct {
 	// is to be sent over. In the see that the signature is valid, a nil
 	// error will be passed. Otherwise, a concrete error detailing the
 	// issue will be passed.
-	errResp chan error
+	errResp chan *htlcIndexErr
+}
+
+// verifyJobErr is a special type of error that also includes a pointer to the
+// original validation job. Ths error message allows us to craft more detailed
+// errors at upper layers.
+type htlcIndexErr struct {
+	error
+
+	*verifyJob
 }
 
 // signJob is a job sent to the sigPool to generate a valid signature according
@@ -69,7 +82,8 @@ type signJob struct {
 	// proper sighash for the input to be signed.
 	tx *wire.MsgTx
 
-	// outputIndex...
+	// outputIndex is the output index of the HTLC on the commitment
+	// transaction being signed.
 	outputIndex int32
 
 	// cancel is a channel that should be closed if the caller wishes to
@@ -216,7 +230,10 @@ func (s *sigPool) poolWorker() {
 			sigHash, err := verifyMsg.sigHash()
 			if err != nil {
 				select {
-				case verifyMsg.errResp <- err:
+				case verifyMsg.errResp <- &htlcIndexErr{
+					error:     err,
+					verifyJob: &verifyMsg,
+				}:
 					continue
 				case <-verifyMsg.cancel:
 					continue
@@ -229,7 +246,10 @@ func (s *sigPool) poolWorker() {
 				err := fmt.Errorf("invalid signature "+
 					"sighash: %x, sig: %x", sigHash, rawSig.Serialize())
 				select {
-				case verifyMsg.errResp <- err:
+				case verifyMsg.errResp <- &htlcIndexErr{
+					error:     err,
+					verifyJob: &verifyMsg,
+				}:
 				case <-verifyMsg.cancel:
 				case <-s.quit:
 					return
@@ -272,9 +292,9 @@ func (s *sigPool) SubmitSignBatch(signJobs []signJob) {
 // allows the caller to cancel all pending jobs in the case that they wish to
 // bail early.
 func (s *sigPool) SubmitVerifyBatch(verifyJobs []verifyJob,
-	cancelChan chan struct{}) <-chan error {
+	cancelChan chan struct{}) <-chan *htlcIndexErr {
 
-	errChan := make(chan error, len(verifyJobs))
+	errChan := make(chan *htlcIndexErr, len(verifyJobs))
 
 	for _, job := range verifyJobs {
 		job.cancel = cancelChan