From 6f0a342f92b1b4eb117632e86535249275a079dd Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 14 Jan 2020 15:07:29 +0200 Subject: [PATCH 1/6] htlcswitch: add ForwardingError constructor Add a constructor for the creation of forwarding errors. A special constructor is added for the case where we have an unknown wire failure, and must set a nil failure message. --- htlcswitch/failure.go | 30 ++++++++--- htlcswitch/mock.go | 5 +- htlcswitch/switch.go | 32 ++++-------- routing/router_test.go | 116 ++++++++++++++++++++--------------------- 4 files changed, 89 insertions(+), 94 deletions(-) diff --git a/htlcswitch/failure.go b/htlcswitch/failure.go index 0d817a07..4352d267 100644 --- a/htlcswitch/failure.go +++ b/htlcswitch/failure.go @@ -40,6 +40,27 @@ func (f *ForwardingError) Error() string { ) } +// NewForwardingError creates a new payment error which wraps a wire error +// with additional metadata. +func NewForwardingError(failure lnwire.FailureMessage, index int, + extraMsg string) *ForwardingError { + + return &ForwardingError{ + FailureSourceIdx: index, + FailureMessage: failure, + ExtraMsg: extraMsg, + } +} + +// NewUnknownForwardingError returns a forwarding error which has a nil failure +// message. This constructor should only be used in the case where we cannot +// decode the failure we have received from a peer. +func NewUnknownForwardingError(index int) *ForwardingError { + return &ForwardingError{ + FailureSourceIdx: index, + } +} + // ErrorDecrypter is an interface that is used to decrypt the onion encrypted // failure reason an extra out a well formed error. type ErrorDecrypter interface { @@ -94,15 +115,10 @@ func (s *SphinxErrorDecrypter) DecryptError(reason lnwire.OpaqueReason) ( r := bytes.NewReader(failure.Message) failureMsg, err := lnwire.DecodeFailure(r, 0) if err != nil { - return &ForwardingError{ - FailureSourceIdx: failure.SenderIdx, - }, nil + return NewUnknownForwardingError(failure.SenderIdx), nil } - return &ForwardingError{ - FailureSourceIdx: failure.SenderIdx, - FailureMessage: failureMsg, - }, nil + return NewForwardingError(failureMsg, failure.SenderIdx, ""), nil } // A compile time check to ensure ErrorDecrypter implements the Deobfuscator diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index 5c905822..828c36b9 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -400,10 +400,7 @@ func (o *mockDeobfuscator) DecryptError(reason lnwire.OpaqueReason) (*Forwarding return nil, err } - return &ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: failure, - }, nil + return NewForwardingError(failure, 1, ""), nil } var _ ErrorDecrypter = (*mockDeobfuscator)(nil) diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index b78f43b1..74276ea8 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -756,10 +756,9 @@ func (s *Switch) handleLocalDispatch(pkt *htlcPacket) error { s.indexMtx.RUnlock() if err != nil { log.Errorf("Link %v not found", pkt.outgoingChanID) - return &ForwardingError{ - FailureSourceIdx: 0, - FailureMessage: &lnwire.FailUnknownNextPeer{}, - } + return NewForwardingError( + &lnwire.FailUnknownNextPeer{}, 0, "", + ) } if !link.EligibleToForward() { @@ -770,11 +769,7 @@ func (s *Switch) handleLocalDispatch(pkt *htlcPacket) error { // The update does not need to be populated as the error // will be returned back to the router. htlcErr := lnwire.NewTemporaryChannelFailure(nil) - return &ForwardingError{ - FailureSourceIdx: 0, - ExtraMsg: err.Error(), - FailureMessage: htlcErr, - } + return NewForwardingError(htlcErr, 0, err.Error()) } // Ensure that the htlc satisfies the outgoing channel policy. @@ -788,10 +783,7 @@ func (s *Switch) handleLocalDispatch(pkt *htlcPacket) error { log.Errorf("Link %v policy for local forward not "+ "satisfied", pkt.outgoingChanID) - return &ForwardingError{ - FailureSourceIdx: 0, - FailureMessage: htlcErr, - } + return NewForwardingError(htlcErr, 0, "") } return link.HandleSwitchPacket(pkt) @@ -930,11 +922,7 @@ func (s *Switch) parseFailedPayment(deobfuscator ErrorDecrypter, failureMsg = lnwire.NewTemporaryChannelFailure(nil) } - return &ForwardingError{ - FailureSourceIdx: 0, - ExtraMsg: userErr, - FailureMessage: failureMsg, - } + return NewForwardingError(failureMsg, 0, userErr) // A payment had to be timed out on chain before it got past // the first hop. In this case, we'll report a permanent @@ -945,11 +933,9 @@ func (s *Switch) parseFailedPayment(deobfuscator ErrorDecrypter, "on-chain, then canceled back (hash=%v, pid=%d)", paymentHash, paymentID) - return &ForwardingError{ - FailureSourceIdx: 0, - ExtraMsg: userErr, - FailureMessage: &lnwire.FailPermanentChannelFailure{}, - } + return NewForwardingError( + &lnwire.FailPermanentChannelFailure{}, 0, userErr, + ) // A regular multi-hop payment error that we'll need to // decrypt. diff --git a/routing/router_test.go b/routing/router_test.go index 6fd9a630..f62473a3 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -288,11 +288,12 @@ func TestSendPaymentRouteFailureFallback(t *testing.T) { roasbeefSongoku := lnwire.NewShortChanIDFromInt(12345) if firstHop == roasbeefSongoku { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - // TODO(roasbeef): temp node failure should be? - FailureMessage: &lnwire.FailTemporaryChannelFailure{}, - } + return [32]byte{}, htlcswitch.NewForwardingError( + // TODO(roasbeef): temp node failure + // should be? + &lnwire.FailTemporaryChannelFailure{}, + 1, "", + ) } return preImage, nil @@ -420,12 +421,12 @@ func TestChannelUpdateValidation(t *testing.T) { // The unsigned channel update is attached to the failure message. ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcher).setPaymentResult( func(firstHop lnwire.ShortChannelID) ([32]byte, error) { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailFeeInsufficient{ + return [32]byte{}, htlcswitch.NewForwardingError( + &lnwire.FailFeeInsufficient{ Update: errChanUpdate, }, - } + 1, "", + ) }) // The payment parameter is mostly redundant in SendToRoute. Can be left @@ -542,16 +543,15 @@ func TestSendPaymentErrorRepeatedFeeInsufficient(t *testing.T) { roasbeefSongoku := lnwire.NewShortChanIDFromInt(chanID) if firstHop == roasbeefSongoku { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - - // Within our error, we'll add a channel update - // which is meant to reflect he new fee - // schedule for the node/channel. - FailureMessage: &lnwire.FailFeeInsufficient{ + return [32]byte{}, htlcswitch.NewForwardingError( + // Within our error, we'll add a + // channel update which is meant to + // reflect the new fee schedule for the + // node/channel. + &lnwire.FailFeeInsufficient{ Update: errChanUpdate, - }, - } + }, 1, "", + ) } return preImage, nil @@ -646,12 +646,11 @@ func TestSendPaymentErrorNonFinalTimeLockErrors(t *testing.T) { func(firstHop lnwire.ShortChannelID) ([32]byte, error) { if firstHop == roasbeefSongoku { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailExpiryTooSoon{ + return [32]byte{}, htlcswitch.NewForwardingError( + &lnwire.FailExpiryTooSoon{ Update: errChanUpdate, - }, - } + }, 1, "", + ) } return preImage, nil @@ -700,12 +699,11 @@ func TestSendPaymentErrorNonFinalTimeLockErrors(t *testing.T) { func(firstHop lnwire.ShortChannelID) ([32]byte, error) { if firstHop == roasbeefSongoku { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailIncorrectCltvExpiry{ + return [32]byte{}, htlcswitch.NewForwardingError( + &lnwire.FailIncorrectCltvExpiry{ Update: errChanUpdate, - }, - } + }, 1, "", + ) } return preImage, nil @@ -763,20 +761,19 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { // We'll first simulate an error from the first // hop to simulate the channel from songoku to // sophon not having enough capacity. - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailTemporaryChannelFailure{}, - } + return [32]byte{}, htlcswitch.NewForwardingError( + &lnwire.FailTemporaryChannelFailure{}, + 1, "", + ) } // Next, we'll create an error from phan nuwen to // indicate that the sophon node is not longer online, // which should prune out the rest of the routes. if firstHop == roasbeefPhanNuwen { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailUnknownNextPeer{}, - } + return [32]byte{}, htlcswitch.NewForwardingError( + &lnwire.FailUnknownNextPeer{}, 1, "", + ) } return preImage, nil @@ -805,10 +802,10 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { func(firstHop lnwire.ShortChannelID) ([32]byte, error) { if firstHop == roasbeefSongoku { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailUnknownNextPeer{}, - } + failure := htlcswitch.NewForwardingError( + &lnwire.FailUnknownNextPeer{}, 1, "", + ) + return [32]byte{}, failure } return preImage, nil @@ -851,10 +848,10 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { // We'll first simulate an error from the first // outgoing link to simulate the channel from luo ji to // roasbeef not having enough capacity. - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailTemporaryChannelFailure{}, - } + return [32]byte{}, htlcswitch.NewForwardingError( + &lnwire.FailTemporaryChannelFailure{}, + 1, "", + ) } return preImage, nil }) @@ -2539,9 +2536,7 @@ func TestUnknownErrorSource(t *testing.T) { // couldn't be decoded (FailureMessage is nil). if firstHop.ToUint64() == 2 { return [32]byte{}, - &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - } + htlcswitch.NewUnknownForwardingError(1) } // Otherwise the payment succeeds. @@ -3105,10 +3100,10 @@ func TestRouterPaymentStateMachine(t *testing.T) { // called, and we respond with a forwarding error case sendToSwitchResultFailure: select { - case sendResult <- &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailTemporaryChannelFailure{}, - }: + case sendResult <- htlcswitch.NewForwardingError( + &lnwire.FailTemporaryChannelFailure{}, + 1, "", + ): case <-time.After(1 * time.Second): t.Fatalf("unable to send result") } @@ -3129,12 +3124,14 @@ func TestRouterPaymentStateMachine(t *testing.T) { // to be called, and we respond with a forwarding // error, indicating that the router should retry. case getPaymentResultFailure: + failure := htlcswitch.NewForwardingError( + &lnwire.FailTemporaryChannelFailure{}, + 1, "", + ) + select { case getPaymentResult <- &htlcswitch.PaymentResult{ - Error: &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailTemporaryChannelFailure{}, - }, + Error: failure, }: case <-time.After(1 * time.Second): t.Fatalf("unable to get result") @@ -3302,12 +3299,11 @@ func TestSendToRouteStructuredError(t *testing.T) { // The unsigned channel update is attached to the failure message. ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcher).setPaymentResult( func(firstHop lnwire.ShortChannelID) ([32]byte, error) { - return [32]byte{}, &htlcswitch.ForwardingError{ - FailureSourceIdx: 1, - FailureMessage: &lnwire.FailFeeInsufficient{ + return [32]byte{}, htlcswitch.NewForwardingError( + &lnwire.FailFeeInsufficient{ Update: lnwire.ChannelUpdate{}, - }, - } + }, 1, "", + ) }) // The payment parameter is mostly redundant in SendToRoute. Can be left From 102f9b003fba4a53de93baa2fb32da6650a78afd Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 14 Jan 2020 15:07:41 +0200 Subject: [PATCH 2/6] htlcswitch: add ClearTextError interface This commit adds a ClearTextError interface which is implemented by non-opaque errors that we know the underlying wire failure message for. This interface is implemented by ForwardingErrors, because we can fully decrypt the onion blob to obtain the underlying failure reason. This interface will also be implemented by errors which originate at our node in following commits, because we know the failure reason when we fail the htlc. The lnwire interface is un-embedded in the ForwardingError struct in favour of implementing this interface. This change is made to protect against accidental passing of a ForwardingError to the wire, where the embedded FailureMessage interface will present as wire failure but will not serialize properly. --- htlcswitch/failure.go | 39 +++++++++++++++++++++++++++----- htlcswitch/link_test.go | 30 ++++++++++++------------ htlcswitch/switch_test.go | 6 ++--- lnrpc/routerrpc/router_server.go | 2 +- routing/router.go | 2 +- routing/router_test.go | 2 +- 6 files changed, 54 insertions(+), 27 deletions(-) diff --git a/htlcswitch/failure.go b/htlcswitch/failure.go index 4352d267..1f8e8f6e 100644 --- a/htlcswitch/failure.go +++ b/htlcswitch/failure.go @@ -9,6 +9,22 @@ import ( "github.com/lightningnetwork/lnd/lnwire" ) +// ClearTextError is an interface which is implemented by errors that occur +// when we know the underlying wire failure message. These errors are the +// opposite to opaque errors which are onion-encrypted blobs only understandable +// to the initiating node. ClearTextErrors are used when we fail a htlc at our +// node, or one of our initiated payments failed and we can decrypt the onion +// encrypted error fully. +type ClearTextError interface { + error + + // WireMessage extracts a valid wire failure message from an internal + // error which may contain additional metadata (which should not be + // exposed to the network). This value may be nil in the case where + // an unknown wire error is returned by one of our peers. + WireMessage() lnwire.FailureMessage +} + // ForwardingError wraps an lnwire.FailureMessage in a struct that also // includes the source of the error. type ForwardingError struct { @@ -22,7 +38,20 @@ type ForwardingError struct { // order to provide context specific error details. ExtraMsg string - lnwire.FailureMessage + // msg is the wire message associated with the error. This value may + // be nil in the case where we fail to decode failure message sent by + // a peer. + msg lnwire.FailureMessage +} + +// WireMessage extracts a valid wire failure message from an internal +// error which may contain additional metadata (which should not be +// exposed to the network). This value may be nil in the case where +// an unknown wire error is returned by one of our peers. +// +// Note this is part of the ClearTextError interface. +func (f *ForwardingError) WireMessage() lnwire.FailureMessage { + return f.msg } // Error implements the built-in error interface. We use this method to allow @@ -30,13 +59,11 @@ type ForwardingError struct { // returned. func (f *ForwardingError) Error() string { if f.ExtraMsg == "" { - return fmt.Sprintf( - "%v@%v", f.FailureMessage, f.FailureSourceIdx, - ) + return fmt.Sprintf("%v@%v", f.msg, f.FailureSourceIdx) } return fmt.Sprintf( - "%v@%v: %v", f.FailureMessage, f.FailureSourceIdx, f.ExtraMsg, + "%v@%v: %v", f.msg, f.FailureSourceIdx, f.ExtraMsg, ) } @@ -47,7 +74,7 @@ func NewForwardingError(failure lnwire.FailureMessage, index int, return &ForwardingError{ FailureSourceIdx: index, - FailureMessage: failure, + msg: failure, ExtraMsg: extraMsg, } } diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 9efe89dc..0bee5271 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -579,7 +579,7 @@ func TestExitNodeTimelockPayloadMismatch(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got: %T", err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailFinalIncorrectCltvExpiry: default: t.Fatalf("incorrect error, expected incorrect cltv expiry, "+ @@ -679,7 +679,7 @@ func TestLinkForwardTimelockPolicyMismatch(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got: %T", err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailIncorrectCltvExpiry: default: t.Fatalf("incorrect error, expected incorrect cltv expiry, "+ @@ -737,7 +737,7 @@ func TestLinkForwardFeePolicyMismatch(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got: %T", err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailFeeInsufficient: default: t.Fatalf("incorrect error, expected fee insufficient, "+ @@ -795,7 +795,7 @@ func TestLinkForwardMinHTLCPolicyMismatch(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got: %T", err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailAmountBelowMinimum: default: t.Fatalf("incorrect error, expected amount below minimum, "+ @@ -862,7 +862,7 @@ func TestLinkForwardMaxHTLCPolicyMismatch(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got: %T", err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailTemporaryChannelFailure: default: t.Fatalf("incorrect error, expected temporary channel failure, "+ @@ -968,7 +968,7 @@ func TestUpdateForwardingPolicy(t *testing.T) { if !ok { t.Fatalf("expected a ForwardingError, instead got (%T): %v", err, err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailFeeInsufficient: default: t.Fatalf("expected FailFeeInsufficient instead got: %v", err) @@ -1008,7 +1008,7 @@ func TestUpdateForwardingPolicy(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got (%T): %v", err, err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailTemporaryChannelFailure: default: t.Fatalf("expected TemporaryChannelFailure, instead got: %v", @@ -1253,9 +1253,9 @@ func TestChannelLinkMultiHopUnknownNextHop(t *testing.T) { if !ok { t.Fatalf("expected ForwardingError") } - if _, ok = fErr.FailureMessage.(*lnwire.FailUnknownNextPeer); !ok { + if _, ok = fErr.WireMessage().(*lnwire.FailUnknownNextPeer); !ok { t.Fatalf("wrong error has been received: %T", - fErr.FailureMessage) + fErr.WireMessage()) } // Wait for Alice to receive the revocation. @@ -1369,7 +1369,7 @@ func TestChannelLinkMultiHopDecodeError(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got: %T", err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailInvalidOnionVersion: default: t.Fatalf("wrong error have been received: %v", err) @@ -1462,7 +1462,7 @@ func TestChannelLinkExpiryTooSoonExitNode(t *testing.T) { err, err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailIncorrectDetails: default: t.Fatalf("expected incorrect_or_unknown_payment_details, "+ @@ -1524,7 +1524,7 @@ func TestChannelLinkExpiryTooSoonMidNode(t *testing.T) { t.Fatalf("expected a ForwardingError, instead got: %T: %v", err, err) } - switch ferr.FailureMessage.(type) { + switch ferr.WireMessage().(type) { case *lnwire.FailExpiryTooSoon: default: t.Fatalf("incorrect error, expected final time lock too "+ @@ -5636,7 +5636,7 @@ func TestChannelLinkCanceledInvoice(t *testing.T) { if !ok { t.Fatalf("expected ForwardingError, but got %v", err) } - _, ok = fErr.FailureMessage.(*lnwire.FailIncorrectDetails) + _, ok = fErr.WireMessage().(*lnwire.FailIncorrectDetails) if !ok { t.Fatalf("expected unknown payment hash, but got %v", err) } @@ -6221,8 +6221,8 @@ func assertFailureCode(t *testing.T, err error, code lnwire.FailCode) { t.Fatalf("expected ForwardingError but got %T", err) } - if fErr.FailureMessage.Code() != code { + if fErr.WireMessage().Code() != code { t.Fatalf("expected %v but got %v", - code, fErr.FailureMessage.Code()) + code, fErr.WireMessage().Code()) } } diff --git a/htlcswitch/switch_test.go b/htlcswitch/switch_test.go index 28493c83..7f78bfe4 100644 --- a/htlcswitch/switch_test.go +++ b/htlcswitch/switch_test.go @@ -2179,10 +2179,10 @@ func TestUpdateFailMalformedHTLCErrorConversion(t *testing.T) { } fwdingErr := err.(*ForwardingError) - failureMsg := fwdingErr.FailureMessage + failureMsg := fwdingErr.WireMessage() if _, ok := failureMsg.(*lnwire.FailInvalidOnionKey); !ok { t.Fatalf("expected onion failure instead got: %v", - fwdingErr.FailureMessage) + fwdingErr.WireMessage()) } } @@ -2448,7 +2448,7 @@ func TestInvalidFailure(t *testing.T) { if fErr.FailureSourceIdx != 2 { t.Fatal("unexpected error source index") } - if fErr.FailureMessage != nil { + if fErr.WireMessage() != nil { t.Fatal("expected empty failure message") } diff --git a/lnrpc/routerrpc/router_server.go b/lnrpc/routerrpc/router_server.go index ba23a079..f761883c 100644 --- a/lnrpc/routerrpc/router_server.go +++ b/lnrpc/routerrpc/router_server.go @@ -331,7 +331,7 @@ func marshallError(sendError error) (*Failure, error) { return nil, sendError } - switch onionErr := fErr.FailureMessage.(type) { + switch onionErr := fErr.WireMessage().(type) { case *lnwire.FailIncorrectDetails: response.Code = Failure_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS diff --git a/routing/router.go b/routing/router.go index d09383e9..0b2ab284 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1917,7 +1917,7 @@ func (r *ChannelRouter) processSendError(paymentID uint64, rt *route.Route, return &internalErrorReason } - failureMessage := fErr.FailureMessage + failureMessage := fErr.WireMessage() failureSourceIdx := fErr.FailureSourceIdx // Apply channel update if the error contains one. For unknown diff --git a/routing/router_test.go b/routing/router_test.go index f62473a3..53eaee8f 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -3320,7 +3320,7 @@ func TestSendToRouteStructuredError(t *testing.T) { t.Fatalf("expected forwarding error") } - if _, ok := fErr.FailureMessage.(*lnwire.FailFeeInsufficient); !ok { + if _, ok := fErr.WireMessage().(*lnwire.FailFeeInsufficient); !ok { t.Fatalf("expected fee insufficient error") } From 6a83b06ab7f0575cf12384a32e56fb8543e10ec1 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 14 Jan 2020 15:07:41 +0200 Subject: [PATCH 3/6] htlcswitch: add LinkError implementation of ClearTextError This change introduces a LinkError implementation of the ClearTextError interface. This error is intended to represent failures which occur on our incoming and outgoing link when sending, receiving and forwarding htlcs. Paired with ForwardingError, which is represents failures that did not occur at our node, this error covers all non-opaque errors that the switch experiences. --- htlcswitch/failure.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/htlcswitch/failure.go b/htlcswitch/failure.go index 1f8e8f6e..20ba6d01 100644 --- a/htlcswitch/failure.go +++ b/htlcswitch/failure.go @@ -25,6 +25,40 @@ type ClearTextError interface { WireMessage() lnwire.FailureMessage } +// LinkError is an implementation of the ClearTextError interface which +// represents failures that occur on our incoming or outgoing link. +type LinkError struct { + // msg returns the wire failure associated with the error. + // This value should *not* be nil, because we should always + // know the failure type for failures which occur at our own + // node. + msg lnwire.FailureMessage +} + +// NewLinkError returns a LinkError with the failure message provided. +// The failure message provided should *not* be nil, because we should +// always know the failure type for failures which occur at our own node. +func NewLinkError(msg lnwire.FailureMessage) *LinkError { + return &LinkError{msg: msg} +} + +// WireMessage extracts a valid wire failure message from an internal +// error which may contain additional metadata (which should not be +// exposed to the network). This value should never be nil for LinkErrors, +// because we are the ones failing the htlc. +// +// Note this is part of the ClearTextError interface. +func (l *LinkError) WireMessage() lnwire.FailureMessage { + return l.msg +} + +// Error returns the string representation of a link error. +// +// Note this is part of the ClearTextError interface. +func (l *LinkError) Error() string { + return l.msg.Error() +} + // ForwardingError wraps an lnwire.FailureMessage in a struct that also // includes the source of the error. type ForwardingError struct { From b5a2d75465f3b027fb556c9ec5dcd3bb3c214127 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 14 Jan 2020 15:07:42 +0200 Subject: [PATCH 4/6] htlcswitch+routing: type check on ClearTextError Update the type check used for checking local payment failures to check on the ClearTextError interface rather than on the ForwardingError type. This change prepares for splitting payment errors up into Link and Forwarding errors. --- htlcswitch/link_test.go | 90 +++++++++++++++++--------------- htlcswitch/switch_test.go | 18 ++++--- lnrpc/routerrpc/router_server.go | 15 ++++-- routing/mock_test.go | 4 +- routing/router.go | 29 +++++++--- 5 files changed, 94 insertions(+), 62 deletions(-) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 0bee5271..16bd92e7 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -574,12 +574,12 @@ func TestExitNodeTimelockPayloadMismatch(t *testing.T) { t.Fatalf("payment should have failed but didn't") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T", err) + t.Fatalf("expected a ClearTextError, instead got: %T", err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailFinalIncorrectCltvExpiry: default: t.Fatalf("incorrect error, expected incorrect cltv expiry, "+ @@ -674,12 +674,12 @@ func TestLinkForwardTimelockPolicyMismatch(t *testing.T) { t.Fatalf("payment should have failed but didn't") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T", err) + t.Fatalf("expected a ClearTextError, instead got: %T", err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailIncorrectCltvExpiry: default: t.Fatalf("incorrect error, expected incorrect cltv expiry, "+ @@ -732,12 +732,12 @@ func TestLinkForwardFeePolicyMismatch(t *testing.T) { t.Fatalf("payment should have failed but didn't") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T", err) + t.Fatalf("expected a ClearTextError, instead got: %T", err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailFeeInsufficient: default: t.Fatalf("incorrect error, expected fee insufficient, "+ @@ -790,12 +790,12 @@ func TestLinkForwardMinHTLCPolicyMismatch(t *testing.T) { t.Fatalf("payment should have failed but didn't") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T", err) + t.Fatalf("expected a ClearTextError, instead got: %T", err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailAmountBelowMinimum: default: t.Fatalf("incorrect error, expected amount below minimum, "+ @@ -857,12 +857,12 @@ func TestLinkForwardMaxHTLCPolicyMismatch(t *testing.T) { t.Fatalf("payment should have failed but didn't") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T", err) + t.Fatalf("expected a ClearTextError, instead got: %T", err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailTemporaryChannelFailure: default: t.Fatalf("incorrect error, expected temporary channel failure, "+ @@ -964,11 +964,12 @@ func TestUpdateForwardingPolicy(t *testing.T) { t.Fatalf("payment should've been rejected") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got (%T): %v", err, err) + t.Fatalf("expected a ClearTextError, instead got (%T): %v", err, err) } - switch ferr.WireMessage().(type) { + + switch rtErr.WireMessage().(type) { case *lnwire.FailFeeInsufficient: default: t.Fatalf("expected FailFeeInsufficient instead got: %v", err) @@ -1003,12 +1004,13 @@ func TestUpdateForwardingPolicy(t *testing.T) { t.Fatalf("payment should've been rejected") } - ferr, ok = err.(*ForwardingError) + rtErr, ok = err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got (%T): %v", + t.Fatalf("expected a ClearTextError, instead got (%T): %v", err, err) } - switch ferr.WireMessage().(type) { + + switch rtErr.WireMessage().(type) { case *lnwire.FailTemporaryChannelFailure: default: t.Fatalf("expected TemporaryChannelFailure, instead got: %v", @@ -1249,13 +1251,14 @@ func TestChannelLinkMultiHopUnknownNextHop(t *testing.T) { if err == nil { t.Fatal("error haven't been received") } - fErr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected ForwardingError") + t.Fatalf("expected ClearTextError") } - if _, ok = fErr.WireMessage().(*lnwire.FailUnknownNextPeer); !ok { + + if _, ok = rtErr.WireMessage().(*lnwire.FailUnknownNextPeer); !ok { t.Fatalf("wrong error has been received: %T", - fErr.WireMessage()) + rtErr.WireMessage()) } // Wait for Alice to receive the revocation. @@ -1364,12 +1367,12 @@ func TestChannelLinkMultiHopDecodeError(t *testing.T) { t.Fatal("error haven't been received") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T", err) + t.Fatalf("expected a ClearTextError, instead got: %T", err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailInvalidOnionVersion: default: t.Fatalf("wrong error have been received: %v", err) @@ -1456,13 +1459,13 @@ func TestChannelLinkExpiryTooSoonExitNode(t *testing.T) { "time lock value") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T %v", - err, err) + t.Fatalf("expected a ClearTextError, instead got: %T %v", + rtErr, err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailIncorrectDetails: default: t.Fatalf("expected incorrect_or_unknown_payment_details, "+ @@ -1519,12 +1522,13 @@ func TestChannelLinkExpiryTooSoonMidNode(t *testing.T) { "time lock value") } - ferr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected a ForwardingError, instead got: %T: %v", err, err) + t.Fatalf("expected a ClearTextError, instead got: %T: %v", + rtErr, err) } - switch ferr.WireMessage().(type) { + switch rtErr.WireMessage().(type) { case *lnwire.FailExpiryTooSoon: default: t.Fatalf("incorrect error, expected final time lock too "+ @@ -5632,11 +5636,11 @@ func TestChannelLinkCanceledInvoice(t *testing.T) { // Because the invoice is canceled, we expect an unknown payment hash // result. - fErr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected ForwardingError, but got %v", err) + t.Fatalf("expected ClearTextError, but got %v", err) } - _, ok = fErr.WireMessage().(*lnwire.FailIncorrectDetails) + _, ok = rtErr.WireMessage().(*lnwire.FailIncorrectDetails) if !ok { t.Fatalf("expected unknown payment hash, but got %v", err) } @@ -6213,16 +6217,16 @@ func TestChannelLinkReceiveEmptySig(t *testing.T) { aliceLink.Stop() } -// assertFailureCode asserts that an error is of type ForwardingError and that +// assertFailureCode asserts that an error is of type ClearTextError and that // the failure code is as expected. func assertFailureCode(t *testing.T, err error, code lnwire.FailCode) { - fErr, ok := err.(*ForwardingError) + rtErr, ok := err.(ClearTextError) if !ok { - t.Fatalf("expected ForwardingError but got %T", err) + t.Fatalf("expected ClearTextError but got %T", err) } - if fErr.WireMessage().Code() != code { + if rtErr.WireMessage().Code() != code { t.Fatalf("expected %v but got %v", - code, fErr.WireMessage().Code()) + code, rtErr.WireMessage().Code()) } } diff --git a/htlcswitch/switch_test.go b/htlcswitch/switch_test.go index 7f78bfe4..0f6ab9e0 100644 --- a/htlcswitch/switch_test.go +++ b/htlcswitch/switch_test.go @@ -2178,11 +2178,11 @@ func TestUpdateFailMalformedHTLCErrorConversion(t *testing.T) { t.Fatalf("unable to send payment: %v", err) } - fwdingErr := err.(*ForwardingError) - failureMsg := fwdingErr.WireMessage() + routingErr := err.(ClearTextError) + failureMsg := routingErr.WireMessage() if _, ok := failureMsg.(*lnwire.FailInvalidOnionKey); !ok { t.Fatalf("expected onion failure instead got: %v", - fwdingErr.WireMessage()) + routingErr.WireMessage()) } } @@ -2441,14 +2441,18 @@ func TestInvalidFailure(t *testing.T) { select { case result := <-resultChan: - fErr, ok := result.Error.(*ForwardingError) + rtErr, ok := result.Error.(ClearTextError) if !ok { - t.Fatal("expected ForwardingError") + t.Fatal("expected ClearTextError") } - if fErr.FailureSourceIdx != 2 { + source, ok := rtErr.(*ForwardingError) + if !ok { + t.Fatalf("expected forwarding error, got: %T", rtErr) + } + if source.FailureSourceIdx != 2 { t.Fatal("unexpected error source index") } - if fErr.WireMessage() != nil { + if rtErr.WireMessage() != nil { t.Fatal("expected empty failure message") } diff --git a/lnrpc/routerrpc/router_server.go b/lnrpc/routerrpc/router_server.go index f761883c..93cbeff0 100644 --- a/lnrpc/routerrpc/router_server.go +++ b/lnrpc/routerrpc/router_server.go @@ -326,12 +326,12 @@ func marshallError(sendError error) (*Failure, error) { return response, nil } - fErr, ok := sendError.(*htlcswitch.ForwardingError) + rtErr, ok := sendError.(htlcswitch.ClearTextError) if !ok { return nil, sendError } - switch onionErr := fErr.WireMessage().(type) { + switch onionErr := rtErr.WireMessage().(type) { case *lnwire.FailIncorrectDetails: response.Code = Failure_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS @@ -425,7 +425,16 @@ func marshallError(sendError error) (*Failure, error) { return nil, fmt.Errorf("cannot marshall failure %T", onionErr) } - response.FailureSourceIndex = uint32(fErr.FailureSourceIdx) + // If the ClearTextError received is a ForwardingError, the error + // originated from a node along the route, not locally on our outgoing + // link. We set failureSourceIdx to the index of the node where the + // failure occurred. If the error is not a ForwardingError, the failure + // occurred at our node, so we leave the index as 0 to indicate that + // we failed locally. + fErr, ok := rtErr.(*htlcswitch.ForwardingError) + if ok { + response.FailureSourceIndex = uint32(fErr.FailureSourceIdx) + } return response, nil } diff --git a/routing/mock_test.go b/routing/mock_test.go index d2f7448c..fac67aa9 100644 --- a/routing/mock_test.go +++ b/routing/mock_test.go @@ -35,12 +35,12 @@ func (m *mockPaymentAttemptDispatcher) SendHTLC(firstHop lnwire.ShortChannelID, var result *htlcswitch.PaymentResult preimage, err := m.onPayment(firstHop) if err != nil { - fwdErr, ok := err.(*htlcswitch.ForwardingError) + rtErr, ok := err.(htlcswitch.ClearTextError) if !ok { return err } result = &htlcswitch.PaymentResult{ - Error: fwdErr, + Error: rtErr, } } else { result = &htlcswitch.PaymentResult{Preimage: preimage} diff --git a/routing/router.go b/routing/router.go index 0b2ab284..ae090144 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1910,18 +1910,33 @@ func (r *ChannelRouter) processSendError(paymentID uint64, rt *route.Route, return reportFail(nil, nil) } - // If an internal, non-forwarding error occurred, we can stop - // trying. - fErr, ok := sendErr.(*htlcswitch.ForwardingError) + + // If the error is a ClearTextError, we have received a valid wire + // failure message, either from our own outgoing link or from a node + // down the route. If the error is not related to the propagation of + // our payment, we can stop trying because an internal error has + // occurred. + rtErr, ok := sendErr.(htlcswitch.ClearTextError) if !ok { return &internalErrorReason } - failureMessage := fErr.WireMessage() - failureSourceIdx := fErr.FailureSourceIdx + // failureSourceIdx is the index of the node that the failure occurred + // at. If the ClearTextError received is not a ForwardingError the + // payment error occurred at our node, so we leave this value as 0 + // to indicate that the failure occurred locally. If the error is a + // ForwardingError, it did not originate at our node, so we set + // failureSourceIdx to the index of the node where the failure occurred. + failureSourceIdx := 0 + source, ok := rtErr.(*htlcswitch.ForwardingError) + if ok { + failureSourceIdx = source.FailureSourceIdx + } - // Apply channel update if the error contains one. For unknown - // failures, failureMessage is nil. + // Extract the wire failure and apply channel update if it contains one. + // If we received an unknown failure message from a node along the + // route, the failure message will be nil. + failureMessage := rtErr.WireMessage() if failureMessage != nil { err := r.tryApplyChannelUpdate( rt, failureSourceIdx, failureMessage, From f430fd50c5b85adaef27c601a8adf75f32ee4417 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 14 Jan 2020 15:07:42 +0200 Subject: [PATCH 5/6] htlcswitch: use LinkError for internal errors Update the ChannelLink interface to specifically return the LinkError struct. This error implements the ClearTextError interface, so will be picked up as a routing realted error by the router. With LinkErrors implemented, the switch now returns a LinkError for all failures on our incoming/outgoing link and ForwardingError when the failure occurs down the line. --- htlcswitch/failure.go | 22 +++++++++++- htlcswitch/failure_detail.go | 58 ++++++++++++++++++++++++++++++ htlcswitch/interfaces.go | 17 ++++----- htlcswitch/link.go | 49 ++++++++++++++----------- htlcswitch/link_test.go | 12 +++---- htlcswitch/mock.go | 8 ++--- htlcswitch/switch.go | 69 +++++++++++++++++++++--------------- htlcswitch/switch_test.go | 29 +++++++++------ 8 files changed, 185 insertions(+), 79 deletions(-) create mode 100644 htlcswitch/failure_detail.go diff --git a/htlcswitch/failure.go b/htlcswitch/failure.go index 20ba6d01..f3aa6ad2 100644 --- a/htlcswitch/failure.go +++ b/htlcswitch/failure.go @@ -33,6 +33,9 @@ type LinkError struct { // know the failure type for failures which occur at our own // node. msg lnwire.FailureMessage + + // FailureDetail enriches the wire error with additional information. + FailureDetail } // NewLinkError returns a LinkError with the failure message provided. @@ -42,6 +45,17 @@ func NewLinkError(msg lnwire.FailureMessage) *LinkError { return &LinkError{msg: msg} } +// NewDetailedLinkError returns a link error that enriches a wire message with +// a failure detail. +func NewDetailedLinkError(msg lnwire.FailureMessage, + detail FailureDetail) *LinkError { + + return &LinkError{ + msg: msg, + FailureDetail: detail, + } +} + // WireMessage extracts a valid wire failure message from an internal // error which may contain additional metadata (which should not be // exposed to the network). This value should never be nil for LinkErrors, @@ -56,7 +70,13 @@ func (l *LinkError) WireMessage() lnwire.FailureMessage { // // Note this is part of the ClearTextError interface. func (l *LinkError) Error() string { - return l.msg.Error() + // If the link error has no failure detail, return the wire message's + // error. + if l.FailureDetail == FailureDetailNone { + return l.msg.Error() + } + + return fmt.Sprintf("%v: %v", l.msg.Error(), l.FailureDetail) } // ForwardingError wraps an lnwire.FailureMessage in a struct that also diff --git a/htlcswitch/failure_detail.go b/htlcswitch/failure_detail.go new file mode 100644 index 00000000..92b25510 --- /dev/null +++ b/htlcswitch/failure_detail.go @@ -0,0 +1,58 @@ +package htlcswitch + +// FailureDetail is an enum which is used to enrich failures with +// additional information. +type FailureDetail int + +const ( + // FailureDetailNone is returned when the wire message contains + // sufficient information. + FailureDetailNone = iota + + // FailureDetailOnionDecode indicates that we could not decode an + // onion error. + FailureDetailOnionDecode + + // FailureDetailLinkNotEligible indicates that a routing attempt was + // made over a link that is not eligible for routing. + FailureDetailLinkNotEligible + + // FailureDetailOnChainTimeout indicates that a payment had to be timed + // out on chain before it got past the first hop by us or the remote + // party. + FailureDetailOnChainTimeout + + // FailureDetailHTLCExceedsMax is returned when a htlc exceeds our + // policy's maximum htlc amount. + FailureDetailHTLCExceedsMax + + // FailureDetailInsufficientBalance is returned when we cannot route a + // htlc due to insufficient outgoing capacity. + FailureDetailInsufficientBalance +) + +// String returns the string representation of a failure detail. +func (fd FailureDetail) String() string { + switch fd { + case FailureDetailNone: + return "no failure detail" + + case FailureDetailOnionDecode: + return "could not decode onion" + + case FailureDetailLinkNotEligible: + return "link not eligible" + + case FailureDetailOnChainTimeout: + return "payment was resolved on-chain, then canceled back" + + case FailureDetailHTLCExceedsMax: + return "htlc exceeds maximum policy amount" + + case FailureDetailInsufficientBalance: + return "insufficient bandwidth to route htlc" + + default: + return "unknown failure detail" + } +} diff --git a/htlcswitch/interfaces.go b/htlcswitch/interfaces.go index bb34b283..1281e951 100644 --- a/htlcswitch/interfaces.go +++ b/htlcswitch/interfaces.go @@ -102,20 +102,21 @@ type ChannelLink interface { // CheckHtlcForward should return a nil error if the passed HTLC details // satisfy the current forwarding policy fo the target link. Otherwise, - // a valid protocol failure message should be returned in order to - // signal to the source of the HTLC, the policy consistency issue. + // a LinkError with a valid protocol failure message should be returned + // in order to signal to the source of the HTLC, the policy consistency + // issue. CheckHtlcForward(payHash [32]byte, incomingAmt lnwire.MilliSatoshi, amtToForward lnwire.MilliSatoshi, incomingTimeout, outgoingTimeout uint32, - heightNow uint32) lnwire.FailureMessage + heightNow uint32) *LinkError // CheckHtlcTransit should return a nil error if the passed HTLC details - // satisfy the current channel policy. Otherwise, a valid protocol - // failure message should be returned in order to signal the violation. - // This call is intended to be used for locally initiated payments for - // which there is no corresponding incoming htlc. + // satisfy the current channel policy. Otherwise, a LinkError with a + // valid protocol failure message should be returned in order to signal + // the violation. This call is intended to be used for locally initiated + // payments for which there is no corresponding incoming htlc. CheckHtlcTransit(payHash [32]byte, amt lnwire.MilliSatoshi, - timeout uint32, heightNow uint32) lnwire.FailureMessage + timeout uint32, heightNow uint32) *LinkError // Bandwidth returns the amount of milli-satoshis which current link // might pass through channel link. The value returned from this method diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 0bba3f65..92ecba65 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -2135,16 +2135,17 @@ func (l *channelLink) UpdateForwardingPolicy(newPolicy ForwardingPolicy) { l.cfg.FwrdingPolicy = newPolicy } -// CheckHtlcForward should return a nil error if the passed HTLC details satisfy -// the current forwarding policy fo the target link. Otherwise, a valid -// protocol failure message should be returned in order to signal to the source -// of the HTLC, the policy consistency issue. +// CheckHtlcForward should return a nil error if the passed HTLC details +// satisfy the current forwarding policy fo the target link. Otherwise, +// a LinkError with a valid protocol failure message should be returned +// in order to signal to the source of the HTLC, the policy consistency +// issue. // // NOTE: Part of the ChannelLink interface. func (l *channelLink) CheckHtlcForward(payHash [32]byte, incomingHtlcAmt, amtToForward lnwire.MilliSatoshi, incomingTimeout, outgoingTimeout uint32, - heightNow uint32) lnwire.FailureMessage { + heightNow uint32) *LinkError { l.RLock() policy := l.cfg.FwrdingPolicy @@ -2176,14 +2177,14 @@ func (l *channelLink) CheckHtlcForward(payHash [32]byte, // As part of the returned error, we'll send our latest routing // policy so the sending node obtains the most up to date data. - - return l.createFailureWithUpdate( + failure := l.createFailureWithUpdate( func(upd *lnwire.ChannelUpdate) lnwire.FailureMessage { return lnwire.NewFeeInsufficient( amtToForward, *upd, ) }, ) + return NewLinkError(failure) } // Finally, we'll ensure that the time-lock on the outgoing HTLC meets @@ -2198,26 +2199,27 @@ func (l *channelLink) CheckHtlcForward(payHash [32]byte, // Grab the latest routing policy so the sending node is up to // date with our current policy. - return l.createFailureWithUpdate( + failure := l.createFailureWithUpdate( func(upd *lnwire.ChannelUpdate) lnwire.FailureMessage { return lnwire.NewIncorrectCltvExpiry( incomingTimeout, *upd, ) }, ) + return NewLinkError(failure) } return nil } -// CheckHtlcTransit should return a nil error if the passed HTLC details satisfy the -// current channel policy. Otherwise, a valid protocol failure message should -// be returned in order to signal the violation. This call is intended to be -// used for locally initiated payments for which there is no corresponding -// incoming htlc. +// CheckHtlcTransit should return a nil error if the passed HTLC details +// satisfy the current channel policy. Otherwise, a LinkError with a +// valid protocol failure message should be returned in order to signal +// the violation. This call is intended to be used for locally initiated +// payments for which there is no corresponding incoming htlc. func (l *channelLink) CheckHtlcTransit(payHash [32]byte, amt lnwire.MilliSatoshi, timeout uint32, - heightNow uint32) lnwire.FailureMessage { + heightNow uint32) *LinkError { l.RLock() policy := l.cfg.FwrdingPolicy @@ -2232,7 +2234,7 @@ func (l *channelLink) CheckHtlcTransit(payHash [32]byte, // the channel's amount and time lock constraints. func (l *channelLink) canSendHtlc(policy ForwardingPolicy, payHash [32]byte, amt lnwire.MilliSatoshi, timeout uint32, - heightNow uint32) lnwire.FailureMessage { + heightNow uint32) *LinkError { // As our first sanity check, we'll ensure that the passed HTLC isn't // too small for the next hop. If so, then we'll cancel the HTLC @@ -2244,13 +2246,14 @@ func (l *channelLink) canSendHtlc(policy ForwardingPolicy, // As part of the returned error, we'll send our latest routing // policy so the sending node obtains the most up to date data. - return l.createFailureWithUpdate( + failure := l.createFailureWithUpdate( func(upd *lnwire.ChannelUpdate) lnwire.FailureMessage { return lnwire.NewAmountBelowMinimum( amt, *upd, ) }, ) + return NewLinkError(failure) } // Next, ensure that the passed HTLC isn't too large. If so, we'll @@ -2261,11 +2264,12 @@ func (l *channelLink) canSendHtlc(policy ForwardingPolicy, // As part of the returned error, we'll send our latest routing // policy so the sending node obtains the most up-to-date data. - return l.createFailureWithUpdate( + failure := l.createFailureWithUpdate( func(upd *lnwire.ChannelUpdate) lnwire.FailureMessage { return lnwire.NewTemporaryChannelFailure(upd) }, ) + return NewDetailedLinkError(failure, FailureDetailHTLCExceedsMax) } // We want to avoid offering an HTLC which will expire in the near @@ -2275,12 +2279,12 @@ func (l *channelLink) canSendHtlc(policy ForwardingPolicy, l.log.Errorf("htlc(%x) has an expiry that's too soon: "+ "outgoing_expiry=%v, best_height=%v", payHash[:], timeout, heightNow) - - return l.createFailureWithUpdate( + failure := l.createFailureWithUpdate( func(upd *lnwire.ChannelUpdate) lnwire.FailureMessage { return lnwire.NewExpiryTooSoon(*upd) }, ) + return NewLinkError(failure) } // Check absolute max delta. @@ -2289,16 +2293,19 @@ func (l *channelLink) canSendHtlc(policy ForwardingPolicy, "the future: got %v, but maximum is %v", payHash[:], timeout-heightNow, l.cfg.MaxOutgoingCltvExpiry) - return &lnwire.FailExpiryTooFar{} + return NewLinkError(&lnwire.FailExpiryTooFar{}) } // Check to see if there is enough balance in this channel. if amt > l.Bandwidth() { - return l.createFailureWithUpdate( + failure := l.createFailureWithUpdate( func(upd *lnwire.ChannelUpdate) lnwire.FailureMessage { return lnwire.NewTemporaryChannelFailure(upd) }, ) + return NewDetailedLinkError( + failure, FailureDetailInsufficientBalance, + ) } return nil diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 16bd92e7..16d64d70 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -5541,7 +5541,7 @@ func TestCheckHtlcForward(t *testing.T) { t.Run("below minhtlc", func(t *testing.T) { result := link.CheckHtlcForward(hash, 100, 50, 200, 150, 0) - if _, ok := result.(*lnwire.FailAmountBelowMinimum); !ok { + if _, ok := result.WireMessage().(*lnwire.FailAmountBelowMinimum); !ok { t.Fatalf("expected FailAmountBelowMinimum failure code") } }) @@ -5549,7 +5549,7 @@ func TestCheckHtlcForward(t *testing.T) { t.Run("above maxhtlc", func(t *testing.T) { result := link.CheckHtlcForward(hash, 1500, 1200, 200, 150, 0) - if _, ok := result.(*lnwire.FailTemporaryChannelFailure); !ok { + if _, ok := result.WireMessage().(*lnwire.FailTemporaryChannelFailure); !ok { t.Fatalf("expected FailTemporaryChannelFailure failure code") } }) @@ -5557,7 +5557,7 @@ func TestCheckHtlcForward(t *testing.T) { t.Run("insufficient fee", func(t *testing.T) { result := link.CheckHtlcForward(hash, 1005, 1000, 200, 150, 0) - if _, ok := result.(*lnwire.FailFeeInsufficient); !ok { + if _, ok := result.WireMessage().(*lnwire.FailFeeInsufficient); !ok { t.Fatalf("expected FailFeeInsufficient failure code") } }) @@ -5565,7 +5565,7 @@ func TestCheckHtlcForward(t *testing.T) { t.Run("expiry too soon", func(t *testing.T) { result := link.CheckHtlcForward(hash, 1500, 1000, 200, 150, 190) - if _, ok := result.(*lnwire.FailExpiryTooSoon); !ok { + if _, ok := result.WireMessage().(*lnwire.FailExpiryTooSoon); !ok { t.Fatalf("expected FailExpiryTooSoon failure code") } }) @@ -5573,7 +5573,7 @@ func TestCheckHtlcForward(t *testing.T) { t.Run("incorrect cltv expiry", func(t *testing.T) { result := link.CheckHtlcForward(hash, 1500, 1000, 200, 190, 0) - if _, ok := result.(*lnwire.FailIncorrectCltvExpiry); !ok { + if _, ok := result.WireMessage().(*lnwire.FailIncorrectCltvExpiry); !ok { t.Fatalf("expected FailIncorrectCltvExpiry failure code") } @@ -5583,7 +5583,7 @@ func TestCheckHtlcForward(t *testing.T) { // Check that expiry isn't too far in the future. result := link.CheckHtlcForward(hash, 1500, 1000, 10200, 10100, 0) - if _, ok := result.(*lnwire.FailExpiryTooFar); !ok { + if _, ok := result.WireMessage().(*lnwire.FailExpiryTooFar); !ok { t.Fatalf("expected FailExpiryTooFar failure code") } }) diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index 828c36b9..e39c6073 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -645,9 +645,9 @@ type mockChannelLink struct { htlcID uint64 - checkHtlcTransitResult lnwire.FailureMessage + checkHtlcTransitResult *LinkError - checkHtlcForwardResult lnwire.FailureMessage + checkHtlcForwardResult *LinkError } // completeCircuit is a helper method for adding the finalized payment circuit @@ -707,14 +707,14 @@ func (f *mockChannelLink) HandleChannelUpdate(lnwire.Message) { func (f *mockChannelLink) UpdateForwardingPolicy(_ ForwardingPolicy) { } func (f *mockChannelLink) CheckHtlcForward([32]byte, lnwire.MilliSatoshi, - lnwire.MilliSatoshi, uint32, uint32, uint32) lnwire.FailureMessage { + lnwire.MilliSatoshi, uint32, uint32, uint32) *LinkError { return f.checkHtlcForwardResult } func (f *mockChannelLink) CheckHtlcTransit(payHash [32]byte, amt lnwire.MilliSatoshi, timeout uint32, - heightNow uint32) lnwire.FailureMessage { + heightNow uint32) *LinkError { return f.checkHtlcTransitResult } diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index 74276ea8..c08bdc0c 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -756,20 +756,19 @@ func (s *Switch) handleLocalDispatch(pkt *htlcPacket) error { s.indexMtx.RUnlock() if err != nil { log.Errorf("Link %v not found", pkt.outgoingChanID) - return NewForwardingError( - &lnwire.FailUnknownNextPeer{}, 0, "", - ) + return NewLinkError(&lnwire.FailUnknownNextPeer{}) } if !link.EligibleToForward() { - err := fmt.Errorf("Link %v is not available to forward", + log.Errorf("Link %v is not available to forward", pkt.outgoingChanID) - log.Error(err) // The update does not need to be populated as the error // will be returned back to the router. - htlcErr := lnwire.NewTemporaryChannelFailure(nil) - return NewForwardingError(htlcErr, 0, err.Error()) + return NewDetailedLinkError( + lnwire.NewTemporaryChannelFailure(nil), + FailureDetailLinkNotEligible, + ) } // Ensure that the htlc satisfies the outgoing channel policy. @@ -782,8 +781,7 @@ func (s *Switch) handleLocalDispatch(pkt *htlcPacket) error { if htlcErr != nil { log.Errorf("Link %v policy for local forward not "+ "satisfied", pkt.outgoingChanID) - - return NewForwardingError(htlcErr, 0, "") + return htlcErr } return link.HandleSwitchPacket(pkt) @@ -907,35 +905,43 @@ func (s *Switch) parseFailedPayment(deobfuscator ErrorDecrypter, // decrypt the error, simply decode it them report back to the // user. case unencrypted: - var userErr string r := bytes.NewReader(htlc.Reason) failureMsg, err := lnwire.DecodeFailure(r, 0) if err != nil { - userErr = fmt.Sprintf("unable to decode onion "+ - "failure (hash=%v, pid=%d): %v", - paymentHash, paymentID, err) - log.Error(userErr) + // If we could not decode the failure reason, return a link + // error indicating that we failed to decode the onion. + linkError := NewDetailedLinkError( + // As this didn't even clear the link, we don't + // need to apply an update here since it goes + // directly to the router. + lnwire.NewTemporaryChannelFailure(nil), + FailureDetailOnionDecode, + ) - // As this didn't even clear the link, we don't need to - // apply an update here since it goes directly to the - // router. - failureMsg = lnwire.NewTemporaryChannelFailure(nil) + log.Errorf("%v: (hash=%v, pid=%d): %v", + linkError.FailureDetail, paymentHash, paymentID, + err) + + return linkError } - return NewForwardingError(failureMsg, 0, userErr) + // If we successfully decoded the failure reason, return it. + return NewLinkError(failureMsg) // A payment had to be timed out on chain before it got past // the first hop. In this case, we'll report a permanent // channel failure as this means us, or the remote party had to // go on chain. case isResolution && htlc.Reason == nil: - userErr := fmt.Sprintf("payment was resolved "+ - "on-chain, then canceled back (hash=%v, pid=%d)", + linkError := NewDetailedLinkError( + &lnwire.FailPermanentChannelFailure{}, + FailureDetailOnChainTimeout, + ) + + log.Info("%v: hash=%v, pid=%d", linkError.FailureDetail, paymentHash, paymentID) - return NewForwardingError( - &lnwire.FailPermanentChannelFailure{}, 0, userErr, - ) + return linkError // A regular multi-hop payment error that we'll need to // decrypt. @@ -1002,18 +1008,21 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { // selection process. This way we can return the error for // precise link that the sender selected, while optimistically // trying all links to utilize our available bandwidth. - linkErrs := make(map[lnwire.ShortChannelID]lnwire.FailureMessage) + linkErrs := make(map[lnwire.ShortChannelID]*LinkError) // Try to find destination channel link with appropriate // bandwidth. var destination ChannelLink for _, link := range interfaceLinks { - var failure lnwire.FailureMessage + var failure *LinkError // We'll skip any links that aren't yet eligible for // forwarding. if !link.EligibleToForward() { - failure = &lnwire.FailUnknownNextPeer{} + failure = NewDetailedLinkError( + &lnwire.FailUnknownNextPeer{}, + FailureDetailLinkNotEligible, + ) } else { // We'll ensure that the HTLC satisfies the // current forwarding conditions of this target @@ -1048,7 +1057,9 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { // If we can't find the error of the source, // then we'll return an unknown next peer, // though this should never happen. - linkErr = &lnwire.FailUnknownNextPeer{} + linkErr = NewLinkError( + &lnwire.FailUnknownNextPeer{}, + ) log.Warnf("unable to find err source for "+ "outgoing_link=%v, errors=%v", packet.outgoingChanID, newLogClosure(func() string { @@ -1061,7 +1072,7 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { htlc.PaymentHash[:], packet.outgoingChanID, linkErr) - return s.failAddPacket(packet, linkErr, addErr) + return s.failAddPacket(packet, linkErr.WireMessage(), addErr) } // Send the packet to the destination channel link which diff --git a/htlcswitch/switch_test.go b/htlcswitch/switch_test.go index 0f6ab9e0..69425dc2 100644 --- a/htlcswitch/switch_test.go +++ b/htlcswitch/switch_test.go @@ -1290,7 +1290,7 @@ func TestSwitchForwardCircuitPersistence(t *testing.T) { type multiHopFwdTest struct { name string eligible1, eligible2 bool - failure1, failure2 lnwire.FailureMessage + failure1, failure2 *LinkError expectedReply lnwire.FailCode } @@ -1308,9 +1308,11 @@ func TestSkipIneligibleLinksMultiHopForward(t *testing.T) { // Channel one has a policy failure and the other channel isn't // available. { - name: "policy fail", - eligible1: true, - failure1: lnwire.NewFinalIncorrectCltvExpiry(0), + name: "policy fail", + eligible1: true, + failure1: NewLinkError( + lnwire.NewFinalIncorrectCltvExpiry(0), + ), expectedReply: lnwire.CodeFinalIncorrectCltvExpiry, }, @@ -1325,11 +1327,16 @@ func TestSkipIneligibleLinksMultiHopForward(t *testing.T) { // The requested channel has insufficient bandwidth and the // other channel's policy isn't satisfied. { - name: "non-strict policy fail", - eligible1: true, - failure1: lnwire.NewTemporaryChannelFailure(nil), - eligible2: true, - failure2: lnwire.NewFinalIncorrectCltvExpiry(0), + name: "non-strict policy fail", + eligible1: true, + failure1: NewDetailedLinkError( + lnwire.NewTemporaryChannelFailure(nil), + FailureDetailInsufficientBalance, + ), + eligible2: true, + failure2: NewLinkError( + lnwire.NewFinalIncorrectCltvExpiry(0), + ), expectedReply: lnwire.CodeTemporaryChannelFailure, }, } @@ -1482,7 +1489,9 @@ func testSkipLinkLocalForward(t *testing.T, eligible bool, aliceChannelLink := newMockChannelLink( s, chanID1, aliceChanID, alicePeer, eligible, ) - aliceChannelLink.checkHtlcTransitResult = policyResult + aliceChannelLink.checkHtlcTransitResult = NewLinkError( + policyResult, + ) if err := s.AddLink(aliceChannelLink); err != nil { t.Fatalf("unable to add alice link: %v", err) } From ec099bf5ddb0c5148e5f3867ecf20bd44b9219fe Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 14 Jan 2020 15:07:42 +0200 Subject: [PATCH 6/6] htlcswitch: remove extramsg string from ForwardingError Remove the extramsg field in ForwardingError because it has been replaced with detailed link errors. --- htlcswitch/failure.go | 17 ++++------------- htlcswitch/mock.go | 2 +- routing/router_test.go | 24 ++++++++++++------------ 3 files changed, 17 insertions(+), 26 deletions(-) diff --git a/htlcswitch/failure.go b/htlcswitch/failure.go index f3aa6ad2..35d78590 100644 --- a/htlcswitch/failure.go +++ b/htlcswitch/failure.go @@ -88,10 +88,6 @@ type ForwardingError struct { // zero is the self node. FailureSourceIdx int - // ExtraMsg is an additional error message that callers can provide in - // order to provide context specific error details. - ExtraMsg string - // msg is the wire message associated with the error. This value may // be nil in the case where we fail to decode failure message sent by // a peer. @@ -112,24 +108,19 @@ func (f *ForwardingError) WireMessage() lnwire.FailureMessage { // the switch or any callers to insert additional context to the error message // returned. func (f *ForwardingError) Error() string { - if f.ExtraMsg == "" { - return fmt.Sprintf("%v@%v", f.msg, f.FailureSourceIdx) - } - return fmt.Sprintf( - "%v@%v: %v", f.msg, f.FailureSourceIdx, f.ExtraMsg, + "%v@%v", f.msg, f.FailureSourceIdx, ) } // NewForwardingError creates a new payment error which wraps a wire error // with additional metadata. -func NewForwardingError(failure lnwire.FailureMessage, index int, - extraMsg string) *ForwardingError { +func NewForwardingError(failure lnwire.FailureMessage, + index int) *ForwardingError { return &ForwardingError{ FailureSourceIdx: index, msg: failure, - ExtraMsg: extraMsg, } } @@ -199,7 +190,7 @@ func (s *SphinxErrorDecrypter) DecryptError(reason lnwire.OpaqueReason) ( return NewUnknownForwardingError(failure.SenderIdx), nil } - return NewForwardingError(failureMsg, failure.SenderIdx, ""), nil + return NewForwardingError(failureMsg, failure.SenderIdx), nil } // A compile time check to ensure ErrorDecrypter implements the Deobfuscator diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index e39c6073..02737a71 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -400,7 +400,7 @@ func (o *mockDeobfuscator) DecryptError(reason lnwire.OpaqueReason) (*Forwarding return nil, err } - return NewForwardingError(failure, 1, ""), nil + return NewForwardingError(failure, 1), nil } var _ ErrorDecrypter = (*mockDeobfuscator)(nil) diff --git a/routing/router_test.go b/routing/router_test.go index 53eaee8f..169557cc 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -292,7 +292,7 @@ func TestSendPaymentRouteFailureFallback(t *testing.T) { // TODO(roasbeef): temp node failure // should be? &lnwire.FailTemporaryChannelFailure{}, - 1, "", + 1, ) } @@ -425,7 +425,7 @@ func TestChannelUpdateValidation(t *testing.T) { &lnwire.FailFeeInsufficient{ Update: errChanUpdate, }, - 1, "", + 1, ) }) @@ -550,7 +550,7 @@ func TestSendPaymentErrorRepeatedFeeInsufficient(t *testing.T) { // node/channel. &lnwire.FailFeeInsufficient{ Update: errChanUpdate, - }, 1, "", + }, 1, ) } @@ -649,7 +649,7 @@ func TestSendPaymentErrorNonFinalTimeLockErrors(t *testing.T) { return [32]byte{}, htlcswitch.NewForwardingError( &lnwire.FailExpiryTooSoon{ Update: errChanUpdate, - }, 1, "", + }, 1, ) } @@ -702,7 +702,7 @@ func TestSendPaymentErrorNonFinalTimeLockErrors(t *testing.T) { return [32]byte{}, htlcswitch.NewForwardingError( &lnwire.FailIncorrectCltvExpiry{ Update: errChanUpdate, - }, 1, "", + }, 1, ) } @@ -763,7 +763,7 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { // sophon not having enough capacity. return [32]byte{}, htlcswitch.NewForwardingError( &lnwire.FailTemporaryChannelFailure{}, - 1, "", + 1, ) } @@ -772,7 +772,7 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { // which should prune out the rest of the routes. if firstHop == roasbeefPhanNuwen { return [32]byte{}, htlcswitch.NewForwardingError( - &lnwire.FailUnknownNextPeer{}, 1, "", + &lnwire.FailUnknownNextPeer{}, 1, ) } @@ -803,7 +803,7 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { if firstHop == roasbeefSongoku { failure := htlcswitch.NewForwardingError( - &lnwire.FailUnknownNextPeer{}, 1, "", + &lnwire.FailUnknownNextPeer{}, 1, ) return [32]byte{}, failure } @@ -850,7 +850,7 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { // roasbeef not having enough capacity. return [32]byte{}, htlcswitch.NewForwardingError( &lnwire.FailTemporaryChannelFailure{}, - 1, "", + 1, ) } return preImage, nil @@ -3102,7 +3102,7 @@ func TestRouterPaymentStateMachine(t *testing.T) { select { case sendResult <- htlcswitch.NewForwardingError( &lnwire.FailTemporaryChannelFailure{}, - 1, "", + 1, ): case <-time.After(1 * time.Second): t.Fatalf("unable to send result") @@ -3126,7 +3126,7 @@ func TestRouterPaymentStateMachine(t *testing.T) { case getPaymentResultFailure: failure := htlcswitch.NewForwardingError( &lnwire.FailTemporaryChannelFailure{}, - 1, "", + 1, ) select { @@ -3302,7 +3302,7 @@ func TestSendToRouteStructuredError(t *testing.T) { return [32]byte{}, htlcswitch.NewForwardingError( &lnwire.FailFeeInsufficient{ Update: lnwire.ChannelUpdate{}, - }, 1, "", + }, 1, ) })